Just making sure we're getting something that we're expecting for both the value and timestamp.
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.
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
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.
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.
I think we should keep it simple now, and add more checking as the data structures land and mature a bit
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.
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 on attachment 8757795 [details] [review] [loop] mancas:bug1274130 > mozilla:akita Adding more reviewers
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.
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.