1169344 - Allow server apps to restrict access to their IAC ports

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Carmen Jimenez Cabezas]

Currently the InterApp Communication API is restricted to certified apps only, and any certified app can connect to any IAC port. Once it's opened to unsigned apps, we'll need some way to restrict access. Initially we propose allow restricting access, on the manifest:

* Per application type (currently privileged, certified, web, to be changed to signed/unsigned when the model changes)
* Per origin URL.

We also propose exposing the origin of the connection to the app implementing the port (currently it isn't exposed).

Comment 1

[Carmen Jimenez Cabezas]

Opened bug 1173666 to expose the URL of the app connecting to a port to the app that exposes that port. Leaving this bug to implement only the restrictions that are manifest based.

Comment 2

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 3

[Carmen Jimenez Cabezas]

Comment on attachment 8621041 [details] [diff] [review]
v1 proposed patch

sorry, forgot to change it so by default (if no rules are present) it only works for certified apps

Comment 4

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8621090 [details] [diff] [review]
v1 proposed patch

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

Fernando know that much better than me I think

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Carmen Jimenez Cabezas from comment #0)
> Currently the InterApp Communication API is restricted to certified apps
> only, and any certified app can connect to any IAC port. Once it's opened to
> unsigned apps, we'll need some way to restrict access. Initially we propose
> allow restricting access, on the manifest:

Are we opening this API to unsigned apps?

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

That was what we agreed with Jonas to advance with the mediator/service-like apps, to add the necessary functionality (and opening to unsigned) to IAC instead of implementing navigator.connect or other communication API.

Comment 8

[Carmen Jimenez Cabezas]

Antonio already answered it,

Comment 9

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 10

[Carmen Jimenez Cabezas]

Comment on attachment 8622420 [details] [diff] [review]
v1 proposed patch

Unit tests are being written in bug 1175154.
Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=54c2464468da
requesting review

Comment 11

[Carmen Jimenez Cabezas]

Attachment: v1 proposed patch

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8624681 [details] [diff] [review]
v1 proposed patch

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

I'd like to see the final version with the change to disable this option for non-certified apps on release builds.

::: dom/apps/InterAppCommService.jsm
@@ +318,5 @@
>      return false;
>    },
>  
> +  _matchPageURLs: function(aRules, aPageURL) {
> +    if (!aRules || !Array.isArray(aRules.pageURLs)) {

You could do this

if (!Array.isArray(aRules.pageURLS)) {
  aRules.pageURLs = [aRules.pageURLs];
}

so we don't push the developer that much with the syntax.

@@ +327,5 @@
> +    }
> +
> +    let pageURLs = aRules.pageURLs;
> +    let isAllowed = false;
> +    for (var i = 0, li = pageURLs.length; i < li && !isAllowed ; i++) {

s/var/let

@@ +333,5 @@
> +      isAllowed = regExpAllowedURL.test(aPageURL);
> +    }
> +
> +    if (DEBUG) {
> +      debug("rules.pageURLs is " + (isAllowed? "" :"not") + " matched!" +

nit: space after isAllowed

@@ +335,5 @@
> +
> +    if (DEBUG) {
> +      debug("rules.pageURLs is " + (isAllowed? "" :"not") + " matched!" +
> +            " pageURLs: " + pageURLs +
> +            " aPageURL : " + aPageURL);

nit: extra space after aPageURL

@@ +364,5 @@
>    },
>  
>    _matchRules: function(aPubAppManifestURL, aPubRules,
> +                        aSubAppManifestURL, aSubRules,
> +                        aPubPageURL, aSubPageURL) {

Could you add a comment here explaining what are the requirements for an app to make a connection, please?

@@ +378,5 @@
> +    let numSubRules =  (aSubRules && Object.keys(aSubRules).length) || 0;
> +    let numPubRules =  (aPubRules && Object.keys(aPubRules).length) || 0;
> +
> +    if ((!isSubAppCertified && !numPubRules) ||
> +        (!isPubAppCertified && !numSubRules)) {

As mentioned via email, we shouldn't allow this for non-certified apps on release builds.

@@ +383,4 @@
>        if (DEBUG) {
> +        debug("If there aren't rules defined only certified apps are allowed " +
> +              "to do connections.");
> +

nit: extra line

Comment 13

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

Comment 14

[Carmen Jimenez Cabezas]

Attachment: v2 proposed patch

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

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

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

::: dom/apps/InterAppCommService.jsm
@@ +384,5 @@
> +  //    allowed. This is only checked for non certified apps!
> +  // The default value (empty or non existant rules) is:
> +  //   * Only certified apps can connect
> +  //   * Any originator/receiving page URLs are valid
> +  //   * Any origin is valid.

\o/ Thank you!

Comment 16

[Carmen Jimenez Cabezas]

try looks mainly green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dbec17ba862
requesting checkin
Note: this patch depends on the one on bug 1173666

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f42771154939

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/f42771154939