Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adapter-node is erroring on all requests #801

Closed
samccone opened this issue Mar 31, 2021 · 8 comments
Closed

adapter-node is erroring on all requests #801

samccone opened this issue Mar 31, 2021 · 8 comments
Labels
bug Something isn't working pkg:adapter-node

Comments

@samccone
Copy link
Contributor

samccone commented Mar 31, 2021

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 Conduitry added the bug Something isn't working label Mar 31, 2021
@Conduitry
Copy link
Member

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

@Conduitry
Copy link
Member

const parsed = new URL(req.url || '');

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

wiesson commented Mar 31, 2021

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
Copy link
Member

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
Copy link
Member

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.

@antony antony changed the title adapt-node is erroring on all requests adapter-node is erroring on all requests Mar 31, 2021
@Conduitry
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:adapter-node
Projects
None yet
Development

No branches or pull requests

4 participants