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

filter duplicate header #80

Closed
roccomuso opened this issue Jun 26, 2020 · 6 comments
Closed

filter duplicate header #80

roccomuso opened this issue Jun 26, 2020 · 6 comments

Comments

@roccomuso
Copy link

Since you have a lot of possibilities when gluing together methods you can easily mess up the requests.
I did something like:

wretch(url).headers( 'content-type': 'application/json' ).post(...).json()
// one of these methods added another application/json header

and saw that in my requests I had duplicate header: Content-Type: application/json, application/json. Causing my server to reject the request.
This shouldn't be possible. Maybe worth filtering duplicate header?

@elbywan
Copy link
Owner

elbywan commented Jun 26, 2020

Hi @roccomuso, thanks for reporting the issue 👍 !

Wretch stores the headers in using the standard Header interface api.

In your example, you are manually appending a lowercase content-type key, and then calling .json({ with_an_object: "argument" }) which appends the same header with a different case (Content-Type).

in my requests I had duplicate header: Content-Type: application/json, application/json

The behaviour when triggering a fetch with a Header object containing the same key with different case differs between browsers, Chrome (and I guess that's the one you are using) will concatenate the values (join with a comma separator) whereas Firefox will only use one of the values (and the request will work in that case).

So to me, the bug comes from the browser implementation of Header (or maybe a bad specification?) that should not allow setting headers with a different case in the first place.

But anyway, despite that I'll consider checking if a Content-Type header is already there and prevent the double entry in the Headers object when calling .json.

I will not be able to work on that in the following days as I don't have a lot of free time, but I'll keep the issue open and update the status when I'll be available to work on wretch again.

@roccomuso
Copy link
Author

Thanks for the immediate reply!
In other libs in the past I used to provide lower-case content type and they had it figured out, avoiding any duplication.

But yeah, it worked fine in node.js but got issues in chrome runtime. Better handle the case

@roccomuso
Copy link
Author

I stumbled again in this issue 1 year later and completely forgot I had encounter this already.
Definetly needs some improvement 🙏🏼

@jimmed
Copy link

jimmed commented Jun 15, 2021

I just stumbled across this issue too, although from the angle of specifying the character set as part of the content-type header.

The API I'm hitting requires us to specify content-type: application/json; charset=utf-8 otherwise it will consume any request body as ASCII by default, and wretch adds an additional content-type: application/json when calling .json().

I love wretch and appreciate you may not have time to address this yourself. I'll take a look at raising a PR for this.

@jimmed
Copy link

jimmed commented Jun 16, 2021

For anybody who needs this, I've written a small middleware that deduplicates headers with the same name.

import { Headers, HeadersInit } from 'node-fetch'
import { ConfiguredMiddleware } from 'wretch'

export const manipulateHeaders =
  (callback: (headers: Headers) => Headers): ConfiguredMiddleware =>
  (next) =>
  (url, { headers, ...opts }) => {
    const nextHeaders = callback(
      new Headers(headers as HeadersInit),
    ) as unknown as globalThis.Headers
    return next(url, { ...opts, headers: nextHeaders })
  }

export const dedupeHeaders = (
  dedupeHeaderLogic: Record<
    string,
    (values: string[]) => Iterable<string>
  > = {},
) => {
  const deduperMap = new Map(
    Object.entries(dedupeHeaderLogic).map(([k, v]) => [k.toLowerCase(), v]),
  )
  const dedupe = (key: string) =>
    deduperMap.get(key.toLowerCase()) ?? ((values: string[]) => new Set(values))

  return manipulateHeaders((headers) => {
    Object.entries(headers.raw()).forEach(([key, values]) => {
      const deduped = Array.from(dedupe(key)(values))
      headers.delete(key)
      deduped.forEach((value, index) =>
        headers[index ? 'append' : 'set'](key.toLowerCase(), value),
      )
    })
    return headers
  })
}

By default, it will deduplicate identical values for a given header. This can be used as follows:

wretch().middlewares([dedupeHeaders()])

If there is a specific header for which the defaults cause problems, then you can provide a callback to handle deduplication yourself:

wretch().middlewares([dedupeHeaders({
  Accept: (values) => values.filter(v => v !== '*/*')
})])

I hope this is helpful to somebody.

@elbywan
Copy link
Owner

elbywan commented Aug 1, 2022

Since @jimmed provided a great workaround in the form of a middleware (thanks! 🙇) I think this issue can be safely closed.

@elbywan elbywan closed this as completed Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants