Skip to content

MSC4127: Removal of query string auth #4127

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

Open
turt2live wants to merge 1 commit into
base: main
Choose a base branch
from
+56 −0
Open

MSC4127: Removal of query string auth #4127

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
56 changes: 56 additions & 0 deletions proposals/4127-remove-query-string-auth.md
View file
Open in desktop
Copy link
Member Author

@turt2live turt2live Apr 10, 2024

Choose a reason for hiding this comment

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

Implementation requirements:

  • None, in my opinion. The MSC is not feasible to implement beyond what has already been described in the proposal itself.

Copy link
Member

@tulir tulir Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I looked through https://matrix.org/ecosystem/ and found the following projects that seemed to still be using the query parameter:

There are also a bunch of matches on GitHub search, but I'd guess most of them are ad-hoc scripts, unmaintained projects and other such things: https://github.com/search?q=%2F_matrix+access_token%3D++NOT+is%3Aarchived+NOT+is%3Afork&type=code

Copy link
Member

@tulir tulir Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure how GitHub sorts those search results, but when I opened it again, the order had changed and the top result was GitLab. Should probably open an issue or PR for them 🤔 https://gitlab.com/gitlab-org/gitlab/-/blob/v17.8.0-ee/app/models/integrations/matrix.rb#L7

Looked through some of the search results, others that are probably worth notifying:

Copy link
Member Author

@turt2live turt2live Jan 20, 2025

Choose a reason for hiding this comment

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

libcmatrix, simplematrixbotlib, mxbt, ttm, and hemppa appear to fit a definition of 'maintained', so may be quickly fixed by the pings (hopefully).

I've tried starting a fix for gitlab, but forking the project is taking too long, so will have to be another time if no one else does it :D

Copy link
Member

@tulir tulir Jan 20, 2025

Choose a reason for hiding this comment

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

GitLab seems to have some kind of system where you don't have to make your own fork https://docs.gitlab.com/ee/development/contributing/first_contribution/index.html

It automatically sent an access request without further prompting when I clicked the link in the second bullet point, so I guess I'll try making a patch too :D

Copy link
Member

@tulir tulir Jan 20, 2025

Choose a reason for hiding this comment

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

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178475

Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# MSC4127: Removal of query string auth

See [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126) for details on query
string authentication and the prerequisite deprecation.

## Proposal

Query string authentication becomes *removed* from the [Client-Server API](https://spec.matrix.org/v1.10/client-server-api/#using-access-tokens)
and [Identity Service API](https://spec.matrix.org/v1.10/identity-service-api/#authentication).
Comment on lines +8 to +9
Copy link
Member

@clokep clokep Apr 12, 2024

Choose a reason for hiding this comment

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

An issue that Synapse ran into with MSC2832 which removed query parameter auth for app services is that we did not want to drop support for old spec versions, but did want to follow this guidance. (matrix-org/synapse#15379 has some of the discussion.)

We ended up saying Synapse was still compatible with 1.4, but had a configuration flag for re-enabling the old behavior (disabling it by default in the name of secure defaults).

I'm not sure if I have a recommendation, but wanting to be both backwards compatible and secure by default was difficult in this situation. I guess one option would have been to only declare support for the older versions if the config flag was flipped, but I think various clients would have stopped working if we did that.


The prior deprecation required by the [deprecation policy](https://spec.matrix.org/v1.10/#deprecation-policy)
is described by [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126).

As a process note, it was considered to bundle the deprecation and removal into MSC4126 instead of
having two separate MSCs, breaking the deprecation policy slightly, though this introduces consequences
which are, in the author's opinion, undesirable. Specifically, the spec release changelog for the
deprecation would say "as per MSC4126" and imply the MSC being moved to the [merged](https://spec.matrix.org/proposals/#process)
state, however the second half of the MSC (the feature removal) would not have landed yet. This would
leave the MSC stuck in an awkward `spec-pr-missing` state because half of it had not been written up
or landed yet. To maintain a clearer changelog, ease process considerations, and give clients another
opportunity to object to the removal, the two MSC approach required by the deprecation policy is best.

## Potential issues

See [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126).

## Alternatives

See [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126).

More of a process note, during drafting of this proposal the author considered combining the deprecation
and removal into a single MSC, against the advice of the deprecation policy. The aim was to reduce
process burden and more quickly get the security risk removed from the specification, though merging
both operations into a single MSC potentially makes the spec release changelog unclear and leaves the
MSC in an awkward released-but-not-merged state. To avoid figuring out how to navigate the process
states, increasing the process burden rather than decreasing it, the deprecation policy's two MSC
requirement has been followed here. However, there are still other process efficiencies which can
take place - see the "Dependencies" section of this MSC for details.

## Security considerations

See [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126).

## Unstable prefix

This proposal cannot feasibly have an unstable prefix. Clients are already discouraged from using
query string authentication and should switch to `Authorization` as soon as possible, regardless of
this MSC.

See [MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126) for related details.

## Dependencies

[MSC4126](https://github.com/matrix-org/matrix-spec-proposals/pull/4126) must be *released* in the
specification before this proposal can be merged, though this MSC *may* progress through Final Comment
Period concurrent to MSC4126 if desirable.