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

undici's new Request is incompatible with HTTP/2 requests #11365

Closed
Elsoberanold opened this issue Dec 16, 2023 · 18 comments · Fixed by #12907 or #12989
Closed

undici's new Request is incompatible with HTTP/2 requests #11365

Elsoberanold opened this issue Dec 16, 2023 · 18 comments · Fixed by #12907 or #12989

Comments

@Elsoberanold
Copy link

Describe the bug

After migration to SvelteKit 2 serving throw https just stopped working. It does not crash on execution but at the moment we reach exposed endpoint it returns the following error:

TypeError: Headers.append: ":method" is an invalid header name.
    at webidl.errors.exception (node:internal/deps/undici/undici:1635:14)
    at webidl.errors.invalidArgument (node:internal/deps/undici/undici:1646:28)
    at appendHeader (node:internal/deps/undici/undici:2052:29)
    at fill (node:internal/deps/undici/undici:2038:11)
    at new Request (node:internal/deps/undici/undici:6151:13)
    at getRequest (file:///Users/gfbatista/Projects/skedmill_ts/frontend/node_modules/@sveltejs/kit/src/exports/node/index.js:101:9)
    at file:///Users/gfbatista/Projects/skedmill_ts/frontend/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:475:27

Reproduction

This error can be reproduced by starting a new SvelteKit 2 project and changing vite.config.ts accordingly with documentation provided here: https://vitejs.dev/guide/migration#remove-https-flag-and-https-true . I have tried to use https://github.com/vitejs/vite-plugin-basic-ssl and https://github.com/liuweiGL/vite-plugin-mkcert both with same result.
vite.config.ts file:

import { sveltekit } from '@sveltejs/kit/vite';
import basicSsl from '@vitejs/plugin-basic-ssl'
import { defineConfig } from 'vite';

export default defineConfig({
	plugins: [basicSsl(), sveltekit()]
});

### Logs

```Shell
TypeError: Headers.append: ":method" is an invalid header name.
    at webidl.errors.exception (node:internal/deps/undici/undici:1635:14)
    at webidl.errors.invalidArgument (node:internal/deps/undici/undici:1646:28)
    at appendHeader (node:internal/deps/undici/undici:2052:29)
    at fill (node:internal/deps/undici/undici:2038:11)
    at new Request (node:internal/deps/undici/undici:6151:13)
    at getRequest (file:///.../node_modules/@sveltejs/kit/src/exports/node/index.js:101:9)
    at file:///.../node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:475:27


### System Info

```Shell
System:
    OS: macOS 14.2
    CPU: (8) arm64 Apple M1 Pro
    Memory: 130.47 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.4.0 - ~/.nvm/versions/node/v21.4.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.12.1/bin/yarn
    npm: 10.2.4 - ~/.nvm/versions/node/v21.4.0/bin/npm
    pnpm: 7.27.0 - ~/.nvm/versions/node/v18.12.1/bin/pnpm
    bun: 1.0.13 - ~/.bun/bin/bun
  Browsers:
    Safari: 17.2
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.0 => 3.0.0 
    @sveltejs/kit: ^2.0.0 => 2.0.0 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.0.1 
    svelte: ^4.2.7 => 4.2.8 
    vite: ^5.0.0 => 5.0.10

Severity

serious, but I can work around it

Additional Information

I think that should be a resurgence of this issue: #3479
I have tryed this work around and it works:

vite.config.ts:

import basicSsl from '@vitejs/plugin-basic-ssl'
import { defineConfig } from 'vite';

export default defineConfig({
	server: {
		proxy: {}
	},
	plugins: [basicSsl(), sveltekit()]
});```
@benmccann
Copy link
Member

It looks like @Conduitry removed the previous workaround in #7142 (which had been added in #3572). I'm not sure what's different about Vite 5 that's triggering this issue now

@CoolLaboratory
Copy link

CoolLaboratory commented Dec 18, 2023

I generated my certs with mkcert and load them manually.

import { sveltekit } from '@sveltejs/kit/vite';
import { defineConfig } from 'vite';
import { readFileSync } from 'node:fs';
import { resolve } from 'node:path';
import type { ServerOptions } from 'vite';

export default defineConfig(({command}) => {

  switch (command) {
    case 'serve': 
      const { key, cert } = JSON.parse(
        readFileSync(resolve(__dirname, 'path/to', '__certs', 'www.json'), 'utf-8'));
			
    const server: ServerOptions = {
      host: 'www.acme.test',
      port: 3002,
      strictPort: true,
      https: { key, cert },
      //  without the proxy does NOT work. 
      proxy: {}
    };
		
    return {
      server,
      plugins: [sveltekit()]
    }
    default: return { plugins: [sveltekit()] }
  }
});

the helper function i came up.

const { createCA, createCert } = require('mkcert');
const { writeFileSync, readFile } = require('node:fs');
const { resolve } = require('node:path');

(async () => {
  const { key: caKey, cert: caCert } = await createCA({
    organization: 'ACME CA',
    countryCode: 'GR',
    state: 'Attica',
    locality: 'Athens',
    validity: 365
  });


  try {
      await readFile (
        resolve(__dirname, 'path/to', '__certs', 'www.json'),
        'utf-8'
      ) // DO NOTHING
    } catch (error) {
      // ELSE CREATE A NEW CERT
      const wwwCert = await createCert({
        ca: { key: caKey, cert: caCert },
        domains: ['127.0.0.1', 'localhost', 'www.acme.test'],
        validity: 365
      });
    
      writeFileSync(
        resolve(__dirname, ''path/to', '__certs', 'www.json'),
        JSON.stringify({ key: wwwCert.key, cert: wwwCert.cert }, null, 2)
      );
    }

})()

for a work round..

@hronro
Copy link

hronro commented Jan 5, 2024

May related to #11213

For workarounds, you can try downgrade Node.js to v20 or try disable HTTP/2 in your browser.

@giviz
Copy link

giviz commented Jan 21, 2024

I have the same issue while upgrading to vite 5 and using node 21.
Downgrading to node 20 fix the issue.

@bogacg
Copy link

bogacg commented Feb 12, 2024

I'm using the solution @Elsoberanold provides in the last paragraph (Additional Information) with a tiny change, server.https instead of server.proxy.

import basicSsl from '@vitejs/plugin-basic-ssl'
import { defineConfig } from 'vite';

export default defineConfig({
	server: {
		https: {}
	},
	plugins: [basicSsl(), sveltekit()]
});```

This way you force HTTPS but it also let you run with --host option when you need to test your app in local network.

Update:

After few days (and few minor version updates in Node 21) I had exact same error @Elsoberanold shares above with this plugin config.
nvm use 20 is for the rescue, for now.

@liamdiprose
Copy link

liamdiprose commented Mar 5, 2024

Downgrading to node 20 worked initially, but then it would crash with a "Transfer-Encoding" header being present.

Adding server.proxy: {} worked:

export default defineConfig({
    server: {
        https: {
            ...
        },
        proxy: {},
    },
     ...
})

@mattpilott
Copy link

mattpilott commented Apr 6, 2024

I've just run into this after updating to 20.12.1

TypeError: Headers.append: ":method" is an invalid header name.
    at webidl.errors.exception (node:internal/deps/undici/undici:1801:14)
    at webidl.errors.invalidArgument (node:internal/deps/undici/undici:1812:28)
    at appendHeader (node:internal/deps/undici/undici:2218:29)
    at fill (node:internal/deps/undici/undici:2204:11)
    at new Request (node:internal/deps/undici/undici:6316:13)
    at getRequest (file:///Users/Matt/Sites/better/node_modules/@sveltejs/kit/src/exports/node/index.js:107:9)
    at file:///Users/Matt/Sites/better/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:497:27
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Log

  System:
    OS: macOS 14.5
    CPU: (16) arm64 Apple M3 Max
    Memory: 1.36 GB / 48.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.12.1 - /opt/homebrew/opt/node@20/bin/node
    npm: 10.5.0 - /opt/homebrew/opt/node@20/bin/npm
    pnpm: 8.15.6 - /opt/homebrew/bin/pnpm
    bun: 1.1.2 - ~/.bun/bin/bun
  Browsers:
    Safari: 17.5
  npmPackages:
    @sveltejs/adapter-vercel: ^5.2.0 => 5.2.0 
    @sveltejs/kit: ^2.5.5 => 2.5.5 
    @sveltejs/vite-plugin-svelte: ^3.0.2 => 3.0.2 
    svelte: ^4.2.12 => 4.2.12 
    vite: ^5.2.8 => 5.2.8 

@bogacg
Copy link

bogacg commented Apr 7, 2024

@mattpilott I also did get that error again (I guess it was last week or early this week) and then updated my packages, clean install and run it in Node 21 latest this time. Added proxy: {} option to config as @liamdiprose advised. I've been running error free this week.

@mattpilott
Copy link

@bogacg yes proxy worked for me too!

@kamerat
Copy link
Contributor

kamerat commented Apr 12, 2024

I tested different recent node versions to see where it broke and it seems v20.12.1 is where this error occurs for me.

@m1027
Copy link

m1027 commented Apr 18, 2024

A simple downgrade from v20.12.1 back to v20.11.0, without any other config file change, worked here.

@dlebech
Copy link

dlebech commented Apr 19, 2024

@kamerat I see the same. The error occurs when upgrading from Node v20.12.0 to v20.12.1. The server.proxy: {} setting did not work for me.

Here's the changelog. Based on the stracktrace above, the error is being thrown in the undici library, which seems to have been updated from v5.28.3 to v5.28.4 in Node v20.12.1, but I'm having trouble seeing what specifically should have changed between these versions to cause this error.

Interestingly, it seems that there was an attempt at fixing this a long time ago in Sveltekit which was then reverted back again: #7142

Update 2024-05-02

This is also an issue on Node 22.1.0

@th0rgall
Copy link

I just upgraded a project to SvelteKit 2 (2.7.1), and I believe I'm experiencing a similar issue with a different error log. I'm using vite-plugin-mkcert@1.17.6 to enable the https server.

I was using node v20.18.0. Downgrading to v20.11.0 as suggested above was also a functional workaround for me.

TypeError: Request constructor: init.headers is a symbol, which cannot be converted to a DOMString.
    at webidl.errors.exception (node:internal/deps/undici/undici:3384:14)
    at webidl.converters.DOMString (node:internal/deps/undici/undici:3650:29)
    at webidl.converters.ByteString (node:internal/deps/undici/undici:3658:35)
    at Object.record<ByteString, ByteString> (node:internal/deps/undici/undici:3567:30)
    at webidl.converters.HeadersInit (node:internal/deps/undici/undici:8715:67)
    at Object.RequestInit (node:internal/deps/undici/undici:3624:21)
    at new Request (node:internal/deps/undici/undici:9267:34)
    at getRequest (file:///<project dir>/node_modules/@sveltejs/kit/src/exports/node/index.js:109:9)
    at file:///<project dir>/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:497:27
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

th0rgall added a commit to WelcometoMyGarden/welcometomygarden that referenced this issue Oct 18, 2024
- refactor: share code to detect relative URLs and apply it in other places with potential absolute URLs, as only relative URLs should be handled by SK's goto()
- refactor: replace our own implementation of PaymentElement, which was added to support passing options to it, with the updated upstream svelte-stripe component that also supports it
- build(deps): update packages that needed to be updated for SvelteKit v2 support
- remove now-irrelevant { server.https } option from the vite config (vite v5)
- locks node to v20.11.0 in the root to enable https in the dev server, see sveltejs/kit#11365 (comment)
- some docs fixes
@dlebech
Copy link

dlebech commented Nov 8, 2024

@benmccann Even though this issue was initially about https, it might still have (re-)surfaced a potential issue with the getRequest implementation, which I don't think was solved with #12907 .

The getRequest function forwards the headers from the original request without inspection. This results in custom servers with http/2 not working, because of the header error.

Funny enough, the specific error message changes a bit over time:

  • Node 20.12.1 (where the error starts occuring):
    • TypeError: Headers.append: ":method" is an invalid header name.
  • Node 20.18.0 (as @th0rgall mentions) and also in Node 22:
    • TypeError: Request constructor: init.headers is a symbol, which cannot be converted to a DOMString.

I have added a minimal reproduction here: https://github.com/dlebech/sveltekit-11365

Perhaps the discussion is more relevant to continue in #1451 though?

@dominikg
Copy link
Member

dominikg commented Nov 8, 2024

that reads a lot like the undici version used in node was being updated and contains changes that are incompatible to what sveltekit has been doing. We had a lot of issues early on with undici not behaving to spec or doing breaking changes in minors.

Are you sure the calls sveltekit is doing are outside of what should be possible with the fetch api and headers? I'd look into the error thrown, the call made to fetch api and report it to undici

@dlebech
Copy link

dlebech commented Nov 8, 2024

@dominikg Yes, I believe the error is thrown by undici, because it rejects any header name that contains a colon :. I think it's here:

Whether or not this rejection is according to spec is a question that I can't find a definitive answer to, but it kind of makes sense it would reject a http/2 specific header, when undici brands itself explicitly as a http/1.1 client.

Either way, Sveltekit's usage of new Request in getRequest does feel a bit off here, because undici is technically just a fetch implementation, but we're not making a fetch request, which is what the new Request would normally be for. Instead, it's using the new Request constructor so the incoming request can be manipulated and used in subsequent code by the user and other Sveltekit functions. It turns an incoming request (from whatever server is being used) into a (technically) outgoing fetch request and then pretends the incoming request is an undici request, which... it is not. Or am I completely misunderstanding something here?

In other words: I am suggesting that Sveltekit probably shouldn't wrap/recreate the incoming request as a (technically) fetch request at all, or at least if it does, then ensure that it can in fact be converted correctly for both http/1.1 and http/2. But then it's probably more appropriate for discussion in the open thread here: #1451

(In my specific case, the incoming request is a nodejs Http2ServerRequest which has the pseudo-headers :method etc. If I manually filter out the headers starting with : in the getRequest function, my custom server works, but probably not as intended)

@dominikg
Copy link
Member

dominikg commented Nov 8, 2024

undici supports http/2 behind a flag and they mentioned wanting to support h2c too, maybe file a request with them to get the ball rolling? I believe their end would be the most beneficial to solve this for the wider community, if we worked around it here, other implementations would also have to work around it.

Either way if they confirm that they do not want Request to be able to be used this way, we have confirmation that work is needed on our end.

nodejs/undici#2868 (comment)

@benmccann benmccann reopened this Nov 8, 2024
@benmccann benmccann changed the title https is no longer working with SvelteKit 2 undici's new Request is incompatible with HTTP/2 requests Nov 8, 2024
@benmccann benmccann removed the vite label Nov 8, 2024
@benmccann
Copy link
Member

Yeah, I think I'm coming to the same conclusion that undici's Request isn't a great fit for our usecase. The HTTP/2 pseudo headers like :method are set in the fetch request by the browser. You can't do something like new Request('/', { headers: { ':method': 'POST' }}); in the browser and so I understand why undici rejects those for compatibility

So I think we have two options here:

  • Option 1 would be to try to map HTTP/2 headers to HTTP 1.1 headers. E.g. we could map :method to method and :authority to host and then drop :scheme and :path. It's not quite representative of what the server is actually receiving, but in some ways it makes things easier for the user and framework as you could just check for method vs checking method and :method. We'd have to make this change in each adapter
  • Option 2 would be to have our own Request implementation whether by forking undici, writing our own, etc.

Option 2 sounds far harder to implement, so I would lean towards option 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet