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

spec for policy rules does not match reality #17

Closed
richvdh opened this issue Feb 1, 2022 · 0 comments · Fixed by #1165
Closed

spec for policy rules does not match reality #17

richvdh opened this issue Feb 1, 2022 · 0 comments · Fixed by #1165
Labels
spec-bug Something which is in the spec, but is wrong

Comments

@richvdh
Copy link
Member

richvdh commented Feb 1, 2022

Link to problem area:

According to https://spec.matrix.org/v1.1/client-server-api/#events-19, within policy rules:

Glob characters * and ? can be used to match zero or more and one or more characters respectively.

Issue

According to discussion at https://matrix.to/#/!NasysSDfxKxZBzJJoE:matrix.org/$-g-gRQFjxbsHNe2OpGf5yGGHVNsJcVCga-sF4WwnWOA?via=matrix.org&via=libera.chat&via=envs.net, this does not match what happens in mjolnir.

Policy rules were introduced by MSC2313, which says: "Simple globs are supported for defining entities (* and ? as wildcards, just like m.room.server_acl)". Mjolnir does interpret globs in the same way as server_acl - ie, they match zero or more, and exactly one, character respectively.

This therefore seems to have been a bug introduced in the spec by matrix-org/matrix-spec-proposals#2434.

Expected behaviour

The use of ? to match one or more characters would be ... unusual. Since it's never been interpreted that way, changing the spec seems more sensible.

@richvdh richvdh added the spec-bug Something which is in the spec, but is wrong label Feb 1, 2022
@richvdh richvdh transferred this issue from matrix-org/matrix-spec-proposals Mar 1, 2022
deepbluev7 added a commit to deepbluev7/matrix-spec that referenced this issue Jul 10, 2022
The specification here does neither match how globs work in common
libraries nor do they match how they are used in practice currently or
how the MSC worded them (which said they should be like server ACLs). As
such this seems to be an issue introduced when writing the spec text.

Ref mjolnir:
- https://github.com/matrix-org/mjolnir/blob/b48904bc2b4fcd636176b12dbe173ad651930f23/src/models/ListRule.ts#L44

Ref matrix bot sdk (which implements the glob used above):
- https://github.com/turt2live/matrix-bot-sdk/blob/473e563236dd6edb25e7bd18d3517d1a61e037a1/src/helpers/MatrixGlob.ts#L26
- https://github.com/turt2live/matrix-bot-sdk/blob/f799b1fe1a72b9a4a6053c50cedfb43bee962558/test/helpers/MatrixGlobTest.ts#L44

Ref original MSC:
- https://github.com/matrix-org/matrix-spec-proposals/blob/c7b3d998537d21694a166b4a6a4cf0490ebc0cc2/proposals/2313-moderation-policy-rooms.md?plain=1#L36
- https://spec.matrix.org/v1.3/client-server-api/#server-access-control-lists-acls-for-rooms

fixes matrix-org#17

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
turt2live pushed a commit that referenced this issue Jul 11, 2022
* Fix wording for globs in policy lists

The specification here does neither match how globs work in common
libraries nor do they match how they are used in practice currently or
how the MSC worded them (which said they should be like server ACLs). As
such this seems to be an issue introduced when writing the spec text.

Ref mjolnir:
- https://github.com/matrix-org/mjolnir/blob/b48904bc2b4fcd636176b12dbe173ad651930f23/src/models/ListRule.ts#L44

Ref matrix bot sdk (which implements the glob used above):
- https://github.com/turt2live/matrix-bot-sdk/blob/473e563236dd6edb25e7bd18d3517d1a61e037a1/src/helpers/MatrixGlob.ts#L26
- https://github.com/turt2live/matrix-bot-sdk/blob/f799b1fe1a72b9a4a6053c50cedfb43bee962558/test/helpers/MatrixGlobTest.ts#L44

Ref original MSC:
- https://github.com/matrix-org/matrix-spec-proposals/blob/c7b3d998537d21694a166b4a6a4cf0490ebc0cc2/proposals/2313-moderation-policy-rooms.md?plain=1#L36
- https://spec.matrix.org/v1.3/client-server-api/#server-access-control-lists-acls-for-rooms

fixes #17

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>

* Add changelog

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec-bug Something which is in the spec, but is wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant