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

Matching remote users against the application service user namespace regex is a footgun #1272

Closed
MadLittleMods opened this issue Oct 4, 2022 · 5 comments · Fixed by matrix-org/matrix-spec-proposals#3905
Labels
wart A point where the protocol is inconsistent or inelegant

Comments

@MadLittleMods
Copy link
Contributor

Thanks for questioning this @erikjohnston. After thinking about this more and asking in #matrix-spec, it indeed seems that there are no valid use cases for needing to specify a remote user in an application service's user namespace. By specifying local users, you will receive room events/presence updates from remote users anyhow.

As @Half-Shot put it, defining remote users is merely a footgun for which an Application Service may assume that it'll receive all events sent by that user, even though it will only receive events in rooms that are shared with a local user.

So by that logic, I'm fine with us defining (and limiting in the code) user namespaces to only cover local users. It would make sense to update the spec to include the same. Sorry for the run-around!

-- @anoadragon453, matrix-org/synapse#14000


This behavior has been updated in Synapse via matrix-org/synapse#13958

@turt2live
Copy link
Member

So by that logic, I'm fine with us defining (and limiting in the code) user namespaces to only cover local users. It would make sense to update the spec to include the same. Sorry for the run-around!

The spec should be clear that it's only events the server has access to (thus rooms which are shared with that server) - changing the server-side code to deny remote users would be a spec violation, at least without an MSC to back it.

@anoadragon453

@turt2live turt2live added wart A point where the protocol is inconsistent or inelegant and removed clarification An area where the expected behaviour is understood, but the spec could do with being more explicit labels Oct 5, 2022
@anoadragon453
Copy link
Member

I agree. So not denying shouldn't require an MSC.

@turt2live
Copy link
Member

I agree. So not denying shouldn't require an MSC.

Too many double negatives: At best, synapse can require a config option be set to allow the namespace, however it cannot take the functionality away on its own.

@turt2live
Copy link
Member

ftr, matrix-org/synapse#13958 (comment) has more words on this (including the archeaology)

@MadLittleMods
Copy link
Contributor Author

Created an MSC for this: MSC3905

turt2live pushed a commit to matrix-org/matrix-spec-proposals that referenced this issue Oct 24, 2022
#3905)

* MSC so appservices only interested in local users

Fix matrix-org/matrix-spec#1272

* Add MSC number

* Fix typo

* Fix title

* Add example implementation

See #3905 (comment)

* `rooms` and `aliases` not affected

See #3905 (comment)

* Add historical context

See #3905 (comment)

* Document how to do unstable

See #3905 (comment)

* fix some typos

* Add link to spec

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Co-authored-by: Andrew Morgan <andrew@amorgan.xyz>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wart A point where the protocol is inconsistent or inelegant
Projects
None yet
3 participants