Bug 58639 - CSP report-uri is missing
Summary: CSP report-uri is missing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 59103
Blocks: 53572
  Show dependency treegraph
 
Reported: 2011-04-15 00:21 PDT by Adam Barth
Modified: 2011-04-21 09:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (20.32 KB, patch)
2011-04-20 16:11 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2011-04-20 16:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (21.34 KB, patch)
2011-04-20 16:24 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2011-04-20 16:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (24.59 KB, patch)
2011-04-20 17:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-04-15 00:21:46 PDT
CSP report-uri is missing
Comment 1 Adam Barth 2011-04-20 16:11:50 PDT
Created attachment 90434 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-20 16:15:48 PDT
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.
Comment 3 Adam Barth 2011-04-20 16:16:32 PDT
Created attachment 90435 [details]
Patch
Comment 4 Adam Barth 2011-04-20 16:24:13 PDT
Created attachment 90438 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-04-20 16:31:04 PDT
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.
Comment 6 Adam Barth 2011-04-20 16:48:27 PDT
Created attachment 90446 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-04-20 17:02:26 PDT
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)
Comment 8 Adam Barth 2011-04-20 17:09:53 PDT
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.
Comment 9 Adam Barth 2011-04-20 17:16:36 PDT
Created attachment 90451 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-04-20 17:23:42 PDT
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?
Comment 11 WebKit Commit Bot 2011-04-21 01:35:16 PDT
Comment on attachment 90451 [details]
Patch

Clearing flags on attachment: 90451

Committed r84478: <http://trac.webkit.org/changeset/84478>
Comment 12 WebKit Commit Bot 2011-04-21 01:35:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2011-04-21 02:23:10 PDT
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
Comment 14 Adam Barth 2011-04-21 08:13:55 PDT
PHP magic quotes :(
Comment 15 Adam Barth 2011-04-21 09:05:55 PDT
Hopefully fixed in http://trac.webkit.org/changeset/84502