Bug 105908 – X-XSS-Protection should log to the console when it sends a report to the server

• Component: New Bugs
• Status: RESOLVED WONTFIX

[WebKit Review Bot]

X-XSS-Protection should log to the console when it sends a report to the server
Requested by abarth on #webkit.

[Adam Barth]

@mkwst: We might want to do something similar for CSP as well.

[Mike West]

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.

[Adam Barth]

Yeah, maybe you're right.

[Mike West]

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

[Thomas Sepez]

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?

[Thomas Sepez]

Created attachment 181070 [details]
Patch.

Patch.  Updates pingloader to catch all kinds of violations should someone decide to add new types down the road.

[Thomas Sepez]

Created attachment 181072 [details]
Patch + addl expected result file.

Add additional affected expected result file.

[Adam Barth]

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?

[Mike West]

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.

[Adam Barth]

Comment on attachment 181072 [details]
Patch + addl expected result file.

OK.  Maybe we shouldn't have the message after all.

[Thomas Sepez]

I'd vote for closing this as resolved wontfix.