fix(widget): gate sandbox HTML delivery by nonce#3628
Conversation
|
@lspassos1 is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
![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: df271d17d7
ℹ️ 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".
| widgetTokenStore.delete(id); | ||
| const html = buildWidgetDoc(body); | ||
| iframeHtmlStore.set(iframe, html); | ||
| mountedWidgetDocs.set(id, { iframe, html, token }); |
There was a problem hiding this comment.
Storing each PRO widget in mountedWidgetDocs without any removal path causes an unbounded memory leak because each entry holds a strong reference to the iframe plus its full HTML document. wrapProWidgetHtml() is called repeatedly in flows like preview rerenders (e.g., WidgetChatModal.renderPreviewHtml), so closed/replaced widgets keep accumulating in memory instead of being garbage-collected (the previous WeakMap behavior avoided this), which will degrade long-lived sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 64f994e. The registry now queues removed PRO widget iframes from the MutationObserver and prunes the mounted entry in a microtask only if that same iframe remains disconnected. That preserves same-cycle DOM moves while avoiding strong retention of closed/replaced iframe HTML.
Greptile SummaryThis PR closes a race condition where the PRO widget iframe could receive generated HTML from the parent unconditionally on every
Confidence Score: 4/5Safe to merge for the security fix it delivers; the two quality concerns (post-handshake map cleanup and referrer-policy silent failure) do not block correctness in the standard deployment but warrant follow-up. The nonce handshake logic is sound — source, token, and origin are all independently checked on both sides. The main residual concern is
Important Files Changed
Sequence DiagramsequenceDiagram
participant Parent as Parent Window (widget-sanitizer.ts)
participant Sandbox as wm-widget-sandbox.html (opaque origin)
Note over Parent: wrapProWidgetHtml() generates widget-id and crypto token,<br/>stores body+token in widgetBodyStore/widgetTokenStore
Parent->>Parent: MutationObserver detects iframe with data-wm-id
Parent->>Parent: mountProWidget() stores iframe+html+token in mountedWidgetDocs
Parent->>Sandbox: Sets iframe.src to sandbox URL with widget-id and token in hash fragment
Note over Sandbox: Script reads widget-id and token from URL hash,<br/>registers wm-html message listener
Sandbox->>Sandbox: getParentOrigin() reads document.referrer
Sandbox-->>Parent: postMessage wm-widget-ready with widget-id and token to parentOrigin
Note over Parent: handleWidgetSandboxReady() validates<br/>event.source matches stored iframe<br/>token matches stored token
Parent->>Sandbox: "postMessage wm-html with widget-id, token, and HTML payload to *"
Note over Sandbox: Validates source is window.parent<br/>Validates widget-id and token<br/>Validates html is a string<br/>Validates isAllowedParentOrigin(e.origin)
Sandbox->>Sandbox: document.open/write/close — handled flag set to block re-delivery
Reviews (1): Last reviewed commit: "fix(widget): gate sandbox HTML delivery ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const mounted = mountedWidgetDocs.get(data.id); | ||
| if (!mounted || data.token !== mounted.token || event.source !== mounted.iframe.contentWindow) { | ||
| return; | ||
| } | ||
|
|
||
| // The sandbox deliberately omits allow-same-origin, so the child document has | ||
| // an opaque origin and cannot be targeted with location.origin. The per-widget | ||
| // token and source check above are the trust boundary before sending HTML. | ||
| mounted.iframe.contentWindow?.postMessage( | ||
| { type: 'wm-html', id: data.id, token: mounted.token, html: mounted.html }, | ||
| '*', | ||
| ); | ||
| } |
There was a problem hiding this comment.
mountedWidgetDocs never cleaned up after handshake
After handleWidgetSandboxReady fires and delivers HTML to the iframe, the entry in mountedWidgetDocs is never removed. This keeps a strong reference to the HTMLIFrameElement alive for the lifetime of the page, preventing the iframe (and any sub-tree attached to it) from being garbage collected. The old implementation used a WeakMap<HTMLIFrameElement, string> which correctly allowed GC when the iframe was removed from the DOM. Since the panel layout supports drag-and-drop and widget removal, many entries can accumulate over a user session.
Calling mountedWidgetDocs.delete(data.id) after the postMessage inside handleWidgetSandboxReady (i.e., once the HTML has been delivered) would restore the previous GC-friendly behaviour. The handled guard on the sandbox side already prevents a second document.write if the parent somehow re-delivers, so deleting the entry is safe.
There was a problem hiding this comment.
Addressed across 64f994e and eb258c0. I kept the entry while the iframe is still connected so sandbox reloads after DOM moves can request the same HTML again, but the MutationObserver now queues removed iframes and deletes the mounted entry in a microtask if that exact iframe remains disconnected. That avoids retaining closed/replaced iframe HTML without breaking same-cycle reparenting.
| function getParentOrigin() { | ||
| try { | ||
| return new URL(document.referrer).origin; | ||
| } catch (_err) { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('message', (e) => { | ||
| if (handled) return; | ||
| if (!e.data || e.data.type !== 'wm-html') return; | ||
| if (e.source !== window.parent) return; | ||
| if (e.data.id !== id || e.data.token !== token) return; | ||
| if (typeof e.data.html !== 'string') return; | ||
|
|
||
| var origin = e.origin || ''; | ||
| if (!origin.endsWith('worldmonitor.app') && !/^https?:\/\/localhost/.test(origin)) return; | ||
| if (!isAllowedParentOrigin(origin)) return; | ||
|
|
||
| handled = true; | ||
| document.open(); | ||
| document.write(e.data.html); | ||
| document.close(); | ||
| }); | ||
| }()); | ||
|
|
||
| var parentOrigin = getParentOrigin(); | ||
| if (id && token && parentOrigin && isAllowedParentOrigin(parentOrigin)) { | ||
| window.parent.postMessage({ type: 'wm-widget-ready', id: id, token: token }, parentOrigin); | ||
| } |
There was a problem hiding this comment.
getParentOrigin() derives the parent's origin from document.referrer. If the parent page (or the server response) specifies Referrer-Policy: no-referrer or no-referrer-when-downgrade, the iframe receives an empty document.referrer. The guard if (id && token && parentOrigin && isAllowedParentOrigin(parentOrigin)) then evaluates to false, so wm-widget-ready is never posted, the parent never sends HTML, and the widget renders blank with no console error.
This is a known trade-off of opaque-origin sandboxing (there's no other way to discover the parent's origin without allow-same-origin), but the failure mode is completely silent. Consider adding a console.warn when parentOrigin is empty so that the handshake break surfaces during development or smoke testing.
There was a problem hiding this comment.
Addressed in eb258c0. The sandbox now emits a console warning when the ready-message handshake is blocked by missing or disallowed parent origin, so a referrer-policy break no longer fails silently during development or smoke testing.
1 participant
Summary
Closes #3544 by replacing the unconditional PRO widget iframe load-time HTML post with a nonce-gated sandbox handshake. The iframe remains isolated with
sandbox="allow-scripts"and noallow-same-origin.Root cause
mountProWidget()sent the generated widget document toiframe.contentWindowwithtargetOrigin: '*'whenever the iframe load event fired. If the iframe navigated before that load path completed, the parent could send widget HTML to an unexpected document.Changes
/wm-widget-sandbox.html.idand nonce through the iframe URL fragment so they are not sent in HTTP referrers.wm-widget-readyonly to the parent origin derived fromdocument.referrer.event.sourceand nonce in the parent before sending HTML.document.write().Notes
The final parent-to-sandbox
postMessagestill uses'*'because the sandbox intentionally has an opaque origin withoutallow-same-origin. Addingallow-same-originwould weaken the isolation. The trust boundary is now the source-checked, per-widget nonce handshake before any HTML is delivered.Validation
node --test tests/widget-builder.test.mjsnpx biome check src/utils/widget-sanitizer.ts tests/widget-builder.test.mjs public/wm-widget-sandbox.htmlnpm run typechecknpm run lint:unicodegit diff --check -- src/utils/widget-sanitizer.ts public/wm-widget-sandbox.html tests/widget-builder.test.mjsRisk
Medium-low. This is isolated to PRO widget iframe mounting. The main compatibility risk is PRO widget rendering if a browser blocks the handshake; the sandbox now warns for the known referrer-policy failure path, and the local browser smoke test covers the expected opaque-sandbox flow. Registry cleanup is deferred to the microtask after DOM mutations so same-cycle DOM moves keep their mounted entry.