Moving Sec-Fetch-Site implementation to the NetworkService. [chromium/src : master]
Łukasz Anforowicz (Gerrit)
Thanks for the early feedback Mike!
4 comments:
File services/network/initiator_lock_compatibility.cc:
Patch Set #1, Line 30:
//DCHECK(!base::FeatureList::IsEnabled(network::features::kNetworkService));I think we're currently in a weird state for the initiator of a user-typed URL in the address bar (or, likewise, a navigation triggered outside of Chrome)
For this feature, that question is covered in https://bugs.chromium.org/p/chromium/issues/detail?id=868286; perhaps we need a new `Site` value for directly initiated navigations?
Thanks for the heads-up. I've added a comment to the bug. I think the current CL doesn't change anything (i.e. continues to report 'cross-site' for browser-initiated navigations).
File services/network/sec_fetch_site.cc:
Patch Set #1, Line 31:
// registrable domain.I'm not sure that was intentional, actually. The check was meant to exclude `appspot. […]
Site Isolation treats different IP addresses as cross-site - see SiteInstanceImpl::GetSiteForOrigin. OTOH, there is no explicit IP address check there - only an indirect check that the domain is empty, which will be true for IP addresses, but also for other things (like intranet hosts):
- I've opened https://github.com/mikewest/sec-metadata/issues/14 (intranet hosts)
- I've also opened https://github.com/mikewest/sec-metadata/issues/15 (trailing dot)
File services/network/url_loader.cc:
Patch Set #1, Line 661:
factory_params_->request_initiator_site_lock);Does this implementation depend on the network service being enabled? It looks like it probably does...
Correct. I think a SecFetchSiteResourceHandler could be potentially introduced for making the pre-NetworkService things also work. I haven't looked into that yet.
What's the timeline for that? Last I heard it was being rolled back out due to some issues?
AFAIK NetworkService is enabled on 100% of the stable channel for M72 for Windows, Mac and Linux. I don't know what is the status for Android and ChromeOS (maybe M73?), but I hope that it will ship everywhere in M74.
File third_party/blink/renderer/core/loader/base_fetch_context.cc:
Can you add a comment about `Sec-Fetch-Site` being added outside of Blink for clarity?
Will do (in the next patchset [soon? :-/] ).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Mike, could you please share your thoughts on how you want to ship/launch Sec-Fetch-Site?
PS. Sorry for the radio silence - I've been working on moving content::IsOriginSecure to //services/network (see the chain of CLs mentioned in the comment in https://crrev.com/c/1508920/1#message-dc774774cb9265db4e89b042bb68fb157ad08c1d). So, I think that right now figuring out the base::Feature / switches stuff is the only blocked for making Sec-Fetch-Site 1) working for redirects and 2) based on trustworthy data (e.g. vetting request_initiator against request_initiator_site_lock).
1 comment:
File content/browser/frame_host/navigation_request.cc:
bool IsSecMetadataEnabled() {
return base::FeatureList::IsEnabled(features::kSecMetadata) ||
base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableExperimentalWebPlatformFeatures);
}Errrr... so how would I replicate this kind of check in the NetworkService? Would I move
1) features::kSecMetadata -> //services/network/public/cpp/features.h
2) switches::kEnableExperimentalWebPlatformFeatures -> services/network/public/cpp/network_switches.h
?Or maybe just move the feature (and if only the switch is not provided, then Sec-Fetch-Site won't work, but other Sec-Metadata headers will work)?
Or maybe I should just wait until all of Sec-Metadata is enabled by default, so I don't have to worry about it?
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Mike?
1 comment:
bool IsSecMetadataEnabled() {
return base::FeatureList::IsEnabled(features::kSecMetadata) ||
base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableExperimentalWebPlatformFeatures);
}
Errrr... so how would I replicate this kind of check in the NetworkService? Would I move […]
I've wrote a slightly longer summary of the problems + proposed a specific way forward in https://bugs.chromium.org/p/chromium/issues/detail?id=872285#c9.
WDYT?
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch Set 2:
(1 comment)
Mike, could you please share your thoughts on how you want to ship/launch Sec-Fetch-Site?
PS. Sorry for the radio silence - I've been working on moving content::IsOriginSecure to //services/network (see the chain of CLs mentioned in the comment in https://crrev.com/c/1508920/1#message-dc774774cb9265db4e89b042bb68fb157ad08c1d). So, I think that right now figuring out the base::Feature / switches stuff is the only blocked for making Sec-Fetch-Site 1) working for redirects and 2) based on trustworthy data (e.g. vetting request_initiator against request_initiator_site_lock).
Sorry, I was OOO last week and didn't set a reasonable away message. :(
I answered you on the bug, but for completeness: let's just I2S and send the header unconditionally. I think resolving https://github.com/mikewest/sec-metadata/issues/16 is a blocker to I2S, so I'll work on getting that cleared up one way or the other.
Łukasz Anforowicz (Gerrit)
Mike, this CL might be needed for another look please?
2 comments:
File services/network/public/cpp/features.cc:
Patch Set #8, Line 64:
ENABLED_BY_DEFAULT};Woo-hoo?
This means that this CL shouldn't land before an intent-to-ship gets 3 lgtms, right?
This is needed, because otherwise all WPT tests for sec-metadata would fail...
File services/network/sec_fetch_site.cc:
Patch Set #8, Line 121:
IsUrlPotentiallyTrustworthy(target_url)Note that this is slightly different than Blink's notion of potentially trustworthy origins (i.e. WebSecurityPolicy::AddOriginTrustworthyWhiteList). Because this doesn't know about blink-only whitelists, it means that some layout test expectations somewhat unexpectedly need to change - for example, http/tests/serviceworker/fetch-event-headers.html (which right now doesn't include the secure origin, because we never get past the trustworthiness check here).
Should be okay, right? (the trustworthiness is spec-compliant - see //services/network/public/cpp/is_potentially_trustworthy.cpp - just doesn't take into account exceptions within Blink)
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
3 comments:
Hmmm... I haven't investigated why the difference in test expectations is needed here. Maybe since the test was failing before my CL, it doesn't matter much? But I am not sure if this difference can be explained by trustworthiness, because the test page here should be https / trustworthy...
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
This test expectations difference is caused by the difference in Blink vs //services/network notions of trustworthiness (different allowlists) and is expected / okay.
HTTP_SEC_FETCH_DEST = nested-document
HTTP_SEC_FETCH_MODE = nested-navigate
HTTP_SEC_FETCH_SITE = cross-siteI assume that this difference comes from flipping the feature to enabled-by-default state. I don't think this CL is touching things like Sec-Fetch-Dest in any other way (other than flipping the feature's default state).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Thanks! I left some comments.
6 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #8, Line 243:
headers->SetHeaderIfMissing("Sec-Fetch-Dest", destination.c_str());I think we need to make this a little more complicated, unfortunately. We're aiming to ship the `Sec-Fetch-Site`, `Sec-Fetch-User`, and `Sec-Fetch-Mode` headers, but we're not yet shipping `Sec-Fetch-Dest`. https://chromium-review.googlesource.com/c/chromium/src/+/1547515 would address this by keeping the flag disabled, but only gating the one header on it.
It probably makes more sense to add another flag rather than reusing the existing flag (just in case things explode and we need to roll back?). I've uploaded https://chromium-review.googlesource.com/c/chromium/src/+/1554662 to do so.
File services/network/sec_fetch_site.cc:
Patch Set #8, Line 40:
return GetDomain(initiator) == GetDomain(target_origin);I think this fails if domains can't be obtained for the name? Like `http://go/` and `http://teams/`? Those both result in empty strings, which are `==`. Perhaps add change the IP address check above to something like:
```
// `GetDomain` returns an empty string for origins which either themselves
// represent a TLD, are IP addresses, or have empty hosts.
if (GetDomain(initiator).empty()) {
return false;
}
```The equality check below will then verify that we do the equivalent of the same check for `target_origin`.
Patch Set #8, Line 121:
IsUrlPotentiallyTrustworthy(target_url)
Note that this is slightly different than Blink's notion of potentially trustworthy origins (i.e. […]
Hrm. This seems strange, because I thought we ran //http/tests files from `http://127.0.0.1:8001/`, which should be considered trustworthy as it's the loopback address.
Hmmm... I haven't investigated why the difference in test expectations is needed here. […]
I think this is the same explanation as `fetch-event-headers.html`. This patch moves the header attachment to post-ServiceWorker (e.g. https://fetch.spec.whatwg.org/#http-network-or-cache-fetch rather than https://fetch.spec.whatwg.org/#http-fetch).
That might be fine? But if it is, we ought to move the rest of the headers there too (doesn't need to be in this CL, of course). Attaching some headers early and some late is strange.
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
This test expectations difference is caused by the difference in Blink vs //services/network notions […]
Have you looked into that explanation? It might instead be that we're now attaching the `Sec-Fetch-Site` header _after_ passing through the service worker rather than before.
HTTP_SEC_FETCH_DEST = nested-document
HTTP_SEC_FETCH_MODE = nested-navigate
HTTP_SEC_FETCH_SITE = cross-site
I assume that this difference comes from flipping the feature to enabled-by-default state. […]
Yes. These are the //virtual/stable tests that show the behavior we're shipping. Flipping the flag on by default is intended to change these results.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Mike! Can you take another look please?
7 comments:
Patch Set #8, Line 243:
headers->SetHeaderIfMissing("Sec-Fetch-Dest", destination.c_str());
I think we need to make this a little more complicated, unfortunately. We're aiming to ship the `Sec-Fetch-Site`, `Sec-Fetch-User`, and `Sec-Fetch-Mode` headers, but we're not yet shipping `Sec-Fetch-Dest`. https://chromium-review.googlesource.com/c/chromium/src/+/1547515 would address this by keeping the flag disabled, but only gating the one header on it.
Good catch - thanks!
It probably makes more sense to add another flag rather than reusing the existing flag (just in case things explode and we need to roll back?). I've uploaded https://chromium-review.googlesource.com/c/chromium/src/+/1554662 to do so.
Makes sense. Thanks for putting the other CL together.
File services/network/initiator_lock_compatibility.cc:
Patch Set #9, Line 29:
SimilarilyDone. Added |iab similarily similarly| to .vimrc ...
File services/network/sec_fetch_site.cc:
Patch Set #8, Line 40:
return GetDomain(initiator) == GetDomain(target_origin);
I think this fails if domains can't be obtained for the name? Like `http://go/` and `http://teams/`? […]
Good catch - thanks! I forgot about our discussion at https://github.com/mikewest/sec-metadata/issues/14. I've added unit tests for this.
Patch Set #8, Line 121:
IsUrlPotentiallyTrustworthy(target_url)
Hrm. This seems strange, because I thought we ran //http/tests files from `http://127.0.0. […]
Hmmm... you're right. |!IsUrlPotentiallyTrustworthy| never happens in SecFetchSite::SetHeader when running the http/tests/serviceworker/fetch-event-headers.html test.
I think this is the same explanation as `fetch-event-headers.html`. […]
Oh, ok. That seems like a feasible explanation. I've opened https://crbug.com/949997 to track this, but I have no idea what the bug means in terms of the implementation... :-/
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
Have you looked into that explanation? It might instead be that we're now attaching the `Sec-Fetch-S […]
You're right. |!IsUrlPotentiallyTrustworthy| never happens in SecFetchSite::SetHeader when running the http/tests/serviceworker/fetch-event-headers.html test.
HTTP_SEC_FETCH_DEST = nested-document
HTTP_SEC_FETCH_MODE = nested-navigate
HTTP_SEC_FETCH_SITE = cross-site
Yes. These are the //virtual/stable tests that show the behavior we're shipping. […]
FWIW, I verified that indeed the test behaves differently after changing the feature's default state.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz uploaded patch set #11 to this change.
Moving Sec-Fetch-Site implementation to the NetworkService.
The move helps in two ways:
- It allows modifying request headers when processing redirects.
- It makes it possible to base the header on trustworthy input
(i.e. allow for verifying of request initiator supplied
by a renderer process against request initiator lock
supplied by the browser process).
This CL enables the network::features::kFetchMetadata feature, to keep
the current behavior covered by the wpt/fetch/sec-metadata tests.
Without enabling the flag, the behavior would changes (since
//services/network doesn't know about
switches::kEnableExperimentalWebPlatformFeatures. Please note that
the intent to ship Sec-Fetch-Site has been approved here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/yQgJlq5PEOQ/discussion
Bug: 872285, 924204
Change-Id: I323239cd76e18037be8eb56aad4e989932585a05
---
M content/browser/BUILD.gn
M content/browser/frame_host/navigation_request.cc
M content/browser/loader/resource_dispatcher_host_impl.cc
A content/browser/loader/sec_fetch_site_resource_handler.cc
A content/browser/loader/sec_fetch_site_resource_handler.h
M content/browser/worker_host/worker_script_fetch_initiator.cc
M services/network/BUILD.gn
M services/network/OWNERS
M services/network/initiator_lock_compatibility.cc
M services/network/public/cpp/features.cc
A services/network/sec_fetch_site.cc
A services/network/sec_fetch_site.h
A services/network/sec_fetch_site_unittest.cc
M services/network/url_loader.cc
M third_party/blink/renderer/core/loader/base_fetch_context.cc
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-cross-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-same-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-origin-redirect.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-site-redirect.tentative.https.sub-expected.txt
M third_party/blink/web_tests/external/wpt/service-workers/service-worker/fetch-request-xhr.https-expected.txt
M third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/form-targets-cross-site-frame-get-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/form-targets-cross-site-frame-no-referrer-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/form-targets-cross-site-frame-post-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/form-with-enctype-targets-cross-site-frame-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/post-basic-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/post-frames-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/post-frames-goback1-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/post-goback1-expected.txt
M third_party/blink/web_tests/virtual/stable/http/tests/navigation/rename-subframe-goback-expected.txt
30 files changed, 443 insertions(+), 77 deletions(-)
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz would like Yutaka Hirano to review this change.
A services/network/sec_fetch_site.cc
A services/network/sec_fetch_site.h
A services/network/sec_fetch_site_unittest.cc
M services/network/url_loader.cc
M third_party/blink/renderer/core/loader/base_fetch_context.cc
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-cross-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-same-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-origin-redirect.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-site-redirect.tentative.https.sub-expected.txt
M third_party/blink/web_tests/external/wpt/service-workers/service-worker/fetch-request-xhr.https-expected.txt
M third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html
20 files changed, 418 insertions(+), 80 deletions(-)
Łukasz Anforowicz (Gerrit)
Yutaka, could you PTAL?
As with https://crrev.com/c/1574584, Mike (in CC) is the primary owner of Sec-Fetch-..., but he can't finish reviewing this CL for a couple of weeks, so I thought that in the meantime I would ask you (as one of //content/browser/loader/OWNERS and //services/network/OWNERS) for a review. FWIW, I don't plan to land this CL until Mike comes back and approves this CL.
4 comments:
File services/network/initiator_lock_compatibility.cc:
Hmmm... the 2 changes in this source file are not directly related to the CL. I may move them to a separate CL. WDYT?
After this CL we can delete failure expectations for wpt/fetch/sec-metadata/redirect/... - yay!
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Yutaka Hirano (Gerrit)
4 comments:
File services/network/sec_fetch_site.h:
Patch Set #13, Line 27:
class COMPONENT_EXPORT(NETWORK_SERVICE) SecFetchSite {Do you want to add more functions in the future? Currently this class has essentially one function, and in that case I prefer having a bare function (network::SetSecFetchSiteHeader?).
Patch Set #13, Line 36:
// Note that |pending_redirect_url| is optional - it should be set only whenCORSURLLoader breaks the redirect chain - Is it OK?
File services/network/sec_fetch_site.cc:
Patch Set #13, Line 28:
bool IsSameSite(const url::Origin& initiator,Is this a useful function in url/? This looks somewhat generic to me.
Patch Set #13, Line 57:
if (target_origin == initiator)https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#code-formatting
"Don't use else after return".
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Yutaka! Can you please take a look at my replies below?
4 comments:
Patch Set #13, Line 27:
class COMPONENT_EXPORT(NETWORK_SERVICE) SecFetchSite {
Do you want to add more functions in the future? Currently this class has essentially one function, […]
There probably won't be more functions here (other Sec-Fetch-... headers can probably be taken care of in separate source files and they seem unlikely to be moved into //services/network). But there is also IsSameSiteForTesting so I'd prefer to keep both of them inside the class?
Patch Set #13, Line 36:
// Note that |pending_redirect_url| is optional - it should be set only whenCORSURLLoader breaks the redirect chain - Is it OK?
I haven't had time to look into that in detail, but let me try to share some notes and questions:
- Value of Sec-Fetch-Site depends on the whole/full redirect chain.
- Are you saying that (with OOR-CORS) URLLoader::OnReceivedRedirect might not see the full URL chain? Or that SecFetchSite::SetHeader should be called from somewhere else?
- I guess you are also implying that wpt/fetch/sec-metadata should be expanded to make sure it covers the intersection of CORS/redirects/Sec-Fetch-Site (because you are worried that this case is not handled correctly), right?
File services/network/sec_fetch_site.cc:
Patch Set #13, Line 28:
bool IsSameSite(const url::Origin& initiator,Is this a useful function in url/? This looks somewhat generic to me.
It is possible that this function may be moved into //url. I think it might be best to do it in a separate CL.
OTOH, I am not 100% sure about the move to //url, because there are various functions with slightly different meaning:
- IsSameSite here - this seems purest
- CrossOriginResourcePolicy::ShouldAllowSameSiteForTesting (treats http->https requests as same-site)
- SiteInstanceImpl::IsSameWebSite (treats file: URLs as same-site; Sec-Fetch-Site and CORP are kind of undefined for non-http/https URLs)
Patch Set #13, Line 57:
if (target_origin == initiator)
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Yutaka Hirano (Gerrit)
1 comment:
File services/network/sec_fetch_site.h:
Patch Set #13, Line 36:
// Note that |pending_redirect_url| is optional - it should be set only whenI haven't had time to look into that in detail, but let me try to share some notes and questions: […]
CORSURLLoader may cancel the request when it sees a cross-origin redirect, and create a new URLLoader. Please see the bottom of CorsURLLoader::FollowRedirect.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Yutaka! Can you please see my replies below?
2 comments:
Patch Set #13, Line 36:
// Note that |pending_redirect_url| is optional - it should be set only when
CORSURLLoader may cancel the request when it sees a cross-origin redirect, and create a new URLLoade […]
Got it - thanks! Thinking about it more, I think we can treat this as a separate problem:
1. Before the current CL, Sec-Fetch-Site is broken (is set for all requests based on the initial URL) for *all* redirects. So fixing all no-cors redirects (all current wpt/fetch/sec-metadata redirect tests) seems like desirable progress (i.e. supports landing the current CL, despite potential CORS-vs-CrossOriginRedirects-vs-SecFetchSite issue.
2. I've opened https://crbug.com/956146 to track and discuss it further (in particular, it is not clear to me whether cross-origin redirects are supported by CORS or not).
WDYT? Does the above sound like a good plan?
File services/network/sec_fetch_site.cc:
Patch Set #13, Line 28:
bool IsSameSite(const url::Origin& initiator,
It is possible that this function may be moved into //url. […]
FWIW, I've also talked with Daniel (added to CC, one of //url/OWNERS) and his initial reaction was that maybe we shouldn't move this function to //url just yet, because there are currently (i.e. after the CL under review) no other users outside of //services/network.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
LGTM (but I own practically none of this code, and it's essential for someone who comprehends the network service to give you feedback on those bits and pieces of integration). Thanks for doing another round or two. :)
Just a few small nits.
Patch set 15:Code-Review +1
3 comments:
Patch Set #15, Line 16:
This CL enables the network::features::kFetchMetadata feature, to keepI think this bit isn't true anymore.
File third_party/blink/renderer/core/loader/base_fetch_context.cc:
Patch Set #15, Line 169:
// Sec-Fetch-Site is covered by network::SecFetchSite class.Can you reword these comments while you're here? Perhaps:
"""
Note that the `Sec-Fetch-User` header is always false (and therefore omitted) for subresource requests. Likewise, note that we rely on Blink's embedder to set `Sec-Fetch-Site`, as we don't want to trust the renderer to assert its own origin.
"""
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
Patch Set #15, Line 34:
["accept", "sec-fetch-dest", "sec-fetch-mode", "upgrade-insecure-requests", "user-agent"],Can you tie this CL to the bug you filed regarding the positioning of these headers' injection vis a vis service worker?
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Mike!
3 comments:
Patch Set #15, Line 16:
This CL enables the network::features::kFetchMetadata feature, to keepI think this bit isn't true anymore.
Ooops, good catch. Done. (not the first time I tweak the code and forget to modify the CL description... :-/).
File third_party/blink/renderer/core/loader/base_fetch_context.cc:
Patch Set #15, Line 169:
// Sec-Fetch-Site is covered by network::SecFetchSite class.
Can you reword these comments while you're here? Perhaps: […]
Done (note that this code was moved by r654375 to third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
File third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html:
Patch Set #15, Line 34:
["accept", "sec-fetch-dest", "sec-fetch-mode", "upgrade-insecure-requests", "user-agent"],
Can you tie this CL to the bug you filed regarding the positioning of these headers' injection vis a […]
Done (https://crbug.com/949997).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz has assigned a change to Yutaka Hirano.
Moving Sec-Fetch-Site implementation to the NetworkService.
The move helps in two ways:
- It allows modifying request headers when processing redirects.
- It makes it possible to base the header on trustworthy input
(i.e. allow for verifying of request initiator supplied
by a renderer process against request initiator lock
supplied by the browser process).
Bug: 872285, 924204, 949997
Change-Id: I323239cd76e18037be8eb56aad4e989932585a05
---
M content/browser/BUILD.gn
M content/browser/frame_host/navigation_request.cc
M content/browser/loader/resource_dispatcher_host_impl.cc
A content/browser/loader/sec_fetch_site_resource_handler.cc
A content/browser/loader/sec_fetch_site_resource_handler.h
M content/browser/worker_host/worker_script_fetch_initiator.cc
M services/network/BUILD.gn
M services/network/OWNERS
M services/network/initiator_lock_compatibility.cc
A services/network/sec_fetch_site.cc
A services/network/sec_fetch_site.h
A services/network/sec_fetch_site_unittest.cc
M services/network/url_loader.cc
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-cross-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-same-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-origin-redirect.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-site-redirect.tentative.https.sub-expected.txt
M third_party/blink/web_tests/external/wpt/service-workers/service-worker/fetch-request-xhr.https-expected.txt
M third_party/blink/web_tests/http/tests/serviceworker/fetch-event-headers.html
20 files changed, 423 insertions(+), 81 deletions(-)
Łukasz Anforowicz (Gerrit)
Yutaka, I've just found net::registry_controlled_domains::SameDomainOrHost which lets me address 2 additional pieces of your feedback. Please take another look when you have a chance.
2 comments:
File services/network/sec_fetch_site.h:
Patch Set #13, Line 27:
class COMPONENT_EXPORT(NETWORK_SERVICE) SecFetchSite {There probably won't be more functions here (other Sec-Fetch-... […]
I guess my argument above doesn't apply anymore after I start using net::registry_controlled_domains::SameDomainOrHost instead of rolling out my own IsSameSite.
Done.
File services/network/sec_fetch_site.cc:
Patch Set #13, Line 28:
bool IsSameSite(const url::Origin& initiator,
FWIW, I've also talked with Daniel (added to CC, one of //url/OWNERS) and his initial reaction was t […]
Actually, I've just discovered that we already have net::registry_controlled_domains::SameDomainOrHost - let me use that instead (augmenting its unit tests a little bit).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Yutaka Hirano (Gerrit)
Looking good, but I'm a bit worried about the service worker interaction. Do you have any plan to recover consistency across sec-fetch-* headers?
1 comment:
You're right. […]
So this is still unresolved, right? https://github.com/whatwg/fetch/issues/885
I'm not sure whether service workers should see the values or not, but it should see all or none of them, not some.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Yutaka!
Looking good, but I'm a bit worried about the service worker interaction. Do you have any plan to recover consistency across sec-fetch-* headers?
Let's follow-up in https://crbug.com/949997 and see if we can figure out a good way forward.
1 comment:
So this is still unresolved, right? https://github.com/whatwg/fetch/issues/885
Correct - this is not resolved.
I'm not sure whether service workers should see the values or not, but it should see all or none of them, not some.
I've left a comment in https://crbug.com/949997 to see 1) if we can figure out what the desired long-term state should be (both from spec/requirements perspective and implementation perspective) and 2) if we can agree on whether the consistency is a must-have for M76 (and block making Sec-Fetch-Site trustworthy even if the renderer is compromised and can lie in its IPCs - this is a prerequisite for announcing that Site Isolation offers robust protections against compromised renderers).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
1 comment:
File services/network/sec_fetch_site.cc:
Patch Set #20, Line 60:
!request.initiator().has_value()) {Do you have any ideas about how to handle NTP navigations in the future? (`initiator` will be something like `chrome-search://most-visited`).
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Thanks Mike!
1 comment:
Patch Set #20, Line 60:
!request.initiator().has_value()) {
Do you have any ideas about how to handle NTP navigations in the future? (`initiator` will be someth […]
This seems to me like something that is definitely fixable and is mostly a matter of prioritization and figuring out the right fix. I've left some comments in https://crbug.com/946489 - let me resolve this CR feedback thread and we can continue the discussion in https://crbug.com/946489 (especially since I believe we would treat NTP as cross-site even before this CL, right?).
At any rate - I would be happy to help with any of the Sec-Fetch-Site bugs (they compete for priority mainly with OOR-CORS-vs-allowlist-for-contentscripts) if you think they are important for adoption of Sec-Fetch-Site as a security mechanism. These 2 seem especially important because of the frequency at which they happen:
- https://crbug.com/946489: `Sec-Fetch-Site` incorrect when clicking a link from the NTP. (I bumped its priority)
- https://crbug.com/946503: `Sec-Fetch-Site` set incorrectly during history traversal.
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Yutaka, can you please take another look? Do you have any other concerns beside https://crbug.com/949997 (which I hope shouldn't block this CL - see https://crbug.com/949997#c3)?
1 comment:
> So this is still unresolved, right? https://github.com/whatwg/fetch/issues/885 […]
Does the course of action outlined by Mike in https://crbug.com/949997#c3 look okay?
To view, visit change 1478304. To unsubscribe, or for help writing mail filters, visit settings.
Yutaka Hirano (Gerrit)
LGTM.
Can you strip sec-fetch-* headers in ServiceWorkerSubresourceLoader and around until the standard discussion settles?
Patch set 20:Code-Review +1
Łukasz Anforowicz (Gerrit)
Thanks Yutaka!
Can you strip sec-fetch-* headers in ServiceWorkerSubresourceLoader and around until the standard discussion settles?
I am trying to do that in patchset #22, using IsExcludedHeaderForServiceWorkerFetchEvent as suggested by Matt in https://crbug.com/949997#c4. I'll also loop him in as a reviewer here.
Łukasz Anforowicz (Gerrit)
Matt, could you PTAL?
Let me try to respond below to some of your comments in https://crbug.com/949997#c4:
Could it work to hide the headers from SW by excluding it in the same way as IsExcludedHeaderForServiceWorkerFetchEvent()?
Thank you very much for the suggestion - this seems like exactly the right place for excluding the headers from the `fetch` event. I've tried making this change in https://chromium-review.googlesource.com/c/chromium/src/+/1478304/21..22 - could you please double-check if it matches what you had in mind?
I would like to be deliberate about this though. We have not implemented https://fetch.spec.whatwg.org/#http-header-layer-division so there may not be a easy path toward fixing it in 77ish after shipping it in 76.
Are the changes in patchset #22 sufficient to land this CL in 76? Are these changes sufficient for shipping Sec-Fetch-... in general in 76? (note that without the current CL, all Sec-Fetch-... headers are visible to the service workers - which AFAIU is the problem you'd like to avoid / is the problem that may block changing the behavior in 77)
It seems most of the issues have been about unexpected CORS preflights getting triggered and rejected by servers that don't expect them. I'm not sure whether these headers would have the same problem.
Note that the current CL moves where Sec-Fetch-Site header is injected - after this CL, that header will be injected *below* OOR-CORS layer, which I think means that after this CL the Sec-Fetch-Site header should not trigger CORS-preflight requests.
Also note that other headers (e.g. Sec-Fetch-Mode and Sec-Fetch-User) are injected by Blink and therefore are visible to OOR-CORS (and might potentially trigger a CORS-preflight request?). OTOH, I don't think this causes an issue in practice - see my reply in https://crbug.com/949997#c5
Matt Falkenhagen (Gerrit)
Thanks for the explanations. It looks good to me to exclude the headers from the SW fetch event. Have you confirmed that the headers get re-added before going to network for the following cases:
1) The SW does respondWith(fetch(event.request)
2) The SW does network fallback (does not call respondWith)
I think the desired behavior is for the header to be re-added. If they aren't re-added I think it'd be up to you whether it's blocking and if we need to think of another solution.
Longer-term, SW team should implement the header layering so we don't need to use this exclusion trick.
Patch set 22:Code-Review +1
Łukasz Anforowicz (Gerrit)
Thanks Matt! It occurred to me that I probably should extract SW-focused changes into a separate CL - I've pulled them into https://crrev.com/c/1611314.
Thanks for the explanations. It looks good to me to exclude the headers from the SW fetch event.
Ack. Let me change the title of https://crbug.com/949997 to reflect this direction.
Have you confirmed that the headers get re-added before going to network for the following cases:
1) The SW does respondWith(fetch(event.request)
2) The SW does network fallback (does not call respondWith)
Not really. I think I need to work on adding new test cases to wpt/fetch/sec-metadata that have a service worker that exercises the behavior you pointed out. Let me try to work on that in the separate CL.
I think the desired behavior is for the header to be re-added. If they aren't re-added I think it'd be up to you whether it's blocking and if we need to think of another solution.
Agreed.
Longer-term, SW team should implement the header layering so we don't need to use this exclusion trick.
Ack.
Łukasz Anforowicz (Gerrit)
+Charlie for //content/browser/frame_host/navigation_request.cc
+Ryan for //net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc (no change in product behavior, just documenting existing behavior via additional unit tests)
Ryan Sleevi (Gerrit)
//net/base/registry_controlled_domains LGTM
Patch set 24:Code-Review +1
Charlie Reis (Gerrit)
Patch Set 23:
+Charlie for //content/browser/frame_host/navigation_request.cc
navigation_request.cc LGTM.
Patch set 25:Code-Review +1
Łukasz Anforowicz (Gerrit)
Thanks for the reviews everyone! I'll push to CQ in a minute.
During last-minute self-review I've also realized that after replacing network::SecFetchSite class with a single network::SetSecFetchSiteHeader function (in patchset #20), I forgot to do the same change in comments - I am fixing this in the latest patchset.
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"s/network::SecFetchSite class/network::SetSecFetchSiteHeader function/ in comments" https://chromium-review.googlesource.com/c/1478304/26
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1478304/26
Bot data: {"action": "start", "triggered_at": "2019-05-15T17:40:09.0Z", "revision": "2fb3d6dbd9a1e1ca2acdd0c38c00b343a5d050bf"}
Łukasz Anforowicz (Gerrit)
Patch set 26:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Moving Sec-Fetch-Site implementation to the NetworkService.
The move helps in two ways:
- It allows modifying request headers when processing redirects.
This fixes existing wpt/fetch/sec-metadata/redirect tests
(although note that redirects might still be mishandled
in CORS mode - see https://crbug.com/956146).
- It makes it possible to base the header on trustworthy input
(i.e. allow for verifying of |request_initiator| supplied
by a renderer process against |request_initiator_site_lock|
supplied by the browser process).
Bug: 872285, 924204
Change-Id: I323239cd76e18037be8eb56aad4e989932585a05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1478304
Commit-Queue: Łukasz Anforowicz <luk...@chromium.org>
Reviewed-by: Charlie Reis <cr...@chromium.org>
Reviewed-by: Ryan Sleevi <rsl...@chromium.org>
Reviewed-by: Matt Falkenhagen <fal...@chromium.org>
Reviewed-by: Yutaka Hirano <yhi...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660044}
---
M content/browser/BUILD.gn
M content/browser/frame_host/navigation_request.cc
M content/browser/loader/resource_dispatcher_host_impl.cc
A content/browser/loader/sec_fetch_site_resource_handler.cc
A content/browser/loader/sec_fetch_site_resource_handler.h
M content/browser/worker_host/worker_script_fetch_initiator.cc
M net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc
M services/network/BUILD.gn
M services/network/OWNERS
M services/network/initiator_lock_compatibility.cc
A services/network/sec_fetch_site.cc
A services/network/sec_fetch_site.h
M services/network/url_loader.cc
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-cross-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-same-site.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-origin-redirect.tentative.https.sub-expected.txt
D third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/same-site-redirect.tentative.https.sub-expected.txt
18 files changed, 333 insertions(+), 76 deletions(-)