Closed Bug 1173666 Opened 7 years ago Closed 7 years ago

Expose the URL of the page that calls mozApps.connect to the app exposing the port

Categories

(Core Graveyard :: DOM: Apps, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: macajc, Assigned: macajc)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

+++ This bug was initially created as a clone of Bug #1169344 +++

Currently the InterApp Communication API doesn't expose the origin URL of the connecting app to the app that exposes the port. This bug is to expose the origin URL so the exposing app can take decisions based on the URL of the calling app.
Summary: Allow server apps to restrict access to their IAC ports → Expose the URL of the page that calls mozApps.connect to the app exposing the port
Assignee: nobody → carmen.jimenezcabezas
Attached patch v1 proposed patch (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8620835 - Attachment is obsolete: true
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8621068 - Attachment is obsolete: true
Attachment #8621073 - Flags: feedback?(fabrice)
Comment on attachment 8621073 [details] [diff] [review]
v1 proposed patch

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

::: dom/apps/InterAppComm.manifest
@@ +1,5 @@
>  component {9dbfa904-0718-11e3-8e77-0721a45514b8} InterAppConnection.js
>  contract @mozilla.org/dom/inter-app-connection;1 {9dbfa904-0718-11e3-8e77-0721a45514b8}
>  
> +component {cfb53421-566c-45c8-8e97-79b8be2822b9} InterAppConnection.js
> +contract @mozilla.org/dom/inter-app-connection-request;1 {cfb53421-566c-45c8-8e97-79b8be2822b9}

no need to change the component ID.

::: dom/apps/InterAppConnection.js
@@ +90,5 @@
>  
>  InterAppConnectionRequest.prototype = {
>    classDescription: "MozInterAppConnectionRequest",
>  
> +  classID: Components.ID("{cfb53421-566c-45c8-8e97-79b8be2822b9}"),

No need to change that.
Attachment #8621073 - Flags: feedback?(fabrice) → feedback+
Attached patch v2 proposed patch (obsolete) — Splinter Review
Attachment #8621073 - Attachment is obsolete: true
Blocks: 1175154
Comment on attachment 8621759 [details] [diff] [review]
v2 proposed patch

Unit tests are being written in bug 1175154.
Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=54c2464468da
requesting review
Attachment #8621759 - Flags: review?(fabrice)
No longer blocks: 1175154
Depends on: 1175154
Attachment #8621759 - Flags: review?(fabrice) → review?(ferjmoreno)
Attached patch v2 proposed patch (obsolete) — Splinter Review
Attachment #8621759 - Attachment is obsolete: true
Attachment #8621759 - Flags: review?(ferjmoreno)
Attachment #8627740 - Flags: review?(ferjmoreno)
Attached patch v2 proposed patch (obsolete) — Splinter Review
Attachment #8627740 - Attachment is obsolete: true
Attachment #8627740 - Flags: review?(ferjmoreno)
Attachment #8627752 - Flags: review?(ferjmoreno)
Comment on attachment 8627752 [details] [diff] [review]
v2 proposed patch

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

r=me with the comments addressed.

You'll need a DOM peer review for the WebIDL changes.

::: dom/apps/InterAppCommService.jsm
@@ +398,5 @@
>      if (DEBUG) {
>        debug("_dispatchMessagePorts: aKeyword: " + aKeyword +
>              " aPubAppManifestURL: " + aPubAppManifestURL +
> +            " aAllowedSubAppManifestURLs: " + aAllowedSubAppManifestURLs +
> +            " aPubPageURL:" + aPubPageURL);

nit: space after ":"

::: dom/apps/InterAppConnection.js
@@ +98,5 @@
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports]),
>  
> +  __init: function(aKeyword, aPort, aPubPageURL) {
> +    if (DEBUG) debug("__init: aKeyword: " + aKeyword + " aPort: " + aPort +
> +                     " aPageURL:" + aPubPageURL);

nit: space after ":"

::: dom/apps/Webapps.js
@@ +611,5 @@
>                                "Webapps:Connect:Return:KO"]);
>      return this.createPromise(function (aResolve, aReject) {
> +      let href = this._window.location.href;
> +      let startParams = href.indexOf("?");
> +      let from = startParams >= 0 ? href.substr(0, startParams) : href;

Use window.location.origin + window.location.path instead

::: dom/webidl/InterAppConnectionRequest.webidl
@@ +11,5 @@
>    readonly attribute DOMString keyword;
>  
>    readonly attribute MozInterAppMessagePort port;
> +
> +  readonly attribute DOMString pageURL;

Could we use a different name? Maybe "from" (i.e: connectionRequest.from)
Attachment #8627752 - Flags: review?(ferjmoreno) → review+
r=ferjm
Attachment #8627752 - Attachment is obsolete: true
Attachment #8629669 - Flags: review+
Fernando, who could review this as a DOM peer?
Flags: needinfo?(ferjmoreno)
Flags: needinfo?(ferjmoreno)
Attachment #8629669 - Flags: review+ → review?(amarchesini)
 Baku, could you take a look to the DOM changes in this patch? The only files affected are  /dom/apps/InterAppConnection.js and /dom/webidl/InterAppConnectionRequest.webidl
> Use window.location.origin + window.location.path instead

Why do you care just about this part of the URL?
Flags: needinfo?(ferjmoreno)
(In reply to Andrea Marchesini (:baku) from comment #13)
> > Use window.location.origin + window.location.path instead
> 
> Why do you care just about this part of the URL?

Exposing the full url (including the query parameters) would expose private user info (i.e. session information, search information, etc.).

I thing the origin + path parts of the URL are enough for the service to make the call for allowing or rejecting the connection.
Flags: needinfo?(ferjmoreno)
Comment on attachment 8629669 [details] [diff] [review]
v2 proposed patch

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

::: dom/apps/Webapps.js
@@ +609,5 @@
>    connect: function(aKeyword, aRules) {
>      this.addMessageListeners(["Webapps:Connect:Return:OK",
>                                "Webapps:Connect:Return:KO"]);
>      return this.createPromise(function (aResolve, aReject) {
> +      let from = this._window.location.origin + this._window.location.pathname;

I really don't like this. Instead do this:

let from = new URL(this._window.location);
from.search = "";

But I let ferjm to decide this.
Attachment #8629669 - Flags: review?(amarchesini)
Attachment #8629669 - Flags: review+
Attachment #8629669 - Flags: feedback?(ferjmoreno)
Comment on attachment 8629669 [details] [diff] [review]
v2 proposed patch

LGTM
Attachment #8629669 - Flags: feedback?(ferjmoreno) → feedback+
try looks mainly green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dbec17ba862
requesting checkin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/932a80206f57
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.