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)
Firefox Graveyard
Webapp Runtime
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: anant, Assigned: Mardak)
References
Details
(Whiteboard: [marketplace-beta=])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: devPreviewNonBlocker
Comment 2•13 years ago
|
||
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808725
Updated•13 years ago
|
Assignee: anant → nobody
Blocks: 731054
Component: Extension → General
Product: Web Apps → Firefox
QA Contact: extension → general
Target Milestone: --- → Firefox 14
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Updated•13 years ago
|
Component: General → Web Apps
QA Contact: general → webapps
Updated•13 years ago
|
Assignee: nobody → anant
Component: Web Apps → General
Product: Firefox → Web Apps
QA Contact: webapps → general
Target Milestone: Firefox 14 → ---
Comment 3•13 years ago
|
||
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.
Updated•13 years ago
|
Assignee: anant → nobody
Component: General → Web Apps
Product: Web Apps → Firefox
QA Contact: general → webapps
Updated•13 years ago
|
Component: Web Apps → Desktop Runtime
Product: Firefox → Web Apps
QA Contact: webapps → desktop-runtime
Updated•13 years ago
|
Updated•13 years ago
|
Whiteboard: devPreviewNonBlocker → devPreviewNonBlocker [marketplace-beta?]
Updated•13 years ago
|
Whiteboard: devPreviewNonBlocker [marketplace-beta?] → r [marketplace-beta?]
Updated•13 years ago
|
Whiteboard: r [marketplace-beta?] → [marketplace-beta?]
Comment 5•13 years ago
|
||
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?]
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
Curtis - Can we merge to FF 14 Aurora with this issue outstanding?
Updated•13 years ago
|
Whiteboard: [marketplace-beta?]
Comment 8•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: [marketplace-beta?]
Updated•13 years ago
|
Whiteboard: [marketplace-beta-]
Updated•13 years ago
|
Whiteboard: [marketplace-beta-] → [marketplace-beta=]
Assignee | ||
Comment 12•13 years ago
|
||
Use content policy to detect top-level document loads and redirect them to the default browser.
Comment 13•13 years ago
|
||
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?
Assignee | ||
Comment 14•13 years ago
|
||
(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)
Comment 15•13 years ago
|
||
(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?
Assignee | ||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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+
Assignee | ||
Comment 19•13 years ago
|
||
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 20•13 years ago
|
||
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+
Assignee | ||
Comment 21•13 years ago
|
||
Comment 22•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 23•13 years ago
|
||
(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?
Assignee | ||
Comment 24•13 years ago
|
||
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
Comment 25•13 years ago
|
||
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 → ---
Comment 26•13 years ago
|
||
Comment 27•13 years ago
|
||
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)
Assignee | ||
Comment 28•13 years ago
|
||
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.
Assignee | ||
Comment 29•13 years ago
|
||
The checkin does indeed add ContentPolicy.js and modifies Makefile.in to EXTRA_PP_COMPONENTS...
https://hg.mozilla.org/mozilla-central/rev/0064e5064b4f
Assignee | ||
Comment 30•13 years ago
|
||
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
Assignee | ||
Comment 31•13 years ago
|
||
Still waiting on the tryserver build to confirm if this is all that's needed..
Attachment #619074 -
Flags: review?(myk)
Assignee | ||
Comment 32•13 years ago
|
||
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
Assignee | ||
Comment 33•13 years ago
|
||
Comment on attachment 619074 [details] [diff] [review]
followup v1
This is under browser/
Attachment #619074 -
Flags: review?(myk) → review?(felipc)
Updated•13 years ago
|
Attachment #619074 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 34•13 years ago
|
||
Comment 35•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 36•13 years ago
|
||
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•13 years ago
|
||
(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?
Assignee | ||
Comment 38•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
Verified. The BarFight issue is a separate bug with the mozapps API not taking redirects into account when requesting the manifest.
Status: RESOLVED → VERIFIED
Updated•12 years ago
|
Component: Desktop Runtime → Webapp Runtime
Product: Web Apps → Firefox
Updated•12 years ago
|
Flags: in-moztrap-
Updated•12 years ago
|
QA Contact: desktop-runtime → jsmith
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•