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

synapse does not escape forward slashes in key IDs when making a federation key/v2/server/... request #14488

Closed
DMRobertson opened this issue Nov 18, 2022 · 4 comments · Fixed by #14490
Assignees

Comments

@DMRobertson
Copy link
Contributor

DMRobertson commented Nov 18, 2022

response = await self.client.get_json(
destination=server_name,
path="/_matrix/key/v2/server/"
+ urllib.parse.quote(requested_key_id),
ignore_backoff=True,
# we only give the remote server 10s to respond. It should be an
# easy request to handle, so if it doesn't reply within 10s, it's
# probably not going to.
#
# Furthermore, when we are acting as a notary server, we cannot
# wait all day for all of the origin servers, as the requesting
# server will otherwise time out before we can respond.
#
# (Note that get_json may make 4 attempts, so this can still take
# almost 45 seconds to fetch the headers, plus up to another 60s to
# read the response).
timeout=10000,
)

urllib.parse.quote doesn't escape slashes by default: because it expects to given an entire url path

ouchie

@DMRobertson DMRobertson self-assigned this Nov 18, 2022
@DMRobertson DMRobertson changed the title synapse does not escape URL-unsafe characters in key IDs when making a federation key/v2/server/... request synapse does not escape forward slashes in key IDs when making a federation key/v2/server/... request Nov 18, 2022
DMRobertson pushed a commit that referenced this issue Nov 18, 2022
@DMRobertson
Copy link
Contributor Author

As @squahtx says, we should audit other uses of urllib.parse.quote to see if there are other problems like this.

Grep for ((urllib\.)?parse\.)?quote\(.

@clokep
Copy link
Member

clokep commented Nov 18, 2022

As @squahtx says, we should audit other uses of urllib.parse.quote to see if there are other problems like this.

Grep for ((urllib\.)?parse\.)?quote\(.

@DMRobertson Did we do this / find anymore?

@DMRobertson
Copy link
Contributor Author

As @squahtx says, we should audit other uses of urllib.parse.quote to see if there are other problems like this.
Grep for ((urllib\.)?parse\.)?quote\(.

@DMRobertson Did we do this / find anymore?

Other than constructing the regex, I haven't done this. It's on my to-do list, but maybe should raise another issue to track it.

@clokep
Copy link
Member

clokep commented Nov 21, 2022

Other than constructing the regex, I haven't done this. It's on my to-do list, but maybe should raise another issue to track it.

Done: #14511.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants