From 1830e05de79d5d4fa9a9dc08cecad11a073d115d Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Sun, 23 Apr 2023 21:31:40 +0800 Subject: [PATCH 01/14] check request method before rendering page --- packages/kit/src/runtime/server/respond.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index ada5b05cf85e..3078fbf1c19e 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -5,7 +5,7 @@ import { render_page } from './page/index.js'; import { render_response } from './page/render.js'; import { respond_with_error } from './page/respond_with_error.js'; import { is_form_content_type } from '../../utils/http.js'; -import { handle_fatal_error, redirect_response } from './utils.js'; +import { handle_fatal_error, method_not_allowed, redirect_response } from './utils.js'; import { decode_pathname, decode_params, @@ -360,9 +360,13 @@ export async function respond(request, options, manifest, state) { } if (route) { + const page_methods = ['GET', 'HEAD', 'POST']; + const method = /** @type {import('types').HttpMethod} */ (event.request.method); + /** @type {Response} */ let response; + let mod = {}; if (is_data_request) { response = await render_data( event, @@ -374,13 +378,12 @@ export async function respond(request, options, manifest, state) { trailing_slash ?? 'never' ); } else if (route.endpoint && (!route.page || is_endpoint_request(event))) { - response = await render_endpoint(event, await route.endpoint(), state); - } else if (route.page) { + mod = await route.endpoint(); + response = await render_endpoint(event, mod, state); + } else if (route.page && page_methods.includes(method)) { response = await render_page(event, route.page, options, manifest, state, resolve_opts); } else { - // a route will always have a page or an endpoint, but TypeScript - // doesn't know that - throw new Error('This should never happen'); + return method_not_allowed(mod, method); } return response; From f196dfd8c65eb5e7b88a96da7e42e3b841aab30e Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Sun, 23 Apr 2023 21:33:58 +0800 Subject: [PATCH 02/14] add test --- .../routes/endpoint-output/co-located/+page.svelte | 0 .../src/routes/endpoint-output/co-located/+server.js | 0 packages/kit/test/apps/basics/test/server.test.js | 11 +++++++++++ 3 files changed, 11 insertions(+) create mode 100644 packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js diff --git a/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte b/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js b/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index 1c7e83f744f6..ca17c9051681 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -412,6 +412,17 @@ test.describe('Load', () => { await page.goto(`/load/fetch-origin-external?port=${port}`); expect(await page.textContent('h1')).toBe(`origin: ${new URL(baseURL).origin}`); }); + + test('does not run when co-located with server endpoint', async ({ request }) => { + const url = '/endpoint-output/co-located'; + + var response = await request.fetch(url, { + method: 'OPTIONS' + }); + + expect(response.status()).toBe(405); + expect(await response.text()).toBe('OPTIONS method not allowed'); + }); }); test.describe('Routing', () => { From 206494954546220d31ebbdf67f49717f655fbafb Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Sun, 23 Apr 2023 22:06:26 +0800 Subject: [PATCH 03/14] return allowed methods if any upon invalid requests --- packages/kit/src/runtime/server/respond.js | 6 +++--- .../src/routes/endpoint-output/co-located/+page.svelte | 0 .../basics/src/routes/endpoint-output/co-located/+server.js | 0 packages/kit/test/apps/basics/test/server.test.js | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) delete mode 100644 packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte delete mode 100644 packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 3078fbf1c19e..077c8c53ab42 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -366,7 +366,6 @@ export async function respond(request, options, manifest, state) { /** @type {Response} */ let response; - let mod = {}; if (is_data_request) { response = await render_data( event, @@ -378,11 +377,12 @@ export async function respond(request, options, manifest, state) { trailing_slash ?? 'never' ); } else if (route.endpoint && (!route.page || is_endpoint_request(event))) { - mod = await route.endpoint(); - response = await render_endpoint(event, mod, state); + response = await render_endpoint(event, await route.endpoint(), state); } else if (route.page && page_methods.includes(method)) { response = await render_page(event, route.page, options, manifest, state, resolve_opts); } else { + // not a valid endpoint request and no page exists. + const mod = route.endpoint ? await route.endpoint() : {}; return method_not_allowed(mod, method); } diff --git a/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte b/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+page.svelte deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js b/packages/kit/test/apps/basics/src/routes/endpoint-output/co-located/+server.js deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index ca17c9051681..e2432741d165 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -413,8 +413,8 @@ test.describe('Load', () => { expect(await page.textContent('h1')).toBe(`origin: ${new URL(baseURL).origin}`); }); - test('does not run when co-located with server endpoint', async ({ request }) => { - const url = '/endpoint-output/co-located'; + test('does not run when using invalid request methods', async ({ request }) => { + const url = '/load'; var response = await request.fetch(url, { method: 'OPTIONS' From 78334164c81ff09c4ef6f5546a414f1775ec4a2b Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Sun, 23 Apr 2023 22:07:40 +0800 Subject: [PATCH 04/14] changeset --- .changeset/giant-rockets-love.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/giant-rockets-love.md diff --git a/.changeset/giant-rockets-love.md b/.changeset/giant-rockets-love.md new file mode 100644 index 000000000000..b2e80e181b2e --- /dev/null +++ b/.changeset/giant-rockets-love.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: avoid running load function on invalid requests From 3751b2cdba692fda2abe82c13123fc20ead40c79 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 02:53:00 +0800 Subject: [PATCH 05/14] move page_methods to top of respond.js file --- packages/kit/src/runtime/server/respond.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 077c8c53ab42..c2ade84eb33d 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -39,6 +39,8 @@ const default_filter = () => false; /** @type {import('types').RequiredResolveOptions['preload']} */ const default_preload = ({ type }) => type === 'js' || type === 'css'; +const page_methods = ['GET', 'HEAD', 'POST']; + /** * @param {Request} request * @param {import('types').SSROptions} options @@ -322,7 +324,7 @@ export async function respond(request, options, manifest, state) { } return await handle_fatal_error(event, options, e); } - + /** * * @param {import('types').RequestEvent} event @@ -360,7 +362,6 @@ export async function respond(request, options, manifest, state) { } if (route) { - const page_methods = ['GET', 'HEAD', 'POST']; const method = /** @type {import('types').HttpMethod} */ (event.request.method); /** @type {Response} */ From e8fd18c95e02f596c99274d40da86cbd1cbd3866 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 02:53:31 +0800 Subject: [PATCH 06/14] prettier --- packages/kit/src/runtime/server/respond.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index c2ade84eb33d..cba934c1bdda 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -324,7 +324,7 @@ export async function respond(request, options, manifest, state) { } return await handle_fatal_error(event, options, e); } - + /** * * @param {import('types').RequestEvent} event From 074727151686489e9dab309826cf922dd11261cf Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 03:11:08 +0800 Subject: [PATCH 07/14] remove empty line --- packages/kit/src/runtime/server/respond.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index cba934c1bdda..38572106222a 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -326,7 +326,6 @@ export async function respond(request, options, manifest, state) { } /** - * * @param {import('types').RequestEvent} event * @param {import('types').ResolveOptions} [opts] */ From d7fe86f2bcbd422792719185b667a4fe10691aea Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 16:50:33 +0800 Subject: [PATCH 08/14] revise comment --- packages/kit/src/runtime/server/respond.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 38572106222a..053069b5d633 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -381,7 +381,7 @@ export async function respond(request, options, manifest, state) { } else if (route.page && page_methods.includes(method)) { response = await render_page(event, route.page, options, manifest, state, resolve_opts); } else { - // not a valid endpoint request and no page exists. + // we end up here if the request method is invalid for the page / endpoint. const mod = route.endpoint ? await route.endpoint() : {}; return method_not_allowed(mod, method); } From 59e35bd72c35b2d7d6326a96c305990af5ea4df2 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 18:28:40 +0800 Subject: [PATCH 09/14] gracefully handle page OPTIONS requests --- packages/kit/src/runtime/server/respond.js | 22 ++++++++++++++++--- .../kit/test/apps/basics/test/server.test.js | 18 +++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 053069b5d633..b3e5bbb1be96 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -39,7 +39,7 @@ const default_filter = () => false; /** @type {import('types').RequiredResolveOptions['preload']} */ const default_preload = ({ type }) => type === 'js' || type === 'css'; -const page_methods = ['GET', 'HEAD', 'POST']; +const page_methods = new Set(['GET', 'HEAD', 'POST']); /** * @param {Request} request @@ -378,10 +378,26 @@ export async function respond(request, options, manifest, state) { ); } else if (route.endpoint && (!route.page || is_endpoint_request(event))) { response = await render_endpoint(event, await route.endpoint(), state); - } else if (route.page && page_methods.includes(method)) { + } else if (route.page && page_methods.has(method)) { + console.log(page_methods.has(method)); response = await render_page(event, route.page, options, manifest, state, resolve_opts); + } else if (route.page && method === 'OPTIONS') { + // if no OPTIONS endpoint exists, fallback to handling it gracefully. + const allowed_methods = new Set(page_methods).add('OPTIONS'); + + const node = await manifest._.nodes[route.page.leaf](); + if (!node?.server?.actions) { + allowed_methods.delete('POST'); + } + + response = new Response(null, { + status: 204, + headers: { + allow: Array.from(allowed_methods.values()).join(', ') + } + }); } else { - // we end up here if the request method is invalid for the page / endpoint. + // if the request method is not allowed by us for the page / endpoint. const mod = route.endpoint ? await route.endpoint() : {}; return method_not_allowed(mod, method); } diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index e2432741d165..d922456faff6 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -414,14 +414,24 @@ test.describe('Load', () => { }); test('does not run when using invalid request methods', async ({ request }) => { - const url = '/load'; + const load_url = '/load'; - var response = await request.fetch(url, { + let response = await request.fetch(load_url, { method: 'OPTIONS' }); - expect(response.status()).toBe(405); - expect(await response.text()).toBe('OPTIONS method not allowed'); + expect(response.status()).toBe(204); + expect(await response.text()).toBe(''); + expect(response.headers()['allow']).toBe('GET, HEAD, OPTIONS'); + + const actions_url = '/actions/enhance'; + response = await request.fetch(actions_url, { + method: 'OPTIONS' + }); + + expect(response.status()).toBe(204); + expect(await response.text()).toBe(''); + expect(response.headers()['allow']).toBe('GET, HEAD, POST, OPTIONS'); }); }); From 8277ee4daf74f337b427e9842e36d0696f1475da Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 18:31:54 +0800 Subject: [PATCH 10/14] add set for allowed page methods --- packages/kit/test/apps/basics/test/server.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index d922456faff6..094e9c1ef794 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -431,7 +431,7 @@ test.describe('Load', () => { expect(response.status()).toBe(204); expect(await response.text()).toBe(''); - expect(response.headers()['allow']).toBe('GET, HEAD, POST, OPTIONS'); + expect(response.headers()['allow']).toBe('GET, HEAD, OPTIONS, POST'); }); }); From 526af3f8c4f6990dfa2eac990371639858ea3139 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 18:32:14 +0800 Subject: [PATCH 11/14] add allowed page methods set --- packages/kit/src/runtime/server/respond.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index b3e5bbb1be96..3ce798dbaa3e 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -41,6 +41,8 @@ const default_preload = ({ type }) => type === 'js' || type === 'css'; const page_methods = new Set(['GET', 'HEAD', 'POST']); +const allowed_page_methods = new Set(['GET', 'HEAD', 'OPTIONS']); + /** * @param {Request} request * @param {import('types').SSROptions} options @@ -383,11 +385,11 @@ export async function respond(request, options, manifest, state) { response = await render_page(event, route.page, options, manifest, state, resolve_opts); } else if (route.page && method === 'OPTIONS') { // if no OPTIONS endpoint exists, fallback to handling it gracefully. - const allowed_methods = new Set(page_methods).add('OPTIONS'); + const allowed_methods = new Set(allowed_page_methods); const node = await manifest._.nodes[route.page.leaf](); if (!node?.server?.actions) { - allowed_methods.delete('POST'); + allowed_methods.add('POST'); } response = new Response(null, { From 68d0e242076e60abc8854002261f9c5bd0ad30df Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 25 Apr 2023 18:49:49 +0800 Subject: [PATCH 12/14] oopsie --- packages/kit/src/runtime/server/respond.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 3ce798dbaa3e..4845173ed89c 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -388,7 +388,7 @@ export async function respond(request, options, manifest, state) { const allowed_methods = new Set(allowed_page_methods); const node = await manifest._.nodes[route.page.leaf](); - if (!node?.server?.actions) { + if (node?.server?.actions) { allowed_methods.add('POST'); } From 28eecdbe3b9ebf412352d7e9776db6ec5aefc246 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Thu, 27 Apr 2023 20:09:26 +0800 Subject: [PATCH 13/14] Update packages/kit/src/runtime/server/respond.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- packages/kit/src/runtime/server/respond.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 4845173ed89c..b01e429fcb95 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -381,7 +381,6 @@ export async function respond(request, options, manifest, state) { } else if (route.endpoint && (!route.page || is_endpoint_request(event))) { response = await render_endpoint(event, await route.endpoint(), state); } else if (route.page && page_methods.has(method)) { - console.log(page_methods.has(method)); response = await render_page(event, route.page, options, manifest, state, resolve_opts); } else if (route.page && method === 'OPTIONS') { // if no OPTIONS endpoint exists, fallback to handling it gracefully. From 7fb4544e83f1f0e052c49bdc86acfd7eef78c32c Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 4 Jul 2023 14:06:02 +0200 Subject: [PATCH 14/14] cleanup --- packages/kit/src/runtime/server/respond.js | 46 +++++++++++++--------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 19fa1db2c14b..f97baa2e6526 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -399,27 +399,37 @@ export async function respond(request, options, manifest, state) { ); } else if (route.endpoint && (!route.page || is_endpoint_request(event))) { response = await render_endpoint(event, await route.endpoint(), state); - } else if (route.page && page_methods.has(method)) { - response = await render_page(event, route.page, options, manifest, state, resolve_opts); - } else if (route.page && method === 'OPTIONS') { - // if no OPTIONS endpoint exists, fallback to handling it gracefully. - const allowed_methods = new Set(allowed_page_methods); - - const node = await manifest._.nodes[route.page.leaf](); - if (node?.server?.actions) { - allowed_methods.add('POST'); - } + } else if (route.page) { + if (page_methods.has(method)) { + response = await render_page(event, route.page, options, manifest, state, resolve_opts); + } else { + const allowed_methods = new Set(allowed_page_methods); + const node = await manifest._.nodes[route.page.leaf](); + if (node?.server?.actions) { + allowed_methods.add('POST'); + } - response = new Response(null, { - status: 204, - headers: { - allow: Array.from(allowed_methods.values()).join(', ') + if (method === 'OPTIONS') { + // This will deny CORS preflight requests implicitly because we don't + // add the required CORS headers to the response. + response = new Response(null, { + status: 204, + headers: { + allow: Array.from(allowed_methods.values()).join(', ') + } + }); + } else { + const mod = [...allowed_methods].reduce((acc, curr) => { + acc[curr] = true; + return acc; + }, /** @type {Record} */ ({})); + response = method_not_allowed(mod, method); } - }); + } } else { - // if the request method is not allowed by us for the page / endpoint. - const mod = route.endpoint ? await route.endpoint() : {}; - return method_not_allowed(mod, method); + // a route will always have a page or an endpoint, but TypeScript + // doesn't know that + throw new Error('This should never happen'); } // If the route contains a page and an endpoint, we need to add a