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 445ee692..d1ba008a 100644 --- a/backend/src/routes/adminInvoices.js +++ b/backend/src/routes/adminInvoices.js @@ -921,6 +921,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({ @@ -929,7 +930,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); }) ); @@ -946,6 +947,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; @@ -955,7 +957,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; }