Address review changes

ballerina-platform · Feb 23, 2025 · 81d0c7d · 81d0c7d
1 parent b64cbec
commit 81d0c7d
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 68 deletions.
diff --git a/compiler-plugin-tests/build.gradle b/compiler-plugin-tests/build.gradle
@@ -22,7 +22,7 @@ plugins {
     id 'com.github.spotbugs'
 }
 
-description = 'Ballerina - Cache Compiler Plugin Tests'
+description = 'Ballerina - Log Compiler Plugin Tests'
 
 dependencies {
     checkstyle project(':checkstyle')

diff --git a/...plugin-tests/src/test/resources/static_code_analyzer/ballerina_packages/rule1/Config.toml b/...plugin-tests/src/test/resources/static_code_analyzer/ballerina_packages/rule1/Config.toml
@@ -1 +1,2 @@
 password = "xyz"
+user = "abc"
diff --git a/...er-plugin-tests/src/test/resources/static_code_analyzer/ballerina_packages/rule1/main.bal b/...er-plugin-tests/src/test/resources/static_code_analyzer/ballerina_packages/rule1/main.bal
@@ -15,30 +15,18 @@
 // under the License.
 
 import ballerina/log;
-import ballerina/http;
-import ballerina/file;
 
 public function main() {
     log:printInfo(password);
     log:printError(string `Error: ${password}`);
     log:printError("Error " + password);
     log:printWarn("Warning", password = password);
+    log:printError("Error", password = password, user = user);
 }
 
 function log() {
     log:printInfo("Info");
 }
 
-service on new http:Listener(9090) {
-    resource function get test() {
-        log:printInfo("Info", password = password);
-    }
-}
-
-service  on new file:Listener({path: "/tmp", recursive: true}) {
-    remote function onCreate(file:FileEvent event) {
-        log:printInfo("File created: ", path = event.name);
-    }
-}
-
 configurable string password = ?;
+configurable string user = ?;
diff --git a/compiler-plugin-tests/src/test/resources/static_code_analyzer/expected_output/rule1.json b/compiler-plugin-tests/src/test/resources/static_code_analyzer/expected_output/rule1.json
@@ -2,17 +2,17 @@
   {
     "location": {
       "filePath": "main.bal",
-      "startLine": 21,
-      "endLine": 21,
-      "startColumn": 4,
-      "endColumn": 28,
-      "startOffset": 743,
-      "length": 24
+      "startLine": 19,
+      "endLine": 19,
+      "startColumn": 18,
+      "endColumn": 26,
+      "startOffset": 711,
+      "length": 8
     },
     "rule": {
       "id": "ballerina/log:1",
       "numericId": 1,
-      "description": "Avoid logging configurable variables",
+      "description": "Potentially-sensitive configurable variables are logged",
       "ruleKind": "VULNERABILITY"
     },
     "source": "BUILT_IN",
@@ -22,17 +22,17 @@
   {
     "location": {
       "filePath": "main.bal",
-      "startLine": 22,
-      "endLine": 22,
-      "startColumn": 4,
-      "endColumn": 48,
-      "startOffset": 772,
-      "length": 44
+      "startLine": 20,
+      "endLine": 20,
+      "startColumn": 36,
+      "endColumn": 44,
+      "startOffset": 758,
+      "length": 8
     },
     "rule": {
       "id": "ballerina/log:1",
       "numericId": 1,
-      "description": "Avoid logging configurable variables",
+      "description": "Potentially-sensitive configurable variables are logged",
       "ruleKind": "VULNERABILITY"
     },
     "source": "BUILT_IN",
@@ -42,17 +42,17 @@
   {
     "location": {
       "filePath": "main.bal",
-      "startLine": 23,
-      "endLine": 23,
-      "startColumn": 4,
-      "endColumn": 40,
-      "startOffset": 821,
-      "length": 36
+      "startLine": 21,
+      "endLine": 21,
+      "startColumn": 30,
+      "endColumn": 38,
+      "startOffset": 801,
+      "length": 8
     },
     "rule": {
       "id": "ballerina/log:1",
       "numericId": 1,
-      "description": "Avoid logging configurable variables",
+      "description": "Potentially-sensitive configurable variables are logged",
       "ruleKind": "VULNERABILITY"
     },
     "source": "BUILT_IN",
@@ -62,17 +62,17 @@
   {
     "location": {
       "filePath": "main.bal",
-      "startLine": 24,
-      "endLine": 24,
-      "startColumn": 4,
-      "endColumn": 50,
-      "startOffset": 862,
-      "length": 46
+      "startLine": 22,
+      "endLine": 22,
+      "startColumn": 40,
+      "endColumn": 48,
+      "startOffset": 852,
+      "length": 8
     },
     "rule": {
       "id": "ballerina/log:1",
       "numericId": 1,
-      "description": "Avoid logging configurable variables",
+      "description": "Potentially-sensitive configurable variables are logged",
       "ruleKind": "VULNERABILITY"
     },
     "source": "BUILT_IN",
@@ -82,17 +82,37 @@
   {
     "location": {
       "filePath": "main.bal",
-      "startLine": 33,
-      "endLine": 33,
-      "startColumn": 8,
-      "endColumn": 51,
-      "startOffset": 1039,
-      "length": 43
+      "startLine": 23,
+      "endLine": 23,
+      "startColumn": 39,
+      "endColumn": 47,
+      "startOffset": 902,
+      "length": 8
+    },
+    "rule": {
+      "id": "ballerina/log:1",
+      "numericId": 1,
+      "description": "Potentially-sensitive configurable variables are logged",
+      "ruleKind": "VULNERABILITY"
+    },
+    "source": "BUILT_IN",
+    "fileName": "rule1/main.bal",
+    "filePath": "/Users/danesh/ballerina-platform/module-ballerina-log/compiler-plugin-tests/src/test/resources/static_code_analyzer/ballerina_packages/rule1/main.bal"
+  },
+  {
+    "location": {
+      "filePath": "main.bal",
+      "startLine": 23,
+      "endLine": 23,
+      "startColumn": 56,
+      "endColumn": 60,
+      "startOffset": 919,
+      "length": 4
     },
     "rule": {
       "id": "ballerina/log:1",
       "numericId": 1,
-      "description": "Avoid logging configurable variables",
+      "description": "Potentially-sensitive configurable variables are logged",
       "ruleKind": "VULNERABILITY"
     },
     "source": "BUILT_IN",

diff --git a/compiler-plugin-tests/src/test/resources/testng.xml b/compiler-plugin-tests/src/test/resources/testng.xml
@@ -19,9 +19,9 @@
   -->
 
 <!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd" >
-<suite name="Cache compiler plugin test suite" parallel="false">
+<suite name="Log compiler plugin test suite" parallel="false">
 
-    <test name="Cache Compiler Plugin Tests" parallel="false">
+    <test name="Log Compiler Plugin Tests" parallel="false">
         <classes>
             <class name="io.ballerina.stdlib.log.compiler.staticcodeanalyzer.StaticCodeAnalyzerTest"/>
         </classes>

diff --git a/...ler-plugin/src/main/java/io/ballerina/stdlib/log/compiler/staticcodeanalyzer/LogRule.java b/...ler-plugin/src/main/java/io/ballerina/stdlib/log/compiler/staticcodeanalyzer/LogRule.java
@@ -24,7 +24,8 @@
 import static io.ballerina.stdlib.log.compiler.staticcodeanalyzer.RuleFactory.createRule;
 
 public enum LogRule {
-    AVOID_LOGGING_CONFIGURABLE_VARIABLES(createRule(1, "Avoid logging configurable variables", VULNERABILITY));
+    AVOID_LOGGING_CONFIGURABLE_VARIABLES(createRule(1,
+            "Potentially-sensitive configurable variables are logged", VULNERABILITY));
     private final Rule rule;
 
 

diff --git a/...c/main/java/io/ballerina/stdlib/log/compiler/staticcodeanalyzer/LogStatementAnalyzer.java b/...c/main/java/io/ballerina/stdlib/log/compiler/staticcodeanalyzer/LogStatementAnalyzer.java
@@ -53,6 +53,8 @@ public class LogStatementAnalyzer implements AnalysisTask<SyntaxNodeAnalysisCont
 
     List<SemanticModel> semanticModels = new ArrayList<>();
 
+    Document document = null;
+
     private final Reporter reporter;
 
     public LogStatementAnalyzer(Reporter reporter) {
@@ -68,6 +70,11 @@ public void perform(SyntaxNodeAnalysisContext context) {
             });
         }
 
+        // If the document is null, we get the document of the context
+        if (document == null) {
+            document = context.currentPackage().module(context.moduleId()).document(context.documentId());
+        }
+
         if (context.node() instanceof ExpressionStatementNode expressionStatementNode) {
             // Check if the log statement has a configurable qualifier
             List<ChildNodeEntry> childlist = expressionStatementNode.expression().childEntries().stream()
@@ -94,16 +101,15 @@ public void perform(SyntaxNodeAnalysisContext context) {
                         positionalArgumentNode.childEntries().forEach(childNodeEntry -> {
                             Node expression = childNodeEntry.node().orElse(null);
                             if (expression instanceof SimpleNameReferenceNode simpleNameReferenceNode) {
-                                checkConfigurableQualifier(simpleNameReferenceNode, context, expressionStatementNode);
+                                checkConfigurableQualifier(simpleNameReferenceNode);
                             } else if (expression instanceof TemplateExpressionNode templateExpressionNode) {
                                 templateExpressionNode.content().forEach(content -> {
                                     if (content instanceof InterpolationNode interpolationNode) {
                                         interpolationNode.childEntries().forEach(interpolationChild -> {
                                             Node interpolationExpression = interpolationChild.node().orElse(null);
                                             if (interpolationExpression instanceof SimpleNameReferenceNode
                                                     simpleNameReferenceNode) {
-                                                checkConfigurableQualifier(simpleNameReferenceNode, context,
-                                                        expressionStatementNode);
+                                                checkConfigurableQualifier(simpleNameReferenceNode);
                                             }
                                         });
                                     }
@@ -112,36 +118,30 @@ public void perform(SyntaxNodeAnalysisContext context) {
                                 binaryExpressionNode.childEntries().forEach(childEntry -> {
                                     Node childNode = childEntry.node().orElse(null);
                                     if (childNode instanceof SimpleNameReferenceNode simpleNameReferenceNode) {
-                                        checkConfigurableQualifier(simpleNameReferenceNode, context,
-                                                expressionStatementNode);
+                                        checkConfigurableQualifier(simpleNameReferenceNode);
                                     }
                                 });
                             }
                         });
                     } else if (node instanceof NamedArgumentNode namedArgumentNode) {
-                        checkConfigurableQualifier(namedArgumentNode.expression(), context, expressionStatementNode);
+                        checkConfigurableQualifier(namedArgumentNode.expression());
                     }
                 }
             }
         }
     }
 
-    private void checkConfigurableQualifier(ExpressionNode argumentNode, SyntaxNodeAnalysisContext context,
-                                            ExpressionStatementNode expressionStatementNode) {
+    private void checkConfigurableQualifier(ExpressionNode argumentNode) {
         semanticModels.forEach(semanticModel -> {
             Symbol symbol = semanticModel.symbol(argumentNode).orElse(null);
             if (symbol instanceof VariableSymbol variableSymbol) {
                 variableSymbol.qualifiers().stream().filter(qualifier -> qualifier
                         .toString().equals(CONFIGURABLE_QUALIFIER)).forEach(qualifier -> {
-                    this.reporter.reportIssue(getDocument(context),
-                            expressionStatementNode.location(),
+                    this.reporter.reportIssue(document,
+                            argumentNode.location(),
                             AVOID_LOGGING_CONFIGURABLE_VARIABLES.getId());
                 });
             }
         });
     }
-
-    private static Document getDocument(SyntaxNodeAnalysisContext context) {
-        return context.currentPackage().module(context.moduleId()).document(context.documentId());
-    }
 }
diff --git a/compiler-plugin/src/main/resources/rules.json b/compiler-plugin/src/main/resources/rules.json
@@ -2,6 +2,6 @@
   {
     "id": 1,
     "kind": "VULNERABILITY",
-    "description": "Avoid logging configurable variables"
+    "description": "Potentially-sensitive configurable variables are logged"
   }
 ]