Bug 204744 - Implement Fetch Metadata.
Summary: Implement Fetch Metadata.
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, WPTImpact
Depends on: 247696
Blocks: 246508
  Show dependency treegraph
 
Reported: 2019-12-02 04:55 PST by Mike West
Modified: 2022-11-09 13:52 PST (History)
15 users (show)

See Also:


Attachments
Patch (66.38 KB, patch)
2022-02-11 14:11 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (69.44 KB, patch)
2022-02-23 11:38 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (46.73 KB, patch)
2022-03-05 13:05 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (48.15 KB, patch)
2022-03-06 10:31 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (45.16 KB, patch)
2022-03-07 12:54 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (45.14 KB, patch)
2022-03-09 10:29 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (45.95 KB, patch)
2022-03-11 10:32 PST, 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 Mike West 2019-12-02 04:55:29 PST
Fetch Metadata defines a set of HTTP request headers which aim to give servers more information about incoming requests in order to make better decisions about whether or not to service them. This will hopefully have a positive effect on developers' ability to deal with some xsleaks-style timing attacks. https://w3c.github.io/webappsec-fetch-metadata/#intro outlines some use cases.

Chrome shipped this mechainsm in 76, and aims to ship `Sec-Fetch-Dest` in 80.

Mozilla has been participating actively in the design, and seems generally on board: https://github.com/mozilla/standards-positions/issues/88 (though there's not much movement on https://bugzilla.mozilla.org/show_bug.cgi?id=1508292).
Comment 1 Emanuele Feliziani 2020-04-02 07:37:50 PDT
Work has resumed on Firefox. They plan to ship this in Firefox 76 in May 2020: https://bugzilla.mozilla.org/show_bug.cgi?id=1508292. Hope we can have this in Safari soon. It's a nice way to increase security, especially for single-page web apps.
Comment 2 Mike West 2021-07-12 06:45:34 PDT
Firefox didn't quite make their initial schedule, but they are now shipping in 90: https://blog.mozilla.org/security/2021/07/12/firefox-90-supports-fetch-metadata-request-headers/. It would be ideal if y'all could take another look at this mechanism as it gains more adoption in the wild!
Comment 3 Radar WebKit Bug Importer 2021-09-13 06:30:46 PDT
<rdar://problem/83052324>
Comment 4 Alexandre Dieulot 2021-11-27 05:32:07 PST
Assuming Safari 15 is the last version for macOS 10.15 and/or for A8/A9/A10 devices it would be very nice to prioritize this security feature so that it’s available to that massive pool of long-lasting devices.
Comment 5 Patrick Griffis 2022-02-11 14:11:06 PST
Created attachment 451744 [details]
Patch

WIP: Implement Fetch Metadata
Comment 6 Alexandre Dieulot 2022-02-12 04:05:11 PST
Has the Safari team okayed the work in progress as something that would ship in Safari?

Would a patch in WebKit ever be used in Safari or is it a part of the stack where they typically depend on macOS/iOS-specific implementation?
Comment 7 Patrick Griffis 2022-02-12 08:47:21 PST
(In reply to Alexandre Dieulot from comment #6)
> Has the Safari team okayed the work in progress as something that would ship
> in Safari?

There is a mailinglist thread over this here: https://lists.webkit.org/pipermail/webkit-dev/2022-February/032114.html
Comment 8 Alex Christensen 2022-02-21 13:03:00 PST
Comment on attachment 451744 [details]
Patch

Please use an experimental feature for this ( WebPreferencesExperimental.yaml ) and have it off until development is complete.  That way we can do incremental development like this and not risk breaking websites with an incomplete implementation shipping in a browser.
Comment 9 Patrick Griffis 2022-02-23 11:38:54 PST
Created attachment 453006 [details]
Patch

This starts with the most basic implementation of Dest and Mode behind an experimental feature.
Comment 10 youenn fablet 2022-02-24 06:04:06 PST
Comment on attachment 453006 [details]
Patch

Some nits below.
It would be great if we could run more of the WPT tests.
It might be that we just need to change hosts[alt][www] to hosts[alt] (hoping that www.localhost is equivalent to 127.0.0.1 for those tests).

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

> Source/WebCore/loader/FetchOptions.h:51
> +    const String& modeAsString() const;

It seems this could be free functions taking destination/mode as input.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:263
> +    m_resourceRequest.removeHTTPHeaderField(HTTPHeaderName::SecFetchUser);

Why removing them?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/download.https.sub-expected.txt:1
> +024e27c3-433a-4d22-bb53-f77063ccc34e

This will probably be always fail, should we change the tests?

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/fetch-preflight.https.sub.any.worker-expected.txt:1
> +Blocked access to external URL https://www.localhost:9443/fetch/metadata/resources/echo-as-json.py

Ah this is sad... it would be cool if we could rely on hosts[alt] (i.e. 127.0.0.1) in all those tests.

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/multiple-redirect-same-site.https.sub-expected.txt:1
> +Blocked access to external URL https://www.localhost:9443/xhr/resources/redirect.py?location=https://localhost:9443/fetch/metadata/resources/record-header.py?file=redirect-multiple-same-site28c6cf90-9e43-469b-81f5-9da30411621c

This will make the tests flaky.
We should change the test here and wherever we see UUIDs.

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/redirect/redirect-http-upgrade.sub-expected.txt:8
> +TIMEOUT Http upgrade embed Test timed out

Can we mark the test as skip for now?
Comment 11 Patrick Griffis 2022-02-24 07:13:41 PST
(In reply to youenn fablet from comment #10)
> Comment on attachment 453006 [details]
> Patch
> 
> Some nits below.
> It would be great if we could run more of the WPT tests.
> It might be that we just need to change hosts[alt][www] to hosts[alt]
> (hoping that www.localhost is equivalent to 127.0.0.1 for those tests).
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453006&action=review
> 
> > Source/WebCore/loader/FetchOptions.h:51
> > +    const String& modeAsString() const;
> 
> It seems this could be free functions taking destination/mode as input.

In my WIP patch here I just put everything in a `FetchMetadata.{cpp,hpp}` that were free standing. I can go back to that layout instead of trying to keep them with FetchOptions which won't make sense for the `Site` values anyway.

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:263
> > +    m_resourceRequest.removeHTTPHeaderField(HTTPHeaderName::SecFetchUser);
> 
> Why removing them?

We just don't want any incorrect headers to be there. I believe its possible for the site to make its own Request() with headers.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/download.https.sub-expected.txt:1
> > +024e27c3-433a-4d22-bb53-f77063ccc34e
> 
> This will probably be always fail, should we change the tests?

Yes these are annoying. I'll look into these UUID failures.
Comment 12 Patrick Griffis 2022-02-28 10:30:48 PST
(In reply to youenn fablet from comment #10)
> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/download.https.sub-expected.txt:1
> > +024e27c3-433a-4d22-bb53-f77063ccc34e
> 
> This will probably be always fail, should we change the tests?
> 

Changed this test: https://github.com/web-platform-tests/wpt/pull/33002

However many of the other cases we see UUIDs are more annoying since they are part of the URL and we only see them in expectations because they are blocked.
Comment 13 youenn fablet 2022-02-28 10:33:32 PST
> > Why removing them?
> 
> We just don't want any incorrect headers to be there. I believe its possible
> for the site to make its own Request() with headers.

We should probably make them forbidden header names, https://fetch.spec.whatwg.org/#forbidden-header-name.
Comment 14 Alex Christensen 2022-02-28 11:17:28 PST
Comment on attachment 453006 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:547
> +      "ENABLE(EXPERIMENTAL_FEATURES)" : true
> +      default: false

Let's just keep the default false everywhere (except the test runners) until it's done.
Comment 15 Simon Pieters (:zcorpan) 2022-02-28 13:08:53 PST
(In reply to youenn fablet from comment #13)
> > We just don't want any incorrect headers to be there. I believe its possible
> > for the site to make its own Request() with headers.
> 
> We should probably make them forbidden header names,
> https://fetch.spec.whatwg.org/#forbidden-header-name.

Fetch Metadata headers are prefixed with Sec- which are therefore part of that list per spec.

https://w3c.github.io/webappsec-fetch-metadata/#sec-prefix
Comment 16 Patrick Griffis 2022-03-05 13:05:24 PST
Created attachment 453919 [details]
Patch
Comment 17 EWS Watchlist 2022-03-05 13:08:02 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 18 Patrick Griffis 2022-03-06 10:31:58 PST
Created attachment 453932 [details]
Patch
Comment 19 youenn fablet 2022-03-06 13:36:29 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=453932&action=review

> Source/WebCore/loader/FetchOptions.h:78
> +const String& fetchDestinationAsString(FetchOptions::Destination);
> +const String& fetchModeAsString(FetchOptions::Mode);


Our binding generator probably already generates these routines as convertEnumerationToString.
We might only need to change their name accordingly and declare them here with a comment stating their implementation is generated by binding generator.

Also, it might be good to file a bug about adding support for websocket mode in websocket code path.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:258
> +        return;

It would probably read better if we would do this check in the call site, something like:
if (frameLoader.frame().settings().fetchMetadataEnabled())
     request.addFetchMetadataHeaders();

We could also mention https://w3c.github.io/webappsec-fetch-metadata/#fetch-integration here.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:260
> +    if (m_resourceRequest.requester() == ResourceRequestBase::Requester::XHR)

Why this XHR check?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:264
> +    if (!requestOrigin->isPotentiallyTrustworthy())

In theory, we should use the isURLPotentiallyTrustworthy function that is in Document.cpp to exactly match the spec.
That does not make a difference though in practice.

> Source/WebCore/platform/network/HTTPHeaderNames.in:90
> +Sec-Fetch-User

Let's only add the two we need for now.

> Source/WebCore/platform/network/HTTPParsers.cpp:985
> +    case HTTPHeaderName::SecFetchUser:

It would be better if we would not have to add them here.

As per spec, this should not be needed since the headers are added after CORS checks but our implementation is not organized in the same way as the spec sadly.
For instance we are adding UserAgent but we are not safe-listing UserAgent here.
We can try removing the headers in cleanHTTPRequestHeadersForAccessControl to prevent service worker interception related issues since they will be readded later on (we might need specific handling in WK1 SubresourceLoader::checkRedirectionCrossOriginAccessControl though).

> LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/download.https.sub.html:15
> +      a.text = 'Download URL';

Is it already upstreamed or is there a plan to upstream that change?
Comment 20 Patrick Griffis 2022-03-07 12:54:30 PST
Created attachment 454016 [details]
Patch
Comment 21 Patrick Griffis 2022-03-07 13:02:09 PST
(In reply to youenn fablet from comment #19)

Thanks for the review it was very helpful.


> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453932&action=review
> 
> > Source/WebCore/loader/FetchOptions.h:78
> > +const String& fetchDestinationAsString(FetchOptions::Destination);
> > +const String& fetchModeAsString(FetchOptions::Mode);
> 
> 
> Our binding generator probably already generates these routines as
> convertEnumerationToString.
> We might only need to change their name accordingly and declare them here
> with a comment stating their implementation is generated by binding
> generator.

Nice, it does.

> Also, it might be good to file a bug about adding support for websocket mode
> in websocket code path.

Sure: https://bugs.webkit.org/show_bug.cgi?id=237550

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:258
> > +        return;
> 
> It would probably read better if we would do this check in the call site,
> something like:
> if (frameLoader.frame().settings().fetchMetadataEnabled())
>      request.addFetchMetadataHeaders();
> 
> We could also mention
> https://w3c.github.io/webappsec-fetch-metadata/#fetch-integration here.

Done.

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:260
> > +    if (m_resourceRequest.requester() == ResourceRequestBase::Requester::XHR)
> 
> Why this XHR check?

Removed, it was a bad workaround.

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:264
> > +    if (!requestOrigin->isPotentiallyTrustworthy())
> 
> In theory, we should use the isURLPotentiallyTrustworthy function that is in
> Document.cpp to exactly match the spec.
> That does not make a difference though in practice.

I left this here as the follow up patch for SecFetchSite uses the SecurityOrigin anyway.

> > Source/WebCore/platform/network/HTTPHeaderNames.in:90
> > +Sec-Fetch-User
> 
> Let's only add the two we need for now.

Done.

> > Source/WebCore/platform/network/HTTPParsers.cpp:985
> > +    case HTTPHeaderName::SecFetchUser:
> 
> It would be better if we would not have to add them here.
> 
> As per spec, this should not be needed since the headers are added after
> CORS checks but our implementation is not organized in the same way as the
> spec sadly.
> For instance we are adding UserAgent but we are not safe-listing UserAgent
> here.
> We can try removing the headers in cleanHTTPRequestHeadersForAccessControl
> to prevent service worker interception related issues since they will be
> readded later on (we might need specific handling in WK1
> SubresourceLoader::checkRedirectionCrossOriginAccessControl though).

Thanks for this. The solution never felt right I just wasn't finding the right place. This worked very well.

> > LayoutTests/imported/w3c/web-platform-tests/fetch/metadata/download.https.sub.html:15
> > +      a.text = 'Download URL';
> 
> Is it already upstreamed or is there a plan to upstream that change?

Landed upstream: https://github.com/web-platform-tests/wpt/commit/b95980870bfbad4969b88bd8c3a8d488608e86f5
Comment 22 youenn fablet 2022-03-07 23:47:03 PST
Comment on attachment 454016 [details]
Patch

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

> Source/WebCore/loader/CrossOriginAccessControl.cpp:224
> +    }

I do not think we need to add SecFetch here since these headers cannot be set by the web application.
We can remove them unilaterally, which should further simplify\y the patch.

As said before, this change might cause removal of those headers in WK1 in case of cross site redirection for CORS loads.
Can you validate that we have proper testing for this case?

> Source/WebCore/loader/FetchOptions.h:80
> +String convertEnumerationToString(FetchOptions::Mode);

Given we are only using them in one place for now, can we move these declarations close to the call site?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:34
> +#include "Frame.h"

No longer needed?
Comment 23 Patrick Griffis 2022-03-08 10:59:36 PST
(In reply to youenn fablet from comment #22)
> As said before, this change might cause removal of those headers in WK1 in
> case of cross site redirection for CORS loads.
> Can you validate that we have proper testing for this case?


This is covered by `web-platform-tests/fetch/metadata/redirect/*` however some parts of these timeout on all ports. I plan on fixing these however I don't really have a way to test WK1 locally.
Comment 24 Patrick Griffis 2022-03-08 13:58:28 PST
(In reply to Patrick Griffis from comment #23)
> This is covered by `web-platform-tests/fetch/metadata/redirect/*` however
> some parts of these timeout on all ports.

For now I'll try to create a WebKit specific test just to cover this.
Comment 25 Simon Pieters (:zcorpan) 2022-03-09 00:51:07 PST
FYI, some test bugs may be fixed in https://github.com/web-platform-tests/wpt/pull/25247
Comment 26 Patrick Griffis 2022-03-09 10:29:08 PST
Created attachment 454265 [details]
Patch
Comment 27 Patrick Griffis 2022-03-11 10:32:43 PST
Created attachment 454497 [details]
Patch
Comment 28 EWS 2022-03-15 13:31:26 PDT
Committed r291310 (248449@main): <https://commits.webkit.org/248449@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454497 [details].
Comment 29 Sam Sneddon [:gsnedders] 2022-09-30 09:06:05 PDT
Patrick: is there any open bug covering the remainder of Fetch Metadata?

Per the commit message above:

> Currently only Fetch-Sec-Mode and Fetch-Sec-Dest are implemented with more to come in later patches.

We have bug 238265 open, but is that the rest of it? We should probably have something tracking the overall feature (including enablement).