[webkit-reviews] review canceled: [Bug 58639] CSP report-uri is missing : [Attachment 90446] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has canceled Adam Barth <abarth at webkit.org>'s
request for review:
Bug 58639: CSP report-uri is missing
https://bugs.webkit.org/show_bug.cgi?id=58639

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90446&action=review

>>
LayoutTests/http/tests/security/contentSecurityPolicy/resources/save-report.php
:14
>> +rename("csp-report.txt.tmp", "csp-report.txt");
> 
> This is disgusting.  Couldn't we at least include the pid or something to
make it possible to run more than one copy of the layout tests at once?
> 
> This is going to break in NRWT.

You already can't run two HTTP servers at once.  This doesn't break NRWT
because NRWT runs each directory in sequence.  We already have a bunch of tests
that use this model, including the ping tests.

>> Source/WebCore/loader/PingLoader.cpp:104
>> +	if (!SecurityOrigin::shouldHideReferrer(reportURL,
frame->loader()->outgoingReferrer()))
>> +	    request.setHTTPReferrer(frame->loader()->outgoingReferrer());
> 
> This seems fragile. :(

Yep.  Originally, this pattern was only need in one place, but over time, lots
of code became responsible for setting the Referer.  We should centralize it
again.

>> Source/WebCore/loader/PingLoader.cpp:108
>> +	PingLoader* leakedPingLoader = pingLoader.leakPtr();
> 
> Why even put it in a local?  You can just cast the result to void to avoid an
unused result error?

I believe this is needed because of an unused result warning.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:503
>> +	frame->domWindow()->console()->addMessage(JSMessageSource,
LogMessageType, ErrorMessageLevel, consoleMessage, 1, String());
> 
> I thought you had some fancy message reporting thing now?

This is the fancy message reporting thing.  Someone needs to call addMessage.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:508
>> +	// We need to be careful here when deciding what information to send to
the
> 
> You might consider making this a stronger warning.

In the end, we're going to hash this stuff out in the working group.  I'm not
sure what a stronger warning here would buy us.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:515
>> +	// sent explicitly. As for which directive was violated, that's pretty
> 
> I would remove the "pretty" here. :)

pretty => mostly ?

>> Source/WebCore/page/ContentSecurityPolicy.cpp:741
>> +	    m_scriptSrc = adoptPtr(new CSPDirective(name, value,
m_document->securityOrigin()));
> 
> Do you want name to be case sensitive here?  I would probably have just used
the static ones you have, but I"m not sure it matters.

I want to report the actual name used by the site in case the developer does a
case-sensitive grep of their source code to figure out what went wrong.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:749
>> +	    m_styleSrc = adoptPtr(new CSPDirective(name, value,
m_document->securityOrigin()));
> 
> These are starting to look like we want a helper function to make these. 
createDirective(name, value)

Ok.