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

Use case for Headers getAll #973

Closed
xtuc opened this issue Nov 28, 2019 · 53 comments
Closed

Use case for Headers getAll #973

xtuc opened this issue Nov 28, 2019 · 53 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api

Comments

@xtuc
Copy link

xtuc commented Nov 28, 2019

Since 42464c8 Headers.prototype.getAll has been deprecated/removed from the spec and implementations.

I understand that in browsers (including service workers) filtered responses don't include headers that could benefit from the getAll method.
However, some JavaScript environment doesn't need to filter response/request headers, like serverless plateforms (for instance Cloudflare Workers) or maybe Nodejs at some point.

For example editing Cookie headers:

const h = new Headers;
h.append("Set-Cookie", "a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT");
h.append("Set-Cookie", "b=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT");

h.get("Set-Cookie")
// a=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT, b=1; Expires=Wed, 21 Oct 2015 07:28:00 GMT

Spliting the combined header value by , will give an invalid result. Instead getAll could be used to retrieve the individual headers.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api labels Nov 29, 2019
@annevk
Copy link
Member

annevk commented Nov 29, 2019

Set-Cookie is the only such header, see also HTTP and #506.

It might make sense to expose it properly for code that wants to use this class generically, but I think we ought to allow for implementations of this class to eagerly combine headers generally and store Set-Cookie separately as a special case. That would suggest something like h.getSetCookie() to me.

cc @youennf @ddragana @yutakahirano

@annevk
Copy link
Member

annevk commented Nov 29, 2019

It's probably also worth investigating if https://www.npmjs.com/package/set-cookie-parser#splitcookiestringcombinedsetcookieheader is a reasonable alternative to expose. Though if that actually works HTTP would not have to consider cookies a special case...

@xtuc
Copy link
Author

xtuc commented Nov 29, 2019

Maybe to clarify the environment a bit, Cloudflare Workers boils down to a proxy written in JavaScript and Node.js a HTTP client.

Set-Cookie is the only such header

For such use-cases, it make sense to not filter headers in the response and let the user define its own headers. Any custom headers may contain , .

@annevk
Copy link
Member

annevk commented Nov 29, 2019

I disagree, HTTP doesn't guarantee intermediaries won't combine such headers so we shouldn't either. I'd want HTTP to call out more than Set-Cookie as requiring a special data model before going there.

@domenic
Copy link
Member

domenic commented Nov 29, 2019

I'm a bit confused why we'd revisit this. Non-browser environments can consider adding nonstandard extensions for whatever purpose they wish, but in browser environments there's no need for this functionality, and our reasoning from the previous discussions seems to hold. (And the spec is written for browser environments.)

@annevk
Copy link
Member

annevk commented Nov 29, 2019

We don't forbid appending Set-Cookie unconditionally. When it's appended, there's no way to get it back out per HTTP semantics. That mismatch irks me a bit.

@xtuc
Copy link
Author

xtuc commented Nov 29, 2019

We are considering adding the getAll method for Cloudflare Workers, I don't know Node.js' plans.

With a basic filtered response you can access almost all the headers. I think this behavior can still happen in a browser.

@annevk
Copy link
Member

annevk commented Nov 30, 2019

To be clear, getAll() with the semantics I think you are suggesting would be incompatible with what we'd add here, if anything, as HTTP doesn't require preserving the distinction between

h.append(name, value)
h.append(name, value2)

and

h.append(name, `${value}, ${value2}`)

for any name other than Set-Cookie and this API shouldn't either. See also the discussion in #189.

@kentonv
Copy link

kentonv commented Dec 1, 2019

Non-browser environments can consider adding nonstandard extensions for whatever purpose they wish, but in browser environments there's no need for this functionality, and our reasoning from the previous discussions seems to hold. (And the spec is written for browser environments.)

In Cloudflare Workers, we'd really like to stick to standard APIs, to promote code portability across a wide range of JavaScript environments. We think it would be a great thing for the JavaScript ecosystem as a whole if common functionality like making HTTP requests actually worked the same everywhere, so it would be nice to see the fetch() standard as applying to more than just browsers.

We have an immediate need to support manipulation of Set-Cookie in Cloudflare Workers. Perhaps ironically, this is actually driven by a request from @rowan-m on the Chrome team related to the default SameSite=Lax change. A lot of people are going to need to work around this soon and doing it in CF Workers will likely be easier for many people than updating their backend code.

We are proceeding with implementing getAll() because although it is deprecated, the past semantics cover our needs, and we're more comfortable implementing something that has been specified in the past than something that has never been specified. The thing we really want to avoid is specifying a new API that turns out incompatible with implementations from other vendors, fragmenting the ecosystem. So while getSetCookie() might be better, getAll() has the advantage that we're pretty confident no one is going to implement it with different semantics. (Yes, I do understand that various platforms may make different decisions about when to automatically combine headers other than Set-Cookie, but I'm comfortable with that.)

@domenic
Copy link
Member

domenic commented Dec 1, 2019

I think that's a really unfortunate decision. I think you'd be better served by clearly signposting nonstandard APIs (e.g. using a Cloudflare global), instead of assuming that things which existed briefly in the spec but were never implemented anywhere have some sort of semi-standard semantics.

@kentonv
Copy link

kentonv commented Dec 1, 2019

I think you'd be better served by clearly signposting nonstandard APIs (e.g. using a Cloudflare global)

I thought vendor prefixes were no longer in favor?

existed briefly in the spec but were never implemented anywhere

Eh? Seems like it was implemented by almost every major browser at one point?

@annevk
Copy link
Member

annevk commented Dec 2, 2019

getAll() is not deprecated, it's removed, because its design is broken as explained here and in linked issues.

@kentonv
Copy link

kentonv commented Dec 2, 2019

@annevk Yes, I understand the issue -- for headers other than Set-Cookie, the header values may be comma-concatenated anyway by any intermediate proxy, so getAll() can't keep its promise.

Do you have a recommendation for how we should proceed, in order to both solve the immediate problem and stay standards-compatible long-term?

@annevk
Copy link
Member

annevk commented Dec 3, 2019

Again, not just by any intermediate proxy, by an implementation of the Headers class as well, as at least one implementer strongly argued for.

getSetCookie() still seems like a reasonable solution. @mnot thoughts?

@xtuc
Copy link
Author

xtuc commented Dec 3, 2019

getSetCookie() sounds good to me now. Could we consider adding it to the browser fetch spec?

@kentonv
Copy link

kentonv commented Dec 3, 2019

What about getAll(), but it is defined to throw an exception if passed any other header name than "Set-Cookie"?

This has the advantage that if ever there comes along another such header (probably, a non-standard header used by some badly-designed HTTP API that one of our customers really really needs to support), we can easily accommodate...

@annevk
Copy link
Member

annevk commented Dec 3, 2019

@youennf @yutakahirano @ddragana @whatwg/http thoughts?

@ddragana
Copy link

ddragana commented Dec 6, 2019

I looks like we cannot read Set-Cookie if we once have set it, therefore I am for adding some function for this.
getSetCookie sound good. I do not like getAll, because it sound like you can use it for every header field, but you cannot. If we want to make it generic getUnmergableHeader or something, but that is only for one header and it is unnecessary, so getSetCookie its fine.

@tonyjmnz
Copy link

cross-posting a suggestion from @nfriedly on #506 - use https://www.npmjs.com/package/set-cookie-parser#usage-in-react-native to parse the mangled cookie header

The fact that this inconsistency is still around is mind boggling to me

@lucacasonato
Copy link
Member

For future reference, this is how we solve this in Deno's fetch implementation:

  1. Setting Set-Cookie headers does not concatenate headers, it will keep them seperate (both in the internal data structure, and on the wire)
  2. h.get("Set-Cookie") returns a concatentated list of Set-Cookie headers.
  3. Iterating over h (or using h.entries, h.values, or h.keys) will result concatentated headers, as specified except for set-cookie headers. These are returned as is, without concatenation. That means it is possible that h.entries returns multiple items where the header name is set-cookie.

This was the solution of us trying to come up with a solution for multiple years. We deem this breaks the least existing code, while also being relatively easy to understand and use.

@kentonv
Copy link

kentonv commented Aug 20, 2021

For the record, Cloudflare Workers went ahead with what I suggested in my last comment -- getAll(), but it is only allowed for the specific header name Set-Cookie.

@adrianhopebailie
Copy link

adrianhopebailie commented Sep 29, 2022

Am I correct that step one of what @lucacasonato proposed in #973 (comment) has now been completed and the impact of special handling set-cookie is now low enough for this to be considered?

Was the conclusion to adopt an implementation like Deno's or to add a getSetCookie() method to the Headers object for this special case?

@annevk
Copy link
Member

annevk commented Sep 29, 2022

The latter, see #1346. It's currently stuck on getting implementer interest from two browsers.

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This commit adds a UseCounter that tracks if a site in any way sets a
header with the name "set-cookie" on a Headers object with a guard of
"request".

This is done to verify if it is a web compatible change to add
"set-cookie" to the request forbidden header list.
See whatwg/fetch#973 (comment)

Bug: 1292458
Change-Id: I1edd161d941d6490838b77a2fec008de904793ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3423793
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/main@{#965654}
NOKEYCHECK=True
GitOrigin-RevId: 4fa1c0bf6242c7bbdf97d25426d0dea7c7aa644c
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
As per whatwg/fetch#1453, the "Set-Cookie"
header is added to the list of forbidden request header names in the
Fetch spec. This is because the "Set-Cookie" header is semantically a
response header, so it is not useful on requests, and we want to
avoid leaking the complexity of handling them in requests. This CL
implements this change.

The impact of this change was already verified using a UseCounter[1], which showed that the % of pages that set a "set-cookie" header on
an outbound fetch request hovers around 0.0003%. Additionally, the
two popular domains that set a "set-cookie" header on request
headers continue to work even when all outbound "set-cookie" headers
were removed [2]. Hence, this change was deemed to be safe to make.

Some tests depended on the assumption that there are no overlapping
header names for forbidden request and response headers, which made
tests fail as 'Set-Cookie' exists in both lists of forbidden names
now. This has been fixed in this CL by finding the non-overlapping
header names.

As this change is sufficiently small, an intent to ship is not being
sent, but the enterprise team were notified to include this in
the release notes and a PSA to blink-dev was also sent[3].

[1] https://chromestatus.com/metrics/feature/timeline/popularity/4152
[2] whatwg/fetch#973 (comment)
[3] https://groups.google.com/a/chromium.org/g/blink-dev/c/SyHAsPfO004

Bug: 1337091
Change-Id: Idf8ffd9c1169e5b9045c5a7e282c4fbdda00f550
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3709684
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1016574}
NOKEYCHECK=True
GitOrigin-RevId: d77381726e0abdf260324bde656ff37620199f87
@FadyMak
Copy link

FadyMak commented Feb 1, 2023

FWIW, using getAll would maintain consistency with other Web APIs such as URLSearchParams.getAll(). It's also what is being used by Cloudflare Workers as @kentonv mentioned above.

I think getSetCookie is just fine - this comment is more to call out API consistency.

@andreubotella
Copy link
Member

andreubotella commented Feb 1, 2023

FWIW, using getAll would maintain consistency with other Web APIs such as URLSearchParams.getAll(). It's also what is being used by Cloudflare Workers as @kentonv mentioned above.

I think getSetCookie is just fine - this comment is more to call out API consistency.

getAll was removed from the specification because Webkit objected to it, since it would not be feasible for them to implement. As I see it, the only options for bringing getAll back without them objecting would be:

  • Allowing browsers to optionally combine all headers but Set-Cookie. This would introduce implementation-defined behavior, which isn't a good practice for things like this. It would also be impossible to effectively communicate to developers that this difference between browsers is intentional.
  • Mandating that all headers must be combined, with the single exception of Set-Cookie. This would mean the semantics of this method would be different from the previously defined getAll method. Furthermore, if server-side implementations like Cloudflare Workers switched to it, the change might possibly lead to code breaking in ways much harder to debug than if the method was removed.

In my opinion, both of these options are bad, and getSetCookie is much better than either.

@kentonv
Copy link

kentonv commented Feb 1, 2023

FWIW Cloudflare Workers' implementation of getAll() throws an exception if the header name is anything other than Set-Cookie.

However, the ship has clearly sailed here, getSetCookie is the standard that has been written and agreed upon.

@med-ab
Copy link

med-ab commented Jun 20, 2024

what about X-Robots-Tag for example where rules specified without a user agent are valid for all

console.log(
  new Headers([
    ['x-robots-tag', 'noindex'],
    ['x-robots-tag', 'unavailable_after: 25 Jun 2099 15:00:00'],
    ['x-robots-tag', 'somebot: nofollow'],
    ['x-robots-tag', 'otherbot: noindex, nofollow'],
    ['x-robots-tag', 'noarchive'],
  ]).get('x-robots-tag')
  ==
  'noindex, unavailable_after: 25 Jun 2099 15:00:00, somebot: nofollow, otherbot: noindex, nofollow, noarchive'
)

now is the noarchive rule for all or just for otherbot ?!

@annevk
Copy link
Member

annevk commented Jun 20, 2024

If you cannot split on comma, then X-Robots-Tag violates HTTP.

@med-ab
Copy link

med-ab commented Jun 20, 2024

If you cannot split on comma, then X-Robots-Tag violates HTTP.

where in the HTTP spec does it say that ?

@Mouvedia
Copy link

Mouvedia commented Jun 20, 2024

what about X-Robots-Tag for example where rules specified without a user agent are valid for all

console.log(
  new Headers([
    ['x-robots-tag', 'noindex'],
    ['x-robots-tag', 'unavailable_after: 25 Jun 2099 15:00:00'],
    ['x-robots-tag', 'somebot: nofollow'],
    ['x-robots-tag', 'otherbot: noindex, nofollow'],
    ['x-robots-tag', 'noarchive'],
  ]).get('x-robots-tag')
  ==
  'noindex, unavailable_after: 25 Jun 2099 15:00:00, somebot: nofollow, otherbot: noindex, nofollow, noarchive'
)

You always have to put the *bot last in your array.
i.e. noindex, noarchive should be the first 2 in your output
It doesn't fix , nofollow though.

@med-ab
Copy link

med-ab commented Jun 20, 2024

. i.e. noindex, noarchive should be the first 2 in your output

they "should" but what to do when a server sends them after other ua rules?

@annevk
Copy link
Member

annevk commented Jun 20, 2024

https://httpwg.org/specs/rfc9110.html#rfc.section.5.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api
Development

No branches or pull requests