Enable Language Server to invoke scanner with local/global exclusions#80
Open
sdmdg wants to merge 1 commit intoballerina-platform:mainfrom
Open
Enable Language Server to invoke scanner with local/global exclusions#80sdmdg wants to merge 1 commit intoballerina-platform:mainfrom
sdmdg wants to merge 1 commit intoballerina-platform:mainfrom
Conversation
…xclusions Added core support for the Language Server (LS) to directly invoke the security scanner, and expanded the exclusion architecture to support both global and local scopes across both IDE and CLI environments. - Implemented the execution pipeline for the LS to invoke the scanner directly. - Expanded the exclusion system to support both global (legacy) and local scopes, ensuring the new local exclusions are fully compatible with the CLI tool. - Added `SymbolResolver` to dynamically map file lines to AST symbols, enabling stable, symbol-based local exclusions. - Created `ScanTomlWriter` and enhanced `ScanTomlFile` to safely manage these exclusion entries. - Updated `ScanUtils` to filter diagnostics based on the TOML exclusions before reporting back to the client.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive exclusion management system for Ballerina security scans, enabling both symbol-specific and rule-based suppression of scan issues. New data models encapsulate excluded issues and scan results, while a new ScanTool class provides language server entry points for scanning and exclusion management. Supporting utilities handle symbol resolution, exclusion persistence via Scan.toml, and exclusion matching logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ScanTool
participant Project
participant ProjectAnalyzer
participant ScanUtils
participant ScanResult as ScanResult<br/>(Output)
Client->>ScanTool: runScan(projectPath, unsavedContent, buildOptions)
ScanTool->>Project: Load Ballerina project with BuildOptions
ScanTool->>ScanTool: Apply unsaved file changes if provided
ScanTool->>ProjectAnalyzer: Create analyzer with Scan.toml config
ScanTool->>ProjectAnalyzer: execute(analyzer, scanToml, project)
ProjectAnalyzer->>ProjectAnalyzer: Run core rules + external analyzers
ProjectAnalyzer->>ScanUtils: Partition issues: active vs. excluded
ScanUtils->>ScanUtils: Match each issue against exclusions (rule/symbol/line-hash)
ScanUtils-->>ProjectAnalyzer: Return active & excluded issues
ProjectAnalyzer-->>ScanTool: Return ScanResult
ScanTool->>ScanTool: Convert to JSON (activeIssues, excludedIssues)
ScanTool-->>Client: Return JSON response
sequenceDiagram
participant Client
participant ScanTool
participant Project
participant SymbolResolver
participant ScanTomlWriter
participant ScanToml as Scan.toml<br/>(File)
Client->>ScanTool: addExclusion(projectPath, filePath, lineNumber, ruleId, buildOptions)
ScanTool->>Project: Load Ballerina project
ScanTool->>SymbolResolver: resolveSymbol(project, filePath, lineNumber)
SymbolResolver->>SymbolResolver: Parse syntax tree, find enclosing named construct
SymbolResolver-->>ScanTool: Return symbol (or "<module>")
ScanTool->>SymbolResolver: resolveLineHash(project, filePath, lineNumber)
SymbolResolver->>SymbolResolver: Retrieve & hash line content
SymbolResolver-->>ScanTool: Return line hash
ScanTool->>ScanTomlWriter: addExclusion(filePath, ruleId, symbol, lineHash)
ScanTomlWriter->>ScanToml: Read existing Scan.toml (if present)
ScanTomlWriter->>ScanToml: Add/update exclusion entry in table
ScanTomlWriter->>ScanToml: Write updated TOML back to disk
ScanTomlWriter-->>ScanTool: Completion status
ScanTool-->>Client: Return JSON success/error response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
scan-command/src/main/java/io/ballerina/scan/utils/SymbolResolver.java (1)
170-187: Consider using a more stable representation for variable bindings.Line 178 uses
bindingPattern().toString().trim()which may include AST formatting artifacts (extra spaces, newlines) that could make the symbol name fragile for matching purposes. If the source code is reformatted, thetoString()output might change even though the logical binding is the same.Consider extracting just the variable name token if possible, similar to how other node types extract specific name tokens:
if (node instanceof io.ballerina.compiler.syntax.tree.VariableDeclarationNode varNode) { // If the binding pattern is a simple capture, extract just the variable name var bindingPattern = varNode.typedBindingPattern().bindingPattern(); if (bindingPattern instanceof io.ballerina.compiler.syntax.tree.CaptureBindingPatternNode capture) { return capture.variableName().text(); } return bindingPattern.toString().trim(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scan-command/src/main/java/io/ballerina/scan/utils/SymbolResolver.java` around lines 170 - 187, The extractLocalSymbolName method uses bindingPattern().toString().trim() for VariableDeclarationNode which is fragile; update extractLocalSymbolName to inspect varNode.typedBindingPattern().bindingPattern() and, if the binding is a CaptureBindingPatternNode, return its variableName().text(), otherwise fall back to the trimmed toString() (or other safe fallback). Reference the extractLocalSymbolName method, io.ballerina.compiler.syntax.tree.VariableDeclarationNode, typedBindingPattern(), bindingPattern(), io.ballerina.compiler.syntax.tree.CaptureBindingPatternNode, and variableName() when making the change.scan-command/src/main/java/io/ballerina/scan/internal/ScanCmd.java (1)
252-271: Consider caching symbol/hash resolution for performance.For each issue,
SymbolResolver.resolveSymbol()andresolveLineHash()traverse the AST and read file content. If multiple issues occur on the same file and line, this work is repeated. With many issues, this could become a bottleneck.♻️ Suggested optimization using a cache
// Apply symbol-based exclusions Set<ScanTomlFile.Exclusion> exclusions = scanTomlFile.get().getExclusions(); if (!exclusions.isEmpty()) { + Map<String, String> symbolCache = new HashMap<>(); + Map<String, String> hashCache = new HashMap<>(); issues.removeIf(issue -> { if (issue.location() == null || issue.location().lineRange() == null || issue.location().lineRange().fileName() == null) { return false; } String issueFileName = issue.location().lineRange().fileName(); int issueLine = issue.location().lineRange().startLine().line(); - String issueSymbol = SymbolResolver.resolveSymbol(project.get(), issueFileName, issueLine); - String issueLineHash = SymbolResolver.resolveLineHash(project.get(), issueFileName, issueLine); + String cacheKey = issueFileName + ":" + issueLine; + String issueSymbol = symbolCache.computeIfAbsent(cacheKey, + k -> SymbolResolver.resolveSymbol(project.get(), issueFileName, issueLine)); + String issueLineHash = hashCache.computeIfAbsent(cacheKey, + k -> SymbolResolver.resolveLineHash(project.get(), issueFileName, issueLine)); return exclusions.stream().anyMatch(exclusion -> ScanUtils.matchesExclusion(issueFileName, issue.rule().id(), issueSymbol, issueLineHash, exclusion)); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanCmd.java` around lines 252 - 271, The loop repeatedly calls SymbolResolver.resolveSymbol(...) and resolveLineHash(...) for the same file+line causing redundant AST/file reads; introduce a small cache (e.g., Map<String, String> or Map<Pair<String,Integer>,String>) keyed by fileName+":"+line before/in the issues.removeIf predicate in ScanCmd so you compute issueSymbol and issueLineHash only once per unique file+line. Replace direct calls to SymbolResolver.resolveSymbol(project.get(), issueFileName, issueLine) and resolveLineHash(...) with cached lookups (compute-and-store on cache miss), then pass the cached values into ScanUtils.matchesExclusion(...) so behavior stays identical but avoids repeated resolution work.scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java (2)
245-262: Consider acceptingProjectdirectly instead ofObject.The
instanceofcheck suggests this method is designed for reflection-based invocation, but it silently returns an empty result if the wrong type is passed. If LS interop requiresObject, consider logging a warning or throwingIllegalArgumentExceptionfor type mismatches.Proposed fix for type safety
- public static ScanResult runScan(Object projectObj) { - if (projectObj instanceof Project) { - Project project = (Project) projectObj; + public static ScanResult runScan(Object projectObj) { + if (!(projectObj instanceof Project)) { + throw new IllegalArgumentException( + "Expected Project instance but got: " + + (projectObj == null ? "null" : projectObj.getClass().getName())); + } + Project project = (Project) projectObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around lines 245 - 262, The runScan method currently takes Object and silently returns an empty ScanResult on type mismatch; update it to be type-safe by changing the signature to accept Project directly (replace runScan(Object projectObj) with runScan(Project project)) and remove the instanceof cast and fallback; if you must keep an Object signature for interop, instead validate and throw an IllegalArgumentException (or log a clear warning) when the argument is not a Project so callers know the misuse. Ensure references to ProjectAnalyzer, ScanUtils.loadScanTomlConfigurations, and execute(analyzer, ...) are preserved and adjusted to use the Project parameter.
78-92: Consider extracting build options parsing into a helper method.The build options extraction logic is duplicated here and in
addExclusion(lines 129-140). A private helper would reduce duplication and ensure consistent option handling.Example helper
private static BuildOptions extractBuildOptions(Map<String, Boolean> buildOptionsMap) { boolean isOffline = buildOptionsMap != null && Boolean.TRUE.equals(buildOptionsMap.get("offline")); boolean isSticky = buildOptionsMap != null && Boolean.TRUE.equals(buildOptionsMap.get("sticky")); boolean isSkipTests = buildOptionsMap != null && Boolean.TRUE.equals(buildOptionsMap.get("skipTests")); return BuildOptions.builder() .setOffline(isOffline) .setSticky(isSticky) .setSkipTests(isSkipTests) .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around lines 78 - 92, Extract the duplicated build options parsing into a private helper (e.g., private static BuildOptions extractBuildOptions(Map<String,Boolean> buildOptionsMap)) that computes isOffline, isSticky, isSkipTests (and isApplyUnsavedChanges if needed) and returns BuildOptions.builder()...build(); then replace the inline parsing in ScanTool (the block that builds BuildOptions) and in addExclusion with calls to extractBuildOptions(buildOptionsMap) to remove duplication and ensure consistent option handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java`:
- Around line 290-292: The current conditional that checks includeRules and
excludeRules and returns new ScanResult(Collections.emptyList(),
Collections.emptyList()) silently hides a configuration error; replace this
silent early return by throwing a descriptive IllegalArgumentException (or
another appropriate checked exception used in this codebase) when both
includeRules and excludeRules are non-empty, and update the Javadoc of the
enclosing method to document that providing both includeRules and excludeRules
is invalid and will result in that exception; locate the check involving
includeRules.isEmpty() && excludeRules.isEmpty() and change the behavior to
throw the exception with a clear message (and adjust callers if needed) and add
the Javadoc note above the method declaration.
- Around line 402-409: The block that assumes non-null line range parts
(issue.location(), issue.location().lineRange(), LineRange.startLine(),
LineRange.endLine()) can NPE during JSON serialization; update ScanTool.java to
defensively null-check issue.location(), then LineRange range =
issue.location().lineRange() and verify range is not null, and separately verify
range.startLine() and range.endLine() are non-null before calling fileName(),
line(), or offset(); when any piece is null, either omit that JSON property or
write a safe default (e.g., -1 or empty string) so serialization never
dereferences a null reference.
- Around line 270-274: The try/catch around
projectAnalyzer.getExternalAnalyzers() in ScanTool.java silently swallows
failures; update the catch for RuntimeException e to log a warning (using the
existing logger) and/or append a user-visible warning to the scan result so
users know external analyzers failed; specifically, keep the externalAnalyzers
fallback but call logger.warn("Failed to load external analyzers", e) and/or add
a warning entry to the result object so the failure of
projectAnalyzer.getExternalAnalyzers() is surfaced.
- Around line 110-114: The catch-all Exception handler in ScanTool that
currently returns "{\"activeIssues\": [], \"excludedIssues\": []}" is swallowing
real errors; change the catch(Exception e) in the ScanTool method that contains
the RuntimeException/Exception handlers so it does not hide failures—either
rethrow a checked exception or (preferred) return a structured error payload
JSON (for example include fields "error": {"message": e.getMessage(), "type":
e.getClass().getName(), "details": optional stack or cause}) and log the
exception via the existing logger before returning; ensure callers/LS client can
detect the error payload instead of treating it as "no issues".
In `@scan-command/src/main/java/io/ballerina/scan/utils/ScanTomlWriter.java`:
- Around line 370-372: The escapeTomlString method currently only escapes
backslashes and double quotes which can produce malformed TOML when values
include control chars; update escapeTomlString to also escape control characters
required by TOML (at minimum newline, carriage return, tab, backspace, form
feed) by replacing "\n" -> "\\n", "\r" -> "\\r", "\t" -> "\\t", "\b" -> "\\b",
"\f" -> "\\f" (while still escaping backslashes and double quotes) so all those
sequences are written as escaped sequences; locate the escapeTomlString method
and add these replacements (ensuring backslashes are handled first) or use a
StringBuilder/loop to perform canonical escaping for control characters and
quotes.
- Around line 362-364: The fallback in ScanTomlWriter.writeValue that replaces
any Map with "{ }" discards nested data; replace it by serializing the Map into
a TOML inline table: iterate the Map entries, serialize each key and its value
into the same sb as "key = <serializedValue>" joined with ", ", and wrap with "{
" and " }". For nested Maps call the same serialization logic recursively (or
extract a helper like serializeInlineTable(Map) that uses writeValue or a
temporary StringBuilder to obtain the correct TOML representation for each
value) so nested configuration is preserved instead of being dropped.
---
Nitpick comments:
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanCmd.java`:
- Around line 252-271: The loop repeatedly calls
SymbolResolver.resolveSymbol(...) and resolveLineHash(...) for the same
file+line causing redundant AST/file reads; introduce a small cache (e.g.,
Map<String, String> or Map<Pair<String,Integer>,String>) keyed by
fileName+":"+line before/in the issues.removeIf predicate in ScanCmd so you
compute issueSymbol and issueLineHash only once per unique file+line. Replace
direct calls to SymbolResolver.resolveSymbol(project.get(), issueFileName,
issueLine) and resolveLineHash(...) with cached lookups (compute-and-store on
cache miss), then pass the cached values into ScanUtils.matchesExclusion(...) so
behavior stays identical but avoids repeated resolution work.
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java`:
- Around line 245-262: The runScan method currently takes Object and silently
returns an empty ScanResult on type mismatch; update it to be type-safe by
changing the signature to accept Project directly (replace runScan(Object
projectObj) with runScan(Project project)) and remove the instanceof cast and
fallback; if you must keep an Object signature for interop, instead validate and
throw an IllegalArgumentException (or log a clear warning) when the argument is
not a Project so callers know the misuse. Ensure references to ProjectAnalyzer,
ScanUtils.loadScanTomlConfigurations, and execute(analyzer, ...) are preserved
and adjusted to use the Project parameter.
- Around line 78-92: Extract the duplicated build options parsing into a private
helper (e.g., private static BuildOptions
extractBuildOptions(Map<String,Boolean> buildOptionsMap)) that computes
isOffline, isSticky, isSkipTests (and isApplyUnsavedChanges if needed) and
returns BuildOptions.builder()...build(); then replace the inline parsing in
ScanTool (the block that builds BuildOptions) and in addExclusion with calls to
extractBuildOptions(buildOptionsMap) to remove duplication and ensure consistent
option handling.
In `@scan-command/src/main/java/io/ballerina/scan/utils/SymbolResolver.java`:
- Around line 170-187: The extractLocalSymbolName method uses
bindingPattern().toString().trim() for VariableDeclarationNode which is fragile;
update extractLocalSymbolName to inspect
varNode.typedBindingPattern().bindingPattern() and, if the binding is a
CaptureBindingPatternNode, return its variableName().text(), otherwise fall back
to the trimmed toString() (or other safe fallback). Reference the
extractLocalSymbolName method,
io.ballerina.compiler.syntax.tree.VariableDeclarationNode,
typedBindingPattern(), bindingPattern(),
io.ballerina.compiler.syntax.tree.CaptureBindingPatternNode, and variableName()
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 109dd44b-6940-4cc3-a144-e0039bd3d822
📒 Files selected for processing (9)
scan-command/src/main/java/io/ballerina/scan/ExcludedIssue.javascan-command/src/main/java/io/ballerina/scan/ScanResult.javascan-command/src/main/java/io/ballerina/scan/internal/ScanCmd.javascan-command/src/main/java/io/ballerina/scan/internal/ScanTool.javascan-command/src/main/java/io/ballerina/scan/utils/Constants.javascan-command/src/main/java/io/ballerina/scan/utils/ScanTomlFile.javascan-command/src/main/java/io/ballerina/scan/utils/ScanTomlWriter.javascan-command/src/main/java/io/ballerina/scan/utils/ScanUtils.javascan-command/src/main/java/io/ballerina/scan/utils/SymbolResolver.java
Comment on lines
+110
to
+114
| } catch (RuntimeException e) { | ||
| throw e; | ||
| } catch (Exception e) { | ||
| return "{\"activeIssues\": [], \"excludedIssues\": []}"; | ||
| } |
There was a problem hiding this comment.
Silent exception swallowing masks errors from the Language Server.
Catching all non-runtime exceptions and returning an empty JSON hides actionable errors (e.g., IOException from loading the project, invalid TOML configuration). The LS client has no way to distinguish "no issues found" from "scan failed."
Consider returning an error payload with diagnostic information:
Proposed fix
- } catch (RuntimeException e) {
- throw e;
- } catch (Exception e) {
- return "{\"activeIssues\": [], \"excludedIssues\": []}";
- }
+ } catch (Exception e) {
+ JsonObject errorResult = new JsonObject();
+ errorResult.add("activeIssues", new JsonArray());
+ errorResult.add("excludedIssues", new JsonArray());
+ errorResult.addProperty("error", e.getClass().getSimpleName() + ": " + e.getMessage());
+ return GSON.toJson(errorResult);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (RuntimeException e) { | |
| throw e; | |
| } catch (Exception e) { | |
| return "{\"activeIssues\": [], \"excludedIssues\": []}"; | |
| } | |
| } catch (RuntimeException e) { | |
| throw e; | |
| } catch (Exception e) { | |
| JsonObject errorResult = new JsonObject(); | |
| errorResult.add("activeIssues", new JsonArray()); | |
| errorResult.add("excludedIssues", new JsonArray()); | |
| errorResult.addProperty("error", e.getClass().getSimpleName() + ": " + e.getMessage()); | |
| return GSON.toJson(errorResult); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around
lines 110 - 114, The catch-all Exception handler in ScanTool that currently
returns "{\"activeIssues\": [], \"excludedIssues\": []}" is swallowing real
errors; change the catch(Exception e) in the ScanTool method that contains the
RuntimeException/Exception handlers so it does not hide failures—either rethrow
a checked exception or (preferred) return a structured error payload JSON (for
example include fields "error": {"message": e.getMessage(), "type":
e.getClass().getName(), "details": optional stack or cause}) and log the
exception via the existing logger before returning; ensure callers/LS client can
detect the error payload instead of treating it as "no issues".
Comment on lines
+270
to
+274
| try { | ||
| externalAnalyzers = projectAnalyzer.getExternalAnalyzers(); | ||
| } catch (RuntimeException e) { | ||
| // Log error if needed | ||
| } |
There was a problem hiding this comment.
Silently swallowing external analyzer failures hides configuration issues.
If getExternalAnalyzers() throws, the scan continues without external rules, giving no indication to the user. Consider logging the error or including a warning in the result.
Proposed fix
try {
externalAnalyzers = projectAnalyzer.getExternalAnalyzers();
} catch (RuntimeException e) {
- // Log error if needed
+ // External analyzers failed to load - scan continues with core rules only
+ System.err.println("Warning: Failed to load external analyzers: " + e.getMessage());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around
lines 270 - 274, The try/catch around projectAnalyzer.getExternalAnalyzers() in
ScanTool.java silently swallows failures; update the catch for RuntimeException
e to log a warning (using the existing logger) and/or append a user-visible
warning to the scan result so users know external analyzers failed;
specifically, keep the externalAnalyzers fallback but call logger.warn("Failed
to load external analyzers", e) and/or add a warning entry to the result object
so the failure of projectAnalyzer.getExternalAnalyzers() is surfaced.
Comment on lines
+290
to
+292
| if (!includeRules.isEmpty() && !excludeRules.isEmpty()) { | ||
| return new ScanResult(Collections.emptyList(), Collections.emptyList()); | ||
| } |
There was a problem hiding this comment.
Early return when both include and exclude rules are present is undocumented.
This silently returns empty results when both includeRules and excludeRules are non-empty. If this is a validation error (conflicting configuration), consider returning an error or warning rather than an empty scan result. The behavior should be documented in the method's Javadoc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around
lines 290 - 292, The current conditional that checks includeRules and
excludeRules and returns new ScanResult(Collections.emptyList(),
Collections.emptyList()) silently hides a configuration error; replace this
silent early return by throwing a descriptive IllegalArgumentException (or
another appropriate checked exception used in this codebase) when both
includeRules and excludeRules are non-empty, and update the Javadoc of the
enclosing method to document that providing both includeRules and excludeRules
is invalid and will result in that exception; locate the check involving
includeRules.isEmpty() && excludeRules.isEmpty() and change the behavior to
throw the exception with a clear message (and adjust callers if needed) and add
the Javadoc note above the method declaration.
Comment on lines
+402
to
+409
| if (issue.location() != null) { | ||
| LineRange range = issue.location().lineRange(); | ||
| obj.addProperty("filePath", range.fileName()); | ||
| obj.addProperty("startLine", range.startLine().line()); | ||
| obj.addProperty("startColumn", range.startLine().offset()); | ||
| obj.addProperty("endLine", range.endLine().line()); | ||
| obj.addProperty("endColumn", range.endLine().offset()); | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when accessing line range components.
After checking issue.location() != null, the code accesses lineRange(), startLine(), and endLine() without null checks. If any of these return null, this will throw an NPE during JSON serialization.
Proposed fix
if (issue.location() != null) {
LineRange range = issue.location().lineRange();
- obj.addProperty("filePath", range.fileName());
- obj.addProperty("startLine", range.startLine().line());
- obj.addProperty("startColumn", range.startLine().offset());
- obj.addProperty("endLine", range.endLine().line());
- obj.addProperty("endColumn", range.endLine().offset());
+ if (range != null) {
+ obj.addProperty("filePath", range.fileName());
+ if (range.startLine() != null) {
+ obj.addProperty("startLine", range.startLine().line());
+ obj.addProperty("startColumn", range.startLine().offset());
+ }
+ if (range.endLine() != null) {
+ obj.addProperty("endLine", range.endLine().line());
+ obj.addProperty("endColumn", range.endLine().offset());
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (issue.location() != null) { | |
| LineRange range = issue.location().lineRange(); | |
| obj.addProperty("filePath", range.fileName()); | |
| obj.addProperty("startLine", range.startLine().line()); | |
| obj.addProperty("startColumn", range.startLine().offset()); | |
| obj.addProperty("endLine", range.endLine().line()); | |
| obj.addProperty("endColumn", range.endLine().offset()); | |
| } | |
| if (issue.location() != null) { | |
| LineRange range = issue.location().lineRange(); | |
| if (range != null) { | |
| obj.addProperty("filePath", range.fileName()); | |
| if (range.startLine() != null) { | |
| obj.addProperty("startLine", range.startLine().line()); | |
| obj.addProperty("startColumn", range.startLine().offset()); | |
| } | |
| if (range.endLine() != null) { | |
| obj.addProperty("endLine", range.endLine().line()); | |
| obj.addProperty("endColumn", range.endLine().offset()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/internal/ScanTool.java` around
lines 402 - 409, The block that assumes non-null line range parts
(issue.location(), issue.location().lineRange(), LineRange.startLine(),
LineRange.endLine()) can NPE during JSON serialization; update ScanTool.java to
defensively null-check issue.location(), then LineRange range =
issue.location().lineRange() and verify range is not null, and separately verify
range.startLine() and range.endLine() are non-null before calling fileName(),
line(), or offset(); when any piece is null, either omit that JSON property or
write a safe default (e.g., -1 or empty string) so serialization never
dereferences a null reference.
Comment on lines
+362
to
+364
| } else if (value instanceof Map) { | ||
| // fallback to empty | ||
| sb.append("{ }"); |
There was a problem hiding this comment.
Inline table fallback discards nested map data.
When writeValue encounters a Map value (e.g., nested configuration), it outputs "{ }" regardless of the map's contents. This could silently discard data if a platform or analyzer has nested configuration beyond simple key-value pairs.
🛡️ Proposed fix to serialize inline tables properly
} else if (value instanceof Map) {
- // fallback to empty
- sb.append("{ }");
+ `@SuppressWarnings`("unchecked")
+ Map<String, Object> map = (Map<String, Object>) value;
+ if (map.isEmpty()) {
+ sb.append("{ }");
+ } else {
+ sb.append("{ ");
+ boolean first = true;
+ for (Map.Entry<String, Object> entry : map.entrySet()) {
+ if (!first) {
+ sb.append(", ");
+ }
+ sb.append(entry.getKey()).append(" = ");
+ writeValue(sb, entry.getValue());
+ first = false;
+ }
+ sb.append(" }");
+ }
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/utils/ScanTomlWriter.java`
around lines 362 - 364, The fallback in ScanTomlWriter.writeValue that replaces
any Map with "{ }" discards nested data; replace it by serializing the Map into
a TOML inline table: iterate the Map entries, serialize each key and its value
into the same sb as "key = <serializedValue>" joined with ", ", and wrap with "{
" and " }". For nested Maps call the same serialization logic recursively (or
extract a helper like serializeInlineTable(Map) that uses writeValue or a
temporary StringBuilder to obtain the correct TOML representation for each
value) so nested configuration is preserved instead of being dropped.
Comment on lines
+370
to
+372
| private static String escapeTomlString(String value) { | ||
| return value.replace("\\", "\\\\").replace("\"", "\\\""); | ||
| } |
There was a problem hiding this comment.
Incomplete TOML string escaping may cause malformed output.
The escapeTomlString method only escapes backslashes and double quotes, but TOML strings can also contain newlines, tabs, and other control characters that need escaping. If a symbol name or file path contains these characters, the generated TOML will be malformed.
🛡️ Proposed fix for comprehensive escaping
private static String escapeTomlString(String value) {
- return value.replace("\\", "\\\\").replace("\"", "\\\"");
+ return value
+ .replace("\\", "\\\\")
+ .replace("\"", "\\\"")
+ .replace("\n", "\\n")
+ .replace("\r", "\\r")
+ .replace("\t", "\\t")
+ .replace("\b", "\\b")
+ .replace("\f", "\\f");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scan-command/src/main/java/io/ballerina/scan/utils/ScanTomlWriter.java`
around lines 370 - 372, The escapeTomlString method currently only escapes
backslashes and double quotes which can produce malformed TOML when values
include control chars; update escapeTomlString to also escape control characters
required by TOML (at minimum newline, carriage return, tab, backspace, form
feed) by replacing "\n" -> "\\n", "\r" -> "\\r", "\t" -> "\\t", "\b" -> "\\b",
"\f" -> "\\f" (while still escaping backslashes and double quotes) so all those
sequences are written as escaped sequences; locate the escapeTomlString method
and add these replacements (ensuring backslashes are handled first) or use a
StringBuilder/loop to perform canonical escaping for control characters and
quotes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR introduces the ability to invoke the Ballerina security scanner directly from the Language Server (LS) and provides a multi-scope diagnostic exclusion system. Previously, developers could not easily suppress specific security warnings in a persistent manner within the IDE. By introducing symbol-based exclusions (via
Scan.toml), developers can now mute specific diagnostics without those rules breaking when lines of code are subsequently added or removed.Approach
The solution integrates the scanner execution into the LS pipeline and builds a new exclusion architecture that is fully compatible with both the IDE and CLI environments:
SymbolResolverto dynamically resolve the enclosing AST symbol (and line hash) for a given file and line number. This ensures exclusions are attached to logical blocks (like functions or services) rather than volatile line numbers.ScanTomlWriterand enhancedScanTomlFileto safely read and write exclusion entries in theScan.tomlconfiguration file.Sample Configuration
Here is an example of how the updated
Scan.tomlhandles both global exclusions and the newly introduced symbol-based local exclusions:Check List
Summary
This pull request introduces Language Server integration capabilities for the security scanner and establishes a multi-scope exclusion system to manage diagnostics at both rule and symbol levels.
Key Changes
Language Server Integration: Added
ScanToolas a new entry point that enables the Language Server to invoke the scanner, handle unsaved editor changes, and return diagnostic results in JSON format. The implementation supports loading Ballerina projects with configurable build options and manages the complete scanning workflow.Symbol-Based Exclusion System: Introduced
SymbolResolverto identify enclosing AST symbols (functions, services, classes, etc.) and compute line content hashes for files and line numbers. This allows exclusions to be anchored to logical code constructs rather than absolute line numbers, ensuring suppressions remain stable when code lines change.Exclusion Persistence and Management: Created
ScanTomlWriterto safely read, write, and update exclusion entries inScan.toml, including support for both symbol-based (local) exclusions and rule-based (global) exclusions. Added new data models (ScanResult,ExcludedIssue, andExclusionrecord) to represent scan output and suppressed issues.Diagnostic Filtering: Updated
ScanCmdandScanUtilsto filter diagnostics based on configured exclusions. The filtering logic supports both legacy global rule exclusions and new symbol-based local exclusions, enabling comprehensive control over reported issues.Result Structures: Introduced
ScanResultto encapsulate scan output, separating active issues from excluded ones, andExcludedIssueto track suppressed issues along with their metadata (rule ID, file path, symbol, line hash, and exclusion scope).Impact
These changes enable the Language Server to provide scanner-based diagnostics to IDE clients while allowing users to suppress specific issues at both the rule and symbol levels. Exclusions are persisted in
Scan.tomland remain stable across code changes, improving the user experience for managing false positives and known issues.