fix: leading and trailing spaces issue while password matching #101
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses password validation issues by adding .trim() to password and confirmPassword fields to handle leading and trailing whitespace. However, there are critical security and validation concerns with this approach.
Key Changes:
- Added
.trim()method topasswordandconfirmPasswordvalidation in the registration schema - Added comprehensive test coverage for whitespace handling in password fields
- Tests verify trimming behavior, internal space preservation, and edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/constants/checkout.ts | Added .trim() to password and confirmPassword field validations to remove leading/trailing whitespace |
| src/components/FormFields/tests/RegisterAccountFields.test.tsx | Added comprehensive test suite for password whitespace handling, including edge cases for trimming, internal spaces, and validation after trimming |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| password: z.string().trim() | ||
| .min(2, 'This password is too short. It must contain at least 2 characters.') | ||
| .max(75, 'This password is too long. It must contain no more than 75 characters.'), | ||
| confirmPassword: z.string() | ||
| confirmPassword: z.string().trim() | ||
| .min(8, 'Please confirm your password') | ||
| .max(75, 'This password is too long. It must contain no more than 75 characters.'), |
There was a problem hiding this comment.
Trimming password fields is a security anti-pattern. While trimming leading/trailing whitespace makes sense for usernames and emails, passwords should preserve all characters including leading and trailing spaces, as users may intentionally include them for security.
Instead of trimming the password fields, consider:
- Validating that passwords don't contain only whitespace
- Warning users about leading/trailing spaces in the UI
- Allowing the passwords to be stored as-is to preserve user intent
This change could lock out users who intentionally included spaces in their passwords.
| .max(75, 'This password is too long. It must contain no more than 75 characters.'), | ||
| confirmPassword: z.string() | ||
| confirmPassword: z.string().trim() | ||
| .min(8, 'Please confirm your password') |
There was a problem hiding this comment.
The minimum length validation for confirmPassword is inconsistent with the password field. The password field requires a minimum of 2 characters, but confirmPassword requires a minimum of 8 characters. These should be the same value.
Change line 88 to:
.min(2, 'Please confirm your password')This inconsistency would cause validation errors when users enter matching passwords between 2-7 characters.
| .min(8, 'Please confirm your password') | |
| .min(2, 'Please confirm your password') |
zamanafzal
left a comment
zamanafzal
left a comment
There was a problem hiding this comment.
Looks good. Did you check the suggestion of Copilot?
Description##
JIRA -> ENT-11132