diff --git a/CHANGELOG.md b/CHANGELOG.md index 85403ed..e2e8dd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and output says "lookup did not run" rather than "no vulnerabilities found" whenever that is the truth. - Exit codes: `0` completed (regardless of findings), `1` scan failed or partial, `2` usage - error. Findings alone never fail a build. + error, `3` `--fail-on` threshold met. Findings alone never fail a build. +- `--fail-on ` gates a build on unsuppressed findings, checking scan completeness + first so it can never pass a scan that failed to look anything up. +- A triage file, `.icebergsca.toml`, records findings that have been assessed and accepted, so + gating is safe to leave switched on rather than being disabled the first week an unfixable + transitive advisory appears. Each entry requires a reason, matches exactly on advisory (or + alias) and package, and carries an expiry that defaults to 90 days. An accepted finding stays + in the report marked `accepted`, is counted by `summary.suppressed`, appears in SARIF as + dismissed rather than fixed, and returns when the entry expires. Rules that match nothing, or + have expired, are reported as warnings. +- JSON schema `1.1`: adds a top-level `suppressed` audit trail, `summary.suppressed`, and + `findings[].suppression`. Additive only — `summary.findings` and `by_severity` now count + active findings, which is unchanged for any scan with no ignore file. - Maven graphs are marked approximate — they are reconstructed, not read, and we never shell out to `mvn`. Each module of a multi-module build resolves against its own POM, so module boundaries hold: one module's nearest-wins choice cannot decide another's versions, and each diff --git a/CLAUDE.md b/CLAUDE.md index a0ab812..c7650e4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,6 +146,27 @@ schema validation in `tests/test_report_formats.py` against the official schemas **Exit code 2 is usage, 1 is scan failure** — the reverse of the original plan, because Click reserves 2 for usage errors and remapping it means overriding framework internals for nothing. +`--fail-on` gets its own code 3 rather than reusing 1: a pipeline that cannot tell "the scanner +broke" from "the scanner found something" ends up treating both as noise, which is the same +failure `|| true` represents. + +**Suppression and gating shipped together, deliberately.** `--fail-on` alone turns a single +unfixable transitive advisory into a build that is red every week forever, and a job that is +always red gets `|| true` or gets deleted. The ignore file is what makes the gate survivable. +The safeguards in `core/triage.py` all follow from it being the one feature whose job is to +remove findings: a suppressed finding stays in `ScanReport.findings` marked rather than being +filtered out (so `table.py`'s `if not report.findings` branch cannot print "no known +vulnerabilities" for an all-suppressed scan — `test_a_fully_suppressed_report_never_reads_as_clean` +guards exactly that), `reason` is mandatory, expiry defaults rather than being optional, matching +is exact on advisory-or-alias plus package, and a rule matching nothing becomes a warning. + +**`parse_ignore_file` is strict where every other parser is forgiving.** Ecosystem parsers skip +what they cannot read, because a third-party manifest is not ours to police. The ignore file is +the user's own and decides which vulnerabilities they stop seeing, so an unknown key is a +`ConfigError` rather than a silent skip. + +**`SeverityLevel` is a `StrEnum`, so `<` compares strings** — `HIGH < LOW` is True. Use +`at_or_above()` or `.rank`; never compare members directly. ## Testing @@ -168,7 +189,10 @@ and versions. This has caught two real bugs (a missing `coverage[toml]` extra tr ## Known gaps -- `--fail-on ` and an ignore/triage file — the obvious next feature. +- The ignore file suppresses; it does not yet express "not exploitable" in VEX terms. + `cyclonedx.py` emits `analysis.detail` only, because `state` and `justification` are closed + enums of security claims a free-text reason cannot back. An optional validated `state` key on + the entry is the obvious next step. - No SPDX output; CycloneDX only. - No SBOM *ingest*. - npm/yarn v1 lockfiles record no scope, so dev transitives are reported as runtime unless a diff --git a/README.md b/README.md index 5d12958..ba50f14 100644 --- a/README.md +++ b/README.md @@ -80,9 +80,11 @@ follows from this: | `0` | Scan completed. Vulnerabilities may have been found — that is not an error | | `1` | Scan failed, or completed only partially — some files or packages went unchecked | | `2` | Usage error: bad flag, unknown format (Click's reserved code) | +| `3` | Only with `--fail-on`: the scan completed and found something at or above the threshold | -Severity-based CI gating (`--fail-on`) is planned; the report model already carries everything -needed for it. +`--fail-on ` gates a build, and `.icebergsca.toml` records the findings you have +assessed and accepted so the gate is safe to leave switched on. An accepted finding stays in +the report, marked and counted separately — it is never silently dropped. ## Ecosystem support diff --git a/src/icebergsca/.agents/skills/icebergsca/SKILL.md b/src/icebergsca/.agents/skills/icebergsca/SKILL.md index 5674f93..7f7b36c 100644 --- a/src/icebergsca/.agents/skills/icebergsca/SKILL.md +++ b/src/icebergsca/.agents/skills/icebergsca/SKILL.md @@ -78,6 +78,7 @@ icebergsca scan . --format json | jq '{ | `scan.vulnerabilities_checked` | `false` means the OSV lookup never ran. No conclusion can be drawn at all | | `summary.unchecked_packages` | Above `0` means some packages could not be looked up. The result is partial | | `scan.complete` | `false` means either of the above, or a file failed to parse | +| `summary.suppressed` | Above `0` means findings were **accepted, not fixed**. Say so; never fold them into a clean summary | A correct summary: @@ -93,6 +94,9 @@ elif not report["summary"]["findings"]: verdict = f"No known vulnerabilities in {report['summary']['packages']} packages." else: verdict = f"{report['summary']['findings']} finding(s): {report['summary']['by_severity']}" + +if report["summary"]["suppressed"]: + verdict += f" ({report['summary']['suppressed']} accepted via the ignore file)" ``` ```python @@ -115,11 +119,36 @@ icebergsca scan .; echo "exit=$?" | `0` | The scan completed. Vulnerabilities may have been found — that is not an error | | `1` | The scan failed, or completed only partially. Treat the report as incomplete | | `2` | Usage error: unknown flag or format | +| `3` | Only with `--fail-on`: the scan completed and found something at or above the threshold | -Findings deliberately never change the exit code, so a non-zero exit always means *the tool -could not do its job*, not *your project has problems*. Do not use the exit code to decide +Findings deliberately never change the exit code unless `--fail-on` was passed, and even then +under a code of their own. So `1` always means *the tool could not do its job*, and `3` always +means *it did its job and you did not like the answer*. Do not use the exit code to decide whether vulnerabilities exist — read `summary.findings`. +## Accepting a finding + +Some findings cannot be fixed and should not be reported forever: an unfixable transitive +advisory, or one that is genuinely unreachable. `.icebergsca.toml` in the project root records +that decision. + +```toml +[[ignore]] +advisory = "GHSA-6757-jp84-gxfx" # or the CVE — aliases match too +package = "pyyaml" # bare name, or a purl with or without a version +reason = "Unreachable: we never call yaml.load on untrusted input" +expires = 2026-10-27 # optional; defaults to 90 days out +``` + +What this does **not** do is hide anything. The finding stays in `findings` with a +`suppression` object, `summary.suppressed` counts it, SARIF marks it dismissed rather than +fixed, and it stops tripping `--fail-on`. When reporting a scan, say how many findings were +accepted and by which rule — an accepted finding is a decision somebody made, not a solved +problem, and it comes back when the entry expires. + +A rule that matches nothing, or has expired, produces a `warnings` entry. Surface those: they +mean either a typo or an entry left behind after the upgrade that fixed it. + ## Reading a finding ```json diff --git a/src/icebergsca/.agents/skills/icebergsca/references/ci-integration.md b/src/icebergsca/.agents/skills/icebergsca/references/ci-integration.md index 8f46d2d..320e8b4 100644 --- a/src/icebergsca/.agents/skills/icebergsca/references/ci-integration.md +++ b/src/icebergsca/.agents/skills/icebergsca/references/ci-integration.md @@ -4,16 +4,54 @@ Findings never change the exit code. A non-zero exit means the scan itself did not complete. +Findings never change the exit code unless you ask them to with `--fail-on`. + | Code | Meaning | Pipeline response | |---|---|---| | `0` | Scan completed | Read `summary.findings` to decide what to do | | `1` | Scan failed or partial | Fail the job — the result is not trustworthy | | `2` | Usage error | Fix the invocation | +| `3` | `--fail-on` threshold met | Fail the job — the scan worked, the result did not pass | + +`1` and `3` are separate on purpose. A pipeline that cannot tell "the scanner broke" from "the +scanner found something" ends up treating both as noise. + +```bash +icebergsca scan . --fail-on critical +``` + +The gate checks completeness first, so it can never pass a scan that failed to look anything +up, and it ignores findings accepted in the ignore file. Do not add `|| true`: that is what +the ignore file is for. + +## Accepting a finding -This is deliberate: a scanner that exits non-zero on findings gets `|| true` appended within a -week, at which point genuine tool failures also go unnoticed. +The scheduled run below matters because most new findings arrive when an advisory is published, +not when the code changes. Which means one unfixable transitive advisory would otherwise turn +the build red every week forever — and a job that is red every week gets deleted. -To gate on severity today, read the JSON: +`.icebergsca.toml` in the project root records the assessment instead: + +```toml +[[ignore]] +advisory = "GHSA-6757-jp84-gxfx" # or the CVE — aliases are matched too +package = "pyyaml" # bare name, or a purl, optionally versioned +reason = "Unreachable: we never call yaml.load on untrusted input" +expires = 2026-10-27 # optional; defaults to 90 days from the scan +``` + +Point elsewhere with `--ignore-file`. What it does not do is hide anything: the finding stays +in the report with a `suppression` object, `summary.suppressed` counts it, SARIF marks it +dismissed rather than fixed, and `--fail-on` skips it. When the entry expires the finding comes +back — expiry never fails a scan by itself. + +A rule that matches nothing, or one past its date, lands in `warnings`. Watch for those: they +mean a typo, or an entry left behind after the upgrade that already fixed it. + +### Gating without `--fail-on` + +Reading the JSON still works, and is the way to gate on something more specific. Check +`scan.complete` **first** — gating on findings alone would pass a scan that checked nothing: ```bash icebergsca scan . --format json --output report.json @@ -22,10 +60,7 @@ jq -e '(.summary.by_severity.critical // 0) == 0' report.json > /dev/null \ || { echo "critical vulnerabilities found"; exit 1; } ``` -Check `scan.complete` **first**. Gating on findings alone would pass a scan that failed to -check anything. - -A built-in `--fail-on ` flag is planned but not yet implemented. +`summary.by_severity` counts active findings only, so accepted ones are already excluded. ## GitHub code scanning diff --git a/src/icebergsca/.agents/skills/icebergsca/references/json-report.md b/src/icebergsca/.agents/skills/icebergsca/references/json-report.md index 60c508a..ef809a4 100644 --- a/src/icebergsca/.agents/skills/icebergsca/references/json-report.md +++ b/src/icebergsca/.agents/skills/icebergsca/references/json-report.md @@ -12,13 +12,14 @@ icebergsca scan . --format json --output report.json ```json { - "schema_version": "1.0", + "schema_version": "1.1", "tool": { "name": "icebergsca", "version": "0.1.0" }, "scan": { ... }, "summary": { ... }, "manifests": [ ... ], "dependencies": [ ... ], "findings": [ ... ], + "suppressed": [ ... ], "unchecked_packages": [ ... ], "skipped": [ ... ], "warnings": [ ... ] @@ -53,6 +54,7 @@ it means the tool did its job. { "dependencies": 21, "direct": 9, "transitive": 12, "packages": 21, "findings": 12, + "suppressed": 2, "by_severity": { "critical": 3, "high": 4, "medium": 4, "low": 1 }, "unchecked_packages": 0, "skipped_files": 0 @@ -63,7 +65,12 @@ it means the tool did its job. manifests appears twice. `packages` counts unique packages. Use `packages` when reporting "we checked N packages". -`by_severity` omits levels with no findings and is ordered most severe first. +`findings` counts only findings nobody has accepted; `suppressed` counts the rest. The total in +the `findings` array is the two added together. **A suppressed finding is not a fixed one** — +somebody recorded a reason for accepting it and a date to revisit. Say so when reporting. + +`by_severity` omits levels with no findings and is ordered most severe first. It counts active +findings only, for the same reason. ## `manifests` @@ -144,11 +151,12 @@ One entry per ecosystem-and-directory scan unit. "direct": true, "introduced_by": [ { "path": "requirements.txt", "line": 3, "scope": "runtime", "direct": true } - ] + ], + "suppression": null } ``` -Sorted most severe first, then by package name. +Sorted active first, then most severe, then by package name. * `severity.level`: `critical`, `high`, `medium`, `low`, `none`, `unknown`. `unknown` means the advisory carries no rating — distinct from `none`, which is a scored zero. @@ -159,6 +167,31 @@ Sorted most severe first, then by package name. removal, not upgrade. * One CVE reported by several advisory databases is merged into a **single** finding, keeping whichever record carried the severity and fix version. The other IDs appear in `aliases`. +* `suppression` is `null` unless an ignore-file entry accepted the finding, in which case it + carries `reason`, `expires` and `rule`. **Read this field.** A suppressed finding is still in + this array — it is deliberately not removed — so code that ignores `suppression` counts it as + outstanding, and code that filters on it must not describe the result as "no vulnerabilities". + +## `suppressed` + +The ignore rules that fired, and what each accepted. This is an audit trail, not a second copy +of the findings — the per-finding state is `findings[].suppression`. + +```json +"suppressed": [ + { + "rule": "GHSA-6757-jp84-gxfx@pkg:pypi/pyyaml", + "reason": "Unreachable: we never call yaml.load on untrusted input", + "expires": "2026-10-27", + "findings": [{ "advisory": "GHSA-6757-jp84-gxfx", "purl": "pkg:pypi/pyyaml@5.1" }] + } +] +``` + +A rule that matched nothing, or one that has expired, does not appear here — it produces a +`warnings` entry instead, because a rule doing no work is usually a typo or an entry left behind +after the upgrade that fixed it. An expired entry stops suppressing, so the finding returns to +the active count. ## `unchecked_packages`, `skipped` and `warnings` diff --git a/src/icebergsca/cli/main.py b/src/icebergsca/cli/main.py index 2634160..924389a 100644 --- a/src/icebergsca/cli/main.py +++ b/src/icebergsca/cli/main.py @@ -23,17 +23,26 @@ from icebergsca.cache import Cache, cache_path from icebergsca.core.discovery import DiscoveryOptions from icebergsca.core.errors import IcebergSCAError -from icebergsca.core.models import EcosystemId, ScanReport, ScanStatus, Scope +from icebergsca.core.models import ( + EcosystemId, + ScanReport, + ScanStatus, + Scope, + SeverityLevel, +) from icebergsca.core.scanner import DEFAULT_SCOPES, ScanOptions, scan +from icebergsca.core.triage import IgnoreRule, parse_ignore_file from icebergsca.report import OutputFormat, cyclonedx, render class ExitCode(IntEnum): """Process exit codes. - Findings deliberately do not appear here. A scan that reports forty critical - vulnerabilities has done its job and exits :attr:`OK`; severity gating arrives - later as an explicit ``--fail-on`` flag. + Findings still do not affect the exit code on their own: a scan reporting forty + critical vulnerabilities has done its job and exits :attr:`OK`. They change it only + when the caller asked for a gate with ``--fail-on``, and even then under a code of + their own, so that "the gate tripped" stays distinguishable from "the scan broke". + A pipeline that cannot tell those apart eventually treats both as noise. :attr:`USAGE` is 2 rather than 1 because Typer's vendored Click reserves that code for usage errors, and remapping it would mean overriding framework @@ -45,6 +54,10 @@ class ExitCode(IntEnum): #: to check. Results are incomplete and must not be read as a clean result. SCAN_FAILED = 1 USAGE = 2 + #: Findings at or above the ``--fail-on`` threshold. Only ever returned when that + #: flag was passed, and only for a scan that completed — an incomplete scan is + #: :attr:`SCAN_FAILED` regardless of what it managed to find. + FINDINGS = 3 app = typer.Typer( @@ -57,6 +70,9 @@ class ExitCode(IntEnum): rich_markup_mode="rich", ) +#: The ignore file looked for beside the scan target when --ignore-file is absent. +IGNORE_FILENAME = ".icebergsca.toml" + logger = logging.getLogger("icebergsca") @@ -65,6 +81,60 @@ def _fail(message: str) -> NoReturn: raise typer.Exit(ExitCode.SCAN_FAILED) +def _ignore_rules(path: Path, ignore_file: Path | None) -> tuple[IgnoreRule, ...]: + """Locate and read the ignore file, if there is one. + + Reading it belongs here rather than in the scanner: it is tool configuration, not + scan input, and ``cli/`` is the only layer that touches either the filesystem for + its own purposes or the user directly. + + An explicitly named file that does not exist is an error — the caller asked for it + by name and a silent fall-through to "nothing is suppressed" would be a surprise in + the dangerous direction. A missing default file is simply the normal case. + """ + explicit = ignore_file is not None + target = ignore_file or _default_ignore_file(path) + + if not target.is_file(): + if explicit: + _fail(f"ignore file not found: {target}") + return () + + try: + content = target.read_text(encoding="utf-8") + except OSError as exc: + _fail(f"could not read {target}: {exc.strerror or exc}") + + try: + return parse_ignore_file(str(target), content) + except IcebergSCAError as exc: + _fail(str(exc)) + + +def _default_ignore_file(path: Path) -> Path: + """``.icebergsca.toml`` beside the scan target, or in it when it is a directory.""" + return (path if path.is_dir() else path.parent) / IGNORE_FILENAME + + +def _gate(report: ScanReport, threshold: SeverityLevel) -> None: + """Exit non-zero when an *unsuppressed* finding meets the threshold. + + Suppressed findings are excluded by design: being able to accept one is what makes + gating safe to turn on in the first place, since otherwise a single unfixable + transitive advisory turns the build red every week until somebody deletes the job. + """ + tripped = [f for f in report.active_findings if f.level.at_or_above(threshold)] + if not tripped: + return + + typer.secho( + f"{len(tripped)} finding(s) at or above {threshold.value}", + fg=typer.colors.RED, + err=True, + ) + raise typer.Exit(ExitCode.FINDINGS) + + def _configure_logging(verbosity: int, quiet: bool) -> None: """Send logs to stderr so that ``--format json | jq`` is never polluted.""" level = ( @@ -175,6 +245,22 @@ def scan_command( no_color: Annotated[ bool, typer.Option("--no-color", help="Disable coloured output.") ] = False, + fail_on: Annotated[ + SeverityLevel | None, + typer.Option( + "--fail-on", + help="Exit 3 when a finding at or above this severity is not suppressed. " + "An incomplete scan still exits 1, whatever it found.", + ), + ] = None, + ignore_file: Annotated[ + Path | None, + typer.Option( + "--ignore-file", + help=f"Accepted findings. Defaults to {IGNORE_FILENAME} beside the scan " + "target when it exists.", + ), + ] = None, verbose: Annotated[ int, typer.Option("-v", "--verbose", count=True, help="Increase log verbosity.") ] = 0, @@ -207,6 +293,7 @@ def scan_command( concurrency=concurrency, no_cache=no_cache, resolve_ranges=not no_resolve, + ignore_rules=_ignore_rules(path, ignore_file), ) try: @@ -216,6 +303,9 @@ def scan_command( _write(report, output_format, output, color=_use_color(no_color, output)) + # Completeness first, always. A gate that passed an incomplete scan because it + # happened to find nothing is the false clean this tool exists to avoid, and it is + # the single easiest way to build one. if report.status is not ScanStatus.OK or report.scan_failed: typer.secho( "scan incomplete — some files or packages could not be checked", @@ -224,6 +314,9 @@ def scan_command( ) raise typer.Exit(ExitCode.SCAN_FAILED) + if fail_on is not None: + _gate(report, fail_on) + def _use_color(no_color: bool, output: Path | None) -> bool: """Colour only when writing to a terminal, honouring the NO_COLOR convention.""" diff --git a/src/icebergsca/core/errors.py b/src/icebergsca/core/errors.py index ab75376..0b0836c 100644 --- a/src/icebergsca/core/errors.py +++ b/src/icebergsca/core/errors.py @@ -56,3 +56,20 @@ class UpstreamError(IcebergSCAError): class CacheError(IcebergSCAError): """The on-disk cache could not be opened or written.""" + + +class ConfigError(IcebergSCAError): + """The user's own configuration file is malformed. + + Deliberately strict, unlike the parsers: a third-party manifest we did not write + gets the benefit of the doubt and anything unrecognised is skipped, because the + alternative is refusing to scan a project over a field we never needed. An ignore + file is the opposite case. It exists to *remove* findings, so a mistyped key that + silently suppresses nothing — or everything — is precisely the failure this tool + must not have. Say so and stop. + """ + + def __init__(self, path: str, reason: str) -> None: + super().__init__(f"{path}: {reason}") + self.path = path + self.reason = reason diff --git a/src/icebergsca/core/models.py b/src/icebergsca/core/models.py index e3ed6b0..6512569 100644 --- a/src/icebergsca/core/models.py +++ b/src/icebergsca/core/models.py @@ -16,13 +16,13 @@ from __future__ import annotations from dataclasses import dataclass, field -from datetime import datetime +from datetime import date, datetime from enum import StrEnum from pathlib import Path from packageurl import PackageURL -SCHEMA_VERSION = "1.0" +SCHEMA_VERSION = "1.1" # --------------------------------------------------------------------------- # Ecosystems @@ -149,6 +149,15 @@ def rank(self) -> int: """Sort key, highest severity first when sorting descending.""" return _SEVERITY_RANK[self] + def at_or_above(self, threshold: SeverityLevel) -> bool: + """True when this level is at least as severe as ``threshold``. + + Use this rather than comparing members. ``SeverityLevel`` is a ``StrEnum``, so + ``<`` and ``>`` compare the *strings*: ``HIGH < LOW`` is True because ``"high"`` + sorts before ``"low"``, which is exactly backwards and silently so. + """ + return self.rank >= threshold.rank + _SEVERITY_RANK: dict[SeverityLevel, int] = { SeverityLevel.CRITICAL: 5, @@ -332,6 +341,25 @@ def cve_ids(self) -> tuple[str, ...]: return tuple(a for a in self.aliases if a.startswith("CVE-")) +@dataclass(frozen=True, slots=True) +class Suppression: + """A human's recorded decision to accept a finding, from the ignore file. + + Note what this is not: a statement that the finding is wrong, or fixed, or that the + package is unaffected. It records only that someone looked and wrote down why, with + a date by which they intend to look again. + """ + + #: Why the finding was accepted. Required — an entry with no justification is + #: indistinguishable from a mistake six months later. + reason: str + #: When the decision lapses. After this date the finding surfaces again. + expires: date + #: The ignore entry that matched, as ``advisory@package``, so a reader can find + #: the rule responsible without diffing the file. + rule: str + + @dataclass(frozen=True, slots=True) class Finding: """One advisory affecting one package, with every path that introduced it.""" @@ -341,11 +369,20 @@ class Finding: #: Lowest fixed version above the installed one, or ``None`` if no fix is known. fixed_version: str | None = None introduced_by: tuple[Dependency, ...] = () + #: Set when an ignore rule matched. A suppressed finding stays in the report and + #: stays in :attr:`ScanReport.findings`; only the counts and the exit-code gate + #: treat it differently. Removing it from the list would let an all-suppressed + #: scan render as "no known vulnerabilities". + suppression: Suppression | None = None @property def level(self) -> SeverityLevel: return self.advisory.level + @property + def is_suppressed(self) -> bool: + return self.suppression is not None + @property def is_direct(self) -> bool: return any(dep.direct for dep in self.introduced_by) @@ -448,18 +485,43 @@ def is_complete(self) -> bool: and self.status is ScanStatus.OK ) + @property + def active_findings(self) -> tuple[Finding, ...]: + """Findings nobody has accepted — what a build should be judged on.""" + return tuple(f for f in self.findings if not f.is_suppressed) + + @property + def suppressed_findings(self) -> tuple[Finding, ...]: + """Findings an ignore rule accepted. Still reported, never discarded.""" + return tuple(f for f in self.findings if f.is_suppressed) + def counts_by_severity(self) -> dict[SeverityLevel, int]: - """Finding counts per severity, highest first, omitting empty levels.""" + """Active finding counts per severity, highest first, omitting empty levels. + + Suppressed findings are excluded — they are counted separately rather than + folded in, so that "3 high" means three someone still has to deal with. With + no ignore file in play this is every finding, exactly as before. + """ counts: dict[SeverityLevel, int] = {} - for finding in self.findings: + for finding in self.active_findings: counts[finding.level] = counts.get(finding.level, 0) + 1 return dict(sorted(counts.items(), key=lambda item: item[0].rank, reverse=True)) def sorted_findings(self) -> tuple[Finding, ...]: - """Findings ordered most severe first, then by package for stable output.""" + """Findings ordered active first, then most severe, then by package. + + Suppressed findings sort last rather than being dropped: every renderer walks + this list, and one that stopped seeing them would report an accepted finding + as a fixed one. + """ return tuple( sorted( self.findings, - key=lambda f: (-f.level.rank, f.package.name, f.advisory.id), + key=lambda f: ( + f.is_suppressed, + -f.level.rank, + f.package.name, + f.advisory.id, + ), ) ) diff --git a/src/icebergsca/core/scanner.py b/src/icebergsca/core/scanner.py index 89d4f19..0160f0d 100644 --- a/src/icebergsca/core/scanner.py +++ b/src/icebergsca/core/scanner.py @@ -22,6 +22,7 @@ from icebergsca import __version__ from icebergsca.cache import Cache +from icebergsca.core import triage from icebergsca.core.discovery import Discovery, DiscoveryOptions, ScanUnit, discover from icebergsca.core.errors import CacheError, ParseError from icebergsca.core.graph import most_included @@ -37,6 +38,7 @@ Scope, SkippedFile, ) +from icebergsca.core.triage import IgnoreRule from icebergsca.ecosystems import get as get_ecosystem from icebergsca.ecosystems.base import FileParser from icebergsca.ecosystems.maven import MavenResolver, MavenResult, MavenUnit @@ -75,6 +77,10 @@ class ScanOptions: #: findings — and the report says so via ``vulnerabilities_checked`` rather than #: presenting an empty finding list as a clean result. check_vulnerabilities: bool = True + #: Accepted findings, already parsed. A plain value like every other option: the + #: CLI locates and reads ``.icebergsca.toml``, because tool configuration is its + #: business, not the scanner's. + ignore_rules: tuple[IgnoreRule, ...] = () async def scan(root: Path, options: ScanOptions | None = None) -> ScanReport: @@ -232,12 +238,18 @@ async def _resolve_and_check( for ref, hits in result.advisories.items() for hit in hits ) + # Applied here, at the one place findings are built, so the accepted ones are the + # same objects the report ships. Note this is below the ``check_vulnerabilities`` + # early return above: there is nothing to accept when nothing was looked up, and + # marking an empty list as triaged would be a clean result nobody earned. + findings, ignore_warnings = triage.apply(findings, options.ignore_rules) + return _VulnOutcome( dependencies, findings, result, checked=True, - warnings=warnings, + warnings=warnings + ignore_warnings, maven_approximate=maven.approximate, ) diff --git a/src/icebergsca/core/triage.py b/src/icebergsca/core/triage.py new file mode 100644 index 0000000..7764a15 --- /dev/null +++ b/src/icebergsca/core/triage.py @@ -0,0 +1,229 @@ +"""Accepting a finding, on the record. + +A weekly scheduled scan is the right way to run this tool, because most new findings +arrive when an advisory is published rather than when code changes. But it means a +project with one unfixable transitive advisory gets a red build every week forever, +with nowhere to say that somebody assessed it. That ends in ``|| true`` on the command +or the job quietly deleted, which is worse than the finding. + +So: an ignore file. Everything here is shaped by the fact that it is the one feature +whose job is to make findings go away, and therefore the one most able to undermine the +rest of the tool: + +* A suppressed finding stays in the report, marked. It never vanishes. +* A reason is required. Without one, an entry is indistinguishable from a mistake. +* An expiry always exists, defaulting rather than being optional, so that no entry + becomes permanent by accident. Past it, the finding comes back. +* Matching is exact. A rule names one advisory and one package, so it can never grow + to swallow a second, unrelated finding. +* A rule that matches nothing says so, because a typo that silently suppresses nothing + looks identical to a rule that is working. + +Parsing takes text, never a path — file reading lives in the layer above, as it does +for every other parser here. +""" + +from __future__ import annotations + +import tomllib +from dataclasses import dataclass, replace +from datetime import date, timedelta +from typing import Any + +from icebergsca.core.errors import ConfigError +from icebergsca.core.models import Finding, PackageRef, Suppression + +#: How long an entry lasts when it does not say. Long enough not to be busywork, short +#: enough that a yearly audit is not the first time anyone revisits it. +DEFAULT_EXPIRY_DAYS = 90 + +_ENTRY_KEYS = frozenset({"advisory", "package", "reason", "expires"}) +_REQUIRED_KEYS = ("advisory", "package", "reason") + + +@dataclass(frozen=True, slots=True) +class IgnoreRule: + """One ``[[ignore]]`` entry.""" + + #: An OSV ID or any of its aliases — ``GHSA-...``, ``CVE-...``, ``PYSEC-...``. + #: Matching either means a user can write the CVE they were sent even though OSV + #: answers with a GHSA. + advisory: str + #: A bare ``name`` (``group:artifact`` for Maven), or a purl with or without a + #: version. Never a pattern. The versioned form pins the decision to the version + #: assessed, so an upgrade brings the finding back for a fresh look; the other two + #: forms outlive it. Both are legitimate — which one is right depends on whether + #: the reason was about this version or about how the package is used. + package: str + reason: str + expires: date + + @property + def key(self) -> str: + return f"{self.advisory}@{self.package}" + + def is_expired(self, today: date) -> bool: + return today > self.expires + + def matches(self, finding: Finding) -> bool: + """Exact match on advisory (or alias) and package (purl or bare name). + + Deliberately not a substring or glob test. A rule for ``GHSA-aaaa`` must not + reach ``GHSA-aaaa-bbbb``, and one written for a package must not travel to + another that merely shares a prefix. + """ + advisory = self.advisory.lower() + known = {finding.advisory.id.lower()} + known.update(alias.lower() for alias in finding.advisory.aliases) + if advisory not in known: + return False + + ref = finding.package + unversioned = PackageRef(ref.ecosystem, ref.name).purl + return self.package in (ref.purl, unversioned, ref.name) + + +def parse_ignore_file( + path: str, content: str, *, today: date | None = None +) -> tuple[IgnoreRule, ...]: + """Parse ``.icebergsca.toml`` into rules, refusing anything it does not understand. + + Every parser in this codebase is forgiving, skipping what it cannot read, because a + third-party manifest is not ours to police and refusing to scan over an unknown + field would be worse. This one is the opposite: the file is the user's own, it + exists to remove findings, and a key silently ignored because it was misspelled is + the difference between a finding being assessed and being invisible. + """ + today = today or date.today() + + try: + data = tomllib.loads(content) + except tomllib.TOMLDecodeError as exc: + raise ConfigError(path, f"invalid TOML: {exc}") from exc + + raw = data.get("ignore", []) + if not isinstance(raw, list): + raise ConfigError(path, "'ignore' must be a list of [[ignore]] entries") + + rules: list[IgnoreRule] = [] + for index, entry in enumerate(raw, start=1): + rules.append(_rule(path, index, entry, today)) + + _reject_duplicates(path, rules) + return tuple(rules) + + +def apply( + findings: tuple[Finding, ...], + rules: tuple[IgnoreRule, ...], + *, + today: date | None = None, +) -> tuple[tuple[Finding, ...], tuple[str, ...]]: + """Mark findings their rules accept, and report on the rules themselves. + + Returns the findings — all of them, with the accepted ones marked — and warnings + about rules that did no work. A rule matching nothing is worth saying out loud: it + is either a typo, or an entry left behind after the upgrade that fixed it, and both + look exactly like a rule that is quietly working. + """ + if not rules: + return findings, () + + today = today or date.today() + live = [rule for rule in rules if not rule.is_expired(today)] + expired = [rule for rule in rules if rule.is_expired(today)] + + used: set[str] = set() + marked: list[Finding] = [] + for finding in findings: + rule = next((r for r in live if r.matches(finding)), None) + if rule is None: + marked.append(finding) + continue + used.add(rule.key) + marked.append( + replace( + finding, + suppression=Suppression( + reason=rule.reason, expires=rule.expires, rule=rule.key + ), + ) + ) + + warnings: list[str] = [] + for rule in expired: + # Not an error, and emphatically not a reason to fail the scan: the finding + # simply stops being accepted and shows up again, which is the point. + warnings.append( + f"ignore entry {rule.key} expired on {rule.expires.isoformat()}; " + "the finding is reported again" + ) + for rule in live: + if rule.key not in used: + warnings.append( + f"ignore entry {rule.key} matched no finding; " + "it may be misspelled, or the finding may already be resolved" + ) + + return tuple(marked), tuple(warnings) + + +def _rule(path: str, index: int, entry: Any, today: date) -> IgnoreRule: + where = f"ignore entry {index}" + if not isinstance(entry, dict): + raise ConfigError(path, f"{where} must be a table") + + unknown = sorted(set(entry) - _ENTRY_KEYS) + if unknown: + known = ", ".join(sorted(_ENTRY_KEYS)) + raise ConfigError( + path, f"{where} has unknown key(s) {', '.join(unknown)}; expected {known}" + ) + + values: dict[str, str] = {} + for key in _REQUIRED_KEYS: + value = entry.get(key) + if not isinstance(value, str) or not value.strip(): + raise ConfigError(path, f"{where} needs a non-empty '{key}'") + values[key] = value.strip() + + return IgnoreRule( + advisory=values["advisory"], + package=values["package"], + reason=values["reason"], + expires=_expires(path, where, entry.get("expires"), today), + ) + + +def _expires(path: str, where: str, value: Any, today: date) -> date: + """A date, or a default — never "no expiry". + + TOML has a native date type, so an unquoted ``2026-10-27`` arrives as a ``date``. A + quoted one arrives as a string and is parsed, because the difference is invisible in + a text editor and refusing it would teach nothing. + """ + if value is None: + return today + timedelta(days=DEFAULT_EXPIRY_DAYS) + if isinstance(value, date): + return value + if isinstance(value, str): + try: + return date.fromisoformat(value.strip()) + except ValueError as exc: + raise ConfigError( + path, f"{where} has an unparseable 'expires': {exc}" + ) from exc + raise ConfigError(path, f"{where} 'expires' must be a date like 2026-10-27") + + +def _reject_duplicates(path: str, rules: list[IgnoreRule]) -> None: + """Two entries for the same advisory and package mean one of them does nothing. + + Which one wins is an implementation detail nobody should have to know, and the + second is usually an edit that was meant to replace the first. + """ + seen: set[str] = set() + for rule in rules: + if rule.key in seen: + raise ConfigError(path, f"duplicate ignore entry for {rule.key}") + seen.add(rule.key) diff --git a/src/icebergsca/report/csv_.py b/src/icebergsca/report/csv_.py index b11064f..7625981 100644 --- a/src/icebergsca/report/csv_.py +++ b/src/icebergsca/report/csv_.py @@ -36,6 +36,10 @@ "source_file", "source_line", "summary", + # Appended, never inserted: consumers index these columns positionally and a + # spreadsheet built last quarter should still line up. + "suppressed", + "suppression_reason", ) @@ -84,6 +88,11 @@ def render(report: ScanReport, *, color: bool = True, width: int | None = None) str(dep.source.path) if dep else "", (dep.source.line or "") if dep else "", advisory.summary.replace("\n", " ").strip(), + "true" if finding.is_suppressed else "false", + # User-supplied text bound for a spreadsheet. It goes through _defuse + # with every other cell below, which is what keeps a reason beginning + # "=" from becoming a formula. + finding.suppression.reason if finding.suppression else "", ) writer.writerow([_defuse(cell) for cell in row]) diff --git a/src/icebergsca/report/cyclonedx.py b/src/icebergsca/report/cyclonedx.py index 3bc2753..c6a9a8f 100644 --- a/src/icebergsca/report/cyclonedx.py +++ b/src/icebergsca/report/cyclonedx.py @@ -99,6 +99,10 @@ def _metadata(report: ScanReport) -> dict[str, Any]: "value": str(report.vulnerabilities_checked).lower(), }, {"name": "icebergsca:complete", "value": str(report.is_complete).lower()}, + { + "name": "icebergsca:suppressedCount", + "value": str(len(report.suppressed_findings)), + }, ], } @@ -188,6 +192,16 @@ def _vulnerability(finding: Finding) -> dict[str, Any]: entry["recommendation"] = ( f"Upgrade {finding.package.name} to {finding.fixed_version} or later." ) + if finding.suppression is not None: + # Only ``detail``. CycloneDX's ``state`` and ``justification`` are closed + # enumerations of security claims — ``not_affected``, ``code_not_reachable`` + # and so on — and all this tool actually knows is that a human wrote a sentence + # and a date. Asserting one of those values from free text would be inventing + # an analysis nobody performed. + entry["analysis"] = { + "detail": f"Accepted until {finding.suppression.expires.isoformat()}: " + f"{finding.suppression.reason}" + } return entry diff --git a/src/icebergsca/report/json_.py b/src/icebergsca/report/json_.py index 772ebbb..872b551 100644 --- a/src/icebergsca/report/json_.py +++ b/src/icebergsca/report/json_.py @@ -45,7 +45,11 @@ def to_dict(report: ScanReport) -> dict[str, Any]: "direct": report.direct_count, "transitive": report.transitive_count, "packages": len(report.packages), - "findings": len(report.findings), + # Outstanding findings. ``suppressed`` is reported beside it rather than + # inside it, so a consumer reading only ``findings`` under-states nothing: + # the total is findings + suppressed, and both are named. + "findings": len(report.active_findings), + "suppressed": len(report.suppressed_findings), "by_severity": { level.value: count for level, count in report.counts_by_severity().items() @@ -56,6 +60,12 @@ def to_dict(report: ScanReport) -> dict[str, Any]: "manifests": [_manifest(result) for result in report.manifests], "dependencies": [_dependency(dep) for dep in report.dependencies], "findings": [_finding(finding) for finding in report.sorted_findings()], + # The rules, not a second copy of the findings. Duplicating finding objects + # across two arrays invites double-counting, and lets a consumer read + # ``findings`` alone and never learn some were accepted — which is why the + # per-finding state lives on the finding itself. What is here instead is the + # audit trail: which entry accepted what, and which did no work at all. + "suppressed": _suppressions(report), "unchecked_packages": [_package(ref) for ref in report.scan_failed], "skipped": [ {"path": str(entry.path), "reason": entry.reason} @@ -65,6 +75,28 @@ def to_dict(report: ScanReport) -> dict[str, Any]: } +def _suppressions(report: ScanReport) -> list[dict[str, Any]]: + """One entry per ignore rule that fired, with what it accepted.""" + grouped: dict[str, dict[str, Any]] = {} + for finding in report.sorted_findings(): + rule = finding.suppression + if rule is None: + continue + entry = grouped.setdefault( + rule.rule, + { + "rule": rule.rule, + "reason": rule.reason, + "expires": rule.expires.isoformat(), + "findings": [], + }, + ) + entry["findings"].append( + {"advisory": finding.advisory.id, "purl": finding.package.purl} + ) + return list(grouped.values()) + + def _package(ref: PackageRef) -> dict[str, Any]: return { "ecosystem": ref.ecosystem.osv_name, @@ -138,4 +170,16 @@ def _finding(finding: Finding) -> dict[str, Any]: } for dep in finding.introduced_by ], + # Present on every finding, ``null`` when nothing accepted it, so that reading + # this field is never optional. A consumer that skips it counts an accepted + # finding as outstanding, which is the harmless direction. + "suppression": ( + None + if finding.suppression is None + else { + "reason": finding.suppression.reason, + "expires": finding.suppression.expires.isoformat(), + "rule": finding.suppression.rule, + } + ), } diff --git a/src/icebergsca/report/sarif.py b/src/icebergsca/report/sarif.py index 9d7f242..db5d31a 100644 --- a/src/icebergsca/report/sarif.py +++ b/src/icebergsca/report/sarif.py @@ -181,6 +181,23 @@ def _help_markdown(finding: Finding) -> str: def _result(finding: Finding) -> dict[str, Any]: + result = _base_result(finding) + if finding.suppression is not None: + # SARIF's own slot for this. Kept in ``results`` rather than filtered out, so + # GitHub renders the alert as dismissed rather than closing it as fixed — and + # so the rule it points at survives, since the rule list is derived from the + # same findings. + result["suppressions"] = [ + { + "kind": "external", + "status": "accepted", + "justification": finding.suppression.reason, + } + ] + return result + + +def _base_result(finding: Finding) -> dict[str, Any]: return { "ruleId": _rule_id(finding), "level": _LEVEL[finding.level], diff --git a/src/icebergsca/report/table.py b/src/icebergsca/report/table.py index e2ea1da..a6bff63 100644 --- a/src/icebergsca/report/table.py +++ b/src/icebergsca/report/table.py @@ -144,19 +144,28 @@ def _render_findings(console: Console, report: ScanReport) -> None: table.add_column("Advisory") table.add_column("Fixed in") table.add_column("Via") + table.add_column("") for finding in report.sorted_findings(): - style = _SEVERITY_STYLE[finding.level] + # Suppressed rows are dimmed rather than dropped, and keep their real severity + # so that nobody reads an accepted critical as a medium. + style = "dim" if finding.is_suppressed else _SEVERITY_STYLE[finding.level] table.add_row( f"[{style}]{finding.level.value}[/{style}]", - escape(finding.package.name), + f"[{style}]{escape(finding.package.name)}[/{style}]", escape(finding.package.version or "?"), escape(_advisory_label(finding)), escape(finding.fixed_version or "—"), escape(_via_label(finding)), + "[dim]accepted[/dim]" if finding.is_suppressed else "", ) console.print(table) + if report.suppressed_findings: + console.print( + "[dim]accepted — matched an entry in the ignore file; still reported, " + "and excluded from the counts below and from --fail-on[/dim]" + ) console.print() _render_summary(console, report) @@ -179,9 +188,15 @@ def _render_summary(console: Console, report: ScanReport) -> None: f"[{_SEVERITY_STYLE[level]}]{count} {level.value}[/{_SEVERITY_STYLE[level]}]" for level, count in report.counts_by_severity().items() ] - console.print( - f"[bold]{len(report.findings)} finding(s):[/bold] " + " ".join(parts) - ) + # The headline number counts what is still outstanding. Accepted findings are named + # separately rather than folded in or dropped: rolled into the total they would look + # like work nobody has done, and omitted entirely they would look like work that + # never existed. + suppressed = len(report.suppressed_findings) + line = f"[bold]{len(report.active_findings)} finding(s):[/bold] " + " ".join(parts) + if suppressed: + line += f" [dim]· {suppressed} accepted[/dim]" + console.print(line) console.print() diff --git a/tests/test_cli.py b/tests/test_cli.py index bee7f73..c7e7e65 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,6 +6,7 @@ import re from pathlib import Path +import httpx import pytest from typer.testing import CliRunner @@ -71,7 +72,7 @@ def test_output_file(tmp_path: Path) -> None: ], ) assert result.exit_code == ExitCode.OK - assert json.loads(destination.read_text())["schema_version"] == "1.0" + assert json.loads(destination.read_text())["schema_version"] == "1.1" def test_dev_dependencies_are_excluded_by_default(tmp_path: Path) -> None: @@ -241,3 +242,185 @@ def explode(self: Cache) -> int: assert result.exit_code == ExitCode.SCAN_FAILED assert isinstance(result.exception, SystemExit) assert "disk is full" in result.stderr + + +# --------------------------------------------------------------------------- +# Suppression and --fail-on +# --------------------------------------------------------------------------- + + +class _FindingTransport(httpx.AsyncBaseTransport): + """Answer OSV as though every queried package had one high-severity advisory.""" + + async def handle_async_request(self, request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/querybatch"): + body = json.loads(request.content or b'{"queries": []}') + results = [ + {"vulns": [{"id": "GHSA-test", "modified": "2026-01-01T00:00:00Z"}]} + for _ in body.get("queries", []) + ] + return httpx.Response(200, json={"results": results}, request=request) + if "/v1/vulns/" in request.url.path: + return httpx.Response( + 200, + json={ + "id": "GHSA-test", + "modified": "2026-01-01T00:00:00Z", + "aliases": ["CVE-2026-9999"], + "summary": "Example", + "severity": [ + { + "type": "CVSS_V3", + "score": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N", + } + ], + }, + request=request, + ) + return httpx.Response(404, json={"error": "not mocked"}, request=request) + + +@pytest.fixture +def with_findings(monkeypatch: pytest.MonkeyPatch) -> None: + from icebergsca.core import scanner + + monkeypatch.setattr( + scanner, + "build_http_client", + lambda options: httpx.AsyncClient(transport=_FindingTransport()), + ) + + +def one_package(tmp_path: Path) -> Path: + """A project with a single dependency, so one ignore entry covers everything.""" + (tmp_path / "requirements.txt").write_text("requests==2.31.0\n") + return tmp_path + + +def ignore_file(tmp_path: Path, **fields: str) -> Path: + entry = { + "advisory": "GHSA-test", + "package": "requests", + "reason": "Assessed: not reachable from our entry points", + } | fields + body = "\n".join(f'{key} = "{value}"' for key, value in entry.items()) + target = tmp_path / ".icebergsca.toml" + target.write_text(f"[[ignore]]\n{body}\n") + return target + + +def test_findings_alone_still_exit_zero(tmp_path: Path, with_findings: None) -> None: + """The design this feature must not undo: a finding is data, not a failure.""" + result = runner.invoke(app, ["scan", str(project(tmp_path))]) + assert result.exit_code == ExitCode.OK + + +def test_fail_on_exits_three_above_the_threshold( + tmp_path: Path, with_findings: None +) -> None: + result = runner.invoke(app, ["scan", str(project(tmp_path)), "--fail-on", "high"]) + assert result.exit_code == ExitCode.FINDINGS + + +def test_fail_on_exits_zero_below_the_threshold( + tmp_path: Path, with_findings: None +) -> None: + result = runner.invoke( + app, ["scan", str(project(tmp_path)), "--fail-on", "critical"] + ) + assert result.exit_code == ExitCode.OK + + +def test_an_accepted_finding_does_not_trip_the_gate( + tmp_path: Path, with_findings: None +) -> None: + """The whole point of pairing the two: gating is only safe once you can accept.""" + target = one_package(tmp_path) + ignore_file(target) + result = runner.invoke(app, ["scan", str(target), "--fail-on", "high"]) + assert result.exit_code == ExitCode.OK + + +def test_an_expired_entry_trips_the_gate_again( + tmp_path: Path, with_findings: None +) -> None: + target = one_package(tmp_path) + ignore_file(target, expires="2020-01-01") + result = runner.invoke(app, ["scan", str(target), "--fail-on", "high"]) + assert result.exit_code == ExitCode.FINDINGS + + +def test_an_incomplete_scan_exits_one_not_three( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The false-clean trap, from the other side. + + Completeness is checked before the gate, so a scan that could not look anything up + fails as a broken scan — never passing because it happened to find nothing, and + never reported as a policy failure it did not actually establish. + """ + from icebergsca.core import scanner + from icebergsca.osv import client as client_module + + # Same collapse as tests/test_osv.py: the retry schedule is asserted there, and + # paying it in real seconds here would put the suite over its time budget. + monkeypatch.setattr(client_module, "_BACKOFF_BASE", 0.0001) + monkeypatch.setattr(client_module, "_BACKOFF_CAP", 0.001) + + class _Unreachable(httpx.AsyncBaseTransport): + async def handle_async_request(self, request: httpx.Request) -> httpx.Response: + raise httpx.ConnectError("no route to host") + + monkeypatch.setattr( + scanner, + "build_http_client", + lambda options: httpx.AsyncClient(transport=_Unreachable()), + ) + result = runner.invoke( + app, ["scan", str(project(tmp_path)), "--fail-on", "critical"] + ) + assert result.exit_code == ExitCode.SCAN_FAILED + + +def test_a_suppressed_finding_is_still_reported( + tmp_path: Path, with_findings: None +) -> None: + target = one_package(tmp_path) + ignore_file(target) + result = runner.invoke(app, ["scan", str(target), "--format", "json"]) + document = json.loads(result.stdout) + + assert document["summary"]["suppressed"] == 1 + assert any(f["suppression"] for f in document["findings"]) + + +def test_an_unreadable_ignore_file_named_explicitly_is_an_error( + tmp_path: Path, +) -> None: + """Asking for a file by name and silently getting no rules is the wrong surprise.""" + result = runner.invoke( + app, + ["scan", str(project(tmp_path)), "--ignore-file", str(tmp_path / "nope.toml")], + ) + assert result.exit_code == ExitCode.SCAN_FAILED + assert "not found" in result.stderr + + +def test_a_malformed_ignore_file_fails_loudly(tmp_path: Path) -> None: + target = project(tmp_path) + (target / ".icebergsca.toml").write_text('[[ignore]]\nadvisroy = "x"\n') + result = runner.invoke(app, ["scan", str(target)]) + assert result.exit_code == ExitCode.SCAN_FAILED + assert "advisroy" in result.stderr + + +def test_unknown_fail_on_severity_is_a_usage_error(tmp_path: Path) -> None: + result = runner.invoke( + app, ["scan", str(project(tmp_path)), "--fail-on", "catastrophic"] + ) + assert result.exit_code == ExitCode.USAGE + + +def test_fail_on_choices_are_listed_in_help() -> None: + result = runner.invoke(app, ["scan", "--help"], env={"COLUMNS": "200"}) + assert "critical|high|medium|low|none|unknown" in plain(result.stdout) diff --git a/tests/test_report.py b/tests/test_report.py index f859f0e..9920eaa 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -8,6 +8,7 @@ import csv import json +from datetime import date from io import StringIO from pathlib import Path @@ -169,7 +170,7 @@ def test_json_document_shape() -> None: ) document = json.loads(render(report, OutputFormat.JSON)) - assert document["schema_version"] == "1.0" + assert document["schema_version"] == "1.1" assert document["tool"]["name"] == "icebergsca" assert document["summary"]["findings"] == 1 assert document["summary"]["by_severity"] == {"high": 1} @@ -352,3 +353,103 @@ def test_json_records_what_a_dependency_excludes() -> None: ) payload = json.loads(render(report, OutputFormat.JSON)) assert payload["dependencies"][0]["exclusions"] == ["com.lowagie:itext"] + + +def suppressed_finding(**overrides: object) -> Finding: + from icebergsca.core.models import Suppression + + defaults: dict[str, object] = { + "package": PackageRef(EcosystemId.PYPI, "pyyaml", "5.1"), + "advisory": make_advisory(), + "suppression": Suppression( + reason="Unreachable from our code paths", + expires=date(2026, 10, 27), + rule="GHSA-xxxx-yyyy-zzzz@pyyaml", + ), + } + return Finding(**{**defaults, **overrides}) # type: ignore[arg-type] + + +def test_a_fully_suppressed_report_never_reads_as_clean() -> None: + """The load-bearing one. + + If suppressed findings were filtered out of ``findings`` rather than marked, this + report would take the empty-findings branch and print "No known vulnerabilities" — + turning an accepted vulnerability into a clean bill of health. + """ + output = table_of( + { + "dependencies": (make_dependency(),), + "vulnerabilities_checked": True, + "findings": (suppressed_finding(),), + } + ) + assert "No known vulnerabilities" not in output + assert "accepted" in output + + +def test_the_summary_counts_accepted_findings_separately() -> None: + output = table_of( + { + "dependencies": (make_dependency(),), + "vulnerabilities_checked": True, + "findings": ( + suppressed_finding(), + Finding( + package=PackageRef(EcosystemId.PYPI, "flask", "2.0"), + advisory=make_advisory("GHSA-live"), + ), + ), + } + ) + assert "1 finding(s)" in output + assert "1 accepted" in output + + +def test_the_accepted_marker_is_explained_rather_than_left_bare() -> None: + output = table_of( + { + "dependencies": (make_dependency(),), + "vulnerabilities_checked": True, + "findings": (suppressed_finding(),), + } + ) + assert "ignore file" in output + + +def test_json_carries_the_suppression_on_the_finding_and_in_the_audit_trail() -> None: + report = make_report(vulnerabilities_checked=True, findings=(suppressed_finding(),)) + document = json.loads(render(report, OutputFormat.JSON)) + + entry = document["findings"][0]["suppression"] + assert entry["reason"] == "Unreachable from our code paths" + assert entry["expires"] == "2026-10-27" + + assert document["summary"]["findings"] == 0 + assert document["summary"]["suppressed"] == 1 + assert document["suppressed"][0]["rule"] == "GHSA-xxxx-yyyy-zzzz@pyyaml" + + +def test_json_suppression_is_null_when_nothing_accepted_the_finding() -> None: + """Always present, so reading it is never optional.""" + report = make_report( + vulnerabilities_checked=True, + findings=( + Finding( + package=PackageRef(EcosystemId.PYPI, "flask", "2.0"), + advisory=make_advisory(), + ), + ), + ) + document = json.loads(render(report, OutputFormat.JSON)) + assert document["findings"][0]["suppression"] is None + assert document["suppressed"] == [] + + +def test_csv_records_suppression_without_disturbing_existing_columns() -> None: + report = make_report(vulnerabilities_checked=True, findings=(suppressed_finding(),)) + text = render(report, OutputFormat.CSV) + assert text.startswith("severity,cvss_score,advisory_id") + row = next(csv.DictReader(StringIO(text))) + assert row["suppressed"] == "true" + assert row["suppression_reason"] == "Unreachable from our code paths" diff --git a/tests/test_report_formats.py b/tests/test_report_formats.py index 0c8139f..854f344 100644 --- a/tests/test_report_formats.py +++ b/tests/test_report_formats.py @@ -8,6 +8,7 @@ from __future__ import annotations import json +from datetime import date from functools import cache from pathlib import Path from typing import Any @@ -23,6 +24,7 @@ PackageRef, Severity, SeverityLevel, + Suppression, ) from icebergsca.report import OutputFormat, cyclonedx, render, sarif from tests.conftest import make_advisory, make_dependency, make_report @@ -80,6 +82,7 @@ def sample_report(**overrides: Any) -> Any: unscored = make_dependency( "left-pad", "1.0.0", ecosystem=EcosystemId.NPM, path="package.json" ) + accepted = make_dependency("jinja2", "3.1.2", path="requirements.txt", line=7) findings = ( Finding( @@ -104,10 +107,22 @@ def sample_report(**overrides: Any) -> Any: advisory=make_advisory("MAL-2024-1", level=None), introduced_by=(unscored,), ), + Finding( + package=accepted.ref, + advisory=make_advisory("GHSA-cccc", level=SeverityLevel.HIGH, score=7.5), + introduced_by=(accepted,), + # Suppressed findings are rendered, not dropped, so they must satisfy the + # official schemas like any other — which is what this entry buys. + suppression=Suppression( + reason="Unreachable: the affected parser is never called", + expires=date(2026, 10, 27), + rule="GHSA-cccc@jinja2", + ), + ), ) defaults: dict[str, Any] = { - "dependencies": (scored, transitive, unscored), + "dependencies": (scored, transitive, unscored, accepted), "findings": findings, "vulnerabilities_checked": True, "warnings": ("an example warning",), @@ -350,3 +365,33 @@ def test_severity_is_carried_through_from_a_vector() -> None: rating = document["vulnerabilities"][0]["ratings"][0] assert rating["method"] == "CVSSv31" assert rating["score"] == 7.5 + + +def test_sarif_marks_a_suppressed_result_rather_than_dropping_it() -> None: + """Dropped, GitHub would close the alert as fixed. Marked, it shows as dismissed.""" + document = sarif.to_dict(sample_report()) + result = result_for(document, "GHSA-cccc") + assert result["suppressions"] == [ + { + "kind": "external", + "status": "accepted", + "justification": "Unreachable: the affected parser is never called", + } + ] + + +def test_sarif_omits_suppressions_entirely_when_nothing_was_accepted() -> None: + assert "suppressions" not in result_for(sarif.to_dict(sample_report()), "GHSA-aaaa") + + +def test_cyclonedx_records_an_accepted_finding_without_claiming_it_is_unaffected() -> ( + None +): + """``state`` is a closed enum of security claims we cannot make from free text.""" + document = cyclonedx.to_dict(sample_report(), include_vulnerabilities=True) + entry = next(v for v in document["vulnerabilities"] if v["id"] == "GHSA-cccc") + assert "Unreachable" in entry["analysis"]["detail"] + assert "state" not in entry["analysis"] + + properties = {p["name"]: p["value"] for p in document["metadata"]["properties"]} + assert properties["icebergsca:suppressedCount"] == "1" diff --git a/tests/test_skill.py b/tests/test_skill.py index 6928c4e..d6544e8 100644 --- a/tests/test_skill.py +++ b/tests/test_skill.py @@ -107,6 +107,9 @@ def test_skill_name_matches_the_distribution() -> None: "vulnerabilities_checked", "unchecked_packages", "--format json", + # An accepted finding read as a fixed one is the second way to produce a + # false clean, so it belongs in the same guard as the first. + "suppressed", ], ) def test_skill_teaches_the_false_clean_checks(claim: str) -> None: diff --git a/tests/test_triage.py b/tests/test_triage.py new file mode 100644 index 0000000..71a526c --- /dev/null +++ b/tests/test_triage.py @@ -0,0 +1,247 @@ +"""The ignore file. + +The negative assertions carry the weight here. This is the one feature whose purpose is +to make findings disappear, so a rule that reaches further than it was written to reach +hides a real vulnerability — the failure this codebase is otherwise arranged to make +impossible. +""" + +from __future__ import annotations + +from datetime import date, timedelta + +import pytest + +from icebergsca.core.errors import ConfigError +from icebergsca.core.models import ( + Advisory, + EcosystemId, + Finding, + PackageRef, + Severity, + SeverityLevel, +) +from icebergsca.core.triage import DEFAULT_EXPIRY_DAYS, apply, parse_ignore_file + +TODAY = date(2026, 7, 29) + + +def finding( + advisory_id: str = "GHSA-aaaa", + name: str = "pyyaml", + *, + aliases: tuple[str, ...] = (), + ecosystem: EcosystemId = EcosystemId.PYPI, + version: str = "5.1", +) -> Finding: + return Finding( + package=PackageRef(ecosystem, name, version), + advisory=Advisory( + id=advisory_id, + modified="2026-01-01T00:00:00Z", + aliases=aliases, + severity=Severity(SeverityLevel.HIGH, 7.5), + ), + ) + + +def entry(**overrides: str) -> str: + fields = { + "advisory": "GHSA-aaaa", + "package": "pyyaml", + "reason": "Unreachable from our code paths", + } | overrides + body = "\n".join(f'{key} = "{value}"' for key, value in fields.items()) + return f"[[ignore]]\n{body}\n" + + +def rules(text: str, *, today: date = TODAY) -> tuple: + return parse_ignore_file(".icebergsca.toml", text, today=today) + + +# --------------------------------------------------------------------------- +# Parsing +# --------------------------------------------------------------------------- + + +def test_a_minimal_entry_parses() -> None: + (rule,) = rules(entry()) + assert rule.advisory == "GHSA-aaaa" + assert rule.package == "pyyaml" + assert rule.reason == "Unreachable from our code paths" + + +def test_no_ignore_file_content_is_no_rules() -> None: + assert rules("") == () + + +def test_expiry_defaults_rather_than_being_absent() -> None: + """An ignore file with no dates becomes permanent by accident. This is the guard.""" + (rule,) = rules(entry()) + assert rule.expires == TODAY + timedelta(days=DEFAULT_EXPIRY_DAYS) + + +def test_an_explicit_expiry_is_honoured() -> None: + (rule,) = rules(entry() + 'expires = "2026-12-25"\n') + assert rule.expires == date(2026, 12, 25) + + +def test_a_native_toml_date_is_accepted_as_well_as_a_quoted_one() -> None: + """Quoted and unquoted look identical in an editor; refusing one teaches nothing.""" + (rule,) = rules(entry() + "expires = 2026-12-25\n") + assert rule.expires == date(2026, 12, 25) + + +@pytest.mark.parametrize("missing", ["advisory", "package", "reason"]) +def test_a_missing_required_field_is_an_error(missing: str) -> None: + fields = { + "advisory": "GHSA-aaaa", + "package": "pyyaml", + "reason": "because", + } + del fields[missing] + text = "[[ignore]]\n" + "\n".join(f'{k} = "{v}"' for k, v in fields.items()) + with pytest.raises(ConfigError, match=missing): + rules(text) + + +def test_an_empty_reason_is_an_error() -> None: + """An entry with no justification is indistinguishable from a mistake later.""" + with pytest.raises(ConfigError, match="reason"): + rules(entry(reason=" ")) + + +def test_an_unknown_key_is_an_error_rather_than_being_skipped() -> None: + """The deliberate divergence from every other parser here. + + A misspelled key in a third-party manifest should be skipped. A misspelled key in + the file that decides which vulnerabilities you stop seeing should not. + """ + with pytest.raises(ConfigError, match="advisroy"): + rules('[[ignore]]\nadvisroy = "GHSA-aaaa"\npackage = "p"\nreason = "r"\n') + + +def test_an_unparseable_expiry_is_an_error() -> None: + with pytest.raises(ConfigError, match="expires"): + rules(entry() + 'expires = "next tuesday"\n') + + +def test_invalid_toml_names_the_file() -> None: + with pytest.raises(ConfigError, match="invalid TOML"): + rules("[[ignore]\n") + + +def test_a_duplicate_entry_is_an_error() -> None: + """Two entries for the same pair mean one silently does nothing.""" + with pytest.raises(ConfigError, match="duplicate"): + rules(entry() + entry()) + + +def test_ignore_must_be_a_list_of_tables() -> None: + with pytest.raises(ConfigError, match="list"): + rules('ignore = "GHSA-aaaa"\n') + + +# --------------------------------------------------------------------------- +# Matching — the half that must never over-reach +# --------------------------------------------------------------------------- + + +def test_a_matching_rule_suppresses() -> None: + marked, warnings = apply((finding(),), rules(entry()), today=TODAY) + assert marked[0].is_suppressed + assert marked[0].suppression is not None + assert marked[0].suppression.reason == "Unreachable from our code paths" + assert warnings == () + + +def test_a_cve_alias_matches_the_ghsa_it_belongs_to() -> None: + """Users are sent CVE numbers; OSV answers in GHSA. Both must work.""" + target = finding("GHSA-aaaa", aliases=("CVE-2026-1234",)) + marked, _ = apply((target,), rules(entry(advisory="CVE-2026-1234")), today=TODAY) + assert marked[0].is_suppressed + + +def test_an_unversioned_purl_matches_as_well_as_a_bare_name() -> None: + marked, _ = apply( + (finding(),), rules(entry(package="pkg:pypi/pyyaml")), today=TODAY + ) + assert marked[0].is_suppressed + + +def test_a_versioned_purl_pins_the_decision_to_that_version() -> None: + """Writing the version means the acceptance does not survive an upgrade.""" + accepted, _ = apply( + (finding(),), rules(entry(package="pkg:pypi/pyyaml@5.1")), today=TODAY + ) + assert accepted[0].is_suppressed + + upgraded, _ = apply( + (finding(version="5.4"),), + rules(entry(package="pkg:pypi/pyyaml@5.1")), + today=TODAY, + ) + assert not upgraded[0].is_suppressed + + +def test_a_rule_does_not_reach_a_longer_advisory_id() -> None: + """``GHSA-aaaa`` must not swallow ``GHSA-aaaa-bbbb``. No substrings, ever.""" + marked, _ = apply((finding("GHSA-aaaa-bbbb"),), rules(entry()), today=TODAY) + assert not marked[0].is_suppressed + + +def test_a_rule_does_not_reach_a_longer_package_name() -> None: + marked, _ = apply((finding(name="pyyaml-extra"),), rules(entry()), today=TODAY) + assert not marked[0].is_suppressed + + +def test_a_rule_for_one_package_does_not_suppress_the_same_advisory_elsewhere() -> None: + """The same CVE often affects several packages. A rule accepts one of them.""" + same_advisory = finding("GHSA-aaaa", name="other-package") + marked, _ = apply((finding(), same_advisory), rules(entry()), today=TODAY) + assert marked[0].is_suppressed + assert not marked[1].is_suppressed + + +def test_a_rule_for_one_advisory_does_not_suppress_another_on_the_same_package() -> ( + None +): + marked, _ = apply((finding("GHSA-bbbb"),), rules(entry()), today=TODAY) + assert not marked[0].is_suppressed + + +def test_matching_is_case_insensitive_on_the_advisory_id() -> None: + marked, _ = apply((finding(),), rules(entry(advisory="ghsa-aaaa")), today=TODAY) + assert marked[0].is_suppressed + + +# --------------------------------------------------------------------------- +# Rules that do no work +# --------------------------------------------------------------------------- + + +def test_an_expired_entry_stops_suppressing_and_says_so() -> None: + """Expiry surfaces the finding again. It never fails the scan.""" + text = entry() + 'expires = "2026-01-01"\n' + marked, warnings = apply((finding(),), rules(text), today=TODAY) + assert not marked[0].is_suppressed + assert any("expired" in w for w in warnings) + + +def test_an_entry_expiring_today_still_applies() -> None: + text = entry() + f'expires = "{TODAY.isoformat()}"\n' + marked, _ = apply((finding(),), rules(text), today=TODAY) + assert marked[0].is_suppressed + + +def test_a_rule_matching_nothing_is_reported() -> None: + """A typo suppresses nothing and looks exactly like a rule that is working.""" + _, warnings = apply((finding("GHSA-bbbb"),), rules(entry()), today=TODAY) + assert any("matched no finding" in w for w in warnings) + + +def test_no_rules_means_no_warnings_and_no_changes() -> None: + original = (finding(),) + marked, warnings = apply(original, (), today=TODAY) + assert marked == original + assert warnings == () diff --git a/website/docs/cli.md b/website/docs/cli.md index 78e6073..d3f20e5 100644 --- a/website/docs/cli.md +++ b/website/docs/cli.md @@ -50,6 +50,29 @@ icebergsca scan ./requirements.txt # a single file Logs go to stderr and the report to stdout, so piping the report is always safe. +### Gating and triage + +| Flag | Effect | +|---|---| +| `--fail-on ` | Exit `3` when an unsuppressed finding is at or above this severity. An incomplete scan still exits `1`, whatever it found | +| `--ignore-file ` | Accepted findings. Defaults to `.icebergsca.toml` beside the scan target when it exists | + +`.icebergsca.toml` records findings somebody has assessed and accepted, so that gating is safe +to leave switched on rather than being disabled the first week an unfixable advisory appears: + +```toml +[[ignore]] +advisory = "GHSA-6757-jp84-gxfx" # or the CVE — aliases are matched too +package = "pyyaml" # bare name, or a purl, optionally versioned +reason = "Unreachable: we never call yaml.load on untrusted input" +expires = 2026-10-27 # optional; defaults to 90 days out +``` + +`reason` is required, matching is exact rather than pattern-based, and an entry that expires +stops suppressing rather than failing the scan. Nothing is hidden: the finding stays in the +report marked `accepted`, `summary.suppressed` counts it, and SARIF reports it as dismissed +rather than fixed. A rule matching nothing produces a warning. + ### Network and cache | Flag | Effect | @@ -94,5 +117,7 @@ pure waste. | `0` | Scan completed. Vulnerabilities may have been found — that is not an error | | `1` | Scan failed, or completed only partially | | `2` | Usage error: bad flag, unknown format | +| `3` | Only with `--fail-on`: an unsuppressed finding met the threshold | -Findings never change the exit code. See [Output and CI](output.md#exit-codes). +Findings never change the exit code unless you ask with `--fail-on`, and even then +under a code of their own. See [Output and CI](output.md#exit-codes). diff --git a/website/docs/output.md b/website/docs/output.md index 733ab41..2774e4d 100644 --- a/website/docs/output.md +++ b/website/docs/output.md @@ -29,16 +29,17 @@ icebergsca scan . --format json --output report.json ```json { - "schema_version": "1.0", + "schema_version": "1.1", "tool": { "name": "icebergsca", "version": "0.1.0" }, "scan": { "root": "...", "status": "ok", "vulnerabilities_checked": true, "complete": true }, - "summary": { "dependencies": 21, "packages": 21, "findings": 12, + "summary": { "dependencies": 21, "packages": 21, "findings": 12, "suppressed": 2, "by_severity": { "critical": 3, "high": 4, "medium": 4, "low": 1 }, "unchecked_packages": 0, "skipped_files": 0 }, "manifests": [ ... ], "dependencies": [ ... ], "findings": [ ... ], + "suppressed": [ ... ], "unchecked_packages": [ ... ], "skipped": [ ... ], "warnings": [ ... ] @@ -58,6 +59,10 @@ Three things are easy to get wrong: - **`dependencies[].exclusions` explains an absence.** A coordinate listed there was removed from the graph on purpose, which is the one case where something missing is not something overlooked. +- **`summary.findings` counts only what is outstanding.** Findings accepted in + the ignore file are counted by `summary.suppressed` and still appear in the + `findings` array, each with a `suppression` object. An accepted finding is a + decision, not a fix. The last three arrays — `unchecked_packages`, `skipped` and `warnings` — are the report's account of what it could not do. They are the difference between @@ -112,10 +117,13 @@ one as evidence that anything was checked. | `0` | Scan completed. Vulnerabilities may have been found — that is not an error | Read `summary.findings` to decide what to do | | `1` | Scan failed, or completed only partially — some files or packages went unchecked | Fail the job; the result is not trustworthy | | `2` | Usage error: bad flag, unknown format (Click's reserved code) | Fix the invocation | +| `3` | `--fail-on` threshold met by an unsuppressed finding | Fail the job; the scan worked, the result did not pass | -Findings deliberately never change the exit code. A scanner that exits non-zero -on findings sooner or later gets `|| true` appended, at which point genuine tool -failures go unnoticed too. +Findings never change the exit code unless you ask for it with `--fail-on`, and +even then under a code of their own. A scanner that exits non-zero on findings by +default sooner or later gets `|| true` appended, at which point genuine tool +failures go unnoticed too — and a pipeline that cannot tell `1` from `3` ends up +treating both as noise. !!! note "Exit 2 is usage, exit 1 is scan failure" @@ -124,10 +132,15 @@ failures go unnoticed too. ## Gating a build on severity -A built-in `--fail-on ` is planned; the report model already carries -everything it needs. Until then, read the JSON — and check `scan.complete` -**first**, because gating on findings alone would pass a scan that failed to -check anything: +```bash +icebergsca scan . --fail-on critical +``` + +The gate checks completeness before severity, so it cannot pass a scan that failed +to look anything up, and it ignores findings accepted in `.icebergsca.toml`. + +For anything more specific, read the JSON — and check `scan.complete` **first**, +because gating on findings alone would pass a scan that failed to check anything: ```bash icebergsca scan . --format json --output report.json