RESOLVED FIXED 113387
X-Frame-Options: Multiple headers are ignored completely.
https://bugs.webkit.org/show_bug.cgi?id=113387
Summary X-Frame-Options: Multiple headers are ignored completely.
Mike West
Reported 2013-03-27 04:02:11 PDT
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
Attachments
Patch (23.26 KB, patch)
2013-03-27 06:24 PDT, Mike West
no flags
Patch for landing (23.27 KB, patch)
2013-03-28 01:32 PDT, Mike West
no flags
Mike West
Comment 1 2013-03-27 06:15:57 PDT
Mike West
Comment 2 2013-03-27 06:24:52 PDT
Mike West
Comment 3 2013-03-27 10:29:35 PDT
Bots seem happy. Nate, would you mind taking a look at this?
Nate Chapin
Comment 4 2013-03-27 10:39:01 PDT
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().
Mike West
Comment 5 2013-03-28 01:32:45 PDT
Created attachment 195500 [details] Patch for landing
Mike West
Comment 6 2013-03-28 01:33:32 PDT
(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.
WebKit Review Bot
Comment 7 2013-03-28 01:57:16 PDT
Comment on attachment 195500 [details] Patch for landing Clearing flags on attachment: 195500 Committed r147086: <http://trac.webkit.org/changeset/147086>
WebKit Review Bot
Comment 8 2013-03-28 01:57:20 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.