Skip to content

Commit

Permalink
SONARJAVA-4871 S5122: Support detection of Permissive CORS policies f…
Browse files Browse the repository at this point in the history
…or Spring (#4747)

* SONARJAVA-4871 S5122: Support detection of Permissive CORS policies for Spring

* Fix autoscan

* Fix malformed spring-webmvc dependency

* Worked on review comments
  • Loading branch information
kaufco authored Apr 2, 2024
1 parent a6869e3 commit 128fa9b
Show file tree
Hide file tree
Showing 7 changed files with 100 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S1874",
"hasTruePositives": true,
"falseNegatives": 150,
"falseNegatives": 151,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S5122",
"hasTruePositives": true,
"falseNegatives": 18,
"falseNegatives": 26,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -198,5 +198,4 @@ public void addCorsMappings(CorsRegistry registry) {
.allowedOrigins("safe.com"); // Compliant
}
}

}
8 changes: 7 additions & 1 deletion java-checks-test-sources/spring-3.2/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,16 @@
<version>6.1.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-webmvc</artifactId>
<version>6.1.5</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-web</artifactId>
<version>5.2.10.RELEASE</version>
<version>6.2.1</version>
<scope>provided</scope>
</dependency>
</dependencies>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package checks;

import java.util.Arrays;
import java.util.List;
import org.springframework.web.cors.CorsConfiguration;
import org.springframework.web.servlet.config.annotation.CorsRegistration;
import org.springframework.web.servlet.config.annotation.CorsRegistry;

public class CORSCheckSample {

private void additional_Spring_5_3_methods() {
CorsConfiguration configuration = new CorsConfiguration();
configuration.setAllowedOrigins(Arrays.asList("*")); // Noncompliant
configuration.setAllowedOrigins(List.of("*")); // Noncompliant
configuration.setAllowedOrigins(List.of("com.mycompany")); // Compliant
configuration.setAllowedOriginPatterns(List.of("*")); // Noncompliant

CorsRegistration registration = new CorsRegistration("");
registration.allowedOrigins("*"); // Noncompliant
registration.allowedOriginPatterns("*"); // Noncompliant
registration.allowedOriginPatterns("com.mycompany/*"); // Complaint

CorsRegistry registry = new CorsRegistry();
registry.addMapping("*"); // Noncompliant
registry.addMapping("com.mycompany"); // Compliant
registry.addMapping("com.mycompany/*"); // Complaint
}

private void coverage() {
CorsConfiguration configuration = new CorsConfiguration();
configuration.setAllowedOrigins(List.of("com.mycompany", "*", "com.yourcompany")); // Noncompliant
configuration.setAllowedOrigins(List.of("com.mycompany", "com.yourcompany")); // Compliant
configuration.setAllowedOrigins(List.of()); // Compliant
configuration.setAllowedOrigins(getList()); // Compliant
var list = List.of("com.mycompany", "*", "com.yourcompany");
configuration.setAllowedOrigins(list); // Compliant

CorsRegistration registration = new CorsRegistration("");
registration.allowedOrigins("com.mycompany", "*", "com.yourcompany"); // Noncompliant
registration.allowedOrigins("com.mycompany", "com.yourcompany"); // Compliant
registration.allowedOrigins(); // Compliant
}

private static List<String> getList() {
return List.of("com.mycompany", "*", "com.yourcompany");
}
}
35 changes: 28 additions & 7 deletions java-checks/src/main/java/org/sonar/java/checks/CORSCheck.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,26 @@ public class CORSCheck extends IssuableSubscriptionVisitor {
private static final MethodMatchers ADD_ALLOWED_ORIGIN_MATCHER = MethodMatchers.or(
MethodMatchers.create()
.ofTypes("org.springframework.web.cors.CorsConfiguration")
.names("addAllowedOrigin")
.names("addAllowedOrigin", "setAllowedOrigins", "setAllowedOriginPatterns")
.withAnyParameters()
.build(),
MethodMatchers.create().ofTypes("org.springframework.web.servlet.config.annotation.CorsRegistration")
.names("allowedOrigins")
.names("allowedOrigins", "allowedOriginPatterns")
.withAnyParameters()
.build(),
MethodMatchers.create().ofTypes("org.springframework.web.servlet.config.annotation.CorsRegistry")
.names("addMapping")
.withAnyParameters()
.build());

private static final MethodMatchers LIST_INITIALIZER_MATCHER = MethodMatchers.or(
MethodMatchers.create()
.ofTypes("java.util.Arrays")
.names("asList")
.withAnyParameters()
.build(),
MethodMatchers.create().ofTypes("java.util.List")
.names("of")
.withAnyParameters()
.build());

Expand Down Expand Up @@ -127,11 +142,17 @@ private void reportTree(Tree tree) {
}

private static boolean isStar(ExpressionTree expressionTree) {
if (expressionTree.is(Tree.Kind.NEW_ARRAY)) {
return ((NewArrayTree) expressionTree).initializers().stream().anyMatch(CORSCheck::isStar);
} else {
return "*".equals(ExpressionsHelper.getConstantValueAsString(expressionTree).value());
if (expressionTree instanceof NewArrayTree tree) {
return containsStar(tree.initializers());
}
if (expressionTree instanceof MethodInvocationTree tree) {
return LIST_INITIALIZER_MATCHER.matches(tree) && containsStar(tree.arguments());
}
return "*".equals(ExpressionsHelper.getConstantValueAsString(expressionTree).value());
}

private static boolean containsStar(List<ExpressionTree> list) {
return list.stream().anyMatch(CORSCheck::isStar);
}

private class MethodInvocationVisitor extends BaseTreeVisitor {
Expand All @@ -147,7 +168,7 @@ public void visitMethodInvocation(MethodInvocationTree mit) {
}
} else if (APPLY_PERMIT_DEFAULT_VALUES.matches(mit)) {
applyPermit.add(mit);
} else if (ADD_ALLOWED_ORIGIN_MATCHER.matches(mit) && isStar(mit.arguments().get(0))) {
} else if (ADD_ALLOWED_ORIGIN_MATCHER.matches(mit) && containsStar(mit.arguments())) {
addAllowedOrigin.add(mit);
}
super.visitMethodInvocation(mit);
Expand Down
14 changes: 14 additions & 0 deletions java-checks/src/test/java/org/sonar/java/checks/CORSCheckTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,19 @@
*/
package org.sonar.java.checks;

import java.io.File;
import java.util.List;
import org.junit.jupiter.api.Test;
import org.sonar.java.checks.verifier.CheckVerifier;
import org.sonar.java.test.classpath.TestClasspathUtils;

import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPathInModule;

class CORSCheckTest {

private static final List<File> SPRING_3_2_CLASSPATH = TestClasspathUtils.loadFromFile(Constants.SPRING_3_2_CLASSPATH);

@Test
void test() {
CheckVerifier.newVerifier()
Expand All @@ -34,4 +40,12 @@ void test() {
.verifyIssues();
}

@Test
void test_with_spring_3_2() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPathInModule(Constants.SPRING_3_2, "checks/CORSCheckSample.java"))
.withCheck(new CORSCheck())
.withClassPath(SPRING_3_2_CLASSPATH)
.verifyIssues();
}
}

0 comments on commit 128fa9b

Please sign in to comment.