Bug 185412

Created attachment 340128 [details] Patch

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.

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?

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?

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

Comment on attachment 340128 [details] Patch Clearing flags on attachment: 340128 Committed r231692: <https://trac.webkit.org/changeset/231692>

All reviewed patches have been landed. Closing bug.

<rdar://problem/40154076>