Skip to content

SONARJAVA-6494 Implement new rule S8914 [TEST]#5709

Closed
romainbrenguier wants to merge 4 commits into
masterfrom
new-rule/SONARJAVA-6494-S8914
Closed

SONARJAVA-6494 Implement new rule S8914 [TEST]#5709
romainbrenguier wants to merge 4 commits into
masterfrom
new-rule/SONARJAVA-6494-S8914

Conversation

@romainbrenguier

@romainbrenguier romainbrenguier commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Do not review or merge. This is a vulnerability. This was just for testing the nigel new rule workflow.


Summary by Gitar

  • New analysis rule:
    • Implemented S8914 to detect security annotations (@Authenticated, @RolesAllowed) placed on Quarkus WebSocket callback methods.
    • Advises moving these annotations to the class level to secure the HTTP upgrade handshake.
  • Test coverage:
    • Added QuarkusWebSocketSecurityAnnotationCheckTest and corresponding sample code to verify rule compliance.
    • Updated pom.xml to include quarkus-websockets-next and quarkus-security dependencies for test sources.
  • Quality profile updates:
    • Registered S8914 in both the Sonar way and Sonar agentic AI profiles.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

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

Copy link
Copy Markdown
Contributor

SONARJAVA-6494

Comment on lines +1 to +15
<p>=== Documentation

* Quarkus WebSockets Next - Security - https://quarkus.io/guides/websockets-next-reference#websocket-next-security[Official Quarkus documentation on securing WebSocket endpoints, explaining HTTP upgrade security]

* Quarkus WebSockets Next - Secure HTTP Upgrade - https://quarkus.io/guides/websockets-next-reference#secure-http-upgrade[Detailed guidance on securing the HTTP upgrade handshake with security annotations]


=== What is the potential impact?

==== Unauthorized Access

Unauthorized clients can establish WebSocket connections to endpoints that lack proper authentication during the HTTP upgrade handshake. While individual message-handling callback methods may implement their own authorization checks, the connection remains open, potentially allowing:

* Access to sensitive real-time data streams
* Information disclosure through connection metadata

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Quality: Rule description S8914.html is AsciiDoc, not HTML

S8914.html is the user-facing rule description rendered in SonarQube/IDE. Unlike every other rule file in this directory (e.g. S8700.html, S8714.html) which use proper HTML tags (<h2>, <p>, <ul>, <li>, <pre>), this file dumps raw AsciiDoc markup wrapped in a single <p> tag:

  • Headings use ===/====/== instead of <h2>/<h3>.
  • Bullet lists use * instead of <ul><li>.
  • Inline emphasis uses *...*/**...** instead of <strong>/<em>.
  • Links use AsciiDoc url[text] syntax instead of <a href>.

None of this will render correctly; users will see literal === and * characters. The section ordering is also jumbled: "Documentation", "What is the potential impact?", and "Standards" appear before "Why is this an issue?", whereas the standard structure is "Why is this an issue?" → "What is the potential impact?" → "How to fix it?" → "Resources". Convert the content to valid HTML matching the convention used by neighboring rule files, and reorder the sections. The file also lacks a trailing newline.

Was this helpful? React with 👍 / 👎

Comment on lines +43 to +46
private static final List<String> SECURITY_ANNOTATIONS = List.of(
AUTHENTICATED_ANNOTATION,
ROLES_ALLOWED_ANNOTATION
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: Check ignores @DenyAll/@permitAll security annotations

SECURITY_ANNOTATIONS only contains io.quarkus.security.Authenticated and jakarta.annotation.security.RolesAllowed. Quarkus/Jakarta also recognize jakarta.annotation.security.PermitAll and jakarta.annotation.security.DenyAll as access-control annotations on WebSocket callbacks. If the intent is to flag any security annotation placed on a callback method instead of the endpoint class, these are missed (false negatives). If the omission is intentional (these two convey access decisions differently), consider documenting the rationale. Verify the intended annotation set against the RSPEC and add the missing annotations to SECURITY_ANNOTATIONS if appropriate.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown
CI failed: The 'Autoscan Tests' job failed because the new rule S8914 caused an unexpected change in scan output, leading to a mismatch with existing regression test expectations.

Overview

1 test failure occurred in the 'Autoscan Tests' job due to a discrepancy between the expected and actual autoscan output. The build successfully compiled, but the regression testing infrastructure detected a diff resulting from the implementation of rule S8914.

Failures

Autoscan Regression Mismatch (confidence: high)

  • Type: test
  • Affected jobs: 84021149251
  • Related to change: yes
  • Root cause: The introduction of rule S8914 changed the scan output, causing it to deviate from the hardcoded 'expected' files in the its/autoscan suite.
  • Suggested fix: Review the generated diff_autoscan.html artifact from the job. If the changes are a correct and intended consequence of the new rule, update the regression test files in src/test/resources/autoscan/diffs to reflect the new expected behavior.

Summary

  • Change-related failures: 1 (Autoscan regression failure due to new rule S8914).
  • Infrastructure/flaky failures: 0
  • Recommended action: Manually verify the output diff in the CI artifacts. If correct, commit the updated expected output files to resolve the regression test mismatch.
Code Review ⚠️ Changes requested 0 resolved / 2 findings

Implements rule S8914 to flag insecure security annotations on Quarkus WebSocket callbacks, but requires correction of the AsciiDoc rule description and expansion of the check to include @DenyAll and @PermitAll.

⚠️ Quality: Rule description S8914.html is AsciiDoc, not HTML

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8914.html:1-15

S8914.html is the user-facing rule description rendered in SonarQube/IDE. Unlike every other rule file in this directory (e.g. S8700.html, S8714.html) which use proper HTML tags (<h2>, <p>, <ul>, <li>, <pre>), this file dumps raw AsciiDoc markup wrapped in a single <p> tag:

  • Headings use ===/====/== instead of <h2>/<h3>.
  • Bullet lists use * instead of <ul><li>.
  • Inline emphasis uses *...*/**...** instead of <strong>/<em>.
  • Links use AsciiDoc url[text] syntax instead of <a href>.

None of this will render correctly; users will see literal === and * characters. The section ordering is also jumbled: "Documentation", "What is the potential impact?", and "Standards" appear before "Why is this an issue?", whereas the standard structure is "Why is this an issue?" → "What is the potential impact?" → "How to fix it?" → "Resources". Convert the content to valid HTML matching the convention used by neighboring rule files, and reorder the sections. The file also lacks a trailing newline.

💡 Edge Case: Check ignores @DenyAll/@PermitAll security annotations

📄 java-checks/src/main/java/org/sonar/java/checks/QuarkusWebSocketSecurityAnnotationCheck.java:43-46

SECURITY_ANNOTATIONS only contains io.quarkus.security.Authenticated and jakarta.annotation.security.RolesAllowed. Quarkus/Jakarta also recognize jakarta.annotation.security.PermitAll and jakarta.annotation.security.DenyAll as access-control annotations on WebSocket callbacks. If the intent is to flag any security annotation placed on a callback method instead of the endpoint class, these are missed (false negatives). If the omission is intentional (these two convey access decisions differently), consider documenting the rationale. Verify the intended annotation set against the RSPEC and add the missing annotations to SECURITY_ANNOTATIONS if appropriate.

🤖 Prompt for agents
Code Review: Implements rule S8914 to flag insecure security annotations on Quarkus WebSocket callbacks, but requires correction of the AsciiDoc rule description and expansion of the check to include @DenyAll and @PermitAll.

1. ⚠️ Quality: Rule description S8914.html is AsciiDoc, not HTML
   Files: sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8914.html:1-15

   `S8914.html` is the user-facing rule description rendered in SonarQube/IDE. Unlike every other rule file in this directory (e.g. `S8700.html`, `S8714.html`) which use proper HTML tags (`<h2>`, `<p>`, `<ul>`, `<li>`, `<pre>`), this file dumps raw AsciiDoc markup wrapped in a single `<p>` tag:
   - Headings use `===`/`====`/`==` instead of `<h2>`/`<h3>`.
   - Bullet lists use `*` instead of `<ul><li>`.
   - Inline emphasis uses `*...*`/`**...**` instead of `<strong>`/`<em>`.
   - Links use AsciiDoc `url[text]` syntax instead of `<a href>`.
   
   None of this will render correctly; users will see literal `===` and `*` characters. The section ordering is also jumbled: "Documentation", "What is the potential impact?", and "Standards" appear before "Why is this an issue?", whereas the standard structure is "Why is this an issue?" → "What is the potential impact?" → "How to fix it?" → "Resources". Convert the content to valid HTML matching the convention used by neighboring rule files, and reorder the sections. The file also lacks a trailing newline.

2. 💡 Edge Case: Check ignores @DenyAll/@PermitAll security annotations
   Files: java-checks/src/main/java/org/sonar/java/checks/QuarkusWebSocketSecurityAnnotationCheck.java:43-46

   `SECURITY_ANNOTATIONS` only contains `io.quarkus.security.Authenticated` and `jakarta.annotation.security.RolesAllowed`. Quarkus/Jakarta also recognize `jakarta.annotation.security.PermitAll` and `jakarta.annotation.security.DenyAll` as access-control annotations on WebSocket callbacks. If the intent is to flag any security annotation placed on a callback method instead of the endpoint class, these are missed (false negatives). If the omission is intentional (these two convey access decisions differently), consider documenting the rationale. Verify the intended annotation set against the RSPEC and add the missing annotations to `SECURITY_ANNOTATIONS` if appropriate.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

Comment with these commands to change:

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

Was this helpful? React with 👍 / 👎 | Gitar

@romainbrenguier romainbrenguier changed the title SONARJAVA-6494 Implement new rule S8914 TEST SONARJAVA-6494 Implement new rule S8914 Jun 29, 2026
@romainbrenguier romainbrenguier changed the title TEST SONARJAVA-6494 Implement new rule S8914 SONARJAVA-6494 Implement new rule S8914 [TEST] Jun 29, 2026
@sonarqube-next

Copy link
Copy Markdown

@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6494-S8914 branch from 2cd3b50 to 0c8d3b7 Compare June 30, 2026 13:08
Detects when security annotations (@authenticated, @RolesAllowed) are
placed on individual WebSocket callback methods instead of on the
WebSocket endpoint class itself. According to Quarkus documentation,
only class-level security annotations secure the HTTP upgrade handshake.
- Replaced three external Quarkus/Jakarta Maven dependencies (quarkus-websockets-next, quarkus-security, jakarta.annotation-api) with local stub annotation files in java-checks-test-sources/default/src/main/java/
- Added S8914 entry to autoscan-diff-by-rules.json with hasTruePositives=false, falseNegatives=8, falsePositives=0
- Fixed 1 code smell: replaced lambda with method reference `this::checkMethod` in QuarkusWebSocketSecurityAnnotationCheck.java (S1612)
- Updated `hasSize(470)` to `hasSize(469)` in JavaAgenticWayProfileTest after merge from master removed S6548 from the Agentic AI profile (-1 rule net change)
- Added diff_S8914.json with hasTruePositives=true, FN=0, FP=0 and updated autoscan-diff-by-rules.json for S8914
- No changes needed: the S1612 issue (line 79 in QuarkusWebSocketSecurityAnnotationCheck.java) is already fixed - the code already uses the method reference 'this::checkMethod' in the forEach call.
@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6494-S8914 branch from 0c8d3b7 to 2579c83 Compare June 30, 2026 13:25
@romainbrenguier

Copy link
Copy Markdown
Contributor Author

I will withdraw the implementation for now because this is a security rule.

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