feat(rule): maven proxy#306
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/rule/rules/maven.yml (1)
6-9: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRule assumes strict username-before-password ordering with limited separator length.
The pattern requires
<username>immediately followed by<password>within\s{1,200}whitespace. Real settings.xml files with comments, blank lines, or reversed element order (password before username) between the two tags won't match, producing false negatives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rule/rules/maven.yml` around lines 6 - 9, The Maven rules pattern is too strict because it only matches <username> immediately before <password> with a short whitespace-only gap, so valid settings.xml variants with comments, blank lines, or reversed order are missed. Update the regex in the maven rule to allow either tag order and to tolerate broader separators between credentials, while keeping the same token capture behavior in the rule pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rule/rules/maven.yml`:
- Around line 5-9: Remove the unsupported negative lookahead from the Maven rule
pattern in maven.yml so ValidateRule can compile it with Go regexp. Keep the
password capture in the pattern and, if you still need to exclude placeholder
values, move that logic to pattern_requirements.ignore_if_contains. Use the
existing pattern block as the target and ensure the rule still loads
successfully.
---
Nitpick comments:
In `@pkg/rule/rules/maven.yml`:
- Around line 6-9: The Maven rules pattern is too strict because it only matches
<username> immediately before <password> with a short whitespace-only gap, so
valid settings.xml variants with comments, blank lines, or reversed order are
missed. Update the regex in the maven rule to allow either tag order and to
tolerate broader separators between credentials, while keeping the same token
capture behavior in the rule pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e56bfceb-060e-4ae3-b354-9e0cd43fb009
📒 Files selected for processing (1)
pkg/rule/rules/maven.yml
![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: b9d9ce92f0
ℹ️ 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".
| @@ -0,0 +1,31 @@ | |||
| rules: | |||
| - name: Maven Settings XML Credentials | |||
| id: np.maven.1 | |||
There was a problem hiding this comment.
As added, np.maven.1 is loaded as a builtin rule but is not referenced by any builtin ruleset. Normal titus scan runs default to --ruleset default (cmd/titus/scan.go) and loadRules applies ApplyRuleset, so this detector is filtered out unless the user explicitly chooses --ruleset all or custom rules; the Maven proxy passwords this change targets will still be missed in the default scan workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/rule/rules/maven.yml (1)
11-15: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider broadening placeholder list.
Only three masking patterns are excluded. Common placeholders like
CHANGEME,REDACTED,PLACEHOLDER,INSERT_PASSWORD_HEREwould reduce false positives from templated/example settings.xml files.♻️ Suggested additions
pattern_requirements: ignore_if_contains: - "****" - "xxxx" - "YOUR_PASSWORD" + - "CHANGEME" + - "REDACTED" + - "PLACEHOLDER" + - "INSERT_PASSWORD_HERE"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rule/rules/maven.yml` around lines 11 - 15, The masking exclusions in the Maven rule are too narrow, so templated example values still trigger false positives. Update the pattern_requirements.ignore_if_contains list in the maven rule to include additional common placeholders such as CHANGEME, REDACTED, PLACEHOLDER, and INSERT_PASSWORD_HERE, alongside the existing entries, so the rule better recognizes example settings content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/rule/rules/maven.yml`:
- Around line 11-15: The masking exclusions in the Maven rule are too narrow, so
templated example values still trigger false positives. Update the
pattern_requirements.ignore_if_contains list in the maven rule to include
additional common placeholders such as CHANGEME, REDACTED, PLACEHOLDER, and
INSERT_PASSWORD_HERE, alongside the existing entries, so the rule better
recognizes example settings content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21125b28-77c3-4063-8d8e-65f1539dd6da
📒 Files selected for processing (2)
pkg/rule/rules/maven.ymlpkg/rule/rulesets/default.yml
|
Thanks for this — proxy/server credential leakage in Maven configs is a real and useful thing to catch. Before we merge, though, we'd like to find a way to make this more targeted / less generalized, because as written it's likely to be a high false-positive source. The pattern matches a bare I built the rule and ran it against a few realistic, non-secret fixtures. It produced 4 false positives out of 5 matches (80%):
Contributing factors:
Suggested direction: anchor to the Maven structure (require the Performance-wise there's no concern — bounded quantifiers, no catastrophic-backtracking risk, and the |
|
Hello @michaelweber I've made some changes let me know if it's better. Also I took example with #282 but I don't see it in the ruleset default.yml is it normal ? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/rule/rules/maven.yml (2)
15-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIgnore list still missing some placeholder terms called out for this rule.
PR objectives asked to exclude
password,admin, andsecretas generic/default words, but they're absent here.♻️ Proposed addition
- "changeit" - "changeme" + - "password" + - "admin" + - "secret" - "example" - "test" - "dummy"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rule/rules/maven.yml` around lines 15 - 25, The ignore list for the Maven rule is still missing the generic placeholder terms requested by the review. Update the ignore_if_contains entries in the maven rule configuration to include password, admin, and secret alongside the existing defaults, keeping the change in the same rule definition so the matcher in this config skips those common placeholder values.
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo negative_examples for known false-positive fixtures.
PR objectives cite specific FP cases (Spring datasource XML,
settings.xml${env.REPO_PASSWORD}placeholder,tomcat-users.xmlchangeit, Markdown docs). Addingnegative_examplescovering these would lock in regression protection now that ignore filters/entropy are being tightened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/rule/rules/maven.yml` around lines 29 - 35, Add negative_examples for the known false-positive fixture cases referenced in the review, since the current examples section only shows a positive secret and does not protect against regressions. Update the rule definition around the examples block to include negative_examples entries for the Spring datasource XML, settings.xml with ${env.REPO_PASSWORD}, tomcat-users.xml using changeit, and Markdown docs so the tightened ignore/entropy logic is validated against these fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rule/rules/maven.yml`:
- Around line 13-14: The min_entropy setting is currently nested under
pattern_requirements in the Maven rule, but yamlPatternRequirements does not
parse it, so it never reaches Rule.MinEntropy. Move min_entropy to the top level
of the yamlRule mapping so pkg/rule/yaml.go can populate MinEntropy, and keep
pattern_requirements limited to the supported nested fields used by
passesEntropyCheck.
---
Nitpick comments:
In `@pkg/rule/rules/maven.yml`:
- Around line 15-25: The ignore list for the Maven rule is still missing the
generic placeholder terms requested by the review. Update the ignore_if_contains
entries in the maven rule configuration to include password, admin, and secret
alongside the existing defaults, keeping the change in the same rule definition
so the matcher in this config skips those common placeholder values.
- Around line 29-35: Add negative_examples for the known false-positive fixture
cases referenced in the review, since the current examples section only shows a
positive secret and does not protect against regressions. Update the rule
definition around the examples block to include negative_examples entries for
the Spring datasource XML, settings.xml with ${env.REPO_PASSWORD},
tomcat-users.xml using changeit, and Markdown docs so the tightened
ignore/entropy logic is validated against these fixtures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68179025-5a80-468f-bfe5-c47f1dbd26b4
📒 Files selected for processing (1)
pkg/rule/rules/maven.yml
2 participants
cf: #283