fix: error resolve for signup user when password contains <> sign #3061
Conversation
Darshan98Solanki
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Password URL encoding on client client/src/Features/Auth/authSlice.js |
Added encodeURIComponent to password before sending in both register and login thunks |
Password URL decoding on server server/src/controllers/v1/authController.js |
Added decodeURIComponent to req.body.password in both registerUser and loginUser handlers prior to validation |
Sequence Diagram
sequenceDiagram
participant User
participant Client as Client (authSlice.js)
participant Network as HTTP Request
participant Server as Server (authController.js)
participant Validator
User->>Client: Enter password with special chars (e.g., "<BIBpIiDXbgdp6M0vr6SO8T7")
Client->>Client: encodeURIComponent(password)
Client->>Network: Send encoded password
Network->>Server: Receive encoded password
Server->>Server: decodeURIComponent(req.body.password)
Server->>Validator: Validate decoded password
Validator-->>Server: ✓ Valid
Server-->>Client: ✓ Success
Client-->>User: Account created
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~8 minutes
- Homogeneous, repetitive pattern applied consistently across two files
- No control flow changes or complex logic
- Critical consideration: Verify that encoding/decoding roundtrip preserves all special characters without data loss
- Confirm no other password handling paths bypass this encoding/decoding mechanism
Poem
A rabbit hops with glee, encoding as it goes,
The password wrapped in URL's protective throes,
Special chars like<now safely pass through space,
No more phantom empties—decoded with grace! 🐰✨
Pre-merge checks and finishing touches
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main fix: resolving signup errors when passwords contain reserved characters like '<>'. |
| Description check | ✅ Passed | The description includes issue reference, completed checklist items, clear explanation of the solution, and supporting screenshots for validation. |
| Linked Issues check | ✅ Passed | The PR code changes directly address issue #3010 by URL-encoding passwords client-side and decoding server-side, preventing '<' and other reserved characters from causing validation failures. |
| Out of Scope Changes check | ✅ Passed | Changes are limited to auth-related files (authSlice.js and authController.js) and directly target the password encoding issue without introducing unrelated 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
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.
![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 (3)
server/src/controllers/v1/authController.js (1)
179-192: Password changes in editUser do not support special characters.The
editUserfunction acceptspasswordandnewPasswordfields for password changes but does not decode them. Users who registered with special characters (like '<') will be unable to update their passwords to new values containing these characters, creating an inconsistent user experience.Ensure the client encodes both
passwordandnewPasswordfields in theupdatethunk (authSlice.js, lines 53-54), then decode them here:editUser = this.asyncHandler( async (req, res) => { + if (req.body.password) { + req.body.password = decodeURIComponent(req.body.password); + } + if (req.body.newPassword) { + req.body.newPassword = decodeURIComponent(req.body.newPassword); + } await editUserBodyValidation.validateAsync(req.body);client/src/Features/Auth/authSlice.js (2)
53-54: Encode passwords in the update flow.The
passwordandnewPasswordfields are added to FormData without encoding, meaning users cannot change their passwords to values containing special characters like '<'. This creates an inconsistent experience where registration supports these characters but password updates do not.Apply this diff:
- form.password && fd.append("password", form.password); - form.newPassword && fd.append("newPassword", form.newPassword); + form.password && fd.append("password", encodeURIComponent(form.password)); + form.newPassword && fd.append("newPassword", encodeURIComponent(form.newPassword));
120-122: Encode password in the recovery flow for consistency with other authentication flows.The
setNewPasswordthunk does not encode the password, while bothloginandregisterthunks useencodeURIComponent()on the password field. This inconsistency should be addressed.+ const encodedForm = { + ...form, + password: form.password ? encodeURIComponent(form.password) : form.password + }; const res = await networkService.setNewPassword({ recoveryToken: token, - form: form, + form: encodedForm, });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Features/Auth/authSlice.js(2 hunks)server/src/controllers/v1/authController.js(2 hunks)
🔇 Additional comments (3)
client/src/Features/Auth/authSlice.js (1)
15-16: LGTM! Password encoding implemented correctly.The password is properly encoded using
encodeURIComponentbefore transmission, and the original form object is preserved by creating a newreqObj. This approach correctly addresses the issue where special characters like '<' caused validation failures.Also applies to: 32-33
server/src/controllers/v1/authController.js (2)
461-477: The review comment is incorrect and should be dismissed.The password handling across endpoints is actually consistent and intentional:
- Public endpoints (register, login): Client encodes password → Server decodes it
- Authenticated endpoints (password change, admin reset, password recovery): No encoding on either side
This design is sound because authenticated endpoints transmit over HTTPS with JWT authentication, making URL encoding unnecessary. The encoding in public endpoints adds an extra layer for form submission.
The
editUserPasswordByIdendpoint correctly follows the authenticated endpoint pattern—it matchesresetPasswordand the user's own password change flow. There is no inconsistency to fix.Likely an incorrect or invalid review comment.
300-311: The original review comment is incorrect—special characters are supported in password recovery.The
resetPasswordendpoint will successfully handle special characters like '<' because the password flows through without encoding/decoding and is validated as-is. Whileregisterandloginuse an encode/decode pattern, andresetPasswordpasses the password directly, both approaches validate the samepasswordPatternagainst the raw character, so the end result is functionally equivalent. No fix is required.Likely an incorrect or invalid review comment.
| if (req.body?.email) { | ||
| req.body.email = req.body.email?.toLowerCase(); | ||
| } | ||
| req.body.password = decodeURIComponent(req.body.password); |
There was a problem hiding this comment.
Add null/undefined check before decoding the password.
If req.body.password is undefined or null, decodeURIComponent will convert it to the string "undefined" or "null", which may bypass Joi validation for required fields.
Apply this defensive pattern:
+if (req.body.password) {
req.body.password = decodeURIComponent(req.body.password);
+}Apply the same fix on line 131 in loginUser.
Also applies to: 131-131
🤖 Prompt for AI Agents
In server/src/controllers/v1/authController.js around lines 94 and 131, the code
unconditionally calls decodeURIComponent on req.body.password which will coerce
null/undefined to the strings "null"/"undefined" and can bypass Joi required
checks; update both places to first verify the password is a non-empty string
(e.g., typeof req.body.password === 'string' and req.body.password !== '')
before calling decodeURIComponent, otherwise leave it as undefined or null so
validation fails properly.
![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 fixes a password validation bug but introduces a critical client-server coupling and a security vulnerability by decoding user input before validation.
🌟 Strengths
- Successfully resolves the reported issue where passwords containing '<' characters were incorrectly rejected.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | client/src/Features/Auth/authSlice.js | Architecture | Client-server coupling risks breaking future client apps | |
| P1 | server/.../authController.js | Security | Decoding before validation risks bypassing security checks | |
| P2 | server/.../authController.js | Maintainability | Duplicated code increases maintenance risk | |
| P2 | client/src/Features/Auth/authSlice.js | Testing | Missing tests risk undetected regressions |
🔍 Notable Themes
- The encoding/decoding workaround introduces systemic coupling and security risks instead of addressing the root cause in the sanitization middleware.
📈 Risk Diagram
This diagram illustrates the new password encoding/decoding flow and its associated security and coupling risks.
sequenceDiagram
participant C as Client
participant AC as Auth Controller
participant US as User Service
C->>AC: POST /register (password encoded)
note over AC: R2(P1): Decodes before validation,<br/>risks security bypass
AC->>US: registerUser(decoded password)
note over C,AC: R1(P1): Tight coupling requires<br/>all clients to encode
💡 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.
| export const register = createAsyncThunk("auth/register", async (form, thunkApi) => { | ||
| try { | ||
| const res = await networkService.registerUser(form); | ||
| const reqObj = { ...form, password: encodeURIComponent(form.password) }; | ||
| const res = await networkService.registerUser(reqObj); | ||
| return res.data; |
There was a problem hiding this comment.
P1 | Confidence: High
This change introduces a client-side encoding dependency that creates a tight coupling between client and server implementations. The client now assumes the server will decode passwords, making the API contract implicit rather than explicit. This breaks the principle of separation of concerns - password encoding/decoding should be handled consistently on the server side where security logic belongs. Any future client implementation (mobile app, CLI) must replicate this encoding logic or authentication will fail.
Code Suggestion:
Remove client-side encoding and fix the root cause in server-side sanitization middleware.P2 | Confidence: Medium
Speculative: The PR description shows manual testing but doesn't mention automated tests. Password encoding/decoding is critical security functionality that should have unit tests covering edge cases (special characters, malformed encoding, empty passwords). Without tests, regressions could go undetected.
Code Suggestion:
Add unit tests for authSlice encoding and authController decoding with various special character combinations.| */ | ||
| registerUser = this.asyncHandler( | ||
| async (req, res) => { | ||
| if (req.body?.email) { |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 89. Anchored to nearest_changed(91) line 91.
P1 | Confidence: High
This introduces a security vulnerability by decoding user input before validation. If an attacker sends URL-encoded malicious content, it will be decoded and then validated, potentially bypassing security checks. The validation schema expects raw password input, but now receives decoded content, creating inconsistency. Additionally, decodeURIComponent will throw on malformed URI components, potentially causing server errors.
Code Suggestion:
Remove server-side decoding and instead configure the sanitization middleware to exclude password fields or use proper escaping rather than removal.| */ | ||
| loginUser = this.asyncHandler( | ||
| async (req, res) => { | ||
| if (req.body?.email) { |
There was a problem hiding this comment.
[Contextual Comment]
This comment refers to code near real line 126. Anchored to nearest_changed(128) line 128.
P2 | Confidence: High
The same decoding logic is duplicated in both register and login methods, violating DRY principles. Any future changes to password handling must be made in multiple places, increasing maintenance burden and risk of inconsistencies.
Code Suggestion:
Extract password decoding (if kept) into a shared middleware or utility function used by both endpoints.
|
Hey @Darshan98Solanki , While it works, this seems like an overly complex solution to the problem. Moreover, it creates a tight dependency between the front and back ends, which is to be avoided. Is there some reason the validation on the frontend can't be modified to allow for the password in question? |
|
hey, @ajhollid I understood your concern give me some time I will find some solution with decoupled for same. |
I think we need to think in terms of regex here, let me know what solution you have thought of. |
|
can we restrict < > this two letter for password or else pass the payload with stringify version but that is also tightly coupled. I think restrict this letter will be good solution ? |
Stringifying or otherwise transforming the input defeats the purpose of input validation, so let's avoid that. The input vaidation schema should be modified to allow for the |
|
New Regex:- /^(?=.[a-z])(?=.[A-Z])(?=.\d)(?=.[!?@#$%^&*()-_=+[]{};:'",.~ Old Regex:- /^(?=.[a-z])(?=.[A-Z])(?=.[0-9])(?=.[!?@#$%^&*()-_=+[]{};:'",.<>~ Difference Note :-
I think this regex is good for validation should I change this for validation of password ? |
|
@ajhollid @Owaiseimdad @gorkemcetin as per we discussed last any inputs ? should I remove <> sign from the password regex to prevent XXS attack and make it more secure also change the frontend relevant if needed. |
|
@ajhollid @Owaiseimdad @gorkemcetin i'm looking inputs from your what should do further? |
|
@Darshan98Solanki the new regex must be only more premissive than the original one, it cannot be any more restrictive or it could possibly disallow existing users. So you can remove the min 8 char and it should be good to go. |
|
@ajhollid |
We can do that in another PR. |
4 participants
Hey @gorkemcetin,
Fixes #3010
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
npm run formatin server and client directories, which automatically formats your code.I just have encoded the password before hit request (cause it is reserved keywords) and decode that on backend before validating and insert option in db.
Note:- from frontend we saw that password is there in payload but when we log password in backend we see empty string like ''. So this method I have added.
Have given the SS for validating the changed on real time.
Signup form:-
Login form:-
After login:-
Please check the pr and let me know is there is any updates.
Summary by CodeRabbit