fix(security): stop delayed and additional tool callbacks after cancellation - #2430
fix(security): stop delayed and additional tool callbacks after cancellation#2430HAYDEN-OAI wants to merge 1 commit 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. |
Castiron custom code✅ No new custom-code files detected. 34 mixed files remain; 0 existing customizations changed. Compared 34 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 32327620158 --repo openai/openai-node \
--name castiron-custom-code-32327620158-1 --dir /tmp/castiron-custom-code-32327620158-1
git apply --stat /tmp/castiron-custom-code-32327620158-1/custom-code.patch
cat /tmp/castiron-custom-code-32327620158-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin a0d68cc53125c2cb82eab31271b8984b8d65d4b2 20219f5f6136a804a776a49a29ce8e31092092a6
python3 scripts/castiron/custom_code_report.py report \
--base a0d68cc53125c2cb82eab31271b8984b8d65d4b2 \
--head 20219f5f6136a804a776a49a29ce8e31092092a6 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-20219f5f6136
cat /tmp/castiron-custom-code-20219f5f6136/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
![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: 20219f5f61
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.controller.signal.aborted) { | ||
| throw new APIUserAbortError(); |
There was a problem hiding this comment.
When an asynchronous fn.parse() rejects after the runner is aborted, the catch returns ordinary tool feedback before reaching this abort check. With maxChatCompletions: 1, the loop then exhausts normally (and can invoke afterCompletion), so runner.done() resolves and runner.aborted remains false despite cancellation. Check the signal in the rejection path before converting the parser error into a tool message.
AGENTS.md reference: AGENTS.md:L109-L113
Useful? React with 👍 / 👎.
| // A completed buffered turn retains its first immediate callback for | ||
| // compatibility; delayed parsed callbacks never inherit this exception. | ||
| allowBufferedToolCall = this.controller.signal.aborted; |
There was a problem hiding this comment.
If a supported custom fetch transport ignores its signal and returns a response after cancellation, this assignment still enables the buffered-turn exception solely because the signal is now aborted. The first immediate tool function then runs after cancellation even though its response was not completed before the abort. Track the actual completion-versus-abort ordering rather than inferring that every completion observed with an aborted signal was already buffered.
AGENTS.md reference: AGENTS.md:L109-L113
Useful? React with 👍 / 👎.
| if (this.controller.signal.aborted && !bufferedToolCall) { | ||
| throw new APIUserAbortError(); | ||
| } |
There was a problem hiding this comment.
This guard only observes cancellation before invoking the tool. If cancellation occurs while an already-started callback is pending and the callback then resolves, a forced single-function tool_choice takes the functionCalled early-return path (and the final maxChatCompletions iteration behaves similarly), causing afterCompletion to run and runner.done() to resolve with runner.aborted === false. Retain the completed callback result as intended, but recheck the signal before any successful loop termination.
AGENTS.md reference: AGENTS.md:L109-L113
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass security review validates the three existing cancellation findings, including a High post-abort callback bypass, and identifies one additional Medium result-preservation regression documented inline.
| return { message: { role, tool_call_id, content }, functionCalled: false }; | ||
| } | ||
| if (this.controller.signal.aborted) { | ||
| throw new APIUserAbortError(); |
There was a problem hiding this comment.
[Medium] Preserve already-completed parallel tool results before propagating cancellation. This new abort rejection makes the parallel Promise.allSettled path throw on its rejected result before appending fulfilled callbacks' messages. Consequently a callback that already performed an irreversible side effect disappears from runner.messages, violating the stated preservation guarantee for both streaming and non-streaming runners. Record completed results in input order before surfacing the abort, and extend the parallel-cancellation regression to assert the completed tool result remains visible without starting further callbacks.
2 participants
Summary
chat.completions.runTools()from starting a privileged parsed tool callback after cancellation occurs during asynchronous argument parsing.runner.abort().APIUserAbortError, wait for callbacks that were already running, and retain results/messages for callbacks that began before cancellation.Regression-first proof
Before the change, the new real-public-client suite produced 10 failing security cases and 6 passing compatibility controls: synthetic privileged transfer callbacks ran with the runner signal already aborted. The final 20-case suite covers non-streaming and streaming public clients, external abort signals and direct runner cancellation, delayed parsers, sequential callbacks, in-flight parallel callback completion, one-shot buffered-turn compatibility, context, parser feedback, and
afterCompletion.Verification
publintpassed with only the pre-existing vendor-export warning.