Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ Helpers and send-flow primitives also exported:

```ts
import {
toSlackBlocks, // scrubs unsafe URLs + retrieval-only fields before sending
toSlackBlocks, // scrubs unsafe URLs + retrieval-only / renderer-only fields before sending
encodeBlocksToString, // base64url-encode a blocks array (for URL state)
decodeBlocksFromString,
defaultPalette, // the built-in palette — spread to customize
Expand Down
19 changes: 18 additions & 1 deletion SECURITY-REVIEW.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@ URL-sanitization story reads in one place.
- `src/components/preview/slack-block-preview.tsx`: a third scrub arm over `iframe[src], embed[src], object[data], source[src], video[src], audio[src], track[src]`, applying the embed allowlist and marking blocked elements `data-bk-blocked-src`.
- `src/components/editors/video-editor.tsx`: all four URL fields flag an unsafe value with `aria-invalid` and an inline note, matching the structured rich-text editor.
- **Tests**: [test/preview-url-scrub.test.tsx](test/preview-url-scrub.test.tsx) is the one that would have caught this. It renders hostile payloads and then 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 — rather than asserting on the fields we happen to know about today. A renderer upgrade that emits a new URL-bearing tag, or a Slack block that adds a new URL field, fails there. Field-level coverage in [test/sanitize-blocks.test.ts](test/sanitize-blocks.test.ts), [test/url-safety.test.ts](test/url-safety.test.ts), [test/public-api.test.ts](test/public-api.test.ts), and [test/video-editor.test.tsx](test/video-editor.test.tsx). All 14 new assertions fail against the pre-fix tree.
- **Follow-up (2026-09-03) — the renderer's `iframeProps` prop bag**: re-verifying the fix above turned up a fourth path into the same `<iframe>`. `slack-blocks-to-jsx` destructures `iframeProps` off the video block payload and spreads it onto the frame *after* its own `src`. It is a documented renderer extension, not a Slack field, so `sanitizeBlock` and `toSlackBlocks` both passed it through: the key-shape URL classifier sees a `src` inside it, but `srcdoc`, `sandbox`, and `allow` are not URLs. A payload of `{"type":"video","video_url":"https://…","iframeProps":{"srcdoc":"<script>…</script>"}}` rendered `<iframe srcdoc="<script>…">`, and an inline frame document runs with the embedding app's **own** origin on every React version — the same-origin outcome the `data:` variant above does not reach. Reproduced in jsdom against the fixed tree.
- `src/lib/sanitize-blocks.ts`: any key whose last `_`-delimited segment is `props` (`iframeProps`, `iframe_props`, a future `imgProps`) is dropped wherever it appears, at both boundaries. Same name-shape reasoning as the URL keys: Slack has no such field and rejects one on send, so over-matching costs nothing.
- `src/components/preview/slack-block-preview.tsx`: a backstop arm strips `srcdoc` from every frame, whatever its value, and marks it `data-bk-blocked-srcdoc`. The renderer never emits one itself.
- `toSlackBlocks` output for such a payload now passes the validator, which had been rejecting the bag as `unknown property 'iframeProps'`.
- Tests: prop-bag cases in [test/preview-url-scrub.test.tsx](test/preview-url-scrub.test.tsx) (payload layer, asserting the legitimate `video_url` survives and no `srcdoc`/`sandbox`/`allow` lands on the frame) and [test/preview-srcdoc-scrub.test.tsx](test/preview-srcdoc-scrub.test.tsx) (DOM layer, with the payload sanitizer mocked to a pass-through so the scrub has to do the work), plus [test/sanitize-blocks.test.ts](test/sanitize-blocks.test.ts) and [test/public-api.test.ts](test/public-api.test.ts).
- **Residual risk**: `slack-blocks-to-jsx` still 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.

---
Expand All @@ -193,7 +198,7 @@ These were inspected, deemed safe as-shipped, and noted here so future reviewers
- **Clipboard reads**: none.
- **Raw `fetch` / XHR**: not performed by the library; all I/O is brokered by consumer callbacks (`loadChannels`, `loadSendAsUserStatus`, `onSend`).
- **File uploads / `FileReader` / `URL.createObjectURL`**: none.
- **`<iframe>` elements**: none constructed by `src/` itself — but `slack-blocks-to-jsx` renders one for the video block's `video_url`, which is what F-009 missed. The preview's DOM scrub now covers `iframe`/`embed`/`object`/`source`/`video`/`audio`/`track` sources, so "we don't write `<iframe>`" is not the same as "no `<iframe>` renders".
- **`<iframe>` elements**: none constructed by `src/` itself — but `slack-blocks-to-jsx` renders one for the video block's `video_url`, which is what F-009 missed, and spreads the payload's `iframeProps` onto it, which the F-009 follow-up missed. The preview's DOM scrub now covers `iframe`/`embed`/`object`/`source`/`video`/`audio`/`track` sources and strips `srcdoc`, and the sanitizer drops the prop bag before render, so "we don't write `<iframe>`" is not the same as "no `<iframe>` renders".
- **`JSON.parse` of untrusted input**: two sites ([url-state.ts:42](src/lib/url-state.ts:42), [json-drawer.tsx:64](src/components/json-drawer.tsx:64)) — both wrapped in try/catch, top-level array check, and now size-capped. `__proto__` keys in JSON do not pollute `Object.prototype` in modern engines and the sanitizer was verified to be free of `Object.assign`-flavored merges that walk the prototype chain ([test/sanitize-blocks.test.ts](test/sanitize-blocks.test.ts) `prototype pollution shape`).
- **Random IDs**: `nanoid@5.x` (CSPRNG-backed). Not used for security tokens; appropriate.
- **Toolbar docs link**: [toolbar.tsx:158-166](src/components/toolbar.tsx:158) is a hardcoded `docs.slack.dev` URL with `rel="noreferrer noopener"`. Safe.
Expand Down Expand Up @@ -249,3 +254,15 @@ test/url-safety.test.ts (edit) F-009
test/sanitize-blocks.test.ts (edit) F-009
test/public-api.test.ts (edit) F-009
```

Follow-up (`iframeProps`):

```
src/lib/sanitize-blocks.ts (edit) F-009
src/lib/to-slack-blocks.ts (edit) F-009
src/components/preview/slack-block-preview.tsx (edit) F-009
test/preview-srcdoc-scrub.test.tsx (new) F-009
test/preview-url-scrub.test.tsx (edit) F-009
test/sanitize-blocks.test.ts (edit) F-009
test/public-api.test.ts (edit) F-009
```
8 changes: 8 additions & 0 deletions src/components/preview/slack-block-preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ export function SlackBlockPreview({
// `#`. Frames get the strictest arm: an `<iframe src>` loads on render
// rather than on click, so `data:text/html` there executes script in an
// opaque origin on every React version and only http(s) is allowed.
// A frame also loses any `srcdoc`: the renderer never emits one itself
// — it could only arrive via the payload's `iframeProps` bag, which the
// sanitizer drops — and an inline document runs in the embedding app's
// own origin, with no scheme to allowlist.
useEffect(() => {
const root = rootRef.current;
if (!root) return;
Expand Down Expand Up @@ -101,6 +105,10 @@ export function SlackBlockPreview({
el.setAttribute('data-bk-blocked-src', '1');
}
}
for (const frame of root.querySelectorAll<HTMLIFrameElement>('iframe[srcdoc]')) {
frame.removeAttribute('srcdoc');
frame.setAttribute('data-bk-blocked-srcdoc', '1');
}
});

return (
Expand Down
53 changes: 47 additions & 6 deletions src/lib/sanitize-blocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
* schemes (e.g. `javascript:`, `data:text/html`) from `url`,
* `image_url`, `video_url`, rich-text link `url` and friends before a
* Block Kit payload reaches a renderer or is handed back to the consumer.
* Also drops the fields no Slack payload should carry: the read-only
* metadata Slack attaches on retrieval, and the prop bags the renderer
* would spread verbatim onto a DOM element (see {@link PROP_BAG_KEY}).
*
* Allocates a new object whenever a child is rewritten; otherwise
* returns the input unchanged so unaffected payloads are reference-stable.
Expand Down Expand Up @@ -56,6 +59,43 @@ const MAYBE_URL_KEY = /url|uri|link|href|src/i;
*/
const IMAGE_KEY_HINT = /image|thumb|icon|avatar|logo|photo|picture/;

/**
* Key names whose value the renderer spreads verbatim onto a DOM
* element. `slack-blocks-to-jsx` reads `iframeProps` off the video block
* and spreads it onto the `<iframe>` *after* its own `src`, so a payload
* can override the frame source, add `srcdoc` (an inline document that
* executes in the embedding app's origin on every React version), or
* loosen `sandbox` / `allow`. None of that is a Slack field — Slack
* rejects an unknown property on send — so the whole bag is dropped, at
* the preview boundary and in `toSlackBlocks` alike. Matched on the last
* `_`-delimited segment for the same reason URL keys are (see
* {@link URL_KEY}): a renderer upgrade that adds `imgProps` or
* `linkProps` is covered on arrival rather than after the next report.
*/
const PROP_BAG_KEY = /(^|_)props$/;

/**
* Folds camelCase to snake_case and lowercases, so a consumer payload
* that isn't strict Slack JSON (`videoUrl`, `iframeProps`) classifies
* the same way as its snake_case form.
* @param key - the payload object's key
* @returns the normalized key
*/
function normalizeKey(key: string): string {
return key.replace(/([a-z0-9])([A-Z])/g, '$1_$2').toLowerCase();
}

/**
* Returns true when `key` names a prop bag the renderer would spread
* onto a DOM element, so the key and its value must be dropped. See
* {@link PROP_BAG_KEY}.
* @param key - the payload object's key
* @returns whether the key is a renderer-only prop bag
*/
function isRendererPropBag(key: string): boolean {
return PROP_BAG_KEY.test(normalizeKey(key));
}

/**
* Classifies a payload key by the kind of URL it carries, or `null`
* when the key holds no URL at all.
Expand All @@ -66,9 +106,7 @@ function classifyUrlKey(key: string): UrlKind | null {
if (!MAYBE_URL_KEY.test(key)) {
return null;
}
// Fold camelCase to snake_case first so a consumer payload that isn't
// strict Slack JSON (`videoUrl`, `iconUrl`) classifies the same way.
const normalized = key.replace(/([a-z0-9])([A-Z])/g, '$1_$2').toLowerCase();
const normalized = normalizeKey(key);
if (EMBED_KEYS.has(normalized)) {
return 'embed';
}
Expand Down Expand Up @@ -99,7 +137,8 @@ const RETRIEVAL_ONLY_KEYS = new Map<string, Set<string>>([
* Block Kit payload fragment. Returns a value with the same structural
* shape, where any field whose name reads as a URL has been replaced by
* the safe variant (`''` if the original scheme was unsafe for the kind
* of URL that field carries — see {@link classifyUrlKey}).
* of URL that field carries — see {@link classifyUrlKey}), and where the
* retrieval-only and renderer-only keys have been dropped.
* @param value - any payload fragment (object, array, primitive)
* @returns the sanitized payload fragment
*/
Expand All @@ -123,7 +162,7 @@ function sanitizeValue(value: unknown): unknown {
const dropKeys = typeof src.type === 'string' ? RETRIEVAL_ONLY_KEYS.get(src.type) : undefined;
let copy: Record<string, unknown> | null = null;
for (const key of Object.keys(src)) {
if (dropKeys?.has(key)) {
if (dropKeys?.has(key) || isRendererPropBag(key)) {
copy ??= { ...src };
delete copy[key];
continue;
Expand Down Expand Up @@ -155,7 +194,9 @@ function sanitizeValue(value: unknown): unknown {
/**
* Sanitize a single Block Kit block, scrubbing dangerous URI schemes
* from every URL-bearing field anywhere in the payload tree (`url`,
* `image_url`, `video_url`, `thumbnail_url`, `title_url`, …).
* `image_url`, `video_url`, `thumbnail_url`, `title_url`, …) and dropping
* the renderer-only prop bags (`iframeProps`) a payload could use to
* reach the DOM around that scrub.
* @param block - the block payload to sanitize
* @returns the sanitized block (same reference if nothing changed)
*/
Expand Down
8 changes: 5 additions & 3 deletions src/lib/to-slack-blocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import type { SupportedBlock } from '../types';
import { sanitizeBlock } from './sanitize-blocks';

/**
* Prepare blocks for the Slack API: scrub dangerous URI schemes from any
* `url`/`image_url` fields and drop the read-only metadata Slack attaches
* on retrieval but rejects on send. Every URL/image-url is routed through
* Prepare blocks for the Slack API: scrub dangerous URI schemes from
* every URL-bearing field, drop the read-only metadata Slack attaches on
* retrieval but rejects on send, and drop the renderer-only prop bags
* (`iframeProps`) that `slack-blocks-to-jsx` would spread onto the DOM
* and Slack rejects as unknown properties. Every URL is routed through
* the allowlist in `lib/url-safety.ts` so a payload that round-trips
* through the builder cannot carry `javascript:`/`data:text/html` URIs to
* a downstream consumer or to the Slack API.
Expand Down
50 changes: 50 additions & 0 deletions test/preview-srcdoc-scrub.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { render } from '@testing-library/react';
import { SlackBlockPreview } from '../src/components/preview/slack-block-preview';
import type { SupportedBlock } from '../src/types';

/**
* Exercises the preview's post-render DOM scrub on its own. The payload
* sanitizer drops the `iframeProps` bag before the renderer sees it, so
* with it in place nothing here would ever reach the DOM; it is mocked
* to a pass-through so the scrub has to do the work.
*/
vi.mock('../src/lib/sanitize-blocks', () => ({
sanitizeBlock: <T,>(block: T): T => block
}));

const videoWith = (iframeProps: Record<string, unknown>): SupportedBlock =>
({
type: 'video',
alt_text: 'poc',
title: { type: 'plain_text', text: 'PoC' },
thumbnail_url: 'https://example.com/thumb.png',
video_url: 'https://www.youtube.com/embed/abc',
iframeProps
}) as unknown as SupportedBlock;

describe('SlackBlockPreview DOM scrub (payload sanitizer bypassed)', () => {
it('strips srcdoc from a frame the renderer emitted with one', () => {
const { container } = render(<SlackBlockPreview block={videoWith({ srcdoc: '<script>top.__pwned=1</script>' })} />);
const iframe = container.querySelector('iframe');
expect(iframe?.hasAttribute('srcdoc')).toBe(false);
expect(iframe?.getAttribute('data-bk-blocked-srcdoc')).toBe('1');
// The legitimate source is untouched.
expect(iframe?.getAttribute('src')).toBe('https://www.youtube.com/embed/abc');
});

it('strips a non-http(s) src override on the same frame', () => {
const { container } = render(
<SlackBlockPreview block={videoWith({ src: 'data:text/html,<script>top.__pwned=1</script>' })} />
);
const iframe = container.querySelector('iframe');
expect(iframe?.getAttribute('src') ?? '').toBe('');
expect(iframe?.getAttribute('data-bk-blocked-src')).toBe('1');
});

it('leaves a frame without srcdoc unmarked', () => {
const { container } = render(<SlackBlockPreview block={videoWith({})} />);
const iframe = container.querySelector('iframe');
expect(iframe?.hasAttribute('data-bk-blocked-srcdoc')).toBe(false);
expect(iframe?.getAttribute('src')).toBe('https://www.youtube.com/embed/abc');
});
});
35 changes: 35 additions & 0 deletions test/preview-url-scrub.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,41 @@ describe('SlackBlockPreview renders no executable URL', () => {
expect(unsafeUrlAttributes(container)).toEqual([]);
});

// `slack-blocks-to-jsx` spreads a video block's `iframeProps` onto the
// `<iframe>` after its own `src`. It is not a Slack field, so nothing in
// it may reach the frame: not `srcdoc` (an inline document runs in the
// embedding app's origin), not a `src` override, not a loosened
// `sandbox` or `allow`. The legitimate `video_url` must survive, so the
// bag is discarded rather than clobbering the frame it was aimed at.
const LEGIT_EMBED = 'https://www.youtube.com/embed/dQw4w9WgXcQ';
const withIframeProps = (iframeProps: Record<string, unknown>): SupportedBlock =>
({
type: 'video',
alt_text: 'poc',
title: { type: 'plain_text', text: 'PoC' },
thumbnail_url: 'https://example.com/thumb.png',
video_url: LEGIT_EMBED,
iframeProps
}) as unknown as SupportedBlock;

it.each([
{ srcdoc: '<script>top.__pwned=1</script>' },
{ srcDoc: '<script>top.__pwned=1</script>' },
{ src: 'data:text/html,<script>top.__pwned=1</script>' },
{ src: 'javascript:top.__pwned=1' },
{ src: '/admin' },
{ sandbox: 'allow-scripts allow-same-origin', allow: 'camera; microphone' }
])('renders a video block carrying iframeProps %j as if the bag were absent', (iframeProps) => {
const { container } = render(<SlackBlockPreview block={withIframeProps(iframeProps)} />);
const iframe = container.querySelector('iframe');
expect(iframe).not.toBeNull();
expect(unsafeUrlAttributes(container)).toEqual([]);
expect(iframe?.getAttribute('src')).toBe(LEGIT_EMBED);
for (const attr of ['srcdoc', 'sandbox', 'allow']) {
expect(iframe?.hasAttribute(attr)).toBe(false);
}
});

it.each(HOSTILE_URLS)('scrubs mrkdwn and rich-text links carrying %p', (url) => {
const blocks: SupportedBlock[] = [
{ type: 'section', text: { type: 'mrkdwn', text: `[click](${url}) and <${url}|label>` } },
Expand Down
22 changes: 22 additions & 0 deletions test/public-api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,28 @@ describe('toSlackBlocks URL sanitization', () => {
expect(out.provider_icon_url).toBe('');
});

// `iframeProps` is a `slack-blocks-to-jsx` extension the renderer
// spreads onto the `<iframe>`; Slack rejects it as an unknown property.
// Dropping it here keeps the bag out of any consumer-side renderer and
// makes a payload that was only ever previewed send-valid again.
it('drops the renderer-only iframeProps bag from a video block', () => {
const input = [
{
type: 'video',
alt_text: 'poc',
title: { type: 'plain_text', text: 'PoC' },
thumbnail_url: 'https://example.com/t.png',
video_url: 'https://www.youtube.com/embed/abc',
iframeProps: { srcdoc: '<script>top.__pwned=1</script>' }
}
] as unknown as SupportedBlock[];
expect(validateBlockKit(input, { target: 'blocks' }).valid).toBe(false);

const [out] = toSlackBlocks(input);
expect(Object.hasOwn(out, 'iframeProps')).toBe(false);
expect(validateBlockKit([out], { target: 'blocks' }).valid).toBe(true);
});

it('passes a legitimate https video block through unchanged', () => {
const input = [
{
Expand Down
Loading
Loading