CON-47: check the scheme of the url a Blossom server returns

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 — and a Content-Security-Policy behind it as a second layer, see below |
+| 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 |

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/a<b>c\\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',
+    )
+  })
 })

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<string> {
   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<string> {
  * 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
+ * `<server>/<hash>` 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<string, unknown> | 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<UploadResult> {
   if (!attachmentsEnabled()) {
     return { ok: false, reason: NO_BLOSSOM_SERVER }
@@ -74,8 +138,7 @@ export async function uploadAttachment(signer: Signer, file: File): Promise<Uplo
     }
 
     const body: unknown = await response.json()
-    const descriptor = body as Record<string, unknown>
-    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<Uplo
   }
 }
 
+/**
+ * A destination that cannot end the link early.
+ *
+ * Every one of these characters means something between the parentheses of a
+ * Markdown link: a bare `)` closes the destination, whitespace begins the
+ * optional title behind it, `<` and `>` 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,12 +193,18 @@ export async function uploadAttachment(signer: Signer, file: File): Promise<Uplo
  * `notes].md` would produce a link whose target swallows the rest of the line.
  * Those characters are dropped and, if nothing readable is left, the label
  * falls back to a neutral word.
+ *
+ * The destination has the same problem from the other side, where a `)` ends
+ * the link just as effectively. Both are handled here, at insertion time: what
+ * gets stored has to be valid Markdown for whoever reads the page later, and a
+ * fix in our own renderer would only ever help us.
  */
 export function attachmentMarkdown(result: {
   url: string
   type: string
 }, name: string): string {
   const isImage = result.type.startsWith('image/')
   const label = name.replace(/[[\]\r\n]/g, '').trim() || 'attachment'
-  return isImage ? `![${label}](${result.url})` : `[${label}](${result.url})`
+  const destination = markdownDestination(result.url)
+  return isImage ? `![${label}](${destination})` : `[${label}](${destination})`
 }