From 0824d4e63431c72d940963019324b7c74f0835f3 Mon Sep 17 00:00:00 2001 From: M <> Date: Mon, 14 Sep 2026 21:38:10 +0200 Subject: [PATCH] CON-47: check the scheme of the url a Blossom server returns, and escape it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server answers an upload with a descriptor whose `url` was taken at its word and written straight into page content. `javascript:` and `data:` parse as perfectly valid URLs, so it is the protocol check and not the parse that rejects them. Our own renderer refuses those schemes and the server is operator-configured, so this is the second line — what is *stored* in a page should be sound for every future reader, not only for this client. The string handed on is the exact one the parser validated: `new URL` ignores leading and trailing C0 controls and spaces, so passing the unstripped string on would store `%20https://x/a%20` — a destination that decodes to a relative path with a space in it, a broken link built out of a URL that was fine. Not `trim()`, which would also strip a trailing U+00A0 that is part of the blob's name. The destination is percent-encoded where Markdown would end the link early: a bare `)` closes it, whitespace starts the optional title, `<`/`>` are the other destination form and a backslash escapes what follows. Encoded over UTF-8 bytes, because `\s` matches non-ASCII whitespace too and encoding U+2003 from its code unit yields `%2003` — `%20` followed by a literal `03`, pointing the link somewhere else entirely. The label was already guarded from the other side; both halves now happen at insertion time. --- docs/09-security-privacy.md | 1 + src/nostr/blossom.test.ts | 93 ++++++++++++++++++++++++++++++- src/nostr/blossom.ts | 106 +++++++++++++++++++++++++++++++++++- 3 files changed, 196 insertions(+), 4 deletions(-) diff --git a/docs/09-security-privacy.md b/docs/09-security-privacy.md index 8aeffc8..c5dd004 100644 --- a/docs/09-security-privacy.md +++ b/docs/09-security-privacy.md @@ -35,6 +35,7 @@ | Risk | Countermeasure | |---|---| | XSS through Markdown from arbitrary npubs | `rehype-sanitize` with a strict allowlist, no `dangerouslySetInnerHTML`, no raw HTML, no `javascript:` links | +| The `url` a Blossom server returns for an upload (`javascript:`, `data:`, or one that ends the Markdown link early) | Accepted only if it parses as an absolute `http(s)` URL, otherwise the blob is addressed by its own sha256; the link destination is percent-encoded at insertion time exactly as the label already was. Second line: the renderer refuses those schemes anyway and the server is operator-configured — this is about what gets *stored*, which every other client reads too (`src/nostr/blossom.ts`) | | Images/iframes used as trackers | **Accepted trade:** every image is loaded directly, whatever host it points at — so a host learns the reader's IP, which page is being read and when, and can count reads. No iframes. See "Images are loaded directly" below | | Forged `h` tags (an event from another group smuggled in) | Checked after loading: `h` must match the open space, otherwise the event is discarded | | Forgetting to verify signatures | Verification is enforced in the data layer, not optional per call | diff --git a/src/nostr/blossom.test.ts b/src/nostr/blossom.test.ts index 6d1e662..e9f7072 100644 --- a/src/nostr/blossom.test.ts +++ b/src/nostr/blossom.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { attachmentMarkdown } from './blossom' +import { attachmentMarkdown, resolveAttachmentUrl } from './blossom' describe('attachmentMarkdown', () => { it('embeds images', () => { @@ -30,4 +30,95 @@ describe('attachmentMarkdown', () => { '[attachment](http://x/abc)', ) }) + + // The other half of the same problem: a `)` ends the destination just as a + // `]` ends the label, and whitespace starts the link title behind it. + it('encodes the characters that would end the destination early', () => { + expect(attachmentMarkdown({ url: 'https://x/a)b.png', type: 'image/png' }, 'p.png')).toBe( + '![p.png](https://x/a%29b.png)', + ) + + expect(attachmentMarkdown({ url: 'https://x/a b.png', type: 'image/png' }, 'p.png')).toBe( + '![p.png](https://x/a%20b.png)', + ) + expect(attachmentMarkdown({ url: 'https://x/ac\\d(e.png', type: '' }, 'p.png')).toBe( + '[p.png](https://x/a%3Cb%3Ec%5Cd%28e.png)', + ) + }) + + // `\s` matches more than a space, and percent-encoding is defined over UTF-8 + // bytes: from its UTF-16 code unit, U+00A0 would come out as the undecodable + // `%A0` and U+2003 as `%2003`, which decodes to `%20` plus a literal `03`. + it('encodes non-ASCII whitespace as the bytes a reader can decode again', () => { + expect(attachmentMarkdown({ url: 'https://x/a\u00a0b.png', type: 'image/png' }, 'p.png')).toBe( + '![p.png](https://x/a%C2%A0b.png)', + ) + expect(attachmentMarkdown({ url: 'https://x/a\u2003b.png', type: 'image/png' }, 'p.png')).toBe( + '![p.png](https://x/a%E2%80%83b.png)', + ) + expect(decodeURIComponent('https://x/a%C2%A0b.png')).toBe('https://x/a\u00a0b.png') + }) + + it('leaves an ordinary url byte for byte alone', () => { + const url = 'https://blossom.example/a1b2c3.png?v=2&x=1' + expect(attachmentMarkdown({ url, type: 'image/png' }, 'p.png')).toBe(`![p.png](${url})`) + }) +}) + +describe('resolveAttachmentUrl', () => { + // `new URL` parses these happily, so the protocol check is what rejects them. + it('refuses a scheme that is not http(s) and addresses the blob by its hash instead', () => { + expect(resolveAttachmentUrl({ url: 'javascript:alert(1)' }, 'abc', 'https://b.example')).toBe( + 'https://b.example/abc', + ) + expect(resolveAttachmentUrl({ url: 'data:text/html,x' }, 'abc', 'https://b.example')).toBe( + 'https://b.example/abc', + ) + }) + + it('keeps an absolute http(s) url the server returned', () => { + expect(resolveAttachmentUrl({ url: 'https://cdn.example/abc' }, 'abc', 'https://b.example')).toBe( + 'https://cdn.example/abc', + ) + expect(resolveAttachmentUrl({ url: 'http://cdn.example/abc' }, 'abc', 'https://b.example')).toBe( + 'http://cdn.example/abc', + ) + }) + + it('falls back when there is no usable url at all', () => { + expect(resolveAttachmentUrl({}, 'abc', 'https://b.example')).toBe('https://b.example/abc') + expect(resolveAttachmentUrl({ url: 42 }, 'abc', 'https://b.example')).toBe('https://b.example/abc') + expect(resolveAttachmentUrl(null, 'abc', 'https://b.example')).toBe('https://b.example/abc') + }) + + // A relative url throws in `new URL` and takes the same way out. + it('falls back for a relative url', () => { + expect(resolveAttachmentUrl({ url: '../relative' }, 'abc', 'https://b.example')).toBe( + 'https://b.example/abc', + ) + }) + + // A configured server ending in `/` used to produce `https://b.example//abc`. + it('does not double the slash when the configured server ends in one', () => { + expect(resolveAttachmentUrl({}, 'abc', 'https://b.example/')).toBe('https://b.example/abc') + }) + + // `new URL` ignores this padding, so the gate passes; handing the padded + // string on would store `%20https://x/a%20` and break the link it accepted. + it('drops the padding the URL parser ignored', () => { + expect(resolveAttachmentUrl({ url: ' https://x/a ' }, 'abc', 'https://b.example')).toBe( + 'https://x/a', + ) + expect(resolveAttachmentUrl({ url: '\r\nhttps://x/a\t' }, 'abc', 'https://b.example')).toBe( + 'https://x/a', + ) + }) + + // …and no further: U+00A0 is part of the path to the URL parser, so removing + // it (as `trim()` would) would point the link at a different blob. + it('keeps Unicode whitespace the URL parser counts as part of the path', () => { + expect(resolveAttachmentUrl({ url: 'https://x/a\u00a0' }, 'abc', 'https://b.example')).toBe( + 'https://x/a\u00a0', + ) + }) }) diff --git a/src/nostr/blossom.ts b/src/nostr/blossom.ts index 4a47376..395234a 100644 --- a/src/nostr/blossom.ts +++ b/src/nostr/blossom.ts @@ -22,6 +22,8 @@ export function attachmentsEnabled(): boolean { /** Why an upload cannot happen without a server — shown, not swallowed. */ export const NO_BLOSSOM_SERVER = 'No Blossom server configured (VITE_BLOSSOM_SERVER).' +const UTF8 = new TextEncoder() + async function sha256Hex(data: ArrayBuffer): Promise { const digest = await crypto.subtle.digest('SHA-256', data) return [...new Uint8Array(digest)].map((byte) => byte.toString(16).padStart(2, '0')).join('') @@ -36,6 +38,68 @@ async function sha256Hex(data: ArrayBuffer): Promise { * suggest a gate that is no longer there. docs/09-security-privacy.md */ +/** + * The address an uploaded blob is reachable at. + * + * The server answers an upload with a descriptor, and its `url` is taken at its + * word — but only once it is an absolute http(s) URL. `javascript:alert(1)` and + * `data:text/html,…` parse as perfectly valid URLs, so it is the protocol check + * and not the parse that rejects them; a relative string throws and falls + * through the same way. Nothing executable gets through rendering either + * (rehype-sanitize and react-markdown's URL transform both refuse those + * schemes) and the server is operator-configured, not attacker-supplied — this + * is the second line, so that what is *stored* in a page is already sound for + * every future reader rather than only for this client's renderer. + * + * The fallback is deterministic: a blob is addressed by its sha256, so + * `/` is where it has to be. The trailing slash is stripped here + * exactly as the upload request strips it, so a `VITE_BLOSSOM_SERVER` that ends + * in `/` cannot produce a `//` in the stored URL. `server` is a parameter + * because the module-level constant is read from `import.meta.env` at load + * time, which a test cannot change afterwards. + */ +export function resolveAttachmentUrl( + descriptor: unknown, + hash: string, + server: string = BLOSSOM_SERVER, +): string { + const candidate = (descriptor as Record | null)?.url + if (typeof candidate === 'string') { + const subject = withoutUrlPadding(candidate) + try { + const url = new URL(subject) + if (url.protocol === 'http:' || url.protocol === 'https:') return subject + } catch { + /* not absolute — fall through to the deterministic path */ + } + } + return `${server.replace(/\/$/, '')}/${hash}` +} + +/** + * The padding the URL parser ignores, removed before the string is handed on. + * + * `new URL` discards leading and trailing C0 controls and spaces, so a url + * padded with a space passes the gate above — and passing the *unstripped* + * string on then stores `%20https://x/a%20`, a destination that decodes to a + * relative path with a literal space in it: a broken link built out of a URL + * that was fine. What is returned is therefore the exact string the parser + * validated. + * + * Not `trim()`, which also strips Unicode whitespace the URL parser treats as + * part of the path: a url ending in U+00A0 addresses a blob whose name ends in + * a no-break space, and trimming it would silently point the link elsewhere. + * Written as a loop rather than a regex because a character class over the C0 + * range is what `no-control-regex` exists to flag. + */ +function withoutUrlPadding(value: string): string { + let start = 0 + let end = value.length + while (start < end && value.charCodeAt(start) <= 0x20) start += 1 + while (end > start && value.charCodeAt(end - 1) <= 0x20) end -= 1 + return value.slice(start, end) +} + export async function uploadAttachment(signer: Signer, file: File): Promise { if (!attachmentsEnabled()) { return { ok: false, reason: NO_BLOSSOM_SERVER } @@ -74,8 +138,7 @@ export async function uploadAttachment(signer: Signer, file: File): Promise - const url = typeof descriptor.url === 'string' ? descriptor.url : `${BLOSSOM_SERVER}/${hash}` + const url = resolveAttachmentUrl(body, hash) return { ok: true, url, @@ -91,6 +154,37 @@ export async function uploadAttachment(signer: Signer, file: File): Promise` are the alternative `<…>` destination + * form, and a backslash escapes whatever follows. Percent-encoding them keeps + * the link working — a server decodes `%29` back to `)` when it resolves the + * path — while the stored text stays unambiguous. + * + * Percent-encoding is defined over UTF-8 *bytes*, not over UTF-16 code units, + * and `\s` also matches non-ASCII whitespace: encoding U+00A0 from its code + * unit yields `%A0`, which no reader can decode, and U+2003 yields `%2003`, + * which decodes to `%20` followed by a literal `03`. Both point the link at + * something other than the blob. Encoding the character's bytes is what makes + * the promise above true for every character the class matches. + * + * Deliberately not `encodeURIComponent` over the whole URL: it would also + * encode `:`, `/`, `?`, `&` and `=`, which buys nothing here and makes a + * stored page harder to read in a diff or a three-way merge. Not per matched + * character either — `encodeURIComponent` leaves `(` and `)` untouched, and + * those are the two characters this function exists for. + */ +function markdownDestination(url: string): string { + return url.replace(/[()<>\s\\]/g, (character) => + [...UTF8.encode(character)] + .map((byte) => '%' + byte.toString(16).toUpperCase().padStart(2, '0')) + .join(''), + ) +} + /** * Markdown embed for an uploaded file. * @@ -99,6 +193,11 @@ export async function uploadAttachment(signer: Signer, file: File): Promise