fix(security): detach replay checkpoint appends from shared prototypes - #4402
Conversation
The direct-stream checkpoint persister is created after
ensureProjectDiscovery() has loaded tenant project code into the shared
realm. It captured JSON.stringify up front, but capturing the function
does not neutralize the dynamic `toJSON` lookup it performs on every
object and array it visits, nor the property reads it routes through
accessors. The append body was built from ordinary object literals, so
project code could install Object.prototype.toJSON during discovery,
receive the private checkpoint (opaque Anthropic replay blocks and
thinking signatures), and return a replacement object that became the
JSON body sent to /runs/{runId}/events under the host's opaque
run-event append token — a confused-deputy path for forging or
corrupting private durable events without ever learning the token.
Rebuild the append body out of null-prototype objects and arrays before
serializing it, using intrinsics captured at module load: own
enumerable data properties only (accessors are never invoked), string
keys only, and bounded by the existing provider replay depth and node
limits so a malformed checkpoint fails closed instead of recursing.
JSON.stringify then has no tenant-reachable prototype to consult.
Claude-Session: https://claude.ai/code/session_01QfWNMiUhvWMKWi6BGfVdY3
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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. |
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe checkpoint persister now detaches checkpoint data before JSON serialization. It protects against prototype and accessor hooks, enforces metadata limits, and adds extensive failure and serialization tests. ChangesCheckpoint persistence hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The security hardening is mergeable, but primitive-heavy checkpoints can still pass persistence limits and fail when read back. This is a bounded compatibility risk that should remain tracked. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Review: 83/100 — solid, well-targeted security fix; one off-by-one edge case and a test-coverage gap on the new fail-closed pathSummary: This correctly neutralizes the confused-deputy path (poisoned Strengths
Concerns
Actionable for the concerns above: change the depth guard to None of these undermine the core fix — the confused-deputy path is closed and the primary regression tests are good. The depth off-by-one is the only one I'd want addressed before merge; the rest are nice-to-haves. Generated by Claude Code |
![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: 0a68abee3b
ℹ️ 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".
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Review follow-ups on the replay checkpoint serializer: - `Object.getOwnPropertyDescriptor` returns an ordinary object that inherits from `Object.prototype`, and a data descriptor owns neither `get` nor `set`. Reading those fields therefore resolved through the shared prototype, letting tenant code installed during discovery run an accessor with the descriptor as `this` — enough to observe the opaque provider block and rewrite `this.value` before it reached the append. Descriptor fields are now probed with a captured `hasOwnProperty`, matching the snapshot validator in json-snapshot.ts. - `Reflect.ownKeys` hands back an ordinary array, so walking it with `for...of` resolved `Symbol.iterator` through `Array.prototype`. A tenant hook there could read the private field names and drop, reorder, or never finish yielding them. The reflected keys are now traversed by index through own data properties. - The depth bound rejected containers at exactly MAX_PROVIDER_REPLAY_RAW_METADATA_DEPTH while the snapshot validator behind parseProviderReplayCheckpointEvent rejects only past it, so a checkpoint could load successfully and then fail its next append. Both layers now use root-at-zero `depth > maxDepth`. Adds regression coverage for each (all three fail against the previous code), plus tests for the append body's remaining failure paths: the JSON.stringify omission parity, the non-serializable, depth and node bounds, an already-cancelled run, the append timeout, and an opaque transport failure. Claude-Session: https://claude.ai/code/session_01QfWNMiUhvWMKWi6BGfVdY3
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ Approved 1 resolved / 1 findingsHardens checkpoint serialization to block prototype-poisoning attacks where tenant code could intercept private run events via ✅ 1 resolved✅ Edge Case: Persister depth bound is stricter than the parse-back layer
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Important Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
`lint:test-semantic-dispositions` rejects a `src/**` unit test that mutates shared-realm prototypes, and the two new regression cases install accessors on `Object.prototype` and replace `Array.prototype[Symbol.iterator]`. They move to the existing tests/integration/semantic-unit-boundary intrinsics suite for this module rather than growing the migration inventory. Claude-Session: https://claude.ai/code/session_01QfWNMiUhvWMKWi6BGfVdY3
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4bd197787
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Hoisting the append body out of the request's try block moved it outside the failure sanitizer. Copying a checkpoint still runs exotic member code — a Proxy trap reached through `Reflect.ownKeys` or `getOwnPropertyDescriptor` — so a tenant-controlled throw escaped into run error handling instead of surfacing as DURABLE_RUN_EVENT_PERSISTENCE_FAILED, which is what happened before this PR when `JSON.stringify` ran inline inside that block. Serialization is now guarded: this module's own typed failures pass through unchanged, everything else becomes the opaque persistence error. The slug test the request catch already performed is factored into `isPersistenceFailure` and shared by both sites. Regression test uses a checkpoint carrying a Proxy whose `ownKeys` trap throws a secret-bearing error and asserts neither the secret nor the append survives. Claude-Session: https://claude.ai/code/session_01QfWNMiUhvWMKWi6BGfVdY3
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
3 participants
Summary
The direct-stream provider replay checkpoint persister is constructed after
ensureProjectDiscovery()has dynamically imported tenant project code into the shared realm (src/server/handlers/request/agent-stream.handler.ts:1137discovery,:1185persister creation). The persister capturedJSON.stringifyat module load, but capturing the function does not neutralize the dynamictoJSONlookup the algorithm performs on every object and array it visits, nor the property reads it routes through accessors. The append body was assembled from ordinary object literals ({ events: [createProviderReplayCheckpointEvent(checkpoint)] }, andcreateProviderReplayCheckpointEventreturns a plain{ type, ...checkpoint }), all inheriting fromObject.prototype. Project code could therefore installObject.prototype.toJSONduring discovery and have it invoked when a checkpoint is persisted: the hook receives the private checkpoint — including opaque Anthropic provider blocks and thinking signatures the parse layer deliberately treats as private — and can return a replacement object that becomes the JSON body POSTed to/runs/{runId}/eventswith the host's opaqueAuthorization: Bearer <runEventAppendToken>. That is a confused-deputy path for forging or corrupting private durable run events without ever learning the token, and it crosses exactly the host-vs-tenant credential boundary this file was written to defend.Finding
3621532baba88191a4317ffda38102e7(finding 163, "Prototype poisoning controls privileged checkpoint appends")8ca99ad3119d18011c4611949d4e2446ebbe954f("fix(agent): persist direct-stream replay checkpoints", fix(agent): persist direct-stream replay checkpoints #4312)src/internal-agents/provider-replay-checkpoint-persister.ts(append body serialization), reached fromsrc/server/handlers/request/agent-stream.handler.tswith event shapes fromsrc/agent/runtime/provider-replay.tsRe-verified on
origin/main(78ec40a9f): the persister was untouched since the introducing commit, no serialization hardening exists (git log -S toJSONfinds none), and no open PR covers it.Fix
Serialize the privileged append body from containers no tenant prototype can reach, rather than from ambient object literals:
Object.create,Object.setPrototypeOf,Object.getOwnPropertyDescriptor,Reflect.ownKeys,Array.isArray) alongside the credential-touching intrinsics already captured at module load.JSON.stringify. With no prototype, there is no inheritedtoJSONto resolve and no inherited index accessor to fire; arrays are detached before entries are written.JSON.stringifyserializes. Accessors are read from the descriptor and skipped, so no getter reachable from the checkpoint runs during serialization.JSON.stringify's own omission semantics (undefined/function/symbol dropped in objects,nullin arrays) so the bytes are unchanged for well-formed checkpoints, and fail closed on anything it could not represent.MAX_PROVIDER_REPLAY_RAW_METADATA_DEPTH/MAX_PROVIDER_REPLAY_RAW_METADATA_NODESlimits, so a malformed or cyclic checkpoint raisesDURABLE_RUN_EVENT_PERSISTENCE_FAILEDinstead of recursing unbounded.No behavior change for well-formed checkpoints: the emitted body is byte-identical.
Test evidence
Two regression tests added next to the existing boundary tests in
src/internal-agents/provider-replay-checkpoint-persister.test.ts:toJSONhook on the prototype chain of the checkpoint'sproviderBlocksarray,providerBlockPositionsarray, block wrapper and opaque block, then asserts the hook is never invoked, that the body is byte-identical to the un-poisoned append, and that the forged marker never reaches the wire. (Poisoning the object's own prototype chain exercises the same dynamictoJSONlookup asObject.prototype.toJSONwhile keeping the unit hermetic, which the repo's semantic unit-boundary gate requires.)Both tests fail on the pre-fix serialization (verified by temporarily restoring the old
JSON.stringifycall:FAILED | 0 passed (4 steps) | 1 failed (2 steps)) and pass with the fix.Commands run locally (Deno repo — no pnpm/biome/vitest here; used the equivalents from the
lint-typecheck/formatjobs in.github/workflows/cicd.yml):deno fmt --check src/internal-agents/— clean (19 files)deno lint src/internal-agents/— clean (19 files)deno check src/internal-agents/provider-replay-checkpoint-persister.ts— cleandeno task test:file src/internal-agents/provider-replay-checkpoint-persister.test.ts—ok | 1 passed (6 steps) | 0 faileddeno task test:fileoversrc/internal-agents/{ag-ui-sse,run-stream,control-plane-auth}.test.ts,src/agent/runtime/provider-replay{,-emission}.test.ts,src/server/handlers/request/agent-stream.handler.test.ts—ok | 8 passed (248 steps) | 0 faileddeno task lint:anti-slop— baseline ok;deno task lint:test-semantic-dispositions— ok;deno task lint:module-boundaries— ok;deno task lint:dependency-boundaries— ok;deno task lint:skipped-tests— ok;deno task lint:ban-test-only— none foundhttps://claude.ai/code/session_01QfWNMiUhvWMKWi6BGfVdY3
Summary by CodeRabbit