Optional whitespace support in Authorization (#1350)

[Timshel]

Fix #1350 by adding support for optional whitespace around ,.

Additionally,

• used \s which is more lax than the spec since OWS is defined as *( SP / HTAB )
• Added the X-Matrix part to the regex ^X-Matrix\s+ to make more clear what it's doing. Added it as insensitive to lower probability of issues, but since it was not checking the X-Matrix part before it could create issues.
• On the whitespace handling of this part replaced + with \s+ since the parsing appear to be more lax than the spec (which if I understand correctly mandate only one space, cf 1*SP)

Will of course remove it if you believe it should not be included.

[CLAassistant]

All committers have signed the CLA.

[Timshel]

Concerning the CLA has no issue with signing it but would prefer to do it only if/when the PR is ready to be merged.

[anoadragon453]

As the spec mentions RFC7235 explicitly for the format of the Authorization header's value, it seems that we should indeed attempt to follow it.

We need to be careful with being more lax than the spec, lest other homeserver implementations start to rely on Synapse's behaviour, and then fail when they encounter other homeservers that follow the spec to the letter.

• Added the X-Matrix part to the regex ^X-Matrix\s+ to make more clear what it's doing. Added it as insensitive to lower probability of issues, but since it was not checking the X-Matrix part before it could create issues.

Note that we already check that the value of Authorization starts with X-Matrix (case-sensitive) here:

synapse/synapse/federation/transport/server/_base.py

Line 105 in 23740ea

if auth.startswith(b"X-Matrix"):

Thus the case-insensitivity here won't apply unless we also make that check case-insensitive. However, as the spec doesn't allow X-Matrix to be lowercase, I think we should keep the check case-sensitive.

• On the whitespace handling of this part replaced + with \s+ since the parsing appear to be more lax than the spec (which if I understand correctly mandate only one space, cf 1*SP)

RFC5234 defines SP as %x20, aka hexadecimal 20, or simply . Therefore \s, which matches any whitespace character, is not the same. Indeed, RFC7235 mandates 1*SP.

I also think that the spec should be updated to mention in its summary of RFC7235 that spaces and tabs are allowed around commas. This would help prevent implementations from drifting apart. I don't currently see a relevant issue. I can open one, unless you'd like to.

[Timshel]

Hey,
Rebased and forced pushed the change since there is not much code.

I also think that the spec should be updated to mention in its summary of RFC7235 that spaces and tabs are allowed around commas. This would help prevent implementations from drifting apart. I don't currently see a relevant issue. I can open one, unless you'd like to.

I'll let you tackle this one, maybe next time :).

[anoadragon453]

The code looks good to me. Spec issue written up at matrix-org/matrix-spec#1817.

[Timshel]

Had the notif for the failed test will add the changelog later today.

[anoadragon453]

Before merging, I've just made a call out out to others to ask if anyone has any concerns with this change accidentally introducing incompatibilities.

[anoadragon453]

Initial reactions are positive. I've made a PR to the spec to add a note about the whitespace to the summary: matrix-org/matrix-spec#1818