Bug 58639 – CSP report-uri is missing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

CSP report-uri is missing

[Adam Barth]

Created attachment 90434 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 90434 [details]
Patch

I fear this change has privacy concerns.  Can you allay those concerns with some big warning comments in the code explaining what you're sending back to those websites and why?  Maybe also in the test cases.

It's also not clear from your ChangeLog what we're sending here.  You know much more about security/privacy than I, but I want to make this very simple to the casual reader to make sure that we're doing the righ tthing.

[Adam Barth]

Created attachment 90435 [details]
Patch

[Adam Barth]

Created attachment 90438 [details]
Patch

[Eric Seidel (no email)]

Adam mentioned adding two more tests:

1.  that CSP on a parent document did not have any effect on an iframe (specifically we won't report URLs from inside that iframe)
2.  An iframe with a CSP policy, make sure it doesn't ever get any URLs from the parent.

[Adam Barth]

Created attachment 90446 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 90446 [details]
Patch

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.

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

This seems fragile. :(

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

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

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:515
> +    // sent explicitly. As for which directive was violated, that's pretty

I would remove the "pretty" here. :)

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

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

[Adam Barth]

Comment on attachment 90446 [details]
Patch

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.

[Adam Barth]

Created attachment 90451 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 90451 [details]
Patch

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

OK.

> LayoutTests/http/tests/security/contentSecurityPolicy/resources/echo-report.php:3
> +    usleep(10000);

10 seconds?

[WebKit Commit Bot]

Comment on attachment 90451 [details]
Patch

Clearing flags on attachment: 90451

Committed r84478: <http://trac.webkit.org/changeset/84478>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84478 might have broken GTK Linux 32-bit Release
The following tests are not passing:
http/tests/security/contentSecurityPolicy/report-uri-from-child-frame.html
http/tests/security/contentSecurityPolicy/report-uri.html

[Adam Barth]

PHP magic quotes :(

[Adam Barth]

Hopefully fixed in http://trac.webkit.org/changeset/84502