fix(#6920): withhold unsafe codex tool results - #6952
Conversation
PR Summary by QodoWithhold unsafe Codex tool results
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1. context_suppressed can leak secrets
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if benign_rewrite(verdict["output"]): | ||
| continue |
There was a problem hiding this comment.
1. context_suppressed can leak secrets 📘 Rule violation ⛨ Security
Context suppression runs before secret redaction, so successful test output containing a secret can be summarized before the redactor scans it. The adapter then treats the rewrite as benign and allows Codex to receive the original unredacted output, contradicting the documented runtime security behavior.
Agent Prompt
## Issue description
Codex can receive an original tool result containing secrets when context suppression removes the secret before the redaction stage scans the rewritten value.
## Issue Context
The post-tool chain performs context suppression before secret redaction. Because Codex cannot apply the summary rewrite, classifying `context_suppressed` as benign is safe only after the original result has independently passed security scanning.
## Fix Focus Areas
- internal/runtime/codex_hook/fullsend-codex-hook.py[463-480]
- internal/security/hooks/posttool_chain.py[376-433]
- internal/runtime/codex_hook_test.go[368-386]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return True | ||
| if not all(isinstance(category, str) for category in categories): | ||
| return True | ||
| return any(category not in {"ansi_escape", "osc_escape"} for category in categories) |
There was a problem hiding this comment.
2. Benign unicode rewrites blocked 🐞 Bug ≡ Correctness
security_rewrite treats every category except ANSI/OSC as removed security content, but fullwidth is detection-only and deliberately remains unchanged. A context-suppression or ordinary terminal-cleanup rewrite containing a fullwidth character is therefore incorrectly blocked and withheld from Codex.
Agent Prompt
## Issue description
The Codex adapter classifies all Unicode metadata categories as rewrite causes, although `fullwidth` is only detected and retained. Consequently, a benign context-suppression or direct terminal-escape rewrite can be withheld merely because its input also contained a compatibility character.
## Issue Context
The distinction must preserve blocking for escapes reconstructed through NFKC while allowing independent ANSI/OSC cleanup and context suppression. The existing category list cannot distinguish a directly removed escape from an NFKC-reassembled escape.
## Fix Focus Areas
- internal/runtime/codex_hook/fullsend-codex-hook.py[377-412]
- internal/security/hooks/posttool_chain.py[314-353]
- internal/security/hooks/unicode_posttool.py[169-211]
- internal/runtime/codex_hook_test.go[285-386]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
32528d3 to
6722d79
Compare
Site previewPreview: https://f179a026-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · Commit: |
6722d79 to
b495cf5
Compare
|
🤖 Review · Commit: |
b495cf5 to
1e7ac66
Compare
|
🤖 Finished Review · ✅ Success · Started 10:32 AM UTC · Completed 11:11 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.32 |
|
Risk Assessment: moderate (2/5) DetailsMedium-sized follow-up fix to the codex runtime hook with moderate test coverage ratio; high fix/revert churn and change coupling in Tier 2 are counterbalanced by no protected or security-sensitive paths, no dependency changes, and an experienced member author, yielding a moderate overall risk. |
ReviewFindingsMedium
Low
Labels: PR modifies security hook behavior in the codex runtime adapter and fixes a category naming issue in the shared unicode posttool hook Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| "fullsend: the previous tool output contained security-sensitive content " | ||
| "that codex cannot safely rewrite; the result was withheld" | ||
| ) | ||
| if context_was_suppressed(verdict["output"]): |
There was a problem hiding this comment.
[medium] edge-case
The context-suppression rescan path (lines 491-524) does not return or continue after the rescan passes cleanly. Control falls through to benign_rewrite(verdict["output"]) at line 525, which must also pass. A context-suppression-only rewrite always satisfies benign_rewrite today, but the coupling is implicit: if a future chain change adds a new metadata key alongside context_suppressed, benign_rewrite would reject the unknown key and block even though the rescan proved the content safe. This is documented as intentional fail-closed behavior.
| return bool(metadata.get("unicode_findings")) and bool(categories) and safe_categories | ||
|
|
||
|
|
||
| def context_was_suppressed(output: Any) -> bool: |
There was a problem hiding this comment.
[low] pattern-inconsistency
Three new helper functions (context_was_suppressed, hook_is_installed, scan_reported_error) lack docstrings, while every other function in this file has one.
Suggested fix: Add a single-line docstring to each of the three functions.
|
|
||
| input := codexBashInput("go test ./...") | ||
| input["hook_event_name"] = "PostToolUse" | ||
| input["tool_response"] = "many lines of successful test output" |
There was a problem hiding this comment.
[low] test-weakened
TestCodexAdapter_PostToolUseIgnoresContextSuppression assigns input["tool_response"] twice in succession. The first assignment is immediately overwritten and is dead code.
Suggested fix: Remove the first assignment so only the intended value remains.
| return digests is not None and script in digests | ||
|
|
||
|
|
||
| def scan_reported_error(output: Any) -> bool: |
There was a problem hiding this comment.
[low] naming-convention
scan_reported_error returns a bool but reads as a verb rather than following the predicate pattern used by the other new boolean helpers (security_rewrite, benign_rewrite, context_was_suppressed, hook_is_installed).
| assert.NotContains(t, got.stderr, "sk-live-abcdef", "the flagged value must not be echoed back") | ||
| } | ||
|
|
||
| func TestCodexAdapter_PostToolUseWithholdsSecretWithTheRealChain(t *testing.T) { |
There was a problem hiding this comment.
[low] pattern-inconsistency
TestCodexAdapter_PostToolUseWithholdsSecretWithTheRealChain manually inlines the same six-entry map that installPostToolChain() encapsulates. Other new tests correctly use h.installPostToolChain().
Suggested fix: Replace the inline map with h.installPostToolChain().
fullsend-ai-review
Bot
added
the
security
Assisted-by: Codex (fix), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
1e7ac66 to
570cd7f
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 11:14 AM UTC · Completed 11:56 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
1 participant
Summary
Test plan
make lintgo test ./internal/security -count=1go test ./internal/runtime -run '^TestCodexAdapter' -count=1make go-test(baseline/environment failures remain in harnessdispatch, dummy runtime, and sandbox tests; changed-path suites pass)