[webkit-reviews] review granted: [Bug 110733] XSSAuditor should send only one console error when blocking a page. : [Attachment 191493] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 110733: XSSAuditor should send only one console error when blocking a page.
https://bugs.webkit.org/show_bug.cgi?id=110733

Attachment 191493: Patch
https://bugs.webkit.org/attachment.cgi?id=191493&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191493&action=review


Thank you Mike for updating the patch. When the URL is long, which is likely
for a GET-based XSS attack, the console message may be long and be difficult to
read. See my remarks (below) for more details. We may want to consider moving
the URL to the end of the sentence similar the proposed messages strings in
comment 8. Regardless, the message text proposed in this patch is reasonable.

> Source/WebCore/html/parser/XSSAuditor.cpp:285
> +	   m_didSendValidXSSProtectionHeader = (xssProtectionHeader !=
ContentSecurityPolicy::ReflectedXSSUnset && xssProtectionHeader !=
ContentSecurityPolicy::ReflectedXSSInvalid);

Please remove the outer-most parenthesis as I don't feel they improve the
readability of this line.

> Source/WebCore/html/parser/XSSAuditor.cpp:299
> +	   m_didSendValidCSPHeader =
(document->contentSecurityPolicy()->reflectedXSSDisposition() !=
ContentSecurityPolicy::ReflectedXSSUnset &&
document->contentSecurityPolicy()->reflectedXSSDisposition() !=
ContentSecurityPolicy::ReflectedXSSInvalid);

I suggest we store the value of the expression
"document->contentSecurityPolicy()->reflectedXSSDisposition()" in a local
variable and then reference it twice. This will save dereferencing two pointers
and making a function call as well as improve the readability of this line.
Also, please remove the outer-most parenthesis as I don't feel they improve the
readability of this line.

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:61
> +    message.append("The XSS Auditor has ");

I suggest we remove the word "has".

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:64
> +    message.append(xssInfo.m_originalURL);

The URL will tend to be long.  And a long URL may affect the readability of
this sentence since it breaks up the explanation of what was blocked/refused
and why it was blocked/refused.

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:66
> +    message.append(xssInfo.m_didBlockEntirePage ? " the source code of a
script" : " its source code");

Nit: I suggest we remove the space character from each of the string literals
in this line and append a space character to the end of the string literal on
the previous line (line 65).

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:74
> +	   message.append(" The auditor was implicitly enabled, as the server
did not send an 'X-XSS-Protection' or 'Content-Security-Policy' header to
control its state.");

I wish there was a better way to state this. Maybe:

The auditor was enabled as the server neither sent an 'X-XSS-Protection' nor
'Content-Security-Policy' header.