matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.79k stars 2.13k forks source link

Unused legacy fallback code for `public_key` response to `/_matrix/identity/v2/store-invite` #6036

Open anoadragon453 opened 5 years ago

anoadragon453 commented 5 years ago

Noticed by @richvdh during review of https://github.com/matrix-org/synapse/pull/5979/files#r323956886

https://github.com/matrix-org/synapse/blob/c755955f335984dc6f97a269b57ad955f257ef8f/synapse/handlers/room_member.py#L1037-L1047

https://github.com/matrix-org/synapse/blob/c755955f335984dc6f97a269b57ad955f257ef8f/synapse/handlers/room_member.py#L873-L876


Original comment:

while I'm here (it's not really relevant to the review, but): wtf is going on here?

  1. public_key isn't specced anywhere, afaict
  2. if public_key isn't set, we set fallback_public_key to public_keys[0] which, according to the spec, has a completely different shape to what we claim to return. (Edit: fixed by https://github.com/matrix-org/matrix-spec/pull/1486)

The calling code seems to imply that fallback_public_key is only used to populate some fields "For backwards compatibility", but said fields are in the spec.

I'm not suggesting changing anything here as part of this PR, but it looks like there's some bogosity, which suggests to me that the code is either unused (so can be killed) or broken (so should be fixed).

richvdh commented 1 year ago

This code is now here.

Sydent returns:

{
    "token": "token",
    "public_key": "sydentPubKeyBase64",
    "public_keys": [
        {
            "public_key": "sydentPubKeyBase64",
            "key_validity_url":  "https://<server>/_matrix/identity/api/v1/pubkey/isvalid"
        },
        {
            "public_key": "ephemeralPublicKeyBase64",
            "key_validity_url": "https://<server>/_matrix/identity/api/v1/pubkey/ephemeral/isvalid",
        }
    ],
    "display_name": "<redacted email>"
}

public_key is undocumented (and as far as I can tell, always has been). public_keys matches the spec if you squint very hard.

Anyway, looking at the response above, we can see that the Synapse code is trying to take a response with no public_keys and map the public_key property onto a similar-shaped object. It will never be used, because there has never existed a version of /_matrix/identity/v2/store-invite which doesn't return a public_keys.

In short, this is dead code which needs ripping out.