fix(#5832): strip OIDC credentials from user-controlled environments

[fullsend-ai-coder]

Summary

Strip OIDC credential env vars from all user-controlled and sandbox-visible contexts to prevent user-authored scripts and LLM sessions from minting their own tokens. The parent harness process retains credentials for mintAgentToken and OIDC token refresh.

Changes

• Add oidcDenyKeys denylist for 4 OIDC env vars (ACTIONS_ID_TOKEN_REQUEST_URL, ACTIONS_ID_TOKEN_REQUEST_TOKEN, FULLSEND_GCP_OIDC_URL, FULLSEND_GCP_OIDC_AUTH_FILE)
• childScriptEnv: strip OIDC vars from pre/post script environments
• shellSafeExpandEnv: refuse OIDC vars in host_files ${VAR} expansion
• reservedSandboxKeys: block OIDC vars from env.sandbox injection
• Harness expander/lookup closures: refuse OIDC vars in runner_env/env expansion
• Preflight check: use childScriptEnv instead of raw os.Environ()
• Validation scripts: use new scrubOIDCEnv helper to filter os.Environ()

Testing

• Unit tests for childScriptEnv stripping OIDC vars (from process env and RunnerEnv)
• Unit test for shellSafeExpandEnv refusing OIDC vars
• Unit test for reservedSandboxKeys including OIDC vars
• Unit test for buildSandboxEnvLines rejecting OIDC vars
• Unit test for scrubOIDCEnv helper
• Unit test for oidcDenyKeys completeness
• All existing childScriptEnv, shellSafeExpandEnv, and buildSandboxEnvLines tests pass
• go vet ./internal/cli/... passes
• Secret scan passes

---

Closes #5832

Post-script verification

• Branch is not main/master (agent/5832-block-mint-oidc-leak)
• Secret scan passed (gitleaks — 93bd394a743d4db23c088b016423657c18f85058..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:12 PM UTC · Completed 12:28 PM UTC
Commit: aa44151 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [behavioral-inconsistency] internal/cli/run.go:530 — The expander closure returns "" for OIDC keys (silent empty expansion), while the lookup closure returns ("", false) (causes validation to fail). The different semantics are intentional and well-documented — lookup returning false causes ValidateRunnerEnvWith to treat the reference as unresolvable and fail-closed, which is the safer behavior. Minor maintainability concern only.

• [deny-list-completeness] internal/cli/run.go:1899 — The oidcDenyKeys list remains a static set of four known OIDC credential variables. This deny list would not automatically catch new OIDC-related environment variables introduced by future mint infrastructure changes. A prefix-based approach (stripping all vars matching ACTIONS_ID_TOKEN_* or FULLSEND_GCP_OIDC_*) would be more resilient. The code includes a MAINTENANCE comment documenting the convention and noting that a single addition propagates to all deny checks.

Previous run

Review

Findings

Medium

• [incomplete-credential-stripping] internal/cli/run.go:2059 — The host_files src path expansion uses os.ExpandEnv(hf.Src), which bypasses the oidcDenyKeys deny check applied at all other expansion sites in this PR. If a harness YAML declared host_files with src: '${FULLSEND_GCP_OIDC_AUTH_FILE}', the path would resolve to the actual OIDC auth file location and its contents could be uploaded into the sandbox. While host_files configuration is authored by trusted harness maintainers (limiting practical exploitability), this is an inconsistency in the defense-in-depth strategy — every other ${VAR} expansion path now checks oidcDenyKeys.
  Remediation: Replace os.ExpandEnv(hf.Src) with os.Expand(hf.Src, expander) using the same oidcDenyKeys-aware expander closure, or create a standalone wrapper function for reuse across all expansion sites.

Low

• [behavioral-inconsistency] internal/cli/run.go:530 — The expander closure returns "" for OIDC keys (silent empty expansion), while the lookup closure returns ("", false) (causes validation to fail). The different semantics are intentional and well-documented — lookup returning false causes ValidateRunnerEnvWith to treat the reference as unresolvable and fail-closed, which is the safer behavior. Minor maintainability concern only.

• [deny-list-completeness] internal/cli/run.go:1899 — The oidcDenyKeys list covers the four known OIDC credential variables. The deny list is static and would not catch new OIDC-related environment variables added in future mint infrastructure changes. Consider a prefix-based approach (e.g., ACTIONS_ID_TOKEN_*, FULLSEND_GCP_OIDC_*) or at minimum a doc comment noting that new credential variables must be added here.

Previous run (2)

Review

Findings

Medium

• [incomplete-scrubbing-path] internal/cli/run.go:1623 — The validation script env is composed as append(scrubOIDCEnv(os.Environ()), validationEnv(h, ..., ...)...). The validationEnv function calls envToList(h.RunnerEnv) which appends RunnerEnv entries after scrubbing. If h.RunnerEnv contained an OIDC key (e.g., a harness YAML declaring runner_env: { ACTIONS_ID_TOKEN_REQUEST_URL: "literal-value" }), the key would survive scrubbing and appear in the validation script's environment. In contrast, childScriptEnv strips OIDC keys from the merged env (os.Environ + RunnerEnv). The same pattern applies at the post-loop sweep (line 2193). Practical risk is low — exploitation requires a deliberately malicious harness YAML — but this is a defense-in-depth gap worth closing.
  Remediation: Apply scrubOIDCEnv to the full composed slice, or switch to childScriptEnv for validation scripts.

• [duplication-dry] internal/cli/run.go — The four OIDC key strings are duplicated between oidcDenyKeys and reservedSandboxKeys with no programmatic link. A future addition to oidcDenyKeys could silently miss the sandbox path.
  Remediation: Add OIDC keys to reservedSandboxKeys programmatically via init() or a shared slice constant.

Low

• [behavioral-inconsistency] internal/cli/run.go:530 — The expander closure returns "" for OIDC keys (silent empty expansion), while the lookup closure returns ("", false) (causes validation to fail). The different semantics are intentional — fail-closed validation is the safer choice — but the divergence could trip a future maintainer.

• [naming-consistency] internal/cli/run.go — The function is named scrubOIDCEnv but inline comments and the PR title use "strip." Picking one verb consistently would aid discoverability.

---

Labels: PR is a security hardening fix affecting the harness, sandbox, and mint components — matches the linked issue's component labels

[ifireball]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 12:58 PM UTC · Completed 1:06 PM UTC
Commit: aa44151 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 4 review findings: closed the incomplete scrubbing path for validation envs, eliminated the DRY violation between oidcDenyKeys and reservedSandboxKeys via init(), documented the intentional expander/lookup behavioral divergence, and renamed scrubOIDCEnv to stripOIDCEnv for naming consistency.

Fixed (4):

1. incomplete-scrubbing-path: validation script env composed with scrubOIDCEnv only on os.Environ(), leaving RunnerEnv OIDC keys unscrubbed (internal/cli/run.go): Moved stripOIDCEnv to wrap the full composed slice (os.Environ + validationEnv) at both the inline validation (line 1627) and post-loop sweep (line 2193) call sites, so OIDC keys injected via h.RunnerEnv are also stripped.
2. duplication-dry: OIDC key strings duplicated between oidcDenyKeys and reservedSandboxKeys with no programmatic link (internal/cli/run.go): Replaced hardcoded OIDC entries in reservedSandboxKeys with an init() function that merges keys from oidcDenyKeys. Future additions to oidcDenyKeys automatically propagate to sandbox injection blocking.
3. behavioral-inconsistency: expander returns empty string for OIDC keys while lookup returns (empty, false) with no documentation of the intentional divergence (internal/cli/run.go): Added a 3-line comment to the lookup closure explaining that it intentionally returns false (unlike expander's silent empty expansion) so ValidateRunnerEnvWith treats the reference as unresolvable and fails validation.
4. naming-consistency: function named scrubOIDCEnv but comments and PR title use strip (internal/cli/run.go): Renamed scrubOIDCEnv to stripOIDCEnv and updated all call sites, doc comments, and test names to use strip consistently.

Tests: passed

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:07 PM UTC · Completed 1:22 PM UTC
Commit: 846df0f · View workflow run →

[ifireball]

/fs-fix the following:

• internal/cli/run.go (file-level): Line 2059 · [medium] incomplete-credential-stripping

  The host_files src path expansion uses os.ExpandEnv(hf.Src), which bypasses the oidcDenyKeys deny check applied at all other expansion sites. If a harness YAML declared host_files with src: '${FULLSEND_GCP_OIDC_AUTH_FILE}', the path would resolve to the actual OIDC auth file location and its contents could be uploaded into the sandbox. While host_files configuration is authored by trusted harness maintainers, this is an inconsistency in the defense-in-depth strategy.

  Suggested fix: Replace os.ExpandEnv(hf.Src) with os.Expand(hf.Src, expander) using the same oidcDenyKeys-aware expander closure, or create a standalone wrapper function for reuse across all expansion sites.

• internal/cli/run.go:530: [low] behavioral-inconsistency

  The expander closure returns '' for OIDC keys (silent empty expansion), while the lookup closure returns ('', false) (causes validation to fail). The different semantics are intentional and well-documented. Minor maintainability concern only.

• internal/cli/run.go:1899: [low] deny-list-completeness

  The oidcDenyKeys list covers four known OIDC credential variables. The deny list is static and would not catch new OIDC-related environment variables added in future mint infrastructure changes.

  Suggested fix: Consider adding a doc comment noting that new credential variables must be added to oidcDenyKeys, or switch to prefix matching for ACTIONS_ID_TOKEN_ and FULLSEND_GCP_OIDC_ prefixes.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 10:04 AM UTC · Completed 10:13 AM UTC
Commit: 846df0f · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Addressed 2 of 3 review findings. Fixed the host_files src expansion gap by creating a safeExpandEnv helper that consults oidcDenyKeys. Added maintenance documentation to oidcDenyKeys noting prefix conventions. Disagreed with finding 2 (behavioral-inconsistency) as the different semantics are intentional and well-documented.

Fixed (2):

1. incomplete-credential-stripping in host_files src expansion (internal/cli/run.go): Replaced os.ExpandEnv(hf.Src) with new safeExpandEnv(hf.Src) helper that consults oidcDenyKeys, closing the last expansion site that bypassed the OIDC credential deny check. Added test TestSafeExpandEnv_RefusesOIDCVars to verify the fix.
2. deny-list-completeness documentation for oidcDenyKeys (internal/cli/run.go): Added MAINTENANCE doc comment to oidcDenyKeys noting that new OIDC credential vars must be added to the map, documenting the ACTIONS_ID_TOKEN_ and FULLSEND_GCP_OIDC_ prefix conventions, and clarifying that all expansion sites consult this single map.

Disagreed (1):

1. behavioral-inconsistency between expander and lookup closures: The different semantics (expander returns empty string for silent expansion vs lookup returns false to fail validation) are intentional and already well-documented in the code comments. The reviewer themselves noted this is a 'Minor maintainability concern only' with no actionable change needed.

Tests: passed

Decision points

• Created standalone safeExpandEnv helper rather than inlining the deny check (alternatives: Inline os.Expand with oidcDenyKeys check at the call site; rationale: A standalone function is reusable at any future expansion site and matches the pattern of shellSafeExpandEnv. The function is placed adjacent to shellSafeExpandEnv for discoverability.)
• Used doc comment for deny-list maintenance guidance rather than prefix matching (alternatives: Switch to prefix-based matching for ACTIONS_ID_TOKEN_ and FULLSEND_GCP_OIDC_; rationale: Prefix matching would change the security boundary from explicit enumeration to pattern-based, which is a larger scope change than what the review requested. A doc comment is the minimal, safe improvement.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:14 AM UTC · Completed 10:31 AM UTC
Commit: 79017d8 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:53 AM UTC · Completed 12:09 PM UTC
Commit: 79017d8 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5837 — strip OIDC credentials from user-controlled environments

Timeline

1. Aug 2, 12:11 — Code agent opened PR with initial implementation (commit aa44151)
2. Aug 2, 12:28 — Review agent posted 2 medium + 2 low findings (all true positives)
3. Aug 2, 12:57 — Human (ifireball) triggered /fs-fix
4. Aug 2, 13:06 — Fix agent addressed all 4 findings (commit 846df0f)
5. Aug 2, 13:22 — Re-review found 1 new medium + 2 low findings
6. Aug 3, 10:03 — Human triggered second /fs-fix (~21h later)
7. Aug 3, 10:13 — Fix agent addressed 2 of 3, reasonably disagreed with 1 (commit 79017d8)
8. Aug 3, 10:31 — Review agent approved (only 2 low findings remaining)
9. Aug 3, 11:02 — Human approved; merged at 11:50

Workflow quality

Review quality was excellent. The review agent produced 9/9 true positives and 0 false positives across 3 iterations. Its two most impactful findings were genuine security gaps: (1) OIDC keys from RunnerEnv surviving stripping due to env composition order (scrubOIDCEnv applied before validationEnv was appended), and (2) os.ExpandEnv(hf.Src) in host_files processing bypassing the oidcDenyKeys deny-list entirely. Both were real defense-in-depth gaps in a credential-isolation PR.

Fix agent quality was high. Both fix commits were well-scoped, correctly targeted, and included tests. The fix agent's disagreement with the behavioral-inconsistency finding was sound — the expander and lookup closures serve different purposes and the divergence was intentional.

Token waste: minimal. Concurrency deduplication worked correctly. One early review run was cancelled by cancel-in-progress within 6 seconds. The 5 review runs visible in the dispatch repo at commit 3 time were for 5 different PRs, not duplicates. Source-repo dispatch triggers that didn't match any stage resolved in ~3 seconds.

Functional test flake: Test 002-needs-info-vague-crash failed on commit 2 (run 30749220000) with a 1-second crash ($0.00 cost), passed on commits 1 and 3. This was an infrastructure-level failure (instant crash during execution phase, not scoring) unrelated to the OIDC changes. Single data point — not filing a proposal, but noting it here.

Evidence for existing issues (not filing new proposals)

• Code agent: analyze all usage sites before implementing, not just the one in the issue #1214 (code agent: analyze all usage sites): The code agent missed the os.ExpandEnv(hf.Src) call site and the env composition order gap in a security hardening PR. The review agent caught both. This is another instance of the code agent not exhaustively grepping for all sites of a function/pattern it's trying to protect against.
• Auto-dispatch fix agent on critical review findings for bot-authored PRs #2596 / Review agent should suggest /fs-fix for auto-fixable low-severity findings to prevent manual rework loops #5001 (auto-dispatch fix on review findings): The human reviewer's sole contributions were two /fs-fix triggers — no independent findings, no design feedback. The ~21-hour gap between the first fix and the second /fs-fix trigger was pure latency from waiting for human intermediation. Auto-dispatching the fix agent on medium-severity review findings would have eliminated this delay.
• Review agent inline comments anchor to wrong line numbers agents#575 (inline comment line anchoring): Review 2 had inline comments that received 422 errors from the GitHub API, falling back to issue-level comments. Likely caused by the comments targeting lines outside the diff hunk.

No new proposals

All candidate improvements identified in this retro are already tracked by existing open issues. The workflow performed well — the review agent caught real security gaps, the fix agent resolved them correctly, and the pipeline converged in 3 iterations with no wasted work.