Closed Bug 838614 Opened 11 years ago Closed 7 years ago

Don't expose properties of APIs when they're not supposed to be available

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bruant.d, Unassigned)

References

(Depends on 1 open bug)

Details

Formalizing a recent conversation with Justin Lebar as a bug.
https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/tnYl_4480GM

Currently, in non-certified apps, doing 'typeof navigator.mozTime' throws an exception and that goes against the feature detection practice that web developers currently use.
They usually do feature detection by using one of these patterns:
     if('mozTime' in navigator){}
     if(navigator.mozTime){}
     if(typeof navigator.mozTime === 'object'){} // in some cases, it
can be "function"

In case "navigator.mozTime" throws, patterns 2 or 3 can't be used and
feature detection involves a try/catch block which shouldn't be necessary.

If the property is exposed as null, the pattern 2 works, but 1 and 3
give false positive (3 because typeof null === 'object' unfortunately...).

If mozTime wasn't exposed, all 3 patterns do what devs would expect.
Hmm.  So I believe right now what we support in DOMClassInfo here is calling a method once, during process startup, to determine whether to expose the API.

Is that good enough to handle this?  In particular, do we know at process startup time whether it's for a certified app or not?
Where by "startup" I mean "when nsDOMClassInfo::Init is called".
(In reply to Boris Zbarsky (:bz) from comment #1)
> Hmm.  So I believe right now what we support in DOMClassInfo here is calling
> a method once, during process startup, to determine whether to expose the
> API.
> 
> Is that good enough to handle this?  In particular, do we know at process
> startup time whether it's for a certified app or not?

We sometimes run apps in process, so this does not work perfectly.  I think all the apps we run in process are certified, but they are not all omnipotent.  AIUI, "certified" / "privileged" / "vanilla app" gives you the /ability/ to request permission to access an API, but does not automatically grant you access to (most) APIs.

In the OOP case, I bet we could read the package manifest before we launch the app.

But actually, I think this is all moot, since we have a preallocated process which can turn into any type of app process.  The preallocated process does a bunch of stuff with chrome JS, so I have to assume nsDOMClassInfo::Init is called.
OK.

So the simplest way forward is probably to convert navigator to WebIDL, then.  That has the benefit of doing the pref check at the time the navigator prototype object is created, so on a per-Window basis....

Alternately, we could keep the property but return undefined if not supported.  That has the drawback of not supporting pattern 1 from comment 0.
> That has the benefit of doing the pref check at the time the navigator prototype object 
> is created, so on a per-Window basis....

We'd have to muck with the pref engine not to forward pref changes from the parent down to the children, but I guess we could do that.

Also, this of course doesn't work for in-process apps.  Not to suggest that we need such a solution, since we control most in-process apps pretty heavily.

Would it be hard to do something other than a pref check at the time the navigator prototype is created?  Optimally we could ask the window whether it's privileged.

Actually, another problem I realized is that even inside an OOP app process, not all windows are equally privileged.  An app can load http://evil.com inside an iframe, and that frame won't be able to access privileged APIs.
> Would it be hard to do something other than a pref check at the time the navigator
> prototype is created?

It would take a bit work.  The way I do it right now is to just set up a boolean for every run of properties that have the same pref, with a pref cache on the boolean as needed, and then at property-definition time I check the booleans as I define the properties.

I'd have to replace the booleans with function pointers or something.  That would mean one extra function call per interface not using prefable stuff, which might be ok, I guess.  And I'd have to figure out a sane way to handle existing [Pref] annotations...

Peter, thoughts?
Just talked to Peter.  We'll make it so you can specify the name of a function in the WebIDL and the binding code will invoke that function and let you return a boolean for whether to define the property.
Depends on: 838691
> so on a per-Window basis....

Per inner-window or per outer-window?

If an <iframe> is initially at app.com, that's privileged.  But if it then navigates to evil.com, that's unprivileged.
> Per inner-window or per outer-window?

Inner, sorry.  "Window" in spec terms is inner window; WindowProxy is outer window.
I don't think that we need to make |"mozTime" in navigator| to return false. I think it's enough that |navigator.mozTime| returns a falsy value.

In fact, you could argue that it's a feature that a developer can use
|"mozTime" in navigator| to check "is this API implemented on this platform" and |navigator.mozTime| to check "can I use this API".

So making it a compile time (or startup time) check to see if the interface should be implemented by the navigator object, and making mozTime return null when the permissions aren't there, sounds like a good and easy solution?
(In reply to Jonas Sicking (:sicking) from comment #10)
> I don't think that we need to make |"mozTime" in navigator| to return false.
> I think it's enough that |navigator.mozTime| returns a falsy value.
A falsy value would be a major improvement over the current throwing situation.
But from a JS dev point of view, exposing a placeholder property with a null value looks... non-idiomatic. JavaScript objects have the choice to not expose a property. Not having a property is how you usually express "not available".

> In fact, you could argue that it's a feature that a developer can use
> |"mozTime" in navigator| to check "is this API implemented on this platform"
When an app is running, it doesn't care about what other more privileged apps can do.
That's important to emphasis, because that's another reason why the current SECURITY_ERROR throwing is a bad idea. Apps only care about what they can do. Whether they can't do something because it's not on the platform, or it is but the platform doesn't let them, or whatever else is unimportant.

hmm... maybe I'm wrong on this for the specific mozTime example. There is value for an app to know if the platform supports the (moz)timechange event. Existence of mozTime could be such a way (because I don't think there is any other way for now?). I'll file a specific bug for that.

But overall, my point remains. Apps only care about what the platform has to offer when they're running, not what it can do in theory.

chrome-only capabilities aren't exposed to web content as "null" properties. They're just not exposed at all. That's an equivalent situation. Apps with some privileges only need to know what they can do.

> and |navigator.mozTime| to check "can I use this API".
With the assumption that truthy implies "it's the right value", which in practice is true.
I know I always check with typeof. Some others do, but it's the exception more than the rule (certainly because of character count more than any other reason).

> So making it a compile time (or startup time) check to see if the interface
> should be implemented by the navigator object, and making mozTime return
> null when the permissions aren't there, sounds like a good and easy solution?
As a JS dev if a feature is absent, I would love to see it expressed as, by order of preference:
1) consistently absent property 
2) consistently the same falsy value
2.1) undefined
2.2) null (because of the unfortunate |typeof null === 'object'|)
3) consistently throwing
4) doing something inconsistent when in comes to APIs not being available (which is the current situation if I understand correctly what Justin said)

I don't think 3 and 4 are acceptable solutions. If that's 2.2), that's acceptable.
Boris in comment #7 suggests that there will be a solution to do 1). If that's doable in a realistic time frame, that'd be preferable in my opinion.
(In reply to David Bruant from comment #11)
> hmm... maybe I'm wrong on this for the specific mozTime example. There is
> value for an app to know if the platform supports the (moz)timechange event.
> Existence of mozTime could be such a way (because I don't think there is any
> other way for now?). I'll file a specific bug for that.
bug 841708
AFAIK no-one is arguing that 3 or 4 are desired solutions. The fact that we throw now is simply a bug. Most other APIs do not do that.

I'm not fully convinced that there aren't reasons to be able to tell "is implemented" from "can I use", though I don't have any good use cases off the top of my head.
(In reply to Jonas Sicking (:sicking) from comment #13)
> I'm not fully convinced that there aren't reasons to be able to tell "is
> implemented" from "can I use", though I don't have any good use cases off
> the top of my head.
To find a use case, I think a good question to answer would be "How can one usefully use this information?" and I can't find a proper answer, but I'd be open to hearing one.
To draw another parallel, web features that are implemented but disabled-by-default behind a config flag are not exposed (property is absent when applicable) to scripts. That's true both for Firefox and Chrome as far as I know.
Blocks: 859554
(In reply to David Bruant from comment #14)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> > I'm not fully convinced that there aren't reasons to be able to tell "is
> > implemented" from "can I use", though I don't have any good use cases off
> > the top of my head.
> To find a use case, I think a good question to answer would be "How can one
> usefully use this information?" and I can't find a proper answer, but I'd be
> open to hearing one.

I believe Marketplace needs to be able to tell those apart, see bug 859554.
(In reply to Reuben Morais [:reuben] from comment #16)
> (In reply to David Bruant from comment #14)
> > (In reply to Jonas Sicking (:sicking) from comment #13)
> > > I'm not fully convinced that there aren't reasons to be able to tell "is
> > > implemented" from "can I use", though I don't have any good use cases off
> > > the top of my head.
> > To find a use case, I think a good question to answer would be "How can one
> > usefully use this information?" and I can't find a proper answer, but I'd be
> > open to hearing one.
> 
> I believe Marketplace needs to be able to tell those apart, see bug 859554.
I was talking about a way to differentiate these cases *within* an app *at runtime*. Does the Marketplace need to do that? Isn't the manifest enough for the marketplace?

I haven't been able to find a detailed description of what Marketplace needs. Could you link to one please?

Also, in comment #0, I showed 3 feature detection patterns. Can the marketplace use one of these for its needs?
(In reply to David Bruant from comment #17)
> I was talking about a way to differentiate these cases *within* an app *at
> runtime*. Does the Marketplace need to do that? Isn't the manifest enough
> for the marketplace?
> 
> I haven't been able to find a detailed description of what Marketplace
> needs. Could you link to one please?
> 
> Also, in comment #0, I showed 3 feature detection patterns. Can the
> marketplace use one of these for its needs?

Someone who actually works on Marketplace should chime in, but we used to do 2.2 in comment 11 for some APIs and they asked us to change it to return undefined in unsupported platforms and null for no access.

And yes, I mean Marketplace the app. My understanding is that they want to show privileged apps that access APIs Marketplace doesn't have access to, but not apps that depend on APIs that are unsupported in that platform.
(In reply to Reuben Morais [:reuben] from comment #18)
> Someone who actually works on Marketplace should chime in, but we used to do
> 2.2 in comment 11 for some APIs and they asked us to change it to return
> undefined in unsupported platforms and null for no access.
So you want to move from 2.2 to 4... This seems like a regression to me for the app developer perspective :-/

> And yes, I mean Marketplace the app.
Oh ok, thanks for the clarification, I initially thought the server-side was involved and didn't understand.

> My understanding is that they want to
> show privileged apps that access APIs Marketplace doesn't have access to,
> but not apps that depend on APIs that are unsupported in that platform.
I understand the need for the Marketplace app to run in the device and feature-detect in live. However, as I've been explaining in this bug and the conversion linked to in bug description, apps don't care of the difference between "not implemented" and "implemented and not available" since the end-result is exactly the same, that is that the feature can't be used.
I've recently reviewed a conference talk where someone used "if('querySelector' in document && 'localStorage' in window)" to make feature detection. That's something that people use in their code and is a spreadout practice among JS devs (whether we like it or not). It should do what they expect.

The subtlety is only useful to the Marketplace app which, as a necessarily certified app should have access to lower-level capabilities and shouldn't impose a subtle distinction to *all* other apps which do not care about the difference.


What's the good mailing-list to talk about these issues?
Where did previous discussion occur so I can catch up?
Marketplace needs to tell "doesn't have permission to use" from "not implemented". It wants to offer applications to the user which requires access to APIs that the marketplace "doesn't have permission to use", but it wants to hide applications which requires access to APIs that are "not implemented".

If we make these properties return null to indicate "doesn't have permission to use", and make the property not be there to indicate "not implemented" that means that webpages can use:

if (navigator.mozSMS) {
  ...
}

to test if they can use the API.

This seems like the best of all worlds: Application developers get to use the simplest possible code to do feature detection, and marketplace can write code that works in all browsers.
(In reply to Jonas Sicking (:sicking) from comment #20)
> if (navigator.mozSMS) {
>   ...
> }
> 
> to test if they can use the API.
> 
> This seems like the best of all worlds: Application developers get to use
> the simplest possible code to do feature detection
The concern I have isn't about "the simplest". It's about what developers do in their code, what code they're writing on a daily basis, convention they use and expectations they have from them.

Currently [1][2][3][4][5] (it took me 5 minutes googling to find that. I can spend an hour to find another hundred instances if that can be more convincing), web developers write:
    if('querySelector' in document){}
and they expect that in the then-clause, it works like they expect (that is document.querySelector is the standard function). If you're defining the property and return null, then you're breaking the expectation of current web developers.


> and marketplace can write code that works in all browsers.
The word "browser" here seems inappropriate. Currently in Firefox, if a feature is implemented and pref'ed off, you don't return null, do you? And for very good reasons actually. Namely that people use if('x' in bla) and that you can't break people's code.
So at least, the marketplace wouldn't work in Firefox. When I turn the ECMAScript features off in Chrome, window.WeakMap returns undefined. So Chrome doesn't work with the marketplace convention.
So the marketplace app will work everywhere the FirefoxOS-specific convention is followed.

The decision being made here is based on a single certified app (it has to be certified to enable installing other apps, doesn't it? Or *at least* privileged), creates a new convention no one knows about, is part of no standard (should at least be discussed on public-script-coord), no one is used to and that will break the expectation of current JS developers.
I hardly believe this is the best of both worlds.

If you believe that the web is the platform, please respect current conventions on the web (if('x' in bla)) and don't invent new convention that go *against* the current practice.
Please provide a privileged/certified-only API. You have the info in the source code anyway [6]; the info you're basing the decision of returning null on. Just expose it in a more legitimate fashion for the marketplace app (and other discover-related apps).


[1] http://dailyjs.com/2010/10/07/framework-part-33/
[2] https://github.com/BBC-News/prototyping/blob/2b5dd148274f5697a3eb07f60a5d1f97e1b2e28d/JS_Shuffle_MarkMcDonnell/index.html#L80
[3] http://decadecity.net/blog/2013/02/06/feature-detection-for-jquery-2
[4] http://remysharp.com/2013/04/19/i-know-jquery-now-what/
[5] http://stackoverflow.com/a/3950950
[6] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm
(In reply to David Bruant from comment #21)
> Currently [1][2][3][4][5] (it took me 5 minutes googling to find that. I can
> spend an hour to find another hundred instances if that can be more
> convincing), web developers write:
>     if('querySelector' in document){}
> and they expect that in the then-clause, it works like they expect (that is
> document.querySelector is the standard function).

I completely agree that there are lots of people that do testing using "if ('foo' in bar)". However, as demonstrated by [1] that you link to, people often do a lot of additional tests too. And [1] also seems to say that jQuery uses "if (bar.foo)" to test if querySelectorAll is implemented.

So clearly people are doing different things to test if a feature exists.

If we make the property not exist when a given page doesn't have security access to an API, then how should we enable marketplace to detect if a feature is implemented or not? Adding a hasFeature API seems like a pretty bad alternative, no?


> > and marketplace can write code that works in all browsers.
> The word "browser" here seems inappropriate. Currently in Firefox, if a
> feature is implemented and pref'ed off, you don't return null, do you? And
> for very good reasons actually. Namely that people use if('x' in bla) and
> that you can't break people's code.
> So at least, the marketplace wouldn't work in Firefox. When I turn the
> ECMAScript features off in Chrome, window.WeakMap returns undefined. So
> Chrome doesn't work with the marketplace convention.
> So the marketplace app will work everywhere the FirefoxOS-specific
> convention is followed.

You completely lost me here. I don't know what you are assuming that marketplace does or would do.

> The decision being made here is based on a single certified app (it has to
> be certified to enable installing other apps, doesn't it? Or *at least*
> privileged),

No. Any website can install apps.

Though only the mozilla marketplace (website or app) can install privileged apps, I.e. apps that has access to APIs that normal websites doesn't, which I suspect is what matters here.

> creates a new convention no one knows about,

"if (foo.bar)" is hardly "a new convention no one knows about". As demonstrated in [1], jQuery uses it for at least some of its feature detection.

I certainly would prefer both "if ('foo' in bar)" and "if (bar.foo)" to work. That way we don't have to debate which is more common. However I also prefer not to repeat mistakes of the past by introducing hasFeature style APIs.

> If you believe that the web is the platform,

Please lets assume good faith here.
(In reply to Jonas Sicking (:sicking) from comment #22)
> > If you believe that the web is the platform,
> 
> Please lets assume good faith here.

You're right. I apologize for that comment.


> If we make the property not exist when a given page doesn't have security
> access to an API, then how should we enable marketplace to detect if a
> feature is implemented or not? Adding a hasFeature API seems like a pretty
> bad alternative, no?
null VS undefined is already an hasFeature API (more at the end). It's just ad-hoc. How much worse could it be to make it a more explicit function that returns true/false? There'd be the exact same level of (in)accuracy.


> > > and marketplace can write code that works in all browsers.
> > The word "browser" here seems inappropriate. Currently in Firefox, if a
> > feature is implemented and pref'ed off, you don't return null, do you? And
> > for very good reasons actually. Namely that people use if('x' in bla) and
> > that you can't break people's code.
> > So at least, the marketplace wouldn't work in Firefox. When I turn the
> > ECMAScript features off in Chrome, window.WeakMap returns undefined. So
> > Chrome doesn't work with the marketplace convention.
> > So the marketplace app will work everywhere the FirefoxOS-specific
> > convention is followed.
> 
> You completely lost me here. I don't know what you are assuming that
> marketplace does or would do.
The marketplace needs to be able to differentiate cases where something is "implemented but not available" and "not implemented".
I was trying to explain that today, browsers (both Chrome and Firefox, but I assume others do the same) ship features hidden behind a flag. These features don't appear as "null"; the properties they're supposed to expose are just absent. I don't think the current use case justifies shifting away from that model.


> > The decision being made here is based on a single certified app (it has to
> > be certified to enable installing other apps, doesn't it? Or *at least*
> > privileged),
> 
> No. Any website can install apps.
But "any website" can only install apps that have the minimum amounts of rights (unless it recently changed?), so the website regardless of its knowledge of the device capabilities can't help the user in installing app with higher-privilege.
 

> Though only the mozilla marketplace (website or app) can install privileged
> apps, I.e. apps that has access to APIs that normal websites doesn't, which
> I suspect is what matters here.
Indeed.
If both the Mozilla Marketplace website and app have enough authority to install privilege/certified app, then, they have enough authority to feature test without having to expose null in non-privileged contexts (worst case, they can install a hidden(?) feature-detecting/test app to get whatever info they need about the device capabilities).
Hmm... It sounds like a good idea actually. It would allow more flexibility on the Marketplace end than if a feature is advertised as "null" in non-privileged contexts (which some forks or some updates will mess up at some point).


> > creates a new convention no one knows about,
> 
> "if (foo.bar)" is hardly "a new convention no one knows about". As
> demonstrated in [1], jQuery uses it for at least some of its feature
> detection.
The new convention I was referring to was "null means implemented by not available". This is new and goes against the current practice of what happens when an feature is in the browser but can't be used.
 
> I certainly would prefer both "if ('foo' in bar)" and "if (bar.foo)" to
> work. That way we don't have to debate which is more common.
My concern isn't about which is the more common, but rather that the "in" pattern is common. TBH, I feature-detect with if(foo.bar) or even if(typeof foo.bar === 'function'). But I fear the "in" practice is very common, people will write that out of habit and they're code will break here and there, because because the principle of least surprised wasn't followed.


> However I also prefer not to repeat mistakes of the past by introducing
> hasFeature style APIs.
I would be interested to know what difference you see between if(navigator['sms'] !== null) and if(navigator.hasFeature('sms')). They both return a boolean based on a label.
Historically, hasFeature has been a mess because the use case and semantics was underdefined. You will have the same issue with exposing null here and there.
Dialog from a future where 'null' may mean "implemented for higher-privilege contexts":
- It returns 'null', I thought it meant the feature is supported in privileged contexts?
- It does, but for that device, for version FFxOS 2.5.6 shipped by device maker Y, subfeature X isn't available.

Anyway, I agree with you, let's not repeat the mistakes from the past ;-)
(In reply to David Bruant from comment #23)
> > > creates a new convention no one knows about,
> > 
> > "if (foo.bar)" is hardly "a new convention no one knows about". As
> > demonstrated in [1], jQuery uses it for at least some of its feature
> > detection.
> The new convention I was referring to was "null means implemented by not
> available". This is new and goes against the current practice of what
> happens when an feature is in the browser but can't be used.

hasFeature is also a new convention though.

> > I certainly would prefer both "if ('foo' in bar)" and "if (bar.foo)" to
> > work. That way we don't have to debate which is more common.
> My concern isn't about which is the more common, but rather that the "in"
> pattern is common. TBH, I feature-detect with if(foo.bar) or even if(typeof
> foo.bar === 'function'). But I fear the "in" practice is very common, people
> will write that out of habit and they're code will break here and there,
> because because the principle of least surprised wasn't followed.

I'm not terribly worried about this. People do what works. If we use the "return null" convention people will very quickly discover that using "in" for detecting sms availability won't work.

> > However I also prefer not to repeat mistakes of the past by introducing
> > hasFeature style APIs.
> I would be interested to know what difference you see between
> if(navigator['sms'] !== null) and if(navigator.hasFeature('sms')). They both
> return a boolean based on a label.
> Historically, hasFeature has been a mess because the use case and semantics
> was underdefined. You will have the same issue with exposing null here and
> there.
> Dialog from a future where 'null' may mean "implemented for higher-privilege
> contexts":
> - It returns 'null', I thought it meant the feature is supported in
> privileged contexts?
> - It does, but for that device, for version FFxOS 2.5.6 shipped by device
> maker Y, subfeature X isn't available.

This discussion will be exactly the same if you change "It returns 'null'" to "hasFeature returns true".

Issues like buggy or half-implemented APIs simply will mean broken content, or content having to detect and work around those bugs/half-implementations. I think that issue is out-of-scope for the discussions that we're having here.

> Anyway, I agree with you, let's not repeat the mistakes from the past ;-)

The main advantage that I see with hasFeature is that it's more explicit and there's lower risk that someone will use an 'in' test wrong.

The main advantage that I see with returning 'null' rather than make the property go away is that it's a lot harder to make a property go away than to have its getter return null.

I guess I'm ok with adding hasFeature and only expose it to the marketplace website/app for now. Though someone will have to step up and do the engineering to make it work since it's non-trivial.
(In reply to Jonas Sicking (:sicking) from comment #24)
> The main advantage that I see with returning 'null' rather than make the
> property go away is that it's a lot harder to make a property go away than
> to have its getter return null.

For what is worth, for JS implemented APIs, it's not hard at all, all you have to do is set the dom.navigator-property.disable.{propertyName} pref in the platforms that should not have that enabled. See bug 863704.
No, the issue here isn't that the property should go away on the platforms where it's not supported. The issue is that the property should go away in the windows which don't have access to to use the API.

So it's not a per-build thing. It's a runtime per-window thing.
> The issue is that the property should go away in the windows which don't have access to
> to use the API.

This is pretty easy in a WebIDL binding, fwiw.  Use https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D and the function will get the prototype object involved passed in, from which you can get the global (that is the window) and do whatever permissions checks you need on that

The same mechanism can be used to expose hasFeature or whatnot to the marketplace app, as long as it's hanging off a WebIDL object.  Or if it's a static WebIDL function on a WebIDL interface, of course; we could even add a WebIDL interface just for this and only expose it to the marketplace app.
Depends on: 838146
Depends on: 908099
So all of the cases in comment 0 works "fine" now.  What's left to do in this bug?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.