Mask secret query parameter when it is the last parameter#4055
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: Improves log desensitization for Changes:
Why: Prevents secrets from being leaked in logs when the parameter is last/first or when the value contains non- 🤖 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. |
|
@codex auggie review again |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
There was a problem hiding this comment.
Pull request overview
该 PR 修复 DataUtils.handleDataWithSecret 在日志输出前对查询串参数 secret 的脱敏缺陷：旧实现依赖 &secret=...&（必须有尾随 &），导致当 secret 位于末尾或开头时可能无法脱敏而泄露敏感信息。
Changes:
- 调整
handleDataWithSecret的正则匹配方式，使secret在参数列表任意位置（含首/尾）都能被替换为******，并覆盖非\\w的取值场景。 - 新增回归测试，覆盖
secret位于末尾、位于开头、以及包含 URL 编码/特殊字符的场景。
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| weixin-java-common/src/main/java/me/chanjar/weixin/common/util/DataUtils.java | 更新脱敏正则逻辑，避免 secret 在首尾参数时漏脱敏 |
| weixin-java-common/src/test/java/me/chanjar/weixin/common/util/DataUtilsTest.java | 增加回归测试覆盖更多 secret 位置与取值边界 |
| if (data instanceof String && StringUtils.contains((String) data, "secret=")) { | ||
| dataForLog = (E) RegExUtils.replaceAll((String) data, "(^|[?&])secret=[^&]*", "$1secret=******"); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.