Summary: | Change XSSAuditor block syntax | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||
Component: | Frames | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, dbates, eric, sam | ||||||||||
Priority: | P2 | Keywords: | XSSAuditor | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 36933 | ||||||||||||
Attachments: |
|
Description
Adam Barth
2010-02-01 10:03:27 PST
Oops: "X-XSS-Protection:" OWS "1" ";" "mode" "=" "block" OWS Note that this is the "new" ABNF syntax, so the following would be valid: X-XSS-Protection: 1;mode=block X-XSS-Protection: 1 ; mode =block because LWS (spaces and tabs) are allowed between each token. Also the tokens are case insensitive. Created attachment 49651 [details]
Patch
Created attachment 49653 [details]
Patch
In the expected results file "xss-protection-parsing-01-expected.txt" there is a timed out fail message: ... +CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request. + +FAIL: Timed out waiting for notifyDone to be called +This tests our parsing of the X-XSS-Protection header. ... Comment on attachment 49653 [details]
Patch
Ah, that's no good.
Created attachment 49678 [details]
Patch
This matches some new spec? How should a reviewer evaluate this patch? > This matches some new spec? How should a reviewer evaluate this patch?
It matches what some IE folks asked us to use.
Comment on attachment 49678 [details] Patch > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,27 @@ > +2010-02-27 Adam Barth <abarth@webkit.org> > [...] > + * WebCore.xcodeproj/project.pbxproj: This line should be removed from the change log since this patch does not contain any changes to the WebCore Xcode project file. > -bool XSSAuditor::shouldFullPageBlockForXSSProtectionHeader() const > +XSSProtectionDisposition XSSAuditor::xssProtection() const > { > // If we detect an XSS attack and find the HTTP header "X-XSS-Protection: 12" then > // we will stop loading the page as opposed to ignoring the script. The value "12" > @@ -302,9 +302,7 @@ bool XSSAuditor::shouldFullPageBlockForXSSProtectionHeader() const Either this comment needs to removed or it needs to be updated since we are no longer using the "12" notation to do full-page blocking. > +XSSProtectionDisposition parseXSSProtectionHeader(const String& header) > +{ > + String stippedHeader = header.stripWhiteSpace(); > + > + if (stippedHeader.isEmpty()) > + return XSSProtectionEnabled; > + > + if (stippedHeader[0] == '0') > + return XSSProtectionDisabled; > + > + int length = (int)header.length(); Minor style issue, the explicit cast to type integer (i.e. "(int)") is unnecessary. r=me Created attachment 51194 [details]
Patch for landing
> This line should be removed from the change log since this patch does not > contain any changes to the WebCore Xcode project file. Fixed. > Either this comment needs to removed or it needs to be updated since we are no > longer using the "12" notation to do full-page blocking. Removed. > Minor style issue, the explicit cast to type integer (i.e. "(int)") is > unnecessary. I left it in because it's converting from unsigned to signed and other instances in this file do it too. (I think one of the ports might have a warning-as-error in this case.) Comment on attachment 51194 [details] Patch for landing Clearing flags on attachment: 51194 Committed r56295: <http://trac.webkit.org/changeset/56295> All reviewed patches have been landed. Closing bug. http/tests/security/xssAuditor/xss-protection-parsing-01.html has failed on the Tiger bot ever since this commit. Please fix. :) |