fix(stage-ui): sanitize context snapshots before clone#1928
Conversation
⏳ Approval required for deploying to Cloudflare Workers (Preview) for stage-web.
Hey, maintainers, kindly take some time to review and approve this deployment when you are available. Thank you! 🙏 |
![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: af7a9609f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof value === 'bigint' || typeof value === 'symbol' || typeof value === 'function') | ||
| return undefined |
There was a problem hiding this comment.
When a mod or hook includes a bigint in a context message's generic metadata or content, the value is already structured-cloneable, so the previous structuredClone(...) broadcast path could carry it. This sanitizer now returns undefined for bigint, causing object properties to be dropped and array items to be removed before the stream event is broadcast, which silently corrupts otherwise valid context snapshots.
Useful? React with 👍 / 👎.
github-actions
Bot
added
bug
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16003f121a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expect(messages.at(-1)).toMatchObject({ | ||
| type: 'context:update', | ||
| data: { |
There was a problem hiding this comment.
When the stage-ui browser Vitest project runs this new case, messages.at(-1) is the raw ContextMessage posted by broadcastContext(toRaw(contextMessage)), not a { type, data } server event, and the handler overwrites event.data.metadata with event.metadata. As written this assertion can never match the broadcasted value, so the browser suite fails instead of guarding the BigInt behavior.
Useful? React with 👍 / 👎.
| if (proto !== Object.prototype && proto !== null) | ||
| return undefined |
There was a problem hiding this comment.
When a local hook or context provider puts a structured-cloneable non-plain object such as Map, Set, Blob, or ArrayBuffer into ContextMessage.content, the registry and previous stream broadcast path could carry it via structuredClone. This branch now returns undefined solely because the prototype is not Object.prototype, so those fields or array entries are silently removed from stream events even though they were valid cloneable context data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319f87ce4b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (seen.has(rawValue)) | ||
| return undefined |
There was a problem hiding this comment.
When a single context message reuses the same object in two places, such as metadata: { a: shared, b: shared } or a Map whose key and value are the same object, structuredClone would broadcast both references successfully. This WeakSet treats the second visit as a cycle and returns undefined, so the sanitizer drops the later field or entry and corrupts otherwise cloneable context snapshots; it should distinguish true recursion from shared references, or cache cloned values.
Useful? React with 👍 / 👎.
| if (rawValue instanceof Date) | ||
| return rawValue.toISOString() | ||
|
|
||
| if (rawValue instanceof RegExp) | ||
| return rawValue.toString() |
There was a problem hiding this comment.
Fresh evidence in this revision is that Date and RegExp are still explicitly converted here even after other cloneable built-ins were preserved. When a hook stores one of these values in ContextMessage.content or metadata, the previous structuredClone(...) path would deliver the same built-in type, but this sanitizer changes it to a string before broadcasting, breaking consumers that expect date or regexp behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d679631c61
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Array.isArray(rawValue)) { | ||
| return rawValue | ||
| .map(item => sanitizeCloneable(item, seen)) | ||
| .filter(item => item !== undefined) |
There was a problem hiding this comment.
When context metadata or content intentionally contains undefined (for example as an array placeholder or Map value), it is already structured-cloneable, but using undefined as the removal sentinel makes this filter and the analogous Map/object filters drop it. For positional arrays this also shifts later entries, so broadcast stream snapshots can no longer match the sender's context even though the original value was clone-safe.
Useful? React with 👍 / 👎.
1 participant
Summary
structuredClone(...)Testing
pnpm exec vitest run --config vitest.sanitize.config.tsgit diff --checkCloses #1909