SONARPY-1569 [S6779] Highlight on the line that actually contains the…

… Flask secret (#1680)

python-checks/src/main/java/org/sonar/python/checks/FlaskHardCodedSecret.java

@@ -51,6 +51,7 @@ public abstract class FlaskHardCodedSecret extends PythonSubscriptionCheck {
     "flask.app.Flask.config",
     "flask.globals.current_app.config"
   );
+  public static final String SECONDARY_LOCATION_MESSAGE = "The secret is used in this call.";
 
   protected abstract String getSecretKeyKeyword();
 
@@ -84,9 +85,13 @@ private void verifyUpdateCallArgument(SubscriptionContext ctx, CallExpression ca
       .flatMap(TreeUtils.toOptionalInstanceOfMapper(RegularArgument.class))
       .map(RegularArgument::expression)
       .map(FlaskHardCodedSecret::getAssignedValue)
-      .filter(this::isIllegalDictArgument)
-      .ifPresent(expr -> ctx.addIssue(callExpression, String.format(MESSAGE, getSecretKeyType())));
+      .flatMap(this::getIllegalDictArgument)
+      .ifPresent(illegalArgument -> ctx.addIssue(illegalArgument, getMessage())
+        .secondary(callExpression.callee(), SECONDARY_LOCATION_MESSAGE));
+  }
 
+  private String getMessage() {
+    return String.format(MESSAGE, getSecretKeyType());
   }
 
   private static Expression getAssignedValue(Expression expression) {
@@ -96,13 +101,16 @@ private static Expression getAssignedValue(Expression expression) {
     return expression;
   }
 
-  private boolean isIllegalDictArgument(Expression expression) {
+  private Optional<Tree> getIllegalDictArgument(Expression expression) {
     if (expression.is(Tree.Kind.CALL_EXPR)) {
-      return isCallToDictConstructor((CallExpression) expression) && hasIllegalKeywordArgument((CallExpression) expression);
+      return TreeUtils.toOptionalInstanceOf(CallExpression.class, expression)
+        .filter(FlaskHardCodedSecret::isCallToDictConstructor)
+        .flatMap(this::getIllegalKeywordArgument);
     } else if (expression.is(Tree.Kind.DICTIONARY_LITERAL)) {
-      return hasIllegalKeyValuePair((DictionaryLiteral) expression);
+      return TreeUtils.toOptionalInstanceOf(DictionaryLiteral.class, expression)
+        .flatMap(this::getIllegalKeyValuePair);
     }
-    return false;
+    return Optional.empty();
   }
 
   private static boolean isCallToDictConstructor(CallExpression callExpression) {
@@ -115,11 +123,12 @@ private static boolean isCallToDictConstructor(CallExpression callExpression) {
       .isPresent();
   }
 
-  private boolean hasIllegalKeyValuePair(DictionaryLiteral dictionaryLiteral) {
+  private Optional<KeyValuePair> getIllegalKeyValuePair(DictionaryLiteral dictionaryLiteral) {
     return dictionaryLiteral.elements().stream()
       .filter(KeyValuePair.class::isInstance)
       .map(KeyValuePair.class::cast)
-      .anyMatch(this::isIllegalKeyValuePair);
+      .filter(this::isIllegalKeyValuePair)
+      .findFirst();
   }
 
   private boolean isIllegalKeyValuePair(KeyValuePair keyValuePair) {
@@ -131,11 +140,12 @@ private boolean isIllegalKeyValuePair(KeyValuePair keyValuePair) {
       .isPresent() && isStringValue(keyValuePair.value());
   }
 
-  private boolean hasIllegalKeywordArgument(CallExpression callExpression) {
+  private Optional<RegularArgument> getIllegalKeywordArgument(CallExpression callExpression) {
     return Optional.ofNullable(TreeUtils.argumentByKeyword(getSecretKeyKeyword(), callExpression.arguments()))
-      .map(RegularArgument::expression)
-      .filter(FlaskHardCodedSecret::isStringValue)
-      .isPresent();
+      .filter(argument -> Optional.of(argument)
+        .map(RegularArgument::expression)
+        .filter(FlaskHardCodedSecret::isStringValue)
+        .isPresent());
   }
 
   private void verifyAssignmentStatement(SubscriptionContext ctx) {
@@ -149,7 +159,7 @@ private void verifyAssignmentStatement(SubscriptionContext ctx) {
       .filter(this::isSensitiveProperty)
       .collect(Collectors.toList());
     if (!expressionList.isEmpty()) {
-      PreciseIssue issue = ctx.addIssue(assignmentStatementTree.assignedValue(), String.format(MESSAGE, getSecretKeyType()));
+      PreciseIssue issue = ctx.addIssue(assignmentStatementTree.assignedValue(), getMessage());
       expressionList.forEach(expr -> issue.secondary(expr, SECONDARY_MESSAGE));
     }
   }

python-checks/src/test/resources/checks/flaskHardCodedJWTSecretKey.py

@@ -38,30 +38,39 @@ def test_non_compliant_call_expressions():
     app = Flask(__name__)
 
     # Tests for "flask.app.Flask.config[.update]"
-    app.config.update(dict( # Noncompliant
-#   ^[el=+3;ec=6]
-        JWT_SECRET_KEY="woopie"
+    app.config.update(dict(
+        JWT_SECRET_KEY="woopie" # Noncompliant
+#       ^^^^^^^^^^^^^^^^^^^^^^^
     ))
 
     app.config.update({"JWT_SECRET_KEY": "woopie"}) # Noncompliant
 
     d = dict(
-        JWT_SECRET_KEY="woopie"
+        JWT_SECRET_KEY="woopie" # Noncompliant 2
     )
-    d1 = {"JWT_SECRET_KEY": "woopie"}
+    d1 = {"JWT_SECRET_KEY": "woopie"} # Noncompliant 2
 
-    app.config.update(d)            # Noncompliant
-    app.config.update(d1)           # Noncompliant
+    app.config.update(d)
+    app.config.update(d1)
 
     # Tests for "flask.globals.current_app.config.update"
-    current_app.config.update(dict( # Noncompliant
-        JWT_SECRET_KEY="woopie"
+    current_app.config.update(dict(
+        JWT_SECRET_KEY="woopie" # Noncompliant
     ))
 
     current_app.config.update({"JWT_SECRET_KEY": "woopie"}) # Noncompliant
 
-    current_app.config.update(d)            # Noncompliant
-    current_app.config.update(d1)           # Noncompliant
+    current_app.config.update(d)
+    current_app.config.update(d1)
+
+    d2 = dict(JWT_SECRET_KEY="woopie")  # Noncompliant {{Don't disclose "Flask" JWT secret keys.}}
+#             ^^^^^^^^^^^^^^^^^^^^^^^
+    app.config.update(d2)
+#   ^^^^^^^^^^^^^^^^^<1 {{The secret is used in this call.}}
+    d3 = {"JWT_SECRET_KEY": "woopie"}  # Noncompliant {{Don't disclose "Flask" JWT secret keys.}}
+#         ^^^^^^^^^^^^^^^^^^^^^^^^^^
+    app.config.update(d3)
+#   ^^^^^^^^^^^^^^^^^<1 {{The secret is used in this call.}}
 
 
 def get_secret_from_vault():

python-checks/src/test/resources/checks/flaskHardCodedSecretKey.py

@@ -70,30 +70,38 @@ def test_non_compliant_call_expressions():
     app = Flask(__name__)
 
     # Tests for "flask.app.Flask.config[.update]"
-    app.config.update(dict( # Noncompliant
-#   ^[el=+3;ec=6]
-        SECRET_KEY="woopie"
+    app.config.update(dict(
+        SECRET_KEY="woopie" # Noncompliant
     ))
 
     app.config.update({"SECRET_KEY": "woopie"}) # Noncompliant
 
     d = dict(
-        SECRET_KEY="woopie"
+        SECRET_KEY="woopie" # Noncompliant 2
     )
-    d1 = {"SECRET_KEY": "woopie"}
+    d1 = {"SECRET_KEY": "woopie"} # Noncompliant 2
 
-    app.config.update(d)            # Noncompliant
-    app.config.update(d1)           # Noncompliant
+    app.config.update(d)
+    app.config.update(d1)
 
     # Tests for "flask.globals.current_app.config.update"
-    current_app.config.update(dict( # Noncompliant
-        SECRET_KEY="woopie"
+    current_app.config.update(dict(
+        SECRET_KEY="woopie" # Noncompliant
     ))
 
     current_app.config.update({"SECRET_KEY": "woopie"}) # Noncompliant
 
-    current_app.config.update(d)            # Noncompliant
-    current_app.config.update(d1)           # Noncompliant
+    current_app.config.update(d)
+    current_app.config.update(d1)
+
+    d2 = dict(SECRET_KEY="woopie") # Noncompliant {{Don't disclose "Flask" secret keys.}}
+#             ^^^^^^^^^^^^^^^^^^^
+    app.config.update(d2)
+#   ^^^^^^^^^^^^^^^^^<1 {{The secret is used in this call.}}
+    d3 = {"SECRET_KEY": "woopie"} # Noncompliant {{Don't disclose "Flask" secret keys.}}
+#         ^^^^^^^^^^^^^^^^^^^^^^
+    app.config.update(d3)
+#   ^^^^^^^^^^^^^^^^^<1 {{The secret is used in this call.}}
 
 
 def get_secret_from_vault():