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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

({privacy})

Trunk
mozilla17
privacy
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Please see: https://groups.google.com/d/msg/mozilla.dev.webapi/pkx1uz_pnhQ/qzCco0g1414J
(Assignee)

Updated

5 years ago
Blocks: 749551
That should block because there are security/privacy issues in having any app with read/write access to any other app alarms.
blocking-basecamp: --- → ?
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Created attachment 646006 [details] [diff] [review]
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
Assignee: nobody → clian
Attachment #646006 - Flags: feedback?(mounir)
Attachment #646006 - Flags: feedback?(21)
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?
Attachment #646006 - Flags: feedback?(mounir)
(Assignee)

Comment 5

5 years ago
(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
Blocking on privacy concern (maybe minor) as well as shipping an api that behaves per the spec.
blocking-basecamp: ? → +
Keywords: privacy
(Assignee)

Comment 7

5 years ago
Created attachment 647853 [details] [diff] [review]
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
Attachment #646006 - Attachment is obsolete: true
Attachment #646006 - Flags: feedback?(21)
Attachment #647853 - Flags: review?(21)
(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 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
Attachment #647853 - Flags: review?(21) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 648298 [details] [diff] [review]
Patch for Checkin

Vivien has reviewed+ at comment #9.
Attachment #647853 - Attachment is obsolete: true
Attachment #648298 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Keywords: checkin-needed

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e80ccb26f3d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e80ccb26f3d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.