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

Fix not being able to DM users with non-ascii usernames #271

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tadzik
Copy link
Contributor

@tadzik tadzik commented Jul 30, 2021

Remote usernames with multibyte characters in them currently get encoded
into mxids in a way that may result in information loss
(see matrix-org/matrix-appservice-bridge#346).

This makes it impossible to reach some users by MXID alone, so this
makes it lookup the MXID in the user store, which contains the original,
unmangled username.

Fixes GH-268.

@tadzik tadzik requested a review from a team July 30, 2021 11:25
@tadzik tadzik self-assigned this Jul 30, 2021
@tadzik tadzik force-pushed the tadzik/gh-268/fix-non-ascii-dms branch 2 times, most recently from 0f81080 to 3450988 Compare July 30, 2021 11:26
@tadzik tadzik marked this pull request as ready for review July 30, 2021 11:27
Remote usernames with multibyte characters in them currently get encoded
into mxids in a way that may result in information loss
(see matrix-org/matrix-appservice-bridge#346).

This makes it impossible to reach some users by MXID alone, so this
makes it lookup the MXID in the user store, which contains the original,
unmangled username.

Fixes GH-268.
@tadzik tadzik force-pushed the tadzik/gh-268/fix-non-ascii-dms branch from 3450988 to d83aed1 Compare August 2, 2021 09:20
Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing test doesn't look trivial.
That should get fixed first.

if (remoteUsers.length > 1) {
log.error(
`Multiple remote users found for ${event.state_key!}:`,
remoteUsers.map(u => `${u.protocolId}://${u.username}`).join(', ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to look like an URI with ://?
The lines below just separate the protocol and username with a colon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah – it follows the convention we often use in the logs, like here.

The regular colon usage below is mostly arbitrary, I think. @Half-Shot?

@tadzik
Copy link
Contributor Author

tadzik commented Aug 6, 2021

The failing test doesn't look trivial.
That should get fixed first.

Which test?

@jaller94 jaller94 dismissed their stale review August 31, 2021 14:17

no failing tests

@Neustradamus
Copy link

@tadzik: Have you progressed on your PR about @jaller94 comment?

@Half-Shot: What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private messages in MUC don't get sent to the right resource if it contains certain characters
3 participants