From 376311cb9091ff1726e8b383312f22c607dcc8a0 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:52:39 +0200 Subject: [PATCH] fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) (#1062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stable backport of #1055 (main: 3a11e6eb). Change content is byte-identical to the main twin; cherry-picked clean, no resolutions needed. The six quote/invoice PDF endpoints interpolated buildPdfFilename()'s result straight into `inline; filename="${filename}"`. That result deliberately preserves non-ASCII (it doubles as the PDF's internal Title metadata), and HTTP header values are latin1, so a customer label reaching the header directly failed in one of two ways: - U+0080-U+00FF (ä ö ü ß — every German umlaut): no throw. The raw byte goes out and the client reads back a mangled name. Silent corruption. - above U+00FF (Polish ł, Czech ř, Turkish ş, €, Cyrillic, CJK, emoji): Node's setHeader rejects it with ERR_INVALID_CHAR. The throw lands after the PDF buffer is already rendered, so the request 500s. This corrects the issue's diagnosis: it reported umlauts as the 500 case, but umlauts are inside latin1 and mangle rather than throw. Route all six through buildContentDisposition(), which emits an ASCII fallback plus the RFC 5987 filename*=UTF-8'' form. Also stops sanitiseSegment splitting surrogate pairs at its 80-unit cap — a dangling high surrogate makes encodeURIComponent throw URIError inside the helper, reaching the same 500 a different way (found by external review on the main twin). Verified on this branch: 14/14 in the new suite, 151/151 across the nine surrounding pdf/filename/quote/invoice suites, lint clean. Co-authored-by: Paul Nothaft --- .../utils/pdfContentDisposition.test.js | 143 ++++++++++++++++++ backend/src/routes/adminInvoices.js | 6 +- backend/src/routes/adminQuotes.js | 6 +- backend/src/routes/customer.js | 6 +- backend/src/utils/pdfFilename.js | 26 +++- 5 files changed, 180 insertions(+), 7 deletions(-) create mode 100644 backend/__tests__/utils/pdfContentDisposition.test.js diff --git a/backend/__tests__/utils/pdfContentDisposition.test.js b/backend/__tests__/utils/pdfContentDisposition.test.js new file mode 100644 index 00000000..33178abc --- /dev/null +++ b/backend/__tests__/utils/pdfContentDisposition.test.js @@ -0,0 +1,143 @@ +/** + * Regression test for #1024: quote/invoice PDF endpoints 500'd (or silently + * corrupted the filename) for customers whose name carries non-ASCII. + * + * The six PDF routes built the header by interpolating buildPdfFilename()'s + * result straight into `inline; filename="${filename}"`. HTTP header values + * are latin1, which splits the failure in two — and the split matters, + * because the issue reported the umlaut case as the 500 and it isn't: + * + * U+0080-U+00FF (ä ö ü ß — every German umlaut) + * No throw. The byte goes out raw and the client reads back a mangled + * name. A silent corruption, not an error. + * + * above U+00FF (Polish ł, Czech ř, Turkish ş, €, Cyrillic, CJK, emoji) + * Node's setHeader rejects it with ERR_INVALID_CHAR. Because the + * throw lands after the PDF buffer is already rendered, the whole + * request fails as an unhandled 500. + * + * buildContentDisposition() fixes both: an ASCII fallback for the legacy + * `filename=` parameter plus the RFC 5987 `filename*=UTF-8''…` form that + * carries the real name. + * + * These assertions run against the real Node header validator via a live + * express server, so they'd fail against the old interpolation rather than + * merely testing the helper in isolation. + */ +const express = require('express'); +const request = require('supertest'); + +const { buildPdfFilename, sanitiseSegment } = require('../../src/utils/pdfFilename'); +const { buildContentDisposition } = require('../../src/utils/filenameSanitizer'); + +// The RFC 5987 parameter prefix, i.e. filename*=UTF-8'' — the two trailing +// quotes are the (empty) language tag the spec puts between the charset and +// the percent-encoded value. +const RFC5987_PREFIX = 'filename*=UTF-8\'\''; + +// Mirrors what the six PDF routes now do. +function buildApp(customer, docNumber = 'Q-2026-0042') { + const app = express(); + app.get('/pdf', (req, res) => { + const filename = buildPdfFilename({ docNumber, customer, fallback: 'quote-preview' }); + res.set('Content-Type', 'application/pdf'); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); + res.send(Buffer.from('%PDF-1.4 fake')); + }); + // Mirrors the real error handler: an ERR_INVALID_CHAR throw inside the + // handler surfaces as a 500, which is what #1024 reported. + // eslint-disable-next-line no-unused-vars + app.use((err, req, res, next) => res.status(500).json({ error: err.code || err.message })); + return app; +} + +describe('#1024 — PDF Content-Disposition with non-ASCII customer names', () => { + it('serves a PDF for a German umlaut name and keeps the name intact', async () => { + const res = await request(buildApp({ company_name: 'Müller Fotografie' })).get('/pdf'); + + expect(res.status).toBe(200); + const cd = res.headers['content-disposition']; + // RFC 5987 form carries the real, unmangled name... + expect(cd).toContain(RFC5987_PREFIX); + expect(cd).toContain(encodeURIComponent('Müller-Fotografie.pdf')); + // ...and the ASCII fallback is legal latin1 with no raw umlaut byte. + const fallback = /filename="([^"]+)"/.exec(cd)[1]; + expect(fallback).toMatch(/^[\x20-\x7e]+$/); + }); + + it.each([ + ['Polish', 'Michał Kowalski'], + ['Czech', 'Dvořák Studio'], + ['Turkish', 'Şahin Fotoğraf'], + ['Cyrillic', 'Иванов Фото'], + ['CJK', '山田写真'], + ['emoji', 'Studio 🎉 Berlin'], + ])('does not 500 for a %s customer name (was ERR_INVALID_CHAR)', async (_label, company) => { + const res = await request(buildApp({ company_name: company })).get('/pdf'); + + expect(res.status).toBe(200); + const cd = res.headers['content-disposition']; + expect(cd).toContain(RFC5987_PREFIX); + // The legacy filename= token drops non-ASCII, so a name written entirely + // in another script degrades to just the document number + // (`Q-2026-0042_.pdf`). That's the intended trade — filename* carries the + // real name — but the fallback must still be a legal, non-empty, + // ASCII-only token, since that is what a client without RFC 5987 support + // ends up saving. + const fallback = /filename="([^"]*)"/.exec(cd)[1]; + expect(fallback.length).toBeGreaterThan(0); + expect(fallback).toMatch(/^[\x20-\x7e]+$/); + expect(fallback).toContain('Q-2026-0042'); + }); + + it('leaves a plain ASCII name on the familiar filename= form', async () => { + const res = await request(buildApp({ company_name: 'Bright Studio' })).get('/pdf'); + + expect(res.status).toBe(200); + expect(res.headers['content-disposition']) + .toContain('filename="Q-2026-0042_Bright-Studio.pdf"'); + }); + + it('still works when the customer row is missing entirely (preview path)', async () => { + const res = await request(buildApp(null, null)).get('/pdf'); + + expect(res.status).toBe(200); + expect(res.headers['content-disposition']).toContain('quote-preview_customer.pdf'); + }); + + // sanitiseSegment caps each segment at 80 UTF-16 code units. A cap landing + // inside an astral character used to leave a dangling high surrogate, which + // makes encodeURIComponent throw URIError inside buildContentDisposition — + // a 500 on the very endpoint this PR fixes, reached a different way. + it.each([ + ['emoji on the 80-char boundary', `${'a'.repeat(79)}🎉`], + ['astral CJK on the boundary', `${'a'.repeat(79)}𠜎`], + ['a label that is entirely astral', '🎉'.repeat(60)], + ])('does not 500 when truncation splits a surrogate pair — %s', async (_label, company) => { + const res = await request(buildApp({ company_name: company })).get('/pdf'); + + expect(res.status).toBe(200); + expect(res.headers['content-disposition']).toContain(RFC5987_PREFIX); + }); + + it('drops the orphaned surrogate rather than widening the length cap', () => { + const seg = sanitiseSegment(`${'a'.repeat(79)}🎉`); + + // 79 'a's + a half-emoji would be 80; the orphan is dropped, not kept. + expect(seg).toHaveLength(79); + expect(seg).toBe('a'.repeat(79)); + // Nothing in the result may be an unpaired surrogate. + expect(seg).toBe(seg.replace(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])/g, '')); + }); + + it('the raw interpolation these routes used to do really does throw', () => { + // Pins the root cause itself, so nobody "simplifies" the helper away. + const filename = buildPdfFilename({ + docNumber: 'Q-2026-0042', + customer: { company_name: 'Michał Kowalski' }, + }); + const res = new (require('http').ServerResponse)({}); + expect(() => res.setHeader('Content-Disposition', `inline; filename="${filename}"`)) + .toThrow(/ERR_INVALID_CHAR|Invalid character/); + }); +}); diff --git a/backend/src/routes/adminInvoices.js b/backend/src/routes/adminInvoices.js index fac20bd9..cc4cfe6e 100644 --- a/backend/src/routes/adminInvoices.js +++ b/backend/src/routes/adminInvoices.js @@ -894,6 +894,7 @@ router.get( // re-fetching here keeps the route a thin shim over the // service rather than reaching inside its internals. const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const inv = await db('invoices').where({ id }).first(); const customer = inv ? await db('customer_accounts').where({ id: inv.customer_account_id }).first() : null; const filename = buildPdfFilename({ @@ -902,7 +903,7 @@ router.get( fallback: `invoice-${id}`, }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); }) ); @@ -919,6 +920,7 @@ router.post( // the customer so the filename still reflects who the invoice // is for; the number segment falls back to "invoice-preview". const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const customer = payload.customerAccountId ? await db('customer_accounts').where({ id: payload.customerAccountId }).first() : null; @@ -928,7 +930,7 @@ router.post( fallback: 'invoice-preview', }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); }) ); diff --git a/backend/src/routes/adminQuotes.js b/backend/src/routes/adminQuotes.js index afaeff29..89384926 100644 --- a/backend/src/routes/adminQuotes.js +++ b/backend/src/routes/adminQuotes.js @@ -537,6 +537,7 @@ router.get( const id = parseInt(req.params.id, 10); const buf = await quoteService.renderQuotePdfBuffer(id); const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const quote = await db('quotes').where({ id }).first(); const customer = quote ? await db('customer_accounts').where({ id: quote.customer_account_id }).first() : null; const filename = buildPdfFilename({ @@ -545,7 +546,7 @@ router.get( fallback: `quote-${id}`, }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); }) ); @@ -559,6 +560,7 @@ router.post( const payload = mapPayloadToService(req.body); const buf = await quoteService.renderQuotePdfFromPayload(payload); const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const customer = payload.customerAccountId ? await db('customer_accounts').where({ id: payload.customerAccountId }).first() : null; @@ -568,7 +570,7 @@ router.post( fallback: 'quote-preview', }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); }) ); diff --git a/backend/src/routes/customer.js b/backend/src/routes/customer.js index 68f210de..04ed26df 100644 --- a/backend/src/routes/customer.js +++ b/backend/src/routes/customer.js @@ -566,6 +566,7 @@ router.get('/quotes/:id/pdf', customerAuth, async (req, res) => { const quoteService = require('../services/quoteService'); const buf = await quoteService.renderQuotePdfBuffer(quote.id); const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const customer = await dbi('customer_accounts').where({ id: req.customer.id }).first(); const filename = buildPdfFilename({ docNumber: quote.quote_number, @@ -573,7 +574,7 @@ router.get('/quotes/:id/pdf', customerAuth, async (req, res) => { fallback: `quote-${quote.id}`, }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); } catch (error) { errorResponse(res, error, 500, 'Failed to render quote PDF'); @@ -597,6 +598,7 @@ router.get('/invoices/:id/pdf', customerAuth, async (req, res) => { const invoiceService = require('../services/invoiceService'); const buf = await invoiceService.renderInvoicePdfBuffer(invoice.id); const { buildPdfFilename } = require('../utils/pdfFilename'); + const { buildContentDisposition } = require('../utils/filenameSanitizer'); const customer = await dbi('customer_accounts').where({ id: req.customer.id }).first(); const filename = buildPdfFilename({ docNumber: invoice.invoice_number, @@ -604,7 +606,7 @@ router.get('/invoices/:id/pdf', customerAuth, async (req, res) => { fallback: `invoice-${invoice.id}`, }); res.set('Content-Type', 'application/pdf'); - res.set('Content-Disposition', `inline; filename="${filename}"`); + res.set('Content-Disposition', buildContentDisposition(filename, 'inline')); res.send(buf); } catch (error) { errorResponse(res, error, 500, 'Failed to render invoice PDF'); diff --git a/backend/src/utils/pdfFilename.js b/backend/src/utils/pdfFilename.js index ea4741b3..cfdf52f5 100644 --- a/backend/src/utils/pdfFilename.js +++ b/backend/src/utils/pdfFilename.js @@ -20,6 +20,19 @@ * - The PDF's internal `Title` metadata (Chrome's PDF viewer * uses this as the default name when saving from a blob URL, * where Content-Disposition can't reach) + * + * IMPORTANT (#1024): the preserved non-ASCII is exactly what a raw + * `filename="${...}"` header cannot carry. HTTP header values are + * latin1, so a customer label reaching a header directly either + * mangles (U+0080-U+00FF — every German umlaut: `Müller` is sent as + * the byte 0xFC and read back as garbage) or throws ERR_INVALID_CHAR + * and 500s the request (anything above U+00FF — Polish ł, Czech ř, + * Turkish ş, €, Cyrillic, CJK, emoji). + * + * Never interpolate this result into a header. Pass it through + * `buildContentDisposition()` in utils/filenameSanitizer, which emits + * an ASCII fallback plus the RFC 5987 `filename*=UTF-8''…` form so the + * unicode name survives in browsers and the header stays legal. */ function sanitiseSegment(input, maxLen = 80) { @@ -33,7 +46,18 @@ function sanitiseSegment(input, maxLen = 80) { s = s.replace(/-+/g, '-'); // Trim leading/trailing dashes + dots. s = s.replace(/^[-.]+|[-.]+$/g, ''); - if (s.length > maxLen) s = s.slice(0, maxLen); + if (s.length > maxLen) { + s = s.slice(0, maxLen); + // slice() cuts UTF-16 code units, so a boundary landing inside an astral + // character (emoji, rarer CJK) leaves a dangling high surrogate. That is + // not merely cosmetic: the lone surrogate makes encodeURIComponent throw + // `URIError: URI malformed` inside buildContentDisposition, which 500s + // the PDF endpoint — the exact failure #1024 set out to remove, just via + // a different route. Drop the orphan rather than widening the cap, so the + // byte budget this limit exists to protect is unchanged. + const lastUnit = s.charCodeAt(s.length - 1); + if (lastUnit >= 0xD800 && lastUnit <= 0xDBFF) s = s.slice(0, -1); + } return s; }