The default bug view has changed. See this FAQ.

Permission prompts need to indicate the app name, not the origin of the content in an app

VERIFIED FIXED in Firefox 23

Status

()

Firefox for Android
Web Apps
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jsmith, Assigned: mhaigh)

Tracking

Trunk
Firefox 23
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox23 verified, fennec24+)

Details

(Whiteboard: [A4A] [packagedapps])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
When web content requests access to a PROMPT_ACTION WebAPI, we currently fire a permission prompt that indicates that X origin is asking for access to X WebAPI. For hosted apps, this flow was okay because hosted apps essentially operate under a web-based origin (https://yourhost.com). For packaged apps, this is actually a problem - they do not have a concept of an app origin right now, so what the user ends up getting exposed to instead is app://{UUID} as the app origin when asking for a WebAPI permission. The app://{UUID} is completely non-understandable to an end-user, so we need to expose something different here.

What we did on FxOS here was that for apps in general, we indicate that app X is asking for access to X WebAPI for PROMPT_ACTION requests if you are within the origin of that app. If you are off the origin of the app (whether that be directly, iframe, etc), then we indicate the typical origin as usual. We should follow in alignment here on FxAndroid.
(Reporter)

Updated

4 years ago
tracking-fennec: --- → ?
Priority: -- → P1
Summary: Permission prompts need to indicate the app name, not the origin of the web content → Permission prompts need to indicate the app name, not the origin of the content in an app
Whiteboard: [A4A]
Assignee: nobody → wjohnston
tracking-fennec: ? → 24+
Assignee: wjohnston → jhugman

Updated

4 years ago
Whiteboard: [A4A] → [A4A] [packagedapps]
(Assignee)

Updated

4 years ago
Assignee: jhugman → mhaigh
Duplicate of this bug: 858414
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Blocks: 862014
Note: fit and finish we want this but it isn't core functionality.
Priority: P1 → P2
(Reporter)

Comment 3

4 years ago
(In reply to Erin Lancaster [:elancaster] from comment #2)
> Note: fit and finish we want this but it isn't core functionality.

No. This breaks virtually any use of a permission prompt within a packaged app in our security model. The fact that the user doesn't know the origin of the content breaks something way too foundational. We held the release on this on b2g v1.0.
Priority: P2 → P1
(Assignee)

Comment 4

4 years ago
Created attachment 739599 [details] [diff] [review]
Proposed patch
Attachment #739599 - Flags: feedback?(wjohnston)
Attachment #739599 - Flags: feedback?(mark.finkle)
Comment on attachment 739599 [details] [diff] [review]
Proposed patch

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js

>         if (app.manifestURL == aUrl) {
>+	  BrowserApp.appName = app.manifest.name;

>         if (manifest.fullLaunchPath() == aUrl) {
>+	  BrowserApp.appName = app.manifest.name;

You have TABs in here. Fix the indent.

>diff --git a/mobile/android/components/ContentPermissionPrompt.js b/mobile/android/components/ContentPermissionPrompt.js

>+    let permissionRequestorName = chromeWin.BrowserApp.appName ? "'" + chromeWin.BrowserApp.appName + "'" : request.principal.URI.host;

naming nit: let's just use "requestor"

>+

No need for the blank line

>     let message = browserBundle.formatStringFromName(entityName + ".ask",
>-                                                     [request.principal.URI.host], 1);
>+                                                     [permissionRequestorName], 1);

While you're here, can you put these lines on a single line? I dislike 80 char breaks in /mobile JS files

>     chromeWin.NativeWindow.doorhanger.show(message,
>                                            entityName + request.principal.URI.host,
>                                            buttons, tab.id, options);

Same here.

Using the host here is probably a good idea since that is used as a cookie.

You also need to address:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/OfflineApps.js#27
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebrtcUI.js#86
(both of these files are lazy loaded into the browser.js scope so BrowserApp is easy to access)

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5336
Attachment #739599 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 739599 [details] [diff] [review]
Proposed patch

Review of attachment 739599 [details] [diff] [review]:
-----------------------------------------------------------------

Seems good.

::: mobile/android/chrome/content/WebAppRT.js
@@ +67,5 @@
>          // First see if this url matches any manifests we have registered
>          // If so, get the launchUrl from the manifest and we'll launch with that
>          //let app = DOMApplicationRegistry.getAppByManifestURL(aUrl);
>          if (app.manifestURL == aUrl) {
> +	  BrowserApp.appName = app.manifest.name;

Is there a good reason to not store the app object entirely?
Attachment #739599 - Flags: feedback?(wjohnston) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 741325 [details] [diff] [review]
Incorporating feedback suggestions
Attachment #739599 - Attachment is obsolete: true
Attachment #741325 - Flags: feedback?(wjohnston)
Attachment #741325 - Flags: feedback?(mark.finkle)
Comment on attachment 741325 [details] [diff] [review]
Incorporating feedback suggestions

>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js

>         if (app.manifestURL == aUrl) {
>+          BrowserApp.manifest = app.manifest;
>           BrowserApp.manifestUrl = aUrl;

>         if (manifest.fullLaunchPath() == aUrl) {
>+          BrowserApp.manifest = app.manifest;
>           BrowserApp.manifestUrl = app.manifestURL;

We should consider using BrowserApp.app instead of saving off pieces. But that can wait for a new bug.
Attachment #741325 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Updated

4 years ago
Attachment #741325 - Flags: review?(mark.finkle)
Attachment #741325 - Flags: review?(mark.finkle)
Attachment #741325 - Flags: review+
Attachment #741325 - Flags: feedback?(wjohnston)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/64d41174028a

I don't know how you created the patch, but the header information got a bit munged in the process.
Keywords: checkin-needed
(Assignee)

Comment 10

4 years ago
Created attachment 744612 [details] [diff] [review]
fixing patch header
Attachment #741325 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 11

4 years ago
The patch is already headed to land on birch. No need to flag checkin-needed again.
Keywords: checkin-needed
Attachment #744612 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/64d41174028a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox23: --- → verified
You need to log in before you can comment on or make changes to this bug.