Closed Bug 1429432 Opened 6 years ago Closed 5 years ago

Consider making Notifications require SecureContext

Categories

(Core :: DOM: Push Subscriptions, enhancement, P1)

Product:
Core
Component:
DOM: Push Subscriptions
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: jkt, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, sec-want, site-compat, Whiteboard: [adv-main67-])

Attachments

(1 file)

Bug 1429432 - Require Secure Context for Notifications. r=Ehsan
47 bytes, text/x-phabricator-request
Details | Review 
Notifications contribute to annoyances in Firefox (Insecure ISP, Cafe wifi could inject these annoyances etc) and also increase HTTPS adoption.

Chrome has already done this in 62. [1]

[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/n37ij1E_1aY
This should probably live in the DOM component. There's likely a dupe of it already. FWIW, I'm still planning to add telemetry for this soon.
Component: Site Identity and Permission Panels → DOM: Push Notifications
Product: Firefox → Core
If chrome has already done this months ago can't we Just Do It?
Flags: needinfo?(jhofmann)
Keywords: sec-want
(In reply to Daniel Veditz [:dveditz] from comment #2)
> If chrome has already done this months ago can't we Just Do It?

Well, the telemetry mentioned in comment 1 has been added (https://mzl.la/2neZb8z) and it shows that ~7.4% of permission prompts shown for notifications still come from HTTP websites (quite a lot considering none of them work in Chrome). OTOH the fact that none of them work in Chrome makes me think that most of these sites are pretty spammy and turning this off for them might actually be a service to the user.

I'm not 100% sure who maintains notifications on DOM side nowadays, it might be worth having them weigh in here. Anne, Andrew, do you know?
Flags: needinfo?(overholt)
Flags: needinfo?(jhofmann)
Flags: needinfo?(annevk)
As far as I know they don't really have an active owner.
Flags: needinfo?(annevk)
Anne's correct. I agree with comment 3.
Flags: needinfo?(overholt)
I can look into this once the thing that I'm currently doing is done...
Flags: needinfo?(jhofmann)

It would probably be good to get this done in 67 to reduce the noise in our upcoming permission prompt annoyance experiments...

Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
Priority: P2 → P1
Keywords: site-compat
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/279a75b5a6d4
Require Secure Context for Notifications. r=Ehsan
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3e646df6c5e
Backed out changeset 279a75b5a6d4 for failing at test_notification_insecure_context.html on a CLOSED TREE.

Backed out changeset 279a75b5a6d4 (bug 1429432) for failing at test_notification_insecure_context.html on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b3e646df6c5e74f625bae22528ab6a6ff739033f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=279a75b5a6d42a41176750f113594139060d8924&selectedJob=230666485

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230666485&repo=autoland&lineNumber=2114

Log snippet:

[task 2019-02-27T00:54:57.212Z] 00:54:57 INFO - TEST-START | dom/notification/test/mochitest/test_notification_insecure_context.html
[task 2019-02-27T00:54:57.304Z] 00:54:57 INFO - GECKO(1430) | ++DOMWINDOW == 37 (0xdd818800) [pid = 1430] [serial = 37] [outer = 0xe1bb7420]
[task 2019-02-27T00:54:57.425Z] 00:54:57 INFO - TEST-INFO | started process screentopng
[task 2019-02-27T00:54:57.926Z] 00:54:57 INFO - TEST-INFO | screentopng: exit 0
[task 2019-02-27T00:54:57.928Z] 00:54:57 INFO - TEST-UNEXPECTED-FAIL | dom/notification/test/mochitest/test_notification_insecure_context.html | Denied permission in insecure context - got "granted", expected "denied"
[task 2019-02-27T00:54:57.930Z] 00:54:57 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:320:16
[task 2019-02-27T00:54:57.931Z] 00:54:57 INFO - runTest@dom/notification/test/mochitest/test_notification_insecure_context.html:32:5
[task 2019-02-27T00:54:57.933Z] 00:54:57 INFO - async*@dom/notification/test/mochitest/test_notification_insecure_context.html:43:5
[task 2019-02-27T00:54:57.934Z] 00:54:57 INFO - TEST-PASS | dom/notification/test/mochitest/test_notification_insecure_context.html | Granted permission in insecure context with pref set
[task 2019-02-27T00:54:57.936Z] 00:54:57 INFO - GECKO(1430) | MEMORY STAT | vsize 593MB | residentFast 278MB | heapAllocated 94MB
[task 2019-02-27T00:54:57.938Z] 00:54:57 INFO - TEST-OK | dom/notification/test/mochitest/test_notification_insecure_context.html | took 297ms

Flags: needinfo?(jhofmann)
Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebc59bbdd7ac
Require Secure Context for Notifications. r=Ehsan

https://hg.mozilla.org/mozilla-central/rev/ebc59bbdd7ac

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Note to MDN writers:

I've added a note about this to the Fx67 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/67#Security

In terms of docs this needs BCD, secure context labels, notifications api adding to secure context page, and anything else you think is needed (check what the notifications guides say too).

Whiteboard: [adv-main67-]

This change seems to have clobbered WebRTC permission request popups on localhost for FF dev-edition.

Sorry nevermind it was actually this that did it: https://bugzilla.mozilla.org/show_bug.cgi?id=1335740

It would be nice if there was a way to manually remove the https restriction for specific sites, if nothing else, for web development.

Having to set up an htttp daemon as a reverse proxy + SSL certificate just to be able to develop and test this feature makes it a non-starter.

Actually, there is a dom.webnotifications.allowinsecure option. But that's not ideal.

It would be nice to add a site-specific exception.

Well, you could use GitHub or Glitch or some such, but there's bug 1409841 to see if we should add some kind of setting.

It seems you can add a site-specific workaround:

  1. In about:config set dom.webnotifications.allowinsecure to true
  2. Visit the http site in question, and allow notifications
  3. In about:config set dom.webnotifications.allowinsecure back to fall
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: