Skip to content

Commit 43b1dfc

Browse files
zhawtofclaude
andauthored
fix(security): sanitize URL schemes in block payloads, preview, and editors (#46)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a77cf11 commit 43b1dfc

16 files changed

Lines changed: 864 additions & 23 deletions

.github/workflows/dependabot-automerge.yml

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,53 @@ jobs:
1717
with:
1818
github-token: "${{ secrets.GITHUB_TOKEN }}"
1919

20+
# Security-sensitive packages are excluded from auto-merge so a
21+
# compromised upstream release cannot ship to consumers without a
22+
# human eyeballing the diff. These are the packages whose code
23+
# directly renders or sanitizes user-controlled content (links,
24+
# markdown, block payloads) — a malicious minor release here is
25+
# the worst case for our supply-chain posture.
26+
- name: Check exclusion list
27+
id: excluded
28+
env:
29+
PACKAGE_NAMES: ${{ steps.metadata.outputs.dependency-names }}
30+
run: |
31+
set -euo pipefail
32+
excluded=false
33+
IFS=', ' read -r -a names <<< "$PACKAGE_NAMES"
34+
for pkg in "${names[@]}"; do
35+
case "$pkg" in
36+
slack-blocks-to-jsx|react-markdown|remark-gfm|@tiptap/extension-link|@tiptap/starter-kit|@tiptap/core|@tiptap/react|@tiptap/pm|@tightknitai/slack-block-kit-validator|ajv|ajv-formats|slack-web-api-client)
37+
excluded=true
38+
;;
39+
esac
40+
done
41+
echo "excluded=$excluded" >> "$GITHUB_OUTPUT"
42+
2043
- name: Enable auto-merge for minor/patch updates
21-
if: steps.metadata.outputs.update-type != 'version-update:semver-major'
44+
if: |
45+
steps.metadata.outputs.update-type != 'version-update:semver-major' &&
46+
steps.excluded.outputs.excluded != 'true'
2247
run: gh pr merge --auto --squash "$PR_URL"
2348
env:
2449
PR_URL: ${{ github.event.pull_request.html_url }}
2550
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
2651

2752
- name: Approve PR
28-
if: steps.metadata.outputs.update-type != 'version-update:semver-major'
53+
if: |
54+
steps.metadata.outputs.update-type != 'version-update:semver-major' &&
55+
steps.excluded.outputs.excluded != 'true'
2956
run: gh pr review --approve "$PR_URL"
3057
env:
3158
PR_URL: ${{ github.event.pull_request.html_url }}
3259
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
60+
61+
- name: Comment when held for manual review
62+
if: |
63+
steps.metadata.outputs.update-type != 'version-update:semver-major' &&
64+
steps.excluded.outputs.excluded == 'true'
65+
run: |
66+
gh pr comment "$PR_URL" --body "Held for manual review: \`${{ steps.metadata.outputs.dependency-names }}\` is on the security-sensitive auto-merge exclusion list. Verify the changelog and source diff before merging."
67+
env:
68+
PR_URL: ${{ github.event.pull_request.html_url }}
69+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

.github/workflows/publish.yml

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,14 @@ jobs:
1818
steps:
1919
- uses: actions/checkout@v6
2020
- uses: ./.github/actions/setup
21+
# Publish-side gating is intentionally minimal: lint + typecheck +
22+
# build. The full test suite (incl. Playwright) already ran in CI
23+
# against the same commit before release-please merged it to main,
24+
# and re-installing Chromium on the npm-token-bearing job widens
25+
# the supply-chain surface for no security gain. Anything that
26+
# would fail here would have failed CI first.
2127
- run: pnpm run lint
2228
- run: pnpm run typecheck
23-
- name: Install Playwright Chromium
24-
run: pnpm exec playwright install --with-deps chromium
25-
- run: pnpm test
2629
- run: pnpm run build:clean
2730
- run: pnpm publish --provenance --access public --no-git-checks
2831
env:

SECURITY-REVIEW.md

Lines changed: 199 additions & 0 deletions
Large diffs are not rendered by default.

src/components/editors/rich-text-editor.tsx

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { Button } from '../../lib/ui/button';
2727
import { Input } from '../../lib/ui/input';
2828
import { Label } from '../../lib/ui/label';
2929
import { Popover, PopoverContent, PopoverTrigger } from '../../lib/ui/popover';
30+
import { isSafeHref } from '../../lib/url-safety';
3031
import { RichTextStructuredEditor } from './rich-text-structured-editor';
3132
import type { BlockEditorProps } from './types';
3233

@@ -106,6 +107,15 @@ function RichTextWysiwygEditor({ block, onChange }: { block: RichTextBlock; onCh
106107
openOnClick: false,
107108
autolink: true,
108109
defaultProtocol: 'https',
110+
// Pin TipTap's link allowlist to the same set as our shared
111+
// `isSafeHref` helper. The upstream default also includes
112+
// ftp/cid/callto which we don't expect inside Slack content.
113+
protocols: ['http', 'https', 'mailto', 'tel', 'sms', 'xmpp'],
114+
// Belt-and-suspenders: even if a downstream contributor widens
115+
// `protocols` later, our isSafeHref guard rejects everything
116+
// outside the safe-link set. setLink and toggleLink both gate
117+
// on this hook before applying the mark.
118+
isAllowedUri: (url) => isSafeHref(url),
109119
HTMLAttributes: { rel: 'noreferrer noopener', target: '_blank' }
110120
})
111121
],
@@ -284,20 +294,27 @@ function LinkPopover({ editor }: { editor: Editor }) {
284294
const currentHref = (editor.getAttributes('link').href as string | undefined) ?? '';
285295
const [open, setOpen] = useState(false);
286296
const [url, setUrl] = useState(currentHref);
297+
const [error, setError] = useState<string | null>(null);
287298

288299
useEffect(() => {
289300
if (open) {
290301
setUrl(currentHref);
302+
setError(null);
291303
}
292304
}, [open, currentHref]);
293305

294306
const apply = () => {
295307
const trimmed = url.trim();
296308
if (!trimmed) {
297309
editor.chain().focus().unsetLink().run();
298-
} else {
299-
editor.chain().focus().extendMarkRange('link').setLink({ href: trimmed }).run();
310+
setOpen(false);
311+
return;
312+
}
313+
if (!isSafeHref(trimmed)) {
314+
setError('Only http(s), mailto, tel, sms, and xmpp links are allowed.');
315+
return;
300316
}
317+
editor.chain().focus().extendMarkRange('link').setLink({ href: trimmed }).run();
301318
setOpen(false);
302319
};
303320

@@ -325,14 +342,20 @@ function LinkPopover({ editor }: { editor: Editor }) {
325342
id="rt-link-url"
326343
value={url}
327344
placeholder="https://example.com"
328-
onChange={(e) => setUrl(e.target.value)}
345+
onChange={(e) => {
346+
setUrl(e.target.value);
347+
if (error) {
348+
setError(null);
349+
}
350+
}}
329351
onKeyDown={(e) => {
330352
if (e.key === 'Enter') {
331353
e.preventDefault();
332354
apply();
333355
}
334356
}}
335357
/>
358+
{error && <p className="text-[11px] text-destructive">{error}</p>}
336359
<div className="flex justify-between">
337360
{active ? (
338361
<Button

src/components/editors/rich-text-structured-editor.tsx

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { Button } from '../../lib/ui/button';
1616
import { Input } from '../../lib/ui/input';
1717
import { Label } from '../../lib/ui/label';
1818
import { RadioGroup, RadioGroupItem } from '../../lib/ui/radio-group';
19+
import { isSafeHref } from '../../lib/url-safety';
1920
import { EditorField } from './field';
2021
import type { BlockEditorProps } from './types';
2122

@@ -454,16 +455,25 @@ function InlineFields({
454455

455456
if (kind === 'link') {
456457
const link = element as RichTextSectionLink;
458+
const urlValue = link.url ?? '';
459+
const urlIsUnsafe = urlValue.length > 0 && !isSafeHref(urlValue);
457460
return (
458461
<div className="flex flex-col gap-2">
459462
<EditorField label="URL" htmlFor={`${idPrefix}-url`}>
460463
<Input
461464
id={`${idPrefix}-url`}
462465
type="url"
463-
value={link.url ?? ''}
466+
value={urlValue}
464467
placeholder="e.g. https://slack.com"
465468
onChange={(e) => onChange({ ...link, url: e.target.value })}
469+
aria-invalid={urlIsUnsafe || undefined}
466470
/>
471+
{urlIsUnsafe && (
472+
<p className="mt-1 text-[11px] text-destructive">
473+
Only http(s), mailto, tel, sms, and xmpp links are allowed. This URL will be stripped before send and
474+
preview.
475+
</p>
476+
)}
467477
</EditorField>
468478
<EditorField
469479
label="Display text"

src/components/json-drawer.tsx

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@ import { useEffect, useMemo, useRef, useState } from 'react';
33
import { Sheet, SheetContent, SheetDescription, SheetTitle } from '../lib/ui/sheet';
44
import type { SupportedBlock } from '../types';
55

6+
/**
7+
* Upper bound on the JSON textarea we accept. A pasted multi-megabyte
8+
* payload would freeze the tab inside `JSON.parse` before the validator
9+
* gets a look; 1 MiB is well above any realistic Slack Block Kit message
10+
* (Slack itself caps blocks at 50 per message and ~3000 chars per text
11+
* field).
12+
*/
13+
const MAX_JSON_BYTES = 1024 * 1024;
14+
615
/**
716
* Side drawer that exposes the current draft as raw JSON in a full-height
817
* code-editor-style textarea. Edits flow live: every valid parse updates
@@ -59,6 +68,11 @@ export function JsonDrawer({
5968

6069
const handleChange = (next: string) => {
6170
setValue(next);
71+
if (next.length > MAX_JSON_BYTES) {
72+
setParseError(`JSON exceeds the ${Math.round(MAX_JSON_BYTES / 1024)} KiB editor limit.`);
73+
setValidationErrors([]);
74+
return;
75+
}
6276
let parsed: unknown;
6377
try {
6478
parsed = JSON.parse(next);

src/components/preview/slack-block-preview.tsx

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import 'slack-blocks-to-jsx/dist/style.css';
22

3-
import { useEffect, useRef } from 'react';
3+
import { useEffect, useMemo, useRef } from 'react';
44
import type { Block } from 'slack-blocks-to-jsx';
55
import { Message } from 'slack-blocks-to-jsx';
6+
import { sanitizeBlock } from '../../lib/sanitize-blocks';
7+
import { isSafeHref, isSafeImageSrc } from '../../lib/url-safety';
68
import type { PreviewHooks, PreviewTheme, SupportedBlock } from '../../types';
79

810
/**
@@ -33,10 +35,28 @@ export function SlackBlockPreview({
3335
}) {
3436
const rootRef = useRef<HTMLDivElement>(null);
3537

38+
// Strip dangerous URI schemes (`javascript:`, `data:text/html`, etc.)
39+
// from every `url`/`image_url` field in the block before handing it
40+
// to slack-blocks-to-jsx, which renders rich-text links, button URLs,
41+
// and image sources directly into `<a href>` / `<img src>` without
42+
// its own scheme filter. Memoized so an unchanged block keeps the
43+
// same reference and doesn't churn the renderer.
44+
const safeBlock = useMemo(() => sanitizeBlock(block), [block]);
45+
3646
// slack-blocks-to-jsx renders an SVG-only collapse toggle in image and
3747
// video blocks without an aria-label, which violates axe's `button-name`
3848
// rule and is unreachable to screen readers. Post-mount we add a label
39-
// to any such buttons we find under our wrapper.
49+
// to any such buttons we find under our wrapper. We also do a final
50+
// pass to neutralize any `<a href>` or `<img src>` that carries a
51+
// disallowed URI scheme — the block-payload sanitizer catches URLs
52+
// that live in structured fields (`url`, `image_url`), but mrkdwn /
53+
// rich-text content can encode link URLs inside text strings
54+
// (`[label](javascript:...)` or `<javascript:...|label>`) that
55+
// `slack-blocks-to-jsx`'s own parser hands straight to `<a href>`
56+
// without filtering. React 19 also blocks `javascript:` URLs at
57+
// setAttribute time, but we don't rely on that — this loop applies
58+
// our allowlist (which is tighter and covers `data:`/`vbscript:`/`file:`
59+
// as well) and replaces unsafe values with `#`.
4060
useEffect(() => {
4161
const root = rootRef.current;
4262
if (!root) return;
@@ -48,6 +68,20 @@ export function SlackBlockPreview({
4868
btn.setAttribute('aria-label', title ? `Toggle ${title}` : 'Toggle media');
4969
}
5070
}
71+
for (const a of root.querySelectorAll<HTMLAnchorElement>('a[href]')) {
72+
const href = a.getAttribute('href');
73+
if (!isSafeHref(href)) {
74+
a.setAttribute('href', '#');
75+
a.setAttribute('data-bk-blocked-href', '1');
76+
}
77+
}
78+
for (const img of root.querySelectorAll<HTMLImageElement>('img[src]')) {
79+
const src = img.getAttribute('src');
80+
if (!isSafeImageSrc(src)) {
81+
img.removeAttribute('src');
82+
img.setAttribute('data-bk-blocked-src', '1');
83+
}
84+
}
5185
});
5286

5387
return (
@@ -64,7 +98,7 @@ export function SlackBlockPreview({
6498
logo=""
6599
withoutWrapper
66100
theme={theme}
67-
blocks={[block as unknown as Block]}
101+
blocks={[safeBlock as unknown as Block]}
68102
hooks={hooks as Record<string, unknown> | undefined}
69103
/>
70104
</div>

src/components/send-dialog.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { toSlackBlocks } from '../lib/to-slack-blocks';
44
import { Button } from '../lib/ui/button';
55
import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from '../lib/ui/dialog';
66
import { Label } from '../lib/ui/label';
7+
import { isSafeHref } from '../lib/url-safety';
78
import type { ChannelOption, SendAsUserStatus, SendPayload, SupportedBlock } from '../types';
89

910
type SendStatus = { kind: 'idle' } | { kind: 'sending' } | { kind: 'success' } | { kind: 'error'; error: string };
@@ -190,12 +191,12 @@ export function SendDialog({
190191
{userStatus && !userStatus.canSendAsUser ? ' (Slack sign-in required)' : ''}
191192
</option>
192193
</select>
193-
{userStatus && !userStatus.canSendAsUser && userStatus.oauthUrl && (
194+
{userStatus && !userStatus.canSendAsUser && userStatus.oauthUrl && isSafeHref(userStatus.oauthUrl) && (
194195
<p className="text-xs text-muted-foreground">
195196
<a
196197
href={userStatus.oauthUrl}
197198
target="_blank"
198-
rel="noreferrer"
199+
rel="noopener noreferrer"
199200
className="inline-flex items-center gap-1 text-primary hover:underline"
200201
>
201202
Sign in with Slack <ExternalLink className="h-3 w-3" />

src/lib/rich-text-tiptap.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type {
55
RichTextSectionElementStyleWithCode,
66
RichTextSectionLink
77
} from 'slack-web-api-client';
8+
import { sanitizeHref } from './url-safety';
89

910
type RichStyle = RichTextSectionElementStyleWithCode;
1011

@@ -415,7 +416,12 @@ function proseMirrorInlinesToRichTextElements(nodes: PMNode[]): AnyRichTextSecti
415416
const linkMark = node.marks?.find((m) => m.type === 'link');
416417
const style = marksToStyle(node.marks ?? []);
417418
if (linkMark) {
418-
const url = String(linkMark.attrs?.href ?? '');
419+
// TipTap's setLink/toggleLink already gate on isAllowedUri, but
420+
// a link mark can also enter the editor via setContent() (used
421+
// when seeding from a payload). Sanitize once more here so a
422+
// crafted Slack rich_text payload that already contains an unsafe
423+
// href cannot round-trip back out unchanged.
424+
const url = sanitizeHref(String(linkMark.attrs?.href ?? ''));
419425
const link: RichTextSectionLink = {
420426
type: 'link',
421427
url,

0 commit comments

Comments
 (0)