1239701 - Datastore should allow us to define stores accessible just by certified apps.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Francisco Jordano [:arcturus] [:francisco]]

In bug 1181329 we allowed homescreen apps to access datastores.

This means we potentially allow homescreen apps to delete your in case of emergency contacts or steal your history.

We should be able to define datastores that could be accessed just by certified apps. And fix the gaia DS that will relay on DS being certified.

Comment 1

[Chris Lord [:cwiiis]]

Really, we just want access-control for datastores, like we have it for GPS and other permissions. If we could, in system, get a signal 'App <manifestURL> is requesting access to <datastore name> owned by <manifestURL>', we could present that to the user (and indeed, block off access to things like the password store by default).

I don't think we should complicate how datastores are declared, I think we should give the user more control and allow datastores to be used by all privileged apps (and not just homescreens, which, really, is a completely arbitrary decision, born of necessity rather than desire).

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

There have been several attempts to open this API to privileged apps. The last one was tracked at [1].

TBH, at this point, given where we are with FxOS, I don't think DataStore will ever be exposed to privileged apps. IMHO we should instead solve the DataStore use case (share data across origins) with a different tool that can be properly exposed to the web (foreign fetch?, shared IDB?).

In any case, this bug is about fixing the privacy issue introduced by bug 1181329. As Francisco mentioned in the description, we shouldn't be exposing information such as emergency contacts or history to privileged homescreens.

Ehsan, Andrea, what do you think?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=942641

Comment 3

[(no longer active)]

I think making DataStores certified-only is fine.  It really isn't worth anyone's time doing something more sophisticated.  DataStore should be considered as dead technology.  :-)

Comment 4

[Chris Lord [:cwiiis]]

(In reply to :Ehsan Akhgari from comment #3)
> I think making DataStores certified-only is fine.  It really isn't worth
> anyone's time doing something more sophisticated.  DataStore should be
> considered as dead technology.  :-)

Unless someone does the work to port bookmarks and history to a different storage mechanism that allows sharing, this isn't an option. 3rd-party Home screens need to be able to display pinned sites and pages.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Untested patch

Comment 6

[(no longer active)]

(In reply to Chris Lord [:cwiiis] from comment #4)
> (In reply to :Ehsan Akhgari from comment #3)
> > I think making DataStores certified-only is fine.  It really isn't worth
> > anyone's time doing something more sophisticated.  DataStore should be
> > considered as dead technology.  :-)
> 
> Unless someone does the work to port bookmarks and history to a different
> storage mechanism that allows sharing, this isn't an option. 3rd-party Home
> screens need to be able to display pinned sites and pages.

Sorry, that should have read "Making *some* DataStores certified-only".

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

This also adds a test for bug 1181329

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8708974 [details] [diff] [review]
v1

Fabrice, could you take a look at the Webapps.jsm part, please? Thanks!

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] ferjm:bug1239701.ds.certifiedonly > mozilla-b2g:master

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8708974 [details] [diff] [review]
v1

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

Just revert all the changes in nsIDataStore.idl and DataStoreImpl.js. Except if we want to expose/use _certifiedOnly

::: dom/datastore/DataStoreImpl.js
@@ +87,5 @@
>      this._window = aWindow;
>      this._name = aName;
>      this._owner = aOwner;
>      this._readOnly = aReadOnly;
> +    this._certifiedOnly = aCertifiedOnly;

This value is not exposed/used in anyway here. Why do we care to have it?

::: dom/datastore/DataStoreService.cpp
@@ +83,3 @@
>                  bool aEnabled)
>    {
> +    Init(aName, aOriginURL, aManifestURL, aReadOnly, aCertifiedOnly, aEnabled);

80chars.

@@ +1054,5 @@
>        continue;
>      }
>  
> +    uint16_t status;
> +    if (NS_FAILED(aPrincipal->GetAppStatus(&status))) {

We don't need to retrieve the status in case mCertifiedOnly is false.
Do:

if (appInfo->mCertifiedOnly) {
  GetStatus();
  ErrorHandling...
  if (status != ..
}

::: dom/datastore/nsIDataStore.idl
@@ +15,5 @@
>    void init(in nsIDOMWindow window,
>              in DOMString name,
>              in DOMString manifestURL,
> +            in boolean readOnly,
> +            in boolean certifiedOnly);

This seems not needed.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9adecefeb1b

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Gaia master https://github.com/mozilla-b2g/gaia/commit/1a4af5ddbaed8a2ad4ed1b4f22c69c5a02c759a1

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89179ddbfb3d