777224 - Alarm API - .getAll() and .remove() can only interact with alarms scheduled by the same app

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

Please see: https://groups.google.com/d/msg/mozilla.dev.webapi/pkx1uz_pnhQ/qzCco0g1414J

Comment 1

[Mounir Lamouri (:mounir)]

That should block because there are security/privacy issues in having any app with read/write access to any other app alarms.

Comment 2

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #1)
> That should block because there are security/privacy issues in having any
> app with read/write access to any other app alarms.

Hi Mounir,

Regarding the implementation details, I think we can simply compare if the manifestURL is matched or not to decide if .getAll()/.remove() can return/delete the alarms, since the manifestURL of an app has been saved to identify the app when adding alarms (for system message purpose). That is, an app can only see the alarms it added before.

Does this solution sound reasonable to you? ;) I'll come back with a WIP patch later.

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

Hi Mounir and Vivien,

This is quick WIP patch. Although it's not yet cleaned up and tested, should be good to go. ;) Need your feedback to make sure I'm on the right way to fix this. Please also see comment #2 for the proposed solution.

Thanks,
Gene

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 646006 [details] [diff] [review]
WIP Patch

I'm on vacation, so you should find someone else to do this review on the DOM side. :sicking or :jlebar maybe?

Comment 5

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #4)
> Comment on attachment 646006 [details] [diff] [review]
> WIP Patch
> 
> I'm on vacation, so you should find someone else to do this review on the
> DOM side. :sicking or :jlebar maybe?

Ah! Sorry for bothering. Bon vacances! :D

Comment 6

[Dietrich Ayala (:dietrich)]

Blocking on privacy concern (maybe minor) as well as shipping an api that behaves per the spec.

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Vivien again :),

This patch covers the following changes:

1. In AlarmDB.jsm, I modify .remove and .getAll() by adding a new parameter aManifestURL, which is used to check if the requesting app can remove or get the alarms.

2. In AlarmService.jsm, we will pass the json.manifestURL from the content process to _db.getAll() and _db.remove(). Note that we don't need to do so in the _onAlarmFired() and _restoreAlarmsFromDb() because it's not requested from a specific content processes.

3. In AlarmsManager.js, for .remove() and .getAll(), we pass the app's manifestURL into the message to be sent to AlarmService.

Please feel free to let me know if you don't agree with any of these changes. Btw, do we need to ask for another DOM peer's review if we only have internal logic changes to fix this issue?

Thanks,
Gene

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Gene Lian [:gene] from comment #7)
> Created attachment 647853 [details] [diff] [review]
> Patch
> 
> Hi Vivien again :),
> 

Hey :)
 
> Please feel free to let me know if you don't agree with any of these
> changes. Btw, do we need to ask for another DOM peer's review if we only
> have internal logic changes to fix this issue?

I would say that it should be fine.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 647853 [details] [diff] [review]
Patch

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

::: dom/alarm/AlarmService.jsm
@@ +161,5 @@
>                return;
>              }
>  
>              // check if the alarm to be removed is in the queue
> +            // by ID and whether it belongs to the requsting app

requsting -> requesting

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Patch for Checkin

Vivien has reviewed+ at comment #9.

Comment 11

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e80ccb26f3d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e80ccb26f3d