diff --git a/CHANGELOG.md b/CHANGELOG.md index 525d50d..8056ad7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,36 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed +- `changed_files` diff-only mode always resolved to zero files in the pre-built + Docker GitHub Action: the container runs as root while the checkout is owned + by the runner user, so git's ownership check refused every diff lookup, the + scope silently resolved to nothing, and the scanners skipped with a green + run. Git subprocesses now mark the scan workspace as `safe.directory` via + command-scope `GIT_CONFIG_*` environment entries. No config files are + touched, and caller-provided `GIT_CONFIG_*` entries (including the previously + documented workaround) are preserved. The same mismatch broke git-based + repository/branch/commit and default-branch discovery in local Docker runs; + those lookups are covered by the same change. +- A failed `changed_files` diff resolution is no longer indistinguishable from + an empty diff. Git errors are captured and logged instead of discarded, and + when the scope cannot be resolved — unreadable repository, unresolvable base + ref, or `pr` mode with no base ref — Socket Basics now **fails with a + configuration error** rather than reporting a green run that scanned nothing. + Shallow checkouts get a more specific error naming `fetch-depth: 0`. A + genuinely empty diff (e.g. a delete-only PR) is a successful resolution and + still skips the scanners as before. + +### Added +- The resolved `changed_files` scope is now logged on every scoped run: file + count at INFO, the full file list at DEBUG — so an empty diff and a failed + lookup are visible and distinguishable in run logs. +- `scan_all` is now a declared action input and doubles as the fail-open escape + hatch for `changed_files`: when the scope cannot be resolved, widen to a + full-repo scan with a warning instead of failing. The widening is partial — + only scanners that read scan targets widen, while secret and container + scanners stay scoped. + ## [3.0.0] - 2026-08-06 Major release: Trivy-backed scanning returns, now built and published through diff --git a/action.yml b/action.yml index 67eeb35..be4e7fe 100644 --- a/action.yml +++ b/action.yml @@ -11,6 +11,7 @@ runs: INPUT_WORKSPACE: ${{ inputs.workspace }} # Scan scope INPUT_CHANGED_FILES: ${{ inputs.changed_files }} + INPUT_SCAN_ALL: ${{ inputs.scan_all }} INPUT_SCAN_FILES: ${{ inputs.scan_files }} # Input mappings for all parameters INPUT_ALL_LANGUAGES_ENABLED: ${{ inputs.all_languages_enabled }} @@ -118,9 +119,21 @@ inputs: 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. + are skipped rather than scanning the whole repo. When the diff cannot be + resolved at all (unreadable repo, missing base ref) the run fails with a + configuration error instead of reporting a green scan of nothing; set + scan_all to widen to a full-repo scan in that case instead. required: false default: "" + scan_all: + description: >- + Scan the entire workspace even when a narrower scope was requested but + could not be produced. Acts as the fail-open escape hatch for + changed_files: instead of failing when a diff cannot be resolved, widen to + a full-repo scan. Note the widening is partial — only scanners that read + scan targets widen, while secret and container scanners stay scoped. + required: false + default: "false" scan_files: description: >- Explicit comma-separated list of files to scan. Scopes SAST/OpenGrep, diff --git a/docs/github-action.md b/docs/github-action.md index c2e577f..d9be60d 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -324,6 +324,40 @@ 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. +> [!NOTE] +> **When the diff cannot be resolved** — the checkout is unreadable, or the base +> branch is missing (most commonly a shallow clone without `fetch-depth: 0`) — +> Socket Basics **fails with a configuration error** naming the underlying git +> error. It does not scan. +> +> This is deliberate. Diff-only scoping is an explicit instruction, and if it +> cannot be honored there is no honest result to report: +> +> - **Skipping the scanners** would exit green having scanned zero files. A +> passing check that inspected nothing is worse than a failing one, and a +> warning buried in a run log is not something anyone acts on. +> - **Silently scanning everything** would do the expensive thing on every PR — +> precisely what asking for a diff scope was avoiding. On a large repository +> that is a slow or OOM-prone check, and it reports **pre-existing** findings +> rather than the PR's own, so a checkout misconfiguration surfaces as large PR +> comments on every PR until corrected. +> +> If the error appears on **every** PR, the cause is almost always a missing +> `fetch-depth: 0` — fix the checkout rather than sizing up the runner. Shallow +> checkouts get a more specific error naming that fix directly, including the +> `no merge base` shape where the base tip was fetched without connecting +> history. +> +> **To scan anyway, set `scan_all: true`.** That widens an unresolvable scope to +> a full-repository scan with a warning instead of failing. Note the widening is +> partial: only scanners that read scan targets widen, while secret and container +> scanners stay scoped. +> +> A genuinely *empty* diff (e.g. a delete-only PR) is a successful resolution and +> still skips the scanners — only a **failed** resolution errors. The resolved +> file count is logged on every scoped run, so an empty diff and a failed lookup +> are always distinguishable in the logs. + ## 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 22adfeb..55b5d75 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -120,6 +120,24 @@ 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. +If the diff **cannot be resolved** — unreadable repository, missing base ref, or +a shallow checkout with no base to diff against — the run **fails with a +configuration error** naming the underlying git error, and nothing is scanned. +Neither alternative is honest: skipping the scanners exits green having scanned +zero files, and widening to the whole repository does the expensive thing on +every PR, which is what requesting a diff scope was avoiding. Shallow checkouts +get a more specific error naming `fetch-depth: 0`, covering both the missing-ref +and disconnected-history (`no merge base`) shapes. + +Set **`scan_all`** to widen instead of failing: an unresolvable scope then falls +back to a full-workspace scan with a warning. The widening is partial — only +scanners that read scan targets widen, while secret and container scanners stay +scoped to `changed_files`. + +The resolved scope is logged on every run (file count at INFO, full file list +at DEBUG), so an empty diff and a failed lookup are distinguishable in run +logs. + **Example:** ```bash socket-basics --changed-files auto diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index f202d83..7445162 100644 --- a/socket_basics/core/config.py +++ b/socket_basics/core/config.py @@ -1650,39 +1650,77 @@ def create_config_from_args(args) -> Config: 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': + _scope_log = logging.getLogger(__name__) + + def _apply_scoped_changed_files(mode_label: str, **detect_kwargs) -> None: + """Resolve the diff scope, distinguishing failure from an empty diff. + + A failed resolution (None) is a configuration error and stops the + run. Diff-only scoping is an explicit instruction; if it cannot be + honored, Socket Basics cannot make any statement about the code, and + both alternatives are worse than stopping. Skipping the scanners + reports a green check having scanned nothing — false assurance, and a + warning in a run log is not a signal anyone acts on. Silently + widening to the whole repository does the expensive thing on every + PR, which is precisely what a caller asking for a diff scope was + avoiding. + + ``scan_all`` is the opt-in for that widening: it already means "when + the scope resolves to nothing, scan everything rather than nothing", + so it doubles as the documented fail-open escape hatch. + + A genuinely empty diff (e.g. a delete-only PR) is a successful + resolution — it keeps the empty scope and skips, as before. + """ + fail_open = bool(config_dict.get('scan_all', False)) try: - git_changed = _detect_git_changed_files(config_dict.get('workspace', os.getcwd()), mode='auto') - config_dict['changed_files'] = git_changed + resolved = _detect_git_changed_files( + config_dict.get('workspace', os.getcwd()), fail_open=fail_open, **detect_kwargs + ) except Exception as e: - logging.getLogger(__name__).warning("Warning: failed to detect git changed files (auto): %s", e) - config_dict['changed_files'] = [] + _scope_log.warning("Warning: failed to detect git changed files (%s): %s", mode_label, e) + resolved = None + if resolved is None: + if fail_open: + _scope_log.warning( + "changed_files scope could not be resolved (%s); scan_all is set, so " + "falling back to a full-repo scan. See the warnings above for the " + "underlying git error.", + mode_label, + ) + config_dict['changed_files'] = [] + config_dict['changed_files_scope_requested'] = False + return + raise SystemExit( + f"changed_files: the requested scope ({mode_label}) could not be resolved, so " + "the scan would either report a green run having scanned nothing or silently " + "widen to the whole repository. See the warnings above for the underlying git " + "error. Fix the git problem, or set scan_all to widen to a full-repo scan when " + "the scope cannot be resolved." + ) + _scope_log.info("changed_files scope resolved to %d file(s) (%s)", len(resolved), mode_label) + if resolved: + _scope_log.debug("changed_files scope: %s", ", ".join(resolved)) + else: + _scope_log.info( + "changed_files diff is genuinely empty (e.g. delete-only change); " + "scoped scanners will be skipped" + ) + config_dict['changed_files'] = resolved + + # 'auto' resolves to the PR base-ref diff in CI, else staged changes. + if val.lower() == 'auto': + _apply_scoped_changed_files('auto', mode='auto') 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'] = [] + _apply_scoped_changed_files('pr', mode='pr') 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'] = [] + _apply_scoped_changed_files('current-commit', mode='current-commit') 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'] = [] + _apply_scoped_changed_files(f'commit {val}', mode='commit', commit=val) else: # parse comma-separated list of files provided manually config_dict['changed_files'] = [f.strip() for f in val.split(',') if f.strip()] @@ -1721,7 +1759,55 @@ def create_config_from_args(args) -> Config: return Config(config_dict) -def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None) -> List[str]: +def _git_env(workspace_path: str | Path | None = None) -> Dict[str, str]: + """Environment for git subprocesses that marks the scan workspace safe to read. + + The pre-built GitHub Action runs as root inside a Docker container while the + checkout at ``GITHUB_WORKSPACE`` is owned by the runner user, so git's + ownership check (git 2.35.2+) refuses the repository and every git lookup + here fails. ``changed_files`` diff-only mode then resolves to zero files and + the scanners silently skip. ``actions/checkout`` cannot help: its + ``safe.directory`` entry is written to the runner's global config, which is + not mounted into container actions. + + The workspace is an explicit scan target, not an incidentally discovered + repository, so mark it safe for these subprocesses only. Injecting via + ``GIT_CONFIG_*`` (command-scope config, honored for ``safe.directory`` since + git 2.38; the bundled image ships newer) touches no config files, and + appending after any caller-provided ``GIT_CONFIG_*`` entries preserves + workarounds users already deployed. The path is resolved to an absolute one + first because git ignores relative ``safe.directory`` values. + """ + env = dict(os.environ) + try: + count = max(0, int(env.get('GIT_CONFIG_COUNT', '0') or '0')) + except ValueError: + count = 0 + ws = workspace_path or os.environ.get('GITHUB_WORKSPACE') or os.getcwd() + env[f'GIT_CONFIG_KEY_{count}'] = 'safe.directory' + env[f'GIT_CONFIG_VALUE_{count}'] = str(Path(ws).resolve()) + env['GIT_CONFIG_COUNT'] = str(count + 1) + return env + + +class _GitScopeError(Exception): + """A git invocation needed for changed-files scoping failed outright. + + Carries git's first stderr line as the message. ``ref_miss`` is True when + the failure only means "this ref does not exist" (safe to try another + candidate) rather than "git could not read the repository at all." + ``merge_base_miss`` is True when the ref exists but shares no history with + HEAD (``A...HEAD: no merge base``) — the signature of a partial/shallow + fetch where the base tip was fetched without connecting history. + """ + + def __init__(self, message: str, ref_miss: bool = False, merge_base_miss: bool = False): + super().__init__(message) + self.ref_miss = ref_miss + self.merge_base_miss = merge_base_miss + + +def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: str | None = None, base_ref: str | None = None, fail_open: bool = False) -> Optional[List[str]]: """Detect changed files in a git repository. mode: @@ -1735,11 +1821,23 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: (``GITHUB_BASE_REF`` is set), 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 []. + Returns a list of file paths relative to the workspace root. An empty list + means git resolved the diff and it is genuinely empty (e.g. a delete-only + change), or the workspace is not a git repo (nothing to diff). Returns + ``None`` when resolution *failed* — git could not read the repository, or a + requested base ref could not be resolved — so callers can distinguish "no + changed files" from "the lookup broke" instead of silently scanning + nothing. The specific git error is logged here at WARNING level. + + ``fail_open`` mirrors the caller's ``scan_all`` setting. When False (the + default) a deterministic misconfiguration raises SystemExit here with the + specific fix, rather than returning None for the caller to turn into a + generic error. When True the caller has opted into widening an unresolvable + scope to a full-repo scan, so those checks are skipped and the failure is + reported as None. """ + log = logging.getLogger(__name__) try: - from subprocess import check_output, CalledProcessError import subprocess # Prefer GITHUB_WORKSPACE if set (GitHub Actions environment) @@ -1757,70 +1855,159 @@ def _detect_git_changed_files(workspace_path: str, mode: str = 'staged', commit: if not git_dir.exists(): return [] + # Mark the workspace safe for the git subprocesses below; without this + # every command fails under the container-action ownership mismatch and + # the diff silently resolves to nothing. + git_env = _git_env(ws) + + # stderr markers that mean "this ref does not exist" — a soft miss the + # base-ref candidate loop may retry — as opposed to git being unable + # to read the repository at all (ownership, corruption, ...). + ref_miss_markers = ('unknown revision', 'bad revision', 'ambiguous argument') + + def _split(out: str) -> List[str]: + return [line.strip() for line in out.splitlines() if line.strip()] + + def _run_git(args: List[str]) -> List[str]: + """Run git, returning stdout lines; raise _GitScopeError on failure. + + stderr is captured rather than discarded so the failure reason — + e.g. git's self-diagnosing ``dubious ownership`` message — survives + into the logs instead of being indistinguishable from an empty diff. + """ + res = subprocess.run(args, text=True, capture_output=True, env=git_env) + if res.returncode != 0: + stderr = (res.stderr or '').strip() + first = stderr.splitlines()[0] if stderr else f'exit code {res.returncode}' + miss = any(m in stderr.lower() for m in ref_miss_markers) + mb_miss = 'no merge base' in stderr.lower() + raise _GitScopeError(first, ref_miss=miss, merge_base_miss=mb_miss) + return _split(res.stdout) + # Change to workspace directory before running git commands # This ensures git runs in the correct repository context original_cwd = os.getcwd() try: os.chdir(str(ws)) - def _split(out: str) -> List[str]: - return [line.strip() for line in out.splitlines() if line.strip()] + def _fail_fast_if_shallow(ref: str) -> None: + """Deterministic misconfiguration check for an unresolvable base. + + A shallow checkout (no ``fetch-depth: 0``) can *never* resolve + the base ref, so every PR on this checkout would fail the same + way. Name the one-line fix here instead of letting the caller + report a generic unresolvable-scope error. Follows the existing + SystemExit convention for unrecoverable configuration errors + (see repository/branch discovery below). + + Skipped under ``fail_open``: the caller set ``scan_all`` and has + opted into widening an unresolvable scope instead of failing. + """ + if fail_open: + return + try: + shallow = _run_git(['git', 'rev-parse', '--is-shallow-repository']) == ['true'] + except Exception: + return # probe failed; fall through to the generic fallback + if shallow: + raise SystemExit( + f"changed_files: cannot diff against base ref '{ref}' (missing ref or no " + "shared history) and this checkout is shallow. Set 'fetch-depth: 0' on " + "actions/checkout (or otherwise fetch the base branch with full history) " + "so the diff has a base to compare against." + ) + + def _resolve_base_diff(ref: str) -> Optional[List[str]]: + """Base diff with the shallow fail-fast applied to both failure shapes. + + A shallow misconfiguration shows up either as a missing base ref + (nothing fetched) or as ``no merge base`` (base tip fetched but + history disconnected). Both are deterministic — fail fast when + shallow; otherwise preserve the original failure semantics. + """ + if not ref: + return None + try: + files = _diff_against_base(ref) + except _GitScopeError as e: + if e.merge_base_miss: + _fail_fast_if_shallow(ref) + log.warning("changed_files scope: base ref %r shares no merge base with HEAD (%s)", ref, e) + return None + raise + if files is None: + _fail_fast_if_shallow(ref) + return files def _diff_against_base(ref: str) -> Optional[List[str]]: """Diff changed files (excluding deletions) against a base ref. 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 - ``--diff-filter=ACMR`` excludes deleted paths so they never - become scan targets. + bare ref. Returns None when neither candidate resolves (the ref + does not exist locally); raises _GitScopeError when git itself + cannot read the repository. The ``--diff-filter=ACMR`` excludes + deleted paths so they never become scan targets. """ if not ref: return None + last_miss = '' 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 + return _run_git(['git', 'diff', '--name-only', '--diff-filter=ACMR', f'{candidate}...HEAD']) + except _GitScopeError as e: + if e.ref_miss: + last_miss = str(e) + continue + raise + log.warning("changed_files scope: base ref %r could not be resolved (%s)", ref, last_miss or 'no candidates tried') return None if mode == 'auto': # Prefer the PR base-ref 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) + pr_files = _resolve_base_diff(base) 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) + if base: + # A base ref was provided (we are in a PR context) but could + # not be diffed against, and the shallow fail-fast did not + # apply (non-shallow checkout). Report failure so the caller + # falls back to a full scan rather than the staged diff, + # which is almost always empty in CI. + return None + return _run_git(['git', 'diff', '--name-only', '--cached']) elif mode == 'pr': base = base_ref or os.environ.get('GITHUB_BASE_REF', '') - return _diff_against_base(base) or [] + if not base: + log.warning("changed_files scope: mode 'pr' but no base ref available (GITHUB_BASE_REF unset)") + return None + return _resolve_base_diff(base) elif mode == 'staged': # staged but not yet committed - out = check_output(['git', 'diff', '--name-only', '--cached'], text=True, stderr=subprocess.DEVNULL) - return _split(out) + return _run_git(['git', 'diff', '--name-only', '--cached']) 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) - return _split(out) + return _run_git(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', 'HEAD']) elif mode == 'commit' and commit: - out = check_output(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit], text=True, stderr=subprocess.DEVNULL) - return _split(out) + return _run_git(['git', 'diff-tree', '--no-commit-id', '--name-only', '-r', commit]) else: return [] finally: # Always restore original working directory os.chdir(original_cwd) - except CalledProcessError: - return [] - except Exception: - return [] + except _GitScopeError as e: + msg = str(e) + hint = '' + if 'dubious ownership' in msg.lower(): + hint = (" — the checkout is owned by a different user; the workspace should be" + " marked safe.directory automatically as of this release, so please report this") + log.warning("changed_files scope: git failed: %s%s", msg, hint) + return None + except Exception as e: + log.warning("changed_files scope: unexpected error during git detection: %s", e) + return None def discover_all_files(workspace_path: str, respect_gitignore: bool = True) -> List[str]: @@ -2016,9 +2203,10 @@ def _discover_repository(cli_repo: str | None, github_repository: str = '', gith # 4. Git information try: url = subprocess.check_output( - ['git', 'config', '--get', 'remote.origin.url'], - text=True, - stderr=subprocess.DEVNULL + ['git', 'config', '--get', 'remote.origin.url'], + text=True, + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if url.endswith('.git'): @@ -2085,9 +2273,10 @@ def _discover_branch(cli_branch: str | None, github_head_ref: str = '', github_r # 4. Git information try: branch = subprocess.check_output( - ['git', 'rev-parse', '--abbrev-ref', 'HEAD'], + ['git', 'rev-parse', '--abbrev-ref', 'HEAD'], text=True, - stderr=subprocess.DEVNULL + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if branch and branch != 'HEAD': @@ -2122,9 +2311,10 @@ def _discover_commit_hash() -> str: # 2. Git information try: commit = subprocess.check_output( - ['git', 'rev-parse', '--short', 'HEAD'], + ['git', 'rev-parse', '--short', 'HEAD'], text=True, - stderr=subprocess.DEVNULL + stderr=subprocess.DEVNULL, + env=_git_env() ).strip() if commit: @@ -2166,7 +2356,8 @@ def _discover_is_default_branch(current_branch: str, workspace_path: str = '') - ['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'], text=True, stderr=subprocess.DEVNULL, - cwd=cwd + cwd=cwd, + env=_git_env(workspace_path) ).strip() # Extract branch name from refs/remotes/origin/branch-name @@ -2187,7 +2378,8 @@ def _discover_is_default_branch(current_branch: str, workspace_path: str = '') - ['git', 'ls-remote', '--symref', 'origin', 'HEAD'], text=True, stderr=subprocess.DEVNULL, - cwd=cwd + cwd=cwd, + env=_git_env(workspace_path) ).strip() # Parse the output: "ref: refs/heads/main\tHEAD" diff --git a/socket_basics/core/connector/trivy/trivy.py b/socket_basics/core/connector/trivy/trivy.py index c4f518b..bedbc3c 100644 --- a/socket_basics/core/connector/trivy/trivy.py +++ b/socket_basics/core/connector/trivy/trivy.py @@ -144,7 +144,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] @@ -174,7 +174,7 @@ def scan_dockerfiles(self) -> Dict[str, Any]: try: # import helper from config module from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] if changed_files: @@ -325,7 +325,7 @@ def scan_vulnerabilities(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] diff --git a/socket_basics/core/connector/trufflehog/__init__.py b/socket_basics/core/connector/trufflehog/__init__.py index 43eef14..522643c 100644 --- a/socket_basics/core/connector/trufflehog/__init__.py +++ b/socket_basics/core/connector/trufflehog/__init__.py @@ -228,7 +228,7 @@ def scan(self) -> Dict[str, Any]: if not changed_files: try: from socket_basics.core.config import _detect_git_changed_files - changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') + changed_files = _detect_git_changed_files(str(self.config.workspace), mode='staged') or [] except Exception: changed_files = [] diff --git a/tests/test_changed_files_scope.py b/tests/test_changed_files_scope.py index 3eda06d..c50b60c 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -7,11 +7,18 @@ import os import subprocess +from pathlib import Path from argparse import Namespace 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, + _discover_repository, + _git_env, + create_config_from_args, +) def _make_config(workspace, **overrides): @@ -170,3 +177,270 @@ def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkey assert cfg.get("changed_files") == [] assert cfg.get_scan_targets() == [] + + +def _git_refuses_repo(repo): + """True when git, told to assume a different owner, refuses to read `repo`. + + ``GIT_TEST_ASSUME_DIFFERENT_OWNER`` is git's own test knob for the + ownership check behind ``safe.directory``; it makes every repository look + like it belongs to another user, which is exactly what a checkout looks + like from inside the container action. Used as a control so these tests + skip (instead of passing vacuously) on a git build without the knob. + """ + probe = subprocess.run( + ["git", "-C", str(repo), "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + env={**os.environ, "GIT_TEST_ASSUME_DIFFERENT_OWNER": "1"}, + ) + return probe.returncode != 0 + + +class TestDubiousOwnership: + """Git subprocesses must survive the container-action ownership mismatch. + + The pre-built Docker action runs as root while the checkout is owned by + the runner user; without ``safe.directory`` git refuses the repo, the diff + resolves to zero files, and the scanners silently skip. + """ + + def test_pr_diff_survives_dubious_ownership(self, pr_repo, monkeypatch): + for i in range(3): + monkeypatch.delenv(f"GIT_CONFIG_KEY_{i}", raising=False) + monkeypatch.delenv(f"GIT_CONFIG_VALUE_{i}", raising=False) + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + 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_relative_workspace_survives_dubious_ownership(self, pr_repo, monkeypatch): + # git ignores relative safe.directory values, so the injected path must + # be absolutized even when --workspace is given as a relative path. + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + monkeypatch.chdir(pr_repo.parent) + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + files = _detect_git_changed_files(pr_repo.name, mode="pr", base_ref="main") + assert sorted(files) == ["base.py", "feat.py"] + + def test_repo_discovery_survives_dubious_ownership(self, pr_repo, monkeypatch): + _git(pr_repo, "remote", "add", "origin", "https://github.com/acme/demo.git") + + if not _git_refuses_repo(pr_repo): + pytest.skip("this git build does not honor GIT_TEST_ASSUME_DIFFERENT_OWNER") + + monkeypatch.chdir(pr_repo) + monkeypatch.setenv("GIT_TEST_ASSUME_DIFFERENT_OWNER", "1") + assert _discover_repository(None, "", "") == "acme/demo" + + +class TestGitEnv: + """_git_env injects safe.directory without clobbering caller config.""" + + def test_injects_safe_directory_for_workspace(self, monkeypatch): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_0"] == "/scan/me" + + def test_appends_after_caller_provided_entries(self, monkeypatch): + # A user already deploying the documented env-var workaround must not + # have their entry clobbered. + monkeypatch.setenv("GIT_CONFIG_COUNT", "1") + monkeypatch.setenv("GIT_CONFIG_KEY_0", "user.name") + monkeypatch.setenv("GIT_CONFIG_VALUE_0", "runner") + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "2" + assert env["GIT_CONFIG_KEY_0"] == "user.name" + assert env["GIT_CONFIG_VALUE_0"] == "runner" + assert env["GIT_CONFIG_KEY_1"] == "safe.directory" + assert env["GIT_CONFIG_VALUE_1"] == "/scan/me" + + def test_garbage_count_treated_as_zero(self, monkeypatch): + monkeypatch.setenv("GIT_CONFIG_COUNT", "not-a-number") + env = _git_env("/scan/me") + assert env["GIT_CONFIG_COUNT"] == "1" + assert env["GIT_CONFIG_KEY_0"] == "safe.directory" + + def test_workspace_value_is_absolute(self, monkeypatch, tmp_path): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + monkeypatch.chdir(tmp_path) + env = _git_env("some/relative/dir") + assert Path(env["GIT_CONFIG_VALUE_0"]).is_absolute() + + def test_defaults_to_github_workspace(self, monkeypatch): + monkeypatch.delenv("GIT_CONFIG_COUNT", raising=False) + monkeypatch.setenv("GITHUB_WORKSPACE", "/github/workspace") + env = _git_env() + assert env["GIT_CONFIG_VALUE_0"] == "/github/workspace" + + +class TestScopeResolutionFailure: + """Failed diff resolution must be distinguishable from an empty diff. + + A git failure (unreadable repo, unresolvable base ref) returns None from the + detection helper, and the config layer turns that into a configuration error + — never a green run that silently scanned nothing, and never a silent widen + to the whole repository. ``scan_all`` opts into the widen instead. A + genuinely empty diff still returns [] and keeps the skip behavior (see the + delete-only test above). + """ + + def test_unreadable_repo_returns_none(self, pr_repo): + # Corrupt HEAD so every git command fails hard (not a ref miss). + (pr_repo / ".git" / "HEAD").write_text("garbage") + result = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert result is None + + def test_unresolvable_base_ref_returns_none(self, pr_repo): + result = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") + assert result is None + + def test_auto_with_unresolvable_base_ref_returns_none(self, pr_repo, monkeypatch): + # In a PR context (base ref set) an unresolvable base must NOT quietly + # fall back to the (usually empty) staged diff. + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + result = _detect_git_changed_files(str(pr_repo), mode="auto") + assert result is None + + def test_pr_mode_without_base_ref_returns_none(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _detect_git_changed_files(str(pr_repo), mode="pr") + assert result is None + + def test_failure_logs_git_stderr(self, pr_repo, caplog): + (pr_repo / ".git" / "HEAD").write_text("garbage") + with caplog.at_level("WARNING"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert any("changed_files scope" in r.getMessage() for r in caplog.records) + + def test_config_creation_fails_closed_on_failure(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("INPUT_SCAN_ALL", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + (pr_repo / ".git" / "HEAD").write_text("garbage") + + # Scope resolution failed -> configuration error. Neither a green run + # that scanned nothing nor a silent full-repo scan. + with pytest.raises(SystemExit, match="could not be resolved"): + create_config_from_args(_config_args(pr_repo, "auto")) + + def test_config_error_names_scan_all_escape_hatch(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.delenv("INPUT_SCAN_ALL", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + (pr_repo / ".git" / "HEAD").write_text("garbage") + + with pytest.raises(SystemExit, match="scan_all"): + create_config_from_args(_config_args(pr_repo, "auto")) + + def test_scan_all_opts_into_full_scan_fallback(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + monkeypatch.setenv("INPUT_SCAN_ALL", "true") + (pr_repo / ".git" / "HEAD").write_text("garbage") + + with caplog.at_level("WARNING"): + cfg = create_config_from_args(_config_args(pr_repo, "auto")) + + # scan_all is the documented fail-open opt-in: widen, do not fail. + assert cfg.get("changed_files") == [] + assert cfg.get("changed_files_scope_requested") is False + assert cfg.get_scan_targets() == [str(pr_repo)] + assert any("falling back to a full-repo scan" in r.getMessage() for r in caplog.records) + + def test_config_creation_logs_resolved_count(self, pr_repo, monkeypatch, caplog): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + with caplog.at_level("INFO"): + cfg = create_config_from_args(_config_args(pr_repo, "auto")) + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert any("resolved to 2 file(s)" in r.getMessage() for r in caplog.records) + + +class TestShallowCheckoutFailFast: + """A shallow checkout that cannot resolve the base ref is a deterministic + misconfiguration (missing fetch-depth: 0), so the detection helper raises + with that one-line fix rather than returning a generic failure. Other + failures return None and let the config layer report the generic + unresolvable-scope error. Under ``fail_open`` (the caller set ``scan_all``) + the check is skipped so the widen can happen instead. + """ + + def test_shallow_missing_base_fails_fast_pr_mode(self, pr_repo): + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") + + def test_shallow_missing_base_fails_fast_auto_mode(self, pr_repo, monkeypatch): + (pr_repo / ".git" / "shallow").touch() + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="auto") + + def test_shallow_with_resolvable_base_still_diffs(self, pr_repo): + # Shallowness alone is fine — only shallow AND unresolvable-base fails. + (pr_repo / ".git" / "shallow").touch() + files = _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + assert sorted(files) == ["base.py", "feat.py"] + + def test_non_shallow_missing_base_returns_none(self, pr_repo): + assert _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="no-such-branch") is None + + def test_fail_open_skips_shallow_fail_fast(self, pr_repo): + # scan_all was set, so the caller wants to widen rather than fail: the + # shallow check must not pre-empt that with a SystemExit. + (pr_repo / ".git" / "shallow").touch() + result = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="no-such-branch", fail_open=True + ) + assert result is None + + def test_fail_open_skips_no_merge_base_fail_fast(self, pr_repo): + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + (pr_repo / ".git" / "shallow").touch() + result = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="main", fail_open=True + ) + assert result is None + + def test_fail_open_still_resolves_a_good_diff(self, pr_repo): + # fail_open only affects failure handling, never a successful diff. + files = _detect_git_changed_files( + str(pr_repo), mode="pr", base_ref="main", fail_open=True + ) + assert sorted(files) == ["base.py", "feat.py"] + + def test_config_creation_propagates_config_error(self, pr_repo, monkeypatch): + monkeypatch.delenv("GITHUB_WORKSPACE", raising=False) + monkeypatch.setenv("GITHUB_BASE_REF", "no-such-branch") + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + create_config_from_args(_config_args(pr_repo, "auto")) + + def test_shallow_no_merge_base_fails_fast(self, pr_repo, monkeypatch): + # Base tip exists but shares no history with HEAD (partial fetch shape): + # `A...HEAD: no merge base`. Shallow -> config error, same as missing ref. + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + (pr_repo / ".git" / "shallow").touch() + with pytest.raises(SystemExit, match="fetch-depth"): + _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") + + def test_non_shallow_no_merge_base_returns_none(self, pr_repo): + _git(pr_repo, "checkout", "--orphan", "disconnected") + _git(pr_repo, "add", "-A") + _git(pr_repo, "commit", "-m", "orphan") + assert _detect_git_changed_files(str(pr_repo), mode="pr", base_ref="main") is None