[webkit-reviews] review granted: [Bug 185567] X-Frame-Options: SAMEORIGIN needs to check all ancestor frames : [Attachment 340230] Patch and layout tests

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 185567: X-Frame-Options: SAMEORIGIN needs to check all ancestor frames
https://bugs.webkit.org/show_bug.cgi?id=185567

Attachment 340230: Patch and layout tests

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




--- Comment #7 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 340230
  --> https://bugs.webkit.org/attachment.cgi?id=340230
Patch and layout tests

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

r=me, but I have a few minor complaints about the patch I hope you can resolve
before landing.

> Source/WebCore/loader/FrameLoader.cpp:3423
> +		   return true;

Ha! It's funny it was every written as a break, considering that the loop
wasn't doing anything meaningful! :-)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:419
> +	   if
(!SecurityOrigin::create(url)->isSameSchemeHostPort(*m_parameters.sourceOrigin)
)

Do we need to create the SecurityOrigin twice?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:421
> +	   for (auto& origin : m_parameters.frameAncestorOrigins) {

It's weird to have origin defined in two scopes, even though it apparently
works. Could you name one the "sourceOrigin" or "topLevelOrigin" or something?