Repository navigation
fix: security/sanitize cached git mirrors - #417
stuartwdouglas merged 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fb6908d3b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // LC_ALL=C pins git's messages to English so the filter fallback | ||
| // warning below is detectable regardless of the host locale. | ||
| cmd.Env = append(os.Environ(), "GIT_LFS_SKIP_SMUDGE=1", "LC_ALL=C") | ||
| cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL="+os.DevNull, |
There was a problem hiding this comment.
When the cachew process is started with Git command-scope configuration via GIT_CONFIG_COUNT/GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_n, os.Environ() preserves those variables and Git still applies them despite GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_GLOBAL=/dev/null. The git(1) environment documentation says these values are added to runtime configuration and “will override values in configuration files,” so an untrusted repository can select a matching smudge/process filter in .gitattributes and execute that host command during this clone; the checkout in internal/strategy/gomod/private_fetcher.go repeats the same construction. Append GIT_CONFIG_COUNT=0 (and clear legacy command-scope config variables) so disabling file-based config actually closes this execution path.
Useful? React with 👍 / 👎.
| if err := repo.ConfigureMirror(ctx, clonePath); err != nil { | ||
| if removeErr := os.RemoveAll(clonePath); removeErr != nil { |
There was a problem hiding this comment.
When an existing mirror is first opened with a canceled or timed-out request context—such as private gomod usage without a prior DiscoverExisting pass—ConfigureMirror's Git subprocess returns the context error, but this branch treats every error as unsafe cache contents, deletes the entire valid mirror, and then returns an empty repository with no error. Propagate cancellation and other transient execution errors, reserving removal for failures that actually establish unsafe metadata, so callers retain control over failure handling.
AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05eeb4377a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| cmd.Env = append(os.Environ(), "GIT_CONFIG_NOSYSTEM=1", "GIT_CONFIG_GLOBAL="+os.DevNull, | ||
| "GIT_CONFIG_COUNT=0", "GIT_CONFIG_PARAMETERS=", "GIT_LFS_SKIP_SMUDGE=1", "LC_ALL=C") |
There was a problem hiding this comment.
When cachew is launched with GIT_TEMPLATE_DIR pointing to a template containing an executable post-checkout hook, this new environment still inherits that directory. Git 2.43's git clone -h describes --template as the "directory from which templates will be used," and githooks(5) says post-checkout is also run after git clone unless --no-checkout is used; consequently this clone copies and executes the hook, then archives it under destDir/.git/hooks for clients to restore. The preceding --no-checkout clone in private_fetcher.go also copies such a hook and its subsequent checkout executes it, so force an empty template or clear GIT_TEMPLATE_DIR and disable hooks for both clone paths.
Useful? React with 👍 / 👎.
| if refStorage != "" { | ||
| config += "\trefStorage = " + refStorage + "\n" | ||
| } | ||
| if err := os.WriteFile(configPath, []byte(config), 0o600); err != nil { |
There was a problem hiding this comment.
If cachew is terminated after the cleanup has removed the existing config, or this WriteFile leaves a partial file after an I/O failure, the mirror still retains HEAD. On the next GetOrCreate, resetMirrorMetadata fails its initial Lstat or config parse with an ordinary error rather than errUnsafeMirrorMetadata, so the broken directory is preserved and every request for that repository keeps failing instead of recloning. Write the replacement through a temporary file and rename it atomically, or classify a missing/malformed config as unsafe so interrupted sanitization is recoverable.
Useful? React with 👍 / 👎.
stuartwdouglas
changed the title
No description provided.