Validate updatedAt on saves and deletes
#30
Conversation
| return true; | ||
| } | ||
| if (typeof this.previous.updatedAt !== "number") { | ||
| errorMessage("This model's previous `updatedAt` is not a number"); |
There was a problem hiding this comment.
The errorMessage function call resulting in a thrown Error is a surprising side-effect that made this diff hard to reason about. I'd recommend something more like
throw createError('...')
|
@hstove I read through the diff -- the code looks good and it appears to fix the issue. However, I have little understanding of the general codebase, and definitely don't have the a grasp on the functional end-to-end workings of Radiks needed to really understand the security model. Is there anything in particular you want reviewed or tested? Otherwise, if you feel confident in the fixes, then I think we should go ahead and merge & release. |
| signature, | ||
| updatedAt, | ||
| }: { signature: string; updatedAt: string } = req.query; | ||
| if (!updatedAt || parseInt(updatedAt, 10) <= attrs.updatedAt) { |
There was a problem hiding this comment.
This may be an ignorant question --
I can't tell if the signature validation encompasses this updatedAt value, is the value from the client already validated before this function?
As in, is there a server-side component preventing a malicious client from just setting this value to Number.MAX_SAFE_INTEGER or something like that?
There was a problem hiding this comment.
There is no such check for updatedAt. We should add a validator check that the value is near the server's time.
Fixes #29
Radiks-server was not properly validating the
updatedAtattribute for model updates and deletes. This could potentially lead to signature jacking. Radiks-server now validated that theupdatedAtfield is greater than previousupdatedAts. This acts similarly to annoncein blockchains.