Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

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

Open
anoadragon453 opened this issue Sep 13, 2019 · 1 comment
Labels
A-Spec-Compliance places where synapse does not conform to the spec O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented Sep 13, 2019

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

# TODO: Check for success
token = data["token"]
public_keys = data.get("public_keys", [])
if "public_key" in data:
fallback_public_key = {
"public_key": data["public_key"],
"key_validity_url": "%s%s/_matrix/identity/api/v1/pubkey/isvalid"
% (id_server_scheme, id_server),
}
else:
fallback_public_key = public_keys[0]

if "public_key" not in key_data:
raise AuthError(
401, "No public key named %s from %s" % (key_name, server_hostname)
)


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 Corrections to the response format of /_matrix/identity/v2/store-invite matrix-spec#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).

@neilisfragile neilisfragile added z-bug (Deprecated Label) z-p2 (Deprecated Label) labels Sep 24, 2019
@richvdh richvdh changed the title Unknown "public_key" nonsense on v2 lookup Synapse doesn't follow the spec for /_matrix/identity/api/v1/pubkey/isvalid Jan 15, 2020
@richvdh richvdh added the A-Spec-Compliance places where synapse does not conform to the spec label Mar 19, 2020
@richvdh richvdh changed the title Synapse doesn't follow the spec for /_matrix/identity/api/v1/pubkey/isvalid Synapse doesn't follow the spec for /_matrix/identity/v2/store-invite Nov 25, 2022
@richvdh richvdh changed the title Synapse doesn't follow the spec for /_matrix/identity/v2/store-invite Unused legacy fallback code for public_key response to /_matrix/identity/v2/store-invite Nov 25, 2022
@richvdh
Copy link
Member

richvdh commented Nov 25, 2022

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.

@DMRobertson DMRobertson added T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. Z-Cleanup Things we want to get rid of, but aren't actively causing pain S-Tolerable Minor significance, cosmetic issues, low or no impact to users. O-Uncommon Most users are unlikely to come across this or unexpected workflow T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. and removed z-bug (Deprecated Label) z-p2 (Deprecated Label) T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Spec-Compliance places where synapse does not conform to the spec O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Cleanup Things we want to get rid of, but aren't actively causing pain
Projects
None yet
Development

No branches or pull requests

4 participants