Bug 224745 – Adhere to policy inheritance according to policy container

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antonio Sartori]

This is an implementation bug tracking this PR on the html spec: https://github.com/whatwg/html/pull/6504

The PR introduces the concept of a policy container, and uses it to clarify inheritance to local schemes for Content Security Policy (as a first step).

It is likely that WebKit needs minor changes to adhere to that PR (see also the tests https://wpt.fyi/results/content-security-policy/inheritance?label=experimental&label=master&aligned). More work could be needed as we add more policies to the policy container.

[Radar WebKit Bug Importer]

<rdar://problem/77144735>

[Ryan Reno]

Created attachment 462406 [details]
Patch

[Ryan Reno]

Created attachment 462408 [details]
Patch

[Ryan Reno]

Trying to make GTK bot happy. I thought having an expectation in gtk-wk2 would solve it but maybe the glib platform expectation is causing the problem. I removed the expectations so hopefully gtk bot just uses the default expected file.

[Ryan Reno]

Created attachment 462430 [details]
Patch

[Brent Fulgham]

Comment on attachment 462430 [details]
Patch

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

> Source/WebCore/dom/SecurityContext.cpp:170
> +    // the policy defined elsewhere.

Is it a mistake for a caller to pass the empty string here? Should it be an ASSERT so we catch any logic errors related to it?

Based on this description, it seems like once we set a referrer policy, we never want to change the sate of the SecurityContext to indicate that we should use a policy defined elsewhere. Is that correct?

[Brent Fulgham]

It looks like the patch is stale, such that the bots can't apply it cleanly. Can you try uploading a rebased patch?

"
patching file Source/WebCore/loader/DocumentLoader.cpp
Hunk #1 FAILED at 1237.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/loader/DocumentLoader.cpp.rej
"

[Ryan Reno]

Created attachment 462431 [details]
Patch

[Ryan Reno]

Comment on attachment 462430 [details]
Patch

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

>> Source/WebCore/dom/SecurityContext.cpp:170
>> +    // the policy defined elsewhere.
> 
> Is it a mistake for a caller to pass the empty string here? Should it be an ASSERT so we catch any logic errors related to it?
> 
> Based on this description, it seems like once we set a referrer policy, we never want to change the sate of the SecurityContext to indicate that we should use a policy defined elsewhere. Is that correct?

I don't know exactly. I simply moved the current implementation from Document.cpp to here as part of the refactoring for the PolicyContainer. I suspect, based on the description and implementation, that it's not an error to try to set empty string as that's the default value for an element when the attribute isn't set (From the spec example here: https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-empty-string).

Since empty string is "look elsewhere" and in the case of an already set policy we don't want to look elsewhere, it seems reasonable to reject that change. I don't have an opinion on whether or not silently discarding is the right move though.

[Ryan Reno]

I guess I missed one line of whitespace in the expectation file :(

Going to let the mac and ios bots finish to be sure I didn't miss anything else before uploading the (hopefully) final patch.

[Ryan Reno]

crashes in mac-wk2 are unrelated and will be fixed by https://bugs.webkit.org/show_bug.cgi?id=245380

[Ryan Reno]

Created attachment 462457 [details]
Patch

[Ryan Reno]

Created attachment 462458 [details]
Patch

[Chris Dumez]

Comment on attachment 462458 [details]
Patch

r=me assuming the bots are happy.

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/sandbox-toggle-in-inactive-document-crash.html

[EWS]

Found 2 new test failures: http/tests/media/fairplay/fps-hls-update-reject.html, imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/navigation-reporting/reporting-coop-navigated-popup.https.html

[Brent Fulgham]

Comment on attachment 462458 [details]
Patch

The crash trace appears in other bot output for other patches, and is does not seem to be caused by this patch.

[Brent Fulgham]

Comment on attachment 462458 [details]
Patch

Readding cq+ since the crash is incorrectly attributed to this patch.

[EWS]

Committed 254679@main (c5a36846f196): <https://commits.webkit.org/254679@main>

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