Bug 34436 – Change XSSAuditor block syntax

• Component: Frames
• Status: RESOLVED FIXED

[Adam Barth]

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

[Adam Barth]

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.

[Adam Barth]

Created attachment 49651 [details]
Patch

[Adam Barth]

Created attachment 49653 [details]
Patch

[Daniel Bates]

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.
...

[Adam Barth]

Comment on attachment 49653 [details]
Patch

Ah, that's no good.

[Adam Barth]

Created attachment 49678 [details]
Patch

[Eric Seidel (no email)]

This matches some new spec?  How should a reviewer evaluate this patch?

[Adam Barth]

> This matches some new spec?  How should a reviewer evaluate this patch?

It matches what some IE folks asked us to use.

[Daniel Bates]

Comment on attachment 49678 [details]
Patch


> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,27 @@
> [phone]  Adam Barth  <[e-mail]>
> [...]
> +        * 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

[Adam Barth]

Created attachment 51194 [details]
Patch for landing

[Adam Barth]

> 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.)

[WebKit Commit Bot]

Comment on attachment 51194 [details]
Patch for landing

Clearing flags on attachment: 51194

Committed r56295: <http://trac.webkit.org/changeset/56295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

http/tests/security/xssAuditor/xss-protection-parsing-01.html has failed on the Tiger bot ever since this commit.  Please fix. :)