http://www.veracode.com/blog/2013/03/security-headers-on-the-top-1000000-websites-march-2013-report/ notes that a small number of sites send multiple 'X-Frame-Options' headers. We're currently ignoring those completely, which probably isn't the safest behavior. Gecko has settled on parsing each header, using the value if they're all the same, and defaulting to 'DENY' if they conflict, which seems like a better solution[1]. I'd suggest that WebKit do the same. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=761655
Also filed downstream as https://code.google.com/p/chromium/issues/detail?id=145659.
Created attachment 195300 [details] Patch
Bots seem happy. Nate, would you mind taking a look at this?
Comment on attachment 195300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195300&action=review One style nit... > Source/WebCore/loader/FrameLoader.cpp:2984 > + default: > + m_frame->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid 'X-Frame-Options' header encountered when loading '" + url.elidedString() + "': '" + content + "' is not a recognized directive. The header will be ignored.", requestIdentifier); > + return false; I think it's more common in WebKit (or at least the parts I frequent) to explicitly state all cases and have the default be ASSERT_NOT_REACHED().
Created attachment 195500 [details] Patch for landing
(In reply to comment #4) > (From update of attachment 195300 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195300&action=review > > One style nit... > > > Source/WebCore/loader/FrameLoader.cpp:2984 > > + default: > > + m_frame->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid 'X-Frame-Options' header encountered when loading '" + url.elidedString() + "': '" + content + "' is not a recognized directive. The header will be ignored.", requestIdentifier); > > + return false; > > I think it's more common in WebKit (or at least the parts I frequent) to explicitly state all cases and have the default be ASSERT_NOT_REACHED(). Thanks Nate. I've taken care of that in the patch up for the CQ.
Comment on attachment 195500 [details] Patch for landing Clearing flags on attachment: 195500 Committed r147086: <http://trac.webkit.org/changeset/147086>
All reviewed patches have been landed. Closing bug.