[PPSC-720] feat(scan): add client-side finding suppression via .armisignore#162
Open
yiftach-armis wants to merge 4 commits intomainfrom
Open
[PPSC-720] feat(scan): add client-side finding suppression via .armisignore#162yiftach-armis wants to merge 4 commits intomainfrom
yiftach-armis wants to merge 4 commits intomainfrom
Conversation
…ignore Wire the suppression directive parser from PPSC-705 into the scan results pipeline. Findings matching severity/category/cwe directives are marked as suppressed and excluded from --fail-on evaluation, human/JUnit output, while SARIF and JSON include them with proper suppression metadata. - Add Suppressed/SuppressionInfo fields to model.Finding, Suppressed to Summary - Create matcher (MatchFinding, ApplySuppression) with priority: severity > category > CWE > rule - Hoist LoadArmisIgnore above scan mode branch so suppression applies to all modes - Add --show-suppressed flag on scanRepoCmd - Update ShouldFail to skip suppressed findings - Human formatter: suppression count in brief/summary, CRITICAL warning, [SUPPRESSED] labels - SARIF: suppressions[] array with kind "inSource" per SARIF 2.1.0 - JUnit: exclude suppressed from test cases - JSON: suppressed fields flow via omitempty struct tags (zero code changes) - Fix DeriveFindingType to classify LICENSE_COMPLIANCE_RISK as LICENSE - Add 30+ test cases across matcher, output, and finding_type packages
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 2 |
Total: 2
View all 2 findings
🟠 HIGH (2)
CWE-770 - The `stripMarkdown` function takes a piece of text that may contain markdown formatting and runs a series of regular‑expression replacements to turn it into plain text
Location: internal/output/sarif.go:195
The stripMarkdown function takes a piece of text that may contain markdown formatting and runs a series of regular‑expression replacements to turn it into plain text. It does this without checking how large the input is or how complex the markdown might be. If someone (or some automated process) supplies a very large markdown string, the function will keep allocating more memory and CPU time to process it. This can cause the program to slow down or run out of memory, which is the problem described by CWE‑770. In this code the function is used when building the SARIF report, converting the markdown description of a finding into plain text for the Help.Text field. Because there is no limit on the size of the markdown that can be passed in, an attacker who can influence that description could cause excessive resource use. No special checks or limits are present, so the risky operation is reachable from the data that fills the finding description. Adding size checks or processing the markdown in smaller chunks would help prevent this kind of exhaustion.
CWEs: CWE-770: Allocation of Resources Without Limits or Throttling
CWE-22 - Broken Access Control (CWE-22
Location: internal/scan/repo/ignore.go:80
Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')): The function LoadArmisIgnore builds a file path by appending ".armisignore" to the repoRoot value it receives. Because repoRoot comes from outside the function and is not checked or cleaned, an attacker who can set this value could point the code to any location on the file system. The program then walks that path and opens files it finds, which means the attacker could cause the program to read files outside the intended repository directory. No part of the code removes dangerous parts like ".." or otherwise restricts the path, so the risky operation can be reached with attacker‑controlled input. Because this code is part of a command‑line tool rather than a network service, the exposure is limited to the tool’s execution context, but the vulnerability still exists and can be exploited if the tool is run with a malicious repoRoot argument.
CWEs: CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Test Coverage Reporttotal: (statements) 76.9% Coverage by function |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end client-side consumption of .armisignore suppression directives for repo scans by marking matched findings as suppressed, excluding them from --fail-on/human/JUnit output by default, and emitting SARIF suppression metadata for GitHub Code Scanning.
Changes:
- Load
.armisignoresuppression directives during repo scans and apply them post-fetch to mark findings as suppressed, then recompute summary totals/counts. - Exclude suppressed findings from failure evaluation (
--fail-on) and from JUnit/human output unless explicitly shown; add--show-suppressedflag. - Extend outputs/models to represent suppression state (
suppressed,suppression_info, SARIFsuppressions[]) and improve finding type derivation for license compliance.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/scan/repo/repo.go | Loads .armisignore earlier and applies suppression + summary recompute after fetching findings. |
| internal/scan/repo/matcher.go | Implements directive-to-finding matching and in-place suppression mutation + summary recompute helper. |
| internal/scan/repo/matcher_test.go | Unit tests for matching, CWE matching, suppression application, and helpers. |
| internal/scan/finding_type.go | Classifies LICENSE_COMPLIANCE_RISK/LICENSE as FindingTypeLicense. |
| internal/scan/finding_type_test.go | Adds tests for license category classification. |
| internal/output/sarif.go | Adds SARIF suppressions[] emission for suppressed findings. |
| internal/output/sarif_test.go | Adds SARIF tests validating suppressions[] behavior. |
| internal/output/output.go | Excludes suppressed findings from ShouldFail; adds FilterActiveFindings. |
| internal/output/output_test.go | Adds tests for suppressed-skipping ShouldFail and filtering helper. |
| internal/output/junit.go | Filters suppressed findings out of JUnit suite stats/cases. |
| internal/output/junit_test.go | Adds tests ensuring suppressed findings are excluded from JUnit output. |
| internal/output/json_test.go | Adds tests for JSON suppression fields serialization behavior. |
| internal/output/human.go | Hides suppressed findings by default; adds suppressed counts to summary/status; warns on suppressed CRITICAL. |
| internal/output/human_test.go | Adds tests for suppressed messaging, warning behavior, and hiding suppressed findings. |
| internal/model/finding.go | Adds Suppressed, SuppressionInfo, and Summary.Suppressed to the result model. |
| internal/cmd/scan_repo.go | Adds --show-suppressed flag wiring into output options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+105
to
+110
| // Load .armisignore: suppression config applies in ALL scan modes, | ||
| // IgnoreMatcher only used in full-scan mode for tar filtering. | ||
| ignoreMatcher, suppressionConfig, err := LoadArmisIgnore(absPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load ignore patterns: %w", err) | ||
| } |
Comment on lines
+10
to
+70
| var cweIntPattern = regexp.MustCompile(`^CWE-(\d+)`) | ||
|
|
||
| // categoryToFindingType maps .armisignore category directive values to FindingType. | ||
| var categoryToFindingType = map[string]model.FindingType{ | ||
| "sast": model.FindingTypeVulnerability, | ||
| "secrets": model.FindingTypeSecret, | ||
| "iac": model.FindingTypeMisconfig, | ||
| "sca": model.FindingTypeSCA, | ||
| "license": model.FindingTypeLicense, | ||
| } | ||
|
|
||
| // MatchResult describes the first directive that matched a finding. | ||
| type MatchResult struct { | ||
| Matched bool | ||
| Directive SuppressionDirective | ||
| } | ||
|
|
||
| // MatchFinding returns the first matching suppression directive for a finding. | ||
| func MatchFinding(finding model.Finding, config *SuppressionConfig) MatchResult { | ||
| if config == nil || config.IsEmpty() { | ||
| return MatchResult{} | ||
| } | ||
|
|
||
| for _, d := range config.Severities { | ||
| if strings.EqualFold(d.Value, string(finding.Severity)) { | ||
| return MatchResult{Matched: true, Directive: d} | ||
| } | ||
| } | ||
|
|
||
| for _, d := range config.Categories { | ||
| expected, ok := categoryToFindingType[d.Value] | ||
| if ok && finding.Type == expected { | ||
| return MatchResult{Matched: true, Directive: d} | ||
| } | ||
| } | ||
|
|
||
| for _, d := range config.CWEs { | ||
| if cweMatches(finding.CWEs, d.Value) { | ||
| return MatchResult{Matched: true, Directive: d} | ||
| } | ||
| } | ||
|
|
||
| // rule: directives are no-ops until scanner_rule_id is available (ADR-0007) | ||
|
|
||
| return MatchResult{} | ||
| } | ||
|
|
||
| // cweMatches checks if any of the finding's CWE identifiers match the directive value. | ||
| // The directive value is a bare integer string (e.g. "89"). | ||
| // Finding CWEs may be in the format "CWE-89: Improper..." or just "89". | ||
| func cweMatches(findingCWEs []string, directiveValue string) bool { | ||
| for _, cwe := range findingCWEs { | ||
| if matches := cweIntPattern.FindStringSubmatch(cwe); len(matches) == 2 { | ||
| if matches[1] == directiveValue { | ||
| return true | ||
| } | ||
| } else if strings.TrimSpace(cwe) == directiveValue { | ||
| return true | ||
| } | ||
| } | ||
| return false |
Comment on lines
+876
to
+881
| // Show suppression label if finding is suppressed and --show-suppressed is active | ||
| if finding.Suppressed { | ||
| suppLabel := s.MutedText.Render("[SUPPRESSED]") | ||
| var reason string | ||
| if finding.SuppressionInfo != nil { | ||
| reason = fmt.Sprintf("%s:%s", finding.SuppressionInfo.Type, finding.SuppressionInfo.Value) |
- Add LoadSuppressionConfig for targeted mode to avoid unnecessary tree walk - Make CWE regex case-insensitive and whitespace-tolerant - Fix misleading comment in renderFinding
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Satisfies static analysis path-traversal check on repoRoot parameter.
Comment on lines
+347
to
+360
| // CRITICAL suppression warning | ||
| if result.Summary.Suppressed > 0 { | ||
| criticalSuppressed := 0 | ||
| for _, f := range result.Findings { | ||
| if f.Suppressed && f.Severity == model.SeverityCritical { | ||
| criticalSuppressed++ | ||
| } | ||
| } | ||
| if criticalSuppressed > 0 { | ||
| ew.write("\n") | ||
| ew.write("%s\n", s.WarningText.Render( | ||
| fmt.Sprintf("WARNING: %d CRITICAL %s suppressed by .armisignore", | ||
| criticalSuppressed, pluralize("finding", criticalSuppressed)))) | ||
| } |
| // Full directory scanning mode — walk tree for both ignore patterns and directives. | ||
| ignoreMatcher, suppCfg, loadErr := LoadArmisIgnore(absPath) | ||
| if loadErr != nil { | ||
| return nil, fmt.Errorf("failed to load ignore patterns: %w", loadErr) |
Comment on lines
+123
to
+127
| // filterActiveFindings returns only non-suppressed findings. | ||
| func filterActiveFindings(findings []model.Finding) []model.Finding { | ||
| active := make([]model.Finding, 0, len(findings)) | ||
| for _, f := range findings { | ||
| if !f.Suppressed { |
- Move CRITICAL suppression warning to stderr via cli.PrintWarningf (#1) - Add early-exit in calculateDirSize when MaxRepoSize exceeded (#2) - Remove duplicate filterActiveFindings from matcher.go (#3) - Add filepath.IsAbs validation in LoadSuppressionConfig (#4) - Update error message for LoadArmisIgnore to be accurate (#5)
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
The
.armisignoreparser from PPSC-705 can parse suppression directives (severity:,category:,cwe:,rule:) but no downstream code consumes them. Findings matching these directives still appear in output and trigger--fail-onexits, making the suppression system non-functional.Solution
Wire the suppression directive parser into the scan results pipeline as a client-side post-filter. After findings are fetched from Armis Cloud, they're matched against suppression directives and marked as suppressed. Suppressed findings are excluded from
--fail-onevaluation, hidden from human/JUnit output (unless--show-suppressed), and annotated with SARIF 2.1.0suppressions[]metadata for GitHub Code Scanning integration.Key design choices:
Suppressed=true+SuppressionInforather than removed, so all formatters can decide how to present themscanner_rule_idlands per ADR-0007)finding.Type(deterministic, set byDeriveFindingType) rather than the freeformFindingCategoryAPI fieldSummary.Totalreflects only active findings whileSummary.Suppressedtracks the suppressed countTesting
Automated Tests
Manual Testing
.armisignorecontainingseverity:LOW,severity:INFO,cwe:79,category:secrets--show-suppresseddisplays all findings with[SUPPRESSED]labels--fail-on LOWreturns exit 0 when LOW findings are suppressedsuppressionsarray withkind: "inSource"and justificationsuppressed: true+suppression_infoon suppressed findingsReviewer Notes
rule:directive matching is intentionally a no-op — blocked onscanner_rule_idfrom ADR-0007suppressions[]will cause GitHub Code Scanning to auto-dismiss matching findings — this is intended behaviorLoadArmisIgnorecall was hoisted above the if/else branch so suppression config is loaded in both full-scan and targeted-file modesDeriveFindingTypeto classifyLICENSE_COMPLIANCE_RISKasLICENSE(D9 from plan review), enablingcategory:licenseto work end-to-endChecklist