Validate data when processing firebase records

RESOLVED INCOMPLETE

Status

P1
normal
Rank:
22
RESOLVED INCOMPLETE
3 years ago
2 years ago

People

(Reporter: Mardak, Assigned: mancas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [akita])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Just making sure we're getting something that we're expecting for both the value and timestamp.
(Reporter)

Comment 1

3 years ago
From https://github.com/mozilla/loop/pull/424#discussion_r63760126


We should probably validate data.timestamp as well - or at least check that if something is wrong here, we a) log it b) don't break other functionality.
(Reporter)

Updated

3 years ago
Blocks: 1274165
Rank: 22
Priority: -- → P2
Rank: 22 → 11
Priority: P2 → P1
(Assignee)

Comment 2

3 years ago
Ed, what kind of validation do you want to implement? A reg exp? or using new Date(timestamp).getTime() > 0?
Assignee: nobody → b.mcb
Status: NEW → ASSIGNED
Flags: needinfo?(edilee)
(Reporter)

Comment 3

3 years ago
ianb and Standard8 provided the review comments on the PR requesting validating the data and timestamp respectively.

It sounds like the reasoning for this bug is potentially malicious or just bad data can make it to firebase as someone with room access can just write records.

I'm not sure how extensive of checks are desired. E.g., is type checking enough? record.timestamp: Number (for all records), record.value: Object (for most record types?)

I suppose we could try checking deeper into record.value if it would be dispatched as an action, but Actions already have required key and type checking.
Flags: needinfo?(standard8)
Flags: needinfo?(ianb)
Flags: needinfo?(edilee)
Rank: 11 → 15
Flags: needinfo?(ianb)
I'm just particularly sensitive about anything that is used to create URLs, and so in those places where I made comments the validation is just intended to ensure that the URL is valid.  E.g., ".." can cause weird things.

In places where a URL is being constructed I'd like to see a regex checked on the string value of the item being inserted.
(Reporter)

Comment 5

3 years ago
The toc-page record hasn't been created yet, but how detailed should the data driver know of the structure of the actual page payload? E.g., should the data driver just treat it as an object? Object with known keys of javascript types? Known keys with specific string interpretation (https? prefixed)?

The dispatcher already enforces javascript types, so would it make more sense to verify the data after those checks? Although that would mean an action could be dispatched with "bad" values that each of the action handlers would need to check...
I think we should keep it simple now, and add more checking as the data structures land and mature a bit
(Reporter)

Comment 7

3 years ago
I suppose with that, we should just enforce data.timestamp is a number, data.value is undefined or an object, type/id from the key are matched.
My main thought with the data.timestamp is that new Date() can throw. If it throws, then we need to make sure our other code doesn't break (e.g. one bad chat message could stop reading of all the messages).

From a security perspective, we need to remember that anyone who gets the firebase url can put in random stuff to the database. We need to be ready to handle that with the normal type of security management.
Flags: needinfo?(standard8)
Created attachment 8757795 [details] [review]
[loop] mancas:bug1274130 > mozilla:akita
(Assignee)

Comment 10

3 years ago
Comment on attachment 8757795 [details] [review]
[loop] mancas:bug1274130 > mozilla:akita

I'm using the loop validator to check the type of each field. I'm not sure which fields we want to validate or if we really want a validator schema per record type. However I think is a good starting point.
Attachment #8757795 - Flags: review?(standard8)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8757795 [details] [review]
[loop] mancas:bug1274130 > mozilla:akita

Adding more reviewers
Attachment #8757795 - Flags: review?(edilee)
Attachment #8757795 - Flags: review?(dmose)
(Reporter)

Comment 12

3 years ago
Comment on attachment 8757795 [details] [review]
[loop] mancas:bug1274130 > mozilla:akita

Overall, I'm not sure if this is the right approach of using Validator.

In particular, Validator does a single level check of a minimum set of required typed keys. At the top level of the record, each must have a timestamp (a number that's new Date()-able) and optionally a value (any value or no value for deleted), so this means any unexpected keys are probably invalid. Similarly, unexpected types or record keys that don't follow our "type!id" pattern should be invalid too.

There's some abstraction of DataDriver not really knowing what the exact value structure of each type, but if we were to have DataDriver validate it, I think Validator would be reasonable to use there. But I don't think we'll need to do that at this point.
Attachment #8757795 - Flags: review?(standard8)
Attachment #8757795 - Flags: review?(edilee)
Attachment #8757795 - Flags: review?(dmose)
Attachment #8757795 - Flags: feedback+
Rank: 15 → 22
Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.