Repository navigation
Constrain Codex config writes to the managed temp directory - #74915
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The path hardening introduces two correctness/security regressions: it rejects the supported ARC/DinD threat-detection CODEX_HOME, and it accepts expression-bearing paths whose runtime expansion can escape the managed root.
Highlights
- Align validation with the generated threat-detection path or allow that trusted root.
- Validate individual CODEX_HOME components and reject workflow expressions/metacharacters before emitting runtime paths.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| const reflectPath = (options && options.reflectPath) || AWF_REFLECT_OUTPUT_PATH; | ||
| const provider = normalizeReflectProviderName(options?.provider || process.env.GH_AW_LLM_PROVIDER, "openai"); | ||
| if (!reflectPath) return { env, configured: false }; | ||
| if (codexConfigPath && !isSafeCodexConfigPath(codexConfigPath)) { |
There was a problem hiding this comment.
This guard rejects the ARC/DinD threat-detection CODEX_HOME, so provider configuration is skipped for those runs and Azure OIDC can fail at the later providerConfig.configured check.
💡 Preserve the supported threat-detection path
threat_detection_external.go emits CODEX_HOME=${RUNNER_TEMP}/gh-aw/threat-detection/codex-home, which is intentionally outside /tmp/gh-aw on normal runners. Either make the Go side emit the same managed root that this check validates, or explicitly validate the threat-detection directory as another trusted root and add a regression test for that generated path.
| if home := getEngineEnvOverrides(workflowData)["CODEX_HOME"]; home != "" { | ||
| return home | ||
| cleanHome := path.Clean(home) | ||
| if cleanHome == constants.TmpGhAwDir || strings.HasPrefix(cleanHome, constants.TmpGhAwDir+"/") { |
There was a problem hiding this comment.
This prefix check still accepts expression or shell-metacharacter content beneath /tmp/gh-aw, allowing the validated compile-time path to become an escaping runtime path after Actions expands it.
💡 Validate path components, not only the prefix
For example, /tmp/gh-aw/${{ github.event.issue.title }} passes this check, and an expanded title containing ../../ can redirect the generated CODEX_HOME operations outside the managed directory. Restrict accepted components to a safe allowlist (or reject ${{, $, backticks, quotes, whitespace, and controls) and add a test for a prefixed expression/metacharacter value.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (triage: bug fix). Requesting changes for one verified correctness bug in the JS guard. The Go-side constraint looks sound.
📋 Key Themes & Highlights
Key Themes
- Symlink/alias false negative in
isSafeCodexConfigPath: the root is realpath'd but the relative path is not, so a config inside an aliased managed root is rejected. I reproduced this with the real function. It fails closed, but it breaks the feature on aliased roots. The suggested fix is in the inline comment. - Test coverage gaps: no aliased-root regression test; the parser fixture rewrite drops
for ... doneloop coverage. - Trust boundary:
codexHome()correctly rejects traversal, relative paths, and${{ }}expressions. The JS readers still trustprocess.env.CODEX_HOMEdirectly.
Positive Highlights
- ✅ The
codexHome()gate is simple and applied at every call site I checked. - ✅ The new outside-root and symlink-outside tests exercise the rejection path.
- ✅ Go tests pass:
TestCodexHomeStaysWithinManagedTemporaryDirectoryand related codex tests.
Limits
- Vitest could not run in this environment (no local binary; npm registry TLS failure). The JS test changes are unverified locally. CI should confirm.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~9 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| function isSafeCodexConfigPath(codexConfigPath) { | ||
| try { | ||
| const root = fs.realpathSync(TMP_GH_AW_PATH); | ||
| const resolvedPath = path.resolve(codexConfigPath); |
There was a problem hiding this comment.
[/diagnosing-bugs] relativePath is computed from the unresolved resolvedPath, but the root is resolved with fs.realpathSync. When /tmp/gh-aw is reached through a symlink (for example macOS /tmp → /private/tmp), a config inside the managed directory is treated as outside it. The provider rewrite is then skipped with a misleading warning. I extracted this function and ran it: the path through the alias returns false, and the canonical path returns true.
💡 Suggested fix
Resolve the directory first, then rejoin the basename. The existing lstat check still rejects a symlinked config.toml:
const resolvedDirectory = fs.realpathSync(path.dirname(path.resolve(codexConfigPath)));
const resolvedPath = path.join(resolvedDirectory, path.basename(codexConfigPath));I tested this variant. The aliased path returns true, and a symlinked config.toml returns false.
@copilot please address this.
| } | ||
| }); | ||
|
|
||
| it("does not rewrite a Codex config symlinked outside the managed temporary directory", () => { |
There was a problem hiding this comment.
[/tdd] No test covers a managed root reached through a symlink, which is how the bug above got through. isSafeCodexConfigPath hardcodes TMP_GH_AW_PATH, so tests cannot point it at a temp root. Let the helper accept an optional root (defaulting to TMP_GH_AW_PATH). That makes an aliased-root case easy to add as a regression test.
@copilot please address this.
| printf '%s\n' '---FILES---' | ||
| if [ -f "$WORK_DIR/result.json" ]; then cat "$WORK_DIR/result.json"; fi | ||
| wc -l file.txt`; | ||
| expect(extractCommandNamesFromPipeline(cmd)).toEqual(["set", "mkdir", "git", "sed", "printf", "cat", "wc"]); |
There was a problem hiding this comment.
[/tdd] The rewritten fixture removes the for ... done loop, but the test is still named "...with variables and control flow". Loop-body extraction now has no coverage. Keep a generic loop, for example for f in file1.txt file2.txt; do wc -l "$f"; done, so the cleanup keeps the original coverage.
@copilot please address this.
| func codexHome(workflowData *WorkflowData) string { | ||
| if home := getEngineEnvOverrides(workflowData)["CODEX_HOME"]; home != "" { | ||
| return home | ||
| cleanHome := path.Clean(home) |
There was a problem hiding this comment.
[/codebase-design] codexHome is the Go-side gate, but convert_gateway_config_codex.cjs (lines 51 and 76) and the harness read path still use process.env.CODEX_HOME with no equivalent check. The guarantee holds only while the compiler always sets CODEX_HOME from codexHome(). I have not confirmed whether engine.env can reach the runtime environment unchanged. Either verify that, or add a JS-side check so the rule lives in one place.
@copilot please address this.
| const { detectNonRetryableHarnessGuard } = require("./harness_retry_guard.cjs"); | ||
|
|
||
| const agentTempDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-harness-tests-")); | ||
| fs.mkdirSync("/tmp/gh-aw", { recursive: true }); |
There was a problem hiding this comment.
[/tdd] Nit: this runs at module load and creates /tmp/gh-aw on every run of this file, and the path is hardcoded. Prefer the TMP_GH_AW_PATH constant from constants.cjs, and create the directory inside the helper, as makeHarnessTempDir already does.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Critical path-validation and environment-override flaws allow configuration writes or loads outside /tmp/gh-aw, bypassing managed-directory confinement.
5 open issues
What changed in this PR
This PR adds managed-directory checks to gh-aw’s Codex configuration flow in response to #70966 and removes runner-specific data from a parser fixture.
Changes:
- Normalize
CODEX_HOMEoverrides and fall back to the managed default for out-of-directory paths. - Add containment and regular-file checks before provider-config rewrites, with regression tests.
- Replace checkout-specific parser fixture data with generic paths and filenames.
| File | Description |
|---|---|
pkg/workflow/codex_config.go |
Filters custom Codex homes against the managed directory. |
pkg/workflow/codex_config_test.go |
Tests home filtering and relocates direct-tool fixtures under /tmp/gh-aw. |
actions/setup/js/codex_harness.test.cjs |
Adds outside-path and symlink rejection tests using managed test directories. |
actions/setup/js/codex_harness.cjs |
Checks configuration paths before provider endpoint rewrites. |
actions/setup/js/bash_command_parser.test.cjs |
Replaces runner-specific fixture content with generic test data. |
Review effort: Max (auto)
| function isSafeCodexConfigPath(codexConfigPath) { | ||
| try { | ||
| const root = fs.realpathSync(TMP_GH_AW_PATH); | ||
| const resolvedPath = path.resolve(codexConfigPath); |
| } | ||
|
|
||
| func codexHome(workflowData *WorkflowData) string { | ||
| if home := getEngineEnvOverrides(workflowData)["CODEX_HOME"]; home != "" { |
| if cleanHome == constants.TmpGhAwDir || strings.HasPrefix(cleanHome, constants.TmpGhAwDir+"/") { | ||
| return cleanHome |
| return home | ||
| cleanHome := path.Clean(home) | ||
| if cleanHome == constants.TmpGhAwDir || strings.HasPrefix(cleanHome, constants.TmpGhAwDir+"/") { | ||
| return cleanHome |
| try { | ||
| const root = fs.realpathSync(TMP_GH_AW_PATH); | ||
| const resolvedPath = path.resolve(codexConfigPath); | ||
| const relativePath = path.relative(root, resolvedPath); |
|
@copilot run pr-finisher skill |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 1cb2405
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 1cb2405
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: d065814
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: d065814
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: d065814
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: d065814
|
|
🎉 This pull request is included in a new release. Release: |
Red-team findings flagged a Codex config write whose path could be influenced by
engine.env.CODEX_HOME, and a parser test fixture containing runner-specific paths. The admission and Claude matches did not reveal dynamic code execution or obfuscated content.CODEX_HOMEonly within/tmp/gh-aw; otherwise use/tmp/gh-aw/mcp-config. Before rewriting, requireconfig.tomlto be a regular, non-symlink file under the managed directory.