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

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

Conversation

tulir
Copy link
Member

@tulir tulir commented Jan 7, 2021

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.

Comment on lines +17 to +19
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.
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

Comment on lines +4 to +6
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].
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.

@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Jan 7, 2021
Comment on lines +25 to +26
2. Recommend that clients always include authorization in requests to the
homeserver if they have an access token.
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.

## Security considerations
None that I can think of.

## Alternatives
Copy link
Contributor

@Kladki Kladki Jun 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants