sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.45k stars 1.89k forks source link

adapter-node is erroring on all requests #801

Closed samccone closed 3 years ago

samccone commented 3 years ago

Updating adapt-node from "1.0.0-next.09", to "1.0.0-next.11" is now resulting in the error always being raised when running the server.

node build/index.js

Listening on port 3000
node:internal/process/promises:245
          triggerUncaughtException(err, true /* fromPromise */);
          ^

TypeError [ERR_INVALID_URL]: Invalid URL: /
    at new NodeError (node:internal/errors:329:5)
    at onParseError (node:internal/url:537:9)
    at new URL (node:internal/url:613:5)
    at Array.<anonymous> (file:///Users/samccone/Desktop/repos/edit/build/index.js:14294:18)
    at loop (file:///Users/samccone/Desktop/repos/edit/build/index.js:13728:58)
    at next (file:///Users/samccone/Desktop/repos/edit/build/index.js:13729:69)
    at Array.noop_handler (file:///Users/samccone/Desktop/repos/edit/build/index.js:14283:44)
    at loop (file:///Users/samccone/Desktop/repos/edit/build/index.js:13728:58)
    at next (file:///Users/samccone/Desktop/repos/edit/build/index.js:13729:69)
    at Array.<anonymous> (file:///Users/samccone/Desktop/repos/edit/build/index.js:14012:28) {
  input: '/',
  code: 'ERR_INVALID_URL'
}

^ This results in the server crashing and not serving a 404 which is also interesting

I have confirmed that downgrading back to 09 fixes the issue on my end.

Conduitry commented 3 years ago

Yikes, yeah I can reproduce this from the starter template.

Conduitry commented 3 years ago

https://github.com/sveltejs/kit/blob/ca33a3596ef0e6e36f5bb30909aa5400fd3fd2d8/packages/adapter-node/src/server.js#L34

req.url is just the path part of a URL, and not the whole URL. We need to provide some sort of base for this to be relative to. It doesn't look like we're using the host part, so I think we should be able to just do

const parsed = new URL(req.url || '', 'http://localhost');
Conduitry commented 3 years ago

This was broken in #774. The Vercel adapter is constructing the origin from req.headers['x-forwarded-proto'] and req.headers.host (so it's not susceptible to this bug) but I don't think that's necessary there, because the host part of the parsed URL is also ignored.

Conduitry commented 3 years ago

My proposed fix is

diff --git a/packages/adapter-node/src/server.js b/packages/adapter-node/src/server.js
index f93e6cb..15947db 100644
--- a/packages/adapter-node/src/server.js
+++ b/packages/adapter-node/src/server.js
@@ -31,7 +31,7 @@ const assets_handler = sirv(join(__dirname, '/assets'), {

 polka()
    .use(compression({ threshold: 0 }), assets_handler, prerendered_handler, async (req, res) => {
-       const parsed = new URL(req.url || '');
+       const parsed = new URL(req.url || '', 'http://localhost');
        const rendered = await app.render({
            method: req.method,
            headers: req.headers, // TODO: what about repeated headers, i.e. string[]
diff --git a/packages/adapter-vercel/src/entry.js b/packages/adapter-vercel/src/entry.js
index 38b7cde..5af07aa 100644
--- a/packages/adapter-vercel/src/entry.js
+++ b/packages/adapter-vercel/src/entry.js
@@ -3,8 +3,7 @@ import { URL } from 'url';
 import { get_body } from '@sveltejs/kit/http';

 export default async (req, res) => {
-   const host = `${req.headers['x-forwarded-proto']}://${req.headers.host}`;
-   const { pathname, searchParams } = new URL(req.url || '', host);
+   const { pathname, searchParams } = new URL(req.url || '', 'http://localhost');

    const { render } = await import('./server/app.mjs');

but I don't know why tests didn't catch this.

wiesson commented 3 years ago

but I don't know why tests didn't catch this.

it works on my machine 😅 - I'm wondering why. I did the initial part of #774, tested it with vercel and it was fine.


  System:
    OS: macOS 11.2.3
    CPU: (8) arm64 Apple M1
    Memory: 105.28 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 15.12.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.6.3 - /usr/local/bin/npm
  Browsers:
    Chrome: 89.0.4389.114
    Firefox: 87.0
    Safari: 14.0.3

> sveltekit3@0.0.1 build
> svelte-kit build

vite v2.1.5 building for production...
✓ 18 modules transformed.
.svelte/output/client/_app/manifest.json                            0.67kb
.svelte/output/client/_app/assets/start-aab6d365.css                0.29kb / brotli: 0.18kb
.svelte/output/client/_app/assets/pages/index.svelte-8cc0db3a.css   0.69kb / brotli: 0.26kb
.svelte/output/client/_app/pages/index.svelte-e1ea79cf.js           1.58kb / brotli: 0.68kb
.svelte/output/client/_app/chunks/vendor-b2527fc7.js                5.14kb / brotli: 2.00kb
.svelte/output/client/_app/start-941b560a.js                        15.47kb / brotli: 5.26kb
vite v2.1.5 building SSR bundle for production...
✓ 15 modules transformed.
.svelte/output/server/app.js   70.18kb

Run npm start to try your app locally.

> Using @sveltejs/adapter-node
  ✔ done
Conduitry commented 3 years ago

No, the Vercel part is fine - it's just doing more than is needed. (We don't need an accurate base URL to resolve req.url against.) The problem is in the Node adapter, where new URL(req.url) crashes because req.url isn't a whole URL, just the path part.

Conduitry commented 3 years ago

This snuck through CI because the adapters don't currently have tests - #639.

I'll open a PR with a fix for the Node adapter and the simplification for the Vercel adapter.

Conduitry commented 3 years ago

This is fixed in @sveltejs/adapter-node@1.0.0-next.12.