Closed
Bug 1100181
Opened 9 years ago
Closed 8 years ago
(CSP 1.1) look at connect-src directive when submitting pings
Categories
(Core :: DOM: Security, defect, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: francois, Assigned: ckerschb)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [CSP 1.1], [domsecurity-active])
Attachments
(2 files, 4 obsolete files)
1.35 KB,
text/html
|
Details | |
6.43 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
As per the fetch spec (https://fetch.spec.whatwg.org/#requests), hyperlink auditing, the ping attribute, is covered by the connect-src directive. Pings are currently not covered by any CSP directive in Firefox. Note: pings are disabled by default and need to be enabled via the browser.send_pings pref.
Reporter | ||
Comment 1•9 years ago
|
||
Here's a test page that demonstrates that should and shouldn't be blocked.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [CSP 1.1]
Reporter | ||
Comment 2•9 years ago
|
||
This is the code I removed from bug 529697.
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8523643 [details] [diff] [review] CSP code and mochitest Review of attachment 8523643 [details] [diff] [review]: ----------------------------------------------------------------- Even though the patch and tests looks fine to me I would like to wait till the WG has decided where PING is going to end up. Potentially your patch is correct and it will end up being mapped to 'connect-src', but might as well be mapped to 'form-action'. Let's just wait and land this patch once the WG has settled. Clearing review-reqeust for now.
Attachment #8523643 -
Flags: review?(mozilla)
Reporter | ||
Comment 4•9 years ago
|
||
Here's a rebased version of the patch. The latest spec has moved PING into connect-src: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-connect-src as per https://lists.w3.org/Archives/Public/public-webappsec/2015Jan/0271.html
Attachment #8523643 -
Attachment is obsolete: true
Attachment #8556827 -
Flags: review?(mozilla)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8556827 [details] [diff] [review] Code changes and mochitest Review of attachment 8556827 [details] [diff] [review]: ----------------------------------------------------------------- Hey Francois, thanks for being on top and moving 'ping' into connect-src, that's where it belongs in my opinion anyway. As discussed the other day I would rather not accept any tests that are not working in e10s. Any chance you could rewrite the test so it's not relying on http-on-opening-request?
Attachment #8556827 -
Flags: review?(mozilla)
Reporter | ||
Comment 6•9 years ago
|
||
Chris, I removed the "skip if e10s" bit from the test and also ran it successfully under e10s using: ./mach mochitest --e10s dom/base/test/csp/test_ping.html How can I reproduce the e10s failure you're seeing?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to François Marier [:francois] from comment #6) > Chris, I removed the "skip if e10s" bit from the test and also ran it > successfully under e10s using: > > ./mach mochitest --e10s dom/base/test/csp/test_ping.html > > How can I reproduce the e10s failure you're seeing? I haven't tried to run the test, I just saw that the test was skipped > [test_ping.html] > skip-if = buildapp == 'b2g' # http-on-opening-request observers are not available in child processes If I remember it's causing problems on e10s as well, but if you can run the test successfully on TBPL not skipping any platforms or build configurations it should be ok. Anyway, I would still rather have a test not relying on http-on-opening-request. (I know we still have CSP tests relying on it, but I would really like to get away from that practice).
Flags: needinfo?(mozilla)
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 8•9 years ago
|
||
Francois, we are triaging at the moment. I see you have a patch, what's missing here to get this landed?
Flags: needinfo?(francois)
Reporter | ||
Comment 9•9 years ago
|
||
The only thing missing here is to rewrite the test in an e10s-compliant way.
Flags: needinfo?(francois)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to François Marier [:francois] from comment #9) > The only thing missing here is to rewrite the test in an e10s-compliant way. What you could do instead of using http-notify-request is an approach where you send the beacon request to an *.sjs server and fetch up the result using an xhr request. Similar do what we do here [1]. Make sure to process the request on the sever async, so we don't end up having any kind of race condition. [1] http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/beacon/test_beaconOriginHeader.html?force=1#37
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10) > (In reply to François Marier [:francois] from comment #9) > > The only thing missing here is to rewrite the test in an e10s-compliant way. > > What you could do instead of using http-notify-request is an approach where > you send the beacon request to an *.sjs server and fetch up the result using > an xhr request. Similar do what we do here [1]. Make sure to process the > request on the sever async, so we don't end up having any kind of race > condition. > > [1] > http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/beacon/ > test_beaconOriginHeader.html?force=1#37 PING is governed by connect-src in CSP Level 3, see: https://w3c.github.io/webappsec-csp/#directive-connect-src Hey Francois, as discussed in the triage meeting we can land that patch now. Feel free to flag me for review once the test works on all platforms.
Flags: needinfo?(francois)
Assignee | ||
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(francois)
Assignee | ||
Comment 12•8 years ago
|
||
Hey Francois, any chance you wanna land this? I suppose we can just remove the B2G specific flag from the mochitest.ini and land as is, right?
Flags: needinfo?(francois)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [CSP 1.1] → [CSP 1.1], [domsecurity-backlog]
Reporter | ||
Comment 13•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12) > Hey Francois, any chance you wanna land this? I suppose we can just remove > the B2G specific flag from the mochitest.ini and land as is, right? I've rebased my patch and I'll test it to see if the tests still pass. Any reason why we shouldn't just like the "disable if B2G" thing in there? I mean we may as well not break the B2G tests on purpose...
Flags: needinfo?(francois) → needinfo?(mozilla)
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to François Marier [:francois] from comment #13) > Any reason why we shouldn't just like the "disable if B2G" thing in there? I > mean we may as well not break the B2G tests on purpose... I thought that flag only applies to Treeherder. But yeah, sure, leave it in (I suppose). You could ping Paul to make sure!
Flags: needinfo?(mozilla)
Reporter | ||
Comment 15•8 years ago
|
||
Rebased patch. Tests don't seem to pass on e10s :(
Attachment #8556827 -
Attachment is obsolete: true
Reporter | ||
Comment 16•8 years ago
|
||
I'm going to unassign myself. I've been sitting on this for far too long and it could be a good first bug for a contributor given that this is just missing a working test.
Assignee: francois → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to François Marier [:francois] from comment #16) > I'm going to unassign myself. I've been sitting on this for far too long and > it could be a good first bug for a contributor given that this is just > missing a working test. Fair enough. Adding tpe-seceng to the whiteboard.
Whiteboard: [CSP 1.1], [domsecurity-backlog] → [CSP 1.1], [domsecurity-backlog], tpe-seceng
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ckerschb
Priority: P3 → P2
Whiteboard: [CSP 1.1], [domsecurity-backlog], tpe-seceng → [CSP 1.1], [domsecurity-active]
Assignee | ||
Comment 18•8 years ago
|
||
Stephanie, pings should be governed by connect-src according to the latest spec: https://www.w3.org/TR/CSP3/#directive-connect-src
Attachment #8733983 -
Attachment is obsolete: true
Attachment #8763680 -
Flags: review?(stephouillon)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 19•8 years ago
|
||
Comment on attachment 8763680 [details] [diff] [review] bug_1100181_csp_ping_connect_src.patch Hi, putting r+, I've just have two small comments: >diff --git a/dom/security/test/csp/file_ping.html b/dom/security/test/csp/file_ping.html >new file mode 100644 >--- /dev/null >+++ b/dom/security/test/csp/file_ping.html >@@ -0,0 +1,19 @@ >+<!DOCTYPE HTML> >+<html> >+<head> >+ <title>Bug 1100181 - CSP: Enforce connect-src when submitting pings</title> >+</head> >+<body> >+<!-- we are using an image for the test, but can be anything --> >+<a id="testlink" >+ href="http://mochi.test:8888/tests/image/test/mochitest/blue.png" >+ ping="http://mochi.test:8888/tests/image/test/mochitest/blue.png?send-ping" >+>Send ping</a> >+ >+<script type="text/javascript"> >+ var link = document.getElementById("testlink"); >+ link.click(); >+</script> >+ >+</body> >+</html> The code could be indented inside the <html> and <body> tags; and, the ">" before "Send ping" could be put on the line before. >diff --git a/dom/security/test/csp/test_ping.html b/dom/security/test/csp/test_ping.html >new file mode 100644 >--- /dev/null >+++ b/dom/security/test/csp/test_ping.html >@@ -0,0 +1,103 @@ >+ >+var tests = [ >+ { >+ result : "allowed", >+ policy : "connect-src 'self'" >+ }, >+ { >+ result : "blocked", >+ policy : "connect-src 'none'" >+ } >+]; >+ >+// initializing to -1 so we start at index 0 when we start the test >+var counter = -1; >+ >+function checkResult(aResult) { >+ is(aResult, tests[counter].result, "should be " + tests[counter].result + " in test " + counter + "!"); Isn't there a space missing before "should be " ?
Attachment #8763680 -
Flags: review?(stephouillon) → review+
Comment 20•8 years ago
|
||
(In reply to Stephanie Ouillon [:arroway] from comment #19) > Isn't there a space missing before "should be " ? Forget this, I hit the publish button too quickly.
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8763680 -
Attachment is obsolete: true
Attachment #8764912 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/74982b5ca182 CSP: Enforce connect-src when submitting pings. r=arroway
Keywords: checkin-needed
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74982b5ca182
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 24•7 years ago
|
||
Doc updated: A browser compat note in: https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives#Browser_compatibility and https://developer.mozilla.org/en-US/Firefox/Releases/50#Networking
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•