MSC 1597: Better spec for matrix identifiers #1598

richvdh · 2018-08-29T14:46:08Z

Rendered: https://github.com/matrix-org/matrix-doc/blob/rav/proposals/id_grammar/proposals/1597-id-grammar.md

proposals/1597-id-grammar.md

ara4n · 2018-08-29T22:42:47Z

proposals/1597-id-grammar.md

+User IDs are
+[well-specified](https://matrix.org/docs/spec/appendices.html#user-identifiers),
+however we should consider dropping `/` from the list of allowed characters, as
+it interferes with URI escaping (XXX: how, specifically?)


I think the handwavey concern is that any HTTP layers between the client and HTTP handler in the server (e.g. proxies, load balancers, http router in the server) might get confused by a / and either change its escaping or treat it as a path separator somehow.

Another reason to consider dropping it is that mxids that begin with / get confused with slash commands, and in practice @foo/bar:example.com in retrospect looks cosmetically quite strange - the / binds tighter than the :, so it almost looks like you are partitioning up your URL into @foo and bar:example.com.

Also, we've not actually ended up using it anywhere since introducing it (although that might just reflect the lack of investment on bridges :|)

Another reason to consider dropping it is that mxids that begin with / get confused with slash commands

As discussed online, I'm failing to see how this is an issue that you solve by forbidding slashes in mxids, unless you also forbid slashes in displaynames.

However, the HTTP proxy argument makes more sense, on consideration. Have updated.

proposals/1597-id-grammar.md

turt2live

Looks sensible to me as a proposal. I've commented on a few minor things, and added some points of information.

I'm also not sure how much editing you'd like to do, however there's some inconsistency between using "CS API" and "C-S API".

turt2live · 2018-08-30T22:47:57Z

proposals/1597-id-grammar.md

+[Spec](https://matrix.org/docs/spec/client_server/unstable.html#post-matrix-client-r0-user-userid-filter)
+
+These are generated by the server and then used in the CS API. They are only
+required to be unique for a given user. `{` is already forbidden by the spec.


Specifically, { is forbidden only at the start of the string. We should definitely forbid it throughout (which is proposed here already).

turt2live · 2018-08-30T22:49:42Z

proposals/1597-id-grammar.md

+
+Generated by sending server. Needs to be unique for a given pair of servers.
+
+[Synapse](https://github.com/matrix-org/synapse/blob/74854a97191191b08101821753c2672efc2a65fd/synapse/federation/transaction_queue.py#L593) uses a stringified int and accepts pretty much anything.


fwiw sniffing through logs on my end I can also see [a-zA-Z0-9]+ for varying lengths. Well covered under the proposed opaque IDs - just a point of information.

richvdh · 2018-08-31T10:36:26Z

I'm also not sure how much editing you'd like to do, however there's some inconsistency between using "CS API" and "C-S API".

given it's only a proposal I'm not too fussed about this sort of thing, but thanks. If I end up editing it anyway I'll try and clean it up.

turt2live · 2018-09-10T15:46:37Z

@mscbot fcp merge

anoadragon453 · 2018-09-10T15:55:33Z

CC @matrix-org/spec-core-team

turt2live · 2018-09-10T15:59:19Z

@mscbot fcp merge

mscbot · 2018-09-10T16:01:20Z

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

KitsuneRal

I'm overall good with the proposal, just one-non blocking comment below.

KitsuneRal · 2018-09-15T07:02:01Z

proposals/1597-id-grammar.md

+> The total length (including sigil and domain) must not exceed 255 characters.
+>
+> This is only enforced for v2 rooms - servers and clients wishing to support
+> v1 rooms should be more tolerant.


"be more tolerant" probably means "accept anything possible"? Probably it's clearer to state that "Servers and clients wishing to support v1 rooms should not make assumptions on the character set and length of IDs." or just drop the part after the dash, implying that v1 is Wild West.

erikjohnston · 2018-12-18T01:38:23Z

@mscbot reviewed

(In an attempt to unwedge mscbot)

anoadragon453 · 2018-12-18T01:53:15Z

@mscbot reviewed

mscbot · 2018-12-18T01:53:47Z

🔔 This is now entering its final comment period, as per the review above. 🔔

mscbot · 2018-12-23T09:21:02Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

jplatte · 2020-06-09T10:21:14Z

This was supposed to resolve #1597, which has comments disabled. That should probably be unlocked or closed? (along with some of the issues it references, possibly)

turt2live · 2020-06-09T13:50:04Z

this, and therefore 1597, are from an older era of MSC systems. The idea was that this PR (labelled proposal-pr) is the discussion point and the issue is to hold the number. Discussion should not happen on the issue.

jplatte · 2020-06-09T14:21:44Z

So what happens to that issue now? Is it considered fixed because this was merged and can be closed?

turt2live · 2020-06-09T14:31:01Z

the old system calls for the issue to remain open until the MSC is merged. The MSC hasn't been merged, it's just landed. "Merged" is once the spec PR has landed and thus ready for the next release.

richvdh · 2020-08-19T14:12:14Z

I'm going to back out the merge of this PR, since it seems to be causing confusion.

…mmar" This reverts commit d9135ef, reversing changes made to f714aaa. People are getting confused by the fact this has been merged.

jplatte · 2020-08-19T14:20:42Z

So the MSC process has changed since this was originally merged, right? If this MSC was newer, it wouldn't have been merged without the necessary spec (and Synapse?) changes being implemented at all?

richvdh · 2020-08-19T14:25:28Z

correct.

jplatte · 2020-08-19T14:28:56Z

Okay, that helps put things into perspective. Thanks. Sorry if I'm being annoying, trying to juggle all of (what the spec says, what other parts of the ecosystem are doing, what makes sense to me) can be really frustrating.

Signed-off-by: Kévin Commaille <[email protected]>

WIP proposal for better grammars

9e5b5d8

richvdh mentioned this pull request Aug 29, 2018

MSC1597: Better spec for matrix identifiers #1597

Open

richvdh added the proposal-pr label Aug 29, 2018

turt2live reviewed Aug 29, 2018

View reviewed changes

proposals/1597-id-grammar.md Outdated Show resolved Hide resolved

richvdh mentioned this pull request Aug 29, 2018

Better spec for room aliases #1608

Closed

descope aliases

646ca25

richvdh requested a review from a team August 29, 2018 22:38

ara4n reviewed Aug 29, 2018

View reviewed changes

proposals/1597-id-grammar.md Outdated Show resolved Hide resolved

ara4n reviewed Aug 29, 2018

View reviewed changes

proposals/1597-id-grammar.md Outdated Show resolved Hide resolved

ara4n reviewed Aug 29, 2018

View reviewed changes

richvdh added 4 commits August 30, 2018 00:11

address review feedback

b19bc73

synapse uses a different format of media IDs for URL previews

da394da

add a note on what matrix-media-repo does

251bd90

ISes use keys too

5c059a5

turt2live reviewed Aug 30, 2018

View reviewed changes

proposals/1597-id-grammar.md Outdated Show resolved Hide resolved

turt2live approved these changes Aug 30, 2018

View reviewed changes

anoadragon453 added the T-Core label Sep 10, 2018

KitsuneRal approved these changes Sep 15, 2018

View reviewed changes

mscbot added the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Dec 17, 2018

mscbot added the disposition-merge label Dec 17, 2018

mscbot added the final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. label Dec 18, 2018

anoadragon453 merged commit d9135ef into master Dec 23, 2018

richvdh removed the finished-final-comment-period label Dec 24, 2018

KitsuneRal mentioned this pull request Jul 25, 2020

MSC2312: Matrix URI scheme proposal #2312

Merged

richvdh added a commit that referenced this pull request Aug 19, 2020

Revert "Merge pull request #1598 from matrix-org/rav/proposals/id_gra…

c00710d

…mmar" This reverts commit d9135ef, reversing changes made to f714aaa. People are getting confused by the fact this has been merged.

richvdh mentioned this pull request Aug 19, 2020

Synapse allows empty localpart in a room ID over federation matrix-org/synapse#8126

Closed

uhoreg mentioned this pull request Mar 1, 2022

Acceptable characters for MXC opaque identifiers should be more clearly described matrix-org/matrix-spec#503

Open

RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023

Fix rendered changelog with new version of towncrier (matrix-org#1598)

639caf0

Signed-off-by: Kévin Commaille <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC 1597: Better spec for matrix identifiers #1598

MSC 1597: Better spec for matrix identifiers #1598

richvdh commented Aug 29, 2018 •

edited

Loading

ara4n Aug 29, 2018

richvdh Aug 29, 2018

turt2live left a comment

turt2live Aug 30, 2018

turt2live Aug 30, 2018

richvdh commented Aug 31, 2018

turt2live commented Sep 10, 2018

anoadragon453 commented Sep 10, 2018

turt2live commented Sep 10, 2018

mscbot commented Sep 10, 2018 •

edited by turt2live

Loading

KitsuneRal left a comment

KitsuneRal Sep 15, 2018

erikjohnston commented Dec 18, 2018

anoadragon453 commented Dec 18, 2018

mscbot commented Dec 18, 2018

mscbot commented Dec 23, 2018

jplatte commented Jun 9, 2020 •

edited

Loading

turt2live commented Jun 9, 2020

jplatte commented Jun 9, 2020

turt2live commented Jun 9, 2020

richvdh commented Aug 19, 2020

jplatte commented Aug 19, 2020 •

edited

Loading

richvdh commented Aug 19, 2020

jplatte commented Aug 19, 2020 •

edited

Loading


		Generated by sending server. Needs to be unique for a given pair of servers.

		[Synapse](https://github.com/matrix-org/synapse/blob/74854a97191191b08101821753c2672efc2a65fd/synapse/federation/transaction_queue.py#L593) uses a stringified int and accepts pretty much anything.

MSC 1597: Better spec for matrix identifiers #1598

MSC 1597: Better spec for matrix identifiers #1598

Conversation

richvdh commented Aug 29, 2018 • edited Loading

ara4n Aug 29, 2018

Choose a reason for hiding this comment

richvdh Aug 29, 2018

Choose a reason for hiding this comment

turt2live left a comment

Choose a reason for hiding this comment

turt2live Aug 30, 2018

Choose a reason for hiding this comment

turt2live Aug 30, 2018

Choose a reason for hiding this comment

richvdh commented Aug 31, 2018

turt2live commented Sep 10, 2018

anoadragon453 commented Sep 10, 2018

turt2live commented Sep 10, 2018

mscbot commented Sep 10, 2018 • edited by turt2live Loading

KitsuneRal left a comment

Choose a reason for hiding this comment

KitsuneRal Sep 15, 2018

Choose a reason for hiding this comment

erikjohnston commented Dec 18, 2018

anoadragon453 commented Dec 18, 2018

mscbot commented Dec 18, 2018

mscbot commented Dec 23, 2018

jplatte commented Jun 9, 2020 • edited Loading

turt2live commented Jun 9, 2020

jplatte commented Jun 9, 2020

turt2live commented Jun 9, 2020

richvdh commented Aug 19, 2020

jplatte commented Aug 19, 2020 • edited Loading

richvdh commented Aug 19, 2020

jplatte commented Aug 19, 2020 • edited Loading

richvdh commented Aug 29, 2018 •

edited

Loading

mscbot commented Sep 10, 2018 •

edited by turt2live

Loading

jplatte commented Jun 9, 2020 •

edited

Loading

jplatte commented Aug 19, 2020 •

edited

Loading

jplatte commented Aug 19, 2020 •

edited

Loading