771915 - Show the origin of the page in the chrome of the app when it is different from webapp's origin

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Jason Smith [:jsmith]]

Flagged by security for desktop in bug 771395, so likely applies to Android as well. If an app goes off the origin of the app origin, it needs to show the current origin of the page in the chrome of the web app. An example use case:

1. Launch mozilla marketplace <-- nothing in the chrome yet
2. Click login <-- pop-up would have chrome with BID as the origin

Comment 1

[Aaron Train [:aaronmt]]

Vlad/Wes, thoughts?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It's all possible to do, but this needs UX input to figure out how to do it.

Comment 3

[Wesley Johnston (:wesj)]

Idea? Show the Android titlebar in these cases, and use setTitle() (which will work here) to make it show the origin?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Idea? Show the Android titlebar in these cases, and use setTitle() (which
> will work here) to make it show the origin?

Works for me.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

This shows a simple titlebar at the top of the screen when you're on a third party and hides it when you're on the trusted app's origin. Tested with Facebook and Twitter links on Jauntly.

Its a bit ugly right now, but I can live with that.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Screenshot

This is a screenshot of this (with the url moved to the bottom of the screen). Probably needs a shadow or a line dividing it from content and a little padding. Also, its shifting content up (i.e. we need to relayout).

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

This seems to work well. Very unobtrusive. Some amazing art by me. Shows the url at the bottom of the screen. Sadly, its spoofable if the app wants to pretend to be off-origin when its not. I'll post a screenshot.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 9

[Wesley Johnston (:wesj)]

That's a bit hard to read. Looks better on my device, but its still light colored. Easy way to test is to use ed.agadak.net/app, install the ed agadak app which has links to google/youtube/etc.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Created attachment 648112 [details] [diff] [review]
> Patch
> 
> This seems to work well. Very unobtrusive. Some amazing art by me. Shows the
> url at the bottom of the screen. Sadly, its spoofable if the app wants to
> pretend to be off-origin when its not. I'll post a screenshot.

Can you clarify how an app could do this? Could this be a security concern?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 648112 [details] [diff] [review]
Patch

Overall this approach is fine, but I want Ian to take a look at the screenshot and offer some tweaks.

For example:
* Top or bottom of the screen? Bottom is more like a statusbar, but Android has no such concept.
* Should we mimic the normal Android title bar a bit more?

Comment 12

[Wesley Johnston (:wesj)]

Sounds good. I can put up a build as well as some instructions too. I found the appearance of the titlebar at the top distracting. It would appear and shove content down. Maybe a nice transition would help (but that would be a layout transition which Android kinda hates). I'd rather do that later though.

Comment 13

[Jason Smith [:jsmith]]

Btw, looking at the screenshot, are you showing the full URL or just the origin?

Comment 14

[Wesley Johnston (:wesj)]

full url

Comment 15

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #14)
> full url

Oh. For meeting the original security requirement, I believe you only need to show the origin (developer.mozilla.org if you were on developer.mozilla.org/en-US/Apps), not the full URL. Not sure of the tradeoffs of only showing the origin vs. the full URL.

Comment 16

[Wesley Johnston (:wesj)]

I talked to ian on irc. He said he'd rather the url was at the top. I'll make that adjustment and file a follow up to animate it? He said he can do a mockup, but I'm fine moving that somewhere else for follow up prettifying.

Comment 17

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Attached is a mockup describing where we'd like to take the design of this bar. Basically, it should look like a micro version of the title bar. 

I have broken the design into two parts here. The first part takes what Wes has already done and adds some styling. The second part adds a few elements that might also be useful down the road, like a place for the Globe / Lock icons, and potentially a progress indicator while the page is loading. 

One possible approach might be to do the first part now for 16, and the second for 17.

Comment 18

[Ian Barlow (:ibarlow)]

(In reply to Ian Barlow (:ibarlow) from comment #17)
> Basically, it should look like a micro version of the title bar. 
> 

Sorry, I meant to say a micro version of the *Firefox* title bar there.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to Ian Barlow (:ibarlow) from comment #17)
> I have broken the design into two parts here. The first part takes what Wes
> has already done and adds some styling. The second part adds a few elements
> that might also be useful down the road, like a place for the Globe / Lock
> icons, and potentially a progress indicator while the page is loading. 

Over in bug 772680, we decided not to show a throbber when an app is doing network activity.  The rationale is not described in the bug, but the summary is that doing so should be the responsibility of the application, not the runtime, because such runtime behavior is conventional (cf. the Java and Adobe Air runtimes), and because the semantics of network activity are complex in modern web apps, and the runtime does not have enough context to present progress indicators on their apps' behalves.

The latter is especially true when the indication is of the loading of a page, since modern webapps are often constructed with lightweight pages that load content dynamically after the static page and its statically-declared resources have loaded.

So I don't think we should be showing a progress indicator for these pages.  Nor show other UI that is designed to emulate browsing affordances.  The goal here is for the normally-invisible runtime to intrude on the user's experience of apps to the minimum extent necessary to protect users from the consequences of malicious or insecure apps.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #19)

> So I don't think we should be showing a progress indicator for these pages. 
> Nor show other UI that is designed to emulate browsing affordances.  The
> goal here is for the normally-invisible runtime to intrude on the user's
> experience of apps to the minimum extent necessary to protect users from the
> consequences of malicious or insecure apps.

That's OK. We can move forward on the basic styling and perhaps the globe

Comment 21

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #19)
> 
> > So I don't think we should be showing a progress indicator for these pages. 
> > Nor show other UI that is designed to emulate browsing affordances.  The
> > goal here is for the normally-invisible runtime to intrude on the user's
> > experience of apps to the minimum extent necessary to protect users from the
> > consequences of malicious or insecure apps.
> 
> That's OK. We can move forward on the basic styling and perhaps the globe

Yup, sounds good.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch

This move this to the top and styles it using an Android gradient.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch

Patch v2 - This changes this to just show the protocol + host

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Grrr. forgot to qref.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 651872 [details] [diff] [review]
Patch v2

* Is the webapp_titlebar_bg.xml Ian-approved? :)
* You add webapp_titlebar_bg.xml into the Makefile twice, but only have one actual file (I think). Is that right?
* Do we need to import AnimationUtils? I don't see it explicitly used.
* SharedPreferences prefs does not seem to be used
* Getting the "app" using the index created from a string seems weak. Do we have any other ways to look it up?

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Updated patch. I added a "getIndex" function that's defined for WebApp# in the fragment. Good idea. I'll attach a screenshot too and ping ibarlow for approval.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 28

[Wesley Johnston (:wesj)]

irc approval from ian with a change in the text color to 666666

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 654718 [details] [diff] [review]
Patch v3

>diff --git a/mobile/android/base/WebAppsFragment.java.frag b/mobile/android/base/WebAppsFragment.java.frag

>+    protected int getIndex() { return @APPNUM@; }

Nice

>diff --git a/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml b/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml

>\ No newline at end of file

nit: add a newline

Comment 30

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8264b7d01a5a

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8264b7d01a5a

Comment 32

[Wesley Johnston (:wesj)]

Comment on attachment 654718 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps
User impact if declined: Security concerns with sites that take people to an unexpected off-origin url
Testing completed (on m-c etc.): Landed on mc 8/27
Risk to taking this patch (and alternatives if risky): Low risk. Webapps only
String or UUID changes made by this patch: None.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 654718 [details] [diff] [review]
Patch v3

Low risk, webapps only (targeted for 17) approving.

Comment 34

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3a804ecaa08