Closed Bug 802872 Opened 12 years ago Closed 11 years ago

CSP should restrict EventSource using the connect-src directive

Categories

(Core :: Security, defect, P1)

Product:
Core
Component:
Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: imelven, Assigned: ckerschb)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [CSP 1.0])

Attachments

(1 file, 4 obsolete files)

bug_802872_mochi_test_verifying_csp_restricts_eventsrc_using_the_connectsrc_directive.patch
(deleted), patch
grobinson
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
Bug 667490 modified EventSource to use the same nsIContentPolicy type as XHR we need to make sure this is wired up correctly for the CSP 1.0 implementation at first glance, it seems like this means we need to make TYPE_DATAREQUEST (11) which is the same as TYPE_XMLHTTPREQUEST (11) map to connect-src for a CSP 1.0 compliant policy See also bug 746978 - sync CSP directive parsing and directive names with w3c CSP 1.0 spec
Blocks: csp-w3c-1.0, CSP
This might have been fixed by 667490. Needs to be checked.
Severity: normal → trivial
Priority: -- → P1
QA Contact: sstamm
Whiteboard: [CSP 1.0]
Component: DOM: Core & HTML → Security
it's not working proberly, because onerror is called! Any guesses why?
Attachment #775968 - Flags: review?(dchan+bugzilla)
Attachment #775968 - Flags: review?(dchan+bugzilla)
Attachment #775968 - Flags: review?
Attachment #775968 - Flags: feedback?(dchan+bugzilla)
Assignee: nobody → mozilla
Comment on attachment 775968 [details] [diff] [review] mochi test verifying CSP restricts EventSource using the connect-src directive Review of attachment 775968 [details] [diff] [review]: ----------------------------------------------------------------- EventSource has a very peculiar format. I was able to get the test working after changing handleRequest. Here is what I have function handleRequest(request, response) { dump("--> server called\n\n\n"); response.setHeader("Cache-Control", "no-cache", false); response.setHeader("Content-Type", "text/event-stream", false); var date = new Date(); response.write("data: " + date); response.write("\n\n"); } EventSource expects a Content-Type of text/event-stream. Without it, I was getting an error about Firefox being unable to connect to the server. You can witness this by running the test with mach, opening the webconsole with ctrl/cmd+shift+k and reloading the test page. Responses must start with a known field name. The final "\n\n" is required by the spec. https://developer.mozilla.org/en-US/docs/Server-sent_events/Using_server-sent_events
Attachment #775968 - Flags: feedback?(dchan+bugzilla) → feedback+
Attachment #775968 - Attachment is obsolete: true
Attachment #775968 - Flags: review?
Attachment #776618 - Flags: review?(grobinson)
Confirming that it is wired up correclty by looking at the debug output, which shows that the cspContext is 'connect-src': CSP debug: shouldLoad cspContext = connect-src CSP debug: blocking request for http://example.com/tests/content/base/test/file_CSP_bug802872.sjs
Comment on attachment 776618 [details] [diff] [review] updated mochi test verifying CSP restricts EventSource using the connect-src directive Review of attachment 776618 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/test_CSP_bug802872.html @@ +22,5 @@ > + * Putting the logic for error checking into this file makes us do > + * assumptions about when we receive callbacks (onmessage, onerror). > + * Therefore we load file_CSP_bug802872.html into an iframe, wait > + * for 1000ms (setInterval()) and check the results after that waiting > + * period. Sorry for this very disruptive comment, but have you read about promises? https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise.jsm A promise is basically a promise to do some work. When the work is done, then the promise resolves. In this case, the promise could resolve when the callback is complete. This will let you avoid race conditions in case sleeping for 1000ms is not sufficient, and also lets you *not* sleep and just do the work when it is ready.
Oh, and in light of our seceng review discussion, please take the above comment as advisory and not necessarily as a request to rewrite the entire test.
Comment on attachment 776618 [details] [diff] [review] updated mochi test verifying CSP restricts EventSource using the connect-src directive Review of attachment 776618 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks pretty good. I do think that you could improve this test by using a proper async primitive like Promises, as mmc suggested. The use of setInterval should be avoided wherever possible to avoid race conditions/unnecessary blocking, and the subsequent contortions for async make the code hard to follow. ::: content/base/test/Makefile.in @@ +641,5 @@ > + test_CSP_bug802872.html \ > + file_CSP_bug802872.html \ > + file_CSP_bug802872.html^headers^ \ > + file_CSP_bug802872.js \ > + file_CSP_bug802872.sjs \ Bitrotted. ::: content/base/test/file_CSP_bug802872.html @@ +2,5 @@ > +<html> > +<head> > + <title>Bug 802872</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> Add a comment explaining that you included these because you use addLoadEvent in file_CSP_bug802872.js. @@ +10,5 @@ > +<div id="event_allowed"></div> > +<div id="event_blocked"></div> > + > +<script src='file_CSP_bug802872.js'></script> > + Nit: unnecessary whitespace. ::: content/base/test/file_CSP_bug802872.js @@ +13,5 @@ > + } > +} > + > +function createBlockedEvent() { > + var src_event = new EventSource("http://example.com/tests/content/base/test/file_CSP_bug802872.sjs"); Include a comment above this line and the similar line in createAllowedEvent explaining why this is simulating allowed/blocked behavior in relation to the CSP header. It is clear to me, but probably wouldn't be to any future reviewers. ::: content/base/test/file_CSP_bug802872.sjs @@ +1,1 @@ > + nit: unnecessary whitespace ::: content/base/test/test_CSP_bug802872.html @@ +58,5 @@ > + > +var steps = [ > +function() { > + ok(true, "Testing EventSource allowed by CSP!"); > + document.getElementById('eventframe').src = 'file_CSP_bug802872.html'; I would put this in the anonymous function in SpecialPowers.pushPrefEnv. It messes up the abstraction that I think you're trying to achieve - that this is a list of interchangable, independent steps. @@ +66,5 @@ > + ok(true, "Testing EventSource blocked by CSP!"); > + checkBlocked(); > +}, > +function () { > + ok(true, "All tests finished!\n"); Unnecessary - all tests implicitly finish if they all pass and then SimpleTest.finish() is called. @@ +68,5 @@ > +}, > +function () { > + ok(true, "All tests finished!\n"); > + SimpleTest.finish(); > +} Nit: indent the elements of the list @@ +72,5 @@ > +} > +]; > + > +function next() { > + ok(true, "Begin!"); As in your other code, functions like ok() and is() are for unit tests. If you want to log something to the console for debugging, use dump() (and remove it from the final patch).
Attachment #776618 - Flags: review?(grobinson)
replaced the setinterval with a callback and addressed your other concerns and nits!
Attachment #776618 - Attachment is obsolete: true
Attachment #789147 - Flags: review?(grobinson)
Comment on attachment 789147 [details] [diff] [review] bug_802872_v2_mochi_test_verifying_csp_restricts_eventsrc_using_the_connectsrc_directive.patch Review of attachment 789147 [details] [diff] [review]: ----------------------------------------------------------------- Dispatching and listening for custom events seems like a neat solution to the problem of testing async features such as EventSource. Why are you still setting innerHTML in file_CSP_bug802872.js and then checking it in test_CSP_bug802872.html? You should use the same method for all of the test cases. ::: content/base/test/file_CSP_bug802872.html @@ +9,5 @@ > + > +<div id="event_allowed"></div> > +<div id="event_blocked"></div> > +<script src='file_CSP_bug802872.js'></script> > + Unnecessary newline. Might be nice to indent this properly as well. ::: content/base/test/file_CSP_bug802872.js @@ +1,1 @@ > + Nit: unnecessary newline. @@ +4,5 @@ > + * Content-Security-Policy: default-src 'self' > + * > + * createAllowedEvent: creates a new EventSource using 'http://mochi.test:8888', > + * which this CSP allows. > + * Nit: trailing whitespace. @@ +11,5 @@ > + * > + */ > + > +function createAllowedEvent() { > + var src_event = new EventSource("http://mochi.test:8888/tests/content/base/test/file_CSP_bug802872.sjs"); A comment here (and/or above the similar line in createBlockedEvent) would be to nice to explain why this URL does not violate the policy, but the other does. I think this is non-obvious, because it depends on both understanding CSP and the mechanics of the test server. @@ +16,5 @@ > + > + src_event.onmessage = function(e) { > + src_event.close(); > + document.getElementById("event_allowed").innerHTML = "OK: Callback onMessage triggered for allowed Event!"; > + parent.dispatchEvent(new Event('allowedEventSrcCallback')); If you're sending an event, why also make the changes to innerHTML? Listening for the event is sufficient. ::: content/base/test/file_CSP_bug802872.sjs @@ +1,1 @@ > + Nit: unnecessary newline. ::: content/base/test/test_CSP_bug802872.html @@ +73,5 @@ > +SpecialPowers.pushPrefEnv( > + {'set':[["security.csp.speccompliant", true]]}, > + function () { > + SimpleTest.waitForExplicitFinish(); > + addLoadEvent(next); You don't need to use the next()/steps construction for this test. Just register all of your event handlers in this function and you should be able to handle everything with significantly less code/complexity.
Attachment #789147 - Flags: review?(grobinson)
You were completely right Garrett, the test was too complicated, now it's way clearer. Thanks for pointing that out! I also prepared the patch so someone else can check in the code!
Attachment #789147 - Attachment is obsolete: true
Attachment #789283 - Flags: review?(grobinson)
fixed problem with async calls!
Attachment #789283 - Attachment is obsolete: true
Attachment #789283 - Flags: review?(grobinson)
Attachment #790818 - Flags: review?(grobinson)
Attachment #790818 - Flags: review?(grobinson) → review+
Comment on attachment 790818 [details] [diff] [review] bug_802872_mochi_test_verifying_csp_restricts_eventsrc_using_the_connectsrc_directive.patch This is ready to be checked in!
Attachment #790818 - Flags: checkin+
Comment on attachment 790818 [details] [diff] [review] bug_802872_mochi_test_verifying_csp_restricts_eventsrc_using_the_connectsrc_directive.patch I guess it has to be the '?' in the checkin-box so someone else can check it in!
Attachment #790818 - Flags: checkin+ → checkin?
(In reply to Christoph Kerschbaumer from comment #14) > I guess it has to be the '?' in the checkin-box so someone else can check it > in! Actually, you'll want to set the checkin-needed keyword. :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc27a76fca8 checkin-needed is fine, thanks :)
Flags: in-testsuite+
Keywords: checkin-needed
Attachment #790818 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/1fc27a76fca8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: