-
Notifications
You must be signed in to change notification settings - Fork 725
SONARJAVA-6494 Implement new rule S8914 [TEST] #5709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
romainbrenguier
wants to merge
5
commits into
master
Choose a base branch
from
new-rule/SONARJAVA-6494-S8914
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5144789
Add S8914 metadata and SonarWay profile entry
romainbrenguier 35deda8
Implement new rule S8914
romainbrenguier 385d47c
Fix CI failures
romainbrenguier 698d847
Merge remote-tracking branch 'origin/master' into new-rule/SONARJAVA-…
romainbrenguier 2cd3b50
Fix CI failures
romainbrenguier File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 6 additions & 0 deletions
6
its/autoscan/src/test/resources/autoscan/diffs/diff_S8914.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8914", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 0, | ||
| "falsePositives": 0 | ||
| } |
146 changes: 146 additions & 0 deletions
146
...t-sources/default/src/main/java/checks/QuarkusWebSocketSecurityAnnotationCheckSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| package checks; | ||
|
|
||
| import io.quarkus.security.Authenticated; | ||
| import io.quarkus.websockets.next.OnBinaryMessage; | ||
| import io.quarkus.websockets.next.OnClose; | ||
| import io.quarkus.websockets.next.OnError; | ||
| import io.quarkus.websockets.next.OnOpen; | ||
| import io.quarkus.websockets.next.OnPongMessage; | ||
| import io.quarkus.websockets.next.OnTextMessage; | ||
| import io.quarkus.websockets.next.WebSocket; | ||
| import jakarta.annotation.security.RolesAllowed; | ||
|
|
||
| class QuarkusWebSocketSecurityAnnotationCheckSample { | ||
|
|
||
| @WebSocket(path = "/secure") | ||
| class AuthenticatedOnMethod { | ||
| @Authenticated // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnTextMessage | ||
| String echo(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/admin") | ||
| class RolesAllowedOnMethod { | ||
| @RolesAllowed("admin") // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnTextMessage | ||
| String handleMessage(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/multi") | ||
| class MultipleCallbacksWithSecurity { | ||
| @Authenticated // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnOpen | ||
| void onOpen() { | ||
| } | ||
|
|
||
| @RolesAllowed("admin") // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnTextMessage | ||
| String onMessage(String msg) { | ||
| return msg; | ||
| } | ||
|
|
||
| @Authenticated // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnClose | ||
| void onClose() { | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/callbacks") | ||
| class DifferentCallbackTypes { | ||
| @Authenticated // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnBinaryMessage | ||
| void onBinary(byte[] data) { | ||
| } | ||
|
|
||
| @RolesAllowed("user") // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnError | ||
| void onError(Throwable t) { | ||
| } | ||
|
|
||
| @Authenticated // Noncompliant {{Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.}} | ||
| @OnPongMessage | ||
| void onPong() { | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/secure-class") | ||
| @Authenticated | ||
| class ClassLevelAuthenticated { | ||
| @OnTextMessage | ||
| String echo(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/admin-class") | ||
| @RolesAllowed("admin") | ||
| class ClassLevelRolesAllowed { | ||
| @OnTextMessage | ||
| String handleMessage(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/hybrid") | ||
| @Authenticated | ||
| class HybridSecurity { | ||
| @OnTextMessage | ||
| String normalMessage(String msg) { | ||
| return msg; | ||
| } | ||
|
|
||
| @RolesAllowed("admin") | ||
| @OnTextMessage | ||
| String adminMessage(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/multi-secure") | ||
| @RolesAllowed("user") | ||
| class SecureClassMultipleMethods { | ||
| @OnOpen | ||
| void onOpen() { | ||
| } | ||
|
|
||
| @OnTextMessage | ||
| String onMessage(String msg) { | ||
| return msg; | ||
| } | ||
|
|
||
| @OnClose | ||
| void onClose() { | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/public") | ||
| class PublicEndpoint { | ||
| @OnTextMessage | ||
| String echo(String msg) { | ||
| return msg; | ||
| } | ||
| } | ||
|
|
||
| class NotAWebSocket { | ||
| @Authenticated | ||
| void someMethod() { | ||
| } | ||
| } | ||
|
|
||
| @WebSocket(path = "/helper") | ||
| class SecurityOnNonCallback { | ||
| @OnTextMessage | ||
| String echo(String msg) { | ||
| return process(msg); | ||
| } | ||
|
|
||
| @Authenticated | ||
| String process(String msg) { | ||
| return msg.toUpperCase(); | ||
| } | ||
| } | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/security/Authenticated.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.security; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Authenticated { | ||
| } |
11 changes: 11 additions & 0 deletions
11
...checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnBinaryMessage.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnBinaryMessage { | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnClose.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnClose { | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnError.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnError { | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnOpen.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnOpen { | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnPongMessage.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnPongMessage { | ||
| } |
11 changes: 11 additions & 0 deletions
11
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/OnTextMessage.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.METHOD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface OnTextMessage { | ||
| } |
12 changes: 12 additions & 0 deletions
12
java-checks-test-sources/default/src/main/java/io/quarkus/websockets/next/WebSocket.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package io.quarkus.websockets.next; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface WebSocket { | ||
| String path(); | ||
| } |
12 changes: 12 additions & 0 deletions
12
java-checks-test-sources/default/src/main/java/jakarta/annotation/security/RolesAllowed.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package jakarta.annotation.security; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface RolesAllowed { | ||
| String[] value(); | ||
| } |
108 changes: 108 additions & 0 deletions
108
java-checks/src/main/java/org/sonar/java/checks/QuarkusWebSocketSecurityAnnotationCheck.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import java.util.List; | ||
| import org.sonar.check.Rule; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.tree.AnnotationTree; | ||
| import org.sonar.plugins.java.api.tree.ClassTree; | ||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
|
|
||
| @Rule(key = "S8914") | ||
| public class QuarkusWebSocketSecurityAnnotationCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final String WEBSOCKET_ANNOTATION = "io.quarkus.websockets.next.WebSocket"; | ||
| private static final String AUTHENTICATED_ANNOTATION = "io.quarkus.security.Authenticated"; | ||
| private static final String ROLES_ALLOWED_ANNOTATION = "jakarta.annotation.security.RolesAllowed"; | ||
|
|
||
| private static final List<String> WEBSOCKET_CALLBACK_ANNOTATIONS = List.of( | ||
| "io.quarkus.websockets.next.OnOpen", | ||
| "io.quarkus.websockets.next.OnTextMessage", | ||
| "io.quarkus.websockets.next.OnBinaryMessage", | ||
| "io.quarkus.websockets.next.OnClose", | ||
| "io.quarkus.websockets.next.OnError", | ||
| "io.quarkus.websockets.next.OnPongMessage" | ||
| ); | ||
|
|
||
| private static final List<String> SECURITY_ANNOTATIONS = List.of( | ||
| AUTHENTICATED_ANNOTATION, | ||
| ROLES_ALLOWED_ANNOTATION | ||
| ); | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return List.of(Tree.Kind.CLASS); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| ClassTree classTree = (ClassTree) tree; | ||
|
|
||
| // Check if the class is annotated with @WebSocket | ||
| boolean isWebSocketEndpoint = classTree.modifiers().annotations().stream() | ||
| .anyMatch(annotation -> annotation.annotationType().symbolType().is(WEBSOCKET_ANNOTATION)); | ||
|
|
||
| if (!isWebSocketEndpoint) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if the class has class-level security annotations | ||
| boolean hasClassLevelSecurity = classTree.modifiers().annotations().stream() | ||
| .anyMatch(annotation -> SECURITY_ANNOTATIONS.stream() | ||
| .anyMatch(securityAnnotation -> annotation.annotationType().symbolType().is(securityAnnotation))); | ||
|
|
||
| // If class already has security at class level, no issues to report | ||
| if (hasClassLevelSecurity) { | ||
| return; | ||
| } | ||
|
|
||
| // Check methods for security annotations on WebSocket callbacks | ||
| classTree.members().stream() | ||
| .filter(member -> member.is(Tree.Kind.METHOD)) | ||
| .map(MethodTree.class::cast) | ||
| .forEach(this::checkMethod); | ||
| } | ||
|
|
||
| private void checkMethod(MethodTree methodTree) { | ||
| // Check if the method is a WebSocket callback | ||
| boolean isWebSocketCallback = methodTree.modifiers().annotations().stream() | ||
| .anyMatch(annotation -> WEBSOCKET_CALLBACK_ANNOTATIONS.stream() | ||
| .anyMatch(callbackAnnotation -> annotation.annotationType().symbolType().is(callbackAnnotation))); | ||
|
|
||
| if (!isWebSocketCallback) { | ||
| return; | ||
| } | ||
|
|
||
| // Find security annotations on the method | ||
| List<AnnotationTree> securityAnnotations = methodTree.modifiers().annotations().stream() | ||
| .filter(annotation -> SECURITY_ANNOTATIONS.stream() | ||
| .anyMatch(securityAnnotation -> annotation.annotationType().symbolType().is(securityAnnotation))) | ||
| .toList(); | ||
|
|
||
| // Report issue for each security annotation on the method | ||
| for (AnnotationTree securityAnnotation : securityAnnotations) { | ||
| reportIssue( | ||
| securityAnnotation, | ||
| "Move this security annotation to the WebSocket endpoint class to secure the HTTP upgrade handshake.", | ||
| List.of(), | ||
| null | ||
| ); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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_ANNOTATIONSonly containsio.quarkus.security.Authenticatedandjakarta.annotation.security.RolesAllowed. Quarkus/Jakarta also recognizejakarta.annotation.security.PermitAllandjakarta.annotation.security.DenyAllas 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 toSECURITY_ANNOTATIONSif appropriate.Was this helpful? React with 👍 / 👎