Harden review frame: drop popup/download rights, validate external hrefs - #37
Open
dacoldest wants to merge 1 commit into
Open
Harden review frame: drop popup/download rights, validate external hrefs#37dacoldest wants to merge 1 commit into
dacoldest wants to merge 1 commit into
Conversation
The review iframe renders artifacts that are untrusted by definition, but its sandbox granted allow-popups and allow-downloads. Neither is needed: the SDK posts external links to the chrome over eh:external and the chrome opens them, and nothing downloads. Removing both narrows what a malicious artifact can do to the reviewer. The eh:external handler also called window.open on an unvalidated href. The origin check is the only thing in front of it, so a compromised frame or a misconfigured policy could land a javascript: URL there, executing on the chrome's origin alongside the session token. Route it through the existing normalizeHref, which already allowlists http/https/mailto/tel and is tested against javascript:, data:, vbscript: and tab-obfuscated variants, rather than adding a second URL-safety helper to diverge from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small hardening changes to the review chrome. Both turned up while I was reading through the frame policy before installing the skill, and both looked worth sending back rather than just patching locally.
Frame sandbox drops
allow-popupsandallow-downloadsThe iframe renders artifacts that are untrusted by definition, and its sandbox granted popups and downloads. Neither appears to be needed.
allow-popupsin particular is redundant with the design that is already there:sdk.jsdoes not open external links itself, it postseh:externalto the chrome, and the chrome callswindow.open. The frame already delegates popup-opening upward, so granting it the capability directly just widens what a hostile artifact can do to the reviewer. Nothing in the tree references downloads at all.Remaining sandbox is
allow-scripts allow-forms allow-modals, plusallow-same-originfor localhost reviews exactly as before.eh:externalvalidates the href beforewindow.openThe handler passed
msg.hrefstraight through. The origin check is the only thing in front of it, so a compromised frame or a misconfigured policy could land ajavascript:URL there, and it would execute on the chrome's origin next to the session token.Rather than add a second URL-safety helper, this routes the href through the existing
normalizeHrefinediting.js. It already allowlists http/https/mailto/tel and already has coverage forjavascript:,data:,vbscript:, and tab-obfuscated variants likejava\tscript:. Reusing it keeps one place where scheme policy lives.Tests
92/92 pass. Added one regression test asserting no review grants the frame popups or downloads, and confirmed it fails against the previous constant before keeping it, so a revert cannot pass silently.
Not changed
framePolicystill returnstargetOrigin: "*"for file and Markdown reviews. Tightening that to"null"for the opaque-origin case might be worth considering, but it changes an existing asserted value and felt like your call rather than mine.Happy to adjust or split this if you would prefer it in separate PRs.