diff --git a/CHANGELOG.md b/CHANGELOG.md index 1526c36..4af999b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog +## 2.6.9 + +### Fixed: unreadable reachability facts no longer report a blocking package + +- Scans with no supported manifest files uploaded a zero-byte `.socket.facts.json` + placeholder. The API cannot parse that, and answers by adding a + `generic/invalid-socket-facts@1.0.0` artifact to the scan, which the CLI then reported + as a new blocking package with no manifest file and no introducing dependency — + failing the run and, on pull requests, leaving a security comment that could not be + acted on. The placeholder is now an empty but well-formed facts document. +- When the API does report `generic/invalid-socket-facts` (a diagnostic for a facts file + it could not parse, not a real dependency), the CLI now excludes it from scan results + and logs a warning instead. It no longer blocks a run, appears in reports, or triggers + a pull request comment. +- Each placeholder is written to its own temporary directory. Two CLI runs sharing a + temporary directory previously used the same path and could remove each other's + placeholder mid-upload. + ## 2.6.8 ### Changed: bump pinned @coana-tech/cli to 15.10.25 diff --git a/pyproject.toml b/pyproject.toml index f206c59..dad62f8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.6.8" +version = "2.6.9" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index cb041be..ba3db03 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.6.8' +__version__ = '2.6.9' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index daab5fd..2fc852f 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1,3 +1,4 @@ +import atexit import copy import fnmatch import importlib @@ -6,6 +7,7 @@ import os import random import re +import shutil import sys import tarfile import tempfile @@ -69,6 +71,19 @@ # Stream the facts file in 1 MiB chunks so large files aren't held fully in memory. SOCKET_FACTS_BROTLI_CHUNK_SIZE = 1024 * 1024 +# Minimal well-formed facts document used for placeholder uploads (see empty_head_scan_file). +# A zero-byte ``.socket.facts.json`` is not parseable, and the API answers an unparseable +# facts file by synthesising the marker artifact below. +SOCKET_FACTS_EMPTY_DOCUMENT = '{"components": []}' + +# Synthetic artifact the Socket API adds to a full scan when an uploaded +# ``.socket.facts.json`` could not be parsed. It is a diagnostic marker rather than a real +# dependency: it has no manifest file and no introducing package, so the blocking alert it +# carries is not actionable by a developer, and a PR comment about it is pure noise +# (CE-422). Drop it from scan results and surface the parse failure as a warning instead. +INVALID_FACTS_MARKER_TYPE = "generic" +INVALID_FACTS_MARKER_NAME = "invalid-socket-facts" + # Full application reachability finalize retry policy. The finalize call links the reachability # scan to the full scan and can fail transiently (network/API blips); a few backoff retries make it robust. TIER1_FINALIZE_MAX_ATTEMPTS = 3 @@ -108,6 +123,17 @@ DIFF_SCAN_POLL_BACKOFF_MULTIPLIER = 1.5 DIFF_SCAN_POLL_TIMEOUT_SECONDS = 30 * 60.0 +# Temp dirs holding placeholder facts files (see Core.empty_head_scan_file). Call sites unlink +# the file itself once the upload finishes; the now-empty directory is removed at process exit +# so a run that raises mid-scan doesn't leak one. +_PLACEHOLDER_FACTS_DIRS: List[str] = [] + + +@atexit.register +def _cleanup_placeholder_facts_dirs() -> None: + for placeholder_dir in _PLACEHOLDER_FACTS_DIRS: + shutil.rmtree(placeholder_dir, ignore_errors=True) + def _humanize_alert_type(alert_type: str) -> str: """Convert a camelCase/PascalCase alert type into a Title-Cased label. @@ -209,13 +235,58 @@ def get_sbom_data(self, full_scan_id: str) -> Dict[str, SocketArtifact]: ) if not hasattr(response, "artifacts") or not response.artifacts: return {} - return response.artifacts + artifacts = { + artifact_id: artifact + for artifact_id, artifact in response.artifacts.items() + if not Core.is_invalid_facts_marker(artifact) + } + Core.warn_if_invalid_facts_marker(len(artifacts) != len(response.artifacts)) + return artifacts def get_sbom_data_list(self, artifacts_dict: Dict[str, SocketArtifact]) -> list[SocketArtifact]: """Converts artifacts dictionary to a list.""" return list(artifacts_dict.values()) + @staticmethod + def is_invalid_facts_marker(artifact) -> bool: + """True for the API's ``generic/invalid-socket-facts`` unparseable-facts marker. + + The marker is a signal that the uploaded ``.socket.facts.json`` failed to parse, not a + dependency anyone added. Treating it as a package makes the CLI report a new blocking + alert with an empty "Introduced by" and "Manifest File" and post a PR comment a + developer has no way to act on (CE-422), so it is filtered out of scan results and + reported through ``warn_if_invalid_facts_marker`` instead. + Matches on any version: the API currently pins it to 1.0.0, but the version carries no + meaning here. + + Args: + artifact: A ``SocketArtifact`` or diff artifact (anything with ``type``/``name``). + + Returns: + True if the artifact is the marker rather than a real package. + """ + return ( + getattr(artifact, "type", None) == INVALID_FACTS_MARKER_TYPE + and getattr(artifact, "name", None) == INVALID_FACTS_MARKER_NAME + ) + + @staticmethod + def warn_if_invalid_facts_marker(found: bool) -> None: + """Log the reachability-facts parse failure that ``is_invalid_facts_marker`` stands for. + + Dropping the marker silently would hide a real (if non-blocking) problem: the scan ran + without the reachability data it was supposed to carry. + """ + if not found: + return + log.warning( + "Socket could not parse the uploaded .socket.facts.json, so reachability facts " + "were not applied to this scan. Ignoring the " + f"{INVALID_FACTS_MARKER_TYPE}/{INVALID_FACTS_MARKER_NAME} marker the API returns " + "for this: it is a diagnostic, not a dependency, so it does not block the build " + "or appear in reports. Scan results are otherwise unaffected." + ) def create_sbom_output(self, diff: Diff) -> dict: """Creates CycloneDX output for a given diff.""" @@ -809,20 +880,35 @@ def to_case_insensitive_regex(input_string: str) -> str: @staticmethod def empty_head_scan_file() -> List[str]: """ - Creates a temporary empty file for baseline scans when no head scan exists. - + Creates a temporary placeholder manifest for scans with no manifest files. + + Used both for baseline scans when a repository has no head scan yet and for the new + scan when no supported manifest files were found. The API rejects unsupported + filenames, so the placeholder is named ``.socket.facts.json``; it must therefore also + *parse* as a facts document. A zero-byte file does not, and the API answers an + unparseable facts file by adding a blocking ``generic/invalid-socket-facts@1.0.0`` + artifact to the scan - which then surfaces as a new blocking package with no manifest + and no introducer (CE-422). Writing an empty-but-well-formed document instead yields a + genuinely empty scan. + + Each call gets its own temp directory: the path used to be a fixed + ``$TMPDIR/.socket.facts.json``, so two CLI runs sharing a temp dir (back-to-back + invocations in the same CI job, matrix jobs on one runner) could delete or truncate + each other's placeholder mid-upload. + Returns: - List containing path to a temporary empty file + List containing path to a temporary placeholder facts file """ - # Create a temporary directory and then create our specific filename - temp_dir = tempfile.gettempdir() - temp_path = os.path.join(temp_dir, '.socket.facts.json') - - # Create the empty file - with open(temp_path, 'w'): - pass # Creates an empty file - - log.debug(f"Created temporary empty file for baseline scan: {temp_path}") + # Own directory per call so concurrent runs can't clobber each other's placeholder; + # the basename must stay exactly SOCKET_FACTS_FILENAME to pass the API's validator. + temp_dir = tempfile.mkdtemp(prefix='socket_baseline_') + _PLACEHOLDER_FACTS_DIRS.append(temp_dir) + temp_path = os.path.join(temp_dir, SOCKET_FACTS_FILENAME) + + with open(temp_path, 'w') as f: + f.write(SOCKET_FACTS_EMPTY_DOCUMENT) + + log.debug(f"Created temporary placeholder facts file for baseline scan: {temp_path}") return [temp_path] def finalize_tier1_scan(self, full_scan_id: str, facts_file_path: str) -> bool: @@ -959,7 +1045,7 @@ def _compress_facts_files_for_upload(self, files: List[str]) -> Tuple[List[str], exactly ``.socket.facts.json.br``, so compressing here keeps a large facts file under the server's per-file size cap without changing the stored result. Files whose basename is not exactly ``.socket.facts.json`` are left untouched (the server only - matches that exact name), as are empty placeholder files (e.g. baseline scans). + matches that exact name), as are zero-byte files. Compression never blocks an upload: if it fails for any reason (missing optional ``brotli`` dependency, unwritable directory, etc.) the original plain file is used. @@ -1780,16 +1866,26 @@ def get_added_and_removed_packages( diff_end = time.time() log.info(f"Diff Report Gathered in {diff_end - diff_start:.2f} seconds") + + # A scan whose facts file failed to parse carries the API's invalid-socket-facts + # marker. Left in, it reads as a newly added blocking package (CE-422), so drop it + # from every bucket - before the counts below, which should describe what the CLI + # actually reports on. + marker_found = False + buckets: Dict[str, List] = {} + for name in ("added", "removed", "unchanged", "replaced", "updated"): + bucket = getattr(diff_artifacts, name) + buckets[name] = [a for a in bucket if not Core.is_invalid_facts_marker(a)] + marker_found = marker_found or len(buckets[name]) != len(bucket) + Core.warn_if_invalid_facts_marker(marker_found) + log.info("Diff report artifact counts:") - log.info(f"Added: {len(diff_artifacts.added)}") - log.info(f"Removed: {len(diff_artifacts.removed)}") - log.info(f"Unchanged: {len(diff_artifacts.unchanged)}") - log.info(f"Replaced: {len(diff_artifacts.replaced)}") - log.info(f"Updated: {len(diff_artifacts.updated)}") - - added_artifacts = diff_artifacts.added + diff_artifacts.updated - removed_artifacts = diff_artifacts.removed + diff_artifacts.replaced - unchanged_artifacts = diff_artifacts.unchanged + for name, bucket in buckets.items(): + log.info(f"{name.capitalize()}: {len(bucket)}") + + added_artifacts = buckets["added"] + buckets["updated"] + removed_artifacts = buckets["removed"] + buckets["replaced"] + unchanged_artifacts = buckets["unchanged"] added_packages: Dict[str, Package] = {} removed_packages: Dict[str, Package] = {} diff --git a/tests/core/test_facts_compression.py b/tests/core/test_facts_compression.py index ba04efa..0e71cdc 100644 --- a/tests/core/test_facts_compression.py +++ b/tests/core/test_facts_compression.py @@ -99,7 +99,7 @@ def test_compress_for_upload_preserves_directory_prefix(tmp_path): def test_empty_facts_file_is_not_compressed(tmp_path): - """Empty placeholder facts files (e.g. baseline scans) are uploaded as-is.""" + """A zero-byte facts file has nothing to compress and is uploaded as-is.""" core = Core.__new__(Core) empty_facts = _write(str(tmp_path / SOCKET_FACTS_FILENAME), b"") diff --git a/tests/core/test_invalid_facts_marker.py b/tests/core/test_invalid_facts_marker.py new file mode 100644 index 0000000..1a44c93 --- /dev/null +++ b/tests/core/test_invalid_facts_marker.py @@ -0,0 +1,280 @@ +"""Tests for CE-422: an unparseable `.socket.facts.json` must not block or comment. + +Two things go wrong when the Socket API cannot parse an uploaded facts file. It answers by +adding a synthetic `generic/invalid-socket-facts@1.0.0` artifact carrying a blocking alert, +which the CLI then reports as a newly added blocking package with no manifest and no +introducer - failing the build and leaving a PR comment a developer cannot action. And the +CLI was handing the API an unparseable facts file itself: the placeholder it uploads for +scans with no manifest files was zero bytes. + +These tests cover both the placeholder (`empty_head_scan_file`) and the marker filtering +that keeps the API's diagnostic out of scan results. +""" +import copy +import json +import os + +import pytest +from socketdev.fullscans import FullScanStreamResponse, StreamDiffResponse + +from socketsecurity.core import ( + INVALID_FACTS_MARKER_NAME, + INVALID_FACTS_MARKER_TYPE, + SOCKET_FACTS_FILENAME, + Core, +) +from socketsecurity.core.socket_config import SocketConfig + + +@pytest.fixture +def core(mock_sdk_with_responses): + return Core(config=SocketConfig(api_key="test_key"), sdk=mock_sdk_with_responses) + + +def make_marker_artifact(diff_type="added", artifact_id="invalid-facts-1"): + """The artifact the API returns for an unparseable facts file (see CE-422 report).""" + return { + "diffType": diff_type, + "type": INVALID_FACTS_MARKER_TYPE, + "name": INVALID_FACTS_MARKER_NAME, + "version": "1.0.0", + "id": artifact_id, + "direct": True, + "manifestFiles": [], + "topLevelAncestors": [], + "license": "", + "licenseDetails": [], + "author": [], + "size": 0, + "score": { + "supplyChain": 0, + "quality": 0, + "maintenance": 0, + "vulnerability": 0, + "license": 0, + "overall": 0, + }, + "scores": { + "supplyChain": 0, + "quality": 0, + "maintenance": 0, + "vulnerability": 0, + "license": 0, + "overall": 0, + }, + "alerts": [ + { + "key": "invalid_facts_alert_1", + "type": "generic", + "severity": "high", + "category": "supplyChainRisk", + "action": "error", + } + ], + } + + +# --- The placeholder the CLI uploads ---------------------------------------------------- + + +def test_empty_head_scan_file_is_parseable_json(): + """The placeholder must parse as a facts document, or the API answers with the marker. + + A zero-byte file (the old behaviour) is what produced the invalid-socket-facts artifact + in the first place. + """ + (path,) = Core.empty_head_scan_file() + + assert os.path.basename(path) == SOCKET_FACTS_FILENAME, ( + "the API rejects unsupported filenames, so the placeholder basename is load-bearing" + ) + with open(path) as f: + assert json.load(f) == {"components": []} + + +def test_empty_head_scan_file_is_unique_per_call(): + """Concurrent runs must not share one placeholder path. + + The path used to be a fixed `$TMPDIR/.socket.facts.json`, so two CLI invocations sharing + a temp dir could delete or truncate each other's placeholder mid-upload. + """ + (first,) = Core.empty_head_scan_file() + (second,) = Core.empty_head_scan_file() + + assert first != second + # Deleting one (what the call sites do after upload) leaves the other intact. + os.unlink(first) + assert os.path.isfile(second) + + +# --- The marker predicate --------------------------------------------------------------- + + +class FakeArtifact: + def __init__(self, type, name): + self.type = type + self.name = name + + +@pytest.mark.parametrize( + "artifact_type,artifact_name,expected", + [ + (INVALID_FACTS_MARKER_TYPE, INVALID_FACTS_MARKER_NAME, True), + ("pypi", "requests", False), + # A real generic package, and a same-named package from another ecosystem, are both + # ordinary dependencies - only the exact type+name pair is the API's marker. + (INVALID_FACTS_MARKER_TYPE, "some-tarball", False), + ("npm", INVALID_FACTS_MARKER_NAME, False), + ], +) +def test_is_invalid_facts_marker(artifact_type, artifact_name, expected): + assert Core.is_invalid_facts_marker(FakeArtifact(artifact_type, artifact_name)) is expected + + +def test_is_invalid_facts_marker_ignores_version(): + """The API pins the marker to 1.0.0 today, but the version carries no meaning.""" + + class Versioned(FakeArtifact): + version = "9.9.9" + + assert Core.is_invalid_facts_marker( + Versioned(INVALID_FACTS_MARKER_TYPE, INVALID_FACTS_MARKER_NAME) + ) + + +# --- Filtering: full-scan SBOM path ------------------------------------------------------- + + +def test_get_sbom_data_drops_marker(core, data_dir, load_json, caplog): + """The marker never reaches packages built from a full scan's SBOM.""" + json_data = load_json(data_dir / "fullscans" / "head_scan" / "stream_scan.json") + artifacts = copy.deepcopy(json_data["artifacts"]) + artifacts["invalid-facts-1"] = make_marker_artifact() + core.sdk.fullscans.stream.side_effect = None + core.sdk.fullscans.stream.return_value = FullScanStreamResponse.from_dict({ + "success": True, + "status": 200, + "artifacts": artifacts, + }) + + with caplog.at_level("WARNING"): + result = core.get_sbom_data("head") + + assert "invalid-facts-1" not in result + assert len(result) == len(json_data["artifacts"]) + assert "could not parse the uploaded .socket.facts.json" in caplog.text + + +def test_get_sbom_data_does_not_warn_without_marker(core, caplog): + """A clean scan produces no facts-parse warning.""" + with caplog.at_level("WARNING"): + core.get_sbom_data("head") + + assert "could not parse the uploaded .socket.facts.json" not in caplog.text + + +# --- Filtering: diff path (the flow that produced the customer's PR comment) -------------- + + +def _diff_response_with_marker(data_dir, load_json, buckets=("added",)): + json_data = load_json(data_dir / "fullscans" / "diff" / "stream_diff.json") + artifacts = copy.deepcopy(json_data["data"]["artifacts"]) + for index, bucket in enumerate(buckets): + artifacts[bucket].append( + make_marker_artifact(diff_type=bucket, artifact_id=f"invalid-facts-{index}") + ) + return StreamDiffResponse.from_dict({ + "success": json_data["success"], + "status": json_data["status"], + "data": {**json_data["data"], "artifacts": artifacts}, + }) + + +def test_diff_drops_marker_from_added_packages(core, data_dir, load_json, caplog): + """An added marker yields no package and no blocking alert (the CE-422 regression). + + Left in, it surfaces as `NEW blocking issues: 1` and a PR comment for a package the + developer never added. + """ + core.sdk.fullscans.stream_diff.side_effect = None + core.sdk.fullscans.stream_diff.return_value = _diff_response_with_marker( + data_dir, load_json + ) + # Force the legacy streaming diff so the fixture above is the artifact source. + core.sdk.diffscans.create_from_ids.side_effect = Exception("diff-scans unavailable") + + with caplog.at_level("WARNING"): + added, removed, packages = core.get_added_and_removed_packages("head", "new") + + assert not any( + pkg.name == INVALID_FACTS_MARKER_NAME + for pkg in list(added.values()) + list(removed.values()) + list(packages.values()) + ) + diff = core.create_diff_report(added, removed) + assert not any(alert.pkg_name == INVALID_FACTS_MARKER_NAME for alert in diff.new_alerts) + assert "could not parse the uploaded .socket.facts.json" in caplog.text + + +def test_diff_drops_marker_from_every_bucket(core, data_dir, load_json): + """Removed and unchanged markers are dropped too. + + An unchanged marker would otherwise become an existing violation under + --strict-blocking, and a removed one would show up as a resolved alert. + """ + core.sdk.fullscans.stream_diff.side_effect = None + core.sdk.fullscans.stream_diff.return_value = _diff_response_with_marker( + data_dir, load_json, buckets=("added", "removed", "unchanged") + ) + core.sdk.diffscans.create_from_ids.side_effect = Exception("diff-scans unavailable") + + added, removed, packages = core.get_added_and_removed_packages("head", "new") + + assert not any( + pkg.name == INVALID_FACTS_MARKER_NAME + for pkg in list(added.values()) + list(removed.values()) + list(packages.values()) + ) + + +def test_diff_artifact_counts_exclude_marker(core, data_dir, load_json, caplog): + """The logged counts describe what the CLI reports on, not the raw API response. + + "Added: 1" in a run whose only added artifact was the marker is what sent the CE-422 + reporter looking for a package that was never there. + """ + core.sdk.fullscans.stream_diff.side_effect = None + core.sdk.fullscans.stream_diff.return_value = _diff_response_with_marker( + data_dir, load_json + ) + core.sdk.diffscans.create_from_ids.side_effect = Exception("diff-scans unavailable") + unfiltered_added = len( + load_json(data_dir / "fullscans" / "diff" / "stream_diff.json")["data"]["artifacts"][ + "added" + ] + ) + + with caplog.at_level("INFO"): + core.get_added_and_removed_packages("head", "new") + + assert f"Added: {unfiltered_added}" in caplog.text + assert f"Added: {unfiltered_added + 1}" not in caplog.text + + +def test_diff_keeps_real_packages(core, data_dir, load_json): + """Filtering the marker leaves genuine packages untouched.""" + core.sdk.fullscans.stream_diff.side_effect = None + unfiltered = load_json(data_dir / "fullscans" / "diff" / "stream_diff.json") + core.sdk.fullscans.stream_diff.return_value = _diff_response_with_marker( + data_dir, load_json + ) + core.sdk.diffscans.create_from_ids.side_effect = Exception("diff-scans unavailable") + + added, removed, _ = core.get_added_and_removed_packages("head", "new") + + expected_added = len(unfiltered["data"]["artifacts"]["added"]) + len( + unfiltered["data"]["artifacts"]["updated"] + ) + expected_removed = len(unfiltered["data"]["artifacts"]["removed"]) + len( + unfiltered["data"]["artifacts"]["replaced"] + ) + assert len(added) == expected_added + assert len(removed) == expected_removed diff --git a/uv.lock b/uv.lock index a1b3704..5845345 100644 --- a/uv.lock +++ b/uv.lock @@ -1282,7 +1282,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.6.8" +version = "2.6.9" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" },