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.

Sign up for GitHub

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

Jump to bottom

Add Set-Cookie as a forbidden header name #1453

Merged
merged 4 commits into from
Jun 28, 2022
Merged

Add Set-Cookie as a forbidden header name #1453

merged 4 commits into from
Jun 28, 2022

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Jun 14, 2022 •
edited by pr-preview bot
Loading

Following a suggestion from @annevk in #973, this adds set-cookie as a
forbidden request header name. The rationale here is to prevent complication of
internal data structures for internal header lists that are attached to
Request objects, in light of #1346.

I added a UseCounter to Chromium in January to check that this is a web
compatible change. The analysis uncovered no issues, and as such I consider that
this change is web compatible. See
#973 (comment) for more
elaboration.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

@lucacasonato lucacasonato mentioned this pull request Jun 14, 2022
Merged
annevk
annevk reviewed Jun 14, 2022
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note at the end explaining why Set-Cookie is part of this list as it might surprise some folks.

@youennf this addresses your concern with respect to exposing Set-Cookie on Headers, right?

@yutakahirano @ddragana @KershawChang concerns?

@youennf
Copy link
Collaborator

youennf commented Jun 14, 2022

@youennf this addresses your concern with respect to exposing Set-Cookie on Headers, right?

Right.
I still do not see high value for UAs to implement Set-Cookie specific handling but it should not be too complex to do it.

@lucacasonato
Copy link
Member Author

Please add a note at the end explaining why Set-Cookie is part of this list as it might surprise some folks.

Done.

annevk
annevk reviewed Jun 15, 2022
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a nit commit to combine this note with the existing one. Let me know what you think. I'm happy with this.

@lucacasonato
Copy link
Member Author

Thanks. Looks great.

@yutakahirano
Copy link
Member

@yutakahirano @ddragana @KershawChang concerns?

No concerns.

@KershawChang
Copy link

@yutakahirano @ddragana @KershawChang concerns?

No concerns. This makes sense to me.

@lucacasonato
Copy link
Member Author

Implementation bugs are now filed.

aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 22, 2022
@nidhijaju
Block "Set-Cookie" header
d773817 
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 <[email protected]>
Reviewed-by: Shakti Sahu <[email protected]>
Commit-Queue: Nidhi Jaju <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1016574}
annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 28, 2022
@lucacasonato
Fetch: add Set-Cookie as an invalid request header
5fb9aee 
See whatwg/fetch#1453 for context.
@annevk annevk merged commit 50d77e6 into whatwg:main Jun 28, 2022
@lucacasonato lucacasonato deleted the set-cookie branch June 28, 2022 13:21
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 4, 2022
@lucacasonato @moz-wptsync-bot
Bug 1774167 [wpt PR 34424] - fetch: Add Set-Cookie as an invalid requ…
01e48e2 
…est header, a=testonly

Automatic update from web-platform-tests
Fetch: add Set-Cookie as an invalid request header

See whatwg/fetch#1453 for context.
--

wpt-commits: 5fb9aee1a0d59531dbe4be9300d4036e00ba2eb5
wpt-pr: 34424
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jul 7, 2022
@lucacasonato @moz-wptsync-bot
Bug 1774167 [wpt PR 34424] - fetch: Add Set-Cookie as an invalid requ…
fb300ef 
…est header, a=testonly

Automatic update from web-platform-tests
Fetch: add Set-Cookie as an invalid request header

See whatwg/fetch#1453 for context.
--

wpt-commits: 5fb9aee1a0d59531dbe4be9300d4036e00ba2eb5
wpt-pr: 34424
@charliewolfe charliewolfe mentioned this pull request Jul 28, 2022
Merged
webkit-early-warning-system pushed a commit to charliewolfe/WebKit that referenced this pull request Aug 11, 2022
@charliewolfe @pascoej
Add Set-Cookie as a forbidden header name
593162b 
https://bugs.webkit.org/show_bug.cgi?id=241703
<rdar://95811508>

Reviewed by J Pascoe.

Added Set-Cookie as a forbidden request header.
whatwg/fetch#1453

* LayoutTests/imported/w3c/web-platform-tests/fetch/api/basic/request-forbidden-headers.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fetch/api/basic/request-forbidden-headers.any.js:
* LayoutTests/imported/w3c/web-platform-tests/fetch/api/basic/request-forbidden-headers.any.worker-expected.txt:
* Source/WebCore/platform/network/HTTPParsers.cpp:
(WebCore::isForbiddenHeaderName):
* LayoutTests/http/tests/cookies/cookie-with-multiple-level-path.html:
* LayoutTests/http/tests/cookies/resources/cookie-utilities.js:
* LayoutTests/http/tests/cookies/resources/cookies-test-pre.js:
(setCookies):
* LayoutTests/http/tests/cookies/resources/setCookies.cgi:
* LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:
* LayoutTests/http/tests/navigation/ping-attribute/resources/utilities.js:
(setCookie):
* LayoutTests/http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies-when-private-browsing-enabled.py:
* LayoutTests/http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies-when-private-browsing-toggled.py:
* LayoutTests/http/tests/security/contentSecurityPolicy/report-cross-origin-no-cookies.py:
* LayoutTests/http/tests/security/contentSecurityPolicy/report-same-origin-no-cookies-when-private-browsing-toggled.py:
* LayoutTests/http/tests/security/contentSecurityPolicy/report-same-origin-with-cookies-when-private-browsing-enabled.py:
* LayoutTests/http/tests/security/contentSecurityPolicy/report-same-origin-with-cookies.py:
* LayoutTests/http/tests/app-privacy-report/app-attribution-beacon-isappinitiated.html:
* LayoutTests/http/tests/app-privacy-report/app-attribution-beacon-isnotappinitiated.html:
* LayoutTests/http/tests/blink/sendbeacon/beacon-cookie.html:
* LayoutTests/http/tests/resourceLoadStatistics/delete-script-accessible-cookies.html:
* LayoutTests/http/tests/resourceLoadStatistics/exemptDomains/app-bound-domains-exempt-from-website-data-deletion-database.html:
* LayoutTests/http/tests/resourceLoadStatistics/exemptDomains/app-bound-domains-exempt-from-website-data-deletion.html:
* LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-not-removed-with-user-interaction-6-days-ago.html:
* LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-with-user-interaction-7-days-ago.html:
* LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-website-data-removed.html:
* LayoutTests/http/tests/resourceLoadStatistics/standalone-web-application-exempt-from-website-data-deletion.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-js-cookie-checking.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-with-user-interaction.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-without-user-interaction-js-cookie-checking.html:
* LayoutTests/http/tests/resourceLoadStatistics/website-data-removal-for-site-without-user-interaction.html:

Canonical link: https://commits.webkit.org/253325@main
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@nidhijaju @copybara-github
Block "Set-Cookie" header
6a62f0c 
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 <[email protected]>
Reviewed-by: Shakti Sahu <[email protected]>
Commit-Queue: Nidhi Jaju <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1016574}
NOKEYCHECK=True
GitOrigin-RevId: d77381726e0abdf260324bde656ff37620199f87
@jmanico
Copy link

jmanico commented Mar 24, 2023

I struggle to see why Set-Cookie (a response header) is now a forbidden Request header.

Is this just to make sure developers do not use Set-Cookie in a request (demonstrating a misunderstanding of cookies)? HTTPOnly currently stops reading cookies from response headers and Set-Cookie never appears in requests since Set-Cookie is a response header.

@annevk
Copy link
Member

annevk commented Mar 24, 2023

As I said on Twitter, it's explained by the note this PR added. Set-Cookie is the only header that cannot be combined (in the HTTP sense) and we don't want implementations to have to support that for requests as it would require a special side table in optimized implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

Assignees
No one assigned
Labels
topic: cookies
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lucacasonato @youennf @yutakahirano @KershawChang @jmanico @annevk