Fix/account settings - #242
Conversation
change_password and set_initial_password only checked length >= 8, so a password signup rejects (e.g. "abcdefgh") could be set from Settings. The policy moves to service/lib/password-policy.js and is shared by signup, change_password and set_initial_password. The new password is trimmed like signup and checked before the credential, so a weak one never burns a single-use email OTP. A weak password still answers uncompliant_password (older clients keep their message) and now lists the unmet rules in `missing`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t renames only the caller update_profile wrote profile.username into the JSON only. Login, the directory and avatars read the drumate.username column and the -u vhost, so a rename showed in Settings and nowhere else, with no uniqueness check. A changed username is now validated (2-80 of [A-Za-z0-9._-], starting alphanumeric), checked free in the caller's domain, and applied with drumate_change_username before the rest of the profile; failures reject the whole save with USERNAME_INVALID / USERNAME_TAKEN. update_ident renamed whatever `id` the client sent, letting any signed-in user rename any account. It now always renames the caller. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10c3d18d7a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (profile && profile.username !== undefined) { | ||
| const failed = await this._changeUsername(profile.username); | ||
| if (failed) return this.output.data(failed); |
There was a problem hiding this comment.
When update_profile is called with the documented secret and code parameters, check_otp_and_change() writes the profile and returns before this new branch runs. A request that includes profile.username therefore succeeds via drumate_update_profile without invoking _changeUsername, leaving the drumate.username column and vhost unchanged and bypassing the new validation/taken-name checks. Run the username path as part of the OTP-confirmed update before returning.
Useful? React with 👍 / 👎.
| if (profile && profile.username !== undefined) { | ||
| const failed = await this._changeUsername(profile.username); |
There was a problem hiding this comment.
For a valid new username supplied with surrounding whitespace, _changeUsername trims only its local argument and updates the column to the trimmed value, but do_update_profile() subsequently receives the original profile object and stores the whitespace-padded profile.username. This leaves profile reads showing a different username from login/directory/vhost behavior; normalize profile.username itself before both the column change and profile write.
Useful? React with 👍 / 👎.
| // drumate.change_password and drumate.set_initial_password. | ||
| // |
There was a problem hiding this comment.
The new helper is described as the password policy for every password-setting endpoint, but the anonymous otp.set_password route in acl/otp.json still reaches service/private/otp.js:set_password, which calls set_password after OTP verification without any password-policy check. A user completing that reset flow can therefore set a password such as a, bypassing the stricter requirements now enforced by signup and account settings. Apply missingPasswordRules before this write as well.
Useful? React with 👍 / 👎.
| // 2-80 chars: letters, digits, dot, dash, underscore; must start with a | ||
| // letter or digit. It ends up in a hostname label (<name>-u.<domain>). | ||
| if (!/^[A-Za-z0-9][A-Za-z0-9._-]{1,79}$/.test(username)) { |
There was a problem hiding this comment.
The validator accepts names up to 80 characters even though the following comment states that the procedure puts the value in a <name>-u hostname label. A 62-character username already produces a 64-character label after that suffix, exceeding the DNS label limit of 63; such a rename can leave the account vhost unresolvable or fail in the procedure and be reported misleadingly as USERNAME_TAKEN. Restrict the accepted length to account for the -u suffix.
Useful? React with 👍 / 👎.
1 participant
No description provided.