Closed Bug 971766 Opened 7 years ago Closed 4 years ago

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

Categories

(Core Graveyard :: DOM: Contacts, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: reuben, Assigned: reuben)

References

(Blocks 1 open bug)

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 5 obsolete files)

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.
Whiteboard: [systemsfe]
Depends on: 958667
Attached patch Add a pref to skip scope checks (obsolete) — Splinter Review
Attachment #8377764 - Flags: review?(bzbarsky)
Attached patch Hide contacts interfaces (obsolete) — Splinter Review
Attachment #8377765 - Flags: review?(anygregor)
Attachment #8377766 - Flags: review?(bzbarsky)
Attached patch Add a pref to skip scope checks (obsolete) — Splinter Review
I uploaded the wrong patch.
Attachment #8377764 - Attachment is obsolete: true
Attachment #8377764 - Flags: review?(bzbarsky)
Attachment #8377769 - Flags: review?(bzbarsky)
Blocks: stdglobal
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?
Attachment #8377766 - Flags: review?(bzbarsky) → review-
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.
Attachment #8377769 - Flags: review?(bzbarsky) → review+
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...
Attachment #8377769 - Flags: feedback?(jonas)
We used to have such a flag afaik but we had to remove it because of security concerns.
(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.
> 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.
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.
(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?
Flags: needinfo?(jonas)
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?
(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.
As discussed on IRC, what we really want here is to check the appStatus, not the permissions.
Attachment #8377766 - Attachment is obsolete: true
Attachment #8378387 - Flags: review?(bzbarsky)
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 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.
Attachment #8378387 - Flags: review?(bzbarsky) → review+
Attachment #8377765 - Flags: review?(anygregor) → review+
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?
(In reply to Gregor Wagner [:gwagner] from comment #18)
> This is privileged + certified right?

Yep.
Blocks: 854600
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.
Flags: needinfo?(jonas)
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.
Depends on: 952486
Reuben, any update here?
Flags: needinfo?(reuben.bmo)
I'm looking at bug 952486 since I'll need to fix that first.
Flags: needinfo?(reuben.bmo)
This will break the MarketPlace feature detection if we do this before bug 983502 lands, right?
Depends on: 983502
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.
Attachment #8377765 - Attachment is obsolete: true
Attachment #8413045 - Flags: review?(anygregor)
(In reply to Reuben Morais [:reuben] from comment #25)
>let's just use the pref.

…for now.
Checks permission instead of app status. Will eventually be replaced with [CheckPermission].
Attachment #8378387 - Attachment is obsolete: true
Attachment #8413046 - Flags: review?(jonas)
Attachment #8377769 - Attachment is obsolete: true
(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.
(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).
(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.
Depends on: 1009645
No longer depends on: 983502
Attachment #8413045 - Flags: review?(anygregor) → review+
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?
Attachment #8413046 - Flags: review?(jonas) → review?(bzbarsky)
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 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.  :(
Attachment #8413046 - Flags: review?(bzbarsky) → review+
This code no longer exists.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.