fix: sanitize video block URL fields and scrub iframe sources - #204
Merged
Conversation
The video block's `video_url`, `title_url`, `thumbnail_url`, and
`provider_icon_url` passed through every sanitization layer untouched,
and `slack-blocks-to-jsx` renders `video_url` straight into an
`<iframe src>`. A crafted payload — via the JSON drawer, the `?blocks=`
URL state, `initialBlocks`, or a consumer backend — reached the DOM as
`<iframe src="javascript:...">` (same-origin script execution on React
18) or `<iframe src="data:text/html,...">`, which renders and executes
in an opaque origin on every React version.
All three layers were scoped to the URL fields that existed when they
were written:
- the payload sanitizer keyed on exact-match sets `{'url'}` /
`{'image_url'}`, so all four video fields were returned verbatim by
both `sanitizeBlock` and `toSlackBlocks`;
- the renderer applies no scheme filter of its own;
- the post-render DOM scrub walked `a[href]` and `img[src]` only.
Rather than add four more keys to a list that has now missed a field
twice, classify keys by name shape: anything whose last `_`-delimited
segment reads as a URL is sanitized, with image-ish names getting the
image allowlist and `video_url` the new embed allowlist. Over-matching
is free — a URL-shaped key holding a non-URL string has no recognized
scheme, so it is treated as relative and passed through.
`isSafeEmbedSrc` is the narrowest of the three predicates: http(s)
only. A frame loads without a click, so it admits neither `data:` of
any media type nor relative URLs (which would frame the embedding app
itself), and none of the link-only schemes.
The DOM scrub gains a third arm over `iframe`/`embed`/`object`/
`source`/`video`/`audio`/`track` sources, and the video editor now
flags an unsafe URL inline like the structured rich-text editor does.
`test/preview-url-scrub.test.tsx` is the layer that was missing: it
renders hostile payloads and walks every element of the result,
asserting no URL-bearing attribute on any tag that navigates or
auto-loads carries anything but an http(s) URL — so a renderer upgrade
that emits a new URL-bearing tag, or a Slack block that adds a new URL
field, fails there rather than in the next report. All 14 new
assertions fail against the pre-fix tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JDfR9ZDU5CvVAf4gpFh9Wc
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
block-kitchen | 565ca8d | Commit Preview URL Branch Preview URL |
Sep 01 2026, 12:29 AM |
Contributor
Cloudflare preview✅ Deployed |
6 tasks
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
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.
Summary
The video block's
video_url,title_url,thumbnail_url, andprovider_icon_urlpassed through every sanitization layer untouched, andslack-blocks-to-jsxrendersvideo_urlstraight into an<iframe src>. This adds an http(s)-only allowlist for frame/subresource sources, replaces the sanitizer's hand-maintained key list with name-shape classification so future URL fields are covered on arrival, and adds a DOM-walking regression test that fails on any unsanitized URL the renderer emits.Why
A crafted payload — via the JSON drawer, the
?blocks=URL state,initialBlocks, or a consumer backend — reached the DOM as<iframe src="javascript:...">(same-origin script execution on React 18, an explicitly supported peer) or<iframe src="data:text/html,...">, which renders and executes in an opaque origin on every React version. This is a bypass of the project's own F-001 remediation, which covered anchors and images.All three defense layers were scoped to the URL fields that existed when they were written:
HREF_KEYS = {'url'}/IMAGE_KEYS = {'image_url'}, so all four video fields were returned verbatim — bysanitizeBlockat the preview boundary and bytoSlackBlocks, the documented consumer hardening boundary.slack-blocks-to-jsx@1.1.2renders<iframe title={alt_text} src={video_url}>with no scheme filter.a[href]andimg[src]only;iframe[src]was never inspected.Reproduced in jsdom before the fix: rendering a video block whose
video_urlisjavascript:...fires jsdom'sevaluateJavaScriptURLon frame attach, before any of our code runs. The same block'stitle_urlanchor was scrubbed tohref="#"— the control showing layer 3 worked for anchors and not for frames.What changed
src/lib/url-safety.ts— newisSafeEmbedSrc/sanitizeEmbedSrc, the narrowest of the three predicates: http(s) only. A frame loads without a click, so it admits neitherdata:of any media type (the variant no React version blocks) nor scheme-less relative URLs (which would frame the embedding app itself), nor the link-only schemesmailto/tel/sms/xmpp/irc.src/lib/sanitize-blocks.ts— rather than add four more keys to a list that has now missed a field twice, keys are classified by name shape: anything whose last_-delimited segment reads as a URL is sanitized, image-ish names get the image allowlist,video_urlgets the embed allowlist, everything else gets the link allowlist. Over-matching is free — a URL-shaped key holding a non-URL string has no recognized scheme, so it is treated as relative and passed through untouched.src/components/preview/slack-block-preview.tsx— a third scrub arm overiframe[src], embed[src], object[data], source[src], video[src], audio[src], track[src], marking blocked elementsdata-bk-blocked-srclike the existing arms.src/components/editors/video-editor.tsx— all four URL fields flag an unsafe value witharia-invalidand an inline note, matching the structured rich-text editor instead of silently vanishing from the preview.SECURITY-REVIEW.md— F-009 documents this follow-up, and the "Items checked and clean" entry claiming no<iframe>elements is corrected:src/constructs none, but the renderer does.test/preview-url-scrub.test.tsxis the layer that was missing. Instead of asserting on the fields we happen to know about, it renders hostile payloads and walks every element of the result, asserting that no URL-bearing attribute on any tag that navigates or auto-loads carries anything but an http(s) URL. A renderer upgrade that emits a new URL-bearing tag, or a Slack block that adds a new URL field, fails there rather than in the next report.Test plan
pnpm typecheck— clean (tsconfig.json+tsconfig.test.json)pnpm lint— cleanpnpm test— 465 unit tests + 124 Storybook browser tests passaria-invalidstates only appear for unsafe input)pnpm build— cleanVerified the tests are real regression tests: with
src/stashed and the new tests in place, 14 assertions fail acrosstest/preview-url-scrub.test.tsxandtest/sanitize-blocks.test.ts; all pass with the fix applied.Notes for reviewer
SAFE_EMBED_PROTOCOLShas the same value asSAFE_IMAGE_PROTOCOLStoday but is kept as a separate constant deliberately — the two policies must be free to diverge, since the image predicate additionally acceptsdata:image/*and relative URLs that must never reach a frame.video_url, so the DOM arm is a backstop for frames the renderer produces from text content. Both layers are exercised by the tests.slack-blocks-to-jsxstill has no scheme allowlist of its own; ours is applied on both sides of it. Pushing one upstream would close this class at the source for every consumer of that package.Generated by Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.