Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

X-Matrix Authorization header format summary does not mention comma whitespace rules #1817

Closed
anoadragon453 opened this issue May 8, 2024 · 6 comments
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented May 8, 2024

Link to problem area: https://spec.matrix.org/v1.10/server-server-api/#authentication

Issue
The spec mentions that:

The format of the Authorization header is given in RFC 7235.

In summary, the header begins with authorization scheme X-Matrix, followed by one or more spaces, followed by a comma-separated list of parameters written as name=value pairs. The names are case insensitive and order does not matter. The values must be enclosed in quotes if they contain characters that are not allowed in tokens, as defined in RFC 7230; if a value is a valid token, it may or may not be enclosed in quotes. Quoted values may include backslash-escaped characters. When parsing the header, the recipient must unescape the characters. That is, a backslash-character pair is replaced by the character that follows the backslash.

This summary has missed out the detail that the comma-separated list of parameters may have optional whitespace (a space 0x20 or tab 0x09 specifically) around them. See this bit of RFC 7235:

Authorization = credentials

credentials = auth-scheme [ 1*SP ( token68 / [ ( "," / auth-param )
    *( OWS "," [ OWS auth-param ] ) ] ) ]

OWS is defined by RFC 7230 as:

OWS            = *( SP / HTAB )
                    ; optional whitespace

This was called out as an issue for Synapse a few years ago: matrix-org/synapse#1350 and a recently-submitted PR to Synapse has been filed to fix it: element-hq/synapse#17145

But I fear that the initial reason for this being missed was that the summary did not include this detail.

The "For compatibility with older servers, the sender should..." section should also be updated to include the bullet point:

  • do not include whitespace around the commas between name=value pairs.

Alternatively, we could update the spec to explicitly disallow whitespace around commas, which would match current implementations. But it would diverge from typical Authorization header behaviour, which may be confusing.

@anoadragon453 anoadragon453 added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label May 8, 2024
@anoadragon453
Copy link
Member Author

It appears that dendrite (or any homeserver using gomatrixserverlib) does not support optional whitespace around the commas either: https://github.com/matrix-org/gomatrixserverlib/blob/c2391f7113a5020d28d9fa5680ae025c20062126/fclient/request.go#L336

@anoadragon453
Copy link
Member Author

Conduit (or any homeserver using ruma) does not support optional whitespace around the commas either.

Pair names or values are not trimmed for whitespace: https://github.com/ruma/ruma/blob/b4d0ab42a370c36b5cbb2daf241eafa5e402a2d7/crates/ruma-server-util/src/authorization.rs#L112

And space or tab is not allowed in the name field: https://github.com/ruma/ruma/blob/b4d0ab42a370c36b5cbb2daf241eafa5e402a2d7/crates/ruma-server-util/src/authorization.rs#L193

@Kladki
Copy link
Contributor

Kladki commented May 8, 2024

For the record, Conduit doesn't use ruma for parsing the X-Matrix header for some reason (probably because @timokoesters didn't know or it didn't exist at the time). Still has the same issue tho.

@anoadragon453
Copy link
Member Author

anoadragon453 commented May 8, 2024

@Kladki Aha, thank you very much! I'll create an issue on Conduit as well then.

@anoadragon453
Copy link
Member Author

@anoadragon453
Copy link
Member Author

Fixed by #1818.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

2 participants