Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Firefox WebExtensions misdetected as Chrome Packaged Apps #16068

Open
1 of 3 tasks
robertknight opened this issue Jun 23, 2017 · 9 comments
Open
1 of 3 tasks

Firefox WebExtensions misdetected as Chrome Packaged Apps #16068

robertknight opened this issue Jun 23, 2017 · 9 comments

Comments

@robertknight
Copy link

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:

In the current version of Firefox WebExtensions, window.chrome and window.chrome.runtime exist but window.chrome.app does not. A check in $sniffer misdetects the application as a Chrome Packaged App. This in turn disables the HTML 5 History API support in the $location service causing unexpected behaviour elsewhere. In the case of our app it caused the $location service to try and redirect the browser to a hash-bangified version (/path/app.html => #!/path/app.html) of the URL on startup, breaking the app.

Expected / new behavior:

The isChromePackagedApp check referenced above should return false in an HTML page loaded from a Firefox WebExtension.

Angular version: 1.5.11
Browser: [Firefox 51]

Other information

A workaround for Firefox WebExtension authors is to set window.chrome.app to a dummy object before Angular bootstraps so that the check does not produce a false positive.

robertknight added a commit to hypothesis/client that referenced this issue Jun 23, 2017
Angular has a check for Chrome Packaged Apps in its `$sniffer` service
which changes the behavior of the $location service and in the Firefox
extension, triggers a redirect from `/client/app.html` to
`/#client/app.html` on startup, breaking the sidebar app.

This commit works around the problem by adding a fake
`window.chrome.app` object to convince Angular that the Firefox
extension is not a Chrome Packaged App.

Issue reported upstream at angular/angular.js#16068
@frederikprijck
Copy link
Contributor

frederikprijck commented Jun 23, 2017

Hi @robertknight,

As you tested it with AngularJS 1.5.11, could you verify the issue also occurs with 1.6.4 ?
A change was made regarding the sniffer, which was released in 1.6.2 (see: 7019d9f ).

I doubt that change is solving your problem, but it's always interesting to know whether or not the behavior is different.

@robertknight
Copy link
Author

Thank-you for the quick response. The isChromePackagedApp expression from the master branch of this repo still incorrectly evaluates to a truthy value in Firefox, including in 1.6.4, so the issue still applies.

@frederikprijck
Copy link
Contributor

frederikprijck commented Jun 23, 2017

Unless you want to provide a PR, I wouldn't mind creating one myself.

Any idea how we should be validating this correctly ?
/cc @gkalpak @Narretz

@Narretz
Copy link
Contributor

Narretz commented Jun 23, 2017

If window.chrome.app isn't true in a FF web extension, how can the check in the sniffer for packaged apps be true? It specifically checks for window.chrome.app

edit: I didn't see the second window.chrome.app check is reversed with !

@frederikprijck
Copy link
Contributor

frederikprijck commented Jun 23, 2017

If I get this straight, the following is happening:

isChromePackagedApp =
            !isNw &&
            $window.chrome &&
            ($window.chrome.app && $window.chrome.app.runtime ||
                !$window.chrome.app && $window.chrome.runtime && $window.chrome.runtime.id),

Which evaluates to

isChromePackagedApp =
            true &&
            true &&
            (false && true ||
                true && true && true),

Which results in true.

The change @robertknight made in the referenced PR in his repo (see https://github.com/hypothesis/client/pull/460/files) changes this so that window.chrome.app is overridden and no runtime property is available on it which will evaluate to:

isChromePackagedApp =
            true &&
            true &&
            (true && false ||
                false && false&& false),

Which will evaluate to false and fixes the issue for him.

@robertknight
Copy link
Author

If I get this straight, the following is happening:

You've got it right.

If you're able to create a PR that would be great. A question I have is whether it is necessary to explicitly test for Chrome packaged apps / NW.js instead of feature-detecting the availability of the history APIs. According to this comment, window.history.{pushState, replaceState} are not defined in Chrome Apps. I'm not certain if that is the case & always has been.

@frederikprijck
Copy link
Contributor

frederikprijck commented Jun 23, 2017

Good question, this is the second time an issue like this arises (5 months ago the one for NW apps was fixed).

Maybe @gkalpak or @Narretz have an idea on why it's being checked this way instead of feature detection.

I guess it has to do with:

// Chrome Packaged Apps are not allowed to access `history.pushState`.
// If not sandboxed, they can be detected by the presence of `chrome.app.runtime`
// (see https://developer.chrome.com/apps/api_index). If sandboxed, they can be detected by
// the presence of an extension runtime ID and the absence of other Chrome runtime APIs
// (see https://developer.chrome.com/apps/manifest/sandbox).
// (NW.js apps have access to Chrome APIs, but do support `history`.)

But my knowlegde of that is limited (if existing).

@Narretz Narretz added this to the Backlog milestone Jun 24, 2017
@Narretz
Copy link
Contributor

Narretz commented Jun 29, 2017

IIRC the problem is that Chrome Packaged Apps will do something bad when you try to access history.pushState.

@gkalpak
Copy link
Member

gkalpak commented Jun 29, 2017

Right. You can find more details in #13945.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants