François Marier [:francois] Reporter
	Description • 10 years ago

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.

	François Marier [:francois] Reporter
	Comment 1 • 10 years ago

Here's a test page that demonstrates that should and shouldn't be blocked.

	François Marier [:francois] Reporter
	Comment 2 • 10 years ago

This is the code I removed from bug 529697.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 3 • 10 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.

	François Marier [:francois] 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

	Christoph Kerschbaumer [:ckerschb] 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?

	François Marier [:francois] 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?

	Christoph Kerschbaumer [:ckerschb] 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).

	Christoph Kerschbaumer [:ckerschb] 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?

	François Marier [:francois] Reporter
	Comment 9 • 9 years ago

The only thing missing here is to rewrite the test in an e10s-compliant way.

	Christoph Kerschbaumer [:ckerschb] 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

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 11 • 9 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.

	Christoph Kerschbaumer [:ckerschb] 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?

	François Marier [:francois] 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...

	Christoph Kerschbaumer [:ckerschb] 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!

	François Marier [:francois] Reporter
	Comment 15 • 8 years ago

Rebased patch. Tests don't seem to pass on e10s :(

	François Marier [:francois] 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.

	Christoph Kerschbaumer [:ckerschb] 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.

	Christoph Kerschbaumer [:ckerschb] 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

	Stephanie Ouillon [:arroway] (needinfo me)
	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 " ?

	Stephanie Ouillon [:arroway] (needinfo me)
	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.

	Pulsebot
	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

	Carsten Book [:Tomcat]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/74982b5ca182

	Jean-Yves Perrier [:teoli]
	Comment 24 • 8 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