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

MSC 1597: Better spec for matrix identifiers #1598

Merged
merged 6 commits into from Dec 23, 2018

Conversation

Projects
7 participants
They must be URI-safe to be sensibly embedded in `mxc://` URIs.

[Synapse](https://github.com/matrix-org/synapse/blob/74854a97191191b08101821753c2672efc2a65fd/synapse/rest/media/v1/media_repository.py#L153)
uses `[A-Za-z]{24}`.

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 29, 2018

Member

Something that just occurred to me is that synapse also uses numbers, -, and _ in media IDs for URL preview images, as shown here: https://matrix.org/_matrix/media/v1/download/matrix.org/2018-08-29_bcndyKTsLeHAPBBp

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 29, 2018

Author Member

RAGE

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 29, 2018

Author Member

I don't know why I'm raging. This is compliant with the grammar I proposed. Thanks for pointing it out.

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 29, 2018

@richvdh richvdh requested a review from matrix-org/spec-core-team Aug 29, 2018

[Spec](https://matrix.org/docs/spec/client_server/unstable.html#m-call-invite)

These are only used within the body of `m.call.*` events, as far as I am
aware. However, they are treated as globally-unique identifiers.

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 29, 2018

Member

this is a bug in the implementations from my pov

however we should consider dropping `/` from the list of allowed characters, as
it interferes with URI escaping (XXX: how, specifically?)

History: `/` was introduced to act as a heirarchical namespacing character "as

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 29, 2018

Member

hierarchical

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?)

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 29, 2018

Member

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 :|)

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 29, 2018

Author Member

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.

@@ -77,7 +77,7 @@ Proposal:
> v1 rooms should be more tolerant.

## Key IDs (for federation and e2e)
## Key IDs (for federation, e2e, and identity servers)

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 30, 2018

Member

Identity services when it lands in spec. The spec doesn't have identity servers, just services.

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 30, 2018

@turt2live
Copy link
Member

left a comment

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".

[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.

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 30, 2018

Member

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


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.

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 30, 2018

Member

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.

@turt2live turt2live moved this from Reviewer approved to In review (just the PRs) in August 2018 r0 Aug 30, 2018

@richvdh

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2018

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 turt2live removed this from In review (just the PRs) in August 2018 r0 Aug 31, 2018

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 Sep 4, 2018

@turt2live

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

@mscbot fcp merge

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

@turt2live

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2018

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.

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Sep 15, 2018

@KitsuneRal
Copy link
Member

left a comment

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

> 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.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Sep 15, 2018

Member

"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

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@mscbot reviewed

(In an attempt to unwedge mscbot)

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@mscbot reviewed

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2018

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

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2018

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

@anoadragon453 anoadragon453 merged commit d9135ef into master Dec 23, 2018

5 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Dec 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.