Fix:- update the regex for password validation #3070
Conversation
Darshan98Solanki
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Password validation regex update server/src/validation/joi.js |
Modified passwordPattern regex to add negative lookahead disallowing <, >, backtick, and " characters; replaced [0-9] with \d for digit validation |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~3 minutes
- Single regex modification in one file addressing a specific validation bug
- No logic flow changes or structural alterations
- Verify the negative lookahead syntax is correct and the excluded characters match the bug report
Poem
A rabbit once found validation's plight,
When angle brackets caused a blight,
With lookahead magic, now set straight,
No sneaky<to seal our fate! 🐰✨
Pre-merge checks and finishing touches
❌ Failed checks (2 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | Title is vague and uses unclear phrasing ('Fiz:-' appears to be a typo or non-standard convention) that doesn't clearly convey the specific change. | Revise title to be more specific and professional, e.g., 'Update password validation regex to allow < character' or 'Fix password validation regex to handle special characters'. |
| Description check | ❓ Inconclusive | Description includes the template placeholder text and most checklist items are unchecked, making it incomplete and unclear which items were actually verified. | Remove template instruction lines, complete all checklist items with proper checks, and clarify testing and deployment status before requesting review. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | ✅ Passed | The regex changes directly address issue #3010 by modifying password validation to disallow '<', '>', '`', and '"' characters, fixing the password validation logic that was causing the false 'empty password' error. |
| Out of Scope Changes check | ✅ Passed | The change is narrowly focused on updating the password validation regex in a single file, which is directly related to resolving the identified bug with no extraneous modifications. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example instruction:
"Divide the high-level summary into five sections:
- 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
- 📓 References — List relevant issues, discussions, documentation, or related PRs.
- 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
- 📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- ✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Darshan98Solanki
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/validation/joi.js (1)
53-53: Add custom error message for password pattern validation.The updated
passwordPatternis used in password validation schemas at lines 53, 64–65, 82, and 708, but none have custom error messages. Without them, users see generic pattern-mismatch errors. Add.messages({"string.pattern.base": "Password must contain at least one lowercase letter, one uppercase letter, one digit, and one special character (!@#$%^&*_-+=[]{};:',~,./|), and cannot contain <, >, backtick, or double quotes"})to each password field, following the pattern already used innameValidation(lines 27–38).Affected locations: lines 53, 64, 65, 82, 708.
Note: Existing users with disallowed characters can still log in (password verification uses bcrypt comparison); they are restricted only when changing passwords.
| const passwordPattern = | ||
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[!?@#$%^&*()\-_=+[\]{};:'",.<>~`|\\/])[A-Za-z0-9!?@#$%^&*()\-_=+[\]{};:'",.<>~`|\\/]+$/; | ||
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*_\-+=\\[\]{};:'~,./|])(?!.*[<>`"])[A-Za-z\d!@#$%^&*_\-+=\\[\]{};:'~,./|]+$/; |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: This fix contradicts the stated objective from issue #3010.
The linked issue #3010 requests: "Ensure successful user creation when the first character of the password is '<' (or other valid symbols)". However, this implementation explicitly disallows passwords containing <, >, backtick, or " anywhere in the string via the negative lookahead (?!.*[<>"])`.
This changes the requirement rather than fixing the bug. The original error "password cannot be empty" was misleading, but the solution should either:
- Allow these characters and fix the root cause of why
<was misinterpreted, OR - Update issue 'Password cannot be empty' user signup bug on fresh install #3010 to reflect that these characters are intentionally disallowed for security reasons
Additional concerns:
-
Redundant negative lookahead: The characters disallowed by
(?!.*[<>"])are already absent from the allowed character class[A-Za-z\d!@#$%^&*_-+=\[]{};:'~,./|]`, so they would be rejected anyway. The negative lookahead adds no functional value. -
Potential security issue: Disallowing HTML-sensitive characters (
<,>,", backtick) in passwords suggests they might be improperly handled elsewhere (e.g., displayed without escaping or used in HTML contexts without sanitization). Passwords should always be hashed and never rendered in HTML, making these restrictions unnecessary. -
Poor user experience: The generic Joi pattern validation error won't clearly indicate which characters are problematic. Users won't understand why their password is rejected.
Please clarify the actual requirement and verify password handling practices:
🏁 Script executed:
#!/bin/bash
# Search for potential password mishandling - displaying, logging, or using passwords in HTML contexts
echo "=== Searching for potential password display or logging ==="
rg -n -C3 "password" --type=js -g '!node_modules' -g '!*.test.js' -g '!*.spec.js' | rg -i "(console\.|log|display|render|html|innerHTML)"
echo -e "\n=== Checking if passwords are being compared in plaintext ==="
ast-grep --pattern $'if ($PASSWORD === $_) { $$$ }'
echo -e "\n=== Checking password hashing implementation ==="
rg -n -C3 "bcrypt|argon2|scrypt|hash.*password" --type=js -g '!node_modules'Length of output: 103973
Critical: This implementation contradicts issue #3010's stated objective.
Issue #3010 requests allowing passwords that start with < (or other valid symbols). However, this implementation explicitly disallows passwords containing <, >, backtick, or " via the negative lookahead (?!.*[<>"])` at line 21.
This reverses the requirement rather than fixing the underlying issue.
Redundant negative lookahead:
The characters blocked by (?!.*[<>"])are already absent from the allowed character class[A-Za-z\d!@#$%^&*_-+=\[]{};:'~,./|]`, making the negative lookahead functionally unnecessary.
Required action:
Clarify the intent: either remove the negative lookahead and allow these characters (fixing the actual bug), or update issue #3010 to reflect that certain characters are intentionally restricted and document the business reason.
🤖 Prompt for AI Agents
In server/src/validation/joi.js around lines 20-21 the regex contains a
redundant negative lookahead (?!.*[<>`"]) that both contradicts issue #3010 and
is unnecessary because those characters are already excluded by the
allowed-character class; to fix, remove the negative lookahead and update the
allowed-character class to permit the intended characters (e.g., include <, >,
backtick, and double-quote if they should be allowed) so passwords starting with
those symbols pass validation, then add/adjust unit tests and a short comment
documenting the intended character policy; if the intent was to keep them banned
instead, update issue #3010 with the business justification and document the
restriction in code comments.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR addresses a security vulnerability by updating password validation to disallow characters that conflict with input sanitization, but introduces a breaking change that could invalidate existing user passwords.
🌟 Strengths
- Fixes a critical issue where passwords starting with '<' were incorrectly sanitized, causing validation errors.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | server/src/validation/joi.js | Security | Breaking change disallowing <, >, `, " characters | |
| P2 | server/src/validation/joi.js | Architecture | Addresses symptom not root cause of sanitization | path:server/src/v1/middleware/sanitization.js |
| P2 | server/src/validation/joi.js | Maintainability | Increased regex complexity reduces maintainability | |
| P2 | server/src/validation/joi.js | Testing | Lacks comprehensive test coverage for changes |
🔍 Notable Themes
- The password validation change addresses a sanitization conflict but may not be the optimal long-term solution.
- Complex regex patterns hinder code maintainability and testability.
📈 Risk Diagram
This diagram illustrates the new password validation logic and its associated breaking change risk.
sequenceDiagram
participant U as User
participant S as Server Validation
U->>S: Submit password
S->>S: Check for disallowed characters [<>`"]
note over S: R1(P1): Breaking change invalidates existing passwords
alt Contains disallowed characters
S-->>U: Reject password
else
S-->>U: Accept password
end
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
|
|
||
| const passwordPattern = | ||
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[!?@#$%^&*()\-_=+[\]{};:'",.<>~`|\\/])[A-Za-z0-9!?@#$%^&*()\-_=+[\]{};:'",.<>~`|\\/]+$/; | ||
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*_\-+=\\[\]{};:'~,./|])(?!.*[<>`"])[A-Za-z\d!@#$%^&*_\-+=\\[\]{};:'~,./|]+$/; |
There was a problem hiding this comment.
P1 | Confidence: High
- Security: The password validation regex has been updated to explicitly disallow characters
<,>,`, and"using a negative lookahead. This addresses the security vulnerability where these characters were being sanitized by the server's input sanitization middleware, causing passwords starting with<to become empty. However, this change represents a breaking modification to the password policy by removing previously allowed special characters. - Architecture: This change addresses a symptom (password validation) rather than the root cause (overly aggressive input sanitization). A more architectural approach would be to modify the sanitization middleware to handle password fields differently, such as excluding them from sanitization or using context-aware strategies.
- Maintainability: The regex complexity has increased significantly with the addition of negative lookahead and character class changes. Complex regex patterns are difficult to maintain, test, and understand, making it harder to modify individual requirements.
- Testing: Given the significant changes to allowed special characters and the addition of character blacklisting, comprehensive test coverage is crucial. Ensure tests cover edge cases, previously allowed characters that are now disallowed, and all positive validation requirements.
| /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[!@#$%^&*_\-+=\\[\]{};:'~,./|])(?!.*[<>`"])[A-Za-z\d!@#$%^&*_\-+=\\[\]{};:'~,./|]+$/; | |
| const hasLowerCase = /[a-z]/; | |
| const hasUpperCase = /[A-Z]/; | |
| const hasDigit = /\d/; | |
| const hasSpecialChar = /[!@#$%^&*_\-+=\[\]{};:'~,./|]/; | |
| const hasInvalidChars = /[<>`"]/; | |
| function validatePassword(password) { | |
| return hasLowerCase.test(password) && | |
| hasUpperCase.test(password) && | |
| hasDigit.test(password) && | |
| hasSpecialChar.test(password) && | |
| !hasInvalidChars.test(password); | |
| } |
Evidence: path:server/src/v1/middleware/sanitization.js
1 participant
(Please remove this line only before submitting your PR. Ensure that all relevant items are checked before submission.)
Describe your changes
Getting the error for password start with <>. The error we have got is about empty password. I have just add the regex part for the backend and will update the frontend in other pr as per @gorkemcetin.
Write your issue number after "Fixes "
Fixes #3010
just update the regex as per last conversation with @ajhollid and @gorkemcetin.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.