Validate the body of requests to /keys/upload
#17097
Conversation
…validate-upload-keys-dict
…validate-upload-keys-dict
…validate-upload-keys-dict
|
|
As we are now well past Synapse 1.135. This was originally added in #17097
I really wanted to use a `UserID` instead of a `StrictStr` for the fields that contain a user ID. But this became too cumbersome due to the handler code wanting to directly json-encode the request body. As `UserID` does not subclass `str`, one would have to rebuild the entire containing object in order to json-encode it. Perhaps a future PR will do this, as it would allow us to validate UserID's more easily at the edge.
Some extra validation of the request body.
anoadragon453
force-pushed
the
s7evink/validate-upload-keys-dict
branch
from
c43ca16 to
0eaf28f
Compare
|
This PR is now ready for re-review and has been rewritten using Pydantic to validate user input. Trial tests failing are expected until #18996 is merged. |
anoadragon453
changed the title
/keys/upload
There's little point in coverting the pydantic model back to a dict when we already have it via the parsed body.
| # storing the result in the DB. There's little point in converted to a | ||
| # parsed object and then back to a dict. | ||
| body = parse_json_object_from_request(request) | ||
| validate_json_object(body, self.KeyUploadRequestBody) |
There was a problem hiding this comment.
We should use parse_and_validate_json_object_from_request(...) and pass in a concrete type to self.e2e_keys_handler.upload_keys_for_user(...)
Parse, don't validate (we shouldn't lose the type data after sussing it out)
There was a problem hiding this comment.
Looks like this was already explained in the comment above:
synapse/synapse/rest/client/keys.py
Lines 232 to 238 in ee9768c
Perhaps pragmatic and better than before so we can move forward with it ⏩
As a break-down of what upload_keys_for_user(...) does with the data:
upload_keys_for_user->upload_device_keys_for_user->encode_canonical_jsonupload_keys_for_user->_upload_one_time_keys_for_user, we manually iterate and re-encode anyway so a parsed object would be good hereupload_keys_for_user->set_e2e_fallback_keys->encode_canonical_json
Pydantic does support serialization so it wouldn't be that awkward if we just used a parsed object until we needed to serialize for encode_canonical_json.
If we want to avoid the deserialization/serialization, we could use TypedDict for these keys in the parsed object 🤔
| if "device_keys" in body: | ||
| # Validate the provided `user_id` and `device_id` fields in | ||
| # `device_keys` match that of the requesting user. We can't do | ||
| # this directly in the pydantic model as we don't have access | ||
| # to the requester yet. | ||
| # | ||
| # TODO: We could use ValidationInfo when we switch to Pydantic v2. | ||
| # https://docs.pydantic.dev/latest/concepts/validators/#validation-info | ||
| if body["device_keys"]["user_id"] != user_id: |
There was a problem hiding this comment.
We could add a check/validate methods to KeyUploadRequestBody for this logic
Plays into wanting to use parse_and_validate_json_object_from_request(...) as well
| if not isinstance(v, dict): | ||
| raise TypeError("fallback_keys must be a mapping") | ||
|
|
||
| for k, _ in v.items(): |
There was a problem hiding this comment.
| for k, _ in v.items(): | |
| for k in v.keys(): |
Might as well
|
|
||
| one_time_keys: Optional[Mapping[StrictStr, Union[StrictStr, KeyObject]]] = None | ||
| """ | ||
| One-time public keys for “pre-key” messages. The names of the properties |
There was a problem hiding this comment.
| One-time public keys for “pre-key” messages. The names of the properties | |
| One-time public keys for "pre-key" messages. The names of the properties |
Weird quotes 🤷
@anoadragon453:
This PR adds Pydantic model-based validation to the
POST /_matrix/client/v3/keys/uploadendpoint. This prevents invalid request bodies from reaching the handler function and producing internal server errors.Requires #18996 before unit tests will pass.
I initially wanted to transform the request body model into some
attrs-based domain objects, rather than the bare dicts we're passing around internally today. Alas, this made the diff explode in size, so I've reverted to just making use of thebodys raw dict. It is validated though! The main issue was that deeper in the stack we attempt to encode a portion of the dict to canonical JSON, and trying to do this withattrsobjects was a nightmare.I had also hoped to include a new
UserIDTypein the Pydantic model, instead of usingStrictStr. But this caused a lot of faffing about with convertingUserIDtostrand back, and we don't even end up pulling any data out of the pydantic model anyhow. So I decided to ditch that.