WebTelephony: Hide Telephony API from regular Web pages

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

I don't think we can ship this yet.
Assignee: nobody → vyang
Summary: Hide Telephony API behind a pref, at least on desktop → WebTelephony: Hide Telephony API behind a pref, at least on desktop
Posted patch patch (obsolete) — Splinter Review
Vicamo, please review Telephony bits.
Kyle, please review the interface change.

Green except a known failure:
https://tbpl.mozilla.org/?tree=Try&rev=14ed6cb1d3d7
Attachment #801883 - Flags: review?(vyang)
Attachment #801883 - Flags: review?(khuey)
Comment on attachment 801883 [details] [diff] [review]
patch

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

Thank you Masatoshi.

r=me
Attachment #801883 - Flags: review?(khuey) → review+
Comment on attachment 801883 [details] [diff] [review]
patch

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

Thank you.
Attachment #801883 - Flags: review?(vyang) → review+
Assignee: vyang → VYV03354
Posted patch patch v2Splinter Review
I found a better function to determine the availability of Telephony API. We don't have to add a pref to complicate things further. Also this patch will reduce the use of the compile-time option.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4bb73289d110
Attachment #801883 - Attachment is obsolete: true
Attachment #803084 - Flags: review?(vyang)
Attachment #803084 - Flags: review?(khuey)
Summary: WebTelephony: Hide Telephony API behind a pref, at least on desktop → WebTelephony: Hide Telephony API behind a permission
Comment on attachment 803084 [details] [diff] [review]
patch v2

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

Today I have a problem to deal with.  In B2G tablet build, we runs master Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined| on tablet builds and Gaia dialer app certainly has "telephony" permission.  This patch simply overthrows one possible solution to this problem.  See bug 911684.

Besides, why is the existence of a certain navigator attribute controlled by the app?  Should we treat it a bug that when an app happens to be able to claim the permission to a certain API set which we want to explicitly disable on a platform/build, and navigator does allow it?  I know the pre-condition itself is a serious security bug, but will its result be a bug, too?
Attachment #803084 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> Today I have a problem to deal with.  In B2G tablet build, we runs master
> Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined|
> on tablet builds and Gaia dialer app certainly has "telephony" permission. 
> This patch simply overthrows one possible solution to this problem.  See bug
> 911684.

Actually this patch will help to fix bug 911684. You can change Navigator::HasTelephonySupport function whatever you like (for example, always return false on desktop and tablet). This is much more flexible than a sigle pref.

> Besides, why is the existence of a certain navigator attribute controlled by
> the app?

This patch doesn't change the behavior of a navigator attribute. This patch will just change the existence of global objects (Telephony, TelephonyCall, TelephonyCallGroup, CallEvent). These objects will be present iff navigator.mozTelephony is present. There is no point exposing these global objects when the navigator property is not present.
FYI, I didn't write Navigator::HasTelephonySupport function.
> Besides, why is the existence of a certain navigator attribute controlled by
> the app?
So this behavior was introduced by someone wrote Navigator::HasTelephonySupport function, not me.
Per hg annotate, :bz write the function and :bent and :sicking reviewed.
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bent.mozilla)
Changing the subject based on what I really meant to.
Summary: WebTelephony: Hide Telephony API behind a permission → WebTelephony: Hide Telephony API from regular Web pages
The pre-webidl code exposed the property unconditionally #ifdef MOZ_B2G_RIL.  Attempting to get the property would check the "telephony" permission and if granted return the object.  

The post-webidl code does something similar: the mozTelephony pref on Navigator is #ifdef MOZ_B2G_RIL and even then only defined in globals with the permission.  Basically, I tried to not rock the boat too much.  ;)  The difference was that lack of permission means undefined, not null, after the WebIDL patch.

> Gaia expects |navigator.mozTelephony == undefined| 

The attached patch does not change the behavior of navigator.mozTelephony.

> Should we treat it a bug that when an app happens to be able to claim the permission to
> a certain API set which we want to explicitly disable on a platform/build,

I'm not sure what this is asking...  .mozTelephony is always undefined #ifndef MOZ_B2G_RIL, no matter what HasTelephonySupport() returns.
Flags: needinfo?(bzbarsky)
Comment on attachment 803084 [details] [diff] [review]
patch v2

This patch does not change the behavior of navigator.mozTelephony. If you are not comfortable with the current navigator.mozTelephony, please file a bug about that.
I don't care much about the behavior in apps as long as Telephony API is hidden from regular Web pages. Note that the v1 patch didn't hide the API from Web pages on B2G. It's impossible to distinguish apps from Web pages by using a simple pref. Again, please file a separate bug and change Navigator::HasTelephonySupport as you wish.
Attachment #803084 - Flags: review?(vyang)
Comment on attachment 803084 [details] [diff] [review]
patch v2

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

::: dom/base/Navigator.h
@@ +87,2 @@
>  class CellBroadcast;
>  #endif

This might need a rebase on bug 910568, which is still in b2g-inbound though.
Attachment #803084 - Flags: review?(vyang) → review+
Hi Masatoshi, could you also confirm we need to apply the same thing to other permission-required DOM APIs like MobileConnection, Voicemail, MobileMessage, CellBroadcast, etc., as well?
Flags: needinfo?(VYV03354)
(In reply to Masatoshi Kimura [:emk] from comment #8)
> Per hg annotate, :bz write the function and :bent and :sicking reviewed.

I'm not sure what you're asking for exactly, but since this is all behind an #ifdef I don't see us gaining much from this change. It doesn't seem like it will hurt either, though.
Flags: needinfo?(bent.mozilla)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Hi Masatoshi, could you also confirm we need to apply the same thing to
> other permission-required DOM APIs like MobileConnection, Voicemail,
> MobileMessage, CellBroadcast, etc., as well?

Yes, but I'll do it in a follow-up bug because the priority is lower than this bug. Unlike Telephony, global objects for those APIs are (unfortunately) already exposed on release versions of Firefox.
Flags: needinfo?(VYV03354)
Comment on attachment 803084 [details] [diff] [review]
patch v2

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

r=me

Thanks Masatoshi.
Attachment #803084 - Flags: review?(khuey) → review+
> but since this is all behind an #ifdef

The existence of window.Telephony/TelephonyCall/TelephonyCallGroup is not behind an ifdef.  It's just there in desktop Firefox.
https://hg.mozilla.org/integration/mozilla-inbound/rev/492fdd7c06fd
Status: NEW → ASSIGNED
Flags: needinfo?(jonas) → in-testsuite+
https://tbpl.mozilla.org/php/getParsedLog.php?id=27793283&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27793063&tree=Mozilla-Inbound

These were failures on the original commit. Were they fixed in the followup, or do we need to back it all out?
Flags: needinfo?(VYV03354)
Need a backout for the marionette failure. (I thought I ran marionette on try, but needed a different token from desktop.)
Flags: needinfo?(VYV03354)
But not sure why mochitest failed... (need clobber?)
Looks like the patch was broken by something in the following range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5417e5da2ceb&tochange=d5fc994ca2ed
I was unable to make marionette test work.
I gave up and landed the v1 patch for now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/014958e3c8c9
https://hg.mozilla.org/mozilla-central/rev/014958e3c8c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
No longer blocks: 912612
nominate for koi+ for it blocks koi+ bug 914182.
blocking-b2g: --- → koi?
Aready in mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/014958e3c8c9
blocking-b2g: koi? → ---
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.