X-Frame-Options: SAMEORIGIN needs to check all ancestor frames.
<rdar://problem/40175008>
Mozilla bug: <https://bugzilla.mozilla.org/show_bug.cgi?id=725490>
Chrome issue: <https://bugs.chromium.org/p/chromium/issues/detail?id=250309>
RFC: <https://tools.ietf.org/html/rfc7034>
Created attachment 340230 [details] Patch and layout tests
Attachment 340230 [details] did not pass style-queue: ERROR: Source/WebKit/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/WebCore/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious [changelog/unwantedsecurityterms] [3] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 340230 [details] 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?
Comment on attachment 340230 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=340230&action=review >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:419 >> + if (!SecurityOrigin::create(url)->isSameSchemeHostPort(*m_parameters.sourceOrigin)) > > Do we need to create the SecurityOrigin twice? Oops! Will fix before landing. >> 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? Oops! I hope you do not mind that I rename the loop variable ancestorOrigin to resolve this ambiguity.
Comment on attachment 340230 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=340230&action=review >>> 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? > > Oops! I hope you do not mind that I rename the loop variable ancestorOrigin to resolve this ambiguity. That sounds like a great name!
Created attachment 340239 [details] For EWS
Attachment 340239 [details] did not pass style-queue: ERROR: Source/WebKit/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious [changelog/unwantedsecurityterms] [3] ERROR: Source/WebCore/ChangeLog:11: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious [changelog/unwantedsecurityterms] [3] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r231730: <https://trac.webkit.org/changeset/231730>