Skip to content

Commit

Permalink
Block "Set-Cookie" header
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nidhijaju authored and Copybara-Service committed Jun 22, 2022
1 parent 2dc6b7a commit 6a62f0c
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ function setCookies(cookie)
try {
var xhr = new XMLHttpRequest();
xhr.open("GET", "resources/setCookies.cgi", false);
xhr.setRequestHeader("SET-COOKIE", cookie);
xhr.setRequestHeader("X-SET-COOKIE", cookie);
xhr.send(null);
if (xhr.status == 200) {
// This is to clear them later.
Expand Down
6 changes: 3 additions & 3 deletions blink/web_tests/http/tests/cookies/resources/setCookies.cgi
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use strict;
print "Content-Type: text/plain\n";
print "Access-Control-Allow-Origin: *\n";
print "Access-Control-Allow-Credentials: true\n";
print "Access-Control-Allow-Headers: SET-COOKIE\n";
print "Access-Control-Allow-Headers: X-SET-COOKIE\n";
print "Cache-Control: no-store\n";
print 'Cache-Control: no-cache="set-cookie"' . "\n";

# We only map the SET_COOKIE request header to "Set-Cookie"
print "Set-Cookie: " . $ENV{"HTTP_SET_COOKIE"} . "\n\n";
# We only map the X-SET-COOKIE request header to "Set-Cookie"
print "Set-Cookie: " . $ENV{"HTTP_X_SET_COOKIE"} . "\n\n";
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ var FORBIDDEN_HEADER_NAMES =
['Accept-Charset', 'Accept-Encoding', 'Access-Control-Request-Headers',
'Access-Control-Request-Method', 'Connection', 'Content-Length',
'Cookie', 'Cookie2', 'Date', 'DNT', 'Expect', 'Host', 'Keep-Alive',
'Origin', 'Referer', 'TE', 'Trailer', 'Transfer-Encoding', 'Upgrade',
'User-Agent', 'Via', 'Proxy-', 'Sec-', 'Proxy-FooBar', 'Sec-FooBar'];
'Origin', 'Referer', 'Set-Cookie', 'TE', 'Trailer', 'Transfer-Encoding',
'Upgrade', 'User-Agent', 'Via', 'Proxy-', 'Sec-', 'Proxy-FooBar', 'Sec-FooBar'];
var FORBIDDEN_RESPONSE_HEADER_NAMES =
['Set-Cookie', 'Set-Cookie2',
'set-cookie', 'set-cookie2',
Expand Down
36 changes: 32 additions & 4 deletions blink/web_tests/http/tests/fetch/script-tests/headers-guard.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,15 @@ test(function() {
testIgnoreHeaderName(headers, header);
});

// Take into account overlapping forbidden header names.
var lower = FORBIDDEN_HEADER_NAMES.map(header => {
return header.toLowerCase();
});
var non_overlapping_headers =
FORBIDDEN_RESPONSE_HEADER_NAMES.filter(x => !lower.includes(x.toLowerCase()));

// Other header names must be accepted.
FORBIDDEN_RESPONSE_HEADER_NAMES
non_overlapping_headers
.concat(SIMPLE_HEADER_NAMES)
.concat([CONTENT_TYPE])
.concat(NON_SIMPLE_HEADER_NAMES)
Expand Down Expand Up @@ -154,8 +161,15 @@ test(function() {
header + ' must be ignored (2)');
});

// Take into account overlapping forbidden header names.
var lower = FORBIDDEN_HEADER_NAMES.map(header => {
return header.toLowerCase();
});
var non_overlapping_headers =
FORBIDDEN_RESPONSE_HEADER_NAMES.filter(x => !lower.includes(x.toLowerCase()));

// Other header names must be accepted.
FORBIDDEN_RESPONSE_HEADER_NAMES
non_overlapping_headers
.concat(SIMPLE_HEADER_NAMES)
.concat([CONTENT_TYPE])
.concat(NON_SIMPLE_HEADER_NAMES)
Expand Down Expand Up @@ -316,8 +330,15 @@ test(function() {
testIgnoreHeaderName(headers, header);
});

// Take into account overlapping forbidden header names.
var lower = FORBIDDEN_RESPONSE_HEADER_NAMES.map(header => {
return header.toLowerCase();
});
var non_overlapping_headers =
FORBIDDEN_HEADER_NAMES.filter(x => !lower.includes(x.toLowerCase()));

// Other header names must be accepted.
FORBIDDEN_HEADER_NAMES
non_overlapping_headers
.concat(SIMPLE_HEADER_NAMES)
.concat([CONTENT_TYPE])
.concat(NON_SIMPLE_HEADER_NAMES)
Expand All @@ -343,8 +364,15 @@ test(function() {
header + ' must be ignored (2)');
});

// Take into account overlapping forbidden header names.
var lower = FORBIDDEN_RESPONSE_HEADER_NAMES.map(header => {
return header.toLowerCase();
});
var non_overlapping_headers =
FORBIDDEN_HEADER_NAMES.filter(x => !lower.includes(x.toLowerCase()));

// Other header names must be accepted.
FORBIDDEN_HEADER_NAMES
non_overlapping_headers
.concat(SIMPLE_HEADER_NAMES)
.concat([CONTENT_TYPE])
.concat(NON_SIMPLE_HEADER_NAMES)
Expand Down
2 changes: 1 addition & 1 deletion blink/web_tests/http/tests/navigation/ping-cookie.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
try {
var xhr = new XMLHttpRequest();
xhr.open("GET", "../cookies/resources/setCookies.cgi", false);
xhr.setRequestHeader("SET-COOKIE", "hello=world;path=/");
xhr.setRequestHeader("X-SET-COOKIE", "hello=world;path=/");
xhr.send(null);
if (xhr.status != 200) {
document.getElementsByTagName("body")[0].appendChild(document.createTextNode("FAILED: cookie not set"));
Expand Down

0 comments on commit 6a62f0c

Please sign in to comment.