Repository navigation
fix(auth): accept the same passwords on reset, sign-up and Profile - #1892
dawsontoth wants to merge 2 commits into
Conversation
Reset Password refused anything over 50 characters, while sign-up and Profile set no limit at all. None of those matched central manager: its only password cap is `VALIDATION_CONSTRAINTS.USER.PASSWORD_MAX_LENGTH` (256), applied by `validateAddUser` to sign-up, while `validateResetPassword` and `validateUpdateUser` take any length. So a password manager's 64-character password worked for sign-up and Profile but could not be reset to, and a sign-up password over 256 passed the form only to be refused by the server. All three forms now cap the password at one shared `ACCOUNT_PASSWORD_MAX_LENGTH` (256) with the same message. Password inputs still have no `maxLength`, so an over-long paste is shown in full with the error rather than silently clamped. Closes #1883 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request standardizes the maximum password length across the application to 256 characters by introducing central constants in password validation and applying them to the sign-up, password reset, and profile update schemas, along with updating and adding corresponding tests. Feedback on the changes suggests replacing the Zod union in the user update schema with a refine check to avoid confusing, multiple validation errors when validating empty or invalid passwords.
| .min(8, { error: 'Password must be 8 characters or more.' }) | ||
| .max(ACCOUNT_PASSWORD_MAX_LENGTH, { error: ACCOUNT_PASSWORD_TOO_LONG_ERROR }) | ||
| .or(z.string().max(0)), |
There was a problem hiding this comment.
Using .or() to allow an empty string creates a ZodUnion. When validation fails (e.g., if the password is too long or too short), Zod evaluates both branches and returns errors for both, which can lead to multiple or confusing error messages in the UI.
We can avoid the union entirely by using a single ZodString with a .refine() check. This ensures only a single, predictable error message is generated. Note that in Zod v4, .refine() accepts error as the unified message parameter.
.max(ACCOUNT_PASSWORD_MAX_LENGTH, { error: ACCOUNT_PASSWORD_TOO_LONG_ERROR })
.refine((val) => val.length === 0 || val.length >= 8, {
error: 'Password must be 8 characters or more.',
}),References
- In Zod v4, '.refine()' accepts 'error' as the unified message parameter.
There was a problem hiding this comment.
Declining: the form only ever shows one message here, and it is the right one. Replaying the schema through zodResolver(UpdateUserSchema) (resolvers 5.9.1, zod 4.6.5): a 257-character password yields message: "Password cannot be longer than 256 characters." and "short" yields "Password must be 8 characters or more."; the empty and valid cases yield no error. Each failing branch has exactly one issue, and the resolver surfaces the closest branch with ties going to the first (@hookform/resolvers/zod/src/zod.ts:117-131), so the .max(0) branch's text never wins. The second branch's text appears only in types under criteriaMode: 'all', which no form sets, and FormMessage renders error.message alone (src/components/ui/form/FormMessage.tsx:7). ProfileIndex.test.tsx pins both messages through the real form. The .or(z.string().max(0)) union also predates this PR, which only adds the .max.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…' into claude/1883-reset-password-max-length
⊙ Problem
Stacked on #1888 (
claude/1290-maxlength-attributes). Review and merge that first, then this PR's base moves tostage. #1888 adds the tests that keepmaxLengthoff every password input, and this PR keeps them passing.Closes #1883.
Reset Password refuses any new password over 50 characters ("Password cannot be longer than 50 characters."). Sign-up and Profile have no limit at all. Neither behaviour matches central manager, so the three forms disagree with each other and with the server:
validateAddUserappliesVALIDATION_CONSTRAINTS.USER.PASSWORD_MAX_LENGTH(src/validation/validation.js:12,:238)validateResetPasswordhasnewPassword: string.required()(:475)validateUpdateUserhaspassword: string.optional()(:255)(central-manager line numbers are from
38449f3f.)So someone whose password manager generates a 64-character password can sign up with it and change to it from Profile, but can't reset to one like it. Separately, a sign-up password over 256 characters passes the form and is then refused by the server.
The 50 doesn't come from the server.
fa320a150("Sync validation with CM and core") removed sign-up's 50-character limit, but Reset Password, which had copied that limit a few weeks earlier (b8b47d7d7), wasn't updated.💡 Solution
There is now one shared constant,
ACCOUNT_PASSWORD_MAX_LENGTH = 256, with one message,ACCOUNT_PASSWORD_TOO_LONG_ERROR, insrc/lib/zod/password.ts. Sign-up, Reset Password and Profile's new-password field all use it. 256 is central manager's only password limit, so no form now refuses a password the server would accept on sign-up, and no form lets through one that sign-up would refuse.The issue asked whether the stored hash has a shorter effective limit than this (bcrypt, for example, ignores everything past 72 bytes). It doesn't. Central manager stores account passwords as Harper users:
resetUserPasswordcallsalter_user(src/resources/user/userHelpers.js:93), and sign-in goes throughserver.authenticateUser(src/resources/Login.js:86). Harper hashes with SHA-256 by default, or argon2id or MD5 if configured (harper/utility/password.ts:28-55at16e5c663c). All three hash the whole password, so a 256-character password counts in full. Harper'suser_validation.tshas no length limit either.Password inputs still have no
maxLengthattribute, as #1888 decided: if a pasted password were clamped, the password manager would save a password that never worked. An over-long password is shown in full, with the error under it.⚖️ Alternatives
🔧 Changes
src/lib/zod/password.ts: addsACCOUNT_PASSWORD_MAX_LENGTH(256) and its message.src/features/auth/ResetPassword.tsx: the 50-character.maxis replaced with the shared one.src/features/auth/SignUp.tsx: the password gets the shared.max. It had no limit before.src/features/profile/mutations/updateUserSchema.ts: the non-empty branch ofnewPasswordgets the shared.max. The empty branch, which means "keep my current password", is unchanged.Instance-user password forms (instance Add User / Alter User, cluster Finish Setup) are not changed. Those passwords belong to Harper users on the cluster, not Fabric accounts, and Harper doesn't limit their length.
✅ Verification
ResetPassword.test.tsx): a 257-character password is shown in full with "Password cannot be longer than 256 characters." and is not submitted. 51- and 256-character passwords are submitted unchanged. The password inputs still have nomaxLength.SignUp.test.tsx): 257 characters is refused before any request is sent, and 256 is posted.ProfileIndex.test.tsx): 257 characters shows the error and disables Save, and 256 plus a matching confirmation enables Save..max(50)restored, the 51, 256 and 257 Reset Password tests all fail..maxremoved from each of the three schemas, each form's 257-character test fails on its own.npx tsc -bexit 0;npx oxlint --format stylish .exit 0;npx dprint checkexit 0. The pre-commit hook's full suite: 399 files, 3,681 tests passed.waitFor, the same waySignUp.test.tsxdoes, instead of asserting straight afteract. I rejected the other four, with evidence:zodRequirePassworditself. That helper also backs sign-in and the instance-user forms. Instance users are Harper users with no limit, and capping sign-in would lock out a password longer than 256 that was set through central manager's API..or()union hides the message. It doesn't. The Profile test checks the exact "cannot be longer than 256" text and passes.confirmPasswordcould exhaust memory on the server. These schemas only run in the browser, andconfirmPasswordis never sent.no-receipt) because the 1Password SSH agent refused to sign. The domain adjudication leg failedauth(expired Claude OAuth session), so the outside findings above were triaged by me, not adjudicated.672f5027merges fix(forms): stop typing at each field's length limit instead of refusing it on submit #1888's branch, which now carriesstage(Edit roles in a visual editor, with JSON as the fallback #1893's role editor). No file conflicted here. Afterwards, focused vitest (profile, auth, role modals; 23 files / 222 tests) andtsc -bboth exited 0..or(z.string().max(0))union with a.refine(), because a union "returns errors for both" branches. I declined it with evidence. I ranzodResolver(UpdateUserSchema)on real inputs: 257 characters gives only "Password cannot be longer than 256 characters.",shortgives only "Password must be 8 characters or more.", and empty or valid input gives no error. Each failing branch has one issue, and the resolver shows the closest branch with ties going to the first (@hookform/resolvers/zod/src/zod.ts:117-131).FormMessagerenderserror.messagealone. The union also predates this PR.🤖 Generated with Claude Code
Related PRs: #1888 overlaps (this PR is stacked on it and extends its ResetPassword test), 2 others independent
Complexity: easy
Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-grok(no-receipt),cursor-composer(no-receipt),domain(auth); declined=cursor-kimi,cursor-muse; rounds=1; full=1 @ 672f502
Review-Attention: study ~20m (critical: updateUserSchema.ts; sensitive: ResetPassword.tsx, SignUp.tsx; raised: degraded review) @ 672f502