Skip to content

Commit

Permalink
Add DevTools backend support for cookie same-site redirect issue
Browse files Browse the repository at this point in the history
Implements DevTools backend support to show an issue when cookies
are blocked due to cross-site redirect chain context downgrades.

This issue will only be shown when kCookieSameSiteConsidersRedirectChain
is enabled.

Bug: 1221316
Change-Id: I0a646953b2d4e82e8f98cf7f73ff17b7b6f81363
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4936302
Reviewed-by: Alex Rudenko <alexrudenko@chromium.org>
Commit-Queue: Steven Bingler <bingler@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1211697}
  • Loading branch information
sbingler authored and Chromium LUCI CQ committed Oct 18, 2023
1 parent 88f7523 commit 508be75
Show file tree
Hide file tree
Showing 8 changed files with 117 additions and 0 deletions.
14 changes: 14 additions & 0 deletions content/browser/devtools/devtools_instrumentation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "content/browser/devtools/devtools_instrumentation.h"

#include "base/containers/adapters.h"
#include "base/feature_list.h"
#include "base/strings/stringprintf.h"
#include "base/trace_event/traced_value.h"
#include "components/download/public/common/download_create_info.h"
Expand Down Expand Up @@ -48,6 +49,7 @@
#include "devtools_instrumentation.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "net/base/features.h"
#include "net/base/load_flags.h"
#include "net/cookies/canonical_cookie.h"
#include "net/cookies/cookie_inclusion_status.h"
Expand Down Expand Up @@ -1713,6 +1715,18 @@ std::unique_ptr<protocol::Array<protocol::String>> BuildWarningReasons(
protocol::Audits::CookieWarningReasonEnum::WarnThirdPartyPhaseout);
}

// This warning only affects cookies when the corresponding feature is
// enabled, therefore we should only create an issue for it then.
if (base::FeatureList::IsEnabled(
net::features::kCookieSameSiteConsidersRedirectChain) &&
status.HasWarningReason(
net::CookieInclusionStatus::
WARN_CROSS_SITE_REDIRECT_DOWNGRADE_CHANGES_INCLUSION)) {
warning_reasons->push_back(
protocol::Audits::CookieWarningReasonEnum::
WarnCrossSiteRedirectDowngradeChangesInclusion);
}

return warning_reasons;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ experimental domain Audits
WarnAttributeValueExceedsMaxSize
WarnDomainNonASCII
WarnThirdPartyPhaseout
WarnCrossSiteRedirectDowngradeChangesInclusion

type CookieOperation extends string
enum
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ enum CookieWarningReason {
kWarnAttributeValueExceedsMaxSize,
kWarnDomainNonASCII,
kWarnThirdPartyPhaseout,
kWarnCrossSiteRedirectDowngradeChangesInclusion,
};

// Specific information about |kCookieIssue| type issues.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ protocol::String BuildCookieWarningReason(
return protocol::Audits::CookieWarningReasonEnum::WarnDomainNonASCII;
case blink::mojom::blink::CookieWarningReason::kWarnThirdPartyPhaseout:
return protocol::Audits::CookieWarningReasonEnum::WarnThirdPartyPhaseout;
case blink::mojom::blink::CookieWarningReason::
kWarnCrossSiteRedirectDowngradeChangesInclusion:
return protocol::Audits::CookieWarningReasonEnum::
WarnCrossSiteRedirectDowngradeChangesInclusion;
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Verifies that after a cross-site redirect SameSite cookies file an Issue

{
issue : {
code : CookieIssue
details : {
cookieIssueDetails : {
cookie : {
domain : firstparty.test
name : fooStrict
path : /
}
cookieExclusionReasons : [
[0] : ExcludeSameSiteStrict
]
cookieUrl : https://firstparty.test:8443/inspector-protocol/resources/redirect-chain.html?end
cookieWarningReasons : [
[0] : WarnCrossSiteRedirectDowngradeChangesInclusion
]
operation : ReadCookie
request : {
requestId : <string>
url : https://firstparty.test:8443/inspector-protocol/resources/redirect-chain.html?end
}
siteForCookies : https://firstparty.test/
}
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
(async testRunner => {
// This test requires kCookieSameSiteConsidersRedirectChain to be enabled in order to pass.
const {page, session, dp} = await testRunner.startBlank(
`Verifies that after a cross-site redirect SameSite cookies file an Issue\n`);

await dp.Network.enable();
await dp.Audits.enable();

// Set the cookie.
const response = await dp.Network.setCookie({
url: 'https://firstparty.test:8443',
secure: true,
name: 'fooStrict',
value: 'bar',
sameSite: 'Strict',
});

if (response.error)
testRunner.log(`setCookie failed: ${response.error.message}`);

// Push events to arrays to prevent async races from causing flakes.
const requestWillBeSentExtraInfos = [];
let issueAdded;

const expectedRequests =
new Promise(resolve => dp.Network.onRequestWillBeSentExtraInfo(event => {
requestWillBeSentExtraInfos.push(event.params);
// There will be the first navigation -> redirect -> final navigation == 3
if (requestWillBeSentExtraInfos.length === 3) {
resolve();
}
}));

const expectedIssue = dp.Audits.onceIssueAdded(event => {
// Safely ignore irrelevant issue...
return event.params.issue.code !== 'QuirksModeIssue';
});

page.navigate(
'https://firstparty.test:8443/inspector-protocol/resources/redirect-chain.html?start');

await expectedRequests;

issueAdded = await expectedIssue;
testRunner.log(issueAdded.params);

testRunner.completeTest();
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
// Redirects back to the first party site.
header('HTTP/1.1 307 Temporary Redirect');
header('Expires: Thu, 01 Dec 2003 16:00:00 GMT');
header('Cache-Control: no-cache, must-revalidate');
header('Pragma: no-cache');
header('Location: https://firstparty.test:8443/inspector-protocol/resources/redirect-chain.html?end');
?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<body>
<script>
const urlParams = new URLSearchParams(window.location.search);
if(urlParams.has("start")) {
window.location = "https://example.test:8443/inspector-protocol/resources/cross-site-redirect.php"
}
</script>
</body>
</html>

0 comments on commit 508be75

Please sign in to comment.