When Content-Length is specified, fetch() raises RequestContentLengthMismatchError on redirection

[geryogam]

Bug Description

When the Content-Length request header field is manually specified (instead of automatically generated), fetch() raises a RequestContentLengthMismatchError in case of redirection.

Reproducible By

Send a POST request to the resource identified by the /sirene/public/recherche target URI to search for a non-existing company whose name foobarbaz is provided in the request body:

const uri = 'https://www.sirene.fr/sirene/public/recherche';
const method = 'POST';
const body = 'recherche.sirenSiret=&recherche.raisonSociale=foobarbaz&recherche.adresse=&recherche.commune=&recherche.excludeClosed=true&__checkbox_recherche.excludeClosed=true&recherche.captcha=';
const headers = {'Content-Type': 'application/x-www-form-urlencoded', 'Content-Length': Buffer.byteLength(body)};
const response = await fetch(uri, {method, headers, body});

The resource replies with a 302 response with a Location: /sirene/error/autre.action header field. So fetch automatically (by default) redirects the original request to the target URI /sirene/error/autre.action and raises a RequestContentLengthMismatchError:

Uncaught TypeError: fetch failed
    at Object.fetch (node:internal/deps/undici/undici:11413:11)
    at async REPL5:1:47 {
  cause: RequestContentLengthMismatchError: Request body length does not match content-length header
      at write (node:internal/deps/undici/undici:9907:41)
      at _resume (node:internal/deps/undici/undici:9885:33)
      at resume (node:internal/deps/undici/undici:9787:7)
      at connect (node:internal/deps/undici/undici:9776:7) {
    code: 'UND_ERR_REQ_CONTENT_LENGTH_MISMATCH'
  }
}

Expected Behavior

The specified Content-Length request header field is correct in the original request so I don’t expect a RequestContentLengthMismatchError after automatic redirection.

Logs & Screenshots

I assume that during redirection, fetch() resends the original request but with the following modifications:

• the target URI is changed from /sirene/public/recherche to /sirene/error/autre.action;
• the POST request method is changed to GET;
• the request body is removed.

But it doesn’t remove the content-specific header fields (Content-Type and Content-Length here), contrary to what the latest HTTP specification RFC 9110 recommends:

When automatically following a redirected request, the user agent SHOULD resend the original request message with the following modifications:

1. Replace the target URI with the URI referenced by the redirection response's Location header field value after resolving it relative to the original request's target URI.
2. […]
3. […]
4. Change the request method according to the redirecting status code's semantics, if applicable.
5. If the request method has been changed to GET or HEAD, remove content-specific header fields, including (but not limited to) Content-Encoding, Content-Language, Content-Location, Content-Type, Content-Length, Digest, Last-Modified.

The non-compliance of Undici to point 5 likely causes the RequestContentLengthMismatchError. Indeed, sending a GET request without a body but with a Content-Length header field raises the same RequestContentLengthMismatchError:

fetch('http://example.com/', {headers: {'Content-Length': 0}})

Output:

Uncaught TypeError: fetch failed
    at Object.fetch (node:internal/deps/undici/undici:11413:11) {
  cause: RequestContentLengthMismatchError: Request body length does not match content-length header
      at write (node:internal/deps/undici/undici:9907:41)
      at _resume (node:internal/deps/undici/undici:9885:33)
      at resume (node:internal/deps/undici/undici:9787:7)
      at connect (node:internal/deps/undici/undici:9776:7) {
    code: 'UND_ERR_REQ_CONTENT_LENGTH_MISMATCH'
  }
}

Environment

MacOS 11.6.7, Node 19.7.0.

[mcollina]

Thanks for reporting!

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that. In this case we need both the server and the client.

A PR with a fix would also be highly appreciated!

[ronag]

@KhafraDev did we or the fetch spec miss a step in the redirection logic?

[KhafraDev]

No, the content-length header is a forbidden header, but we don't filter headers.

[geryogam]

Can you provide steps to reproduce?

@mcollina The code to reproduce the issue is already in the ‘Reproducible By’ section.

[ronag]

No, the content-length header is a forbidden header, but we don't filter headers.

What does the spec say? Do we need to filter or throw an error?

[KhafraDev]

The RFC mentioned in the OP recommends removing some headers on redirect, that's likely what we should do.

[ronag]

@annevk Do you have any guidance here?

[mcollina]

@mcollina The code to reproduce the issue is already in the ‘Reproducible By’ section.

As I said in #2021 (comment), we need both the client and server.

[KhafraDev]

Here's a small repro

import { once } from 'events'
import { createServer } from 'http'
import { fetch } from './index.js'

const server = createServer((req, res) => {
  if (req.url === '/redirect') {
    res.writeHead(302, { Location: '/redirect2' })
    res.end()
    return
  }

  console.log('hello', req.url)
  res.end()
}).listen(0)

await once(server, 'listening')

await fetch(`http://localhost:${server.address().port}/redirect`, {
  method: 'POST',
  body: 'a+b+c',
  headers: {
    'content-length': Buffer.byteLength('a+b+c')
  }
})

[ronag]

@KhafraDev

To validate a [header](https://fetch.spec.whatwg.org/#concept-header) (name, value) for a [Headers](https://fetch.spec.whatwg.org/#headers) object headers:

If name is not a [header name](https://fetch.spec.whatwg.org/#header-name) or value is not a [header value](https://fetch.spec.whatwg.org/#header-value), then [throw](https://webidl.spec.whatwg.org/#dfn-throw) a [TypeError](https://webidl.spec.whatwg.org/#exceptiondef-typeerror).

If headers’s [guard](https://fetch.spec.whatwg.org/#concept-headers-guard) is "immutable", then [throw](https://webidl.spec.whatwg.org/#dfn-throw) a [TypeError](https://webidl.spec.whatwg.org/#exceptiondef-typeerror).

If headers’s [guard](https://fetch.spec.whatwg.org/#concept-headers-guard) is "request" and (name, value) is a [forbidden request-header](https://fetch.spec.whatwg.org/#forbidden-request-header), then return false.

If headers’s [guard](https://fetch.spec.whatwg.org/#concept-headers-guard) is "response" and name is a [forbidden response-header name](https://fetch.spec.whatwg.org/#forbidden-response-header-name), then return false.

Return true.

The "request" guarded header validation should fail with content-length and therefore it should just be thrown away as far as I can see?

[ronag]

// Note: undici does not implement forbidden header names

Why is that?

[ronag]

I don't think our code any longer corresponds with https://fetch.spec.whatwg.org/#headers-class. Maybe time to have another iteration?

[KhafraDev]

The headers are being removed in fetch, this is a bug elsewhere:

undici/lib/fetch/index.js

Lines 1177 to 1195 in e6fc80f

	// 12. If one of the following is true
	// - actualResponse’s status is 301 or 302 and request’s method is `POST`
	// - actualResponse’s status is 303 and request’s method is not `GET` or `HEAD`
	if (
	([301, 302].includes(actualResponse.status) && request.method === 'POST') ||
	(actualResponse.status === 303 &&
	!['GET', 'HEAD'].includes(request.method))
	) {
	// then:
	// 1. Set request’s method to `GET` and request’s body to null.
	request.method = 'GET'
	request.body = null
	// 2. For each headerName of request-body-header name, delete headerName from
	// request’s header list.
	for (const headerName of requestBodyHeader) {
	request.headersList.delete(headerName)
	}
	}

where requestBodyHeader is

const requestBodyHeader = [
  'content-encoding',
  'content-language',
  'content-location',
  'content-type'
]

[KhafraDev]

Oh nvm, adding content-length to that list fixes it lol

[ronag]

But that list should not include it:

A request-body-header name is a [header name](https://fetch.spec.whatwg.org/#header-name) that is a [byte-case-insensitive](https://infra.spec.whatwg.org/#byte-case-insensitive) match for one of:

`Content-Encoding`
`Content-Language`
`Content-Location`
`Content-Type`

[ronag]

I think it's in the header append guard logic where it should be removed.

[KhafraDev]

Why is that?

It's not powerful enough in the server; it disallows setting a bunch of headers. We purposefully deviate from the spec to match deno/other server environments.

See
#1262
#1463

But that list should not include it:

I'm assuming that this is because normally it's already removed in the Headers impl. I think it's fair to add it to that list and add a comment about why it's there (along with a comment linking to the spec).

[KhafraDev]

Maybe time to have another iteration?

I've purposefully kept it close to Rafael's version, as it makes headers faster by tenfold (using a map instead of an array). Unfortunately that decision snowballed to where the implementation is completely different from what the spec says to do.

[ronag]

It's not powerful enough in the server; it disallows setting a bunch of headers. We purposefully deviate from the spec to match deno/other server environments.

Rather than allow everything I think it's better to deviate on specific header names where it makes sens.

[ronag]

Maybe time to have another iteration?

I've purposefully kept it close to Rafael's version, as it makes headers faster by tenfold (using a map instead of an array). Unfortunately that decision snowballed to where the implementation is completely different from what the spec says to do.

I have mixed feelings on this... Not sure what to think.

[KhafraDev]

I have a branch that makes it spec compliant (see: https://github.com/KhafraDev/undici/tree/change-headerslist-to-array), but after benchmarking, the performance was so bad that I decided against it. I actually published that branch for this exact reason - to show how badly it performs compared to our current impl.

Rather than allow everything I think it's better to deviate on specific header names where it makes sens.

I don't think we should filter any headers in fetch. Adding content-length to requestBodyHeader fixes the issue in a spec-compliant & rfc compliant way.

[annevk]

https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch defines how to set Content-Length. If you've set body to null due to a redirect that removes the body and as such it won't set Content-Length. (Content-Length doesn't get copied either as it's set later in the pipeline.)

[geryogam]

I don't think we should filter any headers in fetch. Adding content-length to requestBodyHeader fixes the issue in a spec-compliant & rfc compliant way.

Yes, currently the list is (https://github.com/nodejs/undici/blob/v5.21.0/lib/fetch/constants.js#L51-L56):

const requestBodyHeader = [
  'content-encoding',
  'content-language',
  'content-location',
  'content-type'
]

To be RFC 9110 compliant, we should remove the following header fields (https://www.rfc-editor.org/rfc/rfc9110#section-15.4-6.5.1):

5. If the request method has been changed to GET or HEAD, remove content-specific header fields, including (but not limited to) Content-Encoding, Content-Language, Content-Location, Content-Type, Content-Length, Digest, Last-Modified.

So:

const requestBodyHeader = [
  'content-encoding',
  'content-language',
  'content-location',
  'content-type',
+ 'content-length',
+ 'digest',
+ 'last-modified'
]

[KhafraDev]

digest isn't a filtered header name and last-modified is only a CORS safelisted header (not something undici implements)

[geryogam]

Thank you for solving this issue guys.