diff --git a/.github/workflows/pr-preview.yml b/.github/workflows/pr-preview.yml index 3a2ab1a9..d3ef1c8b 100644 --- a/.github/workflows/pr-preview.yml +++ b/.github/workflows/pr-preview.yml @@ -254,17 +254,25 @@ jobs: - name: Set up Docker publishing uses: ./.github/actions/setup-docker with: - enable-qemu: "false" + # QEMU is required to build the arm64 layer on an amd64 runner. + enable-qemu: "true" dockerhub-username: ${{ secrets.DOCKERHUB_USERNAME }} dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }} - name: Build and push Docker preview uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 with: + # Build from the checked-out workspace, not the default Git context. The + # wheel is only present here as a downloaded artifact, and this also keeps + # Dockerfile.preview on the default-branch checkout rather than the PR ref. + context: . file: Dockerfile.preview push: true pull: true - platforms: linux/amd64 + # Match the arch matrix of the release and stable images so a preview is a + # drop-in replacement for socketdev/cli:latest on arm64 runners too. The + # arm64 layer builds under emulation, so expect roughly double the runtime. + platforms: linux/amd64,linux/arm64 tags: socketdev/cli:pr-${{ needs.context.outputs.pr_number }} build-args: | SDK_PREVIEW_VERSION=${{ inputs.sdk_preview_version }} diff --git a/CHANGELOG.md b/CHANGELOG.md index c87837da..2c3973f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,48 @@ # Changelog +## 2.6.5 + +### Changed: faster local scan setup for large repositories + +- Replaced repeated per-pattern recursive manifest globs with one streaming + filesystem walk per scan root. Excluded directories and `.git` are pruned + before descent, reducing filesystem metadata work without building a + repository-sized in-memory file index. +- Removed the unconditional `git fetch --all` from CLI initialization. Pull + request scans now use local refs first and fetch only a required base or head + ref when the checkout does not contain enough history. +- Added native Buildkite commit, branch, pull-request range, and GitHub SCM + configuration fallbacks so Buildkite jobs no longer need GitHub + Actions-shaped environment-variable shims. +- Added INFO-level timings for CLI run registration, organization setup, Git + initialization and fetches, changed-file detection, supported-pattern lookup, + and manifest discovery. +- Supported manifest patterns are cached for each CLI invocation once the API + returns them, so a transient lookup failure no longer keeps the run on the + smaller local fallback pattern set. Manifest results from `--sub-path` routing + are reused during scan creation. + +### Changed: scan comparisons no longer fetch unused artifacts + +- Scan comparisons now ask the API to omit unchanged artifacts unless an enabled + output actually reads them (`--strict-blocking`, `--enable-gitlab-security`, + `--generate-license`, or `--legal-format fossa`). Cached comparison responses + embed every unchanged artifact at roughly 1 KB each, so on a large dependency + tree this was most of the response — over 10 MB for a tree of ~10k unchanged + packages — downloaded and parsed on every pull request even when nothing read + it. Behavior is unchanged for any run that uses those outputs. + +### Changed: scan comparison timing is easier to attribute + +- Lowered the diff-scan poll ceiling from 30s to 10s. A finished comparison is no + longer left unobserved for up to half a minute, which matters when the CLI runs + inside a CI step with a per-step time budget. +- Diff scans now log their ID, poll count, and the wait before the final poll, so + a CI log distinguishes backend comparison time from time spent between polls. +- Documented the `diff-scans:create`, `diff-scans:list` and `full-scans:list` + token scopes. Without them the comparison silently falls back to the older + streaming path. + ## 2.6.4 ### Changed: bump pinned @coana-tech/cli to 15.10.13 diff --git a/benchmarks/manifest_discovery.py b/benchmarks/manifest_discovery.py new file mode 100644 index 00000000..abc0e8bb --- /dev/null +++ b/benchmarks/manifest_discovery.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +"""Compare legacy per-pattern rglob discovery with the single-pass walker. + +This is an opt-in developer benchmark, not a timing assertion in the test +suite. It creates a synthetic monorepo so filesystem or CI-agent changes do not +make regular tests flaky. +""" + +import argparse +import tempfile +import time +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +from socketsecurity.core import Core +from socketsecurity.core.socket_config import SocketConfig +from socketsecurity.core.utils import socket_globs + + +def seed_tree(root: Path, directories: int, files_per_directory: int) -> None: + for directory_index in range(directories): + directory = root / "packages" / f"package-{directory_index:05d}" + directory.mkdir(parents=True) + (directory / "package.json").write_text("{}\n", encoding="utf-8") + for file_index in range(files_per_directory): + (directory / f"source-{file_index:03d}.txt").write_text( + "not a manifest\n", + encoding="utf-8", + ) + + # These trees model the expensive directories that the new walker prunes + # before descent rather than visiting once for every manifest pattern. + for excluded in (".git/objects", "node_modules/example", ".venv/site-packages"): + directory = root / excluded + directory.mkdir(parents=True) + for index in range(files_per_directory * 10): + (directory / f"object-{index:05d}").write_text("x", encoding="utf-8") + + +def legacy_discover(root: Path) -> set[str]: + results = set() + excluded_dirs = SocketConfig(api_key="benchmark").excluded_dirs + for ecosystem_patterns in socket_globs.values(): + for details in ecosystem_patterns.values(): + for pattern in Core.expand_brace_pattern(details["pattern"]): + insensitive = Core.to_case_insensitive_regex(pattern) + for candidate in root.rglob(insensitive): + if candidate.is_file() and not Core.is_excluded( + str(candidate), + excluded_dirs, + ): + results.add(candidate.as_posix()) + return results + + +def new_core() -> Core: + core = Core.__new__(Core) + core.config = SocketConfig(api_key="benchmark") + core.cli_config = SimpleNamespace(exclude_paths=None) + core.sdk = MagicMock() + core._supported_patterns = socket_globs + return core + + +def timed(function, root: Path) -> tuple[set[str], float]: + start = time.perf_counter() + results = set(function(root)) + return results, time.perf_counter() - start + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--directories", type=int, default=500) + parser.add_argument("--files-per-directory", type=int, default=20) + args = parser.parse_args() + + with tempfile.TemporaryDirectory(prefix="socket-manifest-benchmark-") as temp: + root = Path(temp) + seed_tree(root, args.directories, args.files_per_directory) + legacy_results, legacy_seconds = timed(legacy_discover, root) + new_results, new_seconds = timed( + lambda path: new_core().find_files(str(path)), + root, + ) + + if legacy_results != new_results: + raise SystemExit( + "Manifest result mismatch: " + f"legacy={len(legacy_results)}, single_pass={len(new_results)}" + ) + + speedup = legacy_seconds / new_seconds if new_seconds else float("inf") + print(f"Manifests: {len(new_results)}") + print(f"Legacy per-pattern rglob: {legacy_seconds:.3f}s") + print(f"Single-pass walk: {new_seconds:.3f}s") + print(f"Speedup: {speedup:.1f}x") + + +if __name__ == "__main__": + main() diff --git a/docs/ci-cd.md b/docs/ci-cd.md index 66193f38..061d18ea 100644 --- a/docs/ci-cd.md +++ b/docs/ci-cd.md @@ -81,6 +81,19 @@ steps: SOCKET_SECURITY_API_TOKEN: "${SOCKET_SECURITY_API_TOKEN}" ``` +The CLI reads Buildkite's native `BUILDKITE_COMMIT`, `BUILDKITE_BRANCH`, +`BUILDKITE_PULL_REQUEST`, and `BUILDKITE_PULL_REQUEST_BASE_BRANCH` variables. +For pull-request builds, ensure the checkout contains the base branch and the +checked-out head commit. The CLI uses those local refs first and performs a +targeted fetch only when a required ref or its comparison history is missing; +it does not fetch every remote ref and tag during startup. + +When `--scm github` is used from Buildkite, the CLI also derives GitHub comment +context from `BUILDKITE_REPO`, `BUILDKITE_BUILD_CHECKOUT_PATH`, and the variables +above. Set `GH_API_TOKEN` to a GitHub token with the required repository access. +GitHub Enterprise users should also set `GITHUB_API_URL`; GitHub.com defaults to +`https://api.github.com`. + #### Merge-base baselines in Buildkite (dynamic pipelines) Notes for using `--base-commit-sha` (see the diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 531099cd..9d6d06df 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -1,5 +1,34 @@ # Troubleshooting +## API token scopes for scan comparisons + +PR/MR runs compare the new scan against the repository's head scan. That comparison +first uses the diff-scans endpoints, which require an organization token with these +scopes in addition to whatever the scan itself needs: + +- `diff-scans:create` +- `diff-scans:list` +- `full-scans:list` + +If the token is missing them the scan still succeeds, so this is easy to miss. The only +signal is a warning, after which the CLI falls back to the older streaming comparison: + +``` +Diff scan comparison failed with APIAccessDenied(Insufficient permissions), falling back to the streaming scan comparison +``` + +Grant the scopes to use the diff-scans path. It polls with short, bounded requests +rather than holding one connection open while the backend computes, which is what lets +large comparisons survive network idle timeouts — notably Azure NAT gateways, which +reap idle connections after four minutes and surface as an intermittent +`ConnectionResetError`. + +The two paths can take noticeably different amounts of time on the same repository, +because cached diff-scan responses always embed per-package license details while the +streaming comparison requests a lean payload. On a large dependency tree, compare the +`Diff scan comparison ready in ...` timing against the `Diff Report Gathered in ...` +total before assuming either path is at fault. + ## Common gotchas - In diff scope, `--strict-blocking` uses a stricter alert set (`new + unchanged`) for blocking checks and diff-based output selection. diff --git a/pyproject.toml b/pyproject.toml index 313a8fae..48ae5907 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "hatchling.build" [project] name = "socketsecurity" -version = "2.6.4" +version = "2.6.5" requires-python = ">= 3.11" license = {"file" = "LICENSE"} dependencies = [ diff --git a/socketsecurity/__init__.py b/socketsecurity/__init__.py index 9d9963d7..5875d7cb 100644 --- a/socketsecurity/__init__.py +++ b/socketsecurity/__init__.py @@ -1,3 +1,3 @@ __author__ = 'socket.dev' -__version__ = '2.6.4' +__version__ = '2.6.5' USER_AGENT = f'SocketPythonCLI/{__version__}' diff --git a/socketsecurity/core/__init__.py b/socketsecurity/core/__init__.py index 7be24858..36f9a5b1 100644 --- a/socketsecurity/core/__init__.py +++ b/socketsecurity/core/__init__.py @@ -1,3 +1,7 @@ +import copy +import fnmatch +import importlib +import json import logging import os import random @@ -6,10 +10,9 @@ import tarfile import tempfile import time -import json from dataclasses import asdict -from pathlib import Path, PurePath -from typing import Dict, List, Tuple, Set, TYPE_CHECKING, Optional +from pathlib import PurePath +from typing import TYPE_CHECKING, Dict, List, NamedTuple, Optional, Set, Tuple if TYPE_CHECKING: from socketsecurity.config import CliConfig @@ -18,21 +21,15 @@ from socketdev.fullscans import DiffArtifacts, FullScanParams, SocketArtifact from socketdev.org import Organization from socketdev.repos import RepositoryInfo -import copy -from socketsecurity import __version__, USER_AGENT -from socketsecurity.core.classes import ( - Alert, - Diff, - FullScan, - Issue, - Package, - Purl -) + +from socketsecurity import USER_AGENT, __version__ +from socketsecurity.core.classes import Alert, Diff, FullScan, Issue, Package, Purl from socketsecurity.core.exceptions import APIResourceNotFound + +from .resource_utils import check_file_count_against_ulimit from .socket_config import SocketConfig from .utils import socket_globs -from .resource_utils import check_file_count_against_ulimit -import importlib + logging_std = importlib.import_module("logging") @@ -106,8 +103,15 @@ # minutes to compute. The timeout is a backstop against a diff scan that never # completes; on expiry (or any other failure of this flow) the caller falls back to the # legacy streaming comparison rather than failing the scan outright. +# +# The max interval is also the upper bound on how long a finished comparison sits +# unnoticed between polls, which is dead time added to every PR job. Callers commonly +# run this inside a CI step with a per-step time budget of a few minutes, so the cap is +# kept small: a multi-minute comparison costs roughly 2x the polls of a 30s cap while +# cutting the worst-case dead time from 30s to 10s. Diff scans log their poll count and +# last interval on completion so this tradeoff can be re-evaluated against real timings. DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS = 5.0 -DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS = 30.0 +DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS = 10.0 DIFF_SCAN_POLL_BACKOFF_MULTIPLIER = 1.5 DIFF_SCAN_POLL_TIMEOUT_SECONDS = 30 * 60.0 @@ -125,6 +129,30 @@ def _humanize_alert_type(alert_type: str) -> str: return " ".join(part[:1].upper() + part[1:] for part in parts if part) +class ManifestPatterns(NamedTuple): + """Manifest patterns prepared once per scan root, case-folded for matching. + + The first three fields are the authoritative matchers used by + Core._matches_manifest_pattern. The candidate_* fields are a prefilter over + basenames alone: a manifest-discovery walk visits every file in the repository + but only a few hundred are manifests, so rejecting a name up front avoids + building a relative path and running a path match for the rest. The prefilter's + globs are pre-compiled into one alternation so the cost per rejected file stays + flat as the API's pattern list grows. + """ + + literal_basenames: Set[str] + basename_globs: List[str] + path_globs: List[str] + candidate_basenames: Set[str] + candidate_basename_regex: Optional["re.Pattern"] + + @property + def is_empty(self) -> bool: + """True when every ecosystem was filtered out, so the walk can be skipped.""" + return not (self.literal_basenames or self.basename_globs or self.path_globs) + + class Core: """Main class for interacting with Socket Security API and processing scan results.""" @@ -146,7 +174,13 @@ def __init__(self, config: SocketConfig, sdk: socketdev, cli_config: Optional['C self.config = config self.sdk = sdk self.cli_config = cli_config + self._supported_patterns: Optional[Dict] = None + org_start_time = time.perf_counter() self.set_org_vars() + log.info( + "Organization initialization completed in " + f"{time.perf_counter() - org_start_time:.2f}s" + ) def set_org_vars(self) -> None: """Sets the main shared configuration variables for organization access.""" @@ -421,6 +455,112 @@ def format_bytes(bytes_value): except Exception as e: log.error(f"Failed to save manifest tar.gz to {output_path}: {e}") + @staticmethod + def _prepare_manifest_patterns( + patterns: Dict, + ecosystems: Optional[List[str]], + excluded_ecosystems: List[str] + ) -> "ManifestPatterns": + """Prepare case-folded manifest patterns for a single filesystem walk. + + Literal basenames are kept in a set for the common fast path. Basename + globs and path-shaped globs are kept separately so the latter retain + pathlib's path-segment-aware matching behavior. The candidate basename + collections are derived here so the walker can reject a file on its name + alone; see ManifestPatterns. + """ + included_ecosystems = set(ecosystems) if ecosystems is not None else None + excluded = set(excluded_ecosystems) + literal_basenames: Set[str] = set() + basename_globs: Set[str] = set() + path_globs: Set[str] = set() + + for ecosystem, ecosystem_patterns in patterns.items(): + if included_ecosystems is not None and ecosystem not in included_ecosystems: + continue + if ecosystem in excluded: + continue + log.debug(f"Scanning ecosystem: {ecosystem}") + for details in ecosystem_patterns.values(): + original_pattern = details["pattern"] + for expanded in Core.expand_brace_pattern(original_pattern): + normalized = expanded.replace("\\", "/").casefold() + if "/" in normalized: + path_globs.add(normalized) + elif any(character in normalized for character in "*?["): + basename_globs.add(normalized) + else: + literal_basenames.add(normalized) + + # PurePath.match compares pattern segments right to left, so a path-shaped glob + # can only match a file whose basename matches the glob's final segment. Folding + # those final segments into the basename prefilter lets the walk skip the path + # match for everything else. An empty final segment (a trailing "/") constrains + # nothing, so it becomes "*" and the prefilter admits every name. + candidate_basenames = set(literal_basenames) + candidate_basename_globs = set(basename_globs) + for pattern in path_globs: + final_segment = pattern.rstrip("/").rsplit("/", 1)[-1] or "*" + if any(character in final_segment for character in "*?["): + candidate_basename_globs.add(final_segment) + else: + candidate_basenames.add(final_segment) + + return ManifestPatterns( + literal_basenames=literal_basenames, + basename_globs=sorted(basename_globs), + path_globs=sorted(path_globs), + candidate_basenames=candidate_basenames, + candidate_basename_regex=Core._compile_basename_globs(candidate_basename_globs), + ) + + @staticmethod + def _compile_basename_globs(globs: Set[str]) -> Optional["re.Pattern"]: + """Compile basename globs into a single alternation, or None if there are none. + + fnmatch.translate anchors the tail with ``\\Z`` and re.match anchors the head, + so each alternative matches exactly what fnmatch.fnmatchcase would. + """ + if not globs: + return None + return re.compile( + "|".join(f"(?:{fnmatch.translate(glob)})" for glob in sorted(globs)) + ) + + @staticmethod + def _basename_could_match(normalized_name: str, patterns: "ManifestPatterns") -> bool: + """Cheap prefilter: could a file with this basename match any manifest pattern? + + False is authoritative; True still has to be confirmed by + _matches_manifest_pattern against the scan-root-relative path. + """ + if normalized_name in patterns.candidate_basenames: + return True + return ( + patterns.candidate_basename_regex is not None + and patterns.candidate_basename_regex.match(normalized_name) is not None + ) + + @staticmethod + def _matches_manifest_pattern(relative_path: str, patterns: "ManifestPatterns") -> bool: + """Return whether a scan-root-relative path matches a manifest pattern.""" + normalized_path = relative_path.replace("\\", "/").casefold() + basename = normalized_path.rsplit("/", 1)[-1] + if basename in patterns.literal_basenames: + return True + if any(fnmatch.fnmatchcase(basename, pattern) for pattern in patterns.basename_globs): + return True + if not patterns.path_globs: + return False + + candidate = PurePath(normalized_path) + return any(candidate.match(pattern) for pattern in patterns.path_globs) + + @staticmethod + def _matches_excluded_directory(directory_name: str, excluded_dirs: Set[str]) -> bool: + """Match configured directory exclusions, including entries such as ``*.egg-info``.""" + return any(fnmatch.fnmatchcase(directory_name, pattern) for pattern in excluded_dirs) + def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[str]: """ Finds supported manifest files in the given path. @@ -432,8 +572,8 @@ def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[ Returns: List of found manifest file paths. """ - log.debug("Starting Find Files") - start_time = time.time() + log.debug("Starting manifest discovery") + start_time = time.perf_counter() files: Set[str] = set() # Unified --exclude-paths: filter discovered manifests by the same paths/globs that are @@ -447,50 +587,92 @@ def find_files(self, path: str, ecosystems: Optional[List[str]] = None) -> List[ exclude_paths = getattr(self.cli_config, "exclude_paths", None) if self.cli_config else None exclude_regexes = Core.compile_exclude_paths(exclude_paths) if exclude_paths else [] - # Get supported patterns from the API patterns = self.get_supported_patterns() + manifest_patterns = self._prepare_manifest_patterns( + patterns, + ecosystems, + self.config.excluded_ecosystems, + ) - for ecosystem in patterns: - # If ecosystems filter is provided, only include specified ecosystems - if ecosystems is not None and ecosystem not in ecosystems: - continue - if ecosystem in self.config.excluded_ecosystems: - continue - log.debug(f'Scanning ecosystem: {ecosystem}') - ecosystem_patterns = patterns[ecosystem] - for file_name in ecosystem_patterns: - original_pattern = ecosystem_patterns[file_name]["pattern"] - - # Expand brace patterns - expanded_patterns = Core.expand_brace_pattern(original_pattern) - - for pattern in expanded_patterns: - case_insensitive_pattern = Core.to_case_insensitive_regex(pattern) - - log.debug(f"Searching for pattern: {case_insensitive_pattern}") - glob_start = time.time() - - # Use pathlib.Path.rglob() instead of glob.glob() to properly match dotfiles/dotdirs - base_path = Path(path) - glob_files = base_path.rglob(case_insensitive_pattern) - - for glob_file in glob_files: - glob_file_str = str(glob_file) - if not os.path.isfile(glob_file_str): - continue - if Core.is_excluded(glob_file_str, self.config.excluded_dirs): - continue - if exclude_regexes: - rel = os.path.relpath(glob_file_str, path) - if Core.path_matches_exclude_regexes(rel, exclude_regexes): - continue - files.add(glob_file_str.replace("\\", "/")) - - glob_end = time.time() - log.debug(f"Globbing took {glob_end - glob_start:.4f} seconds") + if manifest_patterns.is_empty: + elapsed = time.perf_counter() - start_time + log.info( + "Manifest discovery completed in " + f"{elapsed:.2f}s: root={os.path.abspath(path)}, " + "directories_visited=0, directories_pruned=0, " + "files_visited=0, manifests_found=0" + ) + log.info("Total files found: 0") + return [] + + directories_visited = 0 + directories_pruned = 0 + files_visited = 0 + excluded_dirs = set(self.config.excluded_dirs) + + def handle_walk_error(error: OSError) -> None: + log.debug(f"Unable to inspect path during manifest discovery: {error}") + + for current_root, directory_names, file_names in os.walk( + path, + topdown=True, + followlinks=False, + onerror=handle_walk_error, + ): + directories_visited += 1 + + kept_directories = [] + for directory_name in directory_names: + if directory_name == ".git" or Core._matches_excluded_directory( + directory_name, + excluded_dirs, + ): + directories_pruned += 1 + continue + # Only --exclude-paths needs a scan-root-relative path, so build one + # lazily rather than for every directory in the repository. + if exclude_regexes: + relative_directory = os.path.relpath( + os.path.join(current_root, directory_name), + path, + ) + if Core.path_matches_exclude_regexes(relative_directory, exclude_regexes): + directories_pruned += 1 + continue + kept_directories.append(directory_name) + directory_names[:] = kept_directories + + files_visited += len(file_names) + for file_name in file_names: + # Reject on the basename first: os.walk already hands us the name, so + # non-manifests cost one set lookup instead of a relative path plus a + # path match. Exclusions are then only evaluated for real candidates. + if not Core._basename_could_match(file_name.casefold(), manifest_patterns): + continue + file_path = os.path.join(current_root, file_name) + relative_path = os.path.relpath(file_path, path) + if not Core._matches_manifest_pattern(relative_path, manifest_patterns): + continue + if exclude_regexes and Core.path_matches_exclude_regexes( + relative_path, + exclude_regexes, + ): + continue + if os.path.isfile(file_path): + files.add(file_path.replace("\\", "/")) file_list = sorted(files) file_count = len(file_list) + elapsed = time.perf_counter() - start_time + log.info( + "Manifest discovery completed in " + f"{elapsed:.2f}s: root={os.path.abspath(path)}, " + f"directories_visited={directories_visited}, " + f"directories_pruned={directories_pruned}, " + f"files_visited={files_visited}, manifests_found={file_count}" + ) + # Retain the established count-only message for log consumers while they + # transition to the stage-level timing above. log.info(f"Total files found: {file_count}") # Check if the number of manifest files might exceed ulimit -n @@ -532,19 +714,36 @@ def get_supported_patterns(self) -> Dict: Returns: Dictionary of supported file patterns with 'general' key removed """ + cached_patterns = getattr(self, "_supported_patterns", None) + if cached_patterns is not None: + log.debug("Using cached supported manifest patterns") + return cached_patterns + + start_time = time.perf_counter() response = self.sdk.report.supported() + source = "api" if not response: log.error("Failed to get supported patterns from API") - # Import the old patterns as fallback - from .utils import socket_globs - return socket_globs - - # Remove the 'general' key if it exists - if 'general' in response: - response.pop('general') - - # The response is already in the format we need - return response + response = socket_globs + source = "local-fallback" + + # Do not mutate the SDK response, which may be shared by its own cache. + patterns = { + ecosystem: ecosystem_patterns + for ecosystem, ecosystem_patterns in response.items() + if ecosystem != "general" + } + # Only cache a successful lookup. The local fallback covers far fewer ecosystems + # than the API, so one transient failure must not pin the rest of the run to it — + # has_manifest_files() runs before find_files() and would poison the cache. + if source == "api": + self._supported_patterns = patterns + elapsed = time.perf_counter() - start_time + log.info( + "Supported manifest patterns loaded in " + f"{elapsed:.2f}s: source={source}, ecosystems={len(patterns)}" + ) + return patterns def has_manifest_files(self, files: list) -> bool: """ @@ -627,7 +826,7 @@ def empty_head_scan_file() -> List[str]: temp_path = os.path.join(temp_dir, '.socket.facts.json') # Create the empty file - with open(temp_path, 'w') as f: + with open(temp_path, 'w'): pass # Creates an empty file log.debug(f"Created temporary empty file for baseline scan: {temp_path}") @@ -1392,6 +1591,10 @@ def get_diff_scan_artifacts( "Error creating or resolving diff scan: " f"unexpected response: {str(response_summary)[:500]}" ) + # Logged at INFO, not debug: this is the only identifier that ties a slow or + # failed comparison in a CI log back to a server-side diff scan, and it is + # needed even when the run later falls back to the streaming comparison. + log.info(f"Diff scan created: id={diff_scan_id}") artifacts_dict = diff_scan.get("artifacts") # cached=true is the polling contract (202 while computing, 200 when @@ -1402,9 +1605,24 @@ def get_diff_scan_artifacts( # payload ever gets a response truncated on a huge dependency tree, # response.json() fails and the caller falls back to the legacy # streaming comparison, which still requests the lean payload. + # + # Verified against the live API: passing omit_license_details alongside + # cached=true leaves the license fields in the response, but omit_unchanged + # IS honored and drops the unchanged artifacts entirely (~1.1 KB each), so + # it is requested whenever no enabled output reads them. See + # _requires_unchanged_artifacts. poll_params = {"cached": "true"} - deadline = time.monotonic() + DIFF_SCAN_POLL_TIMEOUT_SECONDS + omit_unchanged = not self._requires_unchanged_artifacts() + if omit_unchanged: + poll_params["omit_unchanged"] = "true" + poll_start = time.monotonic() + deadline = poll_start + DIFF_SCAN_POLL_TIMEOUT_SECONDS interval = DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS + # Tracked so the completion log can separate backend compute time from time the + # result spent ready-but-unpolled: the wait before the final poll bounds the + # latter, which is otherwise invisible in a CI log. + polls = 0 + last_interval = 0.0 while artifacts_dict is None: try: response = self.sdk.diffscans.get(self.config.org_slug, diff_scan_id, params=poll_params) @@ -1418,6 +1636,7 @@ def get_diff_scan_artifacts( f"({type(error).__name__}), retrying in {interval:.0f}s" ) response = {"status": "processing"} + polls += 1 if response.get("status") != "processing": scan = response.get("diff_scan") or {} if scan.get("artifacts") is None: @@ -1425,6 +1644,12 @@ def get_diff_scan_artifacts( f"Error fetching diff scan {diff_scan_id}: unexpected response: {str(response)[:500]}" ) artifacts_dict = scan["artifacts"] + log.info( + "Diff scan comparison ready in " + f"{time.monotonic() - poll_start:.2f}s: id={diff_scan_id}, " + f"polls={polls}, wait_before_final_poll={last_interval:.0f}s, " + f"omit_unchanged={str(omit_unchanged).lower()}" + ) break if time.monotonic() >= deadline: raise Exception( @@ -1433,6 +1658,7 @@ def get_diff_scan_artifacts( ) log.debug(f"Diff scan {diff_scan_id} still processing, polling again in {interval:.0f}s") time.sleep(interval) + last_interval = interval interval = min(interval * DIFF_SCAN_POLL_BACKOFF_MULTIPLIER, DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS) return DiffArtifacts.from_dict({ @@ -1440,6 +1666,39 @@ def get_diff_scan_artifacts( for key in ("added", "removed", "unchanged", "replaced", "updated") }) + def _requires_unchanged_artifacts(self) -> bool: + """Whether any enabled output reads the unchanged half of a comparison. + + A cached diff-scan response embeds every unchanged artifact at roughly 1 KB + each, so on a large dependency tree they are almost the entire payload + (~11 MB for a tree of ~10k unchanged packages) even though most runs never + look at them. Every consumer is behind an opt-in flag: + + - ``--strict-blocking`` reads ``diff.unchanged_alerts`` to block on + pre-existing issues (socketcli, output, alert_selection, slack plugin). + - ``--enable-gitlab-security`` includes them in the GitLab dependency + scanning report (Messages.create_security_comment_gitlab). + - ``--generate-license`` enumerates ``diff.packages``, which must list every + dependency, not just the changed ones. + - ``--legal-format fossa`` reports all currently-present issues, matching + FOSSA's point-in-time snapshot semantics. + + ``Diff.to_dict`` also serializes them but has no callers. When cli_config is + absent the caller is unknown, so the full payload is kept. + + Keep this in sync with those consumers; test_unchanged_artifacts_gating + pins the list. + """ + config = self.cli_config + if config is None: + return True + return bool( + getattr(config, "strict_blocking", False) + or getattr(config, "enable_gitlab_security", False) + or getattr(config, "generate_license", False) + or getattr(config, "legal_format", "socket") == "fossa" + ) + def get_added_and_removed_packages( self, head_full_scan_id: str, @@ -1754,6 +2013,10 @@ def create_new_diff( else: diff.diff_url = diff.report_url + # PR/MR runs previously logged only the scan IDs, so a CI log had no link to the + # result. Logged here rather than at each call site so every diff flow gets it. + log.info(f"Diff report URL: {diff.diff_url}") + return diff def create_diff_report( diff --git a/socketsecurity/core/git_interface.py b/socketsecurity/core/git_interface.py index da614063..b3c53bdc 100644 --- a/socketsecurity/core/git_interface.py +++ b/socketsecurity/core/git_interface.py @@ -1,6 +1,7 @@ +import os import re +import time import urllib.parse -import os from git import Repo @@ -12,34 +13,33 @@ class Git: path: str def __init__(self, path: str): + initialization_start = time.perf_counter() self.path = path + self._fetched_ref_commits = {} self.ensure_safe_directory(path) self.repo = Repo(path) assert self.repo self.head = self.repo.head - - # Always fetch all remote refs to ensure branches exist for diffing - try: - self.repo.git.fetch('--all') - log.debug("Fetched all remote refs for diffing.") - except Exception as fetch_error: - log.debug(f"Failed to fetch all remote refs: {fetch_error}") # Use CI environment SHA if available, otherwise fall back to current HEAD commit github_sha = os.getenv('GITHUB_SHA') gitlab_sha = os.getenv('CI_COMMIT_SHA') bitbucket_sha = os.getenv('BITBUCKET_COMMIT') - ci_sha = github_sha or gitlab_sha or bitbucket_sha + buildkite_sha = os.getenv('BUILDKITE_COMMIT') + ci_commits = ( + ("BUILDKITE_COMMIT", buildkite_sha), + ("GITHUB_SHA", github_sha), + ("CI_COMMIT_SHA", gitlab_sha), + ("BITBUCKET_COMMIT", bitbucket_sha), + ) + env_source, ci_sha = next( + ((source, sha) for source, sha in ci_commits if sha), + (None, None), + ) if ci_sha: try: self.commit = self.repo.commit(ci_sha) - if github_sha: - env_source = "GITHUB_SHA" - elif gitlab_sha: - env_source = "CI_COMMIT_SHA" - else: - env_source = "BITBUCKET_COMMIT" log.debug(f"Using commit from {env_source}: {ci_sha}") except Exception as error: log.debug(f"Failed to get commit from CI environment: {error}") @@ -82,13 +82,19 @@ def __init__(self, path: str): # Bitbucket Pipelines variables bitbucket_branch = os.getenv('BITBUCKET_BRANCH') + + # Buildkite branch (the source branch for pull-request builds) + buildkite_branch = os.getenv('BUILDKITE_BRANCH') - # Select CI branch with priority: GitLab -> GitHub -> Bitbucket - ci_branch = gitlab_branch or github_branch or bitbucket_branch + # Prefer the native environment when Buildkite is driving the job. This + # also avoids requiring Buildkite users to emulate GitHub Actions vars. + ci_branch = buildkite_branch or gitlab_branch or github_branch or bitbucket_branch if ci_branch: self.branch = ci_branch - if gitlab_branch: + if buildkite_branch: + env_source = "Buildkite" + elif gitlab_branch: env_source = "GitLab CI" elif github_branch: env_source = "GitHub Actions" @@ -141,40 +147,39 @@ def __init__(self, path: str): self.commit_sha = self.commit.binsha self.commit_message = self.commit.message self.committer = self.commit.committer - # Detect changed files in PR/MR context for GitHub, GitLab, Bitbucket; fallback to git show + + # Detect changed files in PR/MR context, using local refs first and + # fetching only a required ref when the checkout does not contain it. + changed_files_start = time.perf_counter() self.show_files = [] detected = False - # GitHub Actions PR context + detection_source = "single-commit" + github_base_ref = os.getenv('GITHUB_BASE_REF') github_head_ref = os.getenv('GITHUB_HEAD_REF') github_event_name = os.getenv('GITHUB_EVENT_NAME') github_before_sha = os.getenv('GITHUB_EVENT_BEFORE') # previous commit for push github_sha = os.getenv('GITHUB_SHA') # current commit - if github_event_name == 'pull_request' and github_base_ref and github_head_ref: - try: - # Fetch both branches individually - self.repo.git.fetch('origin', github_base_ref) - self.repo.git.fetch('origin', github_head_ref) - # Try remote diff first - diff_range = f"origin/{github_base_ref}...origin/{github_head_ref}" - try: - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitHub PR remote): {self.show_files}") - detected = True - except Exception as remote_error: - log.debug(f"Remote diff failed: {remote_error}") - # Try local branch diff - local_diff_range = f"{github_base_ref}...{github_head_ref}" - try: - diff_files = self.repo.git.diff('--name-only', local_diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitHub PR local): {self.show_files}") - detected = True - except Exception as local_error: - log.debug(f"Local diff failed: {local_error}") - except Exception as error: - log.debug(f"Failed to fetch branches or diff for GitHub PR: {error}") + + buildkite_pr = os.getenv('BUILDKITE_PULL_REQUEST') + buildkite_base_ref = os.getenv('BUILDKITE_PULL_REQUEST_BASE_BRANCH') + buildkite_head_ref = os.getenv('BUILDKITE_BRANCH') + if self._is_buildkite_pull_request(buildkite_pr) and buildkite_base_ref: + detected = self._detect_pull_request_changes( + provider="Buildkite", + base_ref=buildkite_base_ref, + head_ref=buildkite_head_ref, + ) + if detected: + detection_source = "buildkite-pr" + elif github_event_name == 'pull_request' and github_base_ref: + detected = self._detect_pull_request_changes( + provider="GitHub", + base_ref=github_base_ref, + head_ref=github_head_ref, + ) + if detected: + detection_source = "github-pr" # Commits to default branch (push events) elif github_event_name == 'push' and github_before_sha and github_sha: try: @@ -182,6 +187,7 @@ def __init__(self, path: str): self.show_files = diff_files.splitlines() log.debug(f"Changed files detected via git diff (GitHub push): {self.show_files}") detected = True + detection_source = "github-push" except Exception as error: log.debug(f"Failed to get changed files via git diff (GitHub push): {error}") elif github_event_name == 'push': @@ -189,6 +195,7 @@ def __init__(self, path: str): self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show (GitHub push fallback): {self.show_files}") detected = True + detection_source = "github-push-fallback" except Exception as error: log.debug(f"Failed to get changed files via git show (GitHub push fallback): {error}") # GitLab CI Merge Request context @@ -196,15 +203,13 @@ def __init__(self, path: str): gitlab_target = os.getenv('CI_MERGE_REQUEST_TARGET_BRANCH_NAME') gitlab_source = os.getenv('CI_MERGE_REQUEST_SOURCE_BRANCH_NAME') if gitlab_target and gitlab_source: - try: - self.repo.git.fetch('origin', gitlab_target, gitlab_source) - diff_range = f"origin/{gitlab_target}...origin/{gitlab_source}" - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (GitLab): {self.show_files}") - detected = True - except Exception as error: - log.debug(f"Failed to get changed files via git diff (GitLab): {error}") + detected = self._detect_pull_request_changes( + provider="GitLab", + base_ref=gitlab_target, + head_ref=gitlab_source, + ) + if detected: + detection_source = "gitlab-mr" # Bitbucket Pipelines PR context if not detected: bitbucket_pr_id = os.getenv('BITBUCKET_PR_ID') @@ -212,15 +217,13 @@ def __init__(self, path: str): bitbucket_dest = os.getenv('BITBUCKET_PR_DESTINATION_BRANCH') # BITBUCKET_BRANCH is the source branch in PR builds if bitbucket_pr_id and bitbucket_source and bitbucket_dest: - try: - self.repo.git.fetch('origin', bitbucket_dest, bitbucket_source) - diff_range = f"origin/{bitbucket_dest}...origin/{bitbucket_source}" - diff_files = self.repo.git.diff('--name-only', diff_range) - self.show_files = diff_files.splitlines() - log.debug(f"Changed files detected via git diff (Bitbucket): {self.show_files}") - detected = True - except Exception as error: - log.debug(f"Failed to get changed files via git diff (Bitbucket): {error}") + detected = self._detect_pull_request_changes( + provider="Bitbucket", + base_ref=bitbucket_dest, + head_ref=bitbucket_source, + ) + if detected: + detection_source = "bitbucket-pr" # Fallback to git show for single commit if not detected: # Check if this is a merge commit first @@ -233,20 +236,132 @@ def __init__(self, path: str): self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show (merge commit fallback): {self.show_files}") detected = True + detection_source = "merge-commit-fallback" + if detected and detection_source == "single-commit": + detection_source = "merge-diff" else: # Regular single commit self.show_files = self.repo.git.show(self.commit, name_only=True, format="%n").splitlines() log.debug(f"Changed files detected via git show: {self.show_files}") detected = True + detection_source = "single-commit" self.changed_files = [] for item in self.show_files: if item != "": # Use relative path for glob matching self.changed_files.append(item) + + log.info( + "Changed-file detection completed in " + f"{time.perf_counter() - changed_files_start:.2f}s: " + f"source={detection_source}, files={len(self.changed_files)}" + ) # Determine if this commit is on the default branch # This considers both GitHub Actions detached HEAD and regular branch situations self.is_default_branch = self._is_commit_and_branch_default() + log.info( + "Git initialization completed in " + f"{time.perf_counter() - initialization_start:.2f}s" + ) + + @staticmethod + def _is_buildkite_pull_request(pull_request: str | None) -> bool: + return bool(pull_request and pull_request.casefold() != "false") + + def _resolve_ref(self, ref: str | None) -> str | None: + """Resolve a branch, tag, or SHA without accessing the network.""" + if not ref: + return None + if ref in self._fetched_ref_commits: + return self._fetched_ref_commits[ref] + + candidates = [ref] + if not ref.startswith("refs/"): + candidates = [f"origin/{ref}", ref] + for candidate in candidates: + try: + return self.repo.commit(candidate).hexsha + except Exception: + continue + return None + + def _fetch_ref(self, ref: str, reason: str) -> str | None: + """Fetch one required ref and return its commit without broadening scope.""" + if ref in self._fetched_ref_commits: + return self._fetched_ref_commits[ref] + + fetch_start = time.perf_counter() + try: + self.repo.git.fetch("origin", ref) + commit_sha = self.repo.commit("FETCH_HEAD").hexsha + self._fetched_ref_commits[ref] = commit_sha + log.info( + "Git fetch completed in " + f"{time.perf_counter() - fetch_start:.2f}s: " + f"remote=origin, ref={ref}, reason={reason}" + ) + return commit_sha + except Exception as error: + log.info( + "Git fetch failed in " + f"{time.perf_counter() - fetch_start:.2f}s: " + f"remote=origin, ref={ref}, reason={reason}" + ) + log.debug(f"Targeted fetch failed for {ref}: {error}") + return None + + def _detect_pull_request_changes( + self, + provider: str, + base_ref: str, + head_ref: str | None, + ) -> bool: + """Detect a full PR range locally, fetching only refs needed to complete it.""" + base_commit = self._resolve_ref(base_ref) + if base_commit is None: + base_commit = self._fetch_ref(base_ref, f"{provider} pull-request base ref missing") + if base_commit is None: + log.debug(f"Unable to resolve {provider} pull-request base ref: {base_ref}") + return False + + head_commit = self.commit.hexsha + diff_range = f"{base_commit}...{head_commit}" + try: + diff_files = self.repo.git.diff("--name-only", diff_range) + self.show_files = diff_files.splitlines() + log.debug( + f"Changed files detected via local git diff ({provider}): {self.show_files}" + ) + return True + except Exception as local_error: + log.debug(f"Local {provider} pull-request diff failed: {local_error}") + + # A shallow checkout can contain both tips but not their merge base. In + # that case refresh only the two relevant branch histories and retry. + base_commit = self._fetch_ref( + base_ref, + f"{provider} pull-request history incomplete", + ) or base_commit + if head_ref: + self._fetch_ref( + head_ref, + f"{provider} pull-request history incomplete", + ) + + try: + diff_files = self.repo.git.diff( + "--name-only", + f"{base_commit}...{head_commit}", + ) + self.show_files = diff_files.splitlines() + log.debug( + f"Changed files detected after targeted fetch ({provider}): {self.show_files}" + ) + return True + except Exception as retry_error: + log.debug(f"Targeted {provider} pull-request diff failed: {retry_error}") + return False def _is_commit_and_branch_default(self) -> bool: """ @@ -268,9 +383,29 @@ def _is_commit_and_branch_default(self) -> bool: gitlab_mr_branch = os.getenv('CI_MERGE_REQUEST_SOURCE_BRANCH_NAME') gitlab_default_branch = os.getenv('CI_DEFAULT_BRANCH', '') bitbucket_branch = os.getenv('BITBUCKET_BRANCH') + buildkite_branch = os.getenv('BUILDKITE_BRANCH') + buildkite_pr = os.getenv('BUILDKITE_PULL_REQUEST') + buildkite_default_branch = os.getenv('BUILDKITE_PIPELINE_DEFAULT_BRANCH') + # Handle Buildkite before GitHub because some Buildkite pipelines + # intentionally provide GitHub-compatible environment variables. + if buildkite_branch: + if self._is_buildkite_pull_request(buildkite_pr): + log.debug( + f"Processing Buildkite pull request from branch: {buildkite_branch}, " + "not default branch" + ) + return False + default_branch_name = buildkite_default_branch or self.get_default_branch_name() + is_default = buildkite_branch == default_branch_name + log.debug( + f"Buildkite branch: {buildkite_branch}, Default: {default_branch_name}, " + f"Is default: {is_default}" + ) + return is_default + # Handle GitHub Actions - if github_ref: + elif github_ref: log.debug(f"GitHub ref: {github_ref}") # Handle pull requests - they're not on the default branch @@ -483,7 +618,7 @@ def get_default_branch_name(self) -> str: if f'origin/{branch_name}' in [str(ref) for ref in self.repo.remotes.origin.refs]: log.debug(f"Using fallback default branch: {branch_name}") return branch_name - except: + except Exception: continue # Last fallback: assume 'main' @@ -505,12 +640,12 @@ def is_commit_on_default_branch(self) -> bool: # Try remote branch first default_branch_ref = self.repo.remotes.origin.refs[default_branch] default_branch_commit = default_branch_ref.commit - except: + except Exception: # Fallback to local branch try: default_branch_ref = self.repo.heads[default_branch] default_branch_commit = default_branch_ref.commit - except: + except Exception: log.debug(f"Could not find default branch '{default_branch}' locally or remotely") return False @@ -572,4 +707,4 @@ def ensure_safe_directory(path: str) -> None: else: log.debug(f"{abs_path} already present in git safe.directory config.") except Exception as safe_error: - log.debug(f"Failed to set safe.directory for git: {safe_error}") \ No newline at end of file + log.debug(f"Failed to set safe.directory for git: {safe_error}") diff --git a/socketsecurity/core/scm/github.py b/socketsecurity/core/scm/github.py index 7d5905d2..7504a46c 100644 --- a/socketsecurity/core/scm/github.py +++ b/socketsecurity/core/scm/github.py @@ -1,6 +1,7 @@ import json import os import sys +import urllib.parse from dataclasses import dataclass from git import Optional @@ -34,6 +35,31 @@ class GithubConfig: event_action: Optional[str] headers: dict + @staticmethod + def _repository_from_buildkite() -> tuple[str, str]: + """Return ``(owner, repository)`` from Buildkite's Git repository URL.""" + repository_url = ( + # Comments and statuses belong to the pipeline/base repository, + # not a contributor's fork from BUILDKITE_PULL_REQUEST_REPO. + os.getenv("BUILDKITE_REPO") + or os.getenv("BUILDKITE_PULL_REQUEST_REPO") + or "" + ).strip() + if not repository_url: + return "", "" + + if "://" in repository_url: + repository_path = urllib.parse.urlparse(repository_url).path + elif ":" in repository_url: + # SCP-style SSH URL: git@github.com:owner/repository.git + repository_path = repository_url.split(":", 1)[1] + else: + repository_path = repository_url + parts = repository_path.strip("/").removesuffix(".git").split("/") + if len(parts) < 2: + return "", "" + return parts[-2], parts[-1] + @classmethod def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': """Create config from environment variables with optional overrides""" @@ -42,12 +68,24 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': log.error("Unable to get Github API Token from GH_API_TOKEN") sys.exit(2) - # Use provided PR number if available, otherwise fall back to env var + is_buildkite = os.getenv("BUILDKITE") == "true" + buildkite_pr = os.getenv("BUILDKITE_PULL_REQUEST") + is_buildkite_pr = bool( + is_buildkite + and buildkite_pr + and buildkite_pr.casefold() != "false" + ) + + # Use explicit/GitHub-compatible values first, then native Buildkite PR context. pr_number = pr_number or os.getenv('PR_NUMBER') + if not pr_number and is_buildkite_pr: + pr_number = buildkite_pr # Add debug logging - sha = os.getenv('GITHUB_SHA', '') - log.debug(f"Loading SHA from GITHUB_SHA: {sha}") + sha = os.getenv('GITHUB_SHA') or ( + os.getenv("BUILDKITE_COMMIT", "") if is_buildkite else "" + ) + log.debug(f"Loading GitHub integration SHA: {sha}") event_action = os.getenv('EVENT_ACTION', None) if not event_action: event_path = os.getenv('GITHUB_EVENT_PATH') @@ -55,29 +93,66 @@ def from_env(cls, pr_number: Optional[str] = None) -> 'GithubConfig': with open(event_path, 'r') as f: event = json.load(f) event_action = event.get('action') + if not event_action and is_buildkite_pr: + # Buildkite provides the current PR state, not the originating + # GitHub webhook action. A running PR build is equivalent to the + # supported synchronize path for comment updates. + event_action = "synchronize" repository = os.getenv('GITHUB_REPOSITORY', '') owner = os.getenv('GITHUB_REPOSITORY_OWNER', '') if '/' in repository: owner = repository.split('/')[0] repository = repository.split('/')[1] + elif is_buildkite: + buildkite_owner, buildkite_repository = cls._repository_from_buildkite() + owner = owner or buildkite_owner + repository = repository or buildkite_repository default_branch_env = os.getenv('DEFAULT_BRANCH') # Consider the variable truthy if it exists and isn't explicitly 'false' - is_default = default_branch_env is not None and default_branch_env.lower() != 'false' + if default_branch_env is not None: + is_default = default_branch_env.lower() != 'false' + elif is_buildkite: + # Require a branch name: comparing two unset variables would otherwise report + # every build as the default branch and overwrite the repository's baseline. + buildkite_branch = os.getenv("BUILDKITE_BRANCH") + is_default = bool( + not is_buildkite_pr + and buildkite_branch + and buildkite_branch == os.getenv("BUILDKITE_PIPELINE_DEFAULT_BRANCH") + ) + else: + is_default = False + + event_name = os.getenv('GITHUB_EVENT_NAME', '') + if not event_name and is_buildkite: + event_name = "pull_request" if is_buildkite_pr else "push" return cls( - sha=os.getenv('GITHUB_SHA', ''), - api_url=os.getenv('GITHUB_API_URL', ''), - ref_type=os.getenv('GITHUB_REF_TYPE', ''), - event_name=os.getenv('GITHUB_EVENT_NAME', ''), - workspace=os.getenv('GITHUB_WORKSPACE', ''), + sha=sha, + api_url=os.getenv('GITHUB_API_URL') or ( + "https://api.github.com" if is_buildkite else "" + ), + ref_type=os.getenv('GITHUB_REF_TYPE') or ( + "branch" if is_buildkite else "" + ), + event_name=event_name, + workspace=os.getenv('GITHUB_WORKSPACE') or ( + os.getenv("BUILDKITE_BUILD_CHECKOUT_PATH", "") if is_buildkite else "" + ), repository=repository, - ref_name=os.getenv('GITHUB_REF_NAME', ''), + ref_name=os.getenv('GITHUB_REF_NAME') or ( + os.getenv("BUILDKITE_BRANCH", "") if is_buildkite else "" + ), default_branch=is_default, is_default_branch=is_default, pr_number=pr_number, pr_name=os.getenv('PR_NAME'), - commit_message=os.getenv('COMMIT_MESSAGE'), - actor=os.getenv('GITHUB_ACTOR', ''), + commit_message=os.getenv('COMMIT_MESSAGE') or ( + os.getenv("BUILDKITE_MESSAGE") if is_buildkite else None + ), + actor=os.getenv('GITHUB_ACTOR') or ( + os.getenv("BUILDKITE_BUILD_CREATOR", "") if is_buildkite else "" + ), env=os.getenv('GITHUB_ENV', ''), token=token, owner=owner, diff --git a/socketsecurity/core/streaming.py b/socketsecurity/core/streaming.py index 20b45eac..e6910b44 100644 --- a/socketsecurity/core/streaming.py +++ b/socketsecurity/core/streaming.py @@ -13,6 +13,7 @@ """ import logging +import time from typing import Optional from .cli_client import CliClient @@ -49,12 +50,17 @@ def set_report_run_id(self, report_run_id: Optional[str]) -> None: self._report_run_id = report_run_id def __enter__(self) -> "StreamingLogs": + registration_start = time.perf_counter() self._run_id = register_cli_run( self._client, client_version=self._client_version, upload_logs=self._upload_logs, ) cli_logger = self._loggers[0] + cli_logger.info( + "CLI run registration completed in " + f"{time.perf_counter() - registration_start:.2f}s" + ) if not self._run_id: cli_logger.debug("server log streaming not active for this run") return self diff --git a/socketsecurity/socketcli.py b/socketsecurity/socketcli.py index 0d8bcccb..24e8e966 100644 --- a/socketsecurity/socketcli.py +++ b/socketsecurity/socketcli.py @@ -207,6 +207,8 @@ def main_code(): if dirs_to_include: core.config.excluded_dirs = set(core.config.excluded_dirs) - dirs_to_include log.debug(f"Re-including normally-excluded directories in scan: {sorted(dirs_to_include)}") + if config.excluded_ecosystems: + core.config.excluded_ecosystems = list(config.excluded_ecosystems) # Check for required dependencies if reachability analysis is enabled if config.reach: @@ -292,6 +294,9 @@ def main_code(): facts_file_to_submit = None # Variable to track SBOM files to submit when using --reach-use-only-pregenerated-sboms sbom_files_to_submit = None + # Manifest results retained from the --sub-path routing pre-check. Reusing + # these avoids walking every selected sub-path again during scan creation. + discovered_scan_files = None # Git setup is_repo = False @@ -534,14 +539,18 @@ def main_code(): # Override file checking to look in the scan paths instead # Get manifest files from all scan paths try: - all_scan_files = [] + discovered_scan_files = [] for scan_path in scan_paths: scan_files = core.find_files(scan_path) - all_scan_files.extend(scan_files) - has_supported_files = len(all_scan_files) > 0 - log.debug(f"Found {len(all_scan_files)} manifest files across {len(scan_paths)} scan paths") + discovered_scan_files.extend(scan_files) + has_supported_files = len(discovered_scan_files) > 0 + log.debug( + f"Found {len(discovered_scan_files)} manifest files across " + f"{len(scan_paths)} scan paths" + ) except Exception as e: log.debug(f"Error finding files in scan paths: {e}") + discovered_scan_files = None has_supported_files = False # Case 3: If no supported files or files are empty, force API mode (no PR comments) @@ -564,8 +573,6 @@ def main_code(): org_slug = core.config.org_slug if config.repo_is_public: core.config.repo_visibility = "public" - if config.excluded_ecosystems and len(config.excluded_ecosystems) > 0: - core.config.excluded_ecosystems = config.excluded_ecosystems integration_type = config.integration_type integration_org_slug = config.integration_org_slug or org_slug try: @@ -613,6 +620,12 @@ def main_code(): diff.diff_url = "" diff.report_url = "" + scan_explicit_files = ( + sbom_files_to_submit + if sbom_files_to_submit is not None + else discovered_scan_files + ) + # Handle SCM-specific flows log.debug(f"Flow decision: scm={scm is not None}, force_diff_mode={force_diff_mode}, force_api_mode={force_api_mode}, enable_diff={config.enable_diff}") @@ -684,7 +697,7 @@ def _is_unprocessed(c): log.info("Push initiated flow") if scm.check_event_type() == "diff": log.info("Starting comment logic for PR/MR event") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=sbom_files_to_submit) + diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) comments = scm.get_comments_for_pr() # FIXME: this overwrites diff.new_alerts, which was previously populated by Core.create_issue_alerts @@ -807,14 +820,14 @@ def _is_unprocessed(c): ) else: log.info("Starting non-PR/MR flow") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=sbom_files_to_submit) + diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) output_handler.handle_output(diff) elif (config.enable_diff or force_diff_mode) and not force_api_mode: # New logic: --enable-diff or force_diff_mode (from --ignore-commit-files in git repos) forces diff mode log.info("Diff mode enabled without SCM integration") - diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=sbom_files_to_submit) + diff = core.create_new_diff(scan_paths, params, no_change=should_skip_scan, save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, explicit_files=scan_explicit_files) output_handler.handle_output(diff) elif (config.enable_diff or force_diff_mode) and force_api_mode: @@ -834,7 +847,7 @@ def _is_unprocessed(c): save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, - explicit_files=sbom_files_to_submit + explicit_files=scan_explicit_files ) log.info(f"Full scan created with ID: {diff.id}") log.info(f"Full scan report URL: {diff.report_url}") @@ -842,7 +855,10 @@ def _is_unprocessed(c): else: if force_api_mode: - log.info("No Manifest files changed, creating Socket Report") + log.info( + "No supported manifest detected in the changed-file set; " + "creating a full Socket report" + ) serializable_params = { key: value if isinstance(value, (int, float, str, list, dict, bool, type(None))) else str(value) for key, value in params.__dict__.items() @@ -855,7 +871,7 @@ def _is_unprocessed(c): save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, - explicit_files=sbom_files_to_submit + explicit_files=scan_explicit_files ) log.info(f"Full scan created with ID: {diff.id}") log.info(f"Full scan report URL: {diff.report_url}") @@ -868,7 +884,7 @@ def _is_unprocessed(c): save_files_list_path=config.save_submitted_files_list, save_manifest_tar_path=config.save_manifest_tar, base_paths=base_paths, - explicit_files=sbom_files_to_submit + explicit_files=scan_explicit_files ) output_handler.handle_output(diff) diff --git a/tests/core/test_diff_scan_polling.py b/tests/core/test_diff_scan_polling.py index be369429..373f8a59 100644 --- a/tests/core/test_diff_scan_polling.py +++ b/tests/core/test_diff_scan_polling.py @@ -120,3 +120,98 @@ def test_fallback_to_streaming_diff_on_failure(core): ) assert "dp3" in added assert "dp2" in removed + + +def test_completion_log_reports_id_polls_and_final_wait( + core, diff_scan_get_response, no_sleep, caplog, monkeypatch +): + """The completion log must let a CI log separate backend compute time from the + time a finished comparison sat unnoticed between polls.""" + import logging + + monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS", 4.0) + monkeypatch.setattr(core_module, "DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS", 6.0) + processing = {"status": "processing", "id": "diff-scan-123"} + core.sdk.diffscans.get.side_effect = [processing, processing, diff_scan_get_response] + + with caplog.at_level(logging.INFO, logger="socketdev"): + core.get_diff_scan_artifacts("head", "new") + + messages = [record.message for record in caplog.records] + assert any("Diff scan created: id=" in message for message in messages) + ready = next(message for message in messages if "Diff scan comparison ready" in message) + assert "polls=3" in ready + # Waits were 4s then 6s (capped); the final poll followed the 6s wait, which is + # the upper bound on how long the result was ready before being observed. + assert "wait_before_final_poll=6s" in ready + + +def test_max_poll_interval_bounds_dead_time_for_ci_budgets(): + """A finished comparison is never left unobserved longer than the max interval.""" + assert core_module.DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS <= 10.0 + assert ( + core_module.DIFF_SCAN_POLL_INITIAL_INTERVAL_SECONDS + <= core_module.DIFF_SCAN_POLL_MAX_INTERVAL_SECONDS + ) + + +UNCHANGED_ARTIFACT_CONSUMERS = [ + # flag name, value that makes the flag active + ("strict_blocking", True), + ("enable_gitlab_security", True), + ("generate_license", True), + ("legal_format", "fossa"), +] + + +@pytest.mark.parametrize(("flag", "value"), UNCHANGED_ARTIFACT_CONSUMERS) +def test_unchanged_artifacts_gating(core, diff_scan_get_response, flag, value): + """Any output that reads unchanged artifacts must keep them in the response. + + This pins the consumer list in Core._requires_unchanged_artifacts: adding a new + reader of diff.unchanged_alerts or diff.packages without adding it here (and to + that method) would silently ship an empty result to that output. + """ + from types import SimpleNamespace + + defaults = {name: (False if name != "legal_format" else "socket") + for name, _ in UNCHANGED_ARTIFACT_CONSUMERS} + core.cli_config = SimpleNamespace(**{**defaults, flag: value}) + core.sdk.diffscans.get.side_effect = None + core.sdk.diffscans.get.return_value = diff_scan_get_response + + core.get_diff_scan_artifacts("head", "new") + + params = core.sdk.diffscans.get.call_args.kwargs["params"] + assert "omit_unchanged" not in params, f"{flag}={value} still needs unchanged artifacts" + + +def test_unchanged_artifacts_omitted_when_no_output_reads_them(core, diff_scan_get_response): + """With no such flag set, the ~1 KB-per-artifact unchanged half is not fetched.""" + from types import SimpleNamespace + + core.cli_config = SimpleNamespace( + strict_blocking=False, + enable_gitlab_security=False, + generate_license=False, + legal_format="socket", + ) + core.sdk.diffscans.get.side_effect = None + core.sdk.diffscans.get.return_value = diff_scan_get_response + + core.get_diff_scan_artifacts("head", "new") + + params = core.sdk.diffscans.get.call_args.kwargs["params"] + assert params["cached"] == "true" + assert params["omit_unchanged"] == "true" + + +def test_unknown_caller_keeps_full_payload(core, diff_scan_get_response): + """cli_config is optional; without it, do not assume unchanged is unused.""" + core.cli_config = None + core.sdk.diffscans.get.side_effect = None + core.sdk.diffscans.get.return_value = diff_scan_get_response + + core.get_diff_scan_artifacts("head", "new") + + assert "omit_unchanged" not in core.sdk.diffscans.get.call_args.kwargs["params"] diff --git a/tests/unit/test_git_interface.py b/tests/unit/test_git_interface.py new file mode 100644 index 00000000..a22cf634 --- /dev/null +++ b/tests/unit/test_git_interface.py @@ -0,0 +1,251 @@ +import logging +import subprocess +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from socketsecurity.core.git_interface import Git + +CI_ENVIRONMENT_VARIABLES = ( + "BUILDKITE", + "BUILDKITE_BRANCH", + "BUILDKITE_COMMIT", + "BUILDKITE_PIPELINE_DEFAULT_BRANCH", + "BUILDKITE_PULL_REQUEST", + "BUILDKITE_PULL_REQUEST_BASE_BRANCH", + "GITHUB_BASE_REF", + "GITHUB_EVENT_BEFORE", + "GITHUB_EVENT_NAME", + "GITHUB_HEAD_REF", + "GITHUB_REF", + "GITHUB_SHA", + "CI_COMMIT_BRANCH", + "CI_COMMIT_SHA", + "CI_DEFAULT_BRANCH", + "CI_MERGE_REQUEST_SOURCE_BRANCH_NAME", + "CI_MERGE_REQUEST_TARGET_BRANCH_NAME", + "BITBUCKET_BRANCH", + "BITBUCKET_COMMIT", + "BITBUCKET_PR_DESTINATION_BRANCH", + "BITBUCKET_PR_ID", +) + + +@pytest.fixture(autouse=True) +def clear_ci_environment(monkeypatch): + for variable in CI_ENVIRONMENT_VARIABLES: + monkeypatch.delenv(variable, raising=False) + + +def _git(path, *args): + return subprocess.run( + ["git", *args], + cwd=path, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +@pytest.fixture +def pull_request_repo(tmp_path): + path = tmp_path / "repo" + path.mkdir() + _git(path, "init", "-b", "main") + _git(path, "config", "user.name", "Socket Test") + _git(path, "config", "user.email", "socket@example.com") + (path / "README.md").write_text("base\n", encoding="utf-8") + _git(path, "add", "README.md") + _git(path, "commit", "-m", "base") + _git(path, "checkout", "-b", "feature") + (path / "package.json").write_text("{}\n", encoding="utf-8") + _git(path, "add", "package.json") + _git(path, "commit", "-m", "add manifest") + return path + + +@pytest.mark.parametrize( + ("environment", "expected_branch", "expected_source"), + [ + ( + { + "BUILDKITE": "true", + "BUILDKITE_BRANCH": "feature", + "BUILDKITE_PULL_REQUEST": "123", + "BUILDKITE_PULL_REQUEST_BASE_BRANCH": "main", + }, + "feature", + "buildkite-pr", + ), + ( + { + "GITHUB_EVENT_NAME": "pull_request", + "GITHUB_BASE_REF": "main", + "GITHUB_HEAD_REF": "feature", + "GITHUB_REF": "refs/pull/123/merge", + }, + "feature", + "github-pr", + ), + ( + { + "CI_MERGE_REQUEST_SOURCE_BRANCH_NAME": "feature", + "CI_MERGE_REQUEST_TARGET_BRANCH_NAME": "main", + }, + "feature", + "gitlab-mr", + ), + ( + { + "BITBUCKET_BRANCH": "feature", + "BITBUCKET_PR_DESTINATION_BRANCH": "main", + "BITBUCKET_PR_ID": "123", + }, + "feature", + "bitbucket-pr", + ), + ], +) +def test_pull_request_context_uses_local_refs_without_fetch( + pull_request_repo, monkeypatch, mocker, caplog, + environment, expected_branch, expected_source, +): + head_sha = _git(pull_request_repo, "rev-parse", "HEAD") + sha_variable = { + "buildkite-pr": "BUILDKITE_COMMIT", + "github-pr": "GITHUB_SHA", + "gitlab-mr": "CI_COMMIT_SHA", + "bitbucket-pr": "BITBUCKET_COMMIT", + }[expected_source] + environment[sha_variable] = head_sha + for name, value in environment.items(): + monkeypatch.setenv(name, value) + + fetch = mocker.patch.object( + Git, + "_fetch_ref", + side_effect=AssertionError("unexpected fetch"), + ) + mocker.patch.object(Git, "ensure_safe_directory") + + with caplog.at_level(logging.INFO, logger="socketdev"): + repository = Git(str(pull_request_repo)) + + assert repository.branch == expected_branch + assert repository.changed_files == ["package.json"] + assert repository.is_default_branch is False + fetch.assert_not_called() + assert any( + f"source={expected_source}" in record.message + for record in caplog.records + ) + assert any( + "Git initialization completed" in record.message + for record in caplog.records + ) + + +def test_buildkite_native_context_wins_over_github_compatibility_shims( + pull_request_repo, monkeypatch, mocker +): + head_sha = _git(pull_request_repo, "rev-parse", "HEAD") + monkeypatch.setenv("BUILDKITE", "true") + monkeypatch.setenv("BUILDKITE_BRANCH", "feature") + monkeypatch.setenv("BUILDKITE_COMMIT", head_sha) + monkeypatch.setenv("BUILDKITE_PULL_REQUEST", "123") + monkeypatch.setenv("BUILDKITE_PULL_REQUEST_BASE_BRANCH", "main") + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_BASE_REF", "wrong-base") + monkeypatch.setenv("GITHUB_HEAD_REF", "wrong-head") + mocker.patch.object( + Git, + "_fetch_ref", + side_effect=AssertionError("unexpected fetch"), + ) + mocker.patch.object(Git, "ensure_safe_directory") + + repository = Git(str(pull_request_repo)) + + assert repository.changed_files == ["package.json"] + + +def test_regular_initialization_never_fetches_all(pull_request_repo, mocker): + fetch = mocker.patch.object( + Git, + "_fetch_ref", + side_effect=AssertionError("unexpected fetch"), + ) + mocker.patch.object(Git, "ensure_safe_directory") + + repository = Git(str(pull_request_repo)) + + assert repository.commit_str == _git(pull_request_repo, "rev-parse", "HEAD") + assert repository.changed_files == ["package.json"] + fetch.assert_not_called() + + +def test_detached_head_uses_buildkite_branch_and_commit(pull_request_repo, monkeypatch, mocker): + head_sha = _git(pull_request_repo, "rev-parse", "HEAD") + _git(pull_request_repo, "checkout", "--detach", head_sha) + monkeypatch.setenv("BUILDKITE", "true") + monkeypatch.setenv("BUILDKITE_BRANCH", "feature") + monkeypatch.setenv("BUILDKITE_COMMIT", head_sha) + monkeypatch.setenv("BUILDKITE_PULL_REQUEST", "123") + monkeypatch.setenv("BUILDKITE_PULL_REQUEST_BASE_BRANCH", "main") + mocker.patch.object( + Git, + "_fetch_ref", + side_effect=AssertionError("unexpected fetch"), + ) + mocker.patch.object(Git, "ensure_safe_directory") + + repository = Git(str(pull_request_repo)) + + assert repository.commit_str == head_sha + assert repository.branch == "feature" + assert repository.changed_files == ["package.json"] + + +def test_missing_base_ref_fetches_only_that_ref( + pull_request_repo, monkeypatch, mocker, caplog +): + head_sha = _git(pull_request_repo, "rev-parse", "HEAD") + monkeypatch.setenv("BUILDKITE_BRANCH", "feature") + monkeypatch.setenv("BUILDKITE_COMMIT", head_sha) + monkeypatch.setenv("BUILDKITE_PULL_REQUEST", "123") + monkeypatch.setenv("BUILDKITE_PULL_REQUEST_BASE_BRANCH", "remote-main") + mocker.patch.object(Git, "ensure_safe_directory") + base_sha = _git(pull_request_repo, "rev-parse", "main") + fetch = mocker.patch.object(Git, "_fetch_ref", return_value=base_sha) + + with caplog.at_level(logging.INFO, logger="socketdev"): + repository = Git(str(pull_request_repo)) + + fetch.assert_called_once_with( + "remote-main", + "Buildkite pull-request base ref missing", + ) + assert repository.changed_files == ["package.json"] + + +def test_targeted_fetch_never_uses_all(): + repository = Git.__new__(Git) + repository.repo = MagicMock() + repository._fetched_ref_commits = {} + main_sha = "a" * 40 + repository.repo.commit.return_value = SimpleNamespace(hexsha=main_sha) + + result = repository._fetch_ref("main", "test") + + repository.repo.git.fetch.assert_called_once_with("origin", "main") + assert "--all" not in repository.repo.git.fetch.call_args.args + assert result == main_sha + + +@pytest.mark.parametrize( + ("value", "expected"), + [(None, False), ("", False), ("false", False), ("False", False), ("0", True), ("123", True)], +) +def test_buildkite_pull_request_detection(value, expected): + assert Git._is_buildkite_pull_request(value) is expected diff --git a/tests/unit/test_github_buildkite_config.py b/tests/unit/test_github_buildkite_config.py new file mode 100644 index 00000000..23f41507 --- /dev/null +++ b/tests/unit/test_github_buildkite_config.py @@ -0,0 +1,173 @@ +import pytest + +from socketsecurity.core.scm.github import Github, GithubConfig + +CONTEXT_VARIABLES = ( + "BUILDKITE", + "BUILDKITE_BRANCH", + "BUILDKITE_BUILD_CHECKOUT_PATH", + "BUILDKITE_BUILD_CREATOR", + "BUILDKITE_COMMIT", + "BUILDKITE_MESSAGE", + "BUILDKITE_PIPELINE_DEFAULT_BRANCH", + "BUILDKITE_PULL_REQUEST", + "BUILDKITE_PULL_REQUEST_REPO", + "BUILDKITE_REPO", + "DEFAULT_BRANCH", + "EVENT_ACTION", + "GH_API_TOKEN", + "GITHUB_ACTOR", + "GITHUB_API_URL", + "GITHUB_EVENT_NAME", + "GITHUB_EVENT_PATH", + "GITHUB_REF_NAME", + "GITHUB_REF_TYPE", + "GITHUB_REPOSITORY", + "GITHUB_REPOSITORY_OWNER", + "GITHUB_SHA", + "GITHUB_WORKSPACE", + "PR_NUMBER", +) + + +@pytest.fixture(autouse=True) +def clear_context(monkeypatch): + for variable in CONTEXT_VARIABLES: + monkeypatch.delenv(variable, raising=False) + monkeypatch.setenv("GH_API_TOKEN", "test-token") + + +def test_github_config_uses_native_buildkite_pull_request_context(monkeypatch): + values = { + "BUILDKITE": "true", + "BUILDKITE_BRANCH": "feature/socket", + "BUILDKITE_BUILD_CHECKOUT_PATH": "/workspace/repo", + "BUILDKITE_BUILD_CREATOR": "octocat", + "BUILDKITE_COMMIT": "a" * 40, + "BUILDKITE_MESSAGE": "Update dependencies", + "BUILDKITE_PIPELINE_DEFAULT_BRANCH": "main", + "BUILDKITE_PULL_REQUEST": "123", + "BUILDKITE_PULL_REQUEST_REPO": "git@github.com:acme/widgets.git", + "BUILDKITE_REPO": "git@github.com:acme/widgets.git", + } + for name, value in values.items(): + monkeypatch.setenv(name, value) + + config = GithubConfig.from_env() + + assert config.sha == "a" * 40 + assert config.api_url == "https://api.github.com" + assert config.ref_type == "branch" + assert config.event_name == "pull_request" + assert config.event_action == "synchronize" + assert config.workspace == "/workspace/repo" + assert config.owner == "acme" + assert config.repository == "widgets" + assert config.ref_name == "feature/socket" + assert config.pr_number == "123" + assert config.commit_message == "Update dependencies" + assert config.actor == "octocat" + assert config.is_default_branch is False + assert Github(client=object(), config=config).check_event_type() == "diff" + + +def test_buildkite_non_pr_build_uses_push_and_default_branch(monkeypatch): + values = { + "BUILDKITE": "true", + "BUILDKITE_BRANCH": "main", + "BUILDKITE_COMMIT": "b" * 40, + "BUILDKITE_PIPELINE_DEFAULT_BRANCH": "main", + "BUILDKITE_PULL_REQUEST": "false", + "BUILDKITE_REPO": "https://github.com/acme/widgets.git", + } + for name, value in values.items(): + monkeypatch.setenv(name, value) + + config = GithubConfig.from_env() + + assert config.event_name == "push" + assert config.pr_number is None + assert config.owner == "acme" + assert config.repository == "widgets" + assert config.is_default_branch is True + assert Github(client=object(), config=config).check_event_type() == "main" + + +@pytest.mark.parametrize( + "branch_variables", + [ + {}, + {"BUILDKITE_BRANCH": "feature/socket"}, + {"BUILDKITE_PIPELINE_DEFAULT_BRANCH": "main"}, + ], +) +def test_buildkite_default_branch_requires_a_matching_branch_name( + monkeypatch, branch_variables +): + """Absent branch context must not be read as 'this build is the default branch'.""" + monkeypatch.setenv("BUILDKITE", "true") + for name, value in branch_variables.items(): + monkeypatch.setenv(name, value) + + config = GithubConfig.from_env() + + assert config.is_default_branch is False + assert config.default_branch is False + + +def test_explicit_github_values_take_priority_in_buildkite(monkeypatch): + values = { + "BUILDKITE": "true", + "BUILDKITE_BRANCH": "buildkite-branch", + "BUILDKITE_COMMIT": "b" * 40, + "BUILDKITE_PULL_REQUEST": "123", + "BUILDKITE_REPO": "git@github.com:buildkite/repository.git", + "EVENT_ACTION": "opened", + "GITHUB_API_URL": "https://github.example/api/v3", + "GITHUB_EVENT_NAME": "pull_request", + "GITHUB_REF_NAME": "github-branch", + "GITHUB_REF_TYPE": "branch", + "GITHUB_REPOSITORY": "github/repository", + "GITHUB_SHA": "c" * 40, + "GITHUB_WORKSPACE": "/github/workspace", + "PR_NUMBER": "456", + } + for name, value in values.items(): + monkeypatch.setenv(name, value) + + config = GithubConfig.from_env() + + assert config.sha == "c" * 40 + assert config.api_url == "https://github.example/api/v3" + assert config.workspace == "/github/workspace" + assert config.owner == "github" + assert config.repository == "repository" + assert config.ref_name == "github-branch" + assert config.pr_number == "456" + assert config.event_action == "opened" + + +@pytest.mark.parametrize( + ("repository_url", "expected"), + [ + ("git@github.com:acme/widgets.git", ("acme", "widgets")), + ("https://github.com/acme/widgets.git", ("acme", "widgets")), + ("ssh://git@github.com/acme/widgets.git", ("acme", "widgets")), + ("", ("", "")), + ("not-a-repository", ("", "")), + ], +) +def test_buildkite_repository_url_parsing(monkeypatch, repository_url, expected): + monkeypatch.setenv("BUILDKITE_REPO", repository_url) + + assert GithubConfig._repository_from_buildkite() == expected + + +def test_buildkite_pipeline_repository_wins_over_pull_request_fork(monkeypatch): + monkeypatch.setenv("BUILDKITE_REPO", "git@github.com:acme/widgets.git") + monkeypatch.setenv( + "BUILDKITE_PULL_REQUEST_REPO", + "git@github.com:contributor/widgets.git", + ) + + assert GithubConfig._repository_from_buildkite() == ("acme", "widgets") diff --git a/tests/unit/test_manifest_discovery.py b/tests/unit/test_manifest_discovery.py new file mode 100644 index 00000000..40af698e --- /dev/null +++ b/tests/unit/test_manifest_discovery.py @@ -0,0 +1,331 @@ +import logging +import os +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from socketsecurity.core import Core +from socketsecurity.core.socket_config import SocketConfig, default_exclude_dirs +from socketsecurity.core.utils import socket_globs + + +def _make_core(*, patterns=socket_globs, excluded_dirs=None, exclude_paths=None): + core = Core.__new__(Core) + core.config = SocketConfig( + api_key="test-key", + excluded_dirs=set(default_exclude_dirs if excluded_dirs is None else excluded_dirs), + ) + core.cli_config = SimpleNamespace(exclude_paths=exclude_paths) + core.sdk = MagicMock() + core._supported_patterns = patterns + return core + + +def _write_files(root: Path, relative_paths): + for relative_path in relative_paths: + target = root / relative_path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("test\n", encoding="utf-8") + + +def _relative_results(root: Path, results): + return {Path(result).relative_to(root).as_posix() for result in results} + + +ALL_PATTERN_EXAMPLES = { + "app.spdx.json", + "bom.json", + "nested/app-cdx.json", + "nested/app-cyclonedx.xml", + "package.json", + "nested/package-lock.json", + "npm-shrinkwrap.json", + "yarn.lock", + "pnpm-lock.yaml", + "pnpm-lock.yml", + "pnpm-workspace.yaml", + "pnpm-workspace.yml", + "bun.lock", + "bun.lockb", + "vlt-lock.json", + "PIPFILE", + "pyproject.toml", + "poetry.lock", + "requirements.txt", + "dev-requirements.txt", + "requirements-dev.txt", + "requirements_test.txt", + "requirements.frozen", + "requirements/base.txt", + "nested/requirements/constraints.txt", + "setup.py", + "go.mod", + "go.sum", + "pom.xml", + "src/Project.CSPROJ", + "Directory.Build.Props", + "build.targets", + "project.nuspec", + "nuget.CONFIG", + "packages.config", + "packages.lock.json", +} + + +def test_all_builtin_manifest_patterns_match_in_one_walk(tmp_path, mocker): + _write_files( + tmp_path, + ALL_PATTERN_EXAMPLES + | { + "README.md", + "requirements/deep/not-a-direct-child.txt", + "src/package.json.backup", + }, + ) + original_walk = os.walk + walk = mocker.patch("socketsecurity.core.os.walk", wraps=original_walk) + + found = _relative_results(tmp_path, _make_core().find_files(str(tmp_path))) + + assert found == ALL_PATTERN_EXAMPLES + walk.assert_called_once() + + +def test_single_walk_matches_legacy_rglob_results_for_builtin_patterns(tmp_path): + _write_files( + tmp_path, + ALL_PATTERN_EXAMPLES + | { + ".hidden/package.json", + "nested/Requirements.TXT", + "src/not-a-manifest.json", + }, + ) + core = _make_core(excluded_dirs=set()) + + legacy_results = set() + for ecosystem_patterns in socket_globs.values(): + for details in ecosystem_patterns.values(): + for expanded in Core.expand_brace_pattern(details["pattern"]): + case_insensitive = Core.to_case_insensitive_regex(expanded) + for result in tmp_path.rglob(case_insensitive): + if result.is_file(): + legacy_results.add(result.as_posix()) + + assert set(core.find_files(str(tmp_path))) == legacy_results + + +def test_prunes_git_default_globs_and_exclude_paths_before_descent( + tmp_path, mocker, caplog +): + _write_files( + tmp_path, + { + "package.json", + ".git/objects/package.json", + "node_modules/pkg/package.json", + "generated.egg-info/package.json", + "legacy/nested/package.json", + ".hidden/package.json", + }, + ) + scanned_directories = [] + original_scandir = os.scandir + + def tracking_scandir(path): + scanned_directories.append(Path(path).relative_to(tmp_path).as_posix()) + return original_scandir(path) + + mocker.patch("socketsecurity.core.os.scandir", side_effect=tracking_scandir) + core = _make_core(exclude_paths=["legacy"]) + + with caplog.at_level(logging.INFO, logger="socketdev"): + found = _relative_results(tmp_path, core.find_files(str(tmp_path))) + + assert found == {"package.json", ".hidden/package.json"} + assert ".git" not in scanned_directories + assert "node_modules" not in scanned_directories + assert "generated.egg-info" not in scanned_directories + assert "legacy" not in scanned_directories + assert any( + "directories_pruned=4" in record.message + and "manifests_found=2" in record.message + for record in caplog.records + ) + + +def test_include_dirs_and_excluded_ecosystems_are_preserved(tmp_path): + _write_files( + tmp_path, + { + "build/package.json", + "build/requirements.txt", + "dist/package.json", + }, + ) + core = _make_core(excluded_dirs=set(default_exclude_dirs) - {"build"}) + core.config.excluded_ecosystems = ["npm"] + + found = _relative_results(tmp_path, core.find_files(str(tmp_path))) + + assert found == {"build/requirements.txt"} + + +def test_excluding_every_ecosystem_skips_the_filesystem_walk(tmp_path, mocker): + core = _make_core() + core.config.excluded_ecosystems = list(socket_globs) + walk = mocker.patch( + "socketsecurity.core.os.walk", + side_effect=AssertionError("unexpected walk"), + ) + + assert core.find_files(str(tmp_path)) == [] + walk.assert_not_called() + + +def test_symlinked_file_is_included_but_symlinked_directory_is_not_followed(tmp_path): + if not hasattr(os, "symlink"): + pytest.skip("symlinks are not supported") + + source_file = tmp_path / "source.txt" + source_file.write_text("{}", encoding="utf-8") + source_directory = tmp_path / "external" + _write_files(source_directory, {"package.json"}) + try: + (tmp_path / "package.json").symlink_to(source_file) + (tmp_path / "linked-directory").symlink_to(source_directory, target_is_directory=True) + except OSError as error: + pytest.skip(f"symlinks are unavailable: {error}") + + found = _relative_results(tmp_path, _make_core().find_files(str(tmp_path))) + + assert "package.json" in found + assert "linked-directory/package.json" not in found + assert "external/package.json" in found + + +def test_supported_patterns_are_cached_without_mutating_sdk_response(): + response = { + "general": {"ignored": {"pattern": "ignored"}}, + "npm": {"package.json": {"pattern": "package.json"}}, + } + core = _make_core(patterns=None) + core.sdk.report.supported.return_value = response + + first = core.get_supported_patterns() + second = core.get_supported_patterns() + + assert first is second + assert first == {"npm": {"package.json": {"pattern": "package.json"}}} + assert "general" in response + core.sdk.report.supported.assert_called_once_with() + + +def test_failed_pattern_lookup_is_not_cached(): + """A transient API failure must not pin the run to the smaller local fallback.""" + api_response = {"npm": {"package.json": {"pattern": "package.json"}}} + core = _make_core(patterns=None) + core.sdk.report.supported.side_effect = [None, api_response] + + fallback = core.get_supported_patterns() + assert set(fallback) == set(socket_globs) + + recovered = core.get_supported_patterns() + assert set(recovered) == {"npm"} + # The successful lookup is still cached, so the API is not re-queried again. + assert core.get_supported_patterns() is recovered + assert core.sdk.report.supported.call_count == 2 + + +def test_basename_prefilter_admits_every_supported_manifest(): + """The cheap prefilter must never reject a path the authoritative matcher accepts.""" + patterns = Core._prepare_manifest_patterns(socket_globs, None, []) + + for relative_path in ALL_PATTERN_EXAMPLES: + basename = relative_path.rsplit("/", 1)[-1].casefold() + assert Core._matches_manifest_pattern(relative_path, patterns), relative_path + assert Core._basename_could_match(basename, patterns), relative_path + + +def test_results_are_sorted_and_deduplicated_across_overlapping_patterns(tmp_path): + _write_files(tmp_path, {"z/package.json", "a/package.json"}) + overlapping_patterns = { + "npm": { + "literal": {"pattern": "package.json"}, + "wildcard": {"pattern": "package*.json"}, + } + } + + found = _make_core(patterns=overlapping_patterns).find_files(str(tmp_path)) + + assert found == sorted(found) + assert len(found) == 2 + + +def test_explicit_discovery_results_prevent_a_second_walk(tmp_path): + manifest = tmp_path / "package.json" + manifest.write_text("{}", encoding="utf-8") + core = _make_core() + core.config.org_slug = "example" + core.cli_config = None + core.find_files = MagicMock(side_effect=AssertionError("unexpected second walk")) + core.create_full_scan = MagicMock(return_value=SimpleNamespace(id="scan-123")) + params = MagicMock() + + diff = core.create_full_scan_with_report_url( + [str(tmp_path)], + params, + explicit_files=[manifest.as_posix()], + ) + + core.find_files.assert_not_called() + core.create_full_scan.assert_called_once_with( + [manifest.as_posix()], + params, + base_paths=None, + ) + assert diff.id == "scan-123" + + +def test_core_initialization_logs_organization_timing(caplog): + sdk = MagicMock() + sdk.org.get.return_value = { + "organizations": {"org-id": {"slug": "example"}}, + } + + with caplog.at_level(logging.INFO, logger="socketdev"): + core = Core(SocketConfig(api_key="test-key"), sdk) + + assert core.config.org_slug == "example" + assert any( + "Organization initialization completed" in record.message + for record in caplog.records + ) + + +def test_discovery_does_not_build_a_repository_sized_index(tmp_path): + """Peak memory must stay bounded by the widest directory and the result set, not + by repository size. This is the property that keeps discovery viable on small + runners; the per-pattern rglob approach it replaced allocated strictly more. + """ + import tracemalloc + + wide_directory = tmp_path / "wide" + wide_directory.mkdir() + for index in range(20000): + (wide_directory / f"source{index:05d}.ts").write_text("x", encoding="utf-8") + (tmp_path / "package.json").write_text("{}", encoding="utf-8") + + core = _make_core() + tracemalloc.start() + try: + found = core.find_files(str(tmp_path)) + _, peak_bytes = tracemalloc.get_traced_memory() + finally: + tracemalloc.stop() + + assert _relative_results(tmp_path, found) == {"package.json"} + # 20k files in one directory; a repo-sized index would be far larger than this. + assert peak_bytes < 8_000_000, f"peak allocation was {peak_bytes / 1e6:.1f} MB" diff --git a/tests/unit/test_streaming.py b/tests/unit/test_streaming.py index 999b44a8..3b9cbd9b 100644 --- a/tests/unit/test_streaming.py +++ b/tests/unit/test_streaming.py @@ -18,14 +18,19 @@ def _make(**overrides): return StreamingLogs(**kwargs) -def test_setup_streaming_is_noop_when_register_fails(): +def test_setup_streaming_is_noop_when_register_fails(caplog): finalize_calls = [] - with patch("socketsecurity.core.streaming.register_cli_run", return_value=None), \ - patch("socketsecurity.core.streaming.finalize_cli_run", side_effect=lambda *a, **k: finalize_calls.append(k)): - with _make(cli_name="t-fail-cli", sdk_name="t-fail-sdk") as streaming: - assert isinstance(streaming, StreamingLogs) + with caplog.at_level(logging.INFO, logger="t-fail-cli"): + with patch("socketsecurity.core.streaming.register_cli_run", return_value=None), \ + patch("socketsecurity.core.streaming.finalize_cli_run", side_effect=lambda *a, **k: finalize_calls.append(k)): + with _make(cli_name="t-fail-cli", sdk_name="t-fail-sdk") as streaming: + assert isinstance(streaming, StreamingLogs) # No run was registered → finalize must not be called. assert finalize_calls == [] + assert any( + "CLI run registration completed" in record.message + for record in caplog.records + ) def test_clean_exit_reports_success(): diff --git a/uv.lock b/uv.lock index 8fd80913..156b9936 100644 --- a/uv.lock +++ b/uv.lock @@ -1282,7 +1282,7 @@ wheels = [ [[package]] name = "socketsecurity" -version = "2.6.4" +version = "2.6.5" source = { editable = "." } dependencies = [ { name = "beautifulsoup4" },