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.

Sign up for GitHub

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

MSC2949: Proposal to clarify "Requires auth" and "Rate-limited" in the spec #2949

Open
wants to merge 1 commit into
base: old_master
Choose a base branch
from
+42 −0
Open

MSC2949: Proposal to clarify "Requires auth" and "Rate-limited" in the spec #2949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions proposals/2949-optional-auth.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Proposal to clarify "Requires auth" and "Rate-limited" in the spec
All endpoints in the spec have values for "Requires auth" and "Rate-limited".
The values are booleans, and the spec does not say how strictly they should be
interpreted. There are already multiple endpoints, such as [GET /publicRooms],
which don't require auth in the spec, but may require auth in [some server
implementations].
Comment on lines +4 to +6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these sorts of auth differences should be their own MSCs, or point 2 of the proposal below should be expanded upon to clarify the benefits.


[GET /publicRooms]: https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-publicrooms
[some server implementations]: https://github.com/matrix-org/synapse/blob/v1.24.0/docs/sample_config.yaml#L103-L107

This ambiguity and inconsistency causes problems when client developers read
the values in the spec as requirements and don't pass authentication even when
they have a valid access token (see [ruma/ruma#380]).

[ruma/ruma#380]: https://github.com/ruma/ruma/issues/380

Inconsistencies in the "Rate-limited" value doesn't have consequences that are
as obvious, but it is a similar problem nevertheless: Synapse has rate limits
on multiple endpoints that are marked as not rate limited in the spec.
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a documentation bug, not something that needs an MSC


## Proposal
1. Make it clear that the rate-limited and requires auth values in the spec are
merely recommendations and the actual rate limits and auth requirements may
vary on different servers.
2. Recommend that clients always include authorization in requests to the
homeserver if they have an access token.
Comment on lines +25 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of having a “maybe” if clients are supposed to send the token anyway? The “Change all the potentially-requires-auth endpoints to always require auth.” makes more sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that a client library could refuse to send a request without an access token to an endpoint that definitely requires auth. For 'maybe', you would disable that check and send the auth token if you have one, but still attempt the request if you don't.


## Potential issues
None that I can think of.

## Security considerations
None that I can think of.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some endpoints that redirect, possibly to external sites. Adding an Authentication header to those in a browser environment will result in the browser sending the same Authentication header in the new request. There is not even a way to prevent this (I spent a long time researching this recently).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, interesting. I guess in those cases the spec should explicitly forbid adding the auth headers. Are there any current endpoints that will redirect to an external site and are used after logging in, or should it just be added as a note for potential future endpoints?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only one in the client-server spec that explicitly mentions redirects is the one for SSO. Not sure about GET /_matrix/media/r0/download/{serverName}/{mediaId}, it seems realistic one would implement that via redirects, but the spec doesn't mention that possibility.


## Alternatives
Copy link
Contributor

@Kladki Kladki Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For rate limiting, we can instead recommend how much endpoints should be rate limited.

* Add a "maybe" value for "Requires auth" and define that any yes/no value is a
requirement instead of recommendation.
* Change all the potentially-requires-auth endpoints to always require auth.
This would mean viewing room directories always requires auth.
* Remove the whole "Rate-limited" value.

## Unstable prefix
Not applicable.