Closed Bug 1245571 Opened 8 years ago Closed 8 years ago

Access to the add-ons installed

Categories

(Toolkit :: Add-ons Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: andy+bugzilla, Assigned: mossop)

References

Details

(Keywords: privacy)

Attachments

(2 files)

Its been requested on AMO that we show if an add-on is already installed. Some recent UX mocks for suggested changes to the first run flow for add-ons also request a similar sort of flow.

As an example, if you have an addon installed in Chrome and visit the Chrome store the button changes to "Added to Chrome" from "Add to Chrome".

I can see why this might not have been done, perhaps for fingerprinting and privacy reasons, but I couldn't find any bugs saying why we don't do that. I'd like to see if we can explore doing this in a way that doesn't expose users information.
See bug 491562
I've read through the comments in bug 491562, and would still like to make this happen.

There's a few things I saw in that bug:

* Concern that there's too much code to be maintained. I don't think that's the case unless we include UI and I don't think we need that. 

* Conversely it will likely mean more code in Firefox, because to implement some of the suggestions from product and UX, we would need to move code into Firefox, instead of serving it from AMO.

* There was some suggestion that AMO should have an Add-on for this, that doesn't seem like a great interaction for a user and 7 years later, that hasn't happened. Certainly for an improved first run discovery, it doesn't make sense.

I'd like to suggest that in the first phase, we implement the two API's mentioned in bug 491562 comment 0, but limit them to only *.mozilla.org (or maybe down to *.addons.mozilla.org) domains. The ability of those sites to get the add-ons is within the privacy policy. 

The opening up of that API to other domains, and the corresponding UI that would be required, would be a separate bug and out of scope for this bug.
(In reply to Andy McKay [:andym] from comment #2)
> I'd like to suggest that in the first phase, we implement the two API's
> mentioned in bug 491562 comment 0, but limit them to only *.mozilla.org (or
> maybe down to *.addons.mozilla.org) domains. The ability of those sites to
> get the add-ons is within the privacy policy. 

I would argue for a better API and at the least not implement one that gives you a simple call to retrieve all add-ons but instead you would have to lookup individual IDs which we could if we wanted rate limit so it's impossible to brute force a full add-on list.

I also have some ideas for better API support for the install process. I can throw something together.
(In reply to Dave Townsend [:mossop] from comment #3)
> I would argue for a better API and at the least not implement one that gives
> you a simple call to retrieve all add-ons but instead you would have to
> lookup individual IDs which we could if we wanted rate limit so it's
> impossible to brute force a full add-on list.

I can see uses for retrieving the list of add-ons, like the SUMO use case and being able to list add-ons on AMO. But I will admit it doesn't meet any project goals I've currently got, so I'm happy to not worry about that scenario right now. My only concern there is perf, would one API call be any different from say 30 API calls?

> I also have some ideas for better API support for the install process. I can
> throw something together.

Sounds good.
Blocks: 1245993
(In reply to Andy McKay [:andym] from comment #4)
> My only concern there is perf, would one API call be any
> different from say 30 API calls?

The former is O(1) i.e. independent of the number of installed add-ons AFAIK. The latter is O(n) i.e. approximately proportional to the number of installed add-ons, which is a different order of magnitude. At some number of installed add-ons the performance degradation will necessarily start to be (subjectively) felt, but at how many will depend on the user's patience as well as on his/her computer and connection "speed". If the estimated threshold is 100 we can probably ignore it; if it's 5 we probably can't.
(In reply to Tony Mechelynck [:tonymec] from comment #6)
> (In reply to Andy McKay [:andym] from comment #4)
> > My only concern there is perf, would one API call be any
> > different from say 30 API calls?
> 
> The former is O(1) i.e. independent of the number of installed add-ons
> AFAIK. The latter is O(n) i.e. approximately proportional to the number of
> installed add-ons, which is a different order of magnitude. At some number
> of installed add-ons the performance degradation will necessarily start to
> be (subjectively) felt, but at how many will depend on the user's patience
> as well as on his/her computer and connection "speed". If the estimated
> threshold is 100 we can probably ignore it; if it's 5 we probably can't.

There's a lot of assumptions here about how the API will work that I don't think necessarily hold. Given that there will be no file or network I/O going on I think it's safe to say that the performance implications will be minimal, particularly since the mockups I saw suggest we only need to look up a dozen or so add-ons.
Priority: -- → P1
Assignee: nobody → dtownsend
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/1-2/
Attachment #8722070 - Flags: feedback?(rhelmer)
I'm not sure who gets to sign off on APIs exposed to webpages these days, Johnny, can you direct me to someone?
Flags: needinfo?(jst)
https://reviewboard.mozilla.org/r/35857/#review32497

Looks ok to me in principle, need someone not-me to sign off on webidl and exposing it :)

::: dom/webidl/InstallTrigger.webidl:94
(Diff revision 2)
> +   * @deprecated use "install" in the future.

What does this @deprecated note mean?

::: toolkit/mozapps/extensions/addonManager.js:35
(Diff revision 2)
> +const { console } = Cu.import("resource://gre/modules/Console.jsm", {});

where is this used?

::: toolkit/mozapps/extensions/addonManager.js:186
(Diff revision 2)
> +        let resolved = false;

discussed in IRC already - if we're using real promises now this check can go away
Attachment #8722070 - Flags: feedback?(rhelmer) → feedback+
This API just returns True of False? Would be cool if it could return a data structure, maybe containing the addon version? That way AMO could possibly show an upgrade option. Bug 491562 comment 0 "had GUID, type, status (userEnabled/Disabled, etc), and version".

I think we might need to allow the dev and stage domains (for testing) and services.addons.mozilla.org because that's the default domain for the discovery pane?
(In reply to Andy McKay [:andym] from comment #12)
> This API just returns True of False? Would be cool if it could return a data
> structure, maybe containing the addon version? That way AMO could possibly
> show an upgrade option. Bug 491562 comment 0 "had GUID, type, status
> (userEnabled/Disabled, etc), and version".

It depends how much we're concerned about AMO being able to fingerprint users.

> I think we might need to allow the dev and stage domains (for testing) and
> services.addons.mozilla.org because that's the default domain for the
> discovery pane?

We can enable that in a hidden pref or something, I wouldn't enable it generally.
(In reply to Dave Townsend [:mossop] from comment #13)
> It depends how much we're concerned about AMO being able to fingerprint
> users.

AMO I'm not worried about, since we could probably already do this but don't.

> We can enable that in a hidden pref or something, I wouldn't enable it
> generally.

Sounds good. I think services.addons.mozilla.org will need to be enabled out of the box (and the similar for dev and stage).
Apologies for the delay in response here, been on the road and unable to keep up with email. As for exposing more of an extension related API surface to the web I'm generally not in favor of doing that as this is not something webpages in general should ever need, this is specific to AMO AFAICT. Cc:ing ehsan, bz, and overholt who should help figure out specifics here, and if we do move ahead and expose more addon info to web pages, we should make sure the security folks are ok with that, so cc:ing dveditz and sworkman as well.
Flags: needinfo?(jst)
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #15)
> Apologies for the delay in response here, been on the road and unable to
> keep up with email. As for exposing more of an extension related API surface
> to the web I'm generally not in favor of doing that as this is not something
> webpages in general should ever need, this is specific to AMO AFAICT. Cc:ing
> ehsan, bz, and overholt who should help figure out specifics here, and if we
> do move ahead and expose more addon info to web pages, we should make sure
> the security folks are ok with that, so cc:ing dveditz and sworkman as well.

Just to be clear this is for AMO only. If there was anything beyond AMO I would expect a seperate bug and discussion.
There's something similar-ish happening with remote new tab, in that higher privileges are being given to a specific hosted page (primarily access to the user's history). However, we're requiring that page to be signed, and it will have CSP and SRI enforced on it. Restricting the API to a specific hostname might not be enough. 

There's also the complexity of managing these special permissions on a per-site basis. How many more sites like this are we going to be exposing APIs to?

Dan, Richard, what do you think here?
Flags: needinfo?(rlb)
Flags: needinfo?(dveditz)
I don't have a problem with AMO having this information in principal.

I have a couple of concerns with exposing this through the permission mechanism, although that's better than hard-coding domains to be sure.

  1. Permissions make it very easy for add-ons to add their associated sites to this
     feature, but unlike prefs--pretty darn hidden to start with--there is no way for a
     user to know about or edit these permissions. They could be silently probed on every
     visit. [Add-ons could directly gather this info then phone home which is about the
     same, but slightly more likely to be discovered via network traffic or code review.]

  2. the Permission API supports any origin. This is sensitive data and needs to be
     restricted to "secure contexts". Until we have the secure context flag to check
     this could be satisfied by adding an "is scheme https?" check on top of the
     permission check, later replaced by checking the secure context flag.

One possible resolution to #1 would be to overload the existing install permission. Kind of makes sense that if a site can install it can check versions of those things, and there is already existing UI for that particular permission. On the down side people may grant this willy-nilly not realizing the full extent of what they're granting. Tough call.

On the other hand if it's only ever going to be "The official Firefox Add-on Site" then a pref with a single origin in it might be best. (needs to be something like a pref so other distributions--enterprise, Tor browser--can customize the "one-true store"). Even with a pref it doesn't hurt to make the 'secure context' check to prevent mistakes.

This is sensitive data, but I don't believe it rises to the level of the remote new tab page such that it requires content signing. Connecting to a secure site with cert pinning (which AMO has) is plenty. It's not worth a check for cert pinning because any potentially malicious site could just set a HPKP header, but it's a nice warm fuzzy for knowing that our initial default is less likely to suffer from DNS spoofing.

A completely different approach, used by the UITour I believe, would be to forego an "api" and instead fire events. The browser itself checks whether the event is allowed from that context and then postMessage()s the info back to the page. It doesn't solve any privacy or preference/permission-managing issues, it just means that the API itself doesn't appear in the DOM. But since we already have the non-standard InstallTrigger to hang this off I guess that isn't much benefit.
Flags: needinfo?(dveditz)
Keywords: privacy
From the API exposure point of view the event approach is somewhat better in that it completely avoids any ability for websites to become dependent even on the existence of the API, even if they can't use it, even if it hangs off of our non-standard InstallTrigger API. You'd be surprised what sites have used to feature detect browsers with over the years, and that stuff is hard to back out of... But my primary concern is the security aspects of exposing this information, which I'll let the security guys settle on.
(In reply to Daniel Veditz [:dveditz] from comment #18)
> I don't have a problem with AMO having this information in principal.
> 
> I have a couple of concerns with exposing this through the permission
> mechanism, although that's better than hard-coding domains to be sure.
> 
>   1. Permissions make it very easy for add-ons to add their associated sites
> to this
>      feature, but unlike prefs--pretty darn hidden to start with--there is
> no way for a
>      user to know about or edit these permissions. They could be silently
> probed on every
>      visit. [Add-ons could directly gather this info then phone home which
> is about the
>      same, but slightly more likely to be discovered via network traffic or
> code review.]
> 
>   2. the Permission API supports any origin. This is sensitive data and
> needs to be
>      restricted to "secure contexts". Until we have the secure context flag
> to check
>      this could be satisfied by adding an "is scheme https?" check on top of
> the
>      permission check, later replaced by checking the secure context flag.

The permission manager does now support scheme checks at least but I don't know if more security check would be forthcoming anytime soon.

> One possible resolution to #1 would be to overload the existing install
> permission. Kind of makes sense that if a site can install it can check
> versions of those things, and there is already existing UI for that
> particular permission. On the down side people may grant this willy-nilly
> not realizing the full extent of what they're granting. Tough call.

Yeah I didn't want to use the existing permission because I know people have already granted it to places. Maybe they should be the same but I'd rather not open this up more automatically.

> On the other hand if it's only ever going to be "The official Firefox Add-on
> Site" then a pref with a single origin in it might be best. (needs to be
> something like a pref so other distributions--enterprise, Tor browser--can
> customize the "one-true store"). Even with a pref it doesn't hurt to make
> the 'secure context' check to prevent mistakes.

Yeah maybe that's best. Sounds like there are a couple of domains (addons.mozilla.org and services.addons.mozilla.org) but since we also want to be able to do QA on dev sites just using prefs probably makes that more straightforward.

Other than https, what constitutes a "secure context" here?
> From the API exposure point of view the event approach is somewhat better in that it completely avoids any
> ability for websites to become dependent even on the existence of the API, even if they can't use it

Note that we can just not expose an API to sites that can't use it, as long as we can synchronously check whether a site can use the API.
This is true... Though the less code like that we need, the happier we'll be I think.
(In reply to Boris Zbarsky [:bz] from comment #21)
> > From the API exposure point of view the event approach is somewhat better in that it completely avoids any
> > ability for websites to become dependent even on the existence of the API, even if they can't use it
> 
> Note that we can just not expose an API to sites that can't use it, as long
> as we can synchronously check whether a site can use the API.

Can you elaborate on this? There are a few other AMO specific things we're looking to do so it might be nice to do this. Is this something through webidl?
Flags: needinfo?(bzbarsky)
> Can you elaborate on this?

See <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func>, so yes, something through webidl.
Flags: needinfo?(bzbarsky)
(In reply to Dave Townsend [:mossop] from comment #20)
> (In reply to Daniel Veditz [:dveditz] from comment #18)
> >   2. the Permission API supports any origin. This is sensitive data and
> >   needs to be restricted to "secure contexts".
> 
> The permission manager does now support scheme checks at least but I don't
> know if more security check would be forthcoming anytime soon.

We would, of course, correctly only use https: as we do with the current install permission: https://mxr.mozilla.org/mozilla-central/source/browser/app/permissions#16

I'm not worried about us, I'm worried about other add-ons or distributions that might tweak this permission and not be very smart about it. Allowing it for any http: origin means allowing it for every local network attacker. At some point the code will call testPermission(uri, "addon-info"). Instead it should do testPermission(uri,"addon-info") && uri.SchemeIs("https").
Blocks: 1255039
Since we're going to be implementing a few different functions for AMO to use I've decided to switch to implementing this as a standalone interface that we only expose on the sites we care about.
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/2-3/
Attachment #8722070 - Attachment description: MozReview Request: Bug 1245571: Allow specific websites to be able to query whether add-ons are installed. f?rhelmer → MozReview Request: Bug 1245571: Add a window.navigator.AddonManager object that can only be accessed by AMO. r?bz
Attachment #8722070 - Flags: feedback+ → review?(bzbarsky)
This adds a bunch of structure supporting a promise-based API on the
AddonManager object that is exposed to webpages and adds the first example,
getAddonByID.

Review commit: https://reviewboard.mozilla.org/r/39267/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39267/
Attachment #8729153 - Flags: review?(rhelmer)
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

https://reviewboard.mozilla.org/r/35857/#review35957

::: dom/bindings/Bindings.conf:87
(Diff revision 3)
> +'AddonManager': {

I'd somewhat prefer we annotate this in the IDL directly.  See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#HeaderFile

::: dom/webidl/AddonManager.webidl:6
(Diff revision 3)
> +[ChromeOnly, JSImplementation="dummy"]

Worth adding a comment that says that the actual string doesn't matter because we don't have a constructor.  And maybe filing a followup to allow JSImplementation with not contractid if there is no constructor...

::: dom/webidl/AddonManager.webidl:26
(Diff revision 3)
> +[Func="AddonManagerWebAPI::IsAPIEnabled", NavigatorProperty="AddonManager",

Should the NavigatorProperty be "addonManager" instead?  I guess it doesn't matter too much, and making it start with a capital makes it less likely that it will collide with something someone adds to the platform later...

We could consider calling it mozAddonManager to avoid that last problem, of course.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:18
(Diff revision 3)
> +AddonManagerWebAPI::IsValidSite(nsPIDOMWindowOuter* win) {

Style guide at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style says curly on next line at start of function.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:36
(Diff revision 3)
> +  if (host.Equals("addons.mozilla.org") ||

Do we do cert pinning for those two hosts?  I assume we do...

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:41
(Diff revision 3)
> +  if (Preferences::GetBool("extensions.webapi.testing", false)) {

This one is a bit of a worry for me because someone could flip the pref and we do _not_ do cert pinning for the things on this list.  But maybe the attack scenario is too improbable (in that if you can get people to flip prefs you can get all the info you'd get out of addonmanager anyway)?

Anyway, this part could use a review by the security team.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:55
(Diff revision 3)
> +AddonManagerWebAPI::IsAPIEnabled(JSContext* cx, JS::Handle<JSObject*> obj) {

Curly on next line.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:58
(Diff revision 3)
> +    return false;

So this means window.AddonManager will never exist.  That might be ok.  Alternately, you could try to check for a window here and if found use that.  It probably doesn't matter that much, and the way you have it _does_ keep the code simple.  Maybe just document that you don't care about window.AddonManager existing?

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:61
(Diff revision 3)
> +  nsCOMPtr<nsPIDOMWindowOuter> win = nav->GetWindow()->GetOuterWindow();

This doesn't work right.

Specifically, consider a site that opens a new window to one of its own urls, then grabs the Navigator object from that window, then navigates the window to <https://addons.mozilla.org>.  Now it does navigator.AddonManager on the navigator object it has.

This code will grab the outer window, look at the URI of its current document (that being <https://addons.mozilla.org>) and allow the access.

I'd like to understand the exact policy we're trying to enforce by this walk up the parent chain (and why we're trying to enforce it) before proposing a specific implementation strategy for that part.

::: toolkit/mozapps/extensions/amWebAPI.js:22
(Diff revision 3)
> +    return this.window.Promise.reject("Not yet implemented");

This has the drawback that if the API is used in a no-longer-current tab it will get a Promise from a different global.  But that should be OK, hopefully...  Not much we can do about it if we're implementing in JS.  :(
Attachment #8722070 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8722070 [details]
> ::: dom/webidl/AddonManager.webidl:26
> (Diff revision 3)
> > +[Func="AddonManagerWebAPI::IsAPIEnabled", NavigatorProperty="AddonManager",
> 
> Should the NavigatorProperty be "addonManager" instead?  I guess it doesn't
> matter too much, and making it start with a capital makes it less likely
> that it will collide with something someone adds to the platform later...
> 
> We could consider calling it mozAddonManager to avoid that last problem, of
> course.
>

I've switched to mozAddonManager, even though this is only exposed to AMO might as well make sure we can't collide in the future.

> ::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:36
> (Diff revision 3)
> > +  if (host.Equals("addons.mozilla.org") ||
> 
> Do we do cert pinning for those two hosts?  I assume we do...

If I'm reading this correctly then yes. If not then we really should fix that: https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/StaticHPKPins.h#661

> ::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:41
> (Diff revision 3)
> > +  if (Preferences::GetBool("extensions.webapi.testing", false)) {
> 
> This one is a bit of a worry for me because someone could flip the pref and
> we do _not_ do cert pinning for the things on this list.  But maybe the
> attack scenario is too improbable (in that if you can get people to flip
> prefs you can get all the info you'd get out of addonmanager anyway)?
> 
> Anyway, this part could use a review by the security team.

It was the best way I could come up with for having a way for QA and developers test AMO changes without needing a custom build of Firefox.

> ::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:61
> (Diff revision 3)
> > +  nsCOMPtr<nsPIDOMWindowOuter> win = nav->GetWindow()->GetOuterWindow();
> 
> This doesn't work right.
> 
> Specifically, consider a site that opens a new window to one of its own
> urls, then grabs the Navigator object from that window, then navigates the
> window to <https://addons.mozilla.org>.  Now it does navigator.AddonManager
> on the navigator object it has.
> 
> This code will grab the outer window, look at the URI of its current
> document (that being <https://addons.mozilla.org>) and allow the access.
> 
> I'd like to understand the exact policy we're trying to enforce by this walk
> up the parent chain (and why we're trying to enforce it) before proposing a
> specific implementation strategy for that part.

So I'm trying to ensure that if a page embeds AMO in an inner frame that it can't get hold of this API. AMO has X-Frame-Options headers that are meant to stop that from ever happening anyway but if something were to cause that header not to be sent I wanted to add protection in the client code. Walking up the frames was the best way I could think of.

If there are better checks I can do here I'm happy to implement them.
Flags: needinfo?(dveditz)
Flags: needinfo?(bzbarsky)
> It was the best way I could come up with for having a way for QA and
> developers test AMO changes without needing a custom build of Firefox.

Right, I understand the intent.  I just think this needs security review.  Possibly along with a much scarier-sounding preference name...

> If there are better checks I can do here I'm happy to implement them.

I think the right thing to do is check whether we have a docshell.  If we don't, we're torn down and should bail out.  Then we want to check whether our docshell is a subframe.  If not, we just check ourselves.  If it _is_ a subframe, we should go up via GetParentDocument() on the nsIDocument.

And maybe file a bug to have something on inner windows that does the right thing instead of having to reinvent it....
Flags: needinfo?(bzbarsky)
I'm not sure what you're asking me so I'll guess?

> > Do we do cert pinning for those two hosts?  I assume we do...
>
> If I'm reading this correctly then yes. If not then we really should fix that

Yes, we pin "addons.mozilla.org" with the "includeSubdomains" flag set.

We don't pin on the list of test domains (and we won't have the right cert for example.com in any case) but I'm not too worried about that as an attack vector. An attacker would have to target Firefox testers (or find a way to flip this pref) and conduct a TLS MITM attack and from all that only gains the ability to gather some add-on info. Seems unlikely: if you have the ability to construct valid MITM certs why waste it on Mozilla test domains; if you have a local vector to flip the pref you can inspect the installed addons directly. This is a reasonable tradeoff to gain testing ability.
Flags: needinfo?(dveditz)
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/3-4/
Attachment #8722070 - Flags: review?(bzbarsky)
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39267/diff/1-2/
Attachment #8722070 - Flags: review?(bzbarsky)
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

https://reviewboard.mozilla.org/r/35857/#review40101

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:82
(Diff revision 4)
> +    if (!IsValidSite(win->GetDocumentURI())) {
> +      return false;
> +    }
> +
> +    nsCOMPtr<nsIDocShellTreeItem> parent;
> +    nsresult rv = docShell->GetSameTypeParent(getter_AddRefs(parent));

This is trying to check for "toplevel frame"?  If so, probably better is to get the scriptable top and compare to what you have; otherwise you run into issues with things like mozbrowser iframes and whatnot, no?

This code should be somewhat better documented to explain why it's considering both sorts of parents.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:91
(Diff revision 4)
> +
> +    if (!parent) {
> +      return true;
> +    }
> +
> +    nsIDocument* doc = docShell->GetDocument();

This will get the _current_ document, which may have nothing to do with your actual document.

This should probably be win->GetDoc() instead.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:98
(Diff revision 4)
> +      return false;
> +    }
> +
> +    doc = doc->GetParentDocument();
> +    if (!doc) {
> +      // There was a parent docshell but no parent document. Can this ever

This can happen if things are torn down enough.  Returning false makes sense.

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp:106
(Diff revision 4)
> +    }
> +
> +    win = doc->GetInnerWindow();
> +  }
> +
> +  return true;

Shouldn't this be return false?  It's a fallback that should not be reached in normal operation, right?
https://reviewboard.mozilla.org/r/35857/#review40101

> This is trying to check for "toplevel frame"?  If so, probably better is to get the scriptable top and compare to what you have; otherwise you run into issues with things like mozbrowser iframes and whatnot, no?
> 
> This code should be somewhat better documented to explain why it's considering both sorts of parents.

I'm trying to walk up every frame to make sure there aren't any non-AMO sites there. Getting the scriptable top could bypass some frames so if someone managed to embed their own site in a frame in AMO and then embedded AMO there might be a way through. Both scriptable top and sameTypeParent don't bypass mozbrowser boundaries but they aren't exposed to the web are they? This needs to work for the case where the extensions manager is in a content page and loads AMO in a XUL iframe type="content"
For comment 36 so I can make sure I'm doing this right...
Flags: needinfo?(bzbarsky)
> I'm trying to walk up every frame to make sure there aren't any non-AMO sites

Yes, but _that_ walk is happening up the GetParentDocument() chain.  The only reason you do the GetSameTypeParent() thing, afaict, is to check "are we the root already?"

In any case, you can use GetScriptableParent() just as well as GetScriptableTop() (and it's probably cheaper when you're not a root...)

> Both scriptable top and sameTypeParent don't bypass mozbrowser boundaries but they aren't exposed to the web are they?

sameTypeParent can totally go through mozbrowser boundaries, iirc.  And I can't speak to whether mozbrowser is exposed to the web, or might become so....

> This needs to work for the case where the extensions manager is in a content page and loads AMO in a XUL iframe type="content"

In that case, you'd end up walking into the extension manager page either way, right?  Or is that no longer loaded in a type="content" <browser>?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #38)
> > Both scriptable top and sameTypeParent don't bypass mozbrowser boundaries but they aren't exposed to the web are they?
> 
> sameTypeParent can totally go through mozbrowser boundaries, iirc.  And I
> can't speak to whether mozbrowser is exposed to the web, or might become
> so....

The IDL comments seem to suggest otherwise: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShellTreeItem.idl#58
> The IDL comments seem to suggest otherwise:

Ah, hmm.  This got changed at some point...  OK, so using GetSameTypeParent() to detect the root of same type is ok.  But please document clearly what you're doing!
Depends on: 1262009
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/4-5/
Attachment #8722070 - Flags: review?(bzbarsky)
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39267/diff/2-3/
Attachment #8722070 - Flags: review?(bzbarsky)
Attachment #8729153 - Flags: review?(rhelmer)
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/5-6/
Attachment #8722070 - Flags: review?(bzbarsky)
Attachment #8729153 - Flags: review?(rhelmer)
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39267/diff/3-4/
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

https://reviewboard.mozilla.org/r/35857/#review40927

r=me
Attachment #8722070 - Flags: review?(bzbarsky) → review+
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

https://reviewboard.mozilla.org/r/39267/#review40895

Looks good to me, I tried this out locally too and works like I'd expect (`window.navigator.mozAddonManager.getAddonByID(${id})` works for AMO, `window.navigator.mozAddonManager` undefined elsewhere).

::: toolkit/mozapps/extensions/AddonManager.jsm:325
(Diff revision 4)
> +    if (typeof(addon[prop]) != "function") {
> +      result[prop] = addon[prop];
> +    }
> +  }
> +
> +  // A few properties are computer for a nicer API

s/computer/computed/
Attachment #8729153 - Flags: review?(rhelmer) → review+
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/6-7/
Attachment #8722070 - Attachment description: MozReview Request: Bug 1245571: Add a window.navigator.AddonManager object that can only be accessed by AMO. r?bz → MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39267/diff/4-5/
I've rebased this to the latest code and now it has stopped working. Is there something that has changed in webidl land? I had to change the function signature to make it compile and it now never manages to unwrap the navigator object in AddonManagerWebAPI::IsAPIEnabled
Flags: needinfo?(bzbarsky)
> Is there something that has changed in webidl land?

Hmm, yes.  Bug 1245650.  That changed NavigatorProperty to just creating a regular IDL attribute on the Navigator interface.  That means, per documentation at <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func>, that the object passed in is the global of the Navigator (i.e. the window), not the Navigator itself.

Since you just want the nsPIDOMWindowInner anyway, and don't care about the Navigator per se, you can use xpc::WindowGlobalOrNull(obj) and if that returns non-null call AsInner() on it to get your nsPIDOMWindowInner.

I just went through all consumers of NavigatorProperty that used Func.  These are:

1) MozWifiManager, just looks at the object principal.
2) PaymentProvider, uses WindowGlobalOrNull.
3) MozWifiP2pManager, just looks at the object principal.
4) MozNFC, uses WindowGlobalOrNull.

so looks like those are all OK with this change.
Flags: needinfo?(bzbarsky)
Comment on attachment 8722070 [details]
MozReview Request: Bug 1245571: Add a window.navigator.mozAddonManager object that can only be accessed by AMO. r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35857/diff/7-8/
Attachment #8729153 - Attachment description: MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r?rhelmer → MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer
Comment on attachment 8729153 [details]
MozReview Request: Bug 1245571: Allow AMO to be able to query details about an add-on. r=rhelmer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39267/diff/5-6/
(In reply to Boris Zbarsky [:bz] from comment #50)
> > Is there something that has changed in webidl land?
> 
> Hmm, yes.  Bug 1245650.  That changed NavigatorProperty to just creating a
> regular IDL attribute on the Navigator interface.  That means, per
> documentation at
> <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func>,
> that the object passed in is the global of the Navigator (i.e. the window),
> not the Navigator itself.
> 
> Since you just want the nsPIDOMWindowInner anyway, and don't care about the
> Navigator per se, you can use xpc::WindowGlobalOrNull(obj) and if that
> returns non-null call AsInner() on it to get your nsPIDOMWindowInner.

Thanks, that solved it. This should be ready to land once bug 1262009 fixes a problem that the tests caught.
Blocks: 1262218
https://hg.mozilla.org/mozilla-central/rev/a01d14eb9caf
https://hg.mozilla.org/mozilla-central/rev/64cba8d398a3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(rlb)
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → qe-verify-
You need to log in before you can comment on or make changes to this bug.