fix: changed_files resolved to zero files in the Docker container action (git dubious ownership) - #105
Conversation
…sses The pre-built Docker action runs as root while the checkout at GITHUB_WORKSPACE is owned by the runner user, so git's ownership check (git 2.35.2+) refused the repository. changed_files diff-only mode then resolved to zero files on every PR and the scanners silently skipped with a green run. Git-based repository/branch/commit and default-branch discovery failed the same way in local Docker runs. actions/checkout's own safe.directory entry cannot help: it lands in the runner's global config, which is not mounted into container actions. Inject safe.directory for the scan workspace into the environment of each git subprocess via command-scope GIT_CONFIG_* entries. No config files are touched, and caller-provided GIT_CONFIG_* entries (including the previously documented env-block workaround) are appended after, not clobbered. Tested: - unit: new TestDubiousOwnership tests drive the real git ownership check via GIT_TEST_ASSUME_DIFFERENT_OWNER; they fail on the unpatched code and pass with the fix. TestGitEnv covers append-after-caller, garbage GIT_CONFIG_COUNT, and the GITHUB_WORKSPACE default. Full suite: 222 passed. - container: on the published 3.0.0 and 2.2.1 images with a uid-1001 checkout and a root process, unpatched runs skip with zero targets; patched runs resolve the PR diff and report the seeded finding, with and without a pre-existing user GIT_CONFIG_* block. Same-owner and delete-only-PR behavior unchanged.
|
bugbot run |
Git ignores relative safe.directory values, so a relative --workspace under an ownership mismatch still failed the check and the diff kept resolving to empty. Resolve the path before writing the entry. Regression tests: a relative --workspace now passes the end-to-end ownership test, and TestGitEnv asserts the injected value is absolute. Both fail without this change. Full suite: 224 passed. Re-ran the container check on the published 3.0.0 image: unchanged, finding still reported.
A git failure during scope resolution previously collapsed into the same empty list as a genuinely empty diff, so any future breakage (beyond the safe.directory fix) would again skip every scanner and report green. - _detect_git_changed_files now captures git stderr (instead of DEVNULL), logs the failure reason, and returns None on failure vs [] for a truly empty diff. Ref-not-found is classified separately so the base-ref candidate loop still falls through, while unreadable-repo errors fail fast. - On failed resolution the config layer falls back to a full-repo scan with a prominent warning, never a silent zero-file skip. Delete-only diffs keep the empty-scope skip (existing test still guards this). - An unresolvable base ref in a PR context (e.g. shallow fetch) is now a failure rather than a quiet fall-through to the usually-empty staged diff. - Resolved scope is logged: file count at INFO, full list at DEBUG (the customer ask from the report). - Connector-internal staged-diff callers get 'or []' for the new contract. Full suite: 231 passed. Signed-off-by: lelia <2418071+lelia@users.noreply.github.com>
|
bugbot run |
|
David Larsen (@dc-larsen) Heads up — I pushed What it adds: detection now distinguishes failed resolution ( One thing worth your eyes specifically: the full-scan-fallback and shallow-fetch-base cases are deliberate semantic changes from "skip silently" — flag if you know a customer scenario where that's the wrong call. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3e836bd. Configure here.
|
Verified On your question, two customer scenarios worth weighing, both from the account that hit the original bug:
Both are loud and discoverable, which beats the silent skip. If you want to blunt them, the deterministic base-unresolvable case could fail fast with a config error naming the one-line fix, keeping the full-scan fallback for the non-deterministic failures. Fail-toward-scanning is a defensible default for a security tool and the warning names the cause, so I'm fine shipping it as is. The shallow-fetch change itself looks strictly better. The staged fallthrough in CI was almost always an empty diff, and local pre-commit runs have no |
Problem
changed_filesdiff-only mode resolves to zero files on every PR when Socket Basics runs as the pre-built Docker container action. Each run logsNo scan targets to analyze (scoped scan matched no existing files); skipping OpenGrepand exits green with zero alerts. The scan looks healthy while scanning nothing. Full-repo mode on the same checkout works. A customer hit this rolling the action out org-wide on v2.2.1 (eng-support thread: https://socketdev.slack.com/archives/C05TT2Q3FEZ/p1786394320795579), and it reproduces identically on 3.0.0.Root cause
GitHub runs Docker container actions as root, while the checkout mounted at
/github/workspaceis owned by the runner user (uid 1001). Git 2.35.2+ refuses to read a repository owned by a different user. Every git subprocess in_detect_git_changed_filesfails, the helper catches theCalledProcessErrorand returns[], and the empty scope makes every scanner skip.actions/checkoutdoes not cover this case: itssafe.directoryentry is written to/home/runner/.gitconfig, but container actions run withHOME=/github/home. A prior workflow step runninggit config --global --add safe.directorymisses the container for the same reason.The same mismatch breaks the git fallbacks in
_discover_repository,_discover_branch,_discover_commit_hash, and_discover_is_default_branch. That surface shows up in local Docker runs, where noGITHUB_*env vars mask it.Fix
A
_git_env()helper injectssafe.directory=<workspace>into the environment of each git subprocess via command-scopeGIT_CONFIG_COUNT/GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_nentries. Git honors command-scopesafe.directorysince 2.38, and the image ships 2.47.GIT_CONFIG_*entries are preserved, with ours appended after them, so users who already deployed the env-var workaround are unaffected.Testing
Unit (
tests/test_changed_files_scope.py)TestDubiousOwnershipdrives the real git ownership check end to end usingGIT_TEST_ASSUME_DIFFERENT_OWNER, git's own test knob for this code path. A control probe skips the tests on a git build without the knob. Both tests fail on the unpatched code and pass with the fix. I confirmed that by disabling the injection and re-running.TestGitEnvcovers appending after caller entries, a malformedGIT_CONFIG_COUNT, and theGITHUB_WORKSPACEdefault.Published container images
Repo owned by uid 1001, process as root,
GITHUB_BASE_REF=main, comment-only Go change:Total alerts: 0(bug)--repo/--branchTotal alerts: 1--repo/--branchTotal alerts: 1Total alerts: 1GIT_CONFIG_*block already setTotal alerts: 1(workaround coexists)Total alerts: 1(no regression)Total alerts: 0(empty-diff semantics preserved)Real GitHub Actions A/B
One workflow, two jobs on a comment-only Go-file PR in a scratch repo. Each job builds this action from source at a different ref. The rig branches on my fork swap
image:toDockerfileand the trivy base to the public upstream, and are otherwise identical to base and fix.baseline(unpatchedmain):No scan targets to analyze (scoped scan matched no existing files); skipping OpenGrep,Total alerts: 0, job passed green. That is the customer symptom on a real runner.fix(this branch): the diff resolved tosvc/main.go, OpenGrep ran on it, and the job failed the check withTotal alerts: 1andFound 1 high/critical severity issues. Failing the check is the intended blocking behavior for a seeded critical finding.Run: https://github.com/dc-larsen/sb-changed-files-test/actions/runs/31432924728 (private scratch repo, log lines quoted verbatim above). The rig branches
test-ownership-baselineandtest-ownership-fixondc-larsen/socket-basicsreproduce this against any scratch repo.Notes for review
fix/changed-files-scope-observabilityis complementary: it makes this failure loud instead of silent. One interaction: its troubleshooting doc recommendsgit config --global --add safe.directoryin a prior step, which cannot reach the container action. After this lands, that row can be dropped or swapped for the env-var form.Note
Medium Risk
Changes CI scan scope and fail-open behavior (full-repo fallback on git errors), which can increase scan volume and findings versus the previous silent skip; git env injection is localized and well-tested.
Overview
Fixes
changed_filesdiff-only mode resolving to zero files in the pre-built Docker GitHub Action (root container vs runner-owned checkout → git “dubious ownership”). Adds_git_env()to inject command-scopesafe.directoryviaGIT_CONFIG_*on git subprocesses (no config files; preserves existingGIT_CONFIG_*workarounds), and applies it to changed-file detection plus repo/branch/commit/default-branch discovery._detect_git_changed_filesnow captures stderr, returnsNoneon hard failures vs[]for a real empty diff, and avoids falling back to staged diff in PR CI when the base ref is missing. Config resolution via_apply_scoped_changed_fileslogs scope at INFO/DEBUG, and onNonefalls back to a full-repo scan with warnings instead of a green skip. Trivy/TruffleHog treatNonefrom detection as empty viaor []for staged fallbacks. Tests cover dubious ownership,_git_env, and failure vs empty-diff behavior.Reviewed by Cursor Bugbot for commit 3e836bd. Configure here.