Mask secret query parameter when it is the last parameter#4055
Mask secret query parameter when it is the last parameter#4055vasiliy-mikhailov wants to merge 2 commits into
Conversation
handleDataWithSecret masks the secret value before logging, but the regex &secret=\w+& requires a trailing &. When secret is the last query parameter (e.g. appid=wx123&secret=abc123) there is no trailing &, so it is not masked and leaks in logs. Drop the trailing-& requirement so the value is masked regardless of position; the middle-parameter case is unchanged. Adds a regression test.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77c9cc6a6d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| E dataForLog = data; | ||
| if(data instanceof String && StringUtils.contains((String)data, "&secret=")){ | ||
| dataForLog = (E) RegExUtils.replaceAll((String)data,"&secret=\\w+&","&secret=******&"); | ||
| dataForLog = (E) RegExUtils.replaceAll((String)data,"&secret=\\w+","&secret=******"); |
There was a problem hiding this comment.
当待记录的请求参数中 secret 值包含 URL 编码或 -、.、+ 等非 \w 字符时，这个正则只会替换到第一个非 word 字符为止；例如 appid=wx&secret=abc%2Fdef 会被记录成 appid=wx&secret=******%2Fdef，仍然泄露后半段密钥。这里是日志脱敏逻辑，建议按参数边界匹配值（如直到下一个 & 或字符串结尾），而不是按 \w+ 匹配。
Useful? React with 👍 / 👎.
|
@codex auggie review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🤖 Augment PR SummarySummary: Fixes Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| E dataForLog = data; | ||
| if(data instanceof String && StringUtils.contains((String)data, "&secret=")){ | ||
| dataForLog = (E) RegExUtils.replaceAll((String)data,"&secret=\\w+&","&secret=******&"); | ||
| dataForLog = (E) RegExUtils.replaceAll((String)data,"&secret=\\w+","&secret=******"); |
There was a problem hiding this comment.
handleDataWithSecret currently only masks when the input contains "&secret=", so cases like "secret=abc123&..." (first param) or "?secret=abc123&..." won’t be desensitized and could still leak secrets in logs. Consider ensuring the match also works when secret is the first/only query parameter.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
handleDataWithSecret only masked secret when it followed an & and stopped at the first non-word character, so a leading secret= (first or only parameter) was not masked and a value containing URL-encoded or non-word characters (e.g. abc%2Fdef) leaked its remainder. Match the secret parameter at a query boundary and mask the whole value up to the next & or end of string.
|
Thanks for the reviews. Tightened the masking to cover both gaps that were raised:
Added tests for both the first-parameter and the encoded-value cases. |
2 participants
DataUtils.handleDataWithSecretmasks thesecretvalue in a query string before it is logged, but the regex&secret=\w+&requires a trailing&. Whensecretis the last parameter — e.g.appid=wx123&secret=abc123— there is no trailing&, so the value is not masked and the secret is exposed in log output.This drops the trailing-
&requirement (&secret=\w+→&secret=******), so the value is masked regardless of its position. The middle-parameter case is unchanged:...&secret=xxx&grant_type=...still becomes...&secret=******&grant_type=....Adds a regression test
testHandleDataWithSecretAtEndthat fails before the change and passes after.AI assistance disclosure
This contribution was produced with the help of an AI pipeline. The pipeline processed a large amount of source code to surface suspected bugs, reproduced a subset of them with failing unit tests and generated candidate fixes, and prepared pull requests from the ones that held up. Each PR was then reviewed and verified by a human before being opened: the fix and test were checked by hand and the test was confirmed to fail before the change and pass after.