From 565d913069be7f7b94a7fd76cd5aa4614282321e Mon Sep 17 00:00:00 2001
From: Marcin Stachniuk <marcin.stachniuk@sonarsource.com>
Date: Wed, 6 Nov 2024 17:07:17 +0100
Subject: [PATCH] Code review remarks

---
 .../php/checks/HardCodedSecretCheck.java      | 23 +++++++--------
 .../checks/HardCodedSecretCheckAST.php        | 29 +++++++++++++++++--
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/php-checks/src/main/java/org/sonar/php/checks/HardCodedSecretCheck.java b/php-checks/src/main/java/org/sonar/php/checks/HardCodedSecretCheck.java
index f73765852..46358d751 100644
--- a/php-checks/src/main/java/org/sonar/php/checks/HardCodedSecretCheck.java
+++ b/php-checks/src/main/java/org/sonar/php/checks/HardCodedSecretCheck.java
@@ -51,14 +51,13 @@ public class HardCodedSecretCheck extends PHPVisitorCheck {
   private static final String DEFAULT_SECRET_WORDS = "api[_.-]?key,auth,credential,secret,token";
   private static final String DEFAULT_RANDOMNESS_SENSIBILITY = "5.0";
   private static final double LANGUAGE_SCORE_INCREMENT = 0.3;
-  private static final int TWO_ARGUMENTS = 2;
   private static final int MAX_RANDOMNESS_SENSIBILITY = 10;
   private static final int MINIMUM_CREDENTIAL_LENGTH = 17;
 
   private static final String FIRST_ACCEPTED_CHARACTER = "[\\w.+/~$:&-]";
   private static final String FOLLOWING_ACCEPTED_CHARACTER = "[=\\w.+/~$:&-]";
   private static final Pattern SECRET_PATTERN = Pattern.compile(FIRST_ACCEPTED_CHARACTER + "(" + FOLLOWING_ACCEPTED_CHARACTER + "|\\\\\\\\" + FOLLOWING_ACCEPTED_CHARACTER + ")++");
-  private static final Pattern IPV_6_PATTERN = Pattern.compile("%s|%s".formatted(IP_V4, IP_V6));
+  private static final Pattern IP_PATTERN = Pattern.compile("%s|%s".formatted(IP_V4, IP_V6));
 
   @RuleProperty(
     key = "secretWords",
@@ -96,7 +95,7 @@ public void visitFunctionCall(FunctionCallTree tree) {
       visitDefineFunctionCall(tree);
     } else if ("strcasecmp".equals(functionName) || "strcmp".equals(functionName)) {
       visitStringCompareFunctionCall(tree);
-    } else if (tree.callArguments().size() == TWO_ARGUMENTS) {
+    } else if (tree.callArguments().size() == 2) {
       visitTwoArgumentsFunctionCall(tree);
     }
     super.visitFunctionCall(tree);
@@ -115,13 +114,13 @@ private void visitDefineFunctionCall(FunctionCallTree tree) {
   private void visitStringCompareFunctionCall(FunctionCallTree tree) {
     var string1 = CheckUtils.resolvedArgumentLiteral(tree, "string1", 0);
     var string2 = CheckUtils.resolvedArgumentLiteral(tree, "string2", 1);
-    if (string1.isPresent()) {
+    if (string1.isPresent() && tree.callArguments().size() == 2) {
       var callArg = tree.callArguments().get(1).value();
       if (callArg instanceof VariableIdentifierTree variableIdentifier) {
         detectSecret(variableIdentifier.text(), string1.get().value(), string1.get());
       }
     }
-    if (string2.isPresent()) {
+    if (string2.isPresent() && tree.callArguments().size() == 2) {
       var callArg = tree.callArguments().get(0).value();
       if (callArg instanceof VariableIdentifierTree variableIdentifier) {
         detectSecret(variableIdentifier.text(), string2.get().value(), string2.get());
@@ -168,13 +167,13 @@ public void visitArrayPair(ArrayPairTree tree) {
 
   @Override
   public void visitBinaryExpression(BinaryExpressionTree tree) {
-    var leftOperant = tree.leftOperand();
-    var rightOperant = tree.rightOperand();
-    if (rightOperant instanceof LiteralTree secretValueTree && leftOperant instanceof VariableIdentifierTree variableTree) {
-      detectSecret(variableTree.text(), secretValueTree.value(), rightOperant);
+    var leftOperand = tree.leftOperand();
+    var rightOperand = tree.rightOperand();
+    if (rightOperand instanceof LiteralTree secretValueTree && leftOperand instanceof VariableIdentifierTree variableTree) {
+      detectSecret(variableTree.text(), secretValueTree.value(), rightOperand);
     }
-    if (leftOperant instanceof LiteralTree secretValueTree && rightOperant instanceof VariableIdentifierTree variableTree) {
-      detectSecret(variableTree.text(), secretValueTree.value(), leftOperant);
+    if (leftOperand instanceof LiteralTree secretValueTree && rightOperand instanceof VariableIdentifierTree variableTree) {
+      detectSecret(variableTree.text(), secretValueTree.value(), leftOperand);
     }
     super.visitBinaryExpression(tree);
   }
@@ -226,7 +225,7 @@ private boolean isRandom(String literal) {
   }
 
   private static boolean isNotIpV6(String literal) {
-    return !IPV_6_PATTERN.matcher(literal).matches();
+    return !IP_PATTERN.matcher(literal).matches();
   }
 
   private EntropyDetector entropyDetector() {
diff --git a/php-checks/src/test/resources/checks/HardCodedSecretCheckAST.php b/php-checks/src/test/resources/checks/HardCodedSecretCheckAST.php
index 66c6e00aa..7971ed0c0 100644
--- a/php-checks/src/test/resources/checks/HardCodedSecretCheckAST.php
+++ b/php-checks/src/test/resources/checks/HardCodedSecretCheckAST.php
@@ -30,11 +30,12 @@
 
 define("namespace\level\Token", "abcdefghijklmnopqrs");   // Noncompliant
 
+define("abcdefghijklmnopqrs");    // Compliant
 
 // Variables declarations (as AssignmentExpression)
-$var_ok = "abcdefghijklmnopqrs"; // Compliant
+$var_ok = "abcdefghijklmnopqrs";  // Compliant
 
-$oauth = "abcdefghijklmnopqrs";  // Noncompliant
+$oauth = "abcdefghijklmnopqrs";   // Noncompliant
 
 // Variables in function
 function do_something(): void
@@ -178,6 +179,18 @@ function compareStrings($auth)
   {
     echo $auth;
   }
+  if (strcmp())
+  {
+    echo $auth;
+  }
+  if (strcmp("abcdefghijklmnopqrs"))
+  {
+    echo $auth;
+  }
+  if (strcmp("abcdefghijklmnopqrs", $auth, "abc"))
+  {
+    echo $auth;
+  }
 
   // case insensitive
   if (strcasecmp($auth, "abcdefghijklmnopqrs"))    // Noncompliant
@@ -208,6 +221,18 @@ function compareStrings($auth)
   {
     echo $auth;
   }
+  if (strcasecmp())
+  {
+    echo $auth;
+  }
+  if (strcasecmp("abcdefghijklmnopqrs"))
+  {
+    echo $auth;
+  }
+  if (strcasecmp("abcdefghijklmnopqrs", $auth, "abc"))
+  {
+    echo $auth;
+  }
 }
 
 function some2ArgsFunction($arg1, $arg2)