diff --git a/CHANGELOG.md b/CHANGELOG.md index b6eebb26..8716b1c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,31 @@ # Changelog +## 2.8.2 + +### Fixed: GitLab report serialization and workspace baselines + +- Full-scan package identities and Socket links now preserve namespaced packages + when the SDK returns enum-backed ecosystem values. +- Namespaced package links separate the namespace from the name instead of + concatenating them, so Maven links no longer fuse groupId and artifactId into a + single unresolvable path segment. A namespaced package whose namespace is + missing now logs a warning rather than emitting a broken link silently. +- GitLab dependency-scanning reports emit CVE and GHSA identifiers from current + API fields while remaining compatible with legacy CVE data. +- GitLab report findings record the manifest they came from when the package's + introducing chain is unavailable, instead of reporting the location as + `unknown`, and report whether a dependency is direct from the package record + rather than inferring it from a dependency-path string that is never produced. +- `--base-commit-sha` degrades to the nearest scanned ancestor of the requested + commit instead of failing the run, and logs which commit was used and how far + back it is. Squash merges, rebases, and multi-commit pushes all leave a merge + base unscanned even when default-branch scanning is configured correctly. The + lookup follows paginated scan history and the run still fails when no scanned + ancestor is reachable or the exact-commit lookup itself fails. +- Implicit diff baselines are selected from the same workspace, scan type, + repository, and default branch, including when no workspace is supplied. A + baseline lookup that fails is reported as an API error instead of resolving to + an empty baseline, and temporary scans are skipped when selecting one. ## 2.8.1 ### Changed: bump pinned @coana-tech/cli to 15.10.40 diff --git a/README.md b/README.md index 5f978d59..5221daf1 100644 --- a/README.md +++ b/README.md @@ -44,21 +44,22 @@ socketcli --enable-gitlab-security --gitlab-security-file gl-dependency-scanning ### PR scan diffed against the merge base -By default, PR scans are diffed against the repository's latest head scan. To diff against -the exact commit your PR branched from instead, pass the merge base as the baseline: +By default, PR scans are diffed against the repository's latest matching head scan. To +prefer the commit your PR branched from as the baseline, pass the merge base: ```bash BASE_SHA=$(git merge-base origin/main HEAD) socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" ``` -> **Requirement:** `--base-commit-sha` only works if Socket already has a full scan for that -> exact commit. In practice this means your CI must run `socketcli` on **every commit that -> lands on your default branch** — not just some of them. If merges can land without a scan -> (skipped/canceled builds, `[skip ci]`, path-filtered pipelines), the PR scan will fail with -> exit code 3 rather than silently diff against the wrong baseline. See +> The CLI uses the exact commit's newest matching full scan when one exists. Otherwise, it +> searches up to 100 first-parent commits in the local checkout and uses the nearest scanned +> ancestor, with a warning that the diff is wider than the merge base. Run `socketcli` +> regularly on your default branch and ensure PR checkouts contain enough history for that +> walk. The run fails with the configured API-error exit code only when no scanned ancestor +> is reachable (or when the scan lookup itself fails). See > [`docs/cli-reference.md`](https://github.com/SocketDev/socket-python-cli/blob/main/docs/cli-reference.md) -> for the full requirements and a backfill pattern that makes PR jobs self-sufficient. +> for the full behavior and an optional exact-baseline backfill pattern. A specific full scan ID also works: `--base-scan-id `. diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 968799b5..2051a691 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -347,11 +347,14 @@ generator rather than a static YAML file: BASE_SHA=$(git merge-base "origin/${TARGET}" HEAD) ``` -- **Emit the backfill step conditionally from the generator.** The generator is the - natural place for the "does a baseline scan exist?" check +- **If an exact baseline is required, emit a backfill step conditionally from the + generator.** The generator is the natural place for the "does an exact baseline + scan exist?" check (`GET /orgs/{org}/full-scans?repo=&commit_hash=$BASE_SHA&per_page=1`): only emit the baseline-scan step when it returns nothing. The emitted pipeline then shows - in the UI whether a backfill will run. + in the UI whether a backfill will run. Without a backfill, the CLI automatically + uses the nearest scanned first-parent ancestor within 100 commits and warns that + the diff is wider. - **Keep the backfill inside one command step.** The checkout-base → scan → checkout-PR sequence must not be split across steps — steps can land on different @@ -360,17 +363,18 @@ generator rather than a static YAML file: checkout: `git worktree add /tmp/socket-base "$BASE_SHA"` then `socketcli --target-path /tmp/socket-base --branch "$TARGET" --disable-blocking`. -- **Soft-fail infra errors, not findings.** A missing baseline (or any API error) - exits with code 3 (`--exit-code-on-api-error` to change it); real findings exit 1. +- **Soft-fail infra errors, not findings.** No reachable scanned ancestor (or any API + error) exits with code 3 (`--exit-code-on-api-error` to change it); real findings exit 1. [`soft_fail: [{exit_status: 3}]`](https://buildkite.com/docs/pipelines/configure/step-types/command-step) on the PR scan step keeps infra errors from blocking merges while security findings still do. - **["Cancel intermediate builds"](https://buildkite.com/docs/pipelines/configure/canceling-builds#cancel-running-intermediate-builds) - on the default branch is the main source of baseline gaps.** Canceled builds never - scan their commit, so merge-base lookups for PRs based on those commits fail. The - conditional backfill step above is the remedy; there is no per-step exemption from - build cancellation in Buildkite. If you need strict scan-once semantics for + on the default branch is a common source of exact-baseline gaps.** Canceled builds + never scan their commit, so these PRs fall back to an older scanned ancestor. Use + the conditional backfill step above when an exact merge-base comparison is required; + there is no per-step exemption from build cancellation in Buildkite. If you need + strict scan-once semantics for concurrent backfills of the same merge base, serialize the backfill step with a [concurrency group](https://buildkite.com/docs/pipelines/configure/workflows/controlling-concurrency) keyed on the merge-base SHA. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index f64de267..d0b61ce5 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -256,24 +256,22 @@ If you don't want to provide the Socket API Token every time then you can use th | `--commit-message` | False | *auto* | Commit message (auto-detected from git) | | `--commit-sha` | False | *auto* | Commit SHA (auto-detected from git) | | `--base-scan-id` | False | | Full scan ID to diff against, overriding the repository's head scan as the baseline. Mutually exclusive with `--base-commit-sha` | -| `--base-commit-sha`| False | | Commit SHA to diff against, overriding the repository's head scan as the baseline. The most recent full scan for that commit is used; the CLI errors (exit code 3, or `--exit-code-on-api-error`) if no scan exists for it. Mutually exclusive with `--base-scan-id` | +| `--base-commit-sha`| False | | Commit SHA to prefer as the diff baseline, overriding the repository's head scan. The CLI uses its most recent matching full scan or the nearest scanned first-parent ancestor within 100 local commits. It errors (exit code 3, or `--exit-code-on-api-error`) if no scanned ancestor is reachable. Mutually exclusive with `--base-scan-id` | -> **Diffing against the merge base** — by default, PR scans are diffed against the repository's *latest* head scan, which may include newer default-branch commits than your PR branched from. To diff against the exact commit your PR is based on, compute the merge base and pass it as the baseline: +> **Diffing against the merge base** — by default, PR scans are diffed against the repository's latest matching head scan, which may include newer default-branch commits than your PR branched from. To prefer the commit your PR is based on, compute the merge base and pass it as the baseline: > > ```shell > BASE_SHA=$(git merge-base origin/main HEAD) > socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" > ``` > -> **Requirement: a full scan must already exist for the merge-base commit.** `--base-commit-sha` does not create a scan of that commit; it looks up an existing one. That lookup only succeeds if your CI runs `socketcli` on **every commit that lands on your default branch** — every merge and direct push, not just periodic or latest-only scans. Common ways commits slip through without a scan: +> `--base-commit-sha` does not create a scan of that commit. The CLI first looks for the newest non-temporary scan matching the repository, workspace, scan type, and exact commit. If the exact commit was not scanned, it walks up to 100 first-parent commits from that SHA in the local checkout and uses the nearest matching scanned ancestor. It logs a warning with the selected commit and distance because this produces a wider diff than the merge base. > -> - CI settings that cancel or skip intermediate builds when newer commits land (e.g. Buildkite's ["cancel intermediate builds"](https://buildkite.com/docs/pipelines/configure/canceling-builds#cancel-running-intermediate-builds)) -> - `[skip ci]` commits, path-filtered pipelines, or failed/canceled scan steps -> - merge-base commits that predate your Socket rollout +> Run `socketcli` regularly on the default branch so recent ancestors have scans. PR checkouts must also retain the merge base and enough first-parent history; shallow clones can shorten the search. Gaps are expected when CI cancels intermediate builds, commits use `[skip ci]`, pipelines are path-filtered, or the merge base predates your Socket rollout. > -> If no scan exists for the commit, the CLI **fails** (exit code 3, or your `--exit-code-on-api-error` value; exit 0 with `--disable-blocking`) instead of silently falling back to the head scan — a wrong baseline would misreport which alerts the PR introduces. Don't adopt this flag without default-branch scan coverage in place; you'll fail PR builds on lookup misses. +> If no scanned ancestor is reachable within the local 100-commit walk, the CLI **fails** (exit code 3, or your `--exit-code-on-api-error` value; exit 0 with `--disable-blocking`) instead of silently falling back to the repository head. API or permission failures also fail rather than being treated as a missing exact scan. > -> **Backfill pattern** — if your default-branch coverage has gaps, the PR job can create the missing baseline itself before scanning: +> **Optional exact-baseline backfill** — if the wider ancestor fallback is not acceptable, the PR job can create the missing exact baseline before scanning: > > ```shell > BASE_SHA=$(git merge-base origin/main HEAD) @@ -285,7 +283,7 @@ If you don't want to provide the Socket API Token every time then you can use th > socketcli --pr-number 123 --base-commit-sha "$BASE_SHA" > ``` > -> Run the baseline step with `--disable-blocking` (findings on the default branch must not fail the PR job) and an explicit `--branch`, since branch auto-detection is unreliable at a detached HEAD. +> Run the baseline step with `--disable-blocking` (findings on the default branch must not fail the PR job) and an explicit `--branch`, since branch auto-detection is unreliable at a detached HEAD. Without this step, the CLI automatically uses the nearest scanned ancestor. > > Buildkite users with dynamically generated pipelines: see [Merge-base baselines in Buildkite](ci-cd.md#merge-base-baselines-in-buildkite-dynamic-pipelines) for generation-time vs. step-time guidance. diff --git a/pyproject.toml b/pyproject.toml index 33d2d4df..5f812f5f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.8.1" +version = "2.8.2" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 6cf31cd7..29fd46e0 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.8.1' +__version__ = '2.8.2' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/config.py b/socketsecurity/config.py index 35904976..ecd2f287 100644 --- a/socketsecurity/config.py +++ b/socketsecurity/config.py @@ -587,9 +587,9 @@ def create_argument_parser() -> argparse.ArgumentParser: metavar="", default=None, help="Commit SHA to diff the new scan against, overriding the repository's head " - "scan as the baseline. The most recent full scan matching this commit (e.g. " - "the merge base from 'git merge-base origin/main HEAD') is used; the CLI " - "errors if no scan exists for it. Mutually exclusive with --base-scan-id." + "scan as the baseline. The CLI uses the most recent matching full scan, or " + "the nearest scanned first-parent ancestor within 100 local commits when " + "the commit itself was not scanned. Mutually exclusive with --base-scan-id." ) # Path and File options diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index b1b1d65b..c5f3f52c 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -14,10 +14,11 @@ import time from dataclasses import asdict from pathlib import PurePath -from typing import TYPE_CHECKING, Dict, List, NamedTuple, Optional, Set, Tuple +from typing import TYPE_CHECKING, Dict, Iterator, List, NamedTuple, Optional, Set, Tuple if TYPE_CHECKING: from socketsecurity.config import CliConfig +from git import Repo from socketdev import socketdev from socketdev.exceptions import APIFailure from socketdev.fullscans import DiffArtifacts, FullScanParams, SocketArtifact @@ -51,6 +52,17 @@ _HUMANIZE_BOUNDARY = re.compile(r"(?<=[a-z0-9])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])") +# How many full scans to request when resolving a diff baseline. The newest scan is +# usually the one we want, but temporary scans have to be skipped (see +# Core.newest_persisted_scan_id), so a single result is not enough. +SCAN_LOOKUP_PAGE_SIZE = 10 + +# Bounds on the search for a scanned ancestor when the requested baseline commit has +# no full scan of its own. Full-scan listing pages are matched against bounded local +# history, so the local walk cannot grow without limit. +ANCESTOR_SCAN_LOOKUP_LIMIT = 100 +ANCESTOR_WALK_MAX_DEPTH = 100 + # Reachability facts-file upload compression. # # The Socket full-scan endpoint transparently brotli-decompresses any multipart part @@ -1463,19 +1475,229 @@ def get_repo_info(self, repo_slug: str, default_branch: str = "socket-default-br return response.data - def get_head_scan_for_repo(self, repo_slug: str) -> str: + def get_head_scan_for_repo( + self, + repo_slug: str, + workspace: Optional[str] = None, + scan_type: Optional[str] = None, + ) -> Optional[str]: """ Gets the head scan ID for a repository. + Without a workspace or scan type this is the repository's head scan pointer. + That pointer tracks a single scan for the whole repository rather than one per + workspace or scan type, so scoped runs instead take the newest matching scan + on the default branch. + Args: repo_slug: Repository slug to get head scan for + workspace: Socket workspace the scan belongs to, if any + scan_type: Socket scan type to match, if any Returns: Head scan ID if it exists, None otherwise + + Raises: + APIFailure: If the scoped scan lookup fails. A failed lookup must not + be reported as "no baseline": the caller answers that by creating an + empty baseline scan, which reports every dependency in the repository + as newly added. """ repo_info = self.get_repo_info(repo_slug) + if workspace or scan_type: + query_params = { + "repo": repo_slug, + "branch": repo_info.default_branch, + "sort": "created_at", + "direction": "desc", + "per_page": SCAN_LOOKUP_PAGE_SIZE, + } + if workspace: + query_params["workspace"] = workspace + if scan_type: + query_params["scan_type"] = Core.query_param_value(scan_type) + for results in self._full_scan_result_pages( + query_params, + f"Failed to list matching full scans for repo {repo_slug}", + ): + scan_id = Core.newest_persisted_scan_id(results) + if scan_id: + return scan_id + return None return repo_info.head_full_scan_id if repo_info.head_full_scan_id else None + @staticmethod + def query_param_value(value): + """ + Unwraps an enum member so it survives URL encoding. + + The SDK types several query params as str-backed enums (ScanType, + IntegrationType). urlencode calls str(), which renders a (str, Enum) member + as "ScanType.SOCKET_TIER1" -- a filter value the API does not recognize. + """ + return getattr(value, "value", value) + + @staticmethod + def newest_persisted_scan_id(results: List[dict]) -> Optional[str]: + """ + Returns the newest scan ID from a full scan listing, skipping temporary scans. + + create_new_diff creates an empty ``tmp`` scan when a repository has no baseline + yet, and that scan inherits the branch and commit of the run that created it. + If the real scan then fails, the empty scan is left behind as the newest scan + for that branch/commit; selecting it as a baseline would report every + dependency as newly added. + + Args: + results: Full scan listing results, newest first + + Returns: + Newest non-temporary scan ID, or None if the listing has none + """ + for result in results or []: + if not isinstance(result, dict) or result.get("tmp"): + continue + scan_id = result.get("id") + if scan_id: + return scan_id + return None + + def _full_scan_result_pages( + self, + query_params: dict, + failure_message: str, + ) -> Iterator[List[dict]]: + """Yields successful full-scan listing pages and rejects failed lookups.""" + request_params = dict(query_params) + seen_pages = {str(request_params.get("page", 1))} + per_page = int(request_params.get("per_page", 30)) + + while True: + response = self.sdk.fullscans.get(self.config.org_slug, request_params) + results = response.get("results") if isinstance(response, dict) else None + if results is None: + # The SDK logs and returns {} for any non-200, so a present results + # key is the only signal that the request itself succeeded. + raise APIFailure(failure_message) + yield results + + next_page = response.get("nextPage") + # The API has historically returned nextPage=1 for a short final page. + if len(results) < per_page or next_page in (None, 0, "0", False, ""): + return + + page_key = str(next_page) + if page_key in seen_pages: + raise APIFailure( + f"{failure_message}: full-scan pagination repeated page {next_page}" + ) + seen_pages.add(page_key) + request_params = {**query_params, "page": next_page} + + def first_parent_commits(self, start_commit_sha: str, max_count: int) -> List[str]: + """ + Lists a commit and its first-parent ancestors, newest first. + + Follows only first parents so a merge commit contributes the branch's own + history rather than everything merged into it. A shallow checkout simply + yields fewer commits, which narrows the search rather than failing it. + + Args: + start_commit_sha: Commit to walk back from, included in the result + max_count: Maximum number of commits to return + + Returns: + Commit SHAs, newest first. Empty when the repository or commit is + unavailable locally. + """ + target_path = self.cli_config.target_path if self.cli_config else None + if not target_path: + return [] + try: + repo = Repo(target_path) + output = repo.git.rev_list( + "--first-parent", + f"--max-count={max_count}", + start_commit_sha, + ) + except Exception as error: + log.debug(f"Unable to walk history back from {start_commit_sha}: {error}") + return [] + return [line.strip() for line in output.splitlines() if line.strip()] + + def find_baseline_scan_for_ancestor( + self, + repo_slug: str, + commit_sha: str, + workspace: Optional[str] = None, + scan_type: Optional[str] = None, + ) -> Tuple[Optional[str], Optional[str], int]: + """ + Finds the nearest ancestor of a commit that does have a full scan. + + Used when --base-commit-sha names a commit that was never scanned. Squash + merges and rebases rewrite commits, and a multi-commit push produces one scan + for the tip, so a merge base can be unscanned even when default-branch + scanning is configured correctly. Diffing against a slightly older ancestor + is a wider diff; failing outright is no diff at all. + + Scan listing pages are matched against local first-parent history. All pages + are considered because scans from other branches and reruns can fill newer + pages without covering the nearest candidate ancestors. + + Args: + repo_slug: Repository slug the scan belongs to + commit_sha: Commit that has no full scan of its own + workspace: Socket workspace the scan belongs to, if any + scan_type: Socket scan type to match, if any + + Returns: + (scan_id, ancestor_commit_sha, commits_back), or (None, None, 0) when no + scanned ancestor is reachable. + """ + ancestors = self.first_parent_commits(commit_sha, ANCESTOR_WALK_MAX_DEPTH) + if not ancestors: + return None, None, 0 + + query_params = { + "repo": repo_slug, + "sort": "created_at", + "direction": "desc", + "per_page": ANCESTOR_SCAN_LOOKUP_LIMIT, + } + if workspace: + query_params["workspace"] = workspace + if scan_type: + query_params["scan_type"] = Core.query_param_value(scan_type) + + scans_by_commit = {} + ancestor_set = set(ancestors) + for results in self._full_scan_result_pages( + query_params, + f"Failed to list ancestor full scans for repo {repo_slug}", + ): + for result in results: + if not isinstance(result, dict) or result.get("tmp"): + continue + result_commit = result.get("commit_hash") + scan_id = result.get("id") + # Newest first, so the first scan seen for a commit is the one to keep. + if ( + result_commit in ancestor_set + and scan_id + and result_commit not in scans_by_commit + ): + scans_by_commit[result_commit] = scan_id + + if not scans_by_commit: + return None, None, 0 + + for distance, ancestor in enumerate(ancestors): + scan_id = scans_by_commit.get(ancestor) + if scan_id: + return scan_id, ancestor, distance + return None, None, 0 + def get_full_scan_id_by_commit( self, repo_slug: str, @@ -1504,31 +1726,32 @@ def get_full_scan_id_by_commit( "commit_hash": commit_sha, "sort": "created_at", "direction": "desc", - "per_page": 1, + "per_page": SCAN_LOOKUP_PAGE_SIZE, } if workspace: query_params["workspace"] = workspace if scan_type: - query_params["scan_type"] = scan_type + query_params["scan_type"] = Core.query_param_value(scan_type) - response = self.sdk.fullscans.get( - self.config.org_slug, - query_params, - ) - results = response.get("results") if isinstance(response, dict) else None - if not results: - return None - return results[0].get("id") + for results in self._full_scan_result_pages( + query_params, + f"Failed to list full scans for commit {commit_sha} in repo {repo_slug}", + ): + scan_id = Core.newest_persisted_scan_id(results) + if scan_id: + return scan_id + return None def resolve_base_full_scan_id(self, params: FullScanParams) -> Optional[str]: """ Resolves the baseline full scan ID to diff a new scan against. Priority: --base-scan-id (used verbatim), then --base-commit-sha (newest - full scan for that commit), then the repository's current head scan. A - --base-commit-sha with no matching full scan is a hard error rather than a - silent fallback to the head scan, because diffing against the wrong - baseline silently misreports which alerts a PR introduces. + full scan for that commit, or its nearest scanned first-parent ancestor), + then the repository's current matching head scan. A --base-commit-sha with + no reachable scanned ancestor is a hard error rather than a silent fallback + to the head scan, because diffing against the wrong baseline silently + misreports which alerts a PR introduces. Returns: Full scan ID to use as the diff baseline, or None when the repository @@ -1549,26 +1772,61 @@ def resolve_base_full_scan_id(self, params: FullScanParams) -> Optional[str]: workspace=params.workspace, scan_type=params.scan_type, ) + baseline_source = "explicit-commit" + baseline_commit = commit_sha if scan_id is None: - log.error( - f"No full scan found for commit {commit_sha} in repo {params.repo} " - "(--base-commit-sha). Ensure a scan was created for that commit " - "(e.g. the CLI runs on default-branch pushes), or pass " - "--base-scan-id instead." + scan_id, ancestor_sha, commits_back = self.find_baseline_scan_for_ancestor( + params.repo, + commit_sha, + workspace=params.workspace, + scan_type=params.scan_type, ) - if self.cli_config.disable_blocking: - sys.exit(0) - sys.exit(self.cli_config.exit_code_on_api_error) + if scan_id: + baseline_source = "explicit-commit-ancestor" + baseline_commit = ancestor_sha + log.warning( + f"No full scan for commit {commit_sha} (--base-commit-sha). " + f"Diffing against its nearest scanned ancestor {ancestor_sha}, " + f"{commits_back} commit(s) earlier, so the diff is wider than " + "the merge base." + ) + else: + log.error( + f"No full scan found for commit {commit_sha} in repo {params.repo} " + "(--base-commit-sha), and no scanned ancestor within " + f"{ANCESTOR_WALK_MAX_DEPTH} commits of it. Ensure a scan was " + "created for that commit (e.g. the CLI runs on default-branch " + "pushes), or pass --base-scan-id instead." + ) + if self.cli_config.disable_blocking: + sys.exit(0) + sys.exit(self.cli_config.exit_code_on_api_error) log.info( - "Baseline selected: source=explicit-commit " - f"scan_id={json.dumps(scan_id)} commit={json.dumps(commit_sha)}" + f"Baseline selected: source={baseline_source} " + f"scan_id={json.dumps(scan_id)} commit={json.dumps(baseline_commit)}" ) return scan_id try: - scan_id = self.get_head_scan_for_repo(params.repo) + scan_id = self.get_head_scan_for_repo( + params.repo, + workspace=params.workspace, + scan_type=params.scan_type, + ) except APIResourceNotFound: return None + except APIFailure as error: + # Returning None would make the caller create an empty baseline scan, + # reporting every dependency as newly added, so fail loudly like the + # --base-commit-sha path above. + log.error( + f"Failed to resolve the matching head scan for repo {params.repo}: {error}" + ) + if self.cli_config is None: + raise + if self.cli_config.disable_blocking: + sys.exit(0) + sys.exit(self.cli_config.exit_code_on_api_error) if scan_id: log.info( "Baseline selected: source=repository-head " @@ -1578,12 +1836,11 @@ def resolve_base_full_scan_id(self, params: FullScanParams) -> Optional[str]: @staticmethod def update_package_values(pkg: Package) -> Package: + pkg.type = Package.normalize_type(pkg.type) pkg.purl = f"{pkg.name}@{pkg.version}" - pkg.url = f"https://socket.dev/{pkg.type}/package" if pkg.namespace: pkg.purl = f"{pkg.namespace}/{pkg.purl}" - pkg.url += f"/{pkg.namespace}" - pkg.url += f"/{pkg.name}/overview/{pkg.version}" + pkg.url = Package.socket_url(pkg.type, pkg.namespace, pkg.name, pkg.version) return pkg def get_license_text_via_purl(self, packages: dict[str, Package], batch_size: int = 5000) -> dict: @@ -2407,6 +2664,8 @@ def add_package_alerts_to_collection(self, package: Package, alerts_collection: suggestion=props.suggestion, next_step_title=props.nextStepTitle, introduced_by=introduced_by, + manifest_files=package.manifestFiles or [], + direct=bool(package.direct), purl=package.purl, url=package.url ) diff --git a/socketsecurity/core/classes.py b/socketsecurity/core/classes.py index db145221..2a19c2bb 100644 --- a/socketsecurity/core/classes.py +++ b/socketsecurity/core/classes.py @@ -1,4 +1,5 @@ import json +import logging from dataclasses import dataclass, field from typing import Dict, List, Optional, TypedDict @@ -11,6 +12,13 @@ SocketScore, ) +log = logging.getLogger("socketdev") + +# Ecosystems whose package pages cannot be addressed by name alone. A Maven +# coordinate is a groupId plus an artifactId; with no namespace the URL collapses to +# one path segment that cannot be split back into two, and the page does not resolve. +NAMESPACE_REQUIRED_TYPES = frozenset({"maven"}) + __all__ = [ "Report", "Score", @@ -142,6 +150,43 @@ class Package(): licenseAttrib: Optional[List] = None + @staticmethod + def normalize_type(package_type) -> str: + """ + Unwraps the SDK's str-backed SocketPURL_Type enum to its value. + + str(SocketPURL_Type.MAVEN) is "SocketPURL_Type.MAVEN", not "maven", so any + enum member reaching an f-string leaks the class name into user-facing output. + """ + return getattr(package_type, "value", package_type) + + @staticmethod + def socket_url(package_type, namespace: Optional[str], name: str, version: str) -> str: + """ + Builds the socket.dev package overview URL for a package. + + Namespace and name are separate path segments, the same form purl strings use. + + Args: + package_type: Ecosystem, as a string or SocketPURL_Type member + namespace: Package namespace (Maven groupId, npm scope), if any + name: Package name + version: Package version + + Returns: + Package overview URL on socket.dev + """ + package_type = Package.normalize_type(package_type) + namespace = (namespace or "").strip("/") + if not namespace and package_type in NAMESPACE_REQUIRED_TYPES: + # The link is still emitted so the finding reports, but it cannot resolve. + log.warning( + f"{package_type} package {name}@{version} has no namespace, so its " + "Socket link collapses to a single path segment and will not resolve" + ) + package_path = "/".join(part for part in (namespace, name) if part) + return f"https://socket.dev/{package_type}/package/{package_path}/overview/{version}" + @classmethod def from_socket_artifact(cls, data: dict) -> "Package": """ @@ -153,18 +198,16 @@ def from_socket_artifact(cls, data: dict) -> "Package": Returns: New Package instance """ - purl = f"{data['type']}/" - namespace = data.get("namespace") - if namespace: - purl += f"{namespace}@" - purl += f"{data['name']}@{data['version']}" - base_url = "https://socket.dev" - url = f"{base_url}/{data['type']}/package/{namespace or ''}{data['name']}/overview/{data['version']}" + package_type = Package.normalize_type(data["type"]) + namespace = (data.get("namespace") or "").strip("/") + package_path = "/".join(part for part in (namespace, data["name"]) if part) + purl = f"{package_type}/{package_path}@{data['version']}" + url = Package.socket_url(package_type, namespace, data["name"], data["version"]) return cls( id=data["id"], name=data["name"], version=data["version"], - type=data["type"], + type=package_type, release=data.get("release"), diffType=data.get("diffType"), score=data["score"], @@ -179,7 +222,7 @@ def from_socket_artifact(cls, data: dict) -> "Package": artifact=data.get("artifact"), purl=purl, url=url, - namespace=namespace + namespace=namespace or None ) @classmethod @@ -274,6 +317,11 @@ class Issue: manifests: str url: str purl: str + # The package's own manifest files, independent of how it was introduced. A + # transitive package whose ancestors are absent from the scan has no + # introduced_by chain, but its manifest is still known. + manifest_files: list + direct: bool def __init__(self, **kwargs): if kwargs: @@ -282,6 +330,10 @@ def __init__(self, **kwargs): if hasattr(self, "created_at"): self.created_at = self.created_at.strip(" (Coordinated Universal Time)") + if not hasattr(self, "manifest_files"): + self.manifest_files = [] + if not hasattr(self, "direct"): + self.direct = False if not hasattr(self, "manifests"): self.manifests = "" if not hasattr(self, "suggestion"): diff --git a/socketsecurity/core/messages.py b/socketsecurity/core/messages.py index d968c14b..14a6f829 100644 --- a/socketsecurity/core/messages.py +++ b/socketsecurity/core/messages.py @@ -648,32 +648,48 @@ def extract_identifiers_gitlab(alert: Issue) -> list: """ identifiers = [] - # Primary identifier: Socket alert type - identifiers.append({ + # Primary identifier: Socket alert type. The GitLab schema types identifier + # url as a string matching ^(https?|ftp)://, so an absent url is omitted + # rather than sent as null, which fails validation for the whole finding. + socket_identifier = { "type": "socket_alert", "name": f"Socket {alert.type}", "value": alert.type, - "url": alert.url if hasattr(alert, 'url') and alert.url else None - }) - - # Extract CVE identifiers from props - if hasattr(alert, 'props') and alert.props: - if 'cve' in alert.props: - cves = alert.props['cve'] - if isinstance(cves, list): - for cve in cves: - identifiers.append({ - "type": "cve", - "name": cve, - "value": cve, - "url": f"https://cve.mitre.org/cgi-bin/cvename.cgi?name={cve}" - }) - elif isinstance(cves, str): + } + alert_url = getattr(alert, "url", None) + if alert_url: + socket_identifier["url"] = alert_url + identifiers.append(socket_identifier) + + props = getattr(alert, "props", None) or {} + # Alerts reach Issue.props from several sources, so both spellings of each + # field are in play; core.alert_selection matches on the same pair. "cve" is + # the older spelling and still appears in some payloads. + identifier_fields = ( + (("cveId", "cve_id", "cve"), "cve", "https://nvd.nist.gov/vuln/detail/"), + (("ghsaId", "ghsa_id"), "ghsa", "https://github.com/advisories/"), + ) + seen = set() + for fields, identifier_type, url_prefix in identifier_fields: + for field in fields: + values = props.get(field) + if isinstance(values, str): + values = [values] + elif not isinstance(values, (list, tuple)): + continue + for value in values: + if not isinstance(value, str) or not value.strip(): + continue + value = value.strip() + identifier_key = (identifier_type, value.upper()) + if identifier_key in seen: + continue + seen.add(identifier_key) identifiers.append({ - "type": "cve", - "name": cves, - "value": cves, - "url": f"https://cve.mitre.org/cgi-bin/cvename.cgi?name={cves}" + "type": identifier_type, + "name": value, + "value": value, + "url": f"{url_prefix}{value}" }) return identifiers @@ -686,37 +702,35 @@ def extract_location_gitlab(alert: Issue) -> dict: GitLab location requires: - file: path to manifest file - dependency: package name and version - - dependency_path (optional): dependency chain """ - # Get manifest file from introduced_by or manifests attribute - manifest_file = "unknown" - dependency_path = [] - is_direct = True - - if hasattr(alert, 'introduced_by') and alert.introduced_by: - if isinstance(alert.introduced_by, list) and len(alert.introduced_by) > 0: - first_entry = alert.introduced_by[0] - if isinstance(first_entry, (list, tuple)) and len(first_entry) >= 2: - dependency_path_str = first_entry[0] - manifest_file = first_entry[1].split(';')[0] if ';' in first_entry[1] else first_entry[1] - - # Parse dependency path - if ' > ' in dependency_path_str: - dependency_path = dependency_path_str.split(' > ') - # If there's a chain, it's transitive (not direct) - is_direct = len(dependency_path) <= 1 - - elif hasattr(alert, 'manifests') and alert.manifests: - manifest_file = alert.manifests.split(';')[0] + manifest_file = "" + + introduced_by = getattr(alert, "introduced_by", None) + if isinstance(introduced_by, list) and introduced_by: + first_entry = introduced_by[0] + if isinstance(first_entry, (list, tuple)) and len(first_entry) >= 2: + manifest_file = (first_entry[1] or "").split(";")[0] + + if not manifest_file: + manifest_file = (getattr(alert, "manifests", "") or "").split(";")[0] + + if not manifest_file: + # A transitive package whose ancestors are not in this scan has no + # introduced_by chain, but the package still records its own manifest. + for entry in getattr(alert, "manifest_files", None) or []: + candidate = entry.get("file") if isinstance(entry, dict) else None + if candidate: + manifest_file = candidate + break location = { - "file": manifest_file, + "file": manifest_file or "unknown", "dependency": { "package": { "name": alert.pkg_name }, "version": alert.pkg_version, - "direct": is_direct + "direct": bool(getattr(alert, "direct", False)) } } diff --git a/tests/core/test_package_and_alerts.py b/tests/core/test_package_and_alerts.py index 171eae77..0e096621 100644 --- a/tests/core/test_package_and_alerts.py +++ b/tests/core/test_package_and_alerts.py @@ -1,8 +1,9 @@ -from dataclasses import dataclass +from dataclasses import asdict, dataclass from unittest.mock import Mock import pytest from socketdev import socketdev +from socketdev.fullscans import SocketArtifact from socketsecurity.core import Core, _humanize_alert_type from socketsecurity.core.classes import Issue, Package @@ -104,6 +105,93 @@ def test_create_packages_dict_basic(self, core): assert pkg.version == "1.0.0" assert pkg.transitives == 0 + def test_full_scan_package_normalizes_enum_type_and_namespace_url(self): + artifact = SocketArtifact.from_dict({ + "id": "pkg:maven/com.example/example-core@1.2.3", + "type": "maven", + "namespace": "com.example", + "name": "example-core", + "version": "1.2.3", + "direct": True, + "topLevelAncestors": [], + "manifestFiles": [{"file": "pom.xml"}], + "alerts": [], + }) + + package = Package.from_socket_artifact(asdict(artifact)) + + assert package.type == "maven" + assert package.purl == "maven/com.example/example-core@1.2.3" + assert package.url == ( + "https://socket.dev/maven/package/com.example/example-core/overview/1.2.3" + ) + + def test_maven_package_url_separates_group_and_artifact(self): + """groupId and artifactId are distinct path segments, not one fused string""" + artifact = SocketArtifact.from_dict({ + "id": "pkg:maven/org.apache.logging.log4j/log4j-api@2.17.2", + "type": "maven", + "namespace": "org.apache.logging.log4j", + "name": "log4j-api", + "version": "2.17.2", + "direct": True, + "topLevelAncestors": [], + "manifestFiles": [{"file": "pom.xml"}], + "alerts": [], + }) + + package = Package.from_socket_artifact(asdict(artifact)) + + assert package.url == ( + "https://socket.dev/maven/package/org.apache.logging.log4j/log4j-api" + "/overview/2.17.2" + ) + # The purl keeps the "/" form, which is what the purl spec and the purl API want. + assert package.purl == "maven/org.apache.logging.log4j/log4j-api@2.17.2" + + def test_non_maven_package_url_keeps_slash_separator(self): + """npm scopes and Go module paths stay slash-delimited""" + scoped_npm = Package.socket_url("npm", "@babel", "core", "7.0.0") + assert scoped_npm == "https://socket.dev/npm/package/@babel/core/overview/7.0.0" + + unscoped = Package.socket_url("nuget", None, "newtonsoft.json", "6.0.8") + assert unscoped == "https://socket.dev/nuget/package/newtonsoft.json/overview/6.0.8" + + def test_maven_package_without_namespace_warns(self, caplog): + """A Maven coordinate missing its groupId cannot produce a resolvable link""" + with caplog.at_level("WARNING", logger="socketdev"): + url = Package.socket_url("maven", None, "orphan-artifact", "1.0.0") + + assert url == "https://socket.dev/maven/package/orphan-artifact/overview/1.0.0" + assert "orphan-artifact@1.0.0" in caplog.text + assert "no namespace" in caplog.text + + def test_namespaced_maven_package_does_not_warn(self, caplog): + """The warning is for missing data, not for every Maven package""" + with caplog.at_level("WARNING", logger="socketdev"): + Package.socket_url("maven", "com.example", "artifact", "1.0.0") + + assert caplog.text == "" + + def test_diff_path_builds_the_same_maven_url_as_the_full_scan_path(self): + """Both package construction paths must agree, or links break on only some runs""" + package = Package( + id="pkg:maven/com.google.code.gson/gson@2.8.6", + type="maven", + name="gson", + version="2.8.6", + namespace="com.google.code.gson", + score={}, + alerts=[], + topLevelAncestors=[], + ) + + package = Core.update_package_values(package) + + assert package.url == ( + "https://socket.dev/maven/package/com.google.code.gson/gson/overview/2.8.6" + ) + def test_create_packages_dict_with_transitives(self, core): """Test package dictionary creation with transitive dependencies""" mock_artifacts = [ @@ -340,4 +428,3 @@ def test_empty_input_returns_empty_string(self): def test_handles_acronyms_conservatively(self): """Adjacent capitals are kept together: SQLInjection -> 'SQL Injection'.""" assert _humanize_alert_type("SQLInjection") == "SQL Injection" - diff --git a/tests/core/test_sdk_methods.py b/tests/core/test_sdk_methods.py index d79f62f3..bf439a3b 100644 --- a/tests/core/test_sdk_methods.py +++ b/tests/core/test_sdk_methods.py @@ -1,9 +1,9 @@ import pytest from socketdev.exceptions import APIFailure -from socketdev.fullscans import FullScanParams, FullScanStreamResponse +from socketdev.fullscans import FullScanParams, FullScanStreamResponse, ScanType from socketsecurity.config import CliConfig -from socketsecurity.core import Core +from socketsecurity.core import ANCESTOR_SCAN_LOOKUP_LIMIT, SCAN_LOOKUP_PAGE_SIZE, Core from socketsecurity.core.socket_config import SocketConfig @@ -63,6 +63,106 @@ def test_get_head_scan_for_repo_no_head(core, mock_sdk_with_responses): head_scan_id = core.get_head_scan_for_repo("no-head") assert head_scan_id is None + +def test_get_head_scan_for_repo_scopes_workspace_to_default_branch( + core, mock_sdk_with_responses +): + mock_sdk_with_responses.fullscans.get.return_value = { + "results": [{"id": "workspace-head"}], + "nextPage": None, + } + + head_scan_id = core.get_head_scan_for_repo( + "test", + workspace="customer-a", + scan_type="socket_tier1", + ) + + assert head_scan_id == "workspace-head" + mock_sdk_with_responses.fullscans.get.assert_called_once_with( + core.config.org_slug, + { + "repo": "test", + "workspace": "customer-a", + "branch": "main", + "sort": "created_at", + "direction": "desc", + "per_page": SCAN_LOOKUP_PAGE_SIZE, + "scan_type": "socket_tier1", + }, + ) + + +def test_get_head_scan_for_repo_scopes_scan_type_without_workspace( + core, mock_sdk_with_responses +): + """Reachability and standard scans must not share one unscoped head pointer""" + mock_sdk_with_responses.fullscans.get.return_value = { + "results": [{"id": "standard-head"}], + "nextPage": None, + } + + head_scan_id = core.get_head_scan_for_repo("test", scan_type="socket") + + assert head_scan_id == "standard-head" + mock_sdk_with_responses.fullscans.get.assert_called_once_with( + core.config.org_slug, + { + "repo": "test", + "branch": "main", + "sort": "created_at", + "direction": "desc", + "per_page": SCAN_LOOKUP_PAGE_SIZE, + "scan_type": "socket", + }, + ) + + +def test_get_head_scan_for_repo_workspace_lookup_failure_raises(core, mock_sdk_with_responses): + """A failed listing is not the same as an empty one and must not resolve to None""" + mock_sdk_with_responses.fullscans.get.return_value = {} + + with pytest.raises(APIFailure): + core.get_head_scan_for_repo("test", workspace="customer-a") + + +def test_get_head_scan_for_repo_workspace_no_scans_yet(core, mock_sdk_with_responses): + """An empty listing is a real answer: the workspace has no baseline yet""" + mock_sdk_with_responses.fullscans.get.return_value = {"results": [], "nextPage": None} + + assert core.get_head_scan_for_repo("test", workspace="customer-a") is None + + +def test_get_head_scan_for_repo_skips_temporary_scans(core, mock_sdk_with_responses): + """A leftover empty tmp scan must not be picked up as the baseline""" + mock_sdk_with_responses.fullscans.get.return_value = { + "results": [ + {"id": "leftover-tmp-scan", "tmp": True}, + {"id": "workspace-head", "tmp": False}, + ], + "nextPage": None, + } + + assert core.get_head_scan_for_repo("test", workspace="customer-a") == "workspace-head" + + +def test_get_head_scan_for_repo_normalizes_enum_scan_type(core, mock_sdk_with_responses): + """ScanType members must be sent as their value, not their repr-style name""" + mock_sdk_with_responses.fullscans.get.return_value = { + "results": [{"id": "workspace-head"}], + "nextPage": None, + } + + core.get_head_scan_for_repo( + "test", + workspace="customer-a", + scan_type=ScanType.SOCKET_TIER1, + ) + + query_params = mock_sdk_with_responses.fullscans.get.call_args.args[1] + assert query_params["scan_type"] == "socket_tier1" + + def test_get_full_scan_id_by_commit(core, mock_sdk_with_responses): """Looks up the newest full scan for a repo + commit via the list endpoint""" mock_sdk_with_responses.fullscans.get.return_value = { @@ -80,7 +180,7 @@ def test_get_full_scan_id_by_commit(core, mock_sdk_with_responses): "commit_hash": "abc123", "sort": "created_at", "direction": "desc", - "per_page": 1, + "per_page": SCAN_LOOKUP_PAGE_SIZE, }, ) @@ -107,24 +207,105 @@ def test_get_full_scan_id_by_commit_scopes_to_workspace_and_scan_type(core, mock "commit_hash": "abc123", "sort": "created_at", "direction": "desc", - "per_page": 1, + "per_page": SCAN_LOOKUP_PAGE_SIZE, "workspace": "customer-a", "scan_type": "socket_tier1", }, ) +def test_get_full_scan_id_by_commit_skips_temporary_scans(core, mock_sdk_with_responses): + """A tmp scan carries the commit hash of the run that created it, so skip it too""" + mock_sdk_with_responses.fullscans.get.return_value = { + "results": [ + {"id": "leftover-tmp-scan", "commit_hash": "abc123", "tmp": True}, + {"id": "base-scan-id", "commit_hash": "abc123"}, + ], + "nextPage": None, + } + + assert core.get_full_scan_id_by_commit("test", "abc123") == "base-scan-id" + + def test_get_full_scan_id_by_commit_not_found(core, mock_sdk_with_responses): - """No scan for the commit returns None (empty results and SDK error dict)""" + """A successful empty listing means the commit has no scan""" mock_sdk_with_responses.fullscans.get.return_value = {"results": [], "nextPage": None} assert core.get_full_scan_id_by_commit("test", "abc123") is None + +def test_get_full_scan_id_by_commit_lookup_failure_raises(core, mock_sdk_with_responses): + """An SDK error dict must not trigger fallback to an older ancestor""" mock_sdk_with_responses.fullscans.get.return_value = {} - assert core.get_full_scan_id_by_commit("test", "abc123") is None + with pytest.raises(APIFailure): + core.get_full_scan_id_by_commit("test", "abc123") + def test_resolve_base_full_scan_id_defaults_to_head_scan(core): - """Without base overrides the repository head scan is the baseline""" - assert core.resolve_base_full_scan_id(make_full_scan_params()) == "head" + """Without base overrides the matching scan type's head scan is the baseline""" + core.sdk.fullscans.get.return_value = { + "results": [{"id": "standard-head"}], + "nextPage": None, + } + + assert core.resolve_base_full_scan_id(make_full_scan_params()) == "standard-head" + + +def test_resolve_base_full_scan_id_scopes_head_to_workspace(core): + core.sdk.fullscans.get.return_value = { + "results": [{"id": "workspace-head"}], + "nextPage": None, + } + + params = make_full_scan_params(workspace="customer-a", scan_type="socket_tier1") + + assert core.resolve_base_full_scan_id(params) == "workspace-head" + core.sdk.fullscans.get.assert_called_once_with( + core.config.org_slug, + { + "repo": "test", + "workspace": "customer-a", + "branch": "main", + "sort": "created_at", + "direction": "desc", + "per_page": SCAN_LOOKUP_PAGE_SIZE, + "scan_type": "socket_tier1", + }, + ) + +def test_resolve_base_full_scan_id_workspace_lookup_failure_exits(core): + """A failed workspace lookup fails the run instead of diffing against an empty scan""" + core.cli_config = make_cli_config() + core.sdk.fullscans.get.return_value = {} + + params = make_full_scan_params(workspace="customer-a") + + with pytest.raises(SystemExit) as exc_info: + core.resolve_base_full_scan_id(params) + assert exc_info.value.code == core.cli_config.exit_code_on_api_error + + +def test_resolve_base_full_scan_id_workspace_lookup_failure_disable_blocking(core): + """--disable-blocking keeps the failed lookup from failing the build""" + core.cli_config = make_cli_config("--disable-blocking") + core.sdk.fullscans.get.return_value = {} + + params = make_full_scan_params(workspace="customer-a") + + with pytest.raises(SystemExit) as exc_info: + core.resolve_base_full_scan_id(params) + assert exc_info.value.code == 0 + + +def test_resolve_base_full_scan_id_workspace_lookup_failure_without_cli_config(core): + """Library callers with no CliConfig see the APIFailure rather than a process exit""" + core.cli_config = None + core.sdk.fullscans.get.return_value = {} + + params = make_full_scan_params(workspace="customer-a") + + with pytest.raises(APIFailure): + core.resolve_base_full_scan_id(params) + def test_resolve_base_full_scan_id_uses_base_scan_id(core): """--base-scan-id is used verbatim, without touching the repo endpoint""" @@ -152,12 +333,109 @@ def test_resolve_base_full_scan_id_uses_base_commit_sha(core): "commit_hash": "abc123", "sort": "created_at", "direction": "desc", - "per_page": 1, + "per_page": SCAN_LOOKUP_PAGE_SIZE, "workspace": "customer-a", "scan_type": "socket_tier1", }, ) +def test_resolve_base_full_scan_id_falls_back_to_scanned_ancestor(core, monkeypatch): + """An unscanned merge base degrades to the nearest scanned ancestor""" + core.cli_config = make_cli_config("--base-commit-sha", "unscanned-sha") + core.sdk.fullscans.get.side_effect = [ + {"results": [], "nextPage": None}, # exact commit + {"results": [ # recent scans + {"id": "tmp-scan", "commit_hash": "ancestor-1", "tmp": True}, + {"id": "ancestor-scan", "commit_hash": "ancestor-2"}, + ], "nextPage": None}, + ] + monkeypatch.setattr( + Core, "first_parent_commits", + lambda self, sha, depth: ["unscanned-sha", "ancestor-1", "ancestor-2"], + ) + + params = make_full_scan_params() + assert core.resolve_base_full_scan_id(params) == "ancestor-scan" + + +def test_find_baseline_scan_for_ancestor_paginates_and_selects_nearest( + core, monkeypatch +): + """Reruns can fill page one while a closer scanned ancestor is on page two""" + first_page = [ + {"id": "farther-scan", "commit_hash": "ancestor-2"}, + *[ + {"id": f"unrelated-{index}", "commit_hash": f"other-{index}"} + for index in range(ANCESTOR_SCAN_LOOKUP_LIMIT - 1) + ], + ] + core.sdk.fullscans.get.side_effect = [ + {"results": first_page, "nextPage": 2}, + { + "results": [{"id": "nearest-scan", "commit_hash": "ancestor-1"}], + "nextPage": 0, + }, + ] + monkeypatch.setattr( + Core, + "first_parent_commits", + lambda self, sha, depth: ["unscanned-sha", "ancestor-1", "ancestor-2"], + ) + + assert core.find_baseline_scan_for_ancestor( + "test", + "unscanned-sha", + scan_type="socket", + ) == ("nearest-scan", "ancestor-1", 1) + assert core.sdk.fullscans.get.call_args_list[1].args[1]["page"] == 2 + + +def test_resolve_base_full_scan_id_exact_lookup_failure_does_not_fallback( + core, monkeypatch +): + core.cli_config = make_cli_config("--base-commit-sha", "abc123") + core.sdk.fullscans.get.return_value = {} + fallback_calls = [] + monkeypatch.setattr( + Core, + "find_baseline_scan_for_ancestor", + lambda *args, **kwargs: fallback_calls.append((args, kwargs)), + ) + + with pytest.raises(APIFailure): + core.resolve_base_full_scan_id(make_full_scan_params()) + assert fallback_calls == [] + + +def test_resolve_base_full_scan_id_ancestor_fallback_skips_temporary_scans(core, monkeypatch): + """A tmp scan on an ancestor is not a usable baseline either""" + core.cli_config = make_cli_config("--base-commit-sha", "unscanned-sha") + core.sdk.fullscans.get.side_effect = [ + {"results": [], "nextPage": None}, + {"results": [{"id": "tmp-scan", "commit_hash": "ancestor-1", "tmp": True}], "nextPage": None}, + ] + monkeypatch.setattr( + Core, "first_parent_commits", + lambda self, sha, depth: ["unscanned-sha", "ancestor-1"], + ) + + with pytest.raises(SystemExit): + core.resolve_base_full_scan_id(make_full_scan_params()) + + +def test_resolve_base_full_scan_id_ancestor_fallback_needs_local_history(core, monkeypatch): + """Without local history there is nothing to match scans against""" + core.cli_config = make_cli_config("--base-commit-sha", "unscanned-sha") + core.sdk.fullscans.get.side_effect = [ + {"results": [], "nextPage": None}, + {"results": [{"id": "ancestor-scan", "commit_hash": "ancestor-2"}], "nextPage": None}, + ] + monkeypatch.setattr(Core, "first_parent_commits", lambda self, sha, depth: []) + + with pytest.raises(SystemExit): + core.resolve_base_full_scan_id(make_full_scan_params()) + + def test_resolve_base_full_scan_id_commit_sha_not_found_exits(core): """A --base-commit-sha with no scan is a hard error (exit_code_on_api_error)""" core.cli_config = make_cli_config("--base-commit-sha", "abc123") @@ -309,10 +587,18 @@ def test_empty_alerts_preserved(core): def test_repository_head_baseline_log(core, caplog): + core.sdk.fullscans.get.return_value = { + "results": [{"id": "standard-head"}], + "nextPage": None, + } + with caplog.at_level("INFO", logger="socketdev"): - assert core.resolve_base_full_scan_id(make_full_scan_params()) == "head" + assert core.resolve_base_full_scan_id(make_full_scan_params()) == "standard-head" - assert 'Baseline selected: source=repository-head scan_id="head"' in caplog.messages + assert ( + 'Baseline selected: source=repository-head scan_id="standard-head"' + in caplog.messages + ) def test_explicit_scan_baseline_log(core, caplog): diff --git a/tests/unit/test_gitlab_format.py b/tests/unit/test_gitlab_format.py index 4a1cf0c1..b817e341 100644 --- a/tests/unit/test_gitlab_format.py +++ b/tests/unit/test_gitlab_format.py @@ -86,7 +86,10 @@ def test_identifier_extraction_with_cve(self): type="vulnerability", severity="critical", title="Known CVE", - props={"cve": ["CVE-2024-5678", "CVE-2024-9012"]}, + props={ + "cveId": ["CVE-2024-5678", "CVE-2024-9012"], + "ghsaId": "GHSA-1234-5678-9012", + }, pkg_type="npm", key="test-key", purl="pkg:npm/vulnerable-pkg@2.0.0" @@ -96,15 +99,17 @@ def test_identifier_extraction_with_cve(self): report = Messages.create_security_comment_gitlab(diff) vuln = report["vulnerabilities"][0] - # Should have socket_alert identifier + 2 CVE identifiers - assert len(vuln["identifiers"]) >= 3 + # Should have socket_alert identifier + CVE and GHSA identifiers + assert len(vuln["identifiers"]) == 4 cve_identifiers = [i for i in vuln["identifiers"] if i["type"] == "cve"] assert len(cve_identifiers) == 2 assert any(i["value"] == "CVE-2024-5678" for i in cve_identifiers) assert any(i["value"] == "CVE-2024-9012" for i in cve_identifiers) + ghsa_identifiers = [i for i in vuln["identifiers"] if i["type"] == "ghsa"] + assert ghsa_identifiers[0]["value"] == "GHSA-1234-5678-9012" def test_identifier_extraction_with_single_cve_string(self): - """Test single CVE identifier as string""" + """Legacy CVE property remains supported""" diff = Diff() diff.id = "test-scan-id" diff.diff_url = "https://socket.dev/test" @@ -129,8 +134,63 @@ def test_identifier_extraction_with_single_cve_string(self): assert len(cve_identifiers) == 1 assert cve_identifiers[0]["value"] == "CVE-2024-1111" + def test_identifier_extraction_deduplicates_legacy_and_current_cve_fields(self): + issue = Issue( + pkg_name="vulnerable-pkg", + pkg_version="2.0.0", + type="vulnerability", + severity="high", + title="Duplicate CVE", + props={"cve": "CVE-2024-1111", "cveId": "CVE-2024-1111"}, + pkg_type="npm", + key="test-key", + purl="pkg:npm/vulnerable-pkg@2.0.0", + ) + + identifiers = Messages.extract_identifiers_gitlab(issue) + + assert [item["value"] for item in identifiers].count("CVE-2024-1111") == 1 + + def test_identifier_extraction_supports_snake_case_props(self): + """Alerts can reach Issue.props with snake_case vulnerability ids""" + issue = Issue( + pkg_name="vulnerable-pkg", + pkg_version="2.0.0", + type="vulnerability", + severity="high", + title="Snake case ids", + props={"cve_id": "CVE-2024-2222", "ghsa_id": "GHSA-2222-3333-4444"}, + pkg_type="npm", + key="test-key", + purl="pkg:npm/vulnerable-pkg@2.0.0", + ) + + identifiers = Messages.extract_identifiers_gitlab(issue) + + by_type = {item["type"]: item for item in identifiers} + assert by_type["cve"]["value"] == "CVE-2024-2222" + assert by_type["ghsa"]["value"] == "GHSA-2222-3333-4444" + + def test_identifier_extraction_ignores_unusable_prop_values(self): + """Malformed props must not take down the whole report""" + issue = Issue( + pkg_name="vulnerable-pkg", + pkg_version="2.0.0", + type="vulnerability", + severity="high", + title="Malformed props", + props={"cveId": 1234, "ghsaId": None}, + pkg_type="npm", + key="test-key", + purl="pkg:npm/vulnerable-pkg@2.0.0", + ) + + identifiers = Messages.extract_identifiers_gitlab(issue) + + assert [item["type"] for item in identifiers] == ["socket_alert"] + def test_dependency_chain_handling_transitive(self): - """Test transitive dependency path is captured""" + """Directness comes from the package record, not from parsing a path string""" diff = Diff() diff.id = "test-scan-id" diff.diff_url = "https://socket.dev/test" @@ -144,6 +204,7 @@ def test_dependency_chain_handling_transitive(self): introduced_by=[ ["top-level > intermediate > transitive-dep", "package.json"] ], + direct=False, pkg_type="npm", key="test-key", purl="pkg:npm/transitive-dep@1.5.0" @@ -171,6 +232,7 @@ def test_dependency_chain_handling_direct(self): introduced_by=[ ["direct-dep", "package.json"] ], + direct=True, pkg_type="npm", key="test-key", purl="pkg:npm/direct-dep@3.0.0" @@ -182,6 +244,62 @@ def test_dependency_chain_handling_direct(self): assert vuln["location"]["dependency"]["direct"] is True + def test_location_file_falls_back_to_the_package_manifest(self): + """A package with no introduced_by chain still knows its own manifest""" + issue = Issue( + pkg_name="transitive-dep", + pkg_version="1.5.0", + type="malware", + severity="critical", + title="Malware Found", + introduced_by=[], + manifest_files=[{"file": "services/api/pom.xml"}], + direct=False, + pkg_type="maven", + key="test-key", + purl="pkg:maven/org.example/transitive-dep@1.5.0", + ) + + location = Messages.extract_location_gitlab(issue) + + assert location["file"] == "services/api/pom.xml" + assert location["dependency"]["direct"] is False + + def test_location_file_is_unknown_only_when_nothing_is_known(self): + """The unknown placeholder is a last resort, not the first answer""" + issue = Issue( + pkg_name="orphan", + pkg_version="1.0.0", + type="malware", + severity="critical", + title="Malware Found", + introduced_by=[], + pkg_type="npm", + key="test-key", + purl="pkg:npm/orphan@1.0.0", + ) + + assert Messages.extract_location_gitlab(issue)["file"] == "unknown" + + def test_identifier_url_is_omitted_rather_than_null(self): + """GitLab types identifier url as a string; null fails schema validation""" + issue = Issue( + pkg_name="nourl-pkg", + pkg_version="1.0.0", + type="malware", + severity="critical", + title="Malware", + pkg_type="npm", + key="test-key", + purl="pkg:npm/nourl-pkg@1.0.0", + ) + + identifiers = Messages.extract_identifiers_gitlab(issue) + + # An absent key is correct; a present-but-null value is what breaks validation. + assert all("url" not in i or i["url"] for i in identifiers) + assert "url" not in identifiers[0] + def test_severity_mapping(self): """Test all Socket severities map to GitLab severities""" severity_tests = [ diff --git a/uv.lock b/uv.lock index 69bb3127..446a9241 100644 --- a/uv.lock +++ b/uv.lock @@ -1293,7 +1293,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.8.1" +version = "2.8.2" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" },