Bug 34436

Summary: Change XSSAuditor block syntax
Product: WebKit Reporter: Adam Barth <abarth>
Component: FramesAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Adam Barth 2010-02-01 10:03:27 PST
Apparently, it would be better if we used the following syntax for the full-page block header for XSSAuditor:

"X-XSS-Protection:" OWS "1" ";" 1*LWS "mode" "=" "block" OWS

in non-ABNF form, that looks like:

X-XSS-Protection: 1; mode=block
Comment 1 Adam Barth 2010-02-01 10:09:47 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.
Comment 2 Adam Barth 2010-02-26 15:03:48 PST
Created attachment 49651 [details]
Patch
Comment 3 Adam Barth 2010-02-26 15:06:55 PST
Created attachment 49653 [details]
Patch
Comment 4 Daniel Bates 2010-02-26 23:46:05 PST
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 5 Adam Barth 2010-02-27 00:28:10 PST
Comment on attachment 49653 [details]
Patch

Ah, that's no good.
Comment 6 Adam Barth 2010-02-27 01:02:16 PST
Created attachment 49678 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-03-15 13:26:24 PDT
This matches some new spec?  How should a reviewer evaluate this patch?
Comment 8 Adam Barth 2010-03-15 13:28:11 PDT
> This matches some new spec?  How should a reviewer evaluate this patch?

It matches what some IE folks asked us to use.
Comment 9 Daniel Bates 2010-03-18 21:17:18 PDT
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
Comment 10 Adam Barth 2010-03-19 15:33:27 PDT
Created attachment 51194 [details]
Patch for landing
Comment 11 Adam Barth 2010-03-19 15:34:33 PDT
> 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 12 WebKit Commit Bot 2010-03-19 20:55:20 PDT
Comment on attachment 51194 [details]
Patch for landing

Clearing flags on attachment: 51194

Committed r56295: <http://trac.webkit.org/changeset/56295>
Comment 13 WebKit Commit Bot 2010-03-19 20:55:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Eric Seidel (no email) 2010-03-31 15:59:22 PDT
http/tests/security/xssAuditor/xss-protection-parsing-01.html has failed on the Tiger bot ever since this commit.  Please fix. :)