Closed Bug 1169344 Opened 7 years ago Closed 7 years ago

Allow server apps to restrict access to their IAC ports

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, 5 obsolete files)

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).
QA Contact: carmen.jimenezcabezas
Blocks: 1169333
No longer depends on: 1169333
Assignee: nobody → carmen.jimenezcabezas
QA Contact: carmen.jimenezcabezas
Status: NEW → ASSIGNED
Depends on: 1173666
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.
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8621041 - Flags: feedback?(fabrice)
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
Attachment #8621041 - Flags: feedback?(fabrice)
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8621041 - Attachment is obsolete: true
Attachment #8621090 - Flags: feedback?(fabrice)
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
Attachment #8621090 - Flags: feedback?(fabrice) → feedback?(ferjmoreno)
(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?
Flags: needinfo?(carmen.jimenezcabezas)
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.
Antonio already answered it,
Flags: needinfo?(carmen.jimenezcabezas)
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8621090 - Attachment is obsolete: true
Attachment #8621090 - Flags: feedback?(ferjmoreno)
Blocks: 1175154
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
Attachment #8622420 - Flags: review?(ferjmoreno)
No longer blocks: 1175154
Depends on: 1175154
Attachment #8622420 - Flags: review?(ferjmoreno)
Attached patch v1 proposed patch (obsolete) — Splinter Review
Attachment #8622420 - Attachment is obsolete: true
Attachment #8624681 - Flags: review?(ferjmoreno)
Attachment #8624681 - Flags: feedback?(ehsan)
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
Attachment #8624681 - Flags: review?(ferjmoreno)
Attached patch v2 proposed patch (obsolete) — Splinter Review
Attachment #8624681 - Attachment is obsolete: true
Attachment #8624681 - Flags: feedback?(ehsan)
Attachment #8627777 - Attachment is obsolete: true
Attachment #8628219 - Flags: review?(ferjmoreno)
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!
Attachment #8628219 - Flags: review?(ferjmoreno) → review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f42771154939
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.