Implement navigation reporting for Cross-Origin-Opener-Policy

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Implement basic reporting for Cross-Origin-Opener-Policy.

[Chris Dumez]

Created attachment 437656 [details]
WIP patch

[Chris Dumez]

Created attachment 437682 [details]
WIP patch

[EWS Watchlist]

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

[Chris Dumez]

Had to update the WPT tests so they can run properly within the WebKit test infrastructure: https://github.com/web-platform-tests/wpt/pull/30411

[Chris Dumez]

Created attachment 437746 [details]
WIP patch

[Chris Dumez]

Created attachment 437748 [details]
Patch

[Chris Dumez]

Created attachment 437765 [details]
Patch

[Chris Dumez]

The first subtest of these tests seem to fail consistently on the Apple Silicon bot:
imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/navigation-reporting/report-only-same-origin-report-to.https.html
imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/navigation-reporting/reporting-popup-unsafe-none-report-to.https.html

The non Apple Silicon bots seem fine and I cannot reproduce locally (I am using Intel). Not sure why this would be Apple-Silicon specific...

[Chris Dumez]

Created attachment 437781 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> The first subtest of these tests seem to fail consistently on the Apple
> Silicon bot:
> imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/
> navigation-reporting/report-only-same-origin-report-to.https.html
> imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/
> navigation-reporting/reporting-popup-unsafe-none-report-to.https.html
> 
> The non Apple Silicon bots seem fine and I cannot reproduce locally (I am
> using Intel). Not sure why this would be Apple-Silicon specific...

I am able to reproduce on a release build like so:
Tools/Scripts/run-webkit-tests --release --no-retry --no-build imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/navigation-reporting/report-only-four-reports.https.html imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/navigation-reporting/report-only-same-origin-report-to.https.html

I am investigating.

[Chris Dumez]

Created attachment 437871 [details]
Patch

[Chris Dumez]

Created attachment 437872 [details]
Patch

[Alex Christensen]

Comment on attachment 437872 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:76
> +#include "PageGroup.h"

I think it would be nicer to use something like Page::forEachPage instead of PageGroup.  Right now every WKWebView is in the same PageGroup unless you use SPI which I'm trying to remove.

> Source/WebCore/loader/PingLoader.cpp:224
> +        options.shouldOmitUserAgent = ShouldOmitUserAgent::Yes;

This isn't in the spec you mentioned above.  I think this might be the wrong place for it.

> Source/WebCore/loader/PingLoader.h:46
>  enum class ViolationReportType {

: uint8_t

> Source/WebCore/loader/ReportingEndpointsCache.cpp:37
> +ReportingEndpointsCache& ReportingEndpointsCache::singleton()

Is there a reason this needs to be a singleton instead of owned by an object such as the DocumentLoader?

> Source/WebCore/loader/ReportingEndpointsCache.cpp:49
> +    auto securityOrigin = SecurityOrigin::create(response.url());

Are you partitioning this cache by SecurityOrigin of the URLs being requested?  That seems strange.

> Source/WebCore/loader/ResourceLoaderOptions.h:229
> +    ShouldOmitUserAgent shouldOmitUserAgent : bitWidthOfLoadedFromOpaqueSource;

make bitWidthOfShouldOmitUserAgent.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:418
>      ASSERT_UNUSED(loader, loader);

This is no longer unused.

[Darin Adler]

Comment on attachment 437872 [details]
Patch

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

> Source/WebCore/loader/CrossOriginOpenerPolicy.cpp:41
> +static URL sanitizeReferrerForURLReport(const URL& referrer)

Consider having this return String instead of URL?

> Source/WebCore/loader/CrossOriginOpenerPolicy.h:111
> +String crossOriginOpenerPolicyValueToString(CrossOriginOpenerPolicyValue);

Can this return ASCIILiteral instead of String?

> Source/WebCore/loader/ReportingEndpointsCache.h:59
> +    struct EndPoint {
> +        EndPoint() = default;
> +        EndPoint(URL&&, Seconds maxAge);
> +
> +        bool hasExpired() const;
> +
> +        URL url;
> +        MonotonicTime addTime;
> +        Seconds maxAge;
> +    };

Seems like if we move the constructor and destructor to the .cpp file then we can move struct EndPoint to the .cpp file and just forward declare it here. And remove most of the includes in this header too.

Also, I suggest the name Endpoint, rather than EndPoint, since it is a single word grammatically.

>> Source/WebCore/loader/ResourceLoaderOptions.h:229
>> +    ShouldOmitUserAgent shouldOmitUserAgent : bitWidthOfLoadedFromOpaqueSource;
> 
> make bitWidthOfShouldOmitUserAgent.

bitWidthOfLoadedFromOpaqueSource seems wrong.

bitWidthOfShouldOmitUserAgent? 1?

[Chris Dumez]

Comment on attachment 437872 [details]
Patch

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

>> Source/WebCore/loader/CrossOriginOpenerPolicy.h:111
>> +String crossOriginOpenerPolicyValueToString(CrossOriginOpenerPolicyValue);
> 
> Can this return ASCIILiteral instead of String?

OK

>> Source/WebCore/loader/DocumentLoader.cpp:76
>> +#include "PageGroup.h"
> 
> I think it would be nicer to use something like Page::forEachPage instead of PageGroup.  Right now every WKWebView is in the same PageGroup unless you use SPI which I'm trying to remove.

OK, I guess I'll rely on Page::nonUtilityPageCount() for now and add a FIXME comment to indicate we need to support the concept as browsing context group.

>> Source/WebCore/loader/PingLoader.cpp:224
>> +        options.shouldOmitUserAgent = ShouldOmitUserAgent::Yes;
> 
> This isn't in the spec you mentioned above.  I think this might be the wrong place for it.

I will look more into this. Basically the issue is that the WPT test fail if the violation reports request contain a User-Agent header. The reason is that a CORS-preflight happens and LayoutTests/imported/w3c/web-platform-tests//reporting/resources/report.py only allows the Content-Type header during preflighting:
```
  # Handle CORS preflight requests
  if request.method == u'OPTIONS':
    # Always reject preflights for one subdomain
    if b"www2" in request.headers[b"Origin"]:
      return (400, [], u"CORS preflight rejected for www2")
    return [
      (b"Content-Type", b"text/plain"),
      (b"Access-Control-Allow-Origin", b"*"),
      (b"Access-Control-Allow-Methods", b"post"),
      (b"Access-Control-Allow-Headers", b"Content-Type"),
    ], u"CORS allowed"
```

>> Source/WebCore/loader/PingLoader.h:46
>>  enum class ViolationReportType {
> 
> : uint8_t

OK

>> Source/WebCore/loader/ReportingEndpointsCache.cpp:37
>> +ReportingEndpointsCache& ReportingEndpointsCache::singleton()
> 
> Is there a reason this needs to be a singleton instead of owned by an object such as the DocumentLoader?

The cache needs to be shared, otherwise it isn't really a cache. Per the specification (https://www.w3.org/TR/reporting/#concept-storage), it is a global browser cache which is separated by origin.

>> Source/WebCore/loader/ReportingEndpointsCache.cpp:49
>> +    auto securityOrigin = SecurityOrigin::create(response.url());
> 
> Are you partitioning this cache by SecurityOrigin of the URLs being requested?  That seems strange.

Browsing context group switches only happen when navigating the top frame. As such, the COOP/COEP headers and the Report-To header only apply to responses to the main resource of the top frame.
The origin definitely needs to be the origin of the response URL, I don't see how it can work otherwise. The origin of the response is going to be the origin of the topDocument as soon as we commit the navigation. The Report-To header applies to reports related to the site we are loading now, not reports for the site that we navigated away from.

>> Source/WebCore/loader/ReportingEndpointsCache.h:59
>> +    };
> 
> Seems like if we move the constructor and destructor to the .cpp file then we can move struct EndPoint to the .cpp file and just forward declare it here. And remove most of the includes in this header too.
> 
> Also, I suggest the name Endpoint, rather than EndPoint, since it is a single word grammatically.

OK.

>>> Source/WebCore/loader/ResourceLoaderOptions.h:229
>>> +    ShouldOmitUserAgent shouldOmitUserAgent : bitWidthOfLoadedFromOpaqueSource;
>> 
>> make bitWidthOfShouldOmitUserAgent.
> 
> bitWidthOfLoadedFromOpaqueSource seems wrong.
> 
> bitWidthOfShouldOmitUserAgent? 1?

Yes, will fix.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:418
>>      ASSERT_UNUSED(loader, loader);
> 
> This is no longer unused.

OK.

[Chris Dumez]

Created attachment 437907 [details]
Patch

[Chris Dumez]

> >> Source/WebCore/loader/ReportingEndpointsCache.cpp:37
> >> +ReportingEndpointsCache& ReportingEndpointsCache::singleton()
> > 
> > Is there a reason this needs to be a singleton instead of owned by an object such as the DocumentLoader?
> 
> The cache needs to be shared, otherwise it isn't really a cache. Per the
> specification (https://www.w3.org/TR/reporting/#concept-storage), it is a
> global browser cache which is separated by origin.
> 
> >> Source/WebCore/loader/ReportingEndpointsCache.cpp:49
> >> +    auto securityOrigin = SecurityOrigin::create(response.url());
> > 
> > Are you partitioning this cache by SecurityOrigin of the URLs being requested?  That seems strange.
> 
> Browsing context group switches only happen when navigating the top frame.
> As such, the COOP/COEP headers and the Report-To header only apply to
> responses to the main resource of the top frame.
> The origin definitely needs to be the origin of the response URL, I don't
> see how it can work otherwise. The origin of the response is going to be the
> origin of the topDocument as soon as we commit the navigation. The Report-To
> header applies to reports related to the site we are loading now, not
> reports for the site that we navigated away from.

From the spec (https://www.w3.org/TR/reporting/#process-header):
"""
Given a response (response) and a request (request), this algorithm extracts a list of endpoints and endpoint groups for the request’s origin, and updates the reporting cache accordingly.
"""

So I believe I should use the request's origin.

[Chris Dumez]

Created attachment 437915 [details]
Patch

[Chris Dumez]

Created attachment 437923 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82999796>

[EWS]

Committed r282305 (241577@main): <https://commits.webkit.org/241577@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437923 [details].