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

AccountSettings: escape forward- and backslashes in MXIDs #846

Merged
merged 5 commits into from
Dec 27, 2024

Conversation

KitsuneRal
Copy link
Member

Fixes #842.

@KitsuneRal
Copy link
Member Author

The test is screwed (and should really be a unit test, not a part of quotest) but the functionality seems to be working, as just tried in Quaternion.

@KitsuneRal KitsuneRal force-pushed the kitsune/fix-forward-slashes-in-mxids branch from 7a220b8 to f82ebbf Compare December 22, 2024 12:33
@TobiasFella
Copy link
Member

TobiasFella commented Dec 22, 2024

This makes the account load fine, but it's not yet perfect: my test account with mxid @tobias/:matrix.org is shown as @tobias%2F:matrix.org.

(some other things are also broken, but I'd look at those in different issues / patches) actually those might just be symptoms of that problem, let's see

@KitsuneRal KitsuneRal force-pushed the kitsune/fix-forward-slashes-in-mxids branch from f82ebbf to a60dfa8 Compare December 22, 2024 20:35
@KitsuneRal
Copy link
Member Author

We could add unescaping in some other places; I'm just trying to stay conservative. Technically, it's possible to recover MXIDs even at the Settings level, by parsing the key and only (un)escaping only the piece that looks like an MXID - e.g., by using the fact that nobody's using : in key names so far, outside of the account names.

@KitsuneRal
Copy link
Member Author

Looks like the Windows runner doesn't allow me to store settings in the registry?..

@KitsuneRal
Copy link
Member Author

This makes the account load fine, but it's not yet perfect: my test account with mxid @tobias/:matrix.org is shown as @tobias%2F:matrix.org.

(some other things are also broken, but I'd look at those in different issues / patches) actually those might just be symptoms of that problem, let's see

Okay, I found why this happens, AccountSettings::userId() has to unescape the returned value too. Fix coming.

It should unescape the user id back again after escaping it in the
constructor.
@KitsuneRal KitsuneRal force-pushed the kitsune/fix-forward-slashes-in-mxids branch from 8260ad6 to 03427c2 Compare December 26, 2024 11:27
@KitsuneRal
Copy link
Member Author

No idea what is happening on Windows CI: it successfully create a settings group for the account but fails to save anything inside the group. Given that it's failing even with "normal" MXIDs, I tend to blame the GitHub runner. Feel free to test it out, if it's fine, I'm merging it.

TobiasFella
TobiasFella previously approved these changes Dec 26, 2024
Test both "normal" MXIDs and MXIDs with slashes; actually test reading
into AccountSettings from the settings storage.
Otherwise QSettings fails to find/allocate its place in the Windows
registry.
@KitsuneRal
Copy link
Member Author

I figured the problem on Windows in the end - QSettings really wants the organisation name to be explicitly set there, the default empty organisation name doesn't turn into Unknown organization as it does on Linux and an empty name for the the registry key doesn't work, of course.

@KitsuneRal KitsuneRal merged commit f10b1fd into dev Dec 27, 2024
7 checks passed
@KitsuneRal KitsuneRal deleted the kitsune/fix-forward-slashes-in-mxids branch December 27, 2024 17:18
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.

Accounts with user ids containing forward slashes can't be loaded correctly
2 participants