fix(security): mirror credential shapes into the PostToolUse hook, scoped to checkout content - #7009
Conversation
The Go redactor and the sandbox hook guard different boundaries — span content and the model's context window — but had drifted: the hook's ya29 pattern missed service-account tokens (one-char c segment defeats the length floor) and it had no bare-JWT shape at all, while its combined gh*_ pattern stopped at the first dot of the 2026 JWT-wrapped installation-token format, leaving payload and signature in the clear. Mirror the three patterns from the Go side (google_oauth_token with the c. alternative, jwt, github_server_token with dots in the class) so both inventories carry the same shapes. Raised by waynesun09's review; the ghs_ gap surfaced during pre-push verification of the mirror. Signed-off-by: Dharit Shah <dhshah@redhat.com>
The PostToolUse chain runs on every tool, and the bare-JWT prefix pattern a3fd5c0 mirrored into it has no fixture-shaped escape: a jwt.io example in a test file is byte-for-byte a valid token, so a Read of one came back as `eyJh...` and the agent edited against text that is not on disk — the failure mode 63a0a7d scoped the structural patterns to avoid. content_skips(hook_input) returns the pattern names redact_text skips: `jwt` when a file-content tool (Read, Grep, Edit, MultiEdit, Write, NotebookEdit, NotebookRead) is called with a path inside the checkout — the nearest `.git` ancestor of the hook input's `cwd`, which follows the agent's persisted `cd`; none means no skip. The path is normalized, then resolved. `..` segments, `~` paths, a non-string tool_name, a missing tool_input or no `cwd` (pi's adapter sends none) mean mask, not skip, so the runner's OIDC token file beside the checkout still masks on Read and Grep, and Bash, WebFetch and MCP output are unchanged. The chain passes the set on the success and failure paths; the standalone script does the same. Same sweep as 63a0a7d, here through redact_text: golang-jwt/jwt as Read goes from 15 rewritten files (48 bare-JWT hits in 9) to 8, all structural or private-key; this repo and fullsend-ai/agents have no tracked file matching the pattern (0/1154, 0/343). Raised by waynesun09's review. Signed-off-by: Dharit Shah <dhshah@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoHarden PostToolUse redaction with checkout-aware JWT handling
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Site previewPreview: https://95db0718-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…end cwd from pi Two review findings on the checkout-scoped bare-JWT skip. The checkout root was the nearest .git ancestor of the resolved cwd, wherever that walk led: a .git an agent plants at /sandbox/workspace, reached through a symlink inside the checkout, made the runner's sibling OIDC token file 'inside' and skipped the mask. The root is now searched only strictly below SANDBOX_WORKSPACE (/sandbox/workspace, read once at process start from FULLSEND_SANDBOX_WORKSPACE; tests set it), so a .git at or above the workspace, or a cwd outside it, yields no root and no skip. The claude and pi launch commands re-export the runtime's value after sourcing the agent-writable .env, and the key is reserved from harness env.sandbox, so an assignment in .env does not move the boundary either; a .env that redefines the claude command is the pre-existing launch shim seam. Residual, stated in the hook: a .git planted in another runner-owned workspace subdirectory (.env.d) can still be a root through the same symlink, for a bare JWT no structural shape covers. pi's adapter sent no cwd, so under pi every file-content Read got no skip and the newly mirrored JWT pattern rewrote checkout fixtures — the edit-corruption the skip exists to prevent, and not prior behaviour, since the pattern is new. The adapter now sends its process working directory: pi is started in the checkout and its tools run in child shells, so the agent cannot move it. codex still sends no cwd; it cannot apply the rewrite at all, so nothing is corrupted there, only warned about. Tests pin a planted .git at and above the workspace (direct and through a symlinked cwd), a checkout outside the workspace, the re-exported boundary after .env on both launch commands, and cwd on every pi PostToolUse payload; the contract doc and the adapter header say the same. Raised by Qodo's review of fullsend-ai#7009. Signed-off-by: Dharit Shah <dhshah@redhat.com>
…end cwd from pi Two review findings on the checkout-scoped bare-JWT skip, and a third gap the second fix exposed. The checkout root was the nearest .git ancestor of the resolved cwd, wherever that walk led: a .git an agent plants at /sandbox/workspace, reached through a symlink inside the checkout, made the runner's sibling OIDC token file 'inside' and skipped the mask. The root is now searched only strictly below SANDBOX_WORKSPACE, so a .git at or above the workspace, or a cwd outside it, yields no root and no skip. The boundary is the runner's constant; the hook takes an override only from its own command line, the seam the subprocess tests use, which the runner-owned hooks.json and the pi adapter build without it. It is not read from environment variables: under fullsend's plain launch, Claude Code (observed locally on 2.1.235; the sandbox image pins 2.1.260) applies a checkout's .claude/settings.json env block to hook processes over the launch environment, so an env-carried boundary could be moved by a committed settings file, and the sandbox .env every launch command sources lives in the sandbox user's workspace. Not covered, and pre-existing: under claude and pi the hook interpreter is a bare python3 resolved through a PATH with the agent-writable workspace bin first, with PYTHONPATH and the user site directory honoured (codex isolates its adapter); left for a follow-up. Residual, stated in the hook: a .git planted in another runner-owned workspace subdirectory (.env.d) can still be a root through a plain cd or a symlink, for a bare JWT no structural shape covers, and only for files under that subdirectory. pi's adapter sent no cwd, so under pi every file-content Read got no skip and the newly mirrored JWT pattern rewrote checkout fixtures — the edit-corruption the skip exists to prevent, and not prior behaviour, since the pattern is new. The adapter now sends its process working directory: pi is started in the checkout and its tools run in child shells, so the agent cannot move it. codex still sends no cwd; it cannot apply the rewrite at all, so nothing is corrupted there, only warned about. Sending cwd exposed a gap of its own under pi: pi strips a leading '@', expands '~' and turns a file:// URL into a path before opening, while the adapter forwards the raw argument, so a read of '@../.gcp-oidc-token' resolved under the checkout in the hook and the token came back unmasked. Those forms now never skip, for every file-content tool; any URL scheme is refused, broader than pi's rewrite on purpose. Tests pin a planted .git at and above the workspace (the at-workspace case direct and through a symlinked cwd), a checkout outside the workspace, the rewritten path forms for Read and Grep, a symlinked workspace path, the boundary ignoring the environment (standalone and through the chain) and honouring only an absolute command-line value (standalone), and cwd on every pi PostToolUse payload across events; the contract doc and the adapter header say the same. Raised by Qodo's review of fullsend-ai#7009. Signed-off-by: Dharit Shah <dhshah@redhat.com>
096d375 to
d571888
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep — 1 finding could not be attached inline because the file it concerns is outside this PR's diff.
MEDIUM — Codex adapter never forwards cwd, so Codex never gets the checkout-scoped JWT skip this PR just fixed for pi
internal/runtime/codex_hook/fullsend-codex-hook.py:404 (run_post_tool_use, and run_pre_tool_use similarly)
run_post_tool_use() builds the payload sent to hook scripts from only tool_name/tool_input/tool_response/tool_result — cwd is never forwarded, even though Codex's own hook input carries it (see fixtures in internal/runtime/codex_hook_test.go:94). Codex's apply_patch tool maps to Claude's Edit (CLAUDE_TOOL_FOR_CODEX), and Edit is one of the tools content_skips keys on (_TOOL_PATH_KEY). Because cwd is always missing, content_skips always returns no skip for Codex, so a Codex edit of a checkout file containing a jwt.io-style fixture is masked and rewritten to something different from what's on disk — the exact edit-corruption failure mode this PR's checkout-scoped skip exists to prevent, and that d5718882 just fixed for pi (see the "Pi corrupts jwt fixtures" thread on this PR). The same gap is unfixed on Codex.
This file isn't touched by this PR's diff, so it's a pre-existing parity gap rather than a regression — but the PR's new docs/contributing/runtime-implementation.md contract line generically covers it ("an adapter that omits it gets no bare-JWT skip"), so it's now a documented, unaddressed hole.
Suggestion: forward hook_input['cwd'] into the payload built in run_post_tool_use (mirroring what Claude Code and the pi adapter now send), so Codex Edit/Read/Grep calls get the same checkout-scoped skip and don't corrupt JWT-shaped fixtures the agent then edits against.
| # host_files credentials) can still be a root through a plain cd into it or an | ||
| # in-checkout symlink; only a bare JWT no structural shape covers would skip | ||
| # there, and only for files under that subdirectory. | ||
| SANDBOX_WORKSPACE: str = "/sandbox/workspace" |
There was a problem hiding this comment.
MEDIUM — No cross-check ties the Python hook's hardcoded SANDBOX_WORKSPACE to internal/sandbox.SandboxWorkspace
secret_redact_posttool.py hardcodes SANDBOX_WORKSPACE: str = "/sandbox/workspace" as a Python literal, documented as mirroring internal/sandbox.SandboxWorkspace (verified equal today, internal/sandbox/sandbox.go:29, pinned by internal/sandbox/sandbox_test.go:33). The checkout-scoped bare-JWT skip's entire security boundary (introduced by this PR's d5718882 fix, replacing the walk that let a forged .git widen the trusted root to the sibling OIDC token file) now depends on this literal matching the Go constant.
Verified no test enforces that: internal/security/hooks_test.go:175 only asserts SecretRedactPostToolHook is non-empty, and hooks.go (which already imports the sandbox package for SandboxHooksDir/SandboxHooksSettings) has no assertion against the embedded Python source. If SandboxWorkspace is ever renamed or changed in Go, this file's literal silently goes stale with no CI failure, and the checkout-boundary control mis-scopes.
Suggestion: add one line to hooks_test.go: assert.Contains(t, string(SecretRedactPostToolHook), \SANDBOX_WORKSPACE: str = "`+sandbox.SandboxWorkspace+`"`)` so a future rename of the Go constant fails CI instead of drifting silently.
2 participants
Split out of #6603 at review request (waynesun09): this changes what the agent's PostToolUse hook does on every file-content tool call, which is a sandbox-side hardening change rather than a telemetry one, and it deserves its own review of the parity argument.
What this does
internal/security/hooks/secret_redact_posttool.py)google_oauth_token(ya29., with the literalc.alternative for service-account tokens) and a bare three-segmentjwt— plusgithub_server_token(ghs_…with dots, ahead of the combinedgh*_pattern, so a JWT-wrapped installation token masks whole instead of stopping at the first dot). Both boundaries — span content and the model's context window — carry the same shapes.content_skips)Readcorrupts what the agent edits against (the failure mode 63a0a7d scoped the structural patterns to avoid). So thejwtpattern is skipped when a file-content tool (Read, Grep, Edit, MultiEdit, Write, NotebookEdit, NotebookRead) is called with a path inside the checkout — the nearest.gitancestor of the hook'scwd, searched only strictly below the sandbox workspace so a.gitan agent plants at or above it can never widen the root to the runner's files; none meaning no skip; the path is normalized, then resolved;..segments,~paths,@-prefixed paths and URL forms (pi strips the@, expands~and convertsfile://before opening while its adapter forwards the raw argument; any scheme is refused), malformed input and nocwdall mean mask. Claude Code sends its working directory; the pi adapter sends its process working directory — the checkout, which child shells cannot move. The runner's own OIDC token file sits beside the checkout, not in it, and still masks on Read or Grep. Bash, WebFetch and MCP output are unaffected.internal/security/hooks/posttool_chain.py, tests)/sandbox/workspace. The hook takes an override only from its own command line (--sandbox-workspace=PATH, the seam the subprocess tests use), which the runner-ownedhooks.jsonand the pi adapter build without it. It is not read from environment variables: under fullsend's plain launch, Claude Code (observed locally on 2.1.235; the sandbox image pins 2.1.260) applies a checkout's.claude/settings.jsonenv block to hook processes over the launch environment, so an env-carried boundary could be moved by a committed settings file. Not covered, and pre-existing: under claude and pi the hook interpreter is a barepython3resolved through a PATH with the agent-writable workspacebinfirst, withPYTHONPATHand the user site directory honoured (codex isolates its adapter); drafted as a follow-up.docs/contributing/runtime-implementation.md: the wire protocol gainscwd(Claude Code's working directory; pi's process working directory), the sanitizer-scope paragraph states the root is searched strictly below/sandbox/workspaceand how the boundary is pinned, and the adapter-contract line says pi sendscwd; the pi adapter's header comment says the same.Known residual: the boundary is the workspace, not the checkout, so a
.gitplanted in another runner-owned subdirectory (.env.d, which holdshost_filescredentials) can still become a root through a plaincdinto it or an in-checkout symlink; only a bare JWT that no structural shape covers would skip there. Pinning the checkout path the runner already knows would remove the.gitwalk entirely.Measured
redact_text): golang-jwt/jwt asReadgoes from 15 rewritten files (48 bare-JWT hits in 9) to 8, all structural or private-key — the class that sweep accepted; this repo and fullsend-ai/agents have no tracked file matching the jwt pattern (0/1154, 0/343). Structural masking of token-named assignments is unchanged; the skip covers the context-free pattern only.Tests
TDD: 26 new hook tests (chain-level Read-untouched / Bash-masked / failure-path flagged; checkout vs outside — token file,
.., symlink, symlink then.., Grep root,cdbelow the root through a symlink, a.git-less directory; a.gitplanted at the workspace reached through a symlinkedcdor directly, a.gitplanted above the workspace, a checkout outside the workspace; the rewritten path forms@…andfile://…for Read and Grep; a symlinked workspace path; malformed input; the skip is jwt-only), a node test that every pi PostToolUse payload carriescwd, across events on one instance, and the boundary ignoring the environment (standalone and through the chain) and honouring only an absolute command-line value (standalone). Every policy branch killed by a named mutant. Full hook suite green.Raised in review of #6603 (rounds 4 and 6 by waynesun09); #6603 keeps the Go redactor patterns, which its span content needs.