SONARPY-831 Rule S3752: Allowing both safe and unsafe HTTP methods is security-sensitive (Flask)

Author: andreaguarino

python-checks/src/main/java/org/sonar/python/checks/hotspots/UnsafeHttpMethodsCheck.java

@@ -22,6 +22,7 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import org.sonar.check.Rule;
@@ -33,18 +34,22 @@
 import org.sonar.plugins.python.api.tree.CallExpression;
 import org.sonar.plugins.python.api.tree.Decorator;
 import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.FileInput;
 import org.sonar.plugins.python.api.tree.FunctionDef;
 import org.sonar.plugins.python.api.tree.ListLiteral;
 import org.sonar.plugins.python.api.tree.RegularArgument;
 import org.sonar.plugins.python.api.tree.StringLiteral;
 import org.sonar.python.semantic.FunctionSymbolImpl;
 import org.sonar.python.tree.FunctionDefImpl;
+import org.sonar.python.tree.TreeUtils;
 
 import static org.sonar.plugins.python.api.tree.Tree.Kind.CALL_EXPR;
+import static org.sonar.plugins.python.api.tree.Tree.Kind.FILE_INPUT;
 import static org.sonar.plugins.python.api.tree.Tree.Kind.FUNCDEF;
 import static org.sonar.plugins.python.api.tree.Tree.Kind.LIST_LITERAL;
 import static org.sonar.plugins.python.api.tree.Tree.Kind.REGULAR_ARGUMENT;
 import static org.sonar.plugins.python.api.tree.Tree.Kind.STRING_LITERAL;
+import static org.sonar.python.tree.TreeUtils.argumentByKeyword;
 import static org.sonar.python.tree.TreeUtils.getSymbolFromTree;
 
 @Rule(key = "S3752")
@@ -65,6 +70,8 @@ public void initialize(Context context) {
       FunctionDef functionDef = (FunctionDef) ctx.syntaxNode();
       if (isDjangoView(functionDef)) {
         checkDjangoView(functionDef, ctx);
+      } else {
+        getFlaskViewDecorator(functionDef).ifPresent(callExpression -> checkFlaskView(callExpression, ctx));
       }
     });
   }
@@ -121,4 +128,39 @@ private static boolean isDjangoView(FunctionDef functionDef) {
       .filter(FunctionSymbolImpl::isDjangoView)
       .isPresent();
   }
+
+  private static Optional<CallExpression> getFlaskViewDecorator(FunctionDef functionDef) {
+    return functionDef.decorators().stream()
+      .map(Decorator::expression)
+      .filter(expression -> expression.is(CALL_EXPR))
+      .map(CallExpression.class::cast)
+      .filter(UnsafeHttpMethodsCheck::isFlaskRouteDecorator)
+      .findFirst();
+  }
+
+  private static boolean isFlaskRouteDecorator(CallExpression callExpression) {
+    Symbol calleeSymbol = callExpression.calleeSymbol();
+    if (calleeSymbol == null) {
+      return false;
+    }
+    return calleeSymbol.name().equals("route");
+  }
+
+  private static void checkFlaskView(CallExpression callExpression, SubscriptionContext ctx) {
+    RegularArgument methodsArg = argumentByKeyword("methods", callExpression.arguments());
+    if (methodsArg != null && hasBothUnsafeAndSafeHttpMethods(methodsArg) && isFlaskImported(callExpression)) {
+      ctx.addIssue(callExpression, MESSAGE);
+    }
+  }
+
+  private static boolean isFlaskImported(CallExpression callExpression) {
+    // When SONARPY-834 will be implemented we can have a cleaner implementation
+    // checking decorator fqn to be equal to flask.blueprints.Blueprint.route
+    return Optional.ofNullable(TreeUtils.firstAncestorOfKind(callExpression, FILE_INPUT))
+      .filter(fileInput -> ((FileInput) fileInput).globalVariables().stream()
+          .map(Symbol::fullyQualifiedName)
+          .filter(Objects::nonNull)
+          .anyMatch(fqn -> fqn.contains("flask")))
+      .isPresent();
+  }
 }

python-checks/src/test/java/org/sonar/python/checks/hotspots/UnsafeHttpMethodsCheckTest.java

@@ -34,4 +34,12 @@ public void test_django() {
       ),
       new UnsafeHttpMethodsCheck());
   }
+
+  @Test
+  public void test_flask() {
+    PythonCheckVerifier.verify(Arrays.asList(
+      "src/test/resources/checks/hotspots/unsafeHttpMethods/flask/views.py",
+      "src/test/resources/checks/hotspots/unsafeHttpMethods/flask/otherDecorator.py"
+    ), new UnsafeHttpMethodsCheck());
+  }
 }

python-checks/src/test/resources/checks/hotspots/unsafeHttpMethods/flask/otherDecorator.py

@@ -0,0 +1,13 @@
+class Methods:
+    def route(self): ...
+def foo(): ...
+
+methods = Methods()
+
+@methods.route('/sensitive1', methods=['GET', 'POST'])  # OK
+def compliant():
+    return Response("Method: " + request.method, 200)
+
+@foo('/sensitive1', methods=['GET', 'POST'])  # OK
+def compliant2():
+    return Response("Method: " + request.method, 200)

python-checks/src/test/resources/checks/hotspots/unsafeHttpMethods/flask/views.py

@@ -0,0 +1,55 @@
+from flask import Blueprint, request
+from flask import Response
+
+methods = Blueprint('methods', __name__)
+
+# 127.0.0.1:5000/methods/sensitive1
+@methods.route('/sensitive1', methods=['GET', 'POST'])  # Noncompliant
+def sensitive1():
+    return Response("Method: " + request.method, 200)
+
+# 127.0.0.1:5000/methods/compliant1
+@methods.route('/compliant1')  # Compliant
+def compliant1():
+    return Response("Method: " + request.method, 200)
+
+# 127.0.0.1:5000/methods/compliant2
+@methods.route('/compliant2', methods=['GET'])  # Compliant
+def compliant2():
+    return Response("Method: " + request.method, 200)
+
+@methods.other('/compliant3', methods=['GET', 'POST'])  # Compliant
+def compliant3():
+    return Response("Method: " + request.method, 200)
+
+@unknown('/compliant4', methods=['GET', 'POST'])  # Compliant
+def compliant4():
+    return Response("Method: " + request.method, 200)
+
+@methods.route('/compliant5', methods=[getMethod()])  # Compliant
+def compliant5():
+    return Response("Method: " + request.method, 200)
+
+@methods.route('/compliant6', methods=getMethods())  # Compliant
+def compliant6():
+    return Response("Method: " + request.method, 200)
+
+@methods.route('/compliant7', methods=['FOO'])  # Compliant
+def compliant7():
+    return Response("Method: " + request.method, 200)
+
+def inside_fn():
+    from flask import Blueprint, request
+    from flask import Response
+
+    methods = Blueprint('methods', __name__)
+
+    # 127.0.0.1:5000/methods/sensitive1
+    @methods.route('/sensitive1', methods=['GET', 'POST'])  # Noncompliant
+    def sensitive1():
+        return Response("Method: " + request.method, 200)
+
+    # 127.0.0.1:5000/methods/compliant1
+    @methods.route('/compliant1')  # Compliant
+    def compliant1():
+        return Response("Method: " + request.method, 200)