diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fde72b..9dcc205 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,52 @@ 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. + 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 + `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. +- 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. 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 + 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 + 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 ### 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..e5bb3f2 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,43 @@ 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 ... 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`. 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 + +`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..7c1b797 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -109,17 +109,74 @@ 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.** + +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. + +**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 +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 socket-basics --changed-files auto diff --git a/socket_basics/core/config.py b/socket_basics/core/config.py index ac623b4..4930732 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,46 @@ 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. + # + # 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 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-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 +777,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 +1084,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 +1628,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 +1666,202 @@ 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, 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] = [] + + 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 _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 _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 _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``. + + 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 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. + """ + plain = ['git'] + if _git_can_read(plain, workspace): + return plain + + trusted = _trusting_git_command(workspace) + 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]: """Detect changed files in a git repository. @@ -1647,15 +1870,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 +1894,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 @@ -1681,63 +1916,156 @@ 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 _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 - 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 + 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. ``_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) + 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. + + 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. 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 + 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 " + "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) + 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) 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..c9c3789 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') @@ -351,8 +351,21 @@ 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. + # 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() 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" + ) + return results scan_paths = [workspace_path] for scan_path in scan_paths: 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..3597897 100644 --- a/tests/test_changed_files_scope.py +++ b/tests/test_changed_files_scope.py @@ -5,13 +5,25 @@ re-scanning the whole repository. """ +import json +import logging import os import subprocess from argparse import Namespace +from pathlib import Path +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, + _git_can_read, + _git_command_for, + _trusting_git_command, + create_config_from_args, + resolve_changed_files_request, +) def _make_config(workspace, **overrides): @@ -107,9 +119,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") @@ -154,6 +170,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") @@ -170,3 +187,639 @@ def test_delete_only_pr_config_creation_keeps_empty_scope(self, tmp_path, monkey assert cfg.get("changed_files") == [] assert cfg.get_scan_targets() == [] + + +# =========================================================================== +# 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 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() + _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"] + + 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. + + 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 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. + """ + + @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 + + 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, refusing_git, monkeypatch): + monkeypatch.setenv("GITHUB_BASE_REF", "main") + + cfg = _make_config(refusing_git, changed_files="auto") + + assert sorted(cfg.get("changed_files")) == ["base.py", "feat.py"] + assert cfg.get_scan_targets() == [ + str(refusing_git / "base.py"), + str(refusing_git / "feat.py"), + ] + + 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, refusing_git, 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(refusing_git), mode="pr") + + assert sorted(files) == ["base.py", "feat.py"] + assert caplog.text == "" + + 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 = [ + value.split("=", 1)[1] + for value in command + if value.startswith("safe.directory=") + ] + 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. + + 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. + + `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" 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" 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" 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: + """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"] + + +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, 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) + 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)] + + 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)]