Closed Bug 707836 Opened 13 years ago Closed 13 years ago

Handle URI navigation outside app domain for native apps

Categories

(Firefox Graveyard :: Webapp Runtime, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: anant, Assigned: Mardak)

References

Details

(Whiteboard: [marketplace-beta=])

Attachments

(2 files, 2 obsolete files)

Currently it is possible to navigate outside an app's domain in the generated native apps. These should be handled and opened outside the native app's context, perhaps using the OS level HTTP protocol handler.
There has been work in Firefox to make app tabs behave this way and a patch was landed to make sure that when a user clicks on a link in an app tab that leads to a different domain it will open in a new tab (bug 575561). However, this does not handle the case of navigating to a URI programatically (f.e. by setting window.location.href). Bug #579874 is tracking all the different ways in which an app tab can be "overwritten" which is probably relevant. This bug is to track the implementation of this in the context of a generate native app shell. Bug #598587 indicates that loadURI and loadURIWithFlags might be right place to override this; and one way to do this is via an XBL binding for the <browser> that is used in the XUL app.
Whiteboard: devPreviewNonBlocker
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808725
Assignee: anant → nobody
Blocks: 731054
Component: Extension → General
Product: Web Apps → Firefox
QA Contact: extension → general
Target Milestone: --- → Firefox 14
OS: Mac OS X → All
Hardware: x86 → All
Component: General → Web Apps
QA Contact: general → webapps
Assignee: nobody → anant
Component: Web Apps → General
Product: Firefox → Web Apps
QA Contact: webapps → general
Target Milestone: Firefox 14 → ---
Why was this migrated back to web apps general? Why was it removed from the tracking bug? This issue has to do "using apps" portion of the native apps experience, which is a requirement for the web apps integration into desktop feature.
Assignee: anant → nobody
Component: General → Web Apps
Product: Web Apps → Firefox
QA Contact: general → webapps
Component: Web Apps → Desktop Runtime
Product: Firefox → Web Apps
QA Contact: webapps → desktop-runtime
Blocks: 737571
No longer blocks: 731054
Whiteboard: devPreviewNonBlocker → devPreviewNonBlocker [marketplace-beta?]
Whiteboard: devPreviewNonBlocker [marketplace-beta?] → r [marketplace-beta?]
Whiteboard: r [marketplace-beta?] → [marketplace-beta?]
Don't know what this bug means. It talks about an app tag in comment #1. So to me this bug is irrelevant to this release and if it did have something to do with this release I would think that security would have flagged this behavior as a blocker. I could be missing something, but not a blocker.
Whiteboard: [marketplace-beta?]
(In reply to Jennifer Arguello :ticachica from comment #5) > Don't know what this bug means. It talks about an app tag in comment #1. > > > > So to me this bug is irrelevant to this release and if it did have something > to do with this release I would think that security would have flagged this > behavior as a blocker. I could be missing something, but not a blocker. Actually, security did flag this in the security review (https://wiki.mozilla.org/Security/Reviews/WebRT#Action_Items). See bug 741954 (this is duplicate of that bug). The issue documented here basically is something like this use case: 1. Launch the Forces of War application 2. Click facebook.com link to go facebook.com At this point, you are no longer on "forcesofwargame.com," instead you are on "facebook.com." The issue stated in this bug and associated security review item is that that the default browser needs to start up if this situation occurs. According to the security review action items, it needs to be implemented by ship. Might not be a blocker for marketplace beta, but this is a blocker for the FF 14 release.
Curtis - Can we merge to FF 14 Aurora with this issue outstanding?
Whiteboard: [marketplace-beta?]
Leaving the whiteboard tag on until I get a response from Curtis on whether to merge or not.
I think a merge to Aurora is fine, this needs to be fixed by merge to Beta if possible, before release for sure.
Whiteboard: [marketplace-beta?]
Whiteboard: [marketplace-beta-]
Whiteboard: [marketplace-beta-] → [marketplace-beta=]
Attached patch v1 (obsolete) (deleted) — Splinter Review
Use content policy to detect top-level document loads and redirect them to the default browser.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #618753 - Flags: review?(myk)
scheme.search(/^about|chrome|resource/) == -1 should be /^(?:about|chrome|resource)$/.test(scheme) (notice grouping/anchors). BTW, what's supposed to happen with cross-origin frames and plugin content?
Attached patch v2 (obsolete) (deleted) — Splinter Review
(In reply to Giorgio Maone from comment #13) > should be > /^(?:about|chrome|resource)$/.test(scheme) Thanks. Added with a ! in front. > BTW, what's supposed to happen with cross-origin frames and plugin content? You mean non-top-level document loads? At least with this patch, it allows any iframes and other types of resources to load.
Attachment #618753 - Attachment is obsolete: true
Attachment #618753 - Flags: review?(myk)
Attachment #618772 - Flags: review?(myk)
(In reply to Edward Lee :Mardak from comment #14) > > BTW, what's supposed to happen with cross-origin frames and plugin content? > You mean non-top-level document loads? At least with this patch, it allows > any iframes and other types of resources to load. I know this patch handle top-level document loads only. I'm arguing that I'd feel a lot better if all the cross-site inclusion/embedding loads (scripts, plugins, stylesheets, fonts...), with the possible exception of images only, were just aborted. Is this already implemented or planned, or should I file a bug report and lobby for it?
That would be interesting if apps had to explicitly allow any 3rd party resources including scripts, stylesheets, images, etc. It would at least have app developers be explicit about allowing certain 3rd party loads and potentially block unintended/injected scripts. But I would say that's a separate bug.
(In reply to Edward Lee :Mardak from comment #16) > That would be interesting if apps had to explicitly allow any 3rd party > resources including scripts, stylesheets, images, etc. It would at least > have app developers be explicit about allowing certain 3rd party loads and > potentially block unintended/injected scripts. > > But I would say that's a separate bug. This sounds similar to bug 741955, although bug 741955 refers to 3rd party navigation, not content. Would be curious to hear security's take on comment 15.
Comment on attachment 618772 [details] [diff] [review] v2 >diff --git a/webapprt/ContentPolicy.js b/webapprt/ContentPolicy.js >+ if (contentType == Ci.nsIContentPolicy.TYPE_DOCUMENT && >+ !/^(?:about|chrome|resource)$/.test(scheme) && >+ allowedOrigins.indexOf(prePath) == -1) { Nit: use simpler capturing parentheses, as it is unnecessary to make these non-capturing: !/^(about|chrome|resource)$/.test(scheme) && >+ >+ // Send the url to the default browser >+ Cc["@mozilla.org/uriloader/external-protocol-service;1"]. >+ getService(Ci.nsIExternalProtocolService). >+ getProtocolHandlerInfo(contentLocation.scheme). >+ launchWithURI(contentLocation); >+ >+ return Ci.nsIContentPolicy.REJECT_SERVER; >+ } >+ } >+ catch(ex) {} Do you expect this to throw for a particular reason? If so, it would be useful to explain the expected exceptions. Otherwise, it'd be handy to log them, either explicitly or by letting them propogate to the default handler. >+ catch(ex) {} >+ >+ return Ci.nsIContentPolicy.ACCEPT; Should we really allow the load if we fail to pass the URL to an external handler? That might enable apps to exploit bugs in our code to load pages they shouldn't. It seems better to only return ACCEPT if the arguments pass the content type, scheme, and origin checks.
Attachment #618772 - Flags: review?(myk) → review+
Attached patch v3 (deleted) — Splinter Review
Re-requesting r? for fixing a bug I noticed when a page uses window.open instead of target=_blank -- window.open first opens the window the navigates, so even if we block the load, it'll leave a blank window.
Attachment #618772 - Attachment is obsolete: true
Attachment #618850 - Flags: review?(myk)
Comment on attachment 618850 [details] [diff] [review] v3 This seems ok for now, although we should figure out how to hook into the URI loading process before the window opens, perhaps via the nsIURIContentListener for the content window.
Attachment #618850 - Flags: review?(myk) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to Ms2ger from comment #22) > https://hg.mozilla.org/mozilla-central/rev/0064e5064b4f Per comment 20, is this really resolved fixed? Or is this only a partial implementation?
Comment 20 was talking about how the fix closes a window after it's been opened up then redirected. Myk's comment was about preventing the window from opening up in the first place. This bug is still fixed. Filed followup bug 749636.
Blocks: 749636
Fails on the following test case: 1. Create an application containing <a href="https://www.google.com/">Test</a> 2. Install and launch the application 3. Click the link Expected: The default browser should start up with google.com Actual: google.com opens up in the same chromeless window.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I can also confirm that it doesn't work on latest inbound. Myk: When I unpacked omni.ja from webapprt directory, components.manifest has the correct lines for ContentPolicy.js: component {75acd178-3d5a-48a7-bd92-fba383520ae6} ContentPolicy.js application=webapprt@mozilla.org Except there's no ContentPolicy.js file.
The checkin does indeed add ContentPolicy.js and modifies Makefile.in to EXTRA_PP_COMPONENTS... https://hg.mozilla.org/mozilla-central/rev/0064e5064b4f
I pushed this to try to test it out: https://hg.mozilla.org/try/rev/12d7a69042fa 1.2 +++ b/browser/installer/package-manifest.in 1.11 @BINPATH@/webapprt/components/CommandLineHandler.js 1.12 +@BINPATH@/webapprt/components/ContentPolicy.js 1.13 @BINPATH@/webapprt/components/DirectoryProvider.js
Attached patch followup v1 (deleted) — Splinter Review
Still waiting on the tryserver build to confirm if this is all that's needed..
Attachment #619074 - Flags: review?(myk)
Comment on attachment 619074 [details] [diff] [review] followup v1 This is under browser/
Attachment #619074 - Flags: review?(myk) → review?(felipc)
Attachment #619074 - Flags: review?(felipc) → review+
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Initial test in comment 25 now passes. Same origin still operates correctly within the app window. Tested on Win 7 64-bit. Still want to do some more testing on this, given that I want to do some digging with more complex test cases.
(In reply to Jason Smith [:jsmith] from comment #36) > Initial test in comment 25 now passes. Same origin still operates correctly > within the app window. Tested on Win 7 64-bit. > > Still want to do some more testing on this, given that I want to do some > digging with more complex test cases. More testing still shows this to be okay, but there's a scenario I'm seeing odd behavior below. Trying to figure out if this is a problem in the app or a problem in our implementation: Steps: 1. Go to https://apps.mozillalabs.com/appdir/ 2. Install BarFight 3. Click Play Now! Expected: From first glance, the origin isn't changing here, but something might be going on behind the scenes. From first glance, I would expect the correct behavior to be the games page to load after clicking play now! Actual: Clicking play now always fires up the browser. Need to dig into why this behavior is being observed. Does this make sense?
(In reply to Jason Smith [:jsmith] from comment #37) > 1. Go to https://apps.mozillalabs.com/appdir/ > 2. Install BarFight > Clicking play now always fires up the browser. The manifest linked from appdir is http://tubagames.net/barfight_manifest.php That redirects to www http://www.tubagames.net/barfight_manifest.php so it seems like any non-www request is first redirected. But the app launches with origin http://tubagames.net and when it redirects to www of a different origin, it opens in the default browser.
(In reply to Edward Lee :Mardak from comment #38) > (In reply to Jason Smith [:jsmith] from comment #37) > > 1. Go to https://apps.mozillalabs.com/appdir/ > > 2. Install BarFight > > Clicking play now always fires up the browser. > > The manifest linked from appdir is http://tubagames.net/barfight_manifest.php > > That redirects to www http://www.tubagames.net/barfight_manifest.php so it > seems like any non-www request is first redirected. > > But the app launches with origin http://tubagames.net and when it redirects > to www of a different origin, it opens in the default browser. Hmm okay. I understand now - BarFight does an immediate redirect on load of the application from tubagames.net --> www.tubagames.com on load. I also just tried the following below, and the redirect to the default browser did work on load of an app with this content: <script type="text/javascript"> <!-- window.location = "http://www.google.com/" //--> </script> I'm still puzzled the following: If I load tubagames.net in the browser, it automatically redirects to a different origin (subdomain). Why isn't the app then not opening the default browser immediately on startup then?
Verified. The BarFight issue is a separate bug with the mozapps API not taking redirects into account when requesting the manifest.
Status: RESOLVED → VERIFIED
No longer blocks: 737571
Component: Desktop Runtime → Webapp Runtime
Product: Web Apps → Firefox
Flags: in-moztrap-
QA Contact: desktop-runtime → jsmith
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: