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

RESOLVED WONTFIX

Status

()

Core
DOM: Contacts
RESOLVED WONTFIX
4 years ago
a year ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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]
(Assignee)

Updated

4 years ago
Depends on: 958667
(Assignee)

Comment 1

4 years ago
Created attachment 8377764 [details] [diff] [review]
Add a pref to skip scope checks
Attachment #8377764 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 8377765 [details] [diff] [review]
Hide contacts interfaces
Attachment #8377765 - Flags: review?(anygregor)
(Assignee)

Comment 3

4 years ago
Created attachment 8377766 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts
Attachment #8377766 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

4 years ago
Created attachment 8377769 [details] [diff] [review]
Add a pref to skip scope checks

I uploaded the wrong patch.
Attachment #8377764 - Attachment is obsolete: true
Attachment #8377764 - Flags: review?(bzbarsky)
Attachment #8377769 - Flags: review?(bzbarsky)

Updated

4 years ago
Blocks: 916605
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.
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
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

4 years ago
(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?
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
Created attachment 8378387 [details] [diff] [review]
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.
Attachment #8377766 - Attachment is obsolete: true
Attachment #8378387 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

4 years ago
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?
(Assignee)

Comment 19

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

Yep.
(Assignee)

Updated

4 years ago
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.
Attachment #8377769 - Flags: feedback?(jonas) → feedback-
(Assignee)

Updated

4 years ago
Depends on: 952486
Reuben, any update here?
Flags: needinfo?(reuben.bmo)
(Assignee)

Comment 23

4 years ago
I'm looking at bug 952486 since I'll need to fix that first.
Flags: needinfo?(reuben.bmo)

Comment 24

4 years ago
This will break the MarketPlace feature detection if we do this before bug 983502 lands, right?
Depends on: 983502
(Assignee)

Comment 25

4 years ago
Created attachment 8413045 [details] [diff] [review]
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.
Attachment #8377765 - Attachment is obsolete: true
Attachment #8413045 - Flags: review?(anygregor)
(Assignee)

Comment 26

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

…for now.
(Assignee)

Comment 27

4 years ago
Created attachment 8413046 [details] [diff] [review]
Add a check to the Navigator resolve hook for mozContacts, v3

Checks permission instead of app status. Will eventually be replaced with [CheckPermission].
Attachment #8378387 - Attachment is obsolete: true
Attachment #8413046 - Flags: review?(jonas)
(Assignee)

Updated

4 years ago
Attachment #8377769 - Attachment is obsolete: true

Comment 28

4 years ago
(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.
(Assignee)

Comment 29

4 years ago
(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

4 years ago
(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.

Updated

4 years ago
Depends on: 1009645

Updated

4 years ago
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)
(Assignee)

Comment 32

4 years ago
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+
(Assignee)

Comment 34

a year ago
This code no longer exists.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.