Skip to content

[SONARJAVA-6484] Implement new rule S8899#5684

Open
romainbrenguier wants to merge 5 commits into
masterfrom
new-rule/S8899
Open

[SONARJAVA-6484] Implement new rule S8899#5684
romainbrenguier wants to merge 5 commits into
masterfrom
new-rule/S8899

Conversation

@romainbrenguier

@romainbrenguier romainbrenguier commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

This rule detects a critical security vulnerability where HTTP request paths are used in security checks without normalizing multiple consecutive slashes. Attackers can bypass authentication and authorization by inserting extra slashes in URLs (e.g., //admin instead of /admin). The rule flags path-based security checks that lack proper normalization via replaceAll("/+", "/") and applies to both javax.servlet and jakarta.servlet frameworks, as well as JAX-RS UriInfo.

Part of


Summary by Gitar

  • New Sonar Rule:
    • Implemented S8899 to detect unnormalized HTTP request paths in security checks.
    • Added full rule specification, including metadata, HTML documentation, and test samples covering various frameworks.
  • Project Configuration:
    • Added .cursor/rules/sonar_code_context.mdc and .sonar-code-context/settings.json to manage development quality standards.
    • Updated .gitignore to track critical Sonar configuration while excluding transient files.
    • Added java-S8899.json tracking files to multiple test projects in its/ruling/.
  • Refactoring:
    • Extracted logic into updatePathStateFromMethodInvocation helper within RequestPathNormalizationCheck.java.
  • Environment Updates:
    • Updated mise.toml to lock Java to version 21.
    • Integrated rule into Sonar_agentic_AI_profile.json and Sonar_way_profile.json and adjusted AutoScanTest expectations.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6484

Comment thread .cursor/rules/sonar_code_context.mdc Outdated
Comment thread .sonar-code-context/settings.json Outdated
Comment thread .gitignore Outdated
Comment thread .agents/RIS_PREPROCESSING.md Outdated
Comment thread .agents/RIS_PREPROCESSING.md Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a separate PR

@romainbrenguier romainbrenguier force-pushed the new-rule/S8899 branch 4 times, most recently from 0cec425 to 63869c9 Compare June 24, 2026 12:00
This rule detects a critical security vulnerability where HTTP request paths
are used in security checks without normalizing multiple consecutive slashes.
Attackers can bypass authentication and authorization by inserting extra slashes
in URLs (e.g., //admin instead of /admin). The rule flags path-based security
checks that lack proper normalization via replaceAll("/+", "/") and applies to
both javax.servlet and jakarta.servlet frameworks, as well as JAX-RS UriInfo.
@romainbrenguier romainbrenguier marked this pull request as ready for review June 24, 2026 15:25
@romainbrenguier romainbrenguier requested review from rombirli and removed request for NoemieBenard June 25, 2026 11:30
@gitar-bot

gitar-bot Bot commented Jun 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 11 resolved / 11 findings

Implements rule S8899 to detect unnormalized HTTP request paths in security checks, addressing identified issues with regex logic, false-positive sensitivity, and redundant code paths.

✅ 11 resolved
Quality: Unrelated local tooling/config files committed in this PR

📄 .cursor/rules/sonar_code_context.mdc:1-15 📄 .sonar-code-context/settings.json:1-15 📄 .mvn/.gradle-enterprise/gradle-enterprise-workspace-id:1 📄 .gitignore:42-46
This feature PR also adds files that appear unrelated to rule S8899 and look like local developer/tooling artifacts: .cursor/rules/sonar_code_context.mdc, .sonar-code-context/settings.json, and .mvn/.gradle-enterprise/gradle-enterprise-workspace-id (a machine-specific workspace identifier). The .gitignore change is also crafted specifically to force-track settings.json while ignoring the rest of .sonar-code-context/. Committing per-developer tooling configuration and workspace IDs into a shared repository is usually undesirable and pollutes the history. Recommend removing these from the PR (or moving them to a separate, intentional change) so the PR contains only the new rule and its resources.

Edge Case: Over-broad security-context detection causes many false positives

📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:69-73 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:147-161
isInSecurityContext treats any enclosing if, ternary, return, throw, &&, or || as a security check, and SECURITY_CHECK_METHODS includes very common String operations (contains, indexOf, equals, endsWith, startsWith, matches). Combined, this flags virtually every conditional that inspects a request path, even for legitimate non-security purposes such as routing, feature toggles, content negotiation, redirect handling, or logging guards (e.g. if (request.getRequestURI().endsWith(".css")) ...). For a Critical VULNERABILITY rule this high false-positive rate will erode trust and bury real issues.

Consider narrowing the heuristic: require correlation with an actual authorization/authentication action in the guarded block (e.g. a throw of a Forbidden/Unauthorized exception, a checkPermission-style call, or returning a 401/403), and/or restrict the matched String methods to comparison-style operations most associated with access decisions. At minimum, document the precision trade-off and add negative test cases (e.g. an endsWith(".css") static-resource check) to the sample file.

Edge Case: Normalization detection too narrow; misses common forms

📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:63-67 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:185-195
Normalization is only recognized when String.replaceAll is called with a string literal containing the substring /+ or //+. This produces false positives for equally valid normalizations the rule will not recognize, e.g. replaceAll("/{2,}", "/"), a pre-compiled Pattern.matcher(path).replaceAll("/"), URI.create(path).normalize(), or framework canonicalization helpers. A user who normalizes correctly via any of these will still be flagged. Conversely, matching merely on the literal containing /+ (e.g. replaceAll("foo/+bar", "x")) can be mistaken for path normalization. Consider broadening the recognized normalization patterns (or at least documenting the supported form in S8899.html) and adding sample cases for the unsupported variants.

Quality: Dead state: normalizedVariables set is written but never read

📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:21 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:76 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:86 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:122 📄 java-checks/src/main/java/org/sonar/java/checks/security/RequestPathNormalizationCheck.java:140
The normalizedVariables Set<Symbol> (and its HashSet/Set imports) is populated in handleVariableDeclaration/handleAssignment and cleared in setContext, but it is never queried anywhere. All normalization tracking is actually done through pathVariables with PathState.NORMALIZED. The set is redundant dead state and should be removed to avoid confusion.

Quality: Internal AI analysis doc committed to repo

📄 .agents/RIS_PREPROCESSING.md:1-15
This commit adds .agents/RIS_PREPROCESSING.md, a 479-line internal analysis of the rule engine ('SonarQube Java Analyzer - Existing Rule Engine Analysis'). This is preprocessing/scratch documentation, not part of the S8899 rule feature, and likely should not be committed to the repository. It mirrors the previously resolved finding about unrelated local tooling/config files (.cursor/rules/sonar_code_context.mdc) being committed in this PR. Consider removing it from the PR (or adding .agents/ to .gitignore if these files are meant to stay local).

...and 6 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant