Skip to content

Conversation

@anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Oct 20, 2025

This was causing Synapse to respond with validation errors for these tests:

2025-10-18 09:24:32,660 - synapse.http.server - 135 - INFO - POST-6877 - <SynapseRequest at 0x7f071c58d9d0 method='POST' uri='/_matrix/client/v3/keys/upload?access_token=<redacted>' clientproto='HTTP/1.1' site='8800'> SynapseError: 400 - 3 validation errors for KeyUploadRequestBody
device_keys.algorithms
  Field required [type=missing, input_value={'user_id': '@anon-202510...evice_id': 'TRIISNQZFQ'}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.12/v/missing
device_keys.keys
  Field required [type=missing, input_value={'user_id': '@anon-202510...evice_id': 'TRIISNQZFQ'}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.12/v/missing
device_keys.signatures
  Field required [type=missing, input_value={'user_id': '@anon-202510...evice_id': 'TRIISNQZFQ'}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.12/v/missing

...which caused the test to fail (which we were expecting). Yet, we were looking for failure due to the wrong user ID being passed, not the request body's structure.

This PR fixes up the structure of the request body to avoid the validation error. We now see the correct error in the logs:

2025-10-20 17:09:20,357 - synapse.http.server - 135 - INFO - POST-3 - <SynapseRequest at 0x7f091c1570d0 method='POST' uri='/_matrix/client/v3/keys/upload?access_token=<redacted>' clientproto='HTTP/1.1' site='8800'> SynapseError: 400 - Provided `user_id` in `device_keys` does not match that of the authenticated user

Ideally we would differentiate the errors based on errcode, but the spec doesn't currently define any for this endpoint.

This test was failing due to the shape of the request missing fields,
rather than due to the user ID being different from the requester, which
is what we're trying to test.
@anoadragon453 anoadragon453 requested a review from a team as a code owner October 20, 2025 17:10
"ed25519:".$user->device_id => "ed25519+key",
},
signatures => {},
},
Copy link
Contributor

@MadLittleMods MadLittleMods Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is caused by changes from element-hq/synapse#17097 / element-hq/synapse#19023 ?

I'm a bit confused about how Sytest is passing on Synapse develop if it's from those PRs.

Or since it's optional, why it's failing in (unknown, as described in PR title)?

Copy link
Member Author

@anoadragon453 anoadragon453 Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was passing because it expects a 4xx error:

)->main::expect_http_4xx;

but, as explained in the PR description, we were getting a 400 due to the Pydantic model validation fail. Whereas the test is trying to check that the homeserver will return a 400 when uploading device keys for a user other than yourself.

Comment on lines +94 to +99
algorithms => ["m.olm.curve25519-aes-sha256", "m.megolm.v1.aes-sha"],
keys => {
"curve25519:".$user->device_id => "curve25519+key",
"ed25519:".$user->device_id => "ed25519+key",
},
signatures => {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields make sense in terms of Pydantic model in Synapse (see DeviceKeys)

Feel free to merge if things make sense to you. The fields themselves seem sane.

@anoadragon453
Copy link
Member Author

Dendrite test failures appear unrelated to this PR.

@anoadragon453 anoadragon453 merged commit 14832b8 into develop Oct 24, 2025
12 of 14 checks passed
@anoadragon453 anoadragon453 deleted the anoa/missing_algorithms_fields branch October 24, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants