Bug 204744

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.

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!

<rdar://problem/83052324>

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.

Created attachment 451744 [details] Patch WIP: Implement Fetch Metadata

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?

(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 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.

Created attachment 453006 [details] Patch This starts with the most basic implementation of Dest and Mode behind an experimental feature.

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?

(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.

(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.

> > 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 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.

(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

Created attachment 453919 [details] Patch

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

Created attachment 453932 [details] Patch

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?

Created attachment 454016 [details] Patch

(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 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?

(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.

(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.

FYI, some test bugs may be fixed in https://github.com/web-platform-tests/wpt/pull/25247

Created attachment 454265 [details] Patch

Created attachment 454497 [details] Patch

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].

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).