diff --git a/CHANGELOG.md b/CHANGELOG.md index 810aabd4..f16c39ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## 2.12.1 + +### Fixed: duplicate findings with `--strict-blocking` + +- Alerts on added packages are no longer also reported as existing alerts. Under + `--strict-blocking`, each finding on an added package was listed twice in JSON, + SARIF, Slack and GitLab output, and counted again in the summary's + "EXISTING blocking issues" line and the GitLab commit status. The exit code + was not affected. + ## 2.12.0 ### Added diff --git a/pyproject.toml b/pyproject.toml index b62bd60f..3ee6508e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.12.0" +version = "2.12.1" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 203f24cf..01e27579 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,4 +1,4 @@ __author__ = 'socket.dev' -__version__ = '2.12.0' +__version__ = '2.12.1' USER_AGENT_NAME = 'socket-python-cli' USER_AGENT = f'{USER_AGENT_NAME}/{__version__}' diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index f78f6c19..305d6309 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -2480,11 +2480,7 @@ def create_new_diff( external_href=external_href, ) - # Separate unchanged packages from added/removed for --strict-blocking support - unchanged_packages = { - pkg_id: pkg for pkg_id, pkg in packages.items() - if pkg_id not in added_packages and pkg_id not in removed_packages - } + unchanged_packages = Core.get_unchanged_packages(packages, added_packages, removed_packages) diff = self.create_diff_report(added_packages, removed_packages, unchanged_packages) diff.packages = packages @@ -2509,6 +2505,24 @@ def create_new_diff( return diff + @staticmethod + def get_unchanged_packages( + packages: Dict[str, Package], + added_packages: Dict[str, Package], + removed_packages: Dict[str, Package], + ) -> Dict[str, Package]: + """Return the packages in ``packages`` that were neither added nor removed. + + ``packages`` is keyed by ``type/purl`` while the added and removed dicts are + keyed by artifact ID, so membership is checked on ``Package.id``. The result + is keyed by artifact ID to match the other two, which is what + ``get_source_data`` expects when it resolves ``topLevelAncestors``. + """ + return { + pkg.id: pkg for pkg in packages.values() + if pkg.id not in added_packages and pkg.id not in removed_packages + } + def create_diff_report( self, added_packages: Dict[str, Package], diff --git a/socketsecurity/core/alert_selection.py b/socketsecurity/core/alert_selection.py index 132be294..8c89c8b5 100644 --- a/socketsecurity/core/alert_selection.py +++ b/socketsecurity/core/alert_selection.py @@ -15,14 +15,29 @@ def select_diff_alerts(diff: Diff, strict_blocking: bool = False) -> List[Issue] """Select diff alerts for output rendering. In strict blocking mode, include unchanged alerts so rendered output aligns - with pass/fail policy evaluation. + with pass/fail policy evaluation. An unchanged alert that is already listed + as new (same ecosystem, purl and alert type) is skipped so each finding is + rendered once. The ecosystem is part of the key because ``Issue.purl`` holds + only name and version, so an npm and a PyPI package can share it. """ selected = list(getattr(diff, "new_alerts", []) or []) if strict_blocking: - selected.extend(getattr(diff, "unchanged_alerts", []) or []) + seen = {_alert_identity(alert) for alert in selected} - {None} + for alert in getattr(diff, "unchanged_alerts", []) or []: + identity = _alert_identity(alert) + if identity is None or identity not in seen: + selected.append(alert) + seen.add(identity) return selected +def _alert_identity(alert: Issue) -> Optional[Tuple[str, str, str]]: + purl = getattr(alert, "purl", "") or "" + if not purl: + return None + return (getattr(alert, "pkg_type", "") or "", purl, getattr(alert, "type", "") or "") + + def clone_diff_with_selected_alerts(diff: Diff, selected_alerts: List[Issue]) -> Diff: """Clone a diff object while replacing new_alerts with selected alerts.""" selected_diff = Diff( diff --git a/tests/core/test_strict_blocking_duplicates.py b/tests/core/test_strict_blocking_duplicates.py new file mode 100644 index 00000000..a37f67cc --- /dev/null +++ b/tests/core/test_strict_blocking_duplicates.py @@ -0,0 +1,213 @@ +"""`--strict-blocking` lists each finding once, whatever the baseline. + +Added packages must not also count as unchanged. An added package that leaks into +the unchanged set has its alerts reported twice under `--strict-blocking`, once as +new and once as existing, doubling the JSON, SARIF, Slack and GitLab findings and +inflating the "EXISTING blocking issues" count in the summary. + +These tests run `Core.create_new_diff` against a mocked diff response, with the +scan upload mocked out, then check what the output layer renders. +""" +import json +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import Mock + +import pytest +from socketdev.fullscans import FullScanParams, StreamDiffResponse + +from socketsecurity.config import CliConfig +from socketsecurity.core import Core +from socketsecurity.core.alert_selection import select_diff_alerts +from socketsecurity.core.classes import Diff, Issue +from socketsecurity.core.socket_config import SocketConfig +from socketsecurity.output import OutputHandler + +BUCKETS = ("added", "removed", "unchanged", "replaced", "updated") + + +@pytest.fixture +def core(mock_sdk_with_responses): + core = Core(config=SocketConfig(api_key="test_key"), sdk=mock_sdk_with_responses) + # Force the streaming diff so the mocked response below is the artifact source. + core.sdk.diffscans.create_from_ids.side_effect = Exception("diff-scans unavailable") + core.sdk.fullscans.stream_diff.side_effect = None + return core + + +def _artifact(diff_type, artifact_id, name, alerts=None, direct=True, ancestors=None): + ref = { + "direct": direct, + "manifestFiles": [{"file": "package.json", "start": 0, "end": 0}] if direct else [], + "topLevelAncestors": ancestors or [], + } + return { + "diffType": diff_type, + "type": "npm", + "name": name, + "version": "1.0.0", + "id": artifact_id, + "head": [] if diff_type in ("removed", "replaced") else [ref], + "base": [ref] if diff_type in ("removed", "replaced") else [], + "license": "MIT", + "licenseDetails": [], + "author": [], + "size": 1, + "score": {}, + "alerts": alerts or [], + } + + +def _blocking_alert(key, alert_type="malware"): + return { + "key": key, + "type": alert_type, + "severity": "critical", + "category": "supplyChainRisk", + "props": {}, + "action": "error", + } + + +def _diff_response(**buckets): + raw = json.loads((Path(__file__).parent.parent / "data/fullscans/diff/stream_diff.json").read_text()) + artifacts = {name: buckets.get(name, []) for name in BUCKETS} + return StreamDiffResponse.from_dict({ + "success": raw["success"], + "status": raw["status"], + "data": {**raw["data"], "artifacts": artifacts}, + }) + + +def _build_diff(core, response): + """Run Core.create_new_diff with the upload mocked out and `response` as the diff.""" + core.sdk.fullscans.stream_diff.return_value = response + core.resolve_base_full_scan_id = Mock(return_value="head") + core.create_full_scan = Mock(return_value=SimpleNamespace(id="new")) + params = FullScanParams(repo="repo", branch="feature") + params.include_license_details = True + return core.create_new_diff( + ["/repo"], params, explicit_files=["/repo/package.json"], + ) + + +def _handler(strict_blocking=True): + config = Mock(spec=CliConfig) + config.disable_blocking = False + config.strict_blocking = strict_blocking + config.legal_format = "socket" + config.legal = False + config.repo = None + config.branch = None + config.commit_sha = None + return OutputHandler(config, Mock()) + + +def test_empty_baseline_lists_added_alerts_once(core): + """Against an empty baseline every package is added and none is unchanged.""" + diff = _build_diff(core, _diff_response(added=[ + _artifact("added", "a1", "alpha", [_blocking_alert("k1")]), + _artifact("added", "a2", "beta", [_blocking_alert("k2")]), + ])) + + assert len(diff.new_alerts) == 2 + assert diff.unchanged_alerts == [] + + handler = _handler() + report = handler.build_json_report(diff) + assert len(report["new_alerts"]) == 2 + assert not handler.report_pass(diff) + + summary = handler.build_summary_text(diff) + assert "NEW blocking issues: 2" in summary + assert "EXISTING blocking issues" not in summary + + +def test_real_baseline_lists_new_and_existing_alerts_once_each(core): + """With a baseline, added alerts are new and unchanged alerts are existing.""" + diff = _build_diff(core, _diff_response( + added=[_artifact("added", "a1", "alpha", [_blocking_alert("k1")])], + unchanged=[_artifact("unchanged", "u1", "gamma", [_blocking_alert("k3")])], + )) + + assert [a.pkg_name for a in diff.new_alerts] == ["alpha"] + assert [a.pkg_name for a in diff.unchanged_alerts] == ["gamma"] + + handler = _handler() + report = handler.build_json_report(diff) + assert sorted(a["pkg_name"] for a in report["new_alerts"]) == ["alpha", "gamma"] + assert not handler.report_pass(diff) + + summary = handler.build_summary_text(diff) + assert "NEW blocking issues: 1" in summary + assert "EXISTING blocking issues: 1" in summary + + +def test_real_baseline_fails_on_existing_alerts_alone(core): + """An unchanged blocking alert fails the run when the API returns it.""" + diff = _build_diff(core, _diff_response( + unchanged=[_artifact("unchanged", "u1", "gamma", [_blocking_alert("k3")])], + )) + + assert diff.new_alerts == [] + assert not _handler(strict_blocking=True).report_pass(diff) + assert _handler(strict_blocking=False).report_pass(diff) + + +def test_without_strict_blocking_output_is_unchanged(core): + diff = _build_diff(core, _diff_response( + added=[_artifact("added", "a1", "alpha", [_blocking_alert("k1")])], + unchanged=[_artifact("unchanged", "u1", "gamma", [_blocking_alert("k3")])], + )) + + report = _handler(strict_blocking=False).build_json_report(diff) + assert [a["pkg_name"] for a in report["new_alerts"]] == ["alpha"] + + +def test_unchanged_packages_are_keyed_by_artifact_id(core): + """Transitive unchanged alerts resolve their introducer through the ancestor ID.""" + diff = _build_diff(core, _diff_response(unchanged=[ + _artifact("unchanged", "top", "parent"), + _artifact( + "unchanged", "dep", "child", [_blocking_alert("k5")], + direct=False, ancestors=["top"], + ), + ])) + + assert diff.unchanged_alerts[0].introduced_by == [("npm/parent@1.0.0", "package.json")] + + +def _issue(name, alert_type="malware", pkg_type="npm"): + return Issue( + pkg_type=pkg_type, + pkg_name=name, + pkg_version="1.0.0", + type=alert_type, + purl=f"{name}@1.0.0", + error=True, + ) + + +def test_select_diff_alerts_skips_unchanged_alert_already_listed_as_new(): + diff = Diff() + diff.new_alerts = [_issue("alpha")] + diff.unchanged_alerts = [_issue("alpha"), _issue("alpha", "typosquat"), _issue("gamma")] + + selected = select_diff_alerts(diff, strict_blocking=True) + + assert [(a.pkg_name, a.type) for a in selected] == [ + ("alpha", "malware"), + ("alpha", "typosquat"), + ("gamma", "malware"), + ] + + +def test_select_diff_alerts_keeps_same_purl_from_another_ecosystem(): + """`Issue.purl` has no ecosystem, so npm and PyPI packages can share one.""" + diff = Diff() + diff.new_alerts = [_issue("alpha", pkg_type="npm")] + diff.unchanged_alerts = [_issue("alpha", pkg_type="pypi")] + + selected = select_diff_alerts(diff, strict_blocking=True) + + assert [a.pkg_type for a in selected] == ["npm", "pypi"] diff --git a/uv.lock b/uv.lock index dc276bb5..6077c132 100644 --- a/uv.lock +++ b/uv.lock @@ -1334,7 +1334,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.12.0" +version = "2.12.1" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" },