Escape user IDs correctly #112
Conversation
lib/models/users/matrix.js
Outdated
| @@ -58,7 +59,7 @@ MatrixUser.prototype.set = function(key, val) { | |||
| this._data[key] = val; | |||
| }; | |||
|
|
|||
| /** | |||
| /**u | |||
| const badChars = new Set(this.localpart.replace(/([A-z0-9]|-|\.|=|_)+/g, "")); | ||
| // NOTE: Currently Matrix accepts / in the userId, although going forward it will be removed. | ||
| // NOTE: We also allow uppercase for the time being. | ||
| const badChars = new Set(this.localpart.replace(/([A-Z]|[a-z]|[0-9]|-|\.|=|_)+/g, "")); |
There was a problem hiding this comment.
we should remove characters which aren't allowed, such as uppercase. I'm pretty sure Synapse has started failing to register uppercase usernames by now.
lib/models/users/matrix.js
Outdated
| const code = c.charCodeAt(0); | ||
| const hex = code.toString(16).toLowerCase(); | ||
| if (code < 65 || code > 90) { | ||
| // Alpha codes do not need escaping. |
There was a problem hiding this comment.
... but should be lowercased.
There was a problem hiding this comment.
are we lowercasing them rather than escaping them?
There was a problem hiding this comment.
I don't see why not. User IDs are semi-opaque anyways, so Bob and bob are just going to have to get along.
There was a problem hiding this comment.
I can see that causing lots of unexpected conflicts :c
There was a problem hiding this comment.
tbh I see anything we do here as creating unexpected conflicts. See also: IRC bridge creating duplicate users, resulting in the bridge doing it's own escaping.
Maybe we just keep uppercase as allowed (sorry) and never visit this code again.
spec/unit/matrix-user.spec.js
Outdated
| @@ -6,8 +6,8 @@ describe("MatrixUser", function() { | |||
| [ | |||
| new MatrixUser("@test:localhost", null, false), | |||
| new MatrixUser("@42:localhost", null, false), | |||
| new MatrixUser("@Test42:localhost", null, false), | |||
| new MatrixUser("@A=Good-set.of_chars:localhost", null, false) | |||
There was a problem hiding this comment.
which means keeping this test to result in a lowercase version
Half-Shot
force-pushed
the
hs/escaping-fixes
branch
from
12f0e0b
to
cab087b
Compare
This fixes #109. In addition, this PR adds a global variable to disable userid escaping for bridges that don't support it yet.