Last Comment Bug 707836 - Handle URI navigation outside app domain for native apps
: Handle URI navigation outside app domain for native apps
Status: VERIFIED FIXED
[marketplace-beta=]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: unspecified
: All All
: -- normal
: ---
Assigned To: Ed Lee :Mardak
: Jason Smith [:jsmith]
Mentors:
: 734779 741954 749057 (view as bug list)
Depends on:
Blocks: 749636 757320
  Show dependency treegraph
 
Reported: 2011-12-05 14:45 PST by Anant Narayanan [:anant]
Modified: 2016-03-21 12:39 PDT (History)
11 users (show)
jsmith: in‑moztrap-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (3.93 KB, patch)
2012-04-26 12:09 PDT, Ed Lee :Mardak
no flags Details | Diff | Splinter Review
v2 (3.93 KB, patch)
2012-04-26 12:48 PDT, Ed Lee :Mardak
myk: review+
Details | Diff | Splinter Review
v3 (4.09 KB, patch)
2012-04-26 16:37 PDT, Ed Lee :Mardak
myk: review+
Details | Diff | Splinter Review
followup v1 (1.09 KB, patch)
2012-04-27 09:24 PDT, Ed Lee :Mardak
felipc: review+
Details | Diff | Splinter Review

Description Anant Narayanan [:anant] 2011-12-05 14:45:20 PST
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.
Comment 1 Anant Narayanan [:anant] 2011-12-06 14:27:02 PST
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.
Comment 2 Jason Smith [:jsmith] 2012-02-11 16:14:39 PST
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808725
Comment 3 Jason Smith [:jsmith] 2012-03-15 09:06:47 PDT
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.
Comment 4 Jason Smith [:jsmith] 2012-04-15 20:29:49 PDT
*** Bug 741954 has been marked as a duplicate of this bug. ***
Comment 5 Jennifer Arguello :ticachica 2012-04-18 16:38:35 PDT
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.
Comment 6 Jason Smith [:jsmith] 2012-04-18 16:49:32 PDT
(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.
Comment 7 Jason Smith [:jsmith] 2012-04-18 17:16:16 PDT
Curtis - Can we merge to FF 14 Aurora with this issue outstanding?
Comment 8 Jason Smith [:jsmith] 2012-04-18 17:17:38 PDT
Leaving the whiteboard tag on until I get a response from Curtis on whether to merge or not.
Comment 9 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-04-19 18:50:09 PDT
I think a merge to Aurora is fine, this needs to be fixed by merge to Beta if possible, before release for sure.
Comment 10 Jason Smith [:jsmith] 2012-04-24 20:37:01 PDT
*** Bug 734779 has been marked as a duplicate of this bug. ***
Comment 11 Jason Smith [:jsmith] 2012-04-25 22:21:24 PDT
*** Bug 749057 has been marked as a duplicate of this bug. ***
Comment 12 Ed Lee :Mardak 2012-04-26 12:09:06 PDT
Created attachment 618753 [details] [diff] [review]
v1

Use content policy to detect top-level document loads and redirect them to the default browser.
Comment 13 Giorgio Maone [:mao] 2012-04-26 12:33:55 PDT
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?
Comment 14 Ed Lee :Mardak 2012-04-26 12:48:27 PDT
Created attachment 618772 [details] [diff] [review]
v2

(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.
Comment 15 Giorgio Maone [:mao] 2012-04-26 14:10:14 PDT
(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?
Comment 16 Ed Lee :Mardak 2012-04-26 14:15:22 PDT
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.
Comment 17 Jason Smith [:jsmith] 2012-04-26 14:18:10 PDT
(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 18 Myk Melez [:myk] [@mykmelez] 2012-04-26 14:50:35 PDT
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.
Comment 19 Ed Lee :Mardak 2012-04-26 16:37:31 PDT
Created attachment 618850 [details] [diff] [review]
v3

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.
Comment 20 Myk Melez [:myk] [@mykmelez] 2012-04-26 16:52:07 PDT
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.
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2012-04-27 06:53:55 PDT
https://hg.mozilla.org/mozilla-central/rev/0064e5064b4f
Comment 23 Jason Smith [:jsmith] 2012-04-27 07:00:24 PDT
(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 24 Ed Lee :Mardak 2012-04-27 08:37:41 PDT
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.
Comment 25 Jason Smith [:jsmith] 2012-04-27 09:01:53 PDT
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.
Comment 26 Jason Smith [:jsmith] 2012-04-27 09:04:09 PDT
http://screencast.com/t/Ylp5CDiRso
Comment 27 Jason Smith [:jsmith] 2012-04-27 09:06:26 PDT
For comment 25 and comment 26 - This was tested on today's mozilla inbound build (ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-inbound/firefox-15.0a1.en-US.win32.installer.exe)
Comment 28 Ed Lee :Mardak 2012-04-27 09:14:23 PDT
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.
Comment 29 Ed Lee :Mardak 2012-04-27 09:15:32 PDT
The checkin does indeed add ContentPolicy.js and modifies Makefile.in to EXTRA_PP_COMPONENTS...

https://hg.mozilla.org/mozilla-central/rev/0064e5064b4f
Comment 30 Ed Lee :Mardak 2012-04-27 09:19:58 PDT
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
Comment 31 Ed Lee :Mardak 2012-04-27 09:24:03 PDT
Created attachment 619074 [details] [diff] [review]
followup v1

Still waiting on the tryserver build to confirm if this is all that's needed..
Comment 32 Ed Lee :Mardak 2012-04-27 09:40:23 PDT
jsmith: can you check if this build fixes navigating away for you? Seems to work for me now:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/edward.lee@engineering.uiuc.edu-fd18a1dfbf43/try-macosx64-debug/firefox-15.0a1.en-US.mac64.dmg
Comment 33 Ed Lee :Mardak 2012-04-27 09:43:09 PDT
Comment on attachment 619074 [details] [diff] [review]
followup v1

This is under browser/
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-04-29 14:12:21 PDT
http://hg.mozilla.org/mozilla-central/rev/d54672ad27d9
Comment 36 Jason Smith [:jsmith] 2012-04-30 18:06:44 PDT
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.
Comment 37 Jason Smith [:jsmith] 2012-05-01 23:33:52 PDT
(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?
Comment 38 Ed Lee :Mardak 2012-05-01 23:41:56 PDT
(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.
Comment 39 Jason Smith [:jsmith] 2012-05-01 23:59:57 PDT
(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?
Comment 40 Jason Smith [:jsmith] 2012-05-04 15:10:04 PDT
Verified. The BarFight issue is a separate bug with the mozapps API not taking redirects into account when requesting the manifest.

Note You need to log in before you can comment on or make changes to this bug.