Message validation for the rest of Federation messages #1959
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
There was a problem hiding this comment.
Great job !
I have multiple comments :
- some picky comments in the review. Good and complete validation enhances security, but also robustness of the code : other tests using some federation data will not be able to instantiate wrong data, therefore avoiding any case of uncatched bugs and issues. From my experience, as I was adding validations a lot of tests aside were failing due to bad data instantiation. This showed how important it is to give clear and strict guidelines. In principle, the documentation provided in the protocol is all that is needed to know what to check 👍
- you should edit the Issue Complete message validation #1751 with the new classes and check the classes done :)
- I feel like there is a lack of tests. There should be one tests for every construction scenario, checking if the constructor fails or succeeds accordingly. A great example can be found here. I know it looks big but it really is quick, and enforces the well definitions of the validations.
| @@ -19,6 +20,7 @@ class Challenge | |||
|
|
|||
| init { | |||
| this.validUntil = max(0L, validUntil) | |||
There was a problem hiding this comment.
validation should ideally be before instantiating attributes, since if the input is wrong and it is meant to crash, no need to fill the attribute beforehand.
| @@ -19,6 +20,7 @@ class Challenge | |||
|
|
|||
| init { | |||
| this.validUntil = max(0L, validUntil) | |||
| MessageValidator.verify().isNotEmptyBase64(value, "challenge").validFutureTimes(this.validUntil) | |||
There was a problem hiding this comment.
By checking the protocol, it seems that the value argument has some guidelines that should probably be checked (the regex pattern and encoding, the 32 byte length). Also the encoding seems to be Hex and not base64, I am not sure it this changes something about it.
json protocol
// ../protocol/query/method/message/data/dataFederationChallenge.json
{
"$schema": "http://json-schema.org/draft-07/schema#",
"$id": "https://raw.githubusercontent.com/dedis/popstellar/master/protocol/query/method/message/data/dataFederationChallenge.json",
"description": "Challenge object in the context of federation authentication",
"type": "object",
"properties": {
"object": {
"const": "federation"
},
"action": {
"const": "challenge"
},
"value": {
"type": "string",
"contentEncoding": "hex",
"pattern": "^[0-9a-fA-F]{64}$",
"$comment": "A 32 bytes array encoded in hexadecimal"
},
"valid_until": {
"type": "integer",
"description": "[Timestamp] of the expiration time",
"minimum": 0
}
},
"additionalProperties": false,
"required": [
"object",
"action",
"value",
"valid_until"
]
}
this.value never assigned? I might just not have catch the thing
There was a problem hiding this comment.
Ooh true, good catch ! I thought it was base64. However the challenges are not always hex, I need to check that with the team. For now, I'll check that it is not empty, otherwise it would break everything. Oh and this.value is set when constructing the challenge
| @@ -15,6 +16,10 @@ class ChallengeRequest | |||
| */ | |||
| (val timestamp: Long) : Data { | |||
|
|
|||
| init { | |||
| MessageValidator.verify().validPastTimes(timestamp) | |||
There was a problem hiding this comment.
given the protocol :
"timestamp": {
"type": "integer",
"description": "[Timestamp] of the request",
"minimum": 0
}
timestamp should be >= 0, however validPastTime does not check this. I think that this check should be preceded by greaterOrEqualThan
| init { | ||
| MessageValidator.verify() | ||
| .isNotEmptyBase64(laoId, "lao_id") | ||
| .stringNotEmpty(serverAddress, "server_address") |
There was a problem hiding this comment.
I am wondering if it would be a good idea to create a function in MessageValidator to check for server addresses pattern. It is specified in the protocol :
"server_address": {
"type": "string",
"pattern": "^(ws|wss):\/\/.*(:\\d{0,5})?\/.*$",
"$comment": "public address of the remote organizer server"
},
There was a problem hiding this comment.
For that point, the pattern might change as (if I remember well) some addresses didn't fit in the pattern. It will be added once the protocol is updated.
|
|
|
|
There was a problem hiding this comment.
Looks good to me.
Maybe you should just write self-explanatory TODOs for the value validation (encoding, regex), and for the server address. The thing is, by the time these points are clearer in the protocol, nobody will still be there to remember that this was to do to finalize a clean validation of the data.
I think very clear TODOs with your name + date would be a good addition 👍
|
Yes it's a good idea to write that somewhere. However we might forget a TODO in the code (nobody will see it unless working on these specific lines of the project), so I think it's better if I mention that in issue #1751 so we can see what still needs validation more globally :) |
Added message validation on all federation messages that hadn't validation before