Bug 185412 – Use PlatformStrategies to switch between WebKit and WebKitLegacy checking of CSP frame-ancestors and X-Frame-Options

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Bates]

Following up on Ryosuke Niwa's remark in bug 185410, comment 3, we should look to use PlatformStrategies to switch between WebKit and WebKitLegacy checking of CSP frame-ancestors and X-Frame-Options instead of a setting.

[Daniel Bates]

Created attachment 340128 [details]
Patch

[Daniel Bates]

In addition to using loader strategy to toggle whether we perform CSP frame-ancestors and X-Frame-Options checking in DocumentLoader this patch represents a step towards removing duplication and making it clear when we depend on the loader strategy to perform security checks for a request and when we expect those checks to have been performed on a ResourceResponse. We should look to remove the need to query the loader strategy for this information as it is error prone. In particular, LoaderStrategy::havePerformedSecurityChecks() is error prone as it must be kept up-to-date with the kinds of loads performed by NetworkProcess.


From talking with Andy Estes and Brady Eidson today (05/10) we felt it is that it would be less error prone to encode whether a response had been security checked in the ResourceResponse object itself as opposed to indirectly determining this fact by having LoaderStrategy::havePerformedSecurityChecks() look at ResourceResponse::Source. The NetworkProcess loading code would be responsible for setting this state on each ResourceResponse it sends to WebContent process.

[Daniel Bates]

Comment on attachment 340128 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:684
> +    case ResourceResponse::Source::Unknown:

Should we ASSERT_NOT_REACHED() for Source::Unknown?

[Ryosuke Niwa]

Comment on attachment 340128 [details]
Patch

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

> Source/WebCore/loader/LoaderStrategy.h:84
> +    virtual bool shouldPerformSecurityChecks() const { return false; }

This doesn't tell us what kind of security checks we're performing.
How about shouldRestrictedHTTPResponseAccess instead?
That would be very descriptive of the feature we're conditioning.

> Source/WebCore/loader/LoaderStrategy.h:85
> +    virtual bool havePerformedSecurityChecks(const ResourceResponse&) const { return false; }

And how about haveRestrictedHTTPResponseAccess for this one?

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 340128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340128&action=review
> 
> > Source/WebCore/loader/LoaderStrategy.h:84
> > +    virtual bool shouldPerformSecurityChecks() const { return false; }
> 
> This doesn't tell us what kind of security checks we're performing.

I am not happy that we need this function and I am also not happy with the name of this function for the same reason.

> How about shouldRestrictedHTTPResponseAccess instead?

I thought about this name. I ruled it out because it does not convey to the reader the implications of "restricted HTTP response access" in the same sense that a function named "canAccessDocument" or "allowFrameAncestors" does. Notice that RuntimeEnabledFeatures::restrictedHTTPResponseAccess() is enabled in Legacy WebKit by <https://trac.webkit.org/browser/trunk/Source/WebCore/page/RuntimeEnabledFeatures.h?rev=231501#L394>. Maybe this is bug? As a result, the implications of "restricted HTTP response access" are even less clear because it is enabled in WebKit Legacy and conditionally enabled in WebKit. For now, I will defer considering such a renaming until we address whether RuntimeEnabledFeatures::restrictedHTTPResponseAccess() being enabled in Legacy WebKit is intentional.

> That would be very descriptive of the feature we're conditioning.
> 
> > Source/WebCore/loader/LoaderStrategy.h:85
> > +    virtual bool havePerformedSecurityChecks(const ResourceResponse&) const { return false; }
> 
> And how about haveRestrictedHTTPResponseAccess for this one?

Similarly, I will defer considering such a renaming until we address whether RuntimeEnabledFeatures::restrictedHTTPResponseAccess() being enabled in Legacy WebKit is intentional.

[Daniel Bates]

Comment on attachment 340128 [details]
Patch

Clearing flags on attachment: 340128

Committed r231692: <https://trac.webkit.org/changeset/231692>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40154076>