Bug 230269 – Add violations reporting support for Cross-Origin-Embedder-Policy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Add violations reporting support for Cross-Origin-Embedder-Policy.

[Chris Dumez]

Add to fix a WPT test to run within WebKit infra:
https://github.com/web-platform-tests/wpt/pull/30785

[Chris Dumez]

Created attachment 438174 [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]

Created attachment 438175 [details]
WIP patch

[Chris Dumez]

Created attachment 438176 [details]
WIP patch

[Chris Dumez]

Created attachment 438188 [details]
WIP patch

[Chris Dumez]

Created attachment 438244 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83150069>

[youenn fablet]

Comment on attachment 438244 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:803
> +        return frame.isMainFrame() ? FetchOptions::Destination::Document : FetchOptions::Destination::Iframe;

We could differentiate SVGDocumentResource from MainResource so that only MainResource checks for Document vs. Iframe.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:166
> +        return WTFMove(*error);

Why not just returning error here?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:177
> +            return WTFMove(*error);

Why not just returning error here?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:647
>              return true;

Are we sure this return true is correct?
It seems we should return true if ownerPolicy's value is "unsafe-none" OR policy's value is "require-corp", but we do an if (AND) above.

[Chris Dumez]

Comment on attachment 438244 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:803
>> +        return frame.isMainFrame() ? FetchOptions::Destination::Document : FetchOptions::Destination::Iframe;
> 
> We could differentiate SVGDocumentResource from MainResource so that only MainResource checks for Document vs. Iframe.

Ok, will do.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:166
>> +        return WTFMove(*error);
> 
> Why not just returning error here?

Will update.

>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:177
>> +            return WTFMove(*error);
> 
> Why not just returning error here?

Will update.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:647
>>              return true;
> 
> Are we sure this return true is correct?
> It seems we should return true if ownerPolicy's value is "unsafe-none" OR policy's value is "require-corp", but we do an if (AND) above.

The function is named "shouldInterruptWorkerLoadForCrossOriginEmbedderPolicy" so `return true` means we interrupt the load.

The spec says:
"If ownerPolicy's value is "unsafe-none" or policy's value is "require-corp", then return true."

But this is to determine if we should let the load continue.

Given that our code is about interrupting the load, I believe it should be the opposite:
"If ownerPolicy's value is NOT "unsafe-none" (thus it is "require-corp") AND policy's value is NOT "require-corp" (thus it is "unsafe-none"), then return true."

My code looks correct to me.

[Chris Dumez]

Created attachment 438347 [details]
Patch

[Chris Dumez]

Created attachment 438362 [details]
Patch

[EWS]

Committed r282604 (241765@main): <https://commits.webkit.org/241765@main>

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