971766 - Hide window.mozContact and navigator.mozContacts when they're not supposed to be exposed

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Contacts, defect)

Description

[Reuben Morais [:reuben]]

Neither interface should be exposed when the dom.mozContacts.enabled pref is false, and mozContact shouldn't be exposed to browser tabs and unprivileged apps in B2G.

Comment 1

[Reuben Morais [:reuben]]

Attachment: Add a pref to skip scope checks

Comment 2

[Reuben Morais [:reuben]]

Attachment: Hide contacts interfaces

Comment 3

[Reuben Morais [:reuben]]

Attachment: Add a check to the Navigator resolve hook for mozContacts

Comment 4

[Reuben Morais [:reuben]]

Attachment: Add a pref to skip scope checks

I uploaded the wrong patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377766 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts

So this makes navigator.mozContacts not have the same visibility conditions as the interface object, right?  Why?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377769 [details] [diff] [review]
Add a pref to skip scope checks

This is just to make the tests happy, I assume?

I would prefer that we called this preference "dom.test_only.ignore_webidl_app_type_checks" or something, so people don't just go setting it.

Unfortunately, calling Preferences::GetBool in any code where you see NS_IsMainThread conditionals is probably not OK.  At the very least, this should be in the "is on mainthread" branch.

Not sure whether the performance worries us enough that we should have a pref cache here, by the way.  Maybe we don't really need to worry about that part too much if it's rare to have these checks on perf-sensitive paths, so r=me if this is moved to below the workers early return and the pref name is adjusted.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377769 [details] [diff] [review]
Add a pref to skip scope checks

Though I'd like to know what sicking thinks of this approach to testing the behavior...

Comment 8

[Gregor Wagner [:gwagner]]

We used to have such a flag afaik but we had to remove it because of security concerns.

Comment 9

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #5)
> So this makes navigator.mozContacts not have the same visibility conditions
> as the interface object, right?  Why?

ContactManager is [NoInterfaceObject]. Do you mean not the same conditions as window.mozContact?

FWIW, I'm only including this resolve hook check to match other APIs and allow feature detection via |navigator.mozFoo !== undefined|. Not having to do this would be great.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Do you mean not the same conditions as window.mozContact?

Yes.

I'd like to clearly understand under what conditions we want to expose these various bits of API.

Comment 11

[Reuben Morais [:reuben]]

As far as I know, the only requirement for doing this is to allow the Marketplace app to detect the presence of APIs in a platform. The navigator object should be null if the window doesn't have the necessary permissions to use the API, and undefined if it's enabled in that platform (handled by the pref). Alternative approaches have been discussed but I'm not sure if a decision has been made to not do this anymore.

Comment 12

[(no longer active)]

(In reply to Reuben Morais [:reuben] from comment #11)
> As far as I know, the only requirement for doing this is to allow the
> Marketplace app to detect the presence of APIs in a platform. The navigator
> object should be null if the window doesn't have the necessary permissions
> to use the API, and undefined if it's enabled in that platform (handled by
> the pref). Alternative approaches have been discussed but I'm not sure if a
> decision has been made to not do this anymore.

I thought we agreed to not do that, and use navigator.hasFeature() for that purpose?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not talking about the marketplace/navigator weirdness.  I'm talking about window.mozContact.  Should that be exposed if dom.mozContacts.enabled is set, you're a privileged app, but don't have any of the "contacts-*" permissions?  Or do all privileged apps have those?

Comment 14

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #13)
> I'm not talking about the marketplace/navigator weirdness.  I'm talking
> about window.mozContact.  Should that be exposed if dom.mozContacts.enabled
> is set, you're a privileged app, but don't have any of the "contacts-*"
> permissions?  Or do all privileged apps have those?

Yes. We prompt the user asking for permission in that case.

Comment 15

[Reuben Morais [:reuben]]

Attachment: Add a check to the Navigator resolve hook for mozContacts, v2

As discussed on IRC, what we really want here is to check the appStatus, not the permissions.

Comment 16

[Reuben Morais [:reuben]]

Comment on attachment 8378387 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts, v2

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

::: dom/base/Navigator.cpp
@@ +1978,5 @@
>  
> +bool
> +Navigator::CheckAppStatus(uint16_t aStatus)
> +{
> +  return GetAppStatus(mWindow, aStatus);

Attached before qref'ing, this should be CheckAppStatus.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378387 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts, v2

>+  return GetAppStatus(mWindow, aStatus);

You mean return CheckAppStatus?

r=me with that fixed.

Comment 18

[Gregor Wagner [:gwagner]]

Comment on attachment 8377765 [details] [diff] [review]
Hide contacts interfaces

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

::: dom/webidl/Contacts.webidl
@@ +57,5 @@
>  
>  [Constructor(optional ContactProperties properties),
> + JSImplementation="@mozilla.org/contact;1",
> + Pref="dom.mozContacts.enabled",
> + AvailableIn=PrivilegedApps]

This is privileged + certified right?

Comment 19

[Reuben Morais [:reuben]]

(In reply to Gregor Wagner [:gwagner] from comment #18)
> This is privileged + certified right?

Yep.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

These properties should be hidden everywhere except on pages that where nsIPermissionManager.testExactPermissionFromPrincipal returns something other than UNKNOWN_ACTION.

Effectively that will cause the API to get hidden everywhere except in pages that belong to an app where the app has requested the "contacts" permission in the manifest and was then signed by mozilla.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So with that, I don't think we need to add a pref to enable the API everywhere. Simply add the permission instead. The only trick is that you need to add the permission and then run the test in an iframe.

Comment 22

[Gregor Wagner [:gwagner]]

Reuben, any update here?

Comment 23

[Reuben Morais [:reuben]]

I'm looking at bug 952486 since I'll need to fix that first.

Comment 24

[(no longer active)]

This will break the MarketPlace feature detection if we do this before bug 983502 lands, right?

Comment 25

[Reuben Morais [:reuben]]

Attachment: Hide contacts interfaces, v2 (pref only)

Probably. That's going to take a while, and I have to deal with some weird failures in bug 952486 so let's just use the pref.

Comment 26

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #25)
>let's just use the pref.

…for now.

Comment 27

[Reuben Morais [:reuben]]

Attachment: Add a check to the Navigator resolve hook for mozContacts, v3

Checks permission instead of app status. Will eventually be replaced with [CheckPermission].

Comment 28

[(no longer active)]

(In reply to comment #25)
> Created attachment 8413045 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8413045&action=edit
> Hide contacts interfaces, v2 (pref only)
> 
> Probably. That's going to take a while, and I have to deal with some weird
> failures in bug 952486 so let's just use the pref.

Let me clarify.  We should not land this yet if it breaks the market place feature detection.

Comment 29

[Reuben Morais [:reuben]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #28)
> Let me clarify.  We should not land this yet if it breaks the market place
> feature detection.

"This" as in the patches that are on this bug right now, do not break Marketplace feature detection. They hide the Contacts interfaces on platforms that aren't B2G, the way it used to be before the WebIDL conversion (I messed that up, forgot to include the pref in the .webidl files when landing).

Comment 30

[(no longer active)]

(In reply to comment #29)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #28)
> > Let me clarify.  We should not land this yet if it breaks the market place
> > feature detection.
> 
> "This" as in the patches that are on this bug right now, do not break
> Marketplace feature detection. They hide the Contacts interfaces on platforms
> that aren't B2G, the way it used to be before the WebIDL conversion (I messed
> that up, forgot to include the pref in the .webidl files when landing).

OK, sounds good, sorry for the misunderstanding.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8413046 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts, v3

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

I think we have webidl annotations for this these days?

Comment 32

[Reuben Morais [:reuben]]

We do not – remember this is the undefined/null for marketplace funny business. I'm working on navigator.getFeature() et al. so we can stop doing hacks like this.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8413046 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts, v3

r=me given the behavior we need here.  :(

Comment 34

[Reuben Morais [:reuben]]

This code no longer exists.