From 191ee9a5f83ea6748cf51c8069b57cc8aeef0532 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 02:48:19 +0000 Subject: [PATCH 1/2] fix(shared): return a value that fails to percent-decode as-is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `content-disposition` with a hostile `filename*`, such as `%FFa` repeated, made `safeDecodeURIComponent` throw and catch one `URIError` per 4 bytes: after the whole value failed to decode, each run of `%XX` escapes was retried on its own. That cost ~14 ms per 16 KB header on fetch, even for a plain `application/json` request, and ~0.8 s for a 1 MB peer message, which has no header size limit. - `safeDecodeURIComponent` returns a value that fails to decode as-is again, so it throws at most once. `decodeURIComponent` stops at the first malformed escape, so this is cheap whatever the input. - `resolveStandardBodyHint` parses `content-disposition` only when no other header decides the hint, so json, form-data, url-search-params, event-stream and sized bodies never decode the filename. ## Behaviour change This undoes the per-run fallback from 60d865e. One malformed escape now keeps the whole filename encoded: `filename*=utf-8''%E4%B8%AD%ZZ.txt` gives `%E4%B8%AD%ZZ.txt` instead of `中%ZZ.txt`. Well-formed values decode as before. Only senders that do not encode `%` as `%25` are affected. ## Performance | Hostile input | Before | After | |---|---|---| | fetch, `application/json`, 16 KB header | 14 ms | 0.24 ms | | peer, binary message, 1 MB header | 808 ms | 2 ms | Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KkyGkYHZGmXB18XUR6fFbF --- packages/core/src/utils.test.ts | 64 +++++++++++++++++++++++++++++++-- packages/core/src/utils.ts | 17 +++++---- packages/peer/src/body.test.ts | 29 +++++++++++++++ packages/shared/src/uri.test.ts | 33 ++++++++++++----- packages/shared/src/uri.ts | 13 ++----- 5 files changed, 128 insertions(+), 28 deletions(-) diff --git a/packages/core/src/utils.test.ts b/packages/core/src/utils.test.ts index 2b43ae4f..fac39ca0 100644 --- a/packages/core/src/utils.test.ts +++ b/packages/core/src/utils.test.ts @@ -197,11 +197,13 @@ it('getFilenameFromContentDisposition', () => { ) expect(getFilenameFromContentDisposition('inline; filename="a\\"')).toEqual(undefined) - // malformed escapes in filename* are kept as-is, the valid ones are still decoded + // a filename* that fails to decode is kept as-is expect( getFilenameFromContentDisposition("attachment; filename*=utf-8''%E4%B8%AD%ZZ.txt"), - ).toEqual('中%ZZ.txt') - expect(getFilenameFromContentDisposition('inline; filename*=%E2%82%AC%.txt')).toEqual('€%.txt') + ).toEqual('%E4%B8%AD%ZZ.txt') + expect(getFilenameFromContentDisposition('inline; filename*=%E2%82%AC%.txt')).toEqual( + '%E2%82%AC%.txt', + ) // quoted filename* is tolerated expect( @@ -216,6 +218,10 @@ it('getFilenameFromContentDisposition', () => { }) describe('resolveStandardBodyHint', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + it('standard-server header wins over content headers', () => { expect( resolveStandardBodyHint({ @@ -405,6 +411,58 @@ describe('resolveStandardBodyHint', () => { ).toBe('none') }) + it('decodes the filename only when no other header decides the hint', () => { + const decodeSpy = vi.spyOn(globalThis, 'decodeURIComponent') + // just under the 16 KiB header limit of Node's HTTP server + const contentDisposition = `attachment; filename*=utf-8''${'%FFa'.repeat(3900)}` + + expect( + resolveStandardBodyHint({ + 'content-type': 'application/json', + 'content-disposition': contentDisposition, + }), + ).toBe('json') + expect( + resolveStandardBodyHint({ + 'content-type': 'multipart/form-data; boundary=x', + 'content-disposition': contentDisposition, + }), + ).toBe('form-data') + expect( + resolveStandardBodyHint({ + 'content-type': 'application/x-www-form-urlencoded', + 'content-disposition': contentDisposition, + }), + ).toBe('url-search-params') + expect( + resolveStandardBodyHint({ + 'content-type': 'text/event-stream', + 'content-disposition': contentDisposition, + }), + ).toBe('event-stream') + expect( + resolveStandardBodyHint({ + 'standard-server': 'json', + 'content-disposition': contentDisposition, + }), + ).toBe('json') + expect(resolveStandardBodyHint({ 'content-disposition': contentDisposition })).toBe('none') + // content-length is enough to tell a file + expect( + resolveStandardBodyHint({ 'content-length': '3', 'content-disposition': contentDisposition }), + ).toBe('file') + expect(decodeSpy).not.toHaveBeenCalled() + + // left to the filename alone, a malformed one is still a filename + expect( + resolveStandardBodyHint({ + 'content-type': 'application/pdf', + 'content-disposition': contentDisposition, + }), + ).toBe('file') + expect(decodeSpy).toHaveBeenCalled() + }) + it('octet-stream when content-length is absent', () => { expect(resolveStandardBodyHint({ 'content-type': 'application/octet-stream' })).toBe( 'octet-stream', diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index 1e6252c8..cd88f688 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -119,11 +119,6 @@ export function resolveStandardBodyHint(headers: { ?.trim() .toLowerCase() const contentLength = flattenStandardHeader(headers['content-length']) - const contentDisposition = flattenStandardHeader(headers['content-disposition']) - const fileName = - contentDisposition !== undefined - ? getFilenameFromContentDisposition(contentDisposition) - : undefined if (mimeType === undefined && (contentLength === undefined || contentLength === '0')) { return 'none' @@ -145,7 +140,17 @@ export function resolveStandardBodyHint(headers: { return 'event-stream' } - if (fileName !== undefined || contentLength !== undefined) { + if (contentLength !== undefined) { + return 'file' + } + + // parsed last, so a request the checks above already decide never pays for decoding the filename + const contentDisposition = flattenStandardHeader(headers['content-disposition']) + + if ( + contentDisposition !== undefined && + getFilenameFromContentDisposition(contentDisposition) !== undefined + ) { return 'file' } diff --git a/packages/peer/src/body.test.ts b/packages/peer/src/body.test.ts index 892b96aa..d3965a48 100644 --- a/packages/peer/src/body.test.ts +++ b/packages/peer/src/body.test.ts @@ -6,6 +6,10 @@ import { encodeAtomicStandardBody, toStandardBody } from './body' import type { PeerRequestMessage } from './types' describe('toStandardBody', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + function makeMessage(options: { bodyHint?: StandardBodyHint contentType?: string @@ -158,6 +162,31 @@ describe('toStandardBody', () => { expect(cleanup).toHaveBeenCalledWith({ kind: 'success' }) }) + it('keeps a malformed filename as-is without retrying it once per escape', async () => { + const decodeSpy = vi.spyOn(globalThis, 'decodeURIComponent') + const filename = '%FFa'.repeat(262_144) // 1 MiB, peer messages have no header size limit + const cleanup = vi.fn() + const { resolveBody } = toStandardBody( + makeMessage({ + contentType: 'text/plain', + contentDisposition: `attachment; filename*=utf-8''${filename}`, + binary: new TextEncoder().encode('file content'), + }), + cleanup, + ) + + const body = (await resolveBody()) as File + + expect(body).toBeInstanceOf(File) + expect(body.name).toBe(filename) + expect(await body.text()).toBe('file content') + // not retried in parts, which would throw once per malformed escape + expect(decodeSpy).toHaveBeenCalledTimes(1) + + expect(cleanup).toHaveBeenCalledTimes(1) + expect(cleanup).toHaveBeenCalledWith({ kind: 'success' }) + }) + it('receives an empty file', async () => { const cleanup = vi.fn() const { resolveBody } = toStandardBody( diff --git a/packages/shared/src/uri.test.ts b/packages/shared/src/uri.test.ts index eb41ca05..edc15a71 100644 --- a/packages/shared/src/uri.test.ts +++ b/packages/shared/src/uri.test.ts @@ -24,22 +24,37 @@ describe('safeEncodeURIComponent', () => { }) describe('safeDecodeURIComponent', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + it('decodes like decodeURIComponent for well-formed input', () => { expect(safeDecodeURIComponent('test')).toBe('test') expect(safeDecodeURIComponent('test%20value')).toBe('test value') expect(safeDecodeURIComponent('%CE%BB%E4%B8%96%E7%95%8C')).toBe('λ世界') + expect(safeDecodeURIComponent('%f0%9f%98%80')).toBe('😀') expect(safeDecodeURIComponent('')).toBe('') }) - it('keeps malformed escapes as-is and still decodes the valid ones instead of throwing', () => { - expect(safeDecodeURIComponent('invalid%20value%')).toBe('invalid value%') - expect(safeDecodeURIComponent('%E0%A4%A')).toBe('%E0%A4%A') // Invalid UTF-8 sequence + it('returns a value that fails to decode as-is instead of throwing', () => { + expect(safeDecodeURIComponent('invalid%20value%')).toBe('invalid%20value%') expect(safeDecodeURIComponent('%ZZ')).toBe('%ZZ') - expect(safeDecodeURIComponent('%ZZ%e4%b8%ad%')).toBe('%ZZ中%') - expect(safeDecodeURIComponent('%FF-%E2%82%AC')).toBe('%FF-€') - // a run of escapes that is not valid UTF-8 is kept as a whole - expect(safeDecodeURIComponent('%E4%B8%AD%FF%')).toBe('%E4%B8%AD%FF%') - // decoded output is not decoded again - expect(safeDecodeURIComponent('%2541%ZZ')).toBe('%41%ZZ') + // malformed UTF-8: truncated, overlong, surrogate, above U+10FFFF + expect(safeDecodeURIComponent('%E4%B8')).toBe('%E4%B8') + expect(safeDecodeURIComponent('%C0%AF')).toBe('%C0%AF') + expect(safeDecodeURIComponent('%ED%A0%80')).toBe('%ED%A0%80') + expect(safeDecodeURIComponent('%F4%90%80%80')).toBe('%F4%90%80%80') + // one malformed escape keeps the valid ones encoded too + expect(safeDecodeURIComponent('%E4%B8%AD%ZZ')).toBe('%E4%B8%AD%ZZ') + expect(safeDecodeURIComponent('%FF-%E2%82%AC')).toBe('%FF-%E2%82%AC') + }) + + it('tries decodeURIComponent once on hostile input', () => { + const decodeSpy = vi.spyOn(globalThis, 'decodeURIComponent') + const hostile = '%FFa'.repeat(250_000) // ~1 MB + + expect(safeDecodeURIComponent(hostile)).toBe(hostile) + // not retried in parts, which would throw once per malformed escape + expect(decodeSpy).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/shared/src/uri.ts b/packages/shared/src/uri.ts index d16700d2..257cad7a 100644 --- a/packages/shared/src/uri.ts +++ b/packages/shared/src/uri.ts @@ -1,6 +1,5 @@ // with the u flag a well-formed surrogate pair is one code point, so only lone surrogates match const LONE_SURROGATE_REGEX = /[\uD800-\uDFFF]/gu -const PERCENT_ESCAPES_REGEX = /(?:%[0-9A-F]{2})+/gi /** * `encodeURIComponent` that never throws, lone surrogates become U+FFFD. @@ -16,7 +15,7 @@ export function safeEncodeURIComponent(value: string): string { } /** - * `decodeURIComponent` that never throws, runs of `%XX` escapes that fail to decode are kept as-is. + * `decodeURIComponent` that never throws, a value that fails to decode is returned as-is. */ export function safeDecodeURIComponent(value: string): string { if (!value.includes('%')) { @@ -27,13 +26,7 @@ export function safeDecodeURIComponent(value: string): string { // oxlint-disable-next-line no-restricted-globals return decodeURIComponent(value) } catch { - return value.replace(PERCENT_ESCAPES_REGEX, (escapes) => { - try { - // oxlint-disable-next-line no-restricted-globals - return decodeURIComponent(escapes) - } catch { - return escapes - } - }) + // retrying in parts would let a value with many malformed escapes cost one thrown URIError each + return value } } From 04766905231bb88ee8e9ec83e9357394294d66f8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 02:59:21 +0000 Subject: [PATCH 2/2] test: drop comments the tests already cover and condense the hint test - Remove comments that restate a test name or the assertion below them, including the two source comments the new tests pin. - Check the content types that skip the filename decode in one loop instead of seven near-identical blocks. - Restore mocks in one top-level `afterEach` in the core utils tests, next to the existing `beforeEach`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KkyGkYHZGmXB18XUR6fFbF --- packages/core/src/utils.test.ts | 57 ++++++++++----------------------- packages/core/src/utils.ts | 1 - packages/peer/src/body.test.ts | 1 - packages/shared/src/uri.test.ts | 4 +-- packages/shared/src/uri.ts | 1 - 5 files changed, 18 insertions(+), 46 deletions(-) diff --git a/packages/core/src/utils.test.ts b/packages/core/src/utils.test.ts index fac39ca0..1f74d465 100644 --- a/packages/core/src/utils.test.ts +++ b/packages/core/src/utils.test.ts @@ -14,6 +14,10 @@ beforeEach(() => { vi.clearAllMocks() }) +afterEach(() => { + vi.restoreAllMocks() +}) + describe('generateContentDisposition', () => { it('handle normal filename', () => { expect(generateContentDisposition('test.txt')).toEqual( @@ -218,10 +222,6 @@ it('getFilenameFromContentDisposition', () => { }) describe('resolveStandardBodyHint', () => { - afterEach(() => { - vi.restoreAllMocks() - }) - it('standard-server header wins over content headers', () => { expect( resolveStandardBodyHint({ @@ -416,44 +416,21 @@ describe('resolveStandardBodyHint', () => { // just under the 16 KiB header limit of Node's HTTP server const contentDisposition = `attachment; filename*=utf-8''${'%FFa'.repeat(3900)}` - expect( - resolveStandardBodyHint({ - 'content-type': 'application/json', - 'content-disposition': contentDisposition, - }), - ).toBe('json') - expect( - resolveStandardBodyHint({ - 'content-type': 'multipart/form-data; boundary=x', - 'content-disposition': contentDisposition, - }), - ).toBe('form-data') - expect( - resolveStandardBodyHint({ - 'content-type': 'application/x-www-form-urlencoded', - 'content-disposition': contentDisposition, - }), - ).toBe('url-search-params') - expect( - resolveStandardBodyHint({ - 'content-type': 'text/event-stream', - 'content-disposition': contentDisposition, - }), - ).toBe('event-stream') - expect( - resolveStandardBodyHint({ - 'standard-server': 'json', - 'content-disposition': contentDisposition, - }), - ).toBe('json') - expect(resolveStandardBodyHint({ 'content-disposition': contentDisposition })).toBe('none') - // content-length is enough to tell a file - expect( - resolveStandardBodyHint({ 'content-length': '3', 'content-disposition': contentDisposition }), - ).toBe('file') + for (const [headers, hint] of [ + [{ 'content-type': 'application/json' }, 'json'], + [{ 'content-type': 'multipart/form-data; boundary=x' }, 'form-data'], + [{ 'content-type': 'application/x-www-form-urlencoded' }, 'url-search-params'], + [{ 'content-type': 'text/event-stream' }, 'event-stream'], + [{ 'standard-server': 'json' }, 'json'], + [{}, 'none'], + [{ 'content-length': '3' }, 'file'], + ] as const) { + expect( + resolveStandardBodyHint({ ...headers, 'content-disposition': contentDisposition }), + ).toBe(hint) + } expect(decodeSpy).not.toHaveBeenCalled() - // left to the filename alone, a malformed one is still a filename expect( resolveStandardBodyHint({ 'content-type': 'application/pdf', diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index cd88f688..b8fbc42b 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -144,7 +144,6 @@ export function resolveStandardBodyHint(headers: { return 'file' } - // parsed last, so a request the checks above already decide never pays for decoding the filename const contentDisposition = flattenStandardHeader(headers['content-disposition']) if ( diff --git a/packages/peer/src/body.test.ts b/packages/peer/src/body.test.ts index d3965a48..ee694676 100644 --- a/packages/peer/src/body.test.ts +++ b/packages/peer/src/body.test.ts @@ -180,7 +180,6 @@ describe('toStandardBody', () => { expect(body).toBeInstanceOf(File) expect(body.name).toBe(filename) expect(await body.text()).toBe('file content') - // not retried in parts, which would throw once per malformed escape expect(decodeSpy).toHaveBeenCalledTimes(1) expect(cleanup).toHaveBeenCalledTimes(1) diff --git a/packages/shared/src/uri.test.ts b/packages/shared/src/uri.test.ts index edc15a71..a3cc4b5e 100644 --- a/packages/shared/src/uri.test.ts +++ b/packages/shared/src/uri.test.ts @@ -44,17 +44,15 @@ describe('safeDecodeURIComponent', () => { expect(safeDecodeURIComponent('%C0%AF')).toBe('%C0%AF') expect(safeDecodeURIComponent('%ED%A0%80')).toBe('%ED%A0%80') expect(safeDecodeURIComponent('%F4%90%80%80')).toBe('%F4%90%80%80') - // one malformed escape keeps the valid ones encoded too expect(safeDecodeURIComponent('%E4%B8%AD%ZZ')).toBe('%E4%B8%AD%ZZ') expect(safeDecodeURIComponent('%FF-%E2%82%AC')).toBe('%FF-%E2%82%AC') }) it('tries decodeURIComponent once on hostile input', () => { const decodeSpy = vi.spyOn(globalThis, 'decodeURIComponent') - const hostile = '%FFa'.repeat(250_000) // ~1 MB + const hostile = '%FFa'.repeat(250_000) expect(safeDecodeURIComponent(hostile)).toBe(hostile) - // not retried in parts, which would throw once per malformed escape expect(decodeSpy).toHaveBeenCalledTimes(1) }) }) diff --git a/packages/shared/src/uri.ts b/packages/shared/src/uri.ts index 257cad7a..ba31f308 100644 --- a/packages/shared/src/uri.ts +++ b/packages/shared/src/uri.ts @@ -26,7 +26,6 @@ export function safeDecodeURIComponent(value: string): string { // oxlint-disable-next-line no-restricted-globals return decodeURIComponent(value) } catch { - // retrying in parts would let a value with many malformed escapes cost one thrown URIError each return value } }