-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC4127: Removal of query string auth #4127
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Client SDKs
- libcmatrix: https://source.puri.sm/Librem5/libcmatrix/-/blob/c4e9a8552f6870829a987820cc0f66323dd5e9ff/src/cm-net.c#L271 (cc @agx)
- Matrix-ClientServer-API-java: https://github.com/JojiiOfficial/Matrix-ClientServer-API-java/blob/cb733066b8cc0c1ea51047efba8212de67965331/src/main/java/de/jojii/matrixclientserver/Bot/Syncee.java#L83 (cc @JojiiOfficial)
- I couldn't find how https://github.com/Dominaezzz/matrix-kt does auth, but it's archived
- Bot SDKs
- MatrixBot .NET Core SDK: https://github.com/enimatek-nl/matrixbot-dotnet-sdk/blob/adebefc612de3f002a1f1cc3183923745876709a/MatrixBot.Sdk/MatrixBot.cs#L202
- simplematrixbotlib: https://codeberg.org/imbev/simplematrixbotlib/src/commit/17ead9cb2cc86b09e39c21acc015ad1500d4ed4b/simplematrixbotlib/api.py#L112 (I think it uses matrix-nio for most requests, so probably just that one outdated request - cc @imbev)
- mxbt: https://codeberg.org/librehub/mxbt/src/commit/aee47b2a47c2e997277c727dac61d301850785de/mxbt/bot.py#L127 (seems to be copied from simplematrixbotlib)
- Bots
- Bridges: none
- Clients: none (but Chatty uses libcmatrix mentioned above)
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
There was a problem hiding this comment.
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:
- https://github.com/sitespeedio/sitespeed.io/blob/f8a6408b7c0b6734e68976dbd9db93070f8d21ea/lib/plugins/matrix/send.js#L20
- https://github.com/badges/shields/blob/152b8e9a64623ce5677c1ef3cd802f7291855611/services/matrix/matrix.service.js#L200 (apparently shields.io still defaults to guest access for non-matrix.org servers, and the guest access uses the query param)
- https://github.com/netdata/netdata/blob/fa91f42a15470c6ed1d478cbe40f89de81d2d6b8/src/health/notifications/alarm-notify.sh.in#L2099
- https://github.com/TwiN/gatus/blob/fa3e5dcc6e0138953042d0ac68c576f13e6c10ad/alerting/provider/matrix/matrix.go#L106
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
There was a problem hiding this comment.
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.
I'm more of a drive by contributor to libcmatrix but put out a patch here: https://source.puri.sm/Librem5/libcmatrix/-/merge_requests/104 . Is there a recommendation for falling back to the query parameter for older (pre 1.4? versions)? |
No need for fallbacks, the Authorization header has been supported since r0.3.0 (released in 2017) https://spec.matrix.org/legacy/client_server/r0.3.0.html#client-authentication |
Addressed in simplematrixbotlib v2.12.2 https://codeberg.org/imbev/simplematrixbotlib/commit/6522377ebcd1da58f6912cd17696a2dac6dd791e |
Rendered
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am Director of Standards Development at The Matrix.org Foundation C.I.C., Matrix Spec Core Team (SCT) member, employed by Element, and operate the t2bot.io service. This proposal is written and published with my role as a member of the SCT.
Requires #4126
Fixes matrix-org/matrix-spec#1780