Bug 105908 - X-XSS-Protection should log to the console when it sends a report to the server
Summary: X-XSS-Protection should log to the console when it sends a report to the server
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 97978
  Show dependency treegraph
 
Reported: 2013-01-02 00:33 PST by WebKit Review Bot
Modified: 2013-01-02 16:26 PST (History)
5 users (show)

See Also:


Attachments
Patch. (9.64 KB, patch)
2013-01-02 14:38 PST, Thomas Sepez
tsepez: review-
tsepez: commit-queue-
Details | Formatted Diff | Diff
Patch + addl expected result file. (10.70 KB, patch)
2013-01-02 14:44 PST, Thomas Sepez
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2013-01-02 00:33:06 PST
X-XSS-Protection should log to the console when it sends a report to the server
Requested by abarth on #webkit.
Comment 1 Adam Barth 2013-01-02 00:33:47 PST
@mkwst: We might want to do something similar for CSP as well.
Comment 2 Mike West 2013-01-02 01:11:53 PST
I can see a good use case for the XSS auditor to log; it's otherwise completely silent.

What's the use case for CSP to do the same? Generally speaking, it's already generating a console log when a violation occurs.
Comment 3 Adam Barth 2013-01-02 01:22:36 PST
Yeah, maybe you're right.
Comment 4 Mike West 2013-01-02 01:28:45 PST
Relatedly, I'm hoping that adding a DOM event wil address some of the local notification concerns that might have sparked this report. If a developer can subscribe to detailed events, then she's got a good chance of figuring out what's going wrong (or doing her own reporting).
Comment 5 Thomas Sepez 2013-01-02 12:02:43 PST
XSSAuditor isn't completely silent, either.  It spews a "Refused to execute ..." message of its own.
Nonetheless, it might be nice to know that PingLoader::sendViolationReport() has fired.
This is called for both the XSS and CSP cases.  The easiest would be to log in all cases.
Alternatively, there could be a parameter that says whether to log the report, but that seems overzeaolus?
Comment 6 Thomas Sepez 2013-01-02 14:38:28 PST
Created attachment 181070 [details]
Patch.

Patch.  Updates pingloader to catch all kinds of violations should someone decide to add new types down the road.
Comment 7 Thomas Sepez 2013-01-02 14:44:36 PST
Created attachment 181072 [details]
Patch + addl expected result file.

Add additional affected expected result file.
Comment 8 Adam Barth 2013-01-02 15:31:41 PST
Comment on attachment 181072 [details]
Patch + addl expected result file.

This seems useful, but I don't have that strong an opinion.  Mike, what do you think?
Comment 9 Mike West 2013-01-02 15:43:09 PST
Comment on attachment 181072 [details]
Patch + addl expected result file.

View in context: https://bugs.webkit.org/attachment.cgi?id=181072&action=review

Given that we're already generating console messages for each of the cases in which this new message is generated, I'm not sure how valuable this new message is. I do think there's value in informing developers that their reports were sent off correctly, but as-is, this seems like it's simply going to fill the console will messages which don't really give me information I need to act upon.

As a debugging tool, though, it might be valuable. If we drop the message down to DebugMessageLevel, then users can uncheck the debug message box in the console to hide the message. That seems reasonable.

> Source/WebCore/loader/PingLoader.cpp:126
> +    frame->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Violation report sent to " + reportURL.string());

1. Can you add a note here along the lines of; "// FIXME: This message should be moved off the console once a solution to https://bugs.webkit.org/show_bug.cgi?id=103274 exists."?
2. This shouldn't be an Error. I'd suggest DebugMessageLevel instead, as that's closer to the intent.
3. It'd be nice if there was a mechanism to specialize the message for CSP vs XSS vs WhateverHappensNext. "Violation report" is fairly generic.
Comment 10 Adam Barth 2013-01-02 15:49:55 PST
Comment on attachment 181072 [details]
Patch + addl expected result file.

OK.  Maybe we shouldn't have the message after all.
Comment 11 Thomas Sepez 2013-01-02 16:15:17 PST
I'd vote for closing this as resolved wontfix.