Closed Bug 1243431 Opened 4 years ago Closed 3 years ago

Guard vibration API with permission

Categories

(Core :: DOM: Core & HTML, defect)

Unspecified
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed
fennec + ---

People

(Reporter: snorp, Assigned: esawin, NeedInfo)

Details

(Whiteboard: dom-triaged)

Attachments

(8 files, 11 obsolete files)

193.29 KB, image/png
Details
3.39 KB, patch
esawin
: review+
Details | Diff | Splinter Review
3.62 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
5.52 KB, patch
esawin
: review+
Details | Diff | Splinter Review
171.49 KB, image/png
antlam
: feedback+
Details
1.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
731 bytes, patch
gbrown
: review+
Details | Diff | Splinter Review
885 bytes, patch
Margaret
: review+
Details | Diff | Splinter Review
Apparently some websites are abusing the vibration API (shocking, I know) on Android. We should guard it with a permission to avoid this.
Would it be enough to turn it off for background tabs?
Assuming [1] is even the right spec, then we shouldn't vibrate for non-visible tabs:

"If the hidden attribute [PAGE-VISIBILITY] is set to true, then return false and terminate these steps."

(if it's not the right spec, it sounds like a better thing to do)

[1] <http://dev.w3.org/2009/dap/vibration/>
> Would it be enough to turn it off for background tabs?

It _is_ turned off for background tabs:

792 MayVibrate(nsIDocument* doc) {
...
800   return (doc && !doc->Hidden());

It should also be turned off when the browser app is not in foreground, I would think, for the same reason...
Is it possible Android is not setting visibility state correctly on background tabs?
I meant Fennec, not Android, of course.  Simple testcase:

<pre id="log"></pre>
<script>
function log(str) {
  document.getElementById("log").appendChild(document.createTextNode(str + "\n"));
}
document.addEventListener("visibilitychange", function() {
  log(document.hidden);
});
log(document.hidden);
</script>

Load the page, switch to another tab, switch back to this one.  Log should show "false", "true", "false".  And does for me on Fennec, so that part seems to be fine.
And I just tested that if I load that page, switch to another app, then switch back to Firefox on Android, I also get "false", "true", "false".  So we're correctly setting the document hidden state when Firefox is not the current app as well.

Which means the only way a site can use this API is if it's loaded in the foreground tab of a currently-active Firefox.

Oh, and if I load that page, leave Firefox up, then allow the phone to turn off its screen (so it goes into the lockscreen when I hit the little "talk to me" button), I also get "false", "true", "false".  Dunno whether we go into the hidden state when the screen turns off or when the lockscreen comes up, though.  Could probably tell by logging some timestamps too.
According to the reporter (from reddit), the pages are in the foreground. I guess adult and/or sketchy pages open popups that abuse this.
> According to the reporter (from reddit), the pages are in the foreground.

That's not what he's saying in https://www.reddit.com/r/firefox/comments/42yeki/is_something_being_done_to_take_care_of/czg7tdr afaict.
i've spent a long time today trying to get those URL but sorry havn't been able to get any.  
Im not sure if they change their ad hosts/partners but if i do stumble on the issue ill be sure to post the link.  

On another note:
Wouldn't a test site with the pop-up vibration thing replicate the same situation?
Sure, but at that point it only replicates in the foreground tab and if Firefox is the current app.

As in, you navigate to a site in the foreground tab and it vibrates.
Whiteboard: dom-noted
Whiteboard: dom-noted → dom-triaged
A user had a scary incident with this. Sadly didn't get the URL  
https://www.reddit.com/r/firefox/comments/42yeki/is_something_being_done_to_take_care_of/czopweq
Thanks for the update bull500. 

> However, it was not only able to make my phone vibrate, but it could also control the screen brightness, as in, it made the brightness go up and down as if it were a "malfunctioning light bulb".

I don't think it's possible to actually control screen brightness (we have an API to read ambient light, but that can't set screen brightness) -- but you could probably do some kind of stupid opacity animation to mimic it.
It's also possible that on some phone models vibration is accompanied by coordinated screen brightness changes or something.  A "visual vibrate" of sorts...
Got a couple of links

http://mangodigitalc.com/?zoneid=469237

http://j61launchers.com/?l=McboESTmK3H8wTr&s=146467506331&z=469237&g=IN&ba=1&dm=1&ep=1&vi=1&vo=1&fp=1&tr=in&language=hi

http://go.padsdel.com/afu.php?id=469237&var=469237

I don't think you'll get the first two but that padsdel site seems to redirect to mainly vibration causing sites and app downloads as well(pop-up for installation via playstore or fdroid is default is not chosen) 
Take caution!
My friend Tim tweeted about an ad doing this over the weekend:

view-source:http://shell.mobile-winner.net/?subid=1460237137mb92739501774&zoneid=7009&sxid=8ym3031172ky#b

<script type="text/javascript">
  navigator.vibrate = navigator.vibrate || navigator.webkitVibrate || navigator.mozVibrate || navigator.msVibrate;
  navigator.vibrate([1000, 500, 1000, 500, 1000, 500, 1000, 500, 1000]);
</script> 
<script>
alert("Congratulations!\n\nYou have been selected to participate in our survey to receive a $250 Shell Gift Card!\n\nPress OK to continue.");
</script>
tracking-fennec: --- → ?
Assignee: nobody → esawin
tracking-fennec: ? → +
The protection level for "android.permission.VIBRATE" is "normal", which means our behavior of granting the permission automatically at installation is correct.

Here are some of the options we have to prevent abuse of this API:
1. Use a proxy permission with protection level "dangerous"
   * Prompt the user for permission on first use
   * Permission can be revoked via Android's permission UI
2. Blacklist the vibration API via pref/Fennec UI
3. Detect and block abusive API usage (based on user interaction? don't really see a reliable way)

Option 1 requires the user to manually revoke granted permissions manually. I don't see a way to force the prompt once the permission has been granted, is that possible, sebastian?

Options 1 and 2 would require strings (l10n) for the prompt/UI.
Flags: needinfo?(s.kaspari)
(In reply to Eugen Sawin [:esawin] from comment #18)
> Here are some of the options we have to prevent abuse of this API:
> 1. Use a proxy permission with protection level "dangerous"
>    * Prompt the user for permission on first use
>    * Permission can be revoked via Android's permission UI

This sounds like we'd exploit the permission system. This is for granting a permission to the application. Effectively we'd ask the user to grant Firefox a permission it already has. Apart from that I'm not even sure if this works with custom permissions. Would be interesting to try.

Isn't this more about a permission the user grants a website and where we show those doorhangers (Video, Microphone, Notification, ..)? Or if this should be global a setting (but then it probably needs to be off by default to avoid this abuse?).

> Option 1 requires the user to manually revoke granted permissions manually
> I don't see a way to force the prompt once the permission has been granted,
> is that possible, sebastian?

Nope. Those permissions are permanent (until revoked in Android's settings) and you can only show the prompt to ask for it if you don't have it (not revoke).
Flags: needinfo?(s.kaspari)
Yeah, the terminology here is a little confusing. In the context used in the bug summary, I'm referring to a Firefox permission and not an Android one.
Some refactoring to reduce code duplication.
Attachment #8745369 - Flags: review?(bzbarsky)
Navigator will dispatch "Vibration:request" and listen to "Vibration:response:*" events to allow for user permission request handling.
Attachment #8745371 - Flags: review?(bzbarsky)
Show doorhanger UI on "Vibration:request" and dispatch results.

This patch adds strings.
Attachment #8745372 - Flags: review?(margaret.leibovic)
Comment on attachment 8745369 [details] [diff] [review]
0001-Bug-1243431-1.1-Add-permission-handling-utility-func.patch

>+  if (!permMgr || !aWindow || !aType) {

Why would aType ever be null here?  I think you should be able to just assert it's not null.

I think GetPermission or GetPermissionValue is a better name for this helper, since it's not testing anything; all the testing is in the callers.

>+  if (!permMgr || !aPrincipal || !aType) {

Again, no need to check aType.  Probably no need to check aPrincipal either; it should never be null.

>@@ -2217,12 +2254,7 @@ Navigator::CheckPermission(nsPIDOMWindowInner* aWindow, const char* aType)

You don't need the aWindow null-check here; it'll be done by TestPermission anyway.  Please take it out.

r=me
Attachment #8745369 - Flags: review?(bzbarsky) → review+
Comment on attachment 8745372 [details] [diff] [review]
0003-Bug-1243431-3.1-Show-doorhanger-on-vibration-request.patch

Review of attachment 8745372 [details] [diff] [review]:
-----------------------------------------------------------------

You should ask antlam to review the strings here, but otherwise looks fine with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +1951,5 @@
> +        let buttons = [
> +          {
> +            label: Strings.browser.GetStringFromName("vibrationRequest.denyButton"),
> +            callback: function() {
> +              Services.obs.notifyObservers(null, "Vibration:response:deny", null);

Nit: This message doesn't really follow our capitalization convention, but it wasn't added in this patch.

@@ +1964,5 @@
> +          }
> +        ];
> +        let host = browser.currentURI.host;
> +        let message = Strings.browser.formatStringFromName(
> +            "vibrationRequest.message", [host], 1);

Nit: This can all just go on one line, we don't care about long lines in JS.

@@ +1967,5 @@
> +        let message = Strings.browser.formatStringFromName(
> +            "vibrationRequest.message", [host], 1);
> +        let options = {};
> +        NativeWindow.doorhanger.show(message, "vibration-request", buttons,
> +            BrowserApp.selectedTab.id, options, "VIBRATION");

I don't think you need this last parameter, since we don't have a special "VIBRATION" type of doorhanger. This would require a change to DoorHangerPopup.java as well. However, you should probably make that change, so that we can use this value for telemetry.

Look at this changeset for an example of what we did for WebRTC:
http://hg.mozilla.org/mozilla-central/rev/1fde3cca31b4

@@ +1968,5 @@
> +            "vibrationRequest.message", [host], 1);
> +        let options = {};
> +        NativeWindow.doorhanger.show(message, "vibration-request", buttons,
> +            BrowserApp.selectedTab.id, options, "VIBRATION");
> +        break;

You should put braces around the body of this case statement, since let statements have block scope.
Attachment #8745372 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8745372 [details] [diff] [review]
0003-Bug-1243431-3.1-Show-doorhanger-on-vibration-request.patch

Anthony, can you please review the new strings.
Attachment #8745372 - Flags: review+ → review?(alam)
Addressed comments.

Carrying over r+ from Margaret.
Anthony r? for strings.
Attachment #8745372 - Attachment is obsolete: true
Attachment #8745372 - Flags: review?(alam)
Attachment #8746051 - Flags: review?(alam)
Comment on attachment 8746051 [details] [diff] [review]
0003-Bug-1243431-3.2-Show-doorhanger-on-vibration-request.patch

Quick question, is "%S" the site? 

With out current doorhanger design, it should already display the site above this line. That would make this quite repetitive.

If you don't mind, could you share a screenshot? that would be a lot easier too :) thanks!
Flags: needinfo?(esawin)
Attachment #8746051 - Flags: review?(alam) → review-
Attached image Vibration request doorhanger (obsolete) —
Flags: needinfo?(esawin)
I've attached both, the new vibration request and the existing camera request doorhangers. The domain name is redundant but consistent with what we currently have.
Comment on attachment 8745371 [details] [diff] [review]
0002-Bug-1243431-2.1-Guard-Vibration-API-with-user-permis.patch

>+    obs->AddObserver(this, "Vibration:response:allow", false);

This is not a great pattern.  We're passing a zero-refcount object to someone who might end up refcounting it.  That might be OK, or it might cause us to die before the call returns, depending on what the callee does.

If we really need this, we should be adding Navigator as an observer after the constructor returns.  But do we really need to add as observer before the first time someone calls Vibrate()?  Seems like it would be better to register in there, if mVibratorPromise.IsEmpty().

Also, why two separate notifications instead of one notification which sends allow/deny as the string data?

And also also, why notifications at all?  That seems pretty weird to me, especially because we never clear mVibratorPromise.  If two different navigator objects both try to vibrate one after another, won't both of them get the notification that's mean to be a response to one of them?

What I think we should be doing instead is that that our "Vibration:request" notification should send the Navigator as the subject.  Observers should then just call methods on that Navigator directly to tell it whether it's OK to vibrate.

And even then, we should think a bit about what happens if another vibrate() call happens while we're waiting on a permission response to a preceding one on the same Navigator.

>@@ -952,21 +995,36 @@ Navigator::Vibrate(const nsTArray<uint32_t>& aPattern)
>+  if (!aPattern.Length() || (aPattern.Length() == 1 && aPattern[0] == 0) ||
>+      permission == nsIPermissionManager::ALLOW_ACTION) {

I'd prefer the TestPermission() call were just inline here, so we don't even make that call when canceling.

Also, aPattern.IsEmpty() is probably better than !aPattern.Length().

>+  GetVibrator(pattern)->Then(AbstractThread::MainThread(), __func__,

These closures will run async. That means the resolve handler probably needs to recheck sanity stuff like mWindow not being null, mWindow->GetExtantDoc() not being null, and MayVibrate(mWindow->GetExtantDoc()).  Note that you'll want to reget doc too, because the document in mWindow might have changed while we were waiting on that promise.

>+        nsIURI* uri = doc->GetDocumentURI();
>+        AddPermission(uri, "vibration", nsIPermissionManager::ALLOW_ACTION,
>+                      nsIPermissionManager::EXPIRE_SESSION, 0);

This looks wrong to me: it'll fail when someone tries to vibrate in a srcdoc document, or blob:, etc, etc.

You presumably want to tie this permission to the principal URI or something, not the document URI...

I assume the UX for EXPIRE_SESSION is in fact the one we want here.

Also, please make GetVibrator return already_AddRefed instead of RefPtr, and just put it in a temporary on the stack before calling methods on it.  That will make the ownership a lot clearer.
Attachment #8745371 - Flags: review?(bzbarsky) → review-
Due to the new approach taken, most review comments for patch version 2.1 no longer apply. I'm addressing the rest below.

(In reply to Boris Zbarsky [:bz] from comment #32)
> What I think we should be doing instead is that that our "Vibration:request"
> notification should send the Navigator as the subject.  Observers should
> then just call methods on that Navigator directly to tell it whether it's OK
> to vibrate.

I've implemented this approach with this patch (2.2). For this, we expose a chrome-only method (setVibrationPermission) which is then called with the appropriate argument value in the doorhanger callback.

> And even then, we should think a bit about what happens if another vibrate()
> call happens while we're waiting on a permission response to a preceding one
> on the same Navigator.

As per spec, successive calls to vibrate() override previous patterns. I'm keeping the same logic for pre-authorized calls, i.e., only the lastly requested pattern before a successful permission response will be executed.

> >+        nsIURI* uri = doc->GetDocumentURI();
> >+        AddPermission(uri, "vibration", nsIPermissionManager::ALLOW_ACTION,
> >+                      nsIPermissionManager::EXPIRE_SESSION, 0);
> 
> This looks wrong to me: it'll fail when someone tries to vibrate in a srcdoc
> document, or blob:, etc, etc.
> 
> You presumably want to tie this permission to the principal URI or
> something, not the document URI...

I'm now tying the permission to the document's node principal, is this OK?
Attachment #8745371 - Attachment is obsolete: true
Attachment #8746520 - Flags: review?(bzbarsky)
I've removed the domain name from the message.
I'm not sure if it sounds right now, maybe we should add "here" or "on this website" at the end?
Attachment #8746051 - Attachment is obsolete: true
Attachment #8746527 - Flags: review?(alam)
This patch uses the new Navigator API call to permit vibration.
I will flag this for review, once patch 2.2 has been reviewed.

I've also encountered bug 1268471 with this.
Comment on attachment 8746520 [details] [diff] [review]
0002-Bug-1243431-2.2-Guard-Vibration-API-with-user-permis.patch

We should probably clear mRequestedVibrationPattern on the early returns from SetVibrationPermission.  Maybe SwapElements it into an on-stack array up front in that method to make that easy?

I assume using a response to a previous request for a new call is OK because the response won't depend on things that might have changed in the interim, right?  I I guess we're not providing that much information in our request, so I can't see how it can, ok.  I guess that lack of info is OK for the request recipient because they can assume the currently selected tab is what's making the request due to the MayVibrate() check we did?  Might be worth documenting.

>+++ b/dom/webidl/Navigator.webidl

The addition here needs documentation.  And it should go in a separate partial interface or in the nsIDOMNavigator one or something, not in the spec IDL snippet, since this is not part of the spec.

r=me with those nits.
Attachment #8746520 - Flags: review?(bzbarsky) → review+
Oh, one more thing.  This bit:

  static_cast<nsIDOMNavigator*>(this)

can probably be replaced with ToSupports(this).  That's going to invoke http://hg.mozilla.org/mozilla-central/file/4292da9df16b/xpcom/glue/nsCycleCollectionNoteChild.h#l48 which is going to work for Navigator, because it's cycle-collected.
Comment on attachment 8746532 [details] [diff] [review]
0004-Bug-1243431-4.1-Show-permission-request-doorhanger-o.patch

Thanks Boris, I'll address the comments before landing.

I have to run the chrome patch through your review again, Margaret, since the Navigator API has changed.
Attachment #8746532 - Flags: review?(margaret.leibovic)
(In reply to Eugen Sawin [:esawin] from comment #29)
> Created attachment 8746110 [details]
> Vibration request doorhanger

The language in our permissions really needs to be cleaned up... but that's a bug for another day.

I'm thinking we can say something like this for now:


Would you like to allow this site to vibrate your device?

Don't allow | Allow
Flags: needinfo?(esawin)
Attachment #8746527 - Attachment is obsolete: true
Attachment #8746527 - Flags: review?(alam)
Flags: needinfo?(esawin)
Attachment #8747081 - Flags: review?(alam)
One last patch to enable vibration in the mochitest to test the full code path.
Attachment #8747141 - Flags: review?(bzbarsky)
Can you post a screenshot please? :) it'll help me a lot more to see what this looks like "on device".

Also I noticed this in the diff

"vibrationRequest.denyButton = Dont' allow" ... I think you misplaced the apostrophe ;)
Flags: needinfo?(esawin)
Attachment #8747081 - Flags: review?(alam) → review-
Oh, sorry about that.

I'll post a screenshot once the build has finished.
Attachment #8747081 - Attachment is obsolete: true
Attachment #8747154 - Flags: review?(alam)
Attached image Vibration request doorhanger (obsolete) —
Attachment #8746110 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Comment on attachment 8747141 [details] [diff] [review]
0005-Bug-1243431-5.1-Enable-vibration-permission-for-test.patch

Won't this just throw, since the API is chromeonly?
Attachment #8747141 - Flags: review?(bzbarsky) → review-
(In reply to Eugen Sawin [:esawin] from comment #46)
> Created attachment 8747190 [details]
> Vibration request doorhanger

Actually reading it in the doorhanger - it sounds like "Would you like to" seems redundant.

"Allow this site to vibrate your device?" is enough.

Can we update that? Thanks!
Flags: needinfo?(esawin)
Attached image doorhanger.png
Attachment #8747190 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8747752 - Flags: feedback?(alam)
Comment on attachment 8747752 [details]
doorhanger.png

wfm!
Attachment #8747752 - Flags: feedback?(alam) → feedback+
The Try run was green with the broken test because we exclude the test on Android.

With this patch, we add the vibration permission for the test.
Attachment #8747141 - Attachment is obsolete: true
Attachment #8748128 - Flags: review?(bzbarsky)
The vibration test has been disabled for years because of "CRASH_SUTAGENT".

The test run with it enabled looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d75987225f0e

Is it safe to enable this test on Android?
Attachment #8748129 - Flags: review?(gbrown)
Comment on attachment 8748129 [details] [diff] [review]
0006-Bug-1243431-6.1-Enable-test_vibrator-on-Android.-r-g.patch

Review of attachment 8748129 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8748129 - Flags: review?(gbrown) → review+
Comment on attachment 8748130 [details] [diff] [review]
0003-Bug-1243431-3.6-Add-strings-for-Vibration-API-permis.patch

Sorry, I shouldn't review patches :)
Attachment #8748130 - Flags: review?(alam)
Comment on attachment 8748130 [details] [diff] [review]
0003-Bug-1243431-3.6-Add-strings-for-Vibration-API-permis.patch

Anthony has accepted the strings, redirecting to Margaret for review.
Attachment #8748130 - Flags: review?(margaret.leibovic)
Comment on attachment 8748128 [details] [diff] [review]
0005-Bug-1243431-5.2-Enable-vibration-permission-for-test.patch

r=me
Attachment #8748128 - Flags: review?(bzbarsky) → review+
Attachment #8748130 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8746532 [details] [diff] [review]
0004-Bug-1243431-4.1-Show-permission-request-doorhanger-o.patch

Review of attachment 8746532 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, sorry, I saw the other review request but not this one.
Attachment #8746532 - Flags: review?(margaret.leibovic) → review+
We're debating the right design (and eventually spec text) for this over here: https://github.com/WICG/interventions/issues/25

In particular I'm arguing that this is really conceptually just like the autoplay audio debate and vibration should be enabled after the user has tapped the frame.  We'd love any input any of you have on that over in WICG.
Barbara, who is the right Mozilla person in product or engineering to discuss navigator.vibrate() API permissions and UX in Firefox Mobile browsers?
Flags: needinfo?(bbermes)
NIing Joe who will make sure this gets addressed. Most likely UX and Eng from Taipei will take this on.
Flags: needinfo?(jcheng)
Flags: needinfo?(hhsu)
Flags: needinfo?(bbermes)
As Rick mentioned above, we're discussing the design (and eventually spec text) for this over this new thread: https://github.com/WICG/interventions/issues/47, a follow-up effort of https://github.com/WICG/interventions/issues/25.

The idea is that vibration should be enabled after the user has tapped the frame.  We'd love any input any of you have on that over in WICG. Thanks.
Flags: needinfo?(hhsu)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.