From 91e3ffe23d5e09a023258ec79b5854c9b7fbfcdc Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Mon, 10 Apr 2023 10:32:00 +0545 Subject: [PATCH 1/6] init --- packages/kit/src/exports/node/index.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js index 2d7414d43f32..8136c58ce47a 100644 --- a/packages/kit/src/exports/node/index.js +++ b/packages/kit/src/exports/node/index.js @@ -115,7 +115,15 @@ export async function setResponse(res, response) { headers['set-cookie'] = split; } - res.writeHead(response.status, headers); + try { + res.writeHead(response.status, headers); + } catch (error) { + console.log("wat"); + res.writeHead(500); + res.write(error.message); + res.end(); + return; + } if (!response.body) { res.end(); From 07af31e9ee858dc3246e3cbe0f823092640228b1 Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Mon, 10 Apr 2023 18:58:22 +0545 Subject: [PATCH 2/6] set 500 for locked body --- packages/kit/src/exports/node/index.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js index 8136c58ce47a..a48d1212cc5a 100644 --- a/packages/kit/src/exports/node/index.js +++ b/packages/kit/src/exports/node/index.js @@ -118,10 +118,9 @@ export async function setResponse(res, response) { try { res.writeHead(response.status, headers); } catch (error) { - console.log("wat"); + // TODO: remove the other headers from the response? res.writeHead(500); - res.write(error.message); - res.end(); + res.end(String(error)); return; } @@ -131,11 +130,11 @@ export async function setResponse(res, response) { } if (response.body.locked) { - res.write( + res.writeHead(500); + res.end( 'Fatal error: Response body is locked. ' + `This can happen when the response was already read (for example through 'response.json()' or 'response.text()').` ); - res.end(); return; } From 74ef2085e2d704497b4df61a498068d42f0c2438 Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Tue, 11 Apr 2023 07:39:12 +0545 Subject: [PATCH 3/6] redo implementation and add tests --- packages/kit/src/exports/node/index.js | 37 ++++++++++++------- .../kit/test/apps/basics/test/server.test.js | 9 +++++ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js index a48d1212cc5a..d3045dcde7f2 100644 --- a/packages/kit/src/exports/node/index.js +++ b/packages/kit/src/exports/node/index.js @@ -105,32 +105,43 @@ export async function getRequest({ request, base, bodySizeLimit }) { /** @type {import('@sveltejs/kit/node').setResponse} */ export async function setResponse(res, response) { - const headers = Object.fromEntries(response.headers); + let copy_header_error = false; + /** + * @param {string} name + * @param {string | string[]} value + */ + function try_copy_header(name, value) { + try { + res.setHeader(name, value); + } catch (error) { + // clean out the headers that were set for the original response + res.getHeaderNames().forEach((name) => res.removeHeader(name)); + res.writeHead(500).end(String(error)); + copy_header_error = true; + } + } if (response.headers.has('set-cookie')) { const header = /** @type {string} */ (response.headers.get('set-cookie')); - const split = set_cookie_parser.splitCookiesString(header); - - // @ts-expect-error - headers['set-cookie'] = split; + try_copy_header('set-cookie', set_cookie_parser.splitCookiesString(header)); + if (copy_header_error) return; } - try { - res.writeHead(response.status, headers); - } catch (error) { - // TODO: remove the other headers from the response? - res.writeHead(500); - res.end(String(error)); - return; + for (const [name, value] of response.headers) { + if (name !== 'set-cookie') { + try_copy_header(name, value); + if (copy_header_error) return; + } } + res.writeHead(response.status); + if (!response.body) { res.end(); return; } if (response.body.locked) { - res.writeHead(500); res.end( 'Fatal error: Response body is locked. ' + `This can happen when the response was already read (for example through 'response.json()' or 'response.text()').` diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index d6fc639abbbd..6a3c710d05eb 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -177,6 +177,15 @@ test.describe('Endpoints', () => { expect(await response.text()).toEqual(digest); }); + // TODO see above + test('errors in res.writeHead return a 500', async ({ request }) => { + const response = await request.get('/endpoint-output/head-write-error'); + expect(response.status()).toBe(500); + expect(await response.text()).toMatch( + 'TypeError [ERR_INVALID_CHAR]: Invalid character in header content ["x-test"]' + ); + }); + test('OPTIONS handler', async ({ request }) => { const url = '/endpoint-output/options'; From 3d30f20b7e7f2187757af77e15ccf6b5ba23fc94 Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Tue, 11 Apr 2023 07:40:39 +0545 Subject: [PATCH 4/6] changeset --- .changeset/swift-ads-itch.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/swift-ads-itch.md diff --git a/.changeset/swift-ads-itch.md b/.changeset/swift-ads-itch.md new file mode 100644 index 000000000000..92e930b903c5 --- /dev/null +++ b/.changeset/swift-ads-itch.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: prevent unhandled exceptions for invalid header values From a18e69e8fbad19b33a6a794efe3c5b4ac1f3edbc Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Tue, 11 Apr 2023 09:12:48 +0545 Subject: [PATCH 5/6] simplify impl and add tests --- packages/kit/src/exports/node/index.js | 29 +++++-------------- .../head-write-error/+server.js | 10 +++++++ .../kit/test/apps/basics/test/server.test.js | 15 +++++----- 3 files changed, 25 insertions(+), 29 deletions(-) create mode 100644 packages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.js diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js index d3045dcde7f2..37ceab209441 100644 --- a/packages/kit/src/exports/node/index.js +++ b/packages/kit/src/exports/node/index.js @@ -105,32 +105,17 @@ export async function getRequest({ request, base, bodySizeLimit }) { /** @type {import('@sveltejs/kit/node').setResponse} */ export async function setResponse(res, response) { - let copy_header_error = false; - /** - * @param {string} name - * @param {string | string[]} value - */ - function try_copy_header(name, value) { + for (const key of response.headers.keys()) { try { - res.setHeader(name, value); + const value = /** @type {string} */ (response.headers.get(key)); + res.setHeader( + key, + key === 'set-cookie' ? set_cookie_parser.splitCookiesString(value) : value + ); } catch (error) { - // clean out the headers that were set for the original response res.getHeaderNames().forEach((name) => res.removeHeader(name)); res.writeHead(500).end(String(error)); - copy_header_error = true; - } - } - - if (response.headers.has('set-cookie')) { - const header = /** @type {string} */ (response.headers.get('set-cookie')); - try_copy_header('set-cookie', set_cookie_parser.splitCookiesString(header)); - if (copy_header_error) return; - } - - for (const [name, value] of response.headers) { - if (name !== 'set-cookie') { - try_copy_header(name, value); - if (copy_header_error) return; + return; } } diff --git a/packages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.js b/packages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.js new file mode 100644 index 000000000000..72664eee7dcc --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/endpoint-output/head-write-error/+server.js @@ -0,0 +1,10 @@ +import { json } from '@sveltejs/kit'; + +/** @type {import('@sveltejs/kit').RequestHandler} */ +export function GET({ setHeaders }) { + setHeaders({ + 'x-test': '\u001f' + }); + + return json({}); +} diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index 6a3c710d05eb..8fcdef16cdbd 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -178,7 +178,7 @@ test.describe('Endpoints', () => { }); // TODO see above - test('errors in res.writeHead return a 500', async ({ request }) => { + test('invalid headers return a 500', async ({ request }) => { const response = await request.get('/endpoint-output/head-write-error'); expect(response.status()).toBe(500); expect(await response.text()).toMatch( @@ -494,7 +494,9 @@ test.describe('setHeaders', () => { test('allows multiple set-cookie headers with different values', async ({ page }) => { const response = await page.goto('/headers/set-cookie/sub'); const cookies = (await response?.allHeaders())['set-cookie']; - expect(cookies.includes('cookie1=value1') && cookies.includes('cookie2=value2')).toBe(true); + + expect(cookies).toMatch('cookie1=value1'); + expect(cookies).toMatch('cookie2=value2'); }); }); @@ -502,11 +504,10 @@ test.describe('cookies', () => { test('cookie.serialize created correct cookie header string', async ({ page }) => { const response = await page.goto('/cookies/serialize'); const cookies = await response.headerValue('set-cookie'); - expect( - cookies.includes('before=before') && - cookies.includes('after=after') && - cookies.includes('endpoint=endpoint') - ).toBe(true); + + expect(cookies).toMatch('before=before'); + expect(cookies).toMatch('after=after'); + expect(cookies).toMatch('endpoint=endpoint'); }); }); From e692574fed8467575e2beb708762cb78ef3d9c52 Mon Sep 17 00:00:00 2001 From: gtmnayan Date: Tue, 11 Apr 2023 09:46:18 +0545 Subject: [PATCH 6/6] .. --- packages/kit/src/exports/node/index.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/kit/src/exports/node/index.js b/packages/kit/src/exports/node/index.js index 37ceab209441..76233e074a58 100644 --- a/packages/kit/src/exports/node/index.js +++ b/packages/kit/src/exports/node/index.js @@ -105,12 +105,16 @@ export async function getRequest({ request, base, bodySizeLimit }) { /** @type {import('@sveltejs/kit/node').setResponse} */ export async function setResponse(res, response) { - for (const key of response.headers.keys()) { + for (const [key, value] of response.headers) { try { - const value = /** @type {string} */ (response.headers.get(key)); res.setHeader( key, - key === 'set-cookie' ? set_cookie_parser.splitCookiesString(value) : value + key === 'set-cookie' + ? set_cookie_parser.splitCookiesString( + // This is absurd but necessary, TODO: investigate why + /** @type {string}*/ (response.headers.get(key)) + ) + : value ); } catch (error) { res.getHeaderNames().forEach((name) => res.removeHeader(name));