From bf49494e5f52c2f7cdf690bbe8a6e276f30b1cad Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 15:01:43 -0400 Subject: [PATCH 1/9] fix: make changed_files scoping reach every config path and fail loudly changed_files was resolved in exactly one place, create_config_from_args, and INPUT_CHANGED_FILES was missing from the environment loader entirely (unlike INPUT_SCAN_ALL and INPUT_SCAN_FILES). A Config built any other way silently scanned the whole repository, and a raw string value such as 'auto' from a --config JSON file or a Socket dashboard config was never git-resolved -- _resolve_file_targets iterated the string and looked for files named a, u, t and o. Every source now goes through one resolver, called from Config, so the action input, the CLI flag, the env var, a JSON config and a dashboard config are all honored identically. Then make the failures visible. The PR base is now also read from pull_request.base.sha/ref in the GitHub event payload, since GITHUB_BASE_REF is only set on pull_request triggers. When no base can be resolved the run names what it tried and why -- shallow checkout (pointing at fetch-depth: 0), workspace is not a git repository, git refusing to read the repository, or no PR base at all. A scope that resolves to zero files warns that the scanners are being skipped, and scan_all now warns when it discards a requested scope instead of overriding it in silence. Every one of those previously returned an empty list with no log output. TruffleHog and Trivy no longer substitute their own staged-file scope when an explicit request resolved to nothing. Refs: SURF-1452 --- CHANGELOG.md | 27 ++ action.yml | 15 +- docs/github-action.md | 34 +- docs/parameters.md | 31 +- socket_basics/core/config.py | 376 ++++++++++++++---- socket_basics/core/connector/base.py | 18 +- socket_basics/core/connector/trivy/trivy.py | 6 +- .../core/connector/trufflehog/__init__.py | 7 +- tests/test_changed_files_scope.py | 373 ++++++++++++++++- 9 files changed, 785 insertions(+), 102 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fde72b..8a55d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,33 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- `changed_files` is now honored no matter which way the config is built. It was + only resolved inside `create_config_from_args()`, so `INPUT_CHANGED_FILES` was + missing from the environment loader entirely (unlike `INPUT_SCAN_ALL` and + `INPUT_SCAN_FILES`) and any config built another way scanned the whole + repository. All sources now go through one resolver. +- A raw `changed_files` string from a `--config` JSON file or a Socket dashboard + config is now resolved against git. `"auto"` used to be stored verbatim and + then iterated character by character, looking for files named `a`, `u`, `t` + and `o`, which scoped the scan to nothing. +- `scan_all` still overrides `changed_files`, but now logs a warning naming the + scope it discarded. It can come from a Socket dashboard config rather than the + workflow, so discarding the request silently made diff-only mode look broken. +- Pull request base resolution now falls back to `pull_request.base.sha` and + `pull_request.base.ref` from the GitHub event payload when `GITHUB_BASE_REF` + is unset, which is the case on any trigger other than `pull_request` and + `pull_request_target`. +- A scope request that cannot be honored now says why. Shallow checkouts + (naming `fetch-depth: 0`), a workspace that is not a git repository, git + refusing to read the repository (container ownership mismatch), and a missing + PR base each log a specific warning, and a scope that resolves to zero files + warns that the scanners are being skipped. All of these previously returned an + empty list in complete silence. +- TruffleHog and Trivy no longer substitute their own staged-file scope when an + explicit `changed_files` request resolved to nothing. + ## [2.2.1] - 2026-07-30 ### Fixed diff --git a/action.yml b/action.yml index dcb6dc7..3a8fc79 100644 --- a/action.yml +++ b/action.yml @@ -112,12 +112,15 @@ inputs: description: >- Diff-only mode: scope every scanner (SAST/OpenGrep, secrets, containers) to changed files only, instead of the whole repository. Accepts a - comma-separated file list, a commit hash, 'auto' (diffs against the PR - base branch in CI, else staged changes), 'pr' (diff against - GITHUB_BASE_REF), or 'current-commit'. For PR/'auto' modes, check out with - actions/checkout fetch-depth: 0 so the base branch is available. When the - diff resolves to no existing files (e.g. a delete-only PR) the scanners - are skipped rather than scanning the whole repo. + comma-separated file list, a commit hash, 'auto' (the PR base diff when a + pull request base can be found, else staged changes), 'pr' (the PR base + diff only), or 'current-commit'. The PR base comes from GITHUB_BASE_REF or + from pull_request.base in the GitHub event payload. For PR/'auto' modes, + check out with actions/checkout fetch-depth: 0 so the base branch is + available. When the diff resolves to no existing files (e.g. a delete-only + PR) the scanners are skipped rather than scanning the whole repo, and when + the scope cannot be resolved at all the run logs a warning saying why. + Note that scan_all overrides this input. required: false default: "" scan_files: diff --git a/docs/github-action.md b/docs/github-action.md index b9ba113..a1dff0f 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -311,8 +311,9 @@ jobs: `changed_files` accepts: -- `auto` — diff against the PR base branch in CI (`GITHUB_BASE_REF`), else staged changes -- `pr` — diff against the PR base branch (`GITHUB_BASE_REF`) +- `auto` — the PR base diff when a pull request base can be found, else staged changes +- `pr` — the PR base diff, and nothing else +- `current-commit` — files in the `HEAD` commit - a commit hash — files changed in that commit - a comma-separated file list — e.g. `src/app.py,src/utils.js` @@ -322,6 +323,35 @@ jobs: > nothing rather than falling back to the whole repo. To scan an explicit file > list regardless of git state, use the `scan_files` input instead. +### Checking that the scope took effect + +Diff-only mode logs what it did. Look for these lines in the step output: + +```text +INFO Resolved PR diff base to 'origin/main' +INFO Diff-only scan scoping requested (changed_files=auto): resolved 12 changed file(s) +INFO Diff-only scan scoping active: 12 scan target(s) from 12 changed file(s) +``` + +If the scope could not be applied, the run says why instead of quietly scanning +everything or nothing: + +| Warning you will see | What to do | +|----------------------|------------| +| `none of the candidate PR bases (...) could be resolved ... The checkout is shallow` | Add `fetch-depth: 0` to `actions/checkout` | +| `no pull request base was found` | The trigger is not `pull_request`, so there is no base. Use `changed_files: 'current-commit'` or an explicit file list | +| `is not a git repository` | Run `actions/checkout` before the scan step | +| `git refused to read ... detected dubious ownership` | Run `git config --global --add safe.directory "$GITHUB_WORKSPACE"` before the scan step | +| `scan_all is enabled ... changed-files scope ... is being ignored` | Unset `scan_all` — it can come from a Socket dashboard config, not just your workflow | +| `resolved to zero files. The scanners will be SKIPPED` | The diff found nothing scannable. Combined with a warning above, it tells you the diff failed rather than the PR being empty | + +### Where the setting can come from + +`changed_files` is honored identically from the action input, the +`INPUT_CHANGED_FILES` environment variable, the `--changed-files` CLI flag, a +`--config` JSON file, and a Socket dashboard config. `scan_all` outranks all of +them; when it does, the run logs a warning naming the scope it discarded. + ## PR Comment Customization Socket Basics automatically posts enhanced PR comments with **smart defaults that work out of the box** — clickable file links, collapsible sections, syntax highlighting, CVE links, CVSS scores, and auto-labels are all enabled by default. diff --git a/docs/parameters.md b/docs/parameters.md index 4e8ed3f..4f9c03e 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -109,17 +109,42 @@ changed files only, the way Socket SCA Pull Request alerts behave. Accepts: - a comma-separated file list (e.g. `src/app.py,src/utils.js`) - a commit hash — files changed in that commit -- `auto` — the PR base-ref diff when running in a PR CI context - (`GITHUB_BASE_REF` is set), otherwise staged (`--cached`) changes -- `pr` — diff against the PR base branch (`GITHUB_BASE_REF`) +- `auto` — the PR base diff when a pull request base can be found, otherwise + staged (`--cached`) changes +- `pr` — the PR base diff, and nothing else - `current-commit` — files in the `HEAD` commit +The same value works from the CLI (`--changed-files`), the `changed_files` +action input, the `INPUT_CHANGED_FILES` environment variable, a `--config` JSON +file, and a Socket dashboard config. Whichever way it arrives, it is resolved +against git once, in the same place. + Deletions are excluded from PR/`auto`/`pr` diffs so removed paths never become scan targets. When the diff resolves to no existing files (e.g. a delete-only PR), the scanners are skipped rather than falling back to scanning the whole repository. For PR/`auto`/`pr` modes, check out with full history (e.g. `actions/checkout` with `fetch-depth: 0`) so the base branch is available. +**Finding the pull request base.** `auto` and `pr` try, in order: + +1. `GITHUB_BASE_REF` — set by GitHub on `pull_request` and + `pull_request_target` triggers only +2. `pull_request.base.sha` from the event payload at `GITHUB_EVENT_PATH` — an + exact commit, so it works even when no remote-tracking branch exists +3. `pull_request.base.ref` from the same payload + +Each candidate is tried as `origin/` and then bare. If none resolves, the +run logs a warning naming what it tried and why (shallow checkout, workspace is +not a git repository, git refused to read the repository, no PR base at all) and +the scanners are skipped. **A scope request that cannot be honored is never +turned into a whole-repository scan, and it is never silent.** + +**`scan_all` outranks `changed_files`.** If `scan_all` is set — from +`INPUT_SCAN_ALL`, a JSON config, or a Socket dashboard config — the whole +workspace is scanned and the changed-files scope is discarded. The run logs a +warning saying so, because `scan_all` often comes from a different place than +the workflow that asked for diff-only scoping. + **Example:** ```bash socket-basics --changed-files auto diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index ac623b4..9285b24 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -7,6 +7,7 @@ import argparse import json import os +import re import yaml import logging from pathlib import Path @@ -224,7 +225,40 @@ def __init__(self, config_dict: Dict[str, Any] | None = None, json_config_path: self.workspace = ws self.output_dir = Path(self._config.get('output_dir', self.workspace)) self.scan_files = self._parse_scan_files(self._config.get('scan_files', '')) - + self._resolve_changed_files_scope() + + def _resolve_changed_files_scope(self) -> None: + """Turn whatever ``changed_files`` we were handed into a resolved list. + + ``changed_files`` can arrive as an already-resolved list (the CLI path + resolves it before building the config) or as a raw request string such + as ``auto``, ``pr``, a commit hash, or a comma-separated file list. The + raw string form reaches us from ``INPUT_CHANGED_FILES``, from a + ``--config`` JSON file, and from a Socket dashboard config. + + Resolving here means every construction path gets the same treatment. + Previously only ``create_config_from_args`` resolved the value, so a + config built any other way either ignored the scope request entirely + (scanning the whole repository) or, for a raw string, iterated the + string character by character looking for files named ``a``, ``u``, + ``t``, ``o``. + """ + raw = self._config.get('changed_files') + + if isinstance(raw, (list, tuple)): + self._config['changed_files'] = list(raw) + if raw: + self._config.setdefault('changed_files_scope_requested', True) + return + + if raw is None or not str(raw).strip(): + # Normalize to a list so callers never see '' or None here. + self._config['changed_files'] = [] + return + + self._config['changed_files_scope_requested'] = True + self._config['changed_files'] = resolve_changed_files_request(str(raw), str(self.workspace)) + def get(self, key: str, default=None): """Get configuration value""" return self._config.get(key, default) @@ -255,16 +289,37 @@ def get_scan_targets(self) -> List[str]: rather than falling back to scanning the whole workspace or their own working directory. """ + scope_requested = bool( + self.get('changed_files') or self.get('changed_files_scope_requested', False) + ) + # Explicit "scan everything" override. if self.get('scan_all', False): + if scope_requested: + # scan_all can come from a Socket dashboard config or a shared + # workflow template, so the person who set changed_files is not + # necessarily the person who set scan_all. Silently discarding + # the narrower request is what makes diff-only mode look broken. + logging.getLogger(__name__).warning( + "scan_all is enabled, so the whole workspace will be scanned and the " + "requested changed-files scope (%d file(s)) is being ignored. " + "Unset scan_all (INPUT_SCAN_ALL, or the scan_all key in your Socket " + "dashboard/JSON config) to scan only changed files.", + len(self.get('changed_files') or []), + ) return [str(self.workspace)] # Diff-only mode: scope the scan to the files changed in the PR/commit. # Keep honoring the scope when git resolves to zero files, e.g. a # delete-only PR, so callers skip instead of scanning the workspace. changed_files = self.get('changed_files', []) or [] - if changed_files or self.get('changed_files_scope_requested', False): - return self._resolve_file_targets(changed_files) + if scope_requested: + targets = self._resolve_file_targets(changed_files) + logging.getLogger(__name__).info( + "Diff-only scan scoping active: %d scan target(s) from %d changed file(s)", + len(targets), len(changed_files), + ) + return targets # Explicit list of files to scan. if self.scan_files: @@ -713,9 +768,12 @@ def load_config_from_env() -> Dict[str, Any]: 'workspace': os.getenv('GITHUB_WORKSPACE', os.getcwd()), 'output_dir': os.getenv('OUTPUT_DIR', os.getcwd()), - # Scan scope + # Scan scope. changed_files is kept as the raw request string here + # ('auto', 'pr', a commit hash, or a file list); Config resolves it + # against git once, so every construction path honors it identically. 'scan_all': os.getenv('INPUT_SCAN_ALL', 'false').lower() == 'true', 'scan_files': os.getenv('INPUT_SCAN_FILES', ''), + 'changed_files': os.getenv('INPUT_CHANGED_FILES', ''), # Core Socket API configuration (top-level, like workspace) 'socket_org': ( @@ -1017,6 +1075,8 @@ def load_explicit_env_config() -> Dict[str, Any]: config['scan_all'] = os.environ['INPUT_SCAN_ALL'].lower() == 'true' if 'INPUT_SCAN_FILES' in os.environ: config['scan_files'] = os.environ['INPUT_SCAN_FILES'] + if 'INPUT_CHANGED_FILES' in os.environ: + config['changed_files'] = os.environ['INPUT_CHANGED_FILES'] if 'INPUT_OPENGREP_RULES_DIR' in os.environ: config['opengrep_rules_dir'] = os.environ['INPUT_OPENGREP_RULES_DIR'] @@ -1559,55 +1619,13 @@ def create_config_from_args(args) -> Config: except Exception: pass - # Handle changed-files: CLI overrides env/config. Accept 'auto' to detect via git. - # When invoked via the GitHub Action (entrypoint passes no CLI args) the value - # arrives through the INPUT_CHANGED_FILES environment variable instead. + # Handle changed-files. Precedence is CLI -> env/JSON/dashboard config, and + # the raw request string ('auto', 'pr', a commit hash, or a file list) is + # stored as-is. Config resolves it against git exactly once, so a config + # built without going through here honors the scope the same way. changed_files_arg = getattr(args, 'changed_files', '') if args is not None else '' - if not changed_files_arg: - changed_files_arg = os.getenv('INPUT_CHANGED_FILES', '') - if changed_files_arg: - val = str(changed_files_arg).strip() - config_dict['changed_files_scope_requested'] = True - # 'auto' resolves to the PR base-ref diff in CI, else staged changes. - if val.lower() == 'auto': - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='auto') - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (auto): %s", e) - config_dict['changed_files'] = [] - elif val.lower() == 'pr': - # Explicit PR diff against the base branch (GITHUB_BASE_REF). - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='pr') - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (pr): %s", e) - config_dict['changed_files'] = [] - elif val.lower() in ('current-commit', 'current_commit'): - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='current-commit') - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (current-commit): %s", e) - config_dict['changed_files'] = [] - else: - # If value looks like a commit hash, list files in that commit - import re - if re.match(r'^[0-9a-fA-F]{7,40}$', val): - try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='commit', commit=val) - config_dict['changed_files'] = git_changed - except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (commit %s): %s", val, e) - config_dict['changed_files'] = [] - else: - # parse comma-separated list of files provided manually - config_dict['changed_files'] = [f.strip() for f in val.split(',') if f.strip()] - else: - # preserve any changed_files provided via env/config - if 'changed_files' not in config_dict: - config_dict['changed_files'] = [] + if changed_files_arg and str(changed_files_arg).strip(): + config_dict['changed_files'] = str(changed_files_arg).strip() # Post-processing: ensure repository/branch are set; if workspace isn't a git repo, # require explicit --repo and --branch to be provided. @@ -1639,6 +1657,111 @@ def create_config_from_args(args) -> Config: return Config(config_dict) +SCOPE_REQUEST_AUTO = 'auto' +SCOPE_REQUEST_PR = 'pr' +SCOPE_REQUEST_CURRENT_COMMIT = ('current-commit', 'current_commit') + + +def resolve_changed_files_request(request: str, workspace_path: str) -> List[str]: + """Resolve a raw ``changed_files`` request into a list of changed paths. + + ``request`` is whatever the user asked for: + + - ``auto`` -> the PR base diff in CI, else staged changes + - ``pr`` -> the PR base diff, and nothing else + - ``current-commit`` -> files in the HEAD commit + - a commit hash -> files in that commit + - anything else -> a comma-separated list of file paths + + Every outcome is logged. A scope request that cannot be honored used to + resolve to an empty list in silence, which made the run look like it had + simply found nothing. Callers treat an empty list as "skip the scanners", + so the difference between "the PR changed no scannable files" and "I could + not work out what the PR changed" has to be visible in the log. + """ + log = logging.getLogger(__name__) + value = (request or '').strip() + if not value: + return [] + + lowered = value.lower() + + if lowered == SCOPE_REQUEST_AUTO: + files = _detect_git_changed_files(workspace_path, mode='auto') + elif lowered == SCOPE_REQUEST_PR: + files = _detect_git_changed_files(workspace_path, mode='pr') + elif lowered in SCOPE_REQUEST_CURRENT_COMMIT: + files = _detect_git_changed_files(workspace_path, mode='current-commit') + elif re.match(r'^[0-9a-fA-F]{7,40}$', value): + files = _detect_git_changed_files(workspace_path, mode='commit', commit=value) + else: + files = [f.strip() for f in value.split(',') if f.strip()] + log.info( + "Diff-only scan scoping requested with an explicit list of %d file(s)", len(files) + ) + return files + + if files: + log.info( + "Diff-only scan scoping requested (changed_files=%s): resolved %d changed file(s)", + value, len(files), + ) + else: + log.warning( + "Diff-only scan scoping requested (changed_files=%s) but it resolved to zero files. " + "The scanners will be SKIPPED rather than scanning the whole repository. If this PR " + "really did change files, see the warnings above for why the diff could not be " + "computed.", + value, + ) + return files + + +def _read_github_event() -> Dict[str, Any]: + """Load the GitHub Actions event payload, or {} when unavailable.""" + event_path = os.environ.get('GITHUB_EVENT_PATH', '') + if not event_path: + return {} + try: + with open(event_path, 'r', encoding='utf-8') as fh: + payload = json.load(fh) + return payload if isinstance(payload, dict) else {} + except Exception: + logging.getLogger(__name__).debug( + "Could not read GitHub event payload at %s", event_path, exc_info=True + ) + return {} + + +def _pr_base_candidates(base_ref: str | None = None) -> List[str]: + """Every ref we know of that could be the base of the current PR. + + ``GITHUB_BASE_REF`` is only set for ``pull_request`` and + ``pull_request_target`` events. A workflow triggered by ``issue_comment``, + ``workflow_dispatch``, or a re-run has a pull request but no + ``GITHUB_BASE_REF``, so the event payload is checked as well. The payload + also carries ``base.sha``, which is an exact commit and therefore does not + depend on a remote-tracking branch existing in the checkout. + """ + candidates: List[str] = [] + + def _add(value) -> None: + if isinstance(value, str) and value.strip() and value.strip() not in candidates: + candidates.append(value.strip()) + + _add(base_ref) + _add(os.environ.get('GITHUB_BASE_REF', '')) + + pull_request = _read_github_event().get('pull_request') + if isinstance(pull_request, dict): + base = pull_request.get('base') + if isinstance(base, dict): + _add(base.get('sha')) + _add(base.get('ref')) + + return candidates + + def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: """Detect changed files in a git repository. @@ -1647,15 +1770,18 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: - 'current-commit' -> files included in the HEAD commit - 'commit' -> files included in the given commit hash (commit param required) - 'pr' -> files changed relative to a base ref (a GitHub PR). - Uses ``base_ref`` or ``GITHUB_BASE_REF`` and excludes - deletions so removed paths never become scan targets. - - 'auto' -> the PR base-ref diff when running in a PR CI context - (``GITHUB_BASE_REF`` is set), otherwise staged changes. - This is what ``--changed-files auto`` resolves to. + The base comes from ``base_ref``, ``GITHUB_BASE_REF``, + or the GitHub event payload, and deletions are + excluded so removed paths never become scan targets. + - 'auto' -> the PR base diff when a PR base can be found, + otherwise staged changes. This is what + ``--changed-files auto`` resolves to. Returns a list of file paths relative to the workspace root. If not a git - repo or detection fails, returns []. + repo or detection fails, returns [] -- and says so in the log, because + callers turn an empty list into "skip the scanners". """ + log = logging.getLogger(__name__) try: from subprocess import check_output, CalledProcessError import subprocess @@ -1668,11 +1794,20 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: ws = Path(workspace_path) if workspace_path else Path.cwd() if not ws.exists(): + log.warning( + "Cannot scope the scan to changed files: workspace %s does not exist", str(ws) + ) return [] # Ensure this is a git repo git_dir = ws / '.git' if not git_dir.exists(): + log.warning( + "Cannot scope the scan to changed files: %s is not a git repository. " + "Check out the repository (actions/checkout) before running the scan, or pass " + "an explicit file list to changed_files instead of '%s'.", + str(ws), mode, + ) return [] # Change to workspace directory before running git commands @@ -1684,40 +1819,112 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: def _split(out: str) -> List[str]: return [line.strip() for line in out.splitlines() if line.strip()] - def _diff_against_base(ref: str) -> Optional[List[str]]: - """Diff changed files (excluding deletions) against a base ref. + def _is_shallow() -> bool: + try: + out = check_output( + ['git', 'rev-parse', '--is-shallow-repository'], + text=True, stderr=subprocess.DEVNULL, + ) + return out.strip() == 'true' + except Exception: + return False + + def _git_is_usable() -> bool: + """Is git willing to talk to this checkout at all? + + A container action runs as root over a workspace owned by the + runner user, which git refuses with "detected dubious + ownership" -- an error that otherwise looks exactly like an + empty diff. + """ + try: + check_output(['git', 'rev-parse', '--git-dir'], text=True, stderr=subprocess.DEVNULL) + return True + except Exception: + return False + + def _diff_against_base(refs: List[str]) -> Optional[List[str]]: + """Diff changed files (excluding deletions) against a PR base. - Tries the remote-tracking ref (``origin/``) first, then the - bare ref. Returns None when neither ref can be resolved so the - caller can fall back to another detection strategy. The + Each candidate is tried as the remote-tracking ref + (``origin/``) and then bare. Returns None when no + candidate resolves, so the caller can fall back or warn. ``--diff-filter=ACMR`` excludes deleted paths so they never become scan targets. """ - if not ref: - return None - for candidate in (f'origin/{ref}', ref): - try: - out = check_output( - ['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], - text=True, stderr=subprocess.DEVNULL, - ) - return _split(out) - except CalledProcessError: - continue + tried: List[str] = [] + for ref in refs: + for candidate in (f'origin/{ref}', ref): + tried.append(candidate) + try: + out = check_output( + ['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], + text=True, stderr=subprocess.DEVNULL, + ) + log.info("Resolved PR diff base to '%s'", candidate) + return _split(out) + except CalledProcessError: + continue + if tried: + log.debug("No PR diff base resolved; tried: %s", ', '.join(tried)) return None + def _warn_no_base(refs: List[str]) -> None: + """Explain a failed base resolution instead of returning [] quietly.""" + if not _git_is_usable(): + log.warning( + "Cannot scope the scan to changed files: git refused to read %s. " + "This is usually a repository-ownership mismatch inside a container " + "('detected dubious ownership'). Run `git config --global --add " + "safe.directory %s` before the scan step.", + str(ws), str(ws), + ) + return + if not refs: + log.warning( + "Cannot scope the scan to changed files: no pull request base was found. " + "GITHUB_BASE_REF is unset and the GitHub event payload has no " + "pull_request.base, which happens on non-pull_request triggers. Use " + "changed_files: 'current-commit', or pass an explicit file list." + ) + return + shallow_hint = ( + " The checkout is shallow, so the base branch is not in it -- set " + "fetch-depth: 0 on actions/checkout." + if _is_shallow() else + " The base branch is not present in this checkout -- set fetch-depth: 0 on " + "actions/checkout so it is fetched." + ) + log.warning( + "Cannot scope the scan to changed files: none of the candidate PR bases (%s) " + "could be resolved in %s.%s", + ', '.join(refs), str(ws), shallow_hint, + ) + if mode == 'auto': - # Prefer the PR base-ref diff in CI; fall back to staged changes + # Prefer the PR base diff in CI; fall back to staged changes # for local/pre-commit use. - base = base_ref or os.environ.get('GITHUB_BASE_REF', '') - pr_files = _diff_against_base(base) + refs = _pr_base_candidates(base_ref) + pr_files = _diff_against_base(refs) if pr_files is not None: return pr_files - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) - return _split(out) + _warn_no_base(refs) + try: + out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) + except CalledProcessError: + return [] + staged = _split(out) + log.info( + "changed_files='auto' fell back to staged changes: %d file(s)", len(staged) + ) + return staged elif mode == 'pr': - base = base_ref or os.environ.get('GITHUB_BASE_REF', '') - return _diff_against_base(base) or [] + refs = _pr_base_candidates(base_ref) + pr_files = _diff_against_base(refs) + if pr_files is not None: + return pr_files + _warn_no_base(refs) + return [] elif mode == 'staged': # staged but not yet committed out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) @@ -1730,14 +1937,17 @@ def _diff_against_base(ref: str) -> Optional[List[str]]: out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL) return _split(out) else: + log.warning("Unknown changed-files detection mode '%s'; scoping nothing", mode) return [] finally: # Always restore original working directory os.chdir(original_cwd) - except CalledProcessError: + except CalledProcessError as e: + log.warning("Cannot scope the scan to changed files: git command failed (%s)", e) return [] - except Exception: + except Exception as e: + log.warning("Cannot scope the scan to changed files: %s: %s", type(e).__name__, e) return [] diff --git a/socket_basics/core/connector/base.py b/socket_basics/core/connector/base.py index 9574544..34e8769 100644 --- a/socket_basics/core/connector/base.py +++ b/socket_basics/core/connector/base.py @@ -200,12 +200,26 @@ def is_enabled(self) -> bool: def get_scan_targets(self) -> List[str]: """Get list of targets to scan - + Returns: List of file paths or directories to scan """ return self.config.get_scan_targets() if hasattr(self.config, 'get_scan_targets') else [] - + + def _changed_files_scope_requested(self) -> bool: + """Did the user explicitly ask for a changed-files scope? + + Connectors that derive their own changed-file list must not substitute + a different scope when an explicit request resolved to nothing -- that + turns "scan only what the PR touched" into "scan whatever happens to be + staged", which is a different set of files and is never what was asked + for. + """ + try: + return bool(self.config.get('changed_files_scope_requested', False)) + except Exception: + return False + def get_name(self) -> str: """Get the connector name diff --git a/socket_basics/core/connector/trivy/trivy.py b/socket_basics/core/connector/trivy/trivy.py index c4f518b..54dcae2 100644 --- a/socket_basics/core/connector/trivy/trivy.py +++ b/socket_basics/core/connector/trivy/trivy.py @@ -141,7 +141,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: # Try to detect changed Dockerfiles even if none explicitly configured changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else [] - if not changed_files: + if not changed_files and not self._changed_files_scope_requested(): try: from socket_basics.core.config import _detect_git_changed_files changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') @@ -170,7 +170,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: # If changed_files is provided, prefer scanning only changed Dockerfiles changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else [] # Fallback: attempt to detect staged changed files if none present - if not changed_files: + if not changed_files and not self._changed_files_scope_requested(): try: # import helper from config module from socket_basics.core.config import _detect_git_changed_files @@ -322,7 +322,7 @@ def scan_vulnerabilities(self) -> Dict[str, Any]: # Check for changed files to restrict scanning changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else [] - if not changed_files: + if not changed_files and not self._changed_files_scope_requested(): try: from socket_basics.core.config import _detect_git_changed_files changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') diff --git a/socket_basics/core/connector/trufflehog/__init__.py b/socket_basics/core/connector/trufflehog/__init__.py index 43eef14..69586b7 100644 --- a/socket_basics/core/connector/trufflehog/__init__.py +++ b/socket_basics/core/connector/trufflehog/__init__.py @@ -223,9 +223,12 @@ def scan(self) -> Dict[str, Any]: exclude_file_path = None exclude_patterns: List[str] = [] try: - # Prefer explicit changed_files, fallback to git staged + # Prefer explicit changed_files, fallback to git staged. When the + # user asked for a scope and it resolved to nothing (a delete-only + # PR, or a diff base that could not be worked out), do NOT quietly + # substitute a different scope -- honor the empty result and skip. changed_files = self.config.get('changed_files', []) if hasattr(self.config, '_config') else [] - if not changed_files: + if not changed_files and not self._changed_files_scope_requested(): try: from socket_basics.core.config import _detect_git_changed_files changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 3eda06d..3fc28cd 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -5,13 +5,20 @@ re-scanning the whole repository. """ +import logging import os import subprocess from argparse import Namespace +from types import SimpleNamespace import pytest -from socket_basics.core.config import Config, _detect_git_changed_files, create_config_from_args +from socket_basics.core.config import ( + Config, + _detect_git_changed_files, + create_config_from_args, + resolve_changed_files_request, +) def _make_config(workspace, **overrides): @@ -170,3 +177,367 @@ def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkey assert cfg.get("changed_files") == [] assert cfg.get_scan_targets() == [] + + +# =========================================================================== +# SURF-1452: the scope request must reach the file walk from every entry +# point, and a scope it cannot honor must say so in the log. +# =========================================================================== + + +@pytest.fixture +def cloned_pr_repo(tmp_path, monkeypatch): + """An 'upstream' repo plus a CI-style checkout with an ``origin`` remote. + + ``pr_repo`` has no remote, so ``origin/main`` never exists there and the + bare ``main`` fallback always saves it. Real CI checkouts only have the + remote-tracking ref, and only when the fetch was deep enough. + """ + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) + + upstream = tmp_path / "upstream" + upstream.mkdir() + _git(upstream, "init", "-b", "main") + (upstream / "base.py").write_text("base = 1") + (upstream / "untouched.py").write_text("untouched = 1") + _git(upstream, "add", ".") + _git(upstream, "commit", "-m", "base") + _git(upstream, "checkout", "-b", "feature") + (upstream / "feat.py").write_text("feat = 1") + _git(upstream, "add", "-A") + _git(upstream, "commit", "-m", "feature") + _git(upstream, "checkout", "main") + + def checkout(name, depth): + ws = tmp_path / name + ws.mkdir() + _git(ws, "init") + _git(ws, "remote", "add", "origin", str(upstream)) + if depth: + _git(ws, "fetch", "--no-tags", f"--depth={depth}", "origin", + "+refs/heads/feature:refs/remotes/origin/feature") + else: + _git(ws, "fetch", "--no-tags", "origin", "+refs/heads/*:refs/remotes/origin/*") + _git(ws, "checkout", "--force", "origin/feature") + return ws + + return { + "upstream": upstream, + "deep": checkout("deep", 0), # actions/checkout with fetch-depth: 0 + "shallow": checkout("shallow", 1), # actions/checkout default + } + + +def _write_event(tmp_path, monkeypatch, payload): + """Point GITHUB_EVENT_PATH at a pull_request event payload.""" + import json + event = tmp_path / "event.json" + event.write_text(json.dumps(payload)) + monkeypatch.setenv("GITHUB_EVENT_PATH", str(event)) + return event + + +class TestScopeRequestReachesEveryConfigPath: + """`changed_files` used to be resolved only in create_config_from_args(). + + A Config built any other way -- the env loader, a --config JSON file, a + Socket dashboard config -- either ignored the request and scanned the whole + repository, or kept the raw string and iterated it character by character. + """ + + def test_env_only_config_honors_input_changed_files(self, pr_repo, monkeypatch): + from socket_basics.core.config import load_config_from_env + + monkeypatch.setenv("GITHUB_WORKSPACE", str(pr_repo)) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("INPUT_CHANGED_FILES", "auto") + + cfg = Config(load_config_from_env()) + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert cfg.get_scan_targets() == [str(pr_repo / "base.py"), str(pr_repo / "feat.py")] + + def test_env_only_config_without_request_scans_workspace(self, pr_repo, monkeypatch): + from socket_basics.core.config import load_config_from_env + + monkeypatch.setenv("GITHUB_WORKSPACE", str(pr_repo)) + monkeypatch.delenv("INPUT_CHANGED_FILES", raising=False) + + cfg = Config(load_config_from_env()) + + assert cfg.get_scan_targets() == [str(pr_repo)] + + def test_raw_auto_string_is_resolved_not_iterated(self, pr_repo, monkeypatch): + """A JSON/dashboard config value of 'auto' must hit git. + + Before, `_resolve_file_targets("auto")` walked the string and looked + for files named 'a', 'u', 't', 'o'. + """ + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + cfg = _make_config(pr_repo, changed_files="auto") + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert cfg.get_scan_targets() == [str(pr_repo / "base.py"), str(pr_repo / "feat.py")] + + def test_raw_comma_list_string_is_split(self, tmp_path): + (tmp_path / "a.py").write_text("x = 1") + (tmp_path / "b.py").write_text("y = 2") + (tmp_path / "c.py").write_text("z = 3") + + cfg = _make_config(tmp_path, changed_files="a.py,b.py") + + assert cfg.get("changed_files") == ["a.py", "b.py"] + assert cfg.get_scan_targets() == [str(tmp_path / "a.py"), str(tmp_path / "b.py")] + + def test_already_resolved_list_is_not_re_resolved(self, tmp_path): + (tmp_path / "a.py").write_text("x = 1") + cfg = _make_config(tmp_path, changed_files=["a.py"]) + assert cfg.get("changed_files") == ["a.py"] + + def test_empty_request_normalizes_to_empty_list(self, tmp_path): + cfg = _make_config(tmp_path, changed_files="") + assert cfg.get("changed_files") == [] + assert cfg.get_scan_targets() == [str(tmp_path)] + + def test_cli_value_overrides_env_value(self, pr_repo, monkeypatch): + monkeypatch.setenv("GITHUB_WORKSPACE", str(pr_repo)) + monkeypatch.setenv("INPUT_CHANGED_FILES", "auto") + + cfg = create_config_from_args(_config_args(pr_repo, "base.py")) + + assert cfg.get("changed_files") == ["base.py"] + + def test_env_value_is_used_when_no_cli_value(self, pr_repo, monkeypatch): + monkeypatch.setenv("GITHUB_WORKSPACE", str(pr_repo)) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("INPUT_CHANGED_FILES", "pr") + + cfg = create_config_from_args(_config_args(pr_repo, "")) + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + + +class TestPrBaseResolution: + """`auto`/`pr` used to consult GITHUB_BASE_REF and nothing else.""" + + def test_uses_base_sha_from_event_payload(self, pr_repo, tmp_path, monkeypatch): + """GITHUB_BASE_REF is only set on pull_request triggers. + + On an issue_comment or workflow_dispatch run the PR base is only in the + event payload, where base.sha is an exact commit. + """ + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + base_sha = _git(pr_repo, "rev-parse", "main").stdout.strip() + _write_event(tmp_path, monkeypatch, {"pull_request": {"base": {"sha": base_sha}}}) + + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert sorted(files) == ["base.py", "feat.py"] + + def test_uses_base_ref_from_event_payload(self, pr_repo, tmp_path, monkeypatch): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + _write_event(tmp_path, monkeypatch, {"pull_request": {"base": {"ref": "main"}}}) + + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert sorted(files) == ["base.py", "feat.py"] + + def test_ignores_event_payload_without_a_pull_request(self, pr_repo, tmp_path, monkeypatch): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + _write_event(tmp_path, monkeypatch, {"ref": "refs/heads/feature"}) + + assert _detect_git_changed_files(str(pr_repo), mode="pr") == [] + + def test_survives_an_unreadable_event_payload(self, pr_repo, tmp_path, monkeypatch): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(tmp_path / "does-not-exist.json")) + + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert sorted(files) == ["base.py", "feat.py"] + + def test_deep_checkout_resolves_remote_tracking_base(self, cloned_pr_repo, monkeypatch): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + files = _detect_git_changed_files(str(cloned_pr_repo["deep"]), mode="pr") + assert files == ["feat.py"] + + +class TestScopeFailuresAreLoud: + """A scope that cannot be honored must not resolve to nothing in silence. + + Every one of these used to return [] with no log output at all, which is + why "we tried auto, we tried pr, nothing changed" was such a hard report to + act on. + """ + + def test_shallow_checkout_warns_and_names_fetch_depth(self, cloned_pr_repo, monkeypatch, caplog): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(cloned_pr_repo["shallow"]), mode="pr") + + assert files == [] + assert "fetch-depth: 0" in caplog.text + assert "shallow" in caplog.text.lower() + + def test_missing_pr_base_warns(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert files == [] + assert "no pull request base was found" in caplog.text + + def test_non_git_workspace_warns(self, tmp_path, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(tmp_path), mode="pr", base_ref="main") + + assert files == [] + assert "not a git repository" in caplog.text + + def test_zero_resolution_warns_that_scanners_will_be_skipped(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = resolve_changed_files_request("pr", str(pr_repo)) + + assert files == [] + assert "SKIPPED" in caplog.text + + def test_successful_resolution_does_not_warn(self, pr_repo, monkeypatch, caplog): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = resolve_changed_files_request("auto", str(pr_repo)) + + assert sorted(files) == ["base.py", "feat.py"] + assert caplog.text == "" + + +class TestScanAllOverrideIsLoud: + """`scan_all` outranks `changed_files`, and used to do it silently. + + `scan_all` can come from a Socket dashboard config or a shared workflow + template, so the person who asked for diff-only scoping is often not the + person who set it. Discarding their request without a word is what makes + the action look like it ignores `changed_files` entirely. + """ + + def test_scan_all_still_wins(self, tmp_path): + (tmp_path / "a.py").write_text("x = 1") + cfg = _make_config(tmp_path, scan_all=True, changed_files=["a.py"]) + assert cfg.get_scan_targets() == [str(tmp_path)] + + def test_scan_all_warns_when_it_discards_a_scope_request(self, tmp_path, caplog): + (tmp_path / "a.py").write_text("x = 1") + cfg = _make_config(tmp_path, scan_all=True, changed_files=["a.py"]) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + cfg.get_scan_targets() + + assert "scan_all is enabled" in caplog.text + assert "changed-files scope" in caplog.text + + def test_scan_all_warns_for_a_scope_that_resolved_to_nothing(self, tmp_path, caplog): + """The request was made even though it produced no files.""" + cfg = _make_config( + tmp_path, scan_all=True, changed_files=[], changed_files_scope_requested=True + ) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + cfg.get_scan_targets() + + assert "scan_all is enabled" in caplog.text + + def test_scan_all_alone_does_not_warn(self, tmp_path, caplog): + cfg = _make_config(tmp_path, scan_all=True) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + assert cfg.get_scan_targets() == [str(tmp_path)] + + assert "scan_all is enabled" not in caplog.text + + +class TestResolveChangedFilesRequest: + """The single resolver every config path now goes through.""" + + def test_commit_hash_request(self, pr_repo): + head = _git(pr_repo, "rev-parse", "HEAD").stdout.strip() + files = resolve_changed_files_request(head, str(pr_repo)) + # commit/current-commit list every path in the commit, deletions + # included; the deleted path is dropped when targets are resolved. + assert sorted(files) == ["base.py", "feat.py", "old.py"] + + def test_current_commit_request(self, pr_repo): + files = resolve_changed_files_request("current-commit", str(pr_repo)) + assert sorted(files) == ["base.py", "feat.py", "old.py"] + + def test_current_commit_drops_deleted_paths_from_targets(self, pr_repo): + cfg = _make_config(pr_repo, changed_files="current-commit") + targets = [os.path.basename(t) for t in cfg.get_scan_targets()] + assert sorted(targets) == ["base.py", "feat.py"] + + def test_explicit_list_request_does_not_touch_git(self, tmp_path): + assert resolve_changed_files_request("a.py, b.py ", str(tmp_path)) == ["a.py", "b.py"] + + def test_blank_request_returns_empty(self, tmp_path): + assert resolve_changed_files_request(" ", str(tmp_path)) == [] + + +class TestConnectorsHonorTheResolvedScope: + """Connectors derive their own changed-file list when the config has none. + + That fallback must not fire when the user explicitly asked for a scope and + it resolved to nothing. Substituting "whatever is staged" for "what the PR + changed" scans a different set of files than the one that was requested. + """ + + def _scanner(self, tmp_path, changed_files, scope_requested): + from socket_basics.core.connector.trufflehog import TruffleHogScanner + + values = { + "changed_files": changed_files, + "changed_files_scope_requested": scope_requested, + "trufflehog_exclude_dir": "", + "trufflehog_show_unverified": False, + } + config = SimpleNamespace(workspace=tmp_path, _config=values) + config.get = lambda key, default=None: values.get(key, default) + config.get_scan_targets = lambda: [] + scanner = TruffleHogScanner.__new__(TruffleHogScanner) + scanner.config = config + scanner.is_enabled = lambda: True + return scanner + + def test_scope_resolved_to_nothing_is_not_replaced_by_staged(self, pr_repo, monkeypatch): + (pr_repo / "staged_secret.py").write_text("token = 'abc'") + _git(pr_repo, "add", "staged_secret.py") + + called = [] + monkeypatch.setattr( + "socket_basics.core.config._detect_git_changed_files", + lambda *a, **k: called.append(k.get("mode")) or ["staged_secret.py"], + ) + + scanner = self._scanner(pr_repo, changed_files=[], scope_requested=True) + assert scanner.scan() == {} + assert called == [] + + def test_staged_fallback_still_runs_when_no_scope_was_requested(self, pr_repo, monkeypatch): + called = [] + monkeypatch.setattr( + "socket_basics.core.config._detect_git_changed_files", + lambda *a, **k: called.append(k.get("mode")) or [], + ) + + scanner = self._scanner(pr_repo, changed_files=[], scope_requested=False) + scanner.scan() + assert called == ["staged"] From 6380369ff51bb614852bcda70f7d93c09056957b Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 15:07:35 -0400 Subject: [PATCH 2/9] test: clear GITHUB_EVENT_PATH in the pr_repo fixture PR base resolution now consults the GitHub event payload, so when these tests run inside a pull request the ambient payload names a real base ref and the fixture's temp repo happens to have a branch by that name. test_auto_falls_back_to_staged_without_base_ref then diffed against it instead of falling back to staged changes. Passed locally, failed in CI, which is exactly the leak. --- tests/test_changed_files_scope.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 3fc28cd..6b752df 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -114,9 +114,13 @@ def _config_args(workspace, changed_files): def pr_repo(tmp_path, monkeypatch): """A git repo with a 'main' base and a 'feature' branch ahead of it.""" # _detect_git_changed_files prefers GITHUB_WORKSPACE; clear it so the - # explicit workspace path is used. + # explicit workspace path is used. GITHUB_EVENT_PATH has to go too: when + # these tests run inside a pull request, the ambient event payload names a + # real base ref, and PR base resolution would find it and diff against it + # instead of doing what the test asked for. monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) _git(tmp_path, "init", "-b", "main") (tmp_path / "base.py").write_text("base = 1") @@ -161,6 +165,7 @@ def test_non_git_dir_returns_empty(self, tmp_path, monkeypatch): def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkeypatch): monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("GITHUB_EVENT_PATH", raising=False) monkeypatch.setenv("GITHUB_BASE_REF", "main") _git(tmp_path, "init", "-b", "main") From 5a7fe281ddc5b13999804cf2dde838184245b9e8 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 18:01:49 -0400 Subject: [PATCH 3/9] fix(trivy): stop an unresolvable changed-files scope from widening to the whole workspace Trivy's filesystem vulnerability scan is the one scanner that builds its own path list instead of going through Config.get_scan_targets(). Declining the staged-file substitution was not enough on its own: when the requested scope resolved to no scannable paths, scan_paths stayed empty and the existing fallback assigned the whole workspace, so the scan expanded to the full repository instead of skipping. Caught by Cursor Bugbot on #98. --- CHANGELOG.md | 5 +- socket_basics/core/connector/trivy/trivy.py | 12 ++- tests/test_changed_files_scope.py | 91 +++++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a55d9c..5e4588c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). warns that the scanners are being skipped. All of these previously returned an empty list in complete silence. - TruffleHog and Trivy no longer substitute their own staged-file scope when an - explicit `changed_files` request resolved to nothing. + explicit `changed_files` request resolved to nothing. Trivy's filesystem + vulnerability scan also stops widening an empty scope back out to the whole + workspace; it is the one scanner that builds its own path list instead of + going through `get_scan_targets()`, so it needed the check twice. ## [2.2.1] - 2026-07-30 diff --git a/socket_basics/core/connector/trivy/trivy.py b/socket_basics/core/connector/trivy/trivy.py index 54dcae2..601efe9 100644 --- a/socket_basics/core/connector/trivy/trivy.py +++ b/socket_basics/core/connector/trivy/trivy.py @@ -351,8 +351,18 @@ def scan_vulnerabilities(self) -> Dict[str, Any]: if scan_paths: logger.info(f"Restricting Trivy scan to {len(scan_paths)} changed directory(ies)") - # If no changed files or no valid paths, scan entire workspace + # If no changed files or no valid paths, scan entire workspace -- unless + # the user asked for a changed-files scope. Widening an empty scope back + # out to the whole repository is the exact behaviour this scoping exists + # to prevent, and unlike the other scanners this one never goes through + # get_scan_targets(), so it has to make the decision itself. if not scan_paths: + if self._changed_files_scope_requested(): + logger.info( + "Trivy vulnerability scan skipped: a changed-files scope was requested and " + "resolved to no scannable paths, so the whole workspace is not scanned" + ) + return results scan_paths = [workspace_path] for scan_path in scan_paths: diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 6b752df..509e0c4 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -5,6 +5,7 @@ re-scanning the whole repository. """ +import json import logging import os import subprocess @@ -546,3 +547,93 @@ def test_staged_fallback_still_runs_when_no_scope_was_requested(self, pr_repo, m scanner = self._scanner(pr_repo, changed_files=[], scope_requested=False) scanner.scan() assert called == ["staged"] + + +class TestTrivyVulnScanHonorsTheResolvedScope: + """Trivy's filesystem vulnerability scan never calls get_scan_targets(). + + Every other connector inherits the empty-scope behaviour from + ``Config.get_scan_targets()``. This one builds its own path list from + ``changed_files`` and falls back to the whole workspace when that list is + empty, so declining the staged-file substitution is not enough on its own: + the workspace fallback has to be declined too, or an unresolvable scope + still turns into a full-repository scan. + """ + + def _scanner(self, tmp_path, changed_files, scope_requested): + from socket_basics.core.connector.trivy.trivy import TrivyScanner + + values = { + "trivy_vuln_enabled": True, + "changed_files": changed_files, + "changed_files_scope_requested": scope_requested, + } + config = SimpleNamespace(workspace=tmp_path, _config=values) + config.get = lambda key, default=None: values.get(key, default) + scanner = TrivyScanner.__new__(TrivyScanner) + scanner.config = config + return scanner + + def _record_trivy_paths(self, monkeypatch): + """Capture the path argument of every trivy invocation.""" + scanned = [] + + def fake_run(cmd, *args, **kwargs): + scanned.append(cmd[-1]) + # Write the JSON Trivy would have written to --output. + out_index = cmd.index('--output') + 1 + with open(cmd[out_index], 'w') as fh: + json.dump({"Results": []}, fh) + return SimpleNamespace(returncode=0, stdout="", stderr="") + + monkeypatch.setattr( + "socket_basics.core.connector.trivy.trivy.subprocess.run", fake_run + ) + return scanned + + def test_unresolvable_scope_does_not_widen_to_the_whole_workspace( + self, tmp_path, monkeypatch + ): + scanned = self._record_trivy_paths(monkeypatch) + scanner = self._scanner(tmp_path, changed_files=[], scope_requested=True) + + assert scanner.scan_vulnerabilities() == {} + assert scanned == [] + + def test_scope_whose_paths_all_vanished_does_not_widen_either( + self, tmp_path, monkeypatch + ): + # A delete-only PR: the scope resolved to files, but none of them exist + # in the checkout any more, so no directory survives resolution. + scanned = self._record_trivy_paths(monkeypatch) + scanner = self._scanner( + tmp_path, changed_files=["gone/removed.py"], scope_requested=True + ) + + assert scanner.scan_vulnerabilities() == {} + assert scanned == [] + + def test_resolved_scope_still_scans_only_the_changed_directories( + self, tmp_path, monkeypatch + ): + (tmp_path / "pkg").mkdir() + (tmp_path / "pkg" / "requirements.txt").write_text("requests==2.0.0\n") + scanned = self._record_trivy_paths(monkeypatch) + scanner = self._scanner( + tmp_path, changed_files=["pkg/requirements.txt"], scope_requested=True + ) + + scanner.scan_vulnerabilities() + assert scanned == [str(tmp_path / "pkg")] + + def test_whole_workspace_is_still_scanned_when_no_scope_was_requested( + self, tmp_path, monkeypatch + ): + scanned = self._record_trivy_paths(monkeypatch) + monkeypatch.setattr( + "socket_basics.core.config._detect_git_changed_files", lambda *a, **k: [] + ) + scanner = self._scanner(tmp_path, changed_files=[], scope_requested=False) + + scanner.scan_vulnerabilities() + assert scanned == [str(tmp_path)] From 8fa49080ab329aec0839af7a2cedd99c55cb7ea4 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 18:28:07 -0400 Subject: [PATCH 4/9] fix(config): name the issue_comment trigger instead of over-promising the event-payload base fallback The event-payload fallback reads a top-level pull_request.base, which covers pull_request, pull_request_target, pull_request_review and pull_request_review_comment. It does not cover issue_comment: that payload carries issue.pull_request, a set of URLs with no base ref or sha, so the base cannot be worked out without a GitHub API call. The docstring and docs claimed otherwise, and issue_comment is the trigger the change was motivated by. Correct the claim, and give that shape its own warning telling the workflow author to look the base up and pass GITHUB_BASE_REF, so an unsupported trigger reports itself instead of looking like an empty diff. Caught by Cursor Bugbot on #98. --- CHANGELOG.md | 6 +++- docs/parameters.md | 23 ++++++++++++ socket_basics/core/config.py | 44 ++++++++++++++++++++--- tests/test_changed_files_scope.py | 58 +++++++++++++++++++++++++++++-- 4 files changed, 123 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e4588c..38e85a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - Pull request base resolution now falls back to `pull_request.base.sha` and `pull_request.base.ref` from the GitHub event payload when `GITHUB_BASE_REF` is unset, which is the case on any trigger other than `pull_request` and - `pull_request_target`. + `pull_request_target`. That covers `pull_request_review` and + `pull_request_review_comment`, whose payloads carry a top-level + `pull_request`. It cannot cover `issue_comment`, whose payload only has + `issue.pull_request` (URLs, no base ref or sha), so that trigger now gets a + warning naming itself and saying how to supply the base instead. - A scope request that cannot be honored now says why. Shallow checkouts (naming `fetch-depth: 0`), a workspace that is not a git repository, git refusing to read the repository (container ownership mismatch), and a missing diff --git a/docs/parameters.md b/docs/parameters.md index 4f9c03e..9063ba4 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -139,6 +139,29 @@ not a git repository, git refused to read the repository, no PR base at all) and the scanners are skipped. **A scope request that cannot be honored is never turned into a whole-repository scan, and it is never silent.** +Steps 2 and 3 cover the triggers whose payload carries a top-level +`pull_request`: `pull_request`, `pull_request_target`, `pull_request_review` +and `pull_request_review_comment`. They do **not** cover `issue_comment`. That +payload has `issue.pull_request` instead, which is a set of URLs with no base +ref or sha in it, so there is nothing to diff against without a GitHub API +call. If you run the scan from a comment trigger, look the base up in the +workflow and pass it in yourself: + +```yaml +- id: prbase + run: echo "ref=$(gh pr view ${{ github.event.issue.number }} --json baseRefName -q .baseRefName)" >> "$GITHUB_OUTPUT" + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} +- uses: SocketDev/socket-basics@v2 + env: + GITHUB_BASE_REF: ${{ steps.prbase.outputs.ref }} + with: + changed_files: 'auto' +``` + +Otherwise the run warns that it was triggered by a comment on a pull request +and that it cannot work the base out on its own. + **`scan_all` outranks `changed_files`.** If `scan_all` is set — from `INPUT_SCAN_ALL`, a JSON config, or a Socket dashboard config — the whole workspace is scanned and the changed-files scope is discarded. The run logs a diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 9285b24..c997be4 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1737,11 +1737,18 @@ def _pr_base_candidates(base_ref: str | None = None) -> List[str]: """Every ref we know of that could be the base of the current PR. ``GITHUB_BASE_REF`` is only set for ``pull_request`` and - ``pull_request_target`` events. A workflow triggered by ``issue_comment``, - ``workflow_dispatch``, or a re-run has a pull request but no - ``GITHUB_BASE_REF``, so the event payload is checked as well. The payload - also carries ``base.sha``, which is an exact commit and therefore does not - depend on a remote-tracking branch existing in the checkout. + ``pull_request_target`` events, so the event payload is checked as well. + That covers the triggers whose payload carries a top-level ``pull_request`` + object -- ``pull_request_review``, ``pull_request_review_comment`` and the + two above -- and it is worth checking even when ``GITHUB_BASE_REF`` is set + because ``base.sha`` is an exact commit and so does not depend on a + remote-tracking branch existing in the checkout. + + It does **not** cover ``issue_comment``. That payload has no top-level + ``pull_request``; it has ``issue.pull_request``, which is a bag of URLs + with no ref or sha in it, and working the base out from there would take a + GitHub API call. ``_warn_no_base`` recognises that shape and says so rather + than letting the run look like an empty diff. """ candidates: List[str] = [] @@ -1762,6 +1769,21 @@ def _add(value) -> None: return candidates +def _event_is_comment_on_pull_request() -> bool: + """Was this run triggered by a comment on a pull request? + + ``issue_comment`` fires for both issues and pull requests, and the only + thing distinguishing the two is the presence of ``issue.pull_request``. + There is a pull request, but nothing in the payload says what it is based + on, so the scope cannot be resolved from the environment alone. + """ + event = _read_github_event() + if isinstance(event.get('pull_request'), dict): + return False + issue = event.get('issue') + return isinstance(issue, dict) and isinstance(issue.get('pull_request'), dict) + + def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: """Detect changed files in a git repository. @@ -1881,6 +1903,18 @@ def _warn_no_base(refs: List[str]) -> None: ) return if not refs: + if _event_is_comment_on_pull_request(): + log.warning( + "Cannot scope the scan to changed files: no pull request base was " + "found. This run was triggered by a comment on a pull request " + "(issue_comment), which sets no GITHUB_BASE_REF and whose event " + "payload only carries github.event.issue.pull_request -- a set of " + "URLs with no base ref or sha in it. Look the base up in the " + "workflow (`gh pr view --json baseRefName`) and pass it to " + "the scan step as GITHUB_BASE_REF, or use " + "changed_files: 'current-commit'." + ) + return log.warning( "Cannot scope the scan to changed files: no pull request base was found. " "GITHUB_BASE_REF is unset and the GitHub event payload has no " diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 509e0c4..16b5003 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -332,8 +332,9 @@ class TestPrBaseResolution: def test_uses_base_sha_from_event_payload(self, pr_repo, tmp_path, monkeypatch): """GITHUB_BASE_REF is only set on pull_request triggers. - On an issue_comment or workflow_dispatch run the PR base is only in the - event payload, where base.sha is an exact commit. + On a pull_request_review or pull_request_review_comment run the PR base + is only in the event payload, where base.sha is an exact commit that + does not need a remote-tracking branch to resolve. """ monkeypatch.delenv("GITHUB_BASE_REF", raising=False) base_sha = _git(pr_repo, "rev-parse", "main").stdout.strip() @@ -370,6 +371,59 @@ def test_deep_checkout_resolves_remote_tracking_base(self, cloned_pr_repo, monke files = _detect_git_changed_files(str(cloned_pr_repo["deep"]), mode="pr") assert files == ["feat.py"] + def test_issue_comment_payload_yields_no_base_and_says_why( + self, pr_repo, tmp_path, monkeypatch, caplog + ): + """issue_comment is the one PR trigger the payload fallback cannot serve. + + Its payload has no top-level pull_request. It has issue.pull_request, + which is a set of URLs with no base ref or sha in it, so there is + nothing to diff against without a GitHub API call. The run must say + that rather than resolve to nothing and look like an empty diff. + """ + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + _write_event( + tmp_path, + monkeypatch, + { + "action": "created", + "issue": { + "number": 98, + "pull_request": { + "url": "https://api.github.com/repos/o/r/pulls/98", + "html_url": "https://github.com/o/r/pull/98", + }, + }, + }, + ) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert files == [] + assert "issue_comment" in caplog.text + assert "GITHUB_BASE_REF" in caplog.text + + def test_plain_issue_comment_gets_the_generic_warning( + self, pr_repo, tmp_path, monkeypatch, caplog + ): + """A comment on a real issue is not a pull request at all. + + issue_comment fires for both, and only issue.pull_request tells them + apart, so the PR-specific advice must not be given for an issue. + """ + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + _write_event( + tmp_path, monkeypatch, {"action": "created", "issue": {"number": 12}} + ) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert files == [] + assert "no pull request base was found" in caplog.text + assert "issue_comment" not in caplog.text + class TestScopeFailuresAreLoud: """A scope that cannot be honored must not resolve to nothing in silence. From ee741d7d8c3a0b439e0c7a170d0037af35f5d6c7 Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 3 Aug 2026 18:41:42 -0400 Subject: [PATCH 5/9] fix(config): say what the scan_all override actually does, and keep it meaning scan everything The scan_all warning claimed the changed-files scope was simply ignored. Only the scanners that ask Config.get_scan_targets() for their paths widen -- SAST does, while TruffleHog and Trivy read changed_files off the config themselves and stay scoped. Both settings together produce a mixed run, so the warning now says that instead of sending someone looking for a full-repo secret scan that never happens. Also stop the new Trivy empty-scope skip from firing under scan_all. scan_all is an explicit request to scan everything, and turning it into scanning nothing was a regression in the previous commit on this branch. Caught by Cursor Bugbot on #98. --- CHANGELOG.md | 4 ++ docs/parameters.md | 14 ++++--- socket_basics/core/config.py | 17 +++++++-- socket_basics/core/connector/trivy/trivy.py | 5 ++- tests/test_changed_files_scope.py | 41 +++++++++++++++++++-- 5 files changed, 67 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e85a4..dbcaf8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `scan_all` still overrides `changed_files`, but now logs a warning naming the scope it discarded. It can come from a Socket dashboard config rather than the workflow, so discarding the request silently made diff-only mode look broken. + The warning also says that the override is partial: only the scanners that ask + `get_scan_targets()` for their paths widen, while the secret and container + scanners read `changed_files` directly and stay scoped, so setting both + produces a mixed run. - Pull request base resolution now falls back to `pull_request.base.sha` and `pull_request.base.ref` from the GitHub event payload when `GITHUB_BASE_REF` is unset, which is the case on any trigger other than `pull_request` and diff --git a/docs/parameters.md b/docs/parameters.md index 9063ba4..72d8264 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -162,11 +162,15 @@ workflow and pass it in yourself: Otherwise the run warns that it was triggered by a comment on a pull request and that it cannot work the base out on its own. -**`scan_all` outranks `changed_files`.** If `scan_all` is set — from -`INPUT_SCAN_ALL`, a JSON config, or a Socket dashboard config — the whole -workspace is scanned and the changed-files scope is discarded. The run logs a -warning saying so, because `scan_all` often comes from a different place than -the workflow that asked for diff-only scoping. +**`scan_all` outranks `changed_files` — but only for some scanners.** If +`scan_all` is set — from `INPUT_SCAN_ALL`, a JSON config, or a Socket dashboard +config — SAST scans the whole workspace and the changed-files scope is +discarded. The secret and container scanners read `changed_files` off the config +themselves rather than asking for scan targets, so they stay scoped to the +changed files, and a run with both settings is a mix of the two. The run logs a +warning saying exactly that, because `scan_all` often comes from a different +place than the workflow that asked for diff-only scoping. Set one or the other, +not both. **Example:** ```bash diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index c997be4..f640d44 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -300,11 +300,20 @@ def get_scan_targets(self) -> List[str]: # workflow template, so the person who set changed_files is not # necessarily the person who set scan_all. Silently discarding # the narrower request is what makes diff-only mode look broken. + # + # It is only discarded for the scanners that ask this method for + # their targets. TruffleHog and Trivy read changed_files off the + # config themselves, so they keep the narrow scope, and the run + # ends up a mix of the two. Say that plainly -- a warning that + # overstates what it does is its own kind of misleading. logging.getLogger(__name__).warning( - "scan_all is enabled, so the whole workspace will be scanned and the " - "requested changed-files scope (%d file(s)) is being ignored. " - "Unset scan_all (INPUT_SCAN_ALL, or the scan_all key in your Socket " - "dashboard/JSON config) to scan only changed files.", + "scan_all and a changed-files scope (%d file(s)) are both set, and they " + "disagree. SAST will scan the whole workspace because scan_all outranks " + "the scope here, while the secret and container scanners read changed_files " + "directly and will stay scoped to those files, so this run will be a mix of " + "the two. Unset scan_all (INPUT_SCAN_ALL, or the scan_all key in your Socket " + "dashboard/JSON config) to scan only changed files, or drop changed_files to " + "scan everything.", len(self.get('changed_files') or []), ) return [str(self.workspace)] diff --git a/socket_basics/core/connector/trivy/trivy.py b/socket_basics/core/connector/trivy/trivy.py index 601efe9..c9c3789 100644 --- a/socket_basics/core/connector/trivy/trivy.py +++ b/socket_basics/core/connector/trivy/trivy.py @@ -356,8 +356,11 @@ def scan_vulnerabilities(self) -> Dict[str, Any]: # out to the whole repository is the exact behaviour this scoping exists # to prevent, and unlike the other scanners this one never goes through # get_scan_targets(), so it has to make the decision itself. + # scan_all is the explicit "scan everything" override, so it still gets + # the whole workspace -- skipping there would turn an explicit request + # to scan everything into scanning nothing. if not scan_paths: - if self._changed_files_scope_requested(): + if self._changed_files_scope_requested() and not self.config.get('scan_all', False): logger.info( "Trivy vulnerability scan skipped: a changed-files scope was requested and " "resolved to no scannable paths, so the whole workspace is not scanned" diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 16b5003..7c8d747 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -503,7 +503,7 @@ def test_scan_all_warns_when_it_discards_a_scope_request(self, tmp_path, caplog) with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): cfg.get_scan_targets() - assert "scan_all is enabled" in caplog.text + assert "scan_all" in caplog.text assert "changed-files scope" in caplog.text def test_scan_all_warns_for_a_scope_that_resolved_to_nothing(self, tmp_path, caplog): @@ -515,7 +515,7 @@ def test_scan_all_warns_for_a_scope_that_resolved_to_nothing(self, tmp_path, cap with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): cfg.get_scan_targets() - assert "scan_all is enabled" in caplog.text + assert "scan_all" in caplog.text def test_scan_all_alone_does_not_warn(self, tmp_path, caplog): cfg = _make_config(tmp_path, scan_all=True) @@ -523,7 +523,25 @@ def test_scan_all_alone_does_not_warn(self, tmp_path, caplog): with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): assert cfg.get_scan_targets() == [str(tmp_path)] - assert "scan_all is enabled" not in caplog.text + assert "scan_all" not in caplog.text + + def test_warning_says_the_run_will_be_a_mix_not_a_clean_override(self, tmp_path, caplog): + """Only get_scan_targets() callers widen; the others keep the scope. + + TruffleHog and Trivy read ``changed_files`` off the config themselves, + so ``scan_all`` does not actually reach them. Claiming the scope is + simply "ignored" would send someone looking for a full-repo secret scan + that never happens. + """ + (tmp_path / "a.py").write_text("x = 1") + cfg = _make_config(tmp_path, scan_all=True, changed_files=["a.py"]) + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + cfg.get_scan_targets() + + assert "mix" in caplog.text + assert "secret and container scanners" in caplog.text + assert "stay scoped" in caplog.text class TestResolveChangedFilesRequest: @@ -614,13 +632,14 @@ class TestTrivyVulnScanHonorsTheResolvedScope: still turns into a full-repository scan. """ - def _scanner(self, tmp_path, changed_files, scope_requested): + def _scanner(self, tmp_path, changed_files, scope_requested, scan_all=False): from socket_basics.core.connector.trivy.trivy import TrivyScanner values = { "trivy_vuln_enabled": True, "changed_files": changed_files, "changed_files_scope_requested": scope_requested, + "scan_all": scan_all, } config = SimpleNamespace(workspace=tmp_path, _config=values) config.get = lambda key, default=None: values.get(key, default) @@ -691,3 +710,17 @@ def test_whole_workspace_is_still_scanned_when_no_scope_was_requested( scanner.scan_vulnerabilities() assert scanned == [str(tmp_path)] + + def test_scan_all_still_gets_the_whole_workspace(self, tmp_path, monkeypatch): + """scan_all is an explicit "scan everything", so it outranks the skip. + + Without this, an unresolvable scope plus scan_all would turn a request + to scan everything into scanning nothing. + """ + scanned = self._record_trivy_paths(monkeypatch) + scanner = self._scanner( + tmp_path, changed_files=[], scope_requested=True, scan_all=True + ) + + scanner.scan_vulnerabilities() + assert scanned == [str(tmp_path)] From 946cac4ada88fc3e9e277e2101d547ecb8d0cfce Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 4 Aug 2026 18:29:12 -0400 Subject: [PATCH 6/9] fix(config): make changed-file detection work inside the container The action ships as a Docker container action, so the scan runs as root over a workspace owned by the runner user. git refuses that with "detected dubious ownership", which failed every diff and made changed_files: 'auto' and 'pr' resolve to nothing -- the same symptom as a pull request that changed no files. No workflow could fix it. Running `git config --global --add safe.directory` before the scan step writes the runner's git config, and the container has its own, so the advice the previous warning gave could not work. The git reads now declare the trust themselves, for the one directory the workflow asked us to scan and nothing else. Five tests drive real repositories through git's own GIT_TEST_ASSUME_DIFFERENT_OWNER switch, including one asserting the workspace is the only path trusted. --- CHANGELOG.md | 15 +++++-- docs/github-action.md | 11 ++++- socket_basics/core/config.py | 60 +++++++++++++++++++++------ tests/test_changed_files_scope.py | 67 ++++++++++++++++++++++++++++++- 4 files changed, 132 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dbcaf8a..7093599 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,12 +34,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `pull_request`. It cannot cover `issue_comment`, whose payload only has `issue.pull_request` (URLs, no base ref or sha), so that trigger now gets a warning naming itself and saying how to supply the base instead. +- Changed-file detection now works inside the action's container. The scan runs + as root over a workspace owned by the runner user, which git refuses with + `detected dubious ownership`, so every diff failed and `changed_files: 'auto'` + or `'pr'` resolved to nothing. Nothing in a workflow could fix it: setting + `safe.directory` in a workflow step writes the runner's git config, not the + container's. The git reads now trust the workspace they were pointed at, and + only that directory. - A scope request that cannot be honored now says why. Shallow checkouts (naming `fetch-depth: 0`), a workspace that is not a git repository, git - refusing to read the repository (container ownership mismatch), and a missing - PR base each log a specific warning, and a scope that resolves to zero files - warns that the scanners are being skipped. All of these previously returned an - empty list in complete silence. + refusing to read the repository, and a missing PR base each log a specific + warning, and a scope that resolves to zero files warns that the scanners are + being skipped. All of these previously returned an empty list in complete + silence. - TruffleHog and Trivy no longer substitute their own staged-file scope when an explicit `changed_files` request resolved to nothing. Trivy's filesystem vulnerability scan also stops widening an empty scope back out to the whole diff --git a/docs/github-action.md b/docs/github-action.md index a1dff0f..516974c 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -341,10 +341,17 @@ everything or nothing: | `none of the candidate PR bases (...) could be resolved ... The checkout is shallow` | Add `fetch-depth: 0` to `actions/checkout` | | `no pull request base was found` | The trigger is not `pull_request`, so there is no base. Use `changed_files: 'current-commit'` or an explicit file list | | `is not a git repository` | Run `actions/checkout` before the scan step | -| `git refused to read ... detected dubious ownership` | Run `git config --global --add safe.directory "$GITHUB_WORKSPACE"` before the scan step | -| `scan_all is enabled ... changed-files scope ... is being ignored` | Unset `scan_all` — it can come from a Socket dashboard config, not just your workflow | +| `git refused to read ... not the usual container ownership mismatch` | The checkout is damaged or incomplete. Re-run `actions/checkout`, or pass an explicit file list | +| `scan_all and a changed-files scope ... are both set, and they disagree` | Unset `scan_all` — it can come from a Socket dashboard config, not just your workflow | | `resolved to zero files. The scanners will be SKIPPED` | The diff found nothing scannable. Combined with a warning above, it tells you the diff failed rather than the PR being empty | +You do not need `git config --global --add safe.directory` for this. The scan +runs as root inside a container over a workspace owned by the runner user, and +git normally refuses that with `detected dubious ownership`. The scan trusts the +workspace it was pointed at, so the diff works without any workflow change — +and setting `safe.directory` in a workflow step would not have helped anyway, +because it writes the runner's git config rather than the container's. + ### Where the setting can come from `changed_files` is honored identically from the action input, the diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index f640d44..7a643ea 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1793,6 +1793,35 @@ def _event_is_comment_on_pull_request() -> bool: return isinstance(issue, dict) and isinstance(issue.get('pull_request'), dict) +def _git_command_for(workspace: Path) -> List[str]: + """The ``git`` command to use for reading ``workspace``. + + This action ships as a Docker container action, so the scan runs as root + inside the container over a workspace that belongs to the runner user. That + ownership mismatch is exactly what git refuses with "detected dubious + ownership", and it makes every diff fail no matter how the workflow is + written -- the same symptom as a PR that changed nothing. + + Telling the user to run ``git config --global --add safe.directory`` cannot + fix it either, because that writes the *runner's* git config and the + container has its own. So declare the trust here instead, for the one + directory the workflow already asked us to scan and for nothing else. When + ownership matches, this changes nothing. + """ + trusted = [str(workspace)] + try: + resolved = str(workspace.resolve()) + if resolved not in trusted: + trusted.append(resolved) + except OSError: + pass + + command = ['git'] + for path in trusted: + command += ['-c', f'safe.directory={path}'] + return command + + def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: """Detect changed files in a git repository. @@ -1847,13 +1876,15 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: try: os.chdir(str(ws)) + git = _git_command_for(ws) + def _split(out: str) -> List[str]: return [line.strip() for line in out.splitlines() if line.strip()] def _is_shallow() -> bool: try: out = check_output( - ['git', 'rev-parse', '--is-shallow-repository'], + [*git, 'rev-parse', '--is-shallow-repository'], text=True, stderr=subprocess.DEVNULL, ) return out.strip() == 'true' @@ -1866,10 +1897,12 @@ def _git_is_usable() -> bool: A container action runs as root over a workspace owned by the runner user, which git refuses with "detected dubious ownership" -- an error that otherwise looks exactly like an - empty diff. + empty diff. ``_git_command_for`` already trusts the workspace, + so reaching this check means something else is wrong with the + checkout. """ try: - check_output(['git', 'rev-parse', '--git-dir'], text=True, stderr=subprocess.DEVNULL) + check_output([*git, 'rev-parse', '--git-dir'], text=True, stderr=subprocess.DEVNULL) return True except Exception: return False @@ -1889,7 +1922,7 @@ def _diff_against_base(refs: List[str]) -> Optional[List[str]]: tried.append(candidate) try: out = check_output( - ['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], + [*git, 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD'], text=True, stderr=subprocess.DEVNULL, ) log.info("Resolved PR diff base to '%s'", candidate) @@ -1904,11 +1937,12 @@ def _warn_no_base(refs: List[str]) -> None: """Explain a failed base resolution instead of returning [] quietly.""" if not _git_is_usable(): log.warning( - "Cannot scope the scan to changed files: git refused to read %s. " - "This is usually a repository-ownership mismatch inside a container " - "('detected dubious ownership'). Run `git config --global --add " - "safe.directory %s` before the scan step.", - str(ws), str(ws), + "Cannot scope the scan to changed files: git refused to read %s even " + "though the scan already trusts that directory, so this is not the " + "usual container ownership mismatch. The checkout is probably damaged " + "or incomplete -- re-run actions/checkout, or pass an explicit file " + "list to changed_files.", + str(ws), ) return if not refs: @@ -1953,7 +1987,7 @@ def _warn_no_base(refs: List[str]) -> None: return pr_files _warn_no_base(refs) try: - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) + out = check_output([*git, 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) except CalledProcessError: return [] staged = _split(out) @@ -1970,14 +2004,14 @@ def _warn_no_base(refs: List[str]) -> None: return [] elif mode == 'staged': # staged but not yet committed - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) + out = check_output([*git, 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) return _split(out) elif mode == 'current-commit': # files that are part of HEAD commit - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD'], text=True, stderr=subprocess.DEVNULL) + out = check_output([*git, 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD'], text=True, stderr=subprocess.DEVNULL) return _split(out) elif mode == 'commit' and commit: - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL) + out = check_output([*git, 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL) return _split(out) else: log.warning("Unknown changed-files detection mode '%s'; scoping nothing", mode) diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 7c8d747..792e375 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -17,6 +17,7 @@ from socket_basics.core.config import ( Config, _detect_git_changed_files, + _git_command_for, create_config_from_args, resolve_changed_files_request, ) @@ -186,8 +187,8 @@ def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkey # =========================================================================== -# SURF-1452: the scope request must reach the file walk from every entry -# point, and a scope it cannot honor must say so in the log. +# The scope request must reach the file walk from every entry point, and a +# scope it cannot honor must say so in the log. # =========================================================================== @@ -482,6 +483,68 @@ def test_successful_resolution_does_not_warn(self, pr_repo, monkeypatch, caplog) assert caplog.text == "" +class TestContainerOwnershipMismatch: + """The scan runs as root in a container over a runner-owned workspace. + + git refuses that with "detected dubious ownership", which fails every diff + and looks exactly like a PR that changed nothing. Nothing in the workflow + can fix it -- `git config --global --add safe.directory` before the scan + step writes the runner's git config, not the container's -- so the git + calls have to trust the workspace themselves. + + `GIT_TEST_ASSUME_DIFFERENT_OWNER` is git's own switch for that state, so + these run against real repositories and the real git binary. + """ + + def test_pr_diff_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + + files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + + assert sorted(files) == ["base.py", "feat.py"] + + def test_auto_scope_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + + cfg = _make_config(pr_repo, changed_files="auto") + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert cfg.get_scan_targets() == [str(pr_repo / "base.py"), str(pr_repo / "feat.py")] + + def test_current_commit_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + + files = _detect_git_changed_files(str(pr_repo), mode="current-commit") + + assert "feat.py" in files + + def test_ownership_mismatch_no_longer_warns_about_safe_directory( + self, pr_repo, monkeypatch, caplog + ): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + + with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): + files = _detect_git_changed_files(str(pr_repo), mode="pr") + + assert sorted(files) == ["base.py", "feat.py"] + assert caplog.text == "" + + def test_only_the_workspace_is_trusted(self, tmp_path): + command = _git_command_for(tmp_path / "workspace") + + assert command[0] == "git" + trusted = [ + value.split("=", 1)[1] + for value in command + if value.startswith("safe.directory=") + ] + assert trusted + assert all(path.endswith("workspace") for path in trusted) + assert "safe.directory=*" not in command + + class TestScanAllOverrideIsLoud: """`scan_all` outranks `changed_files`, and used to do it silently. From 2be2e9ea07602441d8e3702dc28eaa1daa7fb437 Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 4 Aug 2026 18:40:37 -0400 Subject: [PATCH 7/9] docs: say that Tier 1 reachability is not diff-scoped --- docs/parameters.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/parameters.md b/docs/parameters.md index 72d8264..7c1b797 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -162,6 +162,11 @@ workflow and pass it in yourself: Otherwise the run warns that it was triggered by a comment on a pull request and that it cannot work the base out on its own. +**Socket Tier 1 reachability is not diff-scoped.** It runs `socket scan reach` +over the whole workspace because reachability needs the full dependency graph, +so a `changed_files` scope does not narrow it. That is unchanged behavior, and +the scanners this setting does scope are SAST/OpenGrep, secrets and containers. + **`scan_all` outranks `changed_files` — but only for some scanners.** If `scan_all` is set — from `INPUT_SCAN_ALL`, a JSON config, or a Socket dashboard config — SAST scans the whole workspace and the changed-files scope is From 93fe13bf866ce63524e9ef5d1dbcc714b2fb462f Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 4 Aug 2026 18:59:24 -0400 Subject: [PATCH 8/9] fix(config): only relax git ownership when git is refusing --- CHANGELOG.md | 5 ++-- docs/github-action.md | 9 +++--- socket_basics/core/config.py | 50 ++++++++++++++++++++++++------- tests/test_changed_files_scope.py | 17 +++++++++-- 4 files changed, 62 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7093599..9dcc205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,8 +39,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `detected dubious ownership`, so every diff failed and `changed_files: 'auto'` or `'pr'` resolved to nothing. Nothing in a workflow could fix it: setting `safe.directory` in a workflow step writes the runner's git config, not the - container's. The git reads now trust the workspace they were pointed at, and - only that directory. + container's. When git refuses, the git reads now trust that one workspace + directory so the diff can run, and say so in the log. When git is not + refusing — any ordinary local run — nothing is relaxed. - A scope request that cannot be honored now says why. Shallow checkouts (naming `fetch-depth: 0`), a workspace that is not a git repository, git refusing to read the repository, and a missing PR base each log a specific diff --git a/docs/github-action.md b/docs/github-action.md index 516974c..e5bb3f2 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -347,10 +347,11 @@ everything or nothing: You do not need `git config --global --add safe.directory` for this. The scan runs as root inside a container over a workspace owned by the runner user, and -git normally refuses that with `detected dubious ownership`. The scan trusts the -workspace it was pointed at, so the diff works without any workflow change — -and setting `safe.directory` in a workflow step would not have helped anyway, -because it writes the runner's git config rather than the container's. +git normally refuses that with `detected dubious ownership`. When git refuses, +the scan trusts that one workspace directory so the diff can run, and logs that +it did — so no workflow change is needed. Setting `safe.directory` in a workflow +step would not have helped anyway, because it writes the runner's git config +rather than the container's. When git is not refusing, nothing is relaxed. ### Where the setting can come from diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index 7a643ea..c90af20 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1793,6 +1793,19 @@ def _event_is_comment_on_pull_request() -> bool: return isinstance(issue, dict) and isinstance(issue.get('pull_request'), dict) +def _git_can_read(command: List[str], workspace: Path) -> bool: + """Will ``command`` read the repository at ``workspace``?""" + import subprocess + try: + subprocess.check_output( + [*command, 'rev-parse', '--git-dir'], + cwd=str(workspace), text=True, stderr=subprocess.DEVNULL, + ) + return True + except Exception: + return False + + def _git_command_for(workspace: Path) -> List[str]: """The ``git`` command to use for reading ``workspace``. @@ -1804,22 +1817,39 @@ def _git_command_for(workspace: Path) -> List[str]: Telling the user to run ``git config --global --add safe.directory`` cannot fix it either, because that writes the *runner's* git config and the - container has its own. So declare the trust here instead, for the one - directory the workflow already asked us to scan and for nothing else. When - ownership matches, this changes nothing. + container has its own. So the trust is declared here, for the one directory + we were asked to scan and for nothing else. + + It is only declared when git is actually refusing. ``safe.directory`` guards + against picking up a repository config you do not control, and a plain local + run over your own checkout is never blocked by it, so relaxing it there + would give something up for nothing. """ - trusted = [str(workspace)] + plain = ['git'] + if _git_can_read(plain, workspace): + return plain + + trusted_paths = [str(workspace)] try: resolved = str(workspace.resolve()) - if resolved not in trusted: - trusted.append(resolved) + if resolved not in trusted_paths: + trusted_paths.append(resolved) except OSError: pass - command = ['git'] - for path in trusted: - command += ['-c', f'safe.directory={path}'] - return command + trusted = ['git'] + for path in trusted_paths: + trusted += ['-c', f'safe.directory={path}'] + + if not _git_can_read(trusted, workspace): + return plain + + logging.getLogger(__name__).info( + "git would not read %s on its own, which is the container running as a different " + "user than the workspace owner; trusting that one directory so the changed-file " + "diff can run", str(workspace), + ) + return trusted def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 792e375..ea25513 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -10,6 +10,7 @@ import os import subprocess from argparse import Namespace +from pathlib import Path from types import SimpleNamespace import pytest @@ -531,8 +532,10 @@ def test_ownership_mismatch_no_longer_warns_about_safe_directory( assert sorted(files) == ["base.py", "feat.py"] assert caplog.text == "" - def test_only_the_workspace_is_trusted(self, tmp_path): - command = _git_command_for(tmp_path / "workspace") + def test_only_the_workspace_is_trusted(self, pr_repo, monkeypatch): + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + + command = _git_command_for(pr_repo) assert command[0] == "git" trusted = [ @@ -541,9 +544,17 @@ def test_only_the_workspace_is_trusted(self, tmp_path): if value.startswith("safe.directory=") ] assert trusted - assert all(path.endswith("workspace") for path in trusted) + assert all(Path(path).name == pr_repo.name for path in trusted) assert "safe.directory=*" not in command + def test_nothing_is_trusted_when_git_is_not_refusing(self, pr_repo): + """safe.directory is a real protection for a plain local run. + + git never blocks a checkout you own, so relaxing it there would give + something up and buy nothing. + """ + assert _git_command_for(pr_repo) == ["git"] + class TestScanAllOverrideIsLoud: """`scan_all` outranks `changed_files`, and used to do it silently. From 6451e309923ac1911004bdbb77eb12ae81fd67f1 Mon Sep 17 00:00:00 2001 From: jdalton Date: Tue, 4 Aug 2026 19:08:09 -0400 Subject: [PATCH 9/9] fix(config): say what the ownership retry actually did When both the plain and the trusting probe fail, _git_command_for falls back to plain git, so the warning claiming the directory is trusted was wrong for that path. It now says the retry was attempted and that the problem is more than an ownership mismatch, which is true for the only way that branch is now reachable. Splits the trusting command out of the decision so the "workspace only, never a wildcard" property can be asserted directly. It could not be before: a global safe.directory entry makes git ignore GIT_TEST_ASSUME_DIFFERENT_OWNER, the hosted runners have one, and the test asserting a trusting command was therefore red in CI and green locally. The tests that need git to actually be refusing now skip with a reason instead of passing vacuously. --- socket_basics/core/config.py | 45 ++++++++++++++--------- tests/test_changed_files_scope.py | 59 ++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 34 deletions(-) diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index c90af20..4930732 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1806,6 +1806,27 @@ def _git_can_read(command: List[str], workspace: Path) -> bool: return False +def _trusting_git_command(workspace: Path) -> List[str]: + """A ``git`` command that trusts ``workspace`` and no other directory. + + Both the path as configured and its resolved form are listed, because + ``safe.directory`` entries have to be absolute and a workspace can be given + as a relative path or reached through a symlink. + """ + paths = [str(workspace)] + try: + resolved = str(workspace.resolve()) + if resolved not in paths: + paths.append(resolved) + except OSError: + pass + + command = ['git'] + for path in paths: + command += ['-c', f'safe.directory={path}'] + return command + + def _git_command_for(workspace: Path) -> List[str]: """The ``git`` command to use for reading ``workspace``. @@ -1829,18 +1850,7 @@ def _git_command_for(workspace: Path) -> List[str]: if _git_can_read(plain, workspace): return plain - trusted_paths = [str(workspace)] - try: - resolved = str(workspace.resolve()) - if resolved not in trusted_paths: - trusted_paths.append(resolved) - except OSError: - pass - - trusted = ['git'] - for path in trusted_paths: - trusted += ['-c', f'safe.directory={path}'] - + trusted = _trusting_git_command(workspace) if not _git_can_read(trusted, workspace): return plain @@ -1967,11 +1977,12 @@ def _warn_no_base(refs: List[str]) -> None: """Explain a failed base resolution instead of returning [] quietly.""" if not _git_is_usable(): log.warning( - "Cannot scope the scan to changed files: git refused to read %s even " - "though the scan already trusts that directory, so this is not the " - "usual container ownership mismatch. The checkout is probably damaged " - "or incomplete -- re-run actions/checkout, or pass an explicit file " - "list to changed_files.", + "Cannot scope the scan to changed files: git refused to read %s. The " + "scan already retried with that directory trusted (git -c " + "safe.directory=...), so this is more than the usual container " + "ownership mismatch -- the checkout is probably damaged or incomplete. " + "Re-run actions/checkout, or pass an explicit file list to " + "changed_files.", str(ws), ) return diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index ea25513..3597897 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -18,7 +18,9 @@ from socket_basics.core.config import ( Config, _detect_git_changed_files, + _git_can_read, _git_command_for, + _trusting_git_command, create_config_from_args, resolve_changed_files_request, ) @@ -497,45 +499,53 @@ class TestContainerOwnershipMismatch: these run against real repositories and the real git binary. """ - def test_pr_diff_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): + @pytest.fixture + def refusing_git(self, pr_repo, monkeypatch): + """``pr_repo`` with git refusing to read it on ownership grounds. + + Skips rather than passing vacuously where the switch has no effect, so + a git that ignores it shows up in the run instead of hiding. + """ monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + if _git_can_read(["git"], pr_repo): + pytest.skip("this git ignores GIT_TEST_ASSUME_DIFFERENT_OWNER") + return pr_repo - files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + def test_pr_diff_survives_an_ownership_mismatch(self, refusing_git): + files = _detect_git_changed_files(str(refusing_git), mode="pr", base_ref="main") assert sorted(files) == ["base.py", "feat.py"] - def test_auto_scope_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): + def test_auto_scope_survives_an_ownership_mismatch(self, refusing_git, monkeypatch): monkeypatch.setenv("GITHUB_BASE_REF", "main") - monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") - cfg = _make_config(pr_repo, changed_files="auto") + cfg = _make_config(refusing_git, changed_files="auto") assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] - assert cfg.get_scan_targets() == [str(pr_repo / "base.py"), str(pr_repo / "feat.py")] - - def test_current_commit_survives_an_ownership_mismatch(self, pr_repo, monkeypatch): - monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + assert cfg.get_scan_targets() == [ + str(refusing_git / "base.py"), + str(refusing_git / "feat.py"), + ] - files = _detect_git_changed_files(str(pr_repo), mode="current-commit") + def test_current_commit_survives_an_ownership_mismatch(self, refusing_git): + files = _detect_git_changed_files(str(refusing_git), mode="current-commit") assert "feat.py" in files def test_ownership_mismatch_no_longer_warns_about_safe_directory( - self, pr_repo, monkeypatch, caplog + self, refusing_git, monkeypatch, caplog ): monkeypatch.setenv("GITHUB_BASE_REF", "main") - monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") with caplog.at_level(logging.WARNING, logger="socket_basics.core.config"): - files = _detect_git_changed_files(str(pr_repo), mode="pr") + files = _detect_git_changed_files(str(refusing_git), mode="pr") assert sorted(files) == ["base.py", "feat.py"] assert caplog.text == "" - def test_only_the_workspace_is_trusted(self, pr_repo, monkeypatch): - monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") - - command = _git_command_for(pr_repo) + def test_only_the_workspace_is_trusted(self, pr_repo): + """The trusting command names the workspace, and never a wildcard.""" + command = _trusting_git_command(pr_repo) assert command[0] == "git" trusted = [ @@ -545,8 +555,23 @@ def test_only_the_workspace_is_trusted(self, pr_repo, monkeypatch): ] assert trusted assert all(Path(path).name == pr_repo.name for path in trusted) + assert all(Path(path).is_absolute() for path in trusted) assert "safe.directory=*" not in command + def test_a_relative_workspace_is_trusted_by_its_absolute_path(self, pr_repo, tmp_path): + """safe.directory entries have to be absolute to match anything.""" + command = _trusting_git_command(Path(pr_repo.name)) + + trusted = [ + value.split("=", 1)[1] + for value in command + if value.startswith("safe.directory=") + ] + assert any(Path(path).is_absolute() for path in trusted) + + def test_refusing_git_gets_the_trusting_command(self, refusing_git): + assert _git_command_for(refusing_git) == _trusting_git_command(refusing_git) + def test_nothing_is_trusted_when_git_is_not_refusing(self, pr_repo): """safe.directory is a real protection for a plain local run.