Bug 238265 - FetchMetadata: Implement Sec-Fetch-Site
Summary: FetchMetadata: Implement Sec-Fetch-Site
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Patrick Griffis
URL:
Keywords: InRadar
Depends on: 247696 243428
Blocks: 246508
  Show dependency treegraph
 
Reported: 2022-03-23 09:32 PDT by Patrick Griffis
Modified: 2023-01-10 09:18 PST (History)
9 users (show)

See Also:


Attachments
Patch (51.73 KB, patch)
2022-03-23 09:35 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (67.59 KB, patch)
2022-03-23 11:19 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (68.12 KB, patch)
2022-04-04 08:54 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (77.90 KB, patch)
2022-04-06 13:33 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (77.93 KB, patch)
2022-04-11 11:28 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (80.11 KB, patch)
2022-04-15 12:04 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (80.58 KB, patch)
2022-04-16 09:57 PDT, Patrick Griffis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Implement FetchMetadataSite (76.99 KB, patch)
2022-07-01 13:36 PDT, Patrick Griffis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Implement FetchMetadata Site (75.75 KB, patch)
2022-07-01 13:45 PDT, Patrick Griffis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Implement FetchMetadata Site (126.12 KB, patch)
2022-08-15 14:53 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Griffis 2022-03-23 09:32:29 PDT
FetchMetadata: Implement Sec-Fetch-Site
Comment 1 Patrick Griffis 2022-03-23 09:35:37 PDT
Created attachment 455507 [details]
Patch
Comment 2 Patrick Griffis 2022-03-23 09:54:36 PDT
Comment on attachment 455507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455507&action=review

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/fetch.sub-expected.txt:10
> +FAIL http->http fetch (non-trustworthy destination => no metadata): sec-fetch-site assert_equals: expected "" but got "same-origin"

NOTE: This will always FAIL locally because `localhost` is a trustworthy desintation.

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/form.https.sub-expected.txt:23
> +FAIL localhost -> www.localhost:9443 iframe: forced: sec-fetch-site assert_equals: expected "same-site" but got "cross-site"

NOTE: This FAILs locally because `localhost` and `www.localhost` are not considered same site. This is probably a bug as the Registerable Domain for both of them is `null` in theory.

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/iframe.sub-expected.txt:12
> +FAIL Non-secure same-origin iframe => No headers: sec-fetch-site assert_equals: expected "" but got "same-origin"

NOTE: This will always FAIL locally. `localhost` is trustworthy.

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:26
> +FAIL Https downgrade iframe: sec-fetch-site assert_equals: expected "" but got "cross-site"

NOTE: Same here. Downgrading on `localhost` is still trustworthy.
Comment 3 Patrick Griffis 2022-03-23 10:26:21 PDT Comment hidden (obsolete)
Comment 4 Patrick Griffis 2022-03-23 11:19:08 PDT
Created attachment 455524 [details]
Patch
Comment 5 Patrick Griffis 2022-03-23 11:21:06 PDT
(In reply to Patrick Griffis from comment #3)
> The `topPrivatelyControlledDomain()` function seems to be platform specific
> as it depends on libraries such as libpsl. It is implemented by the soup and
> curl platforms but will also be needed on Apple/win ports.

Maybe only the win port needs it actually.
Comment 6 Patrick Griffis 2022-03-29 09:42:30 PDT Comment hidden (obsolete)
Comment 7 Radar WebKit Bug Importer 2022-03-30 09:33:15 PDT
<rdar://problem/91049146>
Comment 8 Patrick Griffis 2022-04-04 08:54:13 PDT
Created attachment 456576 [details]
Patch
Comment 9 Patrick Griffis 2022-04-04 08:57:00 PDT
(In reply to Patrick Griffis from comment #2)
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/form.https.sub-expected.txt:23
> > +FAIL localhost -> www.localhost:9443 iframe: forced: sec-fetch-site assert_equals: expected "same-site" but got "cross-site"
> 
> NOTE: This FAILs locally because `localhost` and `www.localhost` are not
> considered same site. This is probably a bug.

So we can simply change `topPrivatelyControlledDomain()` to return `localhost` for `*.localhost` and it fixes these tests.

I can't change the macOS impl of this though and its probably a change worth discussing if it makes sense.
Comment 10 Patrick Griffis 2022-04-04 09:03:06 PDT
(In reply to Patrick Griffis from comment #9)
> So we can simply change `topPrivatelyControlledDomain()` to return
> `localhost` for `*.localhost` and it fixes these tests.
> 
> I can't change the macOS impl of this though

(I'm blind and missed the macOS impl in obj-c with my search)
Comment 11 youenn fablet 2022-04-04 10:01:27 PDT
Comment on attachment 456576 [details]
Patch



View in context: https://bugs.webkit.org/attachment.cgi?id=456576&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:719
> +        //        of Sec-Fetch-Site in these cases.

We should be able to get the previous SecFetchSite information from the previous request.
If the previous request is cross origin, we can stick to it. Otherwise, we can check for same site/same origin.

> Source/WebCore/page/SecurityOrigin.cpp:473
> +bool SecurityOrigin::isSameSiteAs(const SecurityOrigin& other) const

We currently implement same site using RegistrableDomain operator ==.
Can you check whether there are differences between the two implementations?
Comment 12 Patrick Griffis 2022-04-04 11:02:06 PDT
Comment on attachment 456576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456576&action=review

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:719
>> +        //        of Sec-Fetch-Site in these cases.
> 
> We should be able to get the previous SecFetchSite information from the previous request.
> If the previous request is cross origin, we can stick to it. Otherwise, we can check for same site/same origin.

I don't think we have enough information as-is.

What we need to defend against is a redirect chain such as:

http://origin1 (site=undefined) -> http://origin2 (site=undefined) -> https://origin2 (site=cross-origin)

When we get to https://origin2 we need the full chain to know the correct value, otherwise its easy to mistakenly call it same-origin.

My comment above mentions the idea of tracking the list with all Requests.
Comment 13 youenn fablet 2022-04-04 11:08:56 PDT
(In reply to Patrick Griffis from comment #12)
> Comment on attachment 456576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456576&action=review
> 
> >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:719
> >> +        //        of Sec-Fetch-Site in these cases.
> > 
> > We should be able to get the previous SecFetchSite information from the previous request.
> > If the previous request is cross origin, we can stick to it. Otherwise, we can check for same site/same origin.
> 
> I don't think we have enough information as-is.
> 
> What we need to defend against is a redirect chain such as:
> 
> http://origin1 (site=undefined) -> http://origin2 (site=undefined) ->
> https://origin2 (site=cross-origin)
> 
> When we get to https://origin2 we need the full chain to know the correct
> value, otherwise its easy to mistakenly call it same-origin.

It might be easy but we do not need the whole chain, since the last computed value is correct for all the previous redirects. We just need to take into account the last value and the new URL. We do this to compute whether a request is cross origin or not (once it is cross origin, it stays cross origin).
Comment 14 Patrick Griffis 2022-04-06 13:33:41 PDT Comment hidden (obsolete)
Comment 15 Patrick Griffis 2022-04-06 13:37:00 PDT
(In reply to youenn fablet from comment #11)
> We should be able to get the previous SecFetchSite information from the
> previous request.
> If the previous request is cross origin, we can stick to it. Otherwise, we
> can check for same site/same origin.

I've implemented this where it's tracked for every request in the chain.


One thing I could use help with is that `www.localhost` requests are blocked, even though it should be considered same-site.

I fixed the `www.127.0.0.1` ones failing upstream: https://github.com/web-platform-tests/wpt/pull/33484
Comment 16 Patrick Griffis 2022-04-11 11:28:50 PDT
Created attachment 457275 [details]
Patch
Comment 17 Patrick Griffis 2022-04-15 12:04:43 PDT
Created attachment 457710 [details]
Patch
Comment 18 Patrick Griffis 2022-04-16 09:57:22 PDT
Created attachment 457749 [details]
Patch
Comment 19 Patrick Griffis 2022-05-12 10:36:14 PDT
(In reply to youenn fablet from comment #11)
> > Source/WebCore/page/SecurityOrigin.cpp:473
> > +bool SecurityOrigin::isSameSiteAs(const SecurityOrigin& other) const
> 
> We currently implement same site using RegistrableDomain operator ==.
> Can you check whether there are differences between the two implementations?

From what I could tell these both compare domains the same and both use topPrivatelyControlledDomain(). However when PSL isn't available RegistrableDomain falls back to full host comparisons. I think a hard requirement on PSL for this feature is a good idea.
Comment 20 youenn fablet 2022-05-16 00:16:03 PDT
Comment on attachment 457749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457749&action=review

> Source/WebCore/loader/FetchOptions.h:63
> +    Site site { Site::SameOrigin };

I don't think site should be within FetchOptions.
FetchOptions should more or less remain immutable during a load. FetchOptions values are set by the creator of the load to properly setup the load.
Here, site is an internal state of the load, which can be stored directly in SubresourceLoader for instance.

> Source/WebCore/loader/SubresourceLoader.cpp:281
> +        m_options.site = m_documentLoader->cachedResourceLoader().computeFetchMetadataSite(newRequest, m_resource->type(), options().mode, originalOrigin, options().site);

Can we make m_options.site a SubresourceLoader member field on its own?
We can then pass it to updateRequestAfterRedirection below as an additional parameter.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:660
> +static void updateRequestFetchMetadataHeaders(ResourceRequest& request, const ResourceLoaderOptions& options)

Let's add a FetchOptions::Site parameter to this function.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:881
> +        request.setFetchMetadataSite(computeFetchMetadataSite(request.resourceRequest(), type, request.options().mode, frameLoader.frame().document()->securityOrigin(), request.options().site));

We do not need to set the site options here if we just pass it to updateRequestFetchMetadataHeaders below.
Comment 21 Patrick Griffis 2022-06-18 11:35:17 PDT
Comment on attachment 457749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457749&action=review

>> Source/WebCore/loader/SubresourceLoader.cpp:281
>> +        m_options.site = m_documentLoader->cachedResourceLoader().computeFetchMetadataSite(newRequest, m_resource->type(), options().mode, originalOrigin, options().site);
> 
> Can we make m_options.site a SubresourceLoader member field on its own?
> We can then pass it to updateRequestAfterRedirection below as an additional parameter.

Maybe I'm missing it. But `CachedResourceLoader::updateHTTPRequestHeaders()` doesn't seem to have easy access to a SubResourceLoader to get this information?

`CachedResourceLoader::requestResource()` updates the request headers first, then creates a `CachedResource` with that information but we can't get the
SubresourceLoader until after that?
Comment 22 youenn fablet 2022-06-21 00:13:22 PDT
Comment on attachment 457749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457749&action=review

>>> Source/WebCore/loader/SubresourceLoader.cpp:281
>>> +        m_options.site = m_documentLoader->cachedResourceLoader().computeFetchMetadataSite(newRequest, m_resource->type(), options().mode, originalOrigin, options().site);
>> 
>> Can we make m_options.site a SubresourceLoader member field on its own?
>> We can then pass it to updateRequestAfterRedirection below as an additional parameter.
> 
> Maybe I'm missing it. But `CachedResourceLoader::updateHTTPRequestHeaders()` doesn't seem to have easy access to a SubResourceLoader to get this information?
> 
> `CachedResourceLoader::requestResource()` updates the request headers first, then creates a `CachedResource` with that information but we can't get the
> SubresourceLoader until after that?

CachedResourceLoader::updateHTTPRequestHeaders() is called for the initial load. We can compute the value from the request itself.
SubresourceLoader will compute this same value as well (we can use the same routine as a free function) and in case of redirection will update this value to a new value.
SubresourceLoader can pass this new value to updateRequestAfterRedirection.
This way, we keep the model of FetchOptions being 'immutable' load options set by the client, and this site property being an internal state of the loader.
Comment 23 Patrick Griffis 2022-07-01 13:36:05 PDT Comment hidden (obsolete)
Comment 24 Patrick Griffis 2022-07-01 13:45:03 PDT
Created attachment 460621 [details]
Implement FetchMetadata Site
Comment 25 Darin Adler 2022-07-01 13:48:51 PDT
Comment on attachment 460620 [details]
Implement FetchMetadataSite

View in context: https://bugs.webkit.org/attachment.cgi?id=460620&action=review

I see a number of new failures in the test results. Can we fix more of them as-part-of/before adding this feature?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/fetch.sub-expected.txt:10
> -PASS http->http fetch (non-trustworthy destination => no metadata): sec-fetch-site
> +FAIL http->http fetch (non-trustworthy destination => no metadata): sec-fetch-site assert_equals: expected "" but got "same-origin"

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/iframe.sub-expected.txt:12
> -PASS Non-secure same-origin iframe => No headers: sec-fetch-dest
> -PASS Non-secure same-origin iframe => No headers: sec-fetch-mode
> -PASS Non-secure same-origin iframe => No headers: sec-fetch-site
> +FAIL Non-secure same-origin iframe => No headers: sec-fetch-dest assert_equals: expected "" but got "iframe"
> +FAIL Non-secure same-origin iframe => No headers: sec-fetch-mode assert_equals: expected "" but got "navigate"
> +FAIL Non-secure same-origin iframe => No headers: sec-fetch-site assert_equals: expected "" but got "same-origin"

What’s the explanation for these new failures? Can we fix them?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/multiple-redirect-https-downgrade-upgrade.sub-expected.txt:10
> -PASS Https downgrade-upgrade embed
> -PASS Https downgrade-upgrade fetch() api
> -PASS Https downgrade-upgrade object
> +TIMEOUT Https downgrade-upgrade embed Test timed out
> +NOTRUN Https downgrade-upgrade fetch() api
> +NOTRUN Https downgrade-upgrade object

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/multiple-redirect-https-downgrade-upgrade.sub-expected.txt:13
> -PASS Https downgrade-upgrade stylesheet
> -TIMEOUT Https downgrade-upgrade track Test timed out
> +NOTRUN Https downgrade-upgrade stylesheet
> +NOTRUN Https downgrade-upgrade track

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-http-upgrade.sub-expected.txt:10
> -PASS Http upgrade embed
> -PASS Http upgrade fetch() api
> -PASS Http upgrade object
> +TIMEOUT Http upgrade embed Test timed out
> +NOTRUN Http upgrade fetch() api
> +NOTRUN Http upgrade object

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-http-upgrade.sub-expected.txt:13
> -PASS Http upgrade stylesheet
> -TIMEOUT Http upgrade track Test timed out
> +NOTRUN Http upgrade stylesheet
> +NOTRUN Http upgrade track

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:10
> -PASS Https downgrade embed
> -PASS Https downgrade fetch() api
> -PASS Https downgrade object
> +TIMEOUT Https downgrade embed Test timed out
> +NOTRUN Https downgrade fetch() api
> +NOTRUN Https downgrade object

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:14
> -PASS Https downgrade stylesheet
> -TIMEOUT Https downgrade track Test timed out
> +NOTRUN Https downgrade stylesheet
> +NOTRUN Https downgrade track
>  NOTRUN Https downgrade image => No headers

What’s the explanation for this new failure? Can we fix it?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:17
> -PASS Https downgrade script => No headers: sec-fetch-mode
> -PASS Https downgrade script => No headers: sec-fetch-site
> +FAIL Https downgrade script => No headers: sec-fetch-mode assert_equals: expected "" but got "no-cors"
> +FAIL Https downgrade script => No headers: sec-fetch-site assert_equals: expected "" but got "cross-site"

What’s the explanation for these new failures? Can we fix them?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:27
> -PASS Https downgrade script => No headers: sec-fetch-dest
> -PASS Https downgrade iframe: sec-fetch-dest
> -PASS Https downgrade iframe: sec-fetch-mode
> -PASS Https downgrade iframe: sec-fetch-site
> -PASS Https downgrade iframe: sec-fetch-user
> -PASS Https downgrade top level navigation: sec-fetch-dest
> -PASS Https downgrade top level navigation: sec-fetch-mode
> -PASS Https downgrade top level navigation: sec-fetch-site
> +FAIL Https downgrade script => No headers: sec-fetch-dest assert_equals: expected "" but got "script"
> +FAIL Https downgrade top level navigation: sec-fetch-dest assert_equals: expected "" but got "document"
> +FAIL Https downgrade top level navigation: sec-fetch-mode assert_equals: expected "" but got "navigate"
> +FAIL Https downgrade top level navigation: sec-fetch-site assert_equals: expected "" but got "cross-site"
>  PASS Https downgrade top level navigation: sec-fetch-user
> +FAIL Https downgrade iframe: sec-fetch-dest assert_equals: expected "" but got "iframe"
> +FAIL Https downgrade iframe: sec-fetch-mode assert_equals: expected "" but got "navigate"
> +FAIL Https downgrade iframe: sec-fetch-site assert_equals: expected "" but got "cross-site"
> +PASS Https downgrade iframe: sec-fetch-user

What’s the explanation for these new failures? Can we fix them?
Comment 26 Patrick Griffis 2022-07-03 09:03:38 PDT
(In reply to Darin Adler from comment #25)
> Comment on attachment 460620 [details]
> Implement FetchMetadataSite
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460620&action=review
> 
> I see a number of new failures in the test results. Can we fix more of them
> as-part-of/before adding this feature?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/fetch.sub-expected.txt:10
> > -PASS http->http fetch (non-trustworthy destination => no metadata): sec-fetch-site
> > +FAIL http->http fetch (non-trustworthy destination => no metadata): sec-fetch-site assert_equals: expected "" but got "same-origin"
> 
> What’s the explanation for this new failure? Can we fix it?

All of our tests run on trustworthy destinations (localhost). So many of these tests cannot work unless we set up a fake domain like WPT does. This requires making system wide changes as root. I don't know that this is appropriate.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/iframe.sub-expected.txt:12
> > -PASS Non-secure same-origin iframe => No headers: sec-fetch-dest
> > -PASS Non-secure same-origin iframe => No headers: sec-fetch-mode
> > -PASS Non-secure same-origin iframe => No headers: sec-fetch-site
> > +FAIL Non-secure same-origin iframe => No headers: sec-fetch-dest assert_equals: expected "" but got "iframe"
> > +FAIL Non-secure same-origin iframe => No headers: sec-fetch-mode assert_equals: expected "" but got "navigate"
> > +FAIL Non-secure same-origin iframe => No headers: sec-fetch-site assert_equals: expected "" but got "same-origin"
> 
> What’s the explanation for these new failures? Can we fix them?

Same. http://localhost is trustworthy.
 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:17
> > -PASS Https downgrade script => No headers: sec-fetch-mode
> > -PASS Https downgrade script => No headers: sec-fetch-site
> > +FAIL Https downgrade script => No headers: sec-fetch-mode assert_equals: expected "" but got "no-cors"
> > +FAIL Https downgrade script => No headers: sec-fetch-site assert_equals: expected "" but got "cross-site"
> 
> What’s the explanation for these new failures? Can we fix them?

Trustworthy destination. http://localhost.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:27
> > -PASS Https downgrade script => No headers: sec-fetch-dest
> > -PASS Https downgrade iframe: sec-fetch-dest
> > -PASS Https downgrade iframe: sec-fetch-mode
> > -PASS Https downgrade iframe: sec-fetch-site
> > -PASS Https downgrade iframe: sec-fetch-user
> > -PASS Https downgrade top level navigation: sec-fetch-dest
> > -PASS Https downgrade top level navigation: sec-fetch-mode
> > -PASS Https downgrade top level navigation: sec-fetch-site
> > +FAIL Https downgrade script => No headers: sec-fetch-dest assert_equals: expected "" but got "script"
> > +FAIL Https downgrade top level navigation: sec-fetch-dest assert_equals: expected "" but got "document"
> > +FAIL Https downgrade top level navigation: sec-fetch-mode assert_equals: expected "" but got "navigate"
> > +FAIL Https downgrade top level navigation: sec-fetch-site assert_equals: expected "" but got "cross-site"
> >  PASS Https downgrade top level navigation: sec-fetch-user
> > +FAIL Https downgrade iframe: sec-fetch-dest assert_equals: expected "" but got "iframe"
> > +FAIL Https downgrade iframe: sec-fetch-mode assert_equals: expected "" but got "navigate"
> > +FAIL Https downgrade iframe: sec-fetch-site assert_equals: expected "" but got "cross-site"
> > +PASS Https downgrade iframe: sec-fetch-user
> 
> What’s the explanation for these new failures? Can we fix them?

All trustworthy.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/multiple-redirect-https-downgrade-upgrade.sub-expected.txt:10
> > -PASS Https downgrade-upgrade embed
> > -PASS Https downgrade-upgrade fetch() api
> > -PASS Https downgrade-upgrade object
> > +TIMEOUT Https downgrade-upgrade embed Test timed out
> > +NOTRUN Https downgrade-upgrade fetch() api
> > +NOTRUN Https downgrade-upgrade object
> 
> What’s the explanation for this new failure? Can we fix it?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/multiple-redirect-https-downgrade-upgrade.sub-expected.txt:13
> > -PASS Https downgrade-upgrade stylesheet
> > -TIMEOUT Https downgrade-upgrade track Test timed out
> > +NOTRUN Https downgrade-upgrade stylesheet
> > +NOTRUN Https downgrade-upgrade track
> 
> What’s the explanation for this new failure? Can we fix it?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-http-upgrade.sub-expected.txt:10
> > -PASS Http upgrade embed
> > -PASS Http upgrade fetch() api
> > -PASS Http upgrade object
> > +TIMEOUT Http upgrade embed Test timed out
> > +NOTRUN Http upgrade fetch() api
> > +NOTRUN Http upgrade object
> 
> What’s the explanation for this new failure? Can we fix it?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-http-upgrade.sub-expected.txt:13
> > -PASS Http upgrade stylesheet
> > -TIMEOUT Http upgrade track Test timed out
> > +NOTRUN Http upgrade stylesheet
> > +NOTRUN Http upgrade track
> 
> What’s the explanation for this new failure? Can we fix it?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:10
> > -PASS Https downgrade embed
> > -PASS Https downgrade fetch() api
> > -PASS Https downgrade object
> > +TIMEOUT Https downgrade embed Test timed out
> > +NOTRUN Https downgrade fetch() api
> > +NOTRUN Https downgrade object
> 
> What’s the explanation for this new failure? Can we fix it?
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-https-downgrade.sub-expected.txt:14
> > -PASS Https downgrade stylesheet
> > -TIMEOUT Https downgrade track Test timed out
> > +NOTRUN Https downgrade stylesheet
> > +NOTRUN Https downgrade track
> >  NOTRUN Https downgrade image => No headers
> 
> What’s the explanation for this new failure? Can we fix it?

I will look into this in a bit more detail why these are NOTRUN rather than FAIL. Some failures are expected for the same domain issue.
Comment 27 Patrick Griffis 2022-07-03 11:02:27 PDT
(In reply to Patrick Griffis from comment #26)
> I will look into this in a bit more detail why these are NOTRUN

I can at least confirm these are genuinely NOTRUN and they were not ran before this patch.

All of the test cases appendChild() an element (be it a link, or embed, etc) and WebKit never loads the elements. The only one that WebKit loads is the link element with `rel=preload` set.
Comment 28 Darin Adler 2022-07-03 17:46:25 PDT
(In reply to Patrick Griffis from comment #26)
> All of our tests run on trustworthy destinations (localhost). So many of
> these tests cannot work unless we set up a fake domain like WPT does. This
> requires making system wide changes as root. I don't know that this is
> appropriate.

This doesn’t sound right to me. It seems to me that we *could* set up our tests so they see a fake domain without system-wide changes as root. These might require platform-specific techniques so the way we do this with CFNetwork on macOS and iOS might not be the same as on other platforms.

I’m not saying we need to do this immediately, or that it is required for this patch, but I would like to know more about whether we plan to deal with this long term or not.

Is there someone else watching this bug who knows about this?
Comment 29 Patrick Griffis 2022-07-03 18:45:42 PDT
(In reply to Darin Adler from comment #28)
> (In reply to Patrick Griffis from comment #26)
> > All of our tests run on trustworthy destinations (localhost). So many of
> > these tests cannot work unless we set up a fake domain like WPT does. This
> > requires making system wide changes as root. I don't know that this is
> > appropriate.
> 
> This doesn’t sound right to me. It seems to me that we *could* set up our
> tests so they see a fake domain without system-wide changes as root. These
> might require platform-specific techniques so the way we do this with
> CFNetwork on macOS and iOS might not be the same as on other platforms.

Sure other ways are possible. On libsoup we can make a custom GResolver to fake DNS responses and CURL does allow hardcoding an IP to a hostname. They are all very platform specific.
Comment 30 Sam Sneddon [:gsnedders] 2022-07-04 05:33:54 PDT
(In reply to Darin Adler from comment #28)
> (In reply to Patrick Griffis from comment #26)
> > All of our tests run on trustworthy destinations (localhost). So many of
> > these tests cannot work unless we set up a fake domain like WPT does. This
> > requires making system wide changes as root. I don't know that this is
> > appropriate.
> 
> This doesn’t sound right to me. It seems to me that we *could* set up our
> tests so they see a fake domain without system-wide changes as root. These
> might require platform-specific techniques so the way we do this with
> CFNetwork on macOS and iOS might not be the same as on other platforms.
> 
> I’m not saying we need to do this immediately, or that it is required for
> this patch, but I would like to know more about whether we plan to deal with
> this long term or not.
> 
> Is there someone else watching this bug who knows about this?

This is Bug 127676.
Comment 31 Patrick Griffis 2022-07-05 08:10:55 PDT
(In reply to Sam Sneddon [:gsnedders] from comment #30)
> This is Bug 127676.

I can work on this for the GLib/libsoup ports.
Comment 32 Patrick Griffis 2022-07-16 11:05:20 PDT
Initial work done here, input appreciated: https://github.com/WebKit/WebKit/pull/2489
Comment 33 Patrick Griffis 2022-08-15 14:53:32 PDT
Created attachment 461641 [details]
Implement FetchMetadata Site

The attached depends on bug 243428 however now every test is a PASS on GTK.
Comment 34 Patrick Griffis 2022-09-09 12:46:47 PDT
Pull request: https://github.com/WebKit/WebKit/pull/4186
Comment 35 EWS 2022-10-20 17:02:28 PDT
Committed 255810@main (e5f68a529bf1): <https://commits.webkit.org/255810@main>

Reviewed commits have been landed. Closing PR #4186 and removing active labels.
Comment 36 Alex Christensen 2023-01-10 09:18:31 PST
Comment on attachment 461641 [details]
Implement FetchMetadata Site

View in context: https://bugs.webkit.org/attachment.cgi?id=461641&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:705
> +    if (m_documentLoader->frame()->settings().fetchMetadataEnabled()) {

This needed a null check.  Adding in https://github.com/WebKit/WebKit/pull/8461