[PPSC-718] feat(scan): add inline armis:ignore comment suppression#170
Merged
Conversation
|
| Severity | Count |
|---|---|
| 🔵 LOW | 1 |
Total: 1
View all 1 findings
🔵 LOW (1)
CWE-253 - In the `ApplyInlineSuppression` function the file is opened with `os.Open` and later closed with `defer f.Close()`
Location: internal/scan/repo/inline.go:81
In the ApplyInlineSuppression function the file is opened with os.Open and later closed with defer f.Close(). The Close method returns an error, but the code deliberately ignores that return value (the comment //nolint:errcheck tells the linter not to warn). Because the error is not checked, any problem that occurs while closing the file – for example, a failure to write buffered data or a permission issue – will be hidden. This means the program could think the file was closed successfully when it was not, potentially leading to lost data or other unexpected behavior. The issue is not about malicious input reaching the code; it is about the missing check on the function’s return value. Adding a proper error check after f.Close() would make the problem go away.
CWEs: CWE-253: Incorrect Check of Function Return Value
Add support for suppressing findings via inline code comments. Developers can now annotate source lines with `armis:ignore` directives to suppress specific findings at the line level, complementing the existing .armisignore file-level suppression. Key features: - Supports all major comment syntaxes (// # -- <!-- /* ;) - Parameters in any order: category, rule, cwe, severity, reason - AND logic: all specified params must match for suppression - Rejects directives inside string literals to prevent bypass - File content cached for performance across multiple findings - SARIF output uses "inSource" kind for inline vs "external" for .armisignore Security hardening: - Path traversal protection via SafeJoinPath - 10MB file size limit prevents memory exhaustion - Quote-aware comment prefix detection prevents false positives from URLs like "http://armis:ignore@..." or string literals
yiftach-armis
force-pushed
the
feat/PPSC-718-inline-suppression
branch
from
1a8c2c1 to
dfce618
Compare
Test Coverage Reporttotal: (statements) 77.3% Coverage by function |
There was a problem hiding this comment.
Pull request overview
Adds inline armis:ignore comment directives to suppress individual scan findings at the source line level, complementing existing .armisignore-based suppression and surfacing suppression provenance in human + SARIF outputs.
Changes:
- Apply inline suppression after
.armisignoresuppression and recompute scan summary when additional suppressions occur. - Introduce inline directive parsing/matching with scope parameters (category/rule/CWE/severity/reason) plus unit tests.
- Differentiate suppression sources (
armisignorevsinline) in SARIF (kind: externalvsinSource) and improve human output suppression summaries/labels.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/scan/repo/repo.go | Runs inline suppression after .armisignore suppression and recomputes summary. |
| internal/scan/repo/matcher.go | Adds suppression source metadata for .armisignore suppressions. |
| internal/scan/repo/inline.go | Implements inline directive parsing, file scanning, and suppression application. |
| internal/scan/repo/inline_test.go | Adds unit tests for directive parsing/matching and suppression application. |
| internal/output/output.go | Introduces shared constant for inline suppression source detection in output. |
| internal/output/sarif.go | Emits SARIF suppression kind/justification based on suppression source. |
| internal/output/sarif_test.go | Updates/adds SARIF tests for external vs in-source suppressions. |
| internal/output/human.go | Improves human output to summarize suppressions by source and label inline-suppressed findings. |
| internal/output/human_test.go | Updates human formatter tests to account for source-aware suppression summaries. |
| internal/model/finding.go | Extends suppression metadata model to include suppression Source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+259
to
+263
| continue | ||
| } | ||
| key := strings.ToLower(token[:colonIdx]) | ||
| value := token[colonIdx+1:] | ||
|
|
Comment on lines
+82
to
+86
| scanner := bufio.NewScanner(f) | ||
| for scanner.Scan() { | ||
| entry.lines = append(entry.lines, scanner.Text()) | ||
| } | ||
| if scanner.Err() == nil { |
Comment on lines
+248
to
+252
| // Try to match key:value | ||
| parts := strings.SplitN(remainder, " ", 2) | ||
| token := parts[0] | ||
| if len(parts) > 1 { | ||
| remainder = parts[1] |
Comment on lines
+185
to
+189
| // findCommentStart finds the first occurrence of prefix that is not inside a | ||
| // quoted string. Returns -1 if not found or only found inside quotes. | ||
| func findCommentStart(line, prefix string) int { | ||
| inSingle := false | ||
| inDouble := false |
Comment on lines
+36
to
+40
| ".js": {"//"}, ".ts": {"//"}, ".jsx": {"//"}, ".tsx": {"//"}, ".java": {"//"}, | ||
| ".c": {"//"}, ".h": {"//"}, ".cpp": {"//"}, ".hpp": {"//"}, ".cs": {"//"}, | ||
| ".go": {"//"}, ".rs": {"//"}, ".swift": {"//"}, ".kt": {"//"}, ".scala": {"//"}, | ||
| ".dart": {"//"}, ".groovy": {"//"}, | ||
|
|
- Add 1MB scanner buffer to handle minified files with long lines - Track backtick quotes in findCommentStart to prevent template literal bypass - Replace SplitN with strings.Fields for robust whitespace handling - Reject directives with only unrecognized keys to catch typos - Add block comment support (/*...*/) to C-family languages
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err != nil { | ||
| return entry | ||
| } | ||
| defer f.Close() //nolint:errcheck |
Collaborator
Author
There was a problem hiding this comment.
False positive — this file is opened read-only (os.Open = O_RDONLY). There is no buffered write data that could be lost on close. For read-only file descriptors, Close() errors are benign (the kernel simply releases the fd), and ignoring the return value is idiomatic Go. The //nolint:errcheck annotation documents this deliberate choice.
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
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.
Related Issue
Type of Change
Problem
Developers need a way to suppress individual findings at the source line level, similar to how
// nolintor# nosecwork in other tools. The existing.armisignorefile only supports project-wide suppression by severity, category, CWE, or rule ID.Solution
Add inline
armis:ignorecomment directives that suppress findings on the annotated line (or the line below). Supports all major comment syntaxes (//,#,--,<!--,/*,;), accepts scope parameters (category:,rule:,cwe:,severity:,reason:) in any order with AND logic, and integrates with both human and SARIF output formatters.Security hardening includes quote-aware parsing to reject directives inside string literals (prevents bypass via URLs like
"http://armis:ignore@..."), path traversal protection viaSafeJoinPath, and a 10MB file size cap.Testing
Automated Tests
Manual Testing
Verified via
go test ./internal/scan/repo/... ./internal/output/...— all pass. Linter passes for changed files.Reviewer Notes
.armisignoresuppression, only on non-suppressed findingskind: "inSource"for inline andkind: "external"for.armisignoreto differentiate suppression sourcesChecklist