diff --git a/apps/sim/app/api/files/utils.test.ts b/apps/sim/app/api/files/utils.test.ts index 3e4e2463cc3..b495df731d5 100644 --- a/apps/sim/app/api/files/utils.test.ts +++ b/apps/sim/app/api/files/utils.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest' -import { createFileResponse, extractFilename, findLocalFile } from '@/app/api/files/utils' +import { + createFileResponse, + encodeFilenameForHeader, + extractFilename, + findLocalFile, +} from '@/app/api/files/utils' describe('extractFilename', () => { describe('legitimate file paths', () => { @@ -317,6 +322,62 @@ describe('extractFilename', () => { }) }) + /** + * `originalName` is attacker-controlled — it only rejects path separators — so a + * quote can reach the header and close the quoted parameter early. RFC 6266 tells + * clients to prefer `filename*`, so an injected one decides the name the file + * lands under on disk regardless of what the product UI displayed. + */ + describe('encodeFilenameForHeader parameter injection', () => { + it('neutralizes a quote that would close the quoted filename parameter', () => { + expect(encodeFilenameForHeader(`report.pdf"; filename*=UTF-8''invoice.html`)).toBe( + `filename="report.pdf__ filename*=UTF-8''invoice.html"; filename*=UTF-8''report.pdf%22%3B%20filename%2A%3DUTF-8%27%27invoice.html` + ) + }) + + it('neutralizes the same injection on the non-ascii branch', () => { + expect(encodeFilenameForHeader(`repört.pdf"; filename*=UTF-8''invoice.html`)).toBe( + `filename="rep_rt.pdf__ filename*=UTF-8''invoice.html"; filename*=UTF-8''rep%C3%B6rt.pdf%22%3B%20filename%2A%3DUTF-8%27%27invoice.html` + ) + }) + + it('emits exactly one filename* parameter, holding the real name', () => { + const name = `report.pdf"; filename*=UTF-8''invoice.html` + const header = encodeFilenameForHeader(name) + // Strip the quoted value: text inside it is inert, so only what follows counts. + const parameters = `${header.slice(0, header.indexOf('filename="'))}${header.slice(header.lastIndexOf('"') + 1)}` + expect(parameters.match(/filename\*=/g)).toHaveLength(1) + // The one surviving filename* is the real name, not the injected one. + expect(decodeURIComponent(parameters.split(`filename*=UTF-8''`)[1])).toBe(name) + }) + + it('percent-encodes an apostrophe so it cannot desync the ext-value delimiter', () => { + const header = encodeFilenameForHeader("it's a café.pdf") + expect(header.split(`filename*=UTF-8''`)[1]).toBe('it%27s%20a%20caf%C3%A9.pdf') + }) + + it('encodes control characters that would otherwise be an invalid header value', () => { + const header = encodeFilenameForHeader('report\r\nX-Injected: 1.pdf') + expect(header).toBe( + `filename="report__X-Injected: 1.pdf"; filename*=UTF-8''report%0D%0AX-Injected%3A%201.pdf` + ) + expect( + () => + new Response('data', { headers: { 'Content-Disposition': `attachment; ${header}` } }) + ).not.toThrow() + }) + + it('leaves an ordinary ascii filename byte-identical', () => { + expect(encodeFilenameForHeader('quarterly-report (final).pdf')).toBe( + 'filename="quarterly-report (final).pdf"' + ) + }) + + it('strips the directory prefix before encoding', () => { + expect(encodeFilenameForHeader('workspace/abc/report.pdf')).toBe('filename="report.pdf"') + }) + }) + describe('Content Security Policy', () => { it('should include CSP header only for SVG responses', () => { const svgResponse = createFileResponse({ diff --git a/apps/sim/app/api/files/utils.ts b/apps/sim/app/api/files/utils.ts index 33b9a7740ba..4ec4abeab9a 100644 --- a/apps/sim/app/api/files/utils.ts +++ b/apps/sim/app/api/files/utils.ts @@ -209,18 +209,47 @@ function getSecureFileHeaders(filename: string, originalContentType: string) { } } +/** + * Percent-encode a filename as an RFC 8187 `ext-value`. + * + * `encodeURIComponent` alone is not enough: it leaves `'`, `(`, `)` and `*` raw, and + * none of those are `attr-char`. The apostrophe is the specific hazard — it is the + * delimiter in `UTF-8''name`, so a filename like `it's.pdf` would emit a third `'` + * and desync the parser. + */ +function encodeExtValue(filename: string): string { + return encodeURIComponent(filename).replace( + /['()*]/g, + (char) => `%${char.charCodeAt(0).toString(16).toUpperCase()}` + ) +} + +/** + * Build the `filename` parameters for a Content-Disposition header. + * + * The name is attacker-controlled (it is the user's `originalName`), so it can never + * be interpolated raw: a `"` closes the quoted-string early and everything after it + * is parsed as further parameters. An injected `filename*` is the payload that + * matters, because RFC 6266 tells clients to prefer `filename*` over `filename` — + * so the attacker's value wins and the download lands under a name the product UI + * never showed. Both parameters are therefore always emitted from sanitized input: + * the quoted form keeps only printable ASCII minus `"` and `\`, and the `filename*` + * form is fully percent-encoded. + * + * `;` is neutralized too, even though a quoted string may legally contain one: the + * quoted parameter exists as the fallback for clients that do not implement + * `filename*`, and those are the same clients liable to split parameters on a bare + * `;` without honouring the quoting. The exact name still survives in `filename*`. + */ export function encodeFilenameForHeader(storageKey: string): string { const filename = storageKey.split('/').pop() || storageKey - - const hasNonAscii = /[^\x00-\x7F]/.test(filename) - - if (!hasNonAscii) { + const asciiSafe = filename.replace(/[^\x20-\x7E]/g, '_').replace(/["\\;]/g, '_') + // Unchanged input proves the name is printable ASCII with no `"` or `\`, so the + // quoted form alone is both safe and sufficient — `filename*` buys nothing here. + if (asciiSafe === filename) { return `filename="${filename}"` } - - const encodedFilename = encodeURIComponent(filename) - const asciiSafe = filename.replace(/[^\x00-\x7F]/g, '_') - return `filename="${asciiSafe}"; filename*=UTF-8''${encodedFilename}` + return `filename="${asciiSafe}"; filename*=UTF-8''${encodeExtValue(filename)}` } export function createFileResponse(file: FileResponse): NextResponse {