1274130 - Validate data when processing firebase records

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P1)

Description

[Ed Lee :Mardak]

Just making sure we're getting something that we're expecting for both the value and timestamp.

Comment 1

[Ed Lee :Mardak]

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.

Comment 2

[Manuel Casas Barrado [:mancas]]

Ed, what kind of validation do you want to implement? A reg exp? or using new Date(timestamp).getTime() > 0?

Comment 3

[Ed Lee :Mardak]

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.

Comment 4

[Ian Bicking (:ianbicking)]

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.

Comment 5

[Ed Lee :Mardak]

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...

Comment 6

[Ian Bicking (:ianbicking)]

I think we should keep it simple now, and add more checking as the data structures land and mature a bit

Comment 7

[Ed Lee :Mardak]

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.

Comment 8

[Mark Banner (:standard8)]

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.

Comment 9

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1274130 > mozilla:akita

Comment 10

[Manuel Casas Barrado [:mancas]]

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.

Comment 11

[Manuel Casas Barrado [:mancas]]

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

Adding more reviewers

Comment 12

[Ed Lee :Mardak]

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.

Comment 13

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

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

Hence closing the old bugs. Thank you for your support.