Closed Bug 1181683 Opened 4 years ago Closed 4 years ago
Mixed-content Ping (TYPE
_PING) and Beacon (TYPE _BEACON) requests should be blocked
See http://www.w3.org/TR/mixed-content/#category-optionally-blockable. 'ping' and 'beacon' are not listed as optionally-blockable, so they must always be blocked when they would result in mixed content requests.
When we implemented MCB, ping was disabled. Now its enabled and we also have beacon. cc'ing Richard. Richard, can we block these by default? I have telemetry on how much mixed passive content is on the web (~1.4% of all page loads) but don't know how much of that is a result of ping vs beacon vs image vs audio/video.
When we add new features to the browser, we should make them blockable mixed content instead of optionally blockable. Since its a new feature, there should be no existing web compat issues. I'm hoping this won't create too much of an issue for beacon and ping. Both will likely get blocked without a way for the user to override them though, because they happen when the user clicks a link. Once the link is clicked, the user is navigating to the next page and will miss the override UI. Beacon and ping requests will just start failing, which will hurt websites that have mixed content but won't hinder usability. Our telemetry data shows users hardly use the override anyway.
Attachment #8640093 - Flags: review?(bugs)
This seems like a very good idea. Given that ping/sendBeacon was a replacement for XHR cases and XHR is blocked as mixed content, it follows that ping/sendBeacon should be MCB'ed. I would be slightly happier if we had direct telemetry on how much MCB is ping/sendBeacon, but given that this isn't user-visible, I say full speed ahead.
Attachment #8640093 - Flags: review?(bugs) → review?(mozilla)
Comment on attachment 8640093 [details] [diff] [review] Bug1181683.patch Review of attachment 8640093 [details] [diff] [review]: ----------------------------------------------------------------- ship it, r=me
Attachment #8640093 - Flags: review?(mozilla) → review+
Try shows that there is a ping test that fails with this change (https://mxr.mozilla.org/mozilla-central/source/dom/html/test/test_anchor_ping.html). The reason is that the test tries to ping http://localhost from a secure origin. I've tried updating the test to use a different domain than http://localhost, but the problem is that I need an https domain that supports a port over 1024, which we don't have: mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt Alternatively, I could make the origin http and run the test; that way http://localhost won't get blocked. Unfortunately, this causes other problems since the test attempts to test what happens when using an encrypted connection: https://mxr.mozilla.org/mozilla-central/source/dom/html/test/test_anchor_ping.html?force=1#130 As a last resort, I could just disable mixed content blocking on this test since it requires http://localhost. I'd much rather find a better work around so our mochitest doesn't have mixed content, but if http://localhost is our only option, then I can do this. needinfo'ing Tim for his advice on how to proceed with this test. Thanks!
It's quite hard to make this work :( To spawn a proper TLS server we'll have to go through some hoops, like the unit tests in security/manager/ssl do. We could ping a .sjs but then there's no shared state and I wouldn't know how to communicate back that we received the ping, other than dirty tricks like writing to a file and waiting for it to appear, or opening a TCPServerSocket waiting for a connection. Disabling MCB is a poor solution, but also the easiest. Our test suites lack support for simple TLS servers unfortunately.
Updated patch to disabled mixed content blocking for the test_anchor_ping mochitest. Original try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffbd7f64289b New try for just the failing oth test - https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ea36d83f99b
Attachment #8642569 - Flags: review?(ttaubert)
Comment on attachment 8642569 [details] [diff] [review] Bug1181683-test_anchor_ping.patch Review of attachment 8642569 [details] [diff] [review]: ----------------------------------------------------------------- Might be worth doing it only for the one subtest.
Attachment #8642569 - Flags: review?(ttaubert) → review+
You need to log in before you can comment on or make changes to this bug.