[webkit-reviews] review granted: [Bug 185410] Check X-Frame-Options and CSP frame-ancestors in network process : [Attachment 339795] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 185410: Check X-Frame-Options and CSP frame-ancestors in network process
https://bugs.webkit.org/show_bug.cgi?id=185410

Attachment 339795: Patch

https://bugs.webkit.org/attachment.cgi?id=339795&action=review




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 339795
  --> https://bugs.webkit.org/attachment.cgi?id=339795
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:770
> +    if
(!m_frame->settings().networkProcessCSPFrameAncestorsCheckingEnabled() ||
!RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess()) {

I think we should add a new method to PlatformStrategies instead of using
settings to behave differently in WK1 and WK2.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:486
> +    if (ancestorOrigins.size() == 1)
> +	   return true; // The top-most frame is always allowed.

Instead of adding a comment like this, why don't we use a local boolean
variable with a descriptive name?
e.g.
bool isNavigatingTopLevelFrame = ancestorOrigins.size() == 1;
if (isNavigatingTopLevelFrame)
    return true;

> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:93
> +	   URL origin { URL { }, (*it)->toString() };

Why are we converting SecurityOrigin > String > URL?
What's the practical implication of this conversion?
I think we should either add a comment here or add a member function with a
descriptive name to SecurityOrigin
referencing
https://w3c.github.io/webappsec-csp/#frame-ancestors-navigation-response

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:407
> +    XFrameOptionsDisposition disposition =
parseXFrameOptionsHeader(xFrameOptions);

Seems like we don't need this local variable. It's only used in switch.