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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Carmen Jimenez Cabezas]

+++ 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.

Comment 1

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 2

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 3

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 4

[[:fabrice] Fabrice Desré]

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.

Comment 5

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

Comment 6

[Carmen Jimenez Cabezas]

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

Comment 7

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

Comment 8

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

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)

Comment 10

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

r=ferjm

Comment 11

[Carmen Jimenez Cabezas]

Fernando, who could review this as a DOM peer?

Comment 12

[Carmen Jimenez Cabezas]

 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

Comment 13

[Andrea Marchesini [:baku]]

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

Why do you care just about this part of the URL?

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

(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.

Comment 15

[Andrea Marchesini [:baku]]

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.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8629669 [details] [diff] [review]
v2 proposed patch

LGTM

Comment 17

[Carmen Jimenez Cabezas]

try looks mainly green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dbec17ba862
requesting checkin

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/932a80206f57

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/932a80206f57