fix: add try-catch around JSON.parse(userData) in auth services#88
fix: add try-catch around JSON.parse(userData) in auth services#88lawrence3699 wants to merge 1 commit intoAkkalDhami:mainfrom
Conversation
|
@lawrence3699 is attempting to deploy a commit to the akkaldhami's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughJSON parsing error handling was enhanced across multiple authentication service templates. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/feature/src/modules/auth/auth.service.ts`:
- Around line 127-133: The code parses userData into parsedData but then
destructures without validating shape, so values like null or arrays will cause
runtime errors or bad inserts; in auth.service.ts add a type guard after
JSON.parse that checks parsedData is a non-null plain object (typeof parsedData
=== "object" && parsedData !== null && !Array.isArray(parsedData)), then verify
required fields (name, email, password, optional role) exist and have expected
primitive types (e.g., strings) and otherwise throw ApiError.badRequest("Invalid
user data format"); only after these checks perform the destructuring and
proceed to the DB insert (the block that currently destructures name, email:
userEmail, role, password and inserts on the following lines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 325653c4-492e-447d-ae95-7756d8dbc8cd
📒 Files selected for processing (6)
packages/templates/node/express/blueprint/hybrid-auth/mongodb/mongoose/feature/src/modules/auth/auth.service.tspackages/templates/node/express/blueprint/hybrid-auth/mongodb/mongoose/mvc/src/services/auth.service.tspackages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/feature/src/modules/auth/auth.service.tspackages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/mvc/src/services/auth.service.tspackages/templates/node/express/blueprint/stateless-auth/mysql/drizzle/feature/src/modules/auth/auth.service.tspackages/templates/node/express/blueprint/stateless-auth/mysql/drizzle/mvc/src/services/auth.service.ts
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { | ||
| throw ApiError.badRequest("Invalid user data format"); | ||
| } | ||
| const { name, email: userEmail, role, password } = parsedData; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "auth.service.ts" | grep -E "auth/auth.service.ts"Repository: AkkalDhami/servercn
Length of output: 753
🏁 Script executed:
cat -n "packages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/feature/src/modules/auth/auth.service.ts" | sed -n '115,145p'Repository: AkkalDhami/servercn
Length of output: 1128
Validate parsed JSON shape before destructuring.
JSON.parse failure is handled, but line 133 still assumes an object payload. Valid JSON like null bypasses the catch and throws a TypeError during destructuring. This can result in unhandled errors and potentially invalid data being inserted into the database on line 140-144.
Add a type guard to ensure parsedData is an object before destructuring:
Proposed fix
- let parsedData;
+ let parsedData: unknown;
try {
parsedData = JSON.parse(userData);
} catch {
throw ApiError.badRequest("Invalid user data format");
}
- const { name, email: userEmail, role, password } = parsedData;
+ if (!parsedData || typeof parsedData !== "object") {
+ throw ApiError.badRequest("Invalid user data format");
+ }
+
+ const {
+ name,
+ email: userEmail,
+ role,
+ password
+ } = parsedData as Partial<{
+ name: string;
+ email: string;
+ role: string;
+ password: string;
+ }>;
+
+ if (
+ typeof name !== "string" ||
+ typeof userEmail !== "string" ||
+ typeof password !== "string"
+ ) {
+ throw ApiError.badRequest("Invalid user data format");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/feature/src/modules/auth/auth.service.ts`
around lines 127 - 133, The code parses userData into parsedData but then
destructures without validating shape, so values like null or arrays will cause
runtime errors or bad inserts; in auth.service.ts add a type guard after
JSON.parse that checks parsedData is a non-null plain object (typeof parsedData
=== "object" && parsedData !== null && !Array.isArray(parsedData)), then verify
required fields (name, email, password, optional role) exist and have expected
primitive types (e.g., strings) and otherwise throw ApiError.badRequest("Invalid
user data format"); only after these checks perform the destructuring and
proceed to the DB insert (the block that currently destructures name, email:
userEmail, role, password and inserts on the following lines).
There was a problem hiding this comment.
Pull request overview
Adds defensive handling around parsing cached signup payloads from Redis during OTP/email verification in the Node/Express auth templates, preventing invalid/corrupted JSON from propagating as an unhandled exception.
Changes:
- Wraps
JSON.parse(userData)in try/catch during the OTP verification flow. - Converts JSON parse failures into
ApiError.badRequest("Invalid user data format")across the affected template variants.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/templates/node/express/blueprint/stateless-auth/mysql/drizzle/mvc/src/services/auth.service.ts | Adds try/catch around parsing Redis-stored signup payload in verifyUser. |
| packages/templates/node/express/blueprint/stateless-auth/mysql/drizzle/feature/src/modules/auth/auth.service.ts | Same parse-guard in feature-module variant. |
| packages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/mvc/src/services/auth.service.ts | Same parse-guard in hybrid Postgres MVC variant. |
| packages/templates/node/express/blueprint/hybrid-auth/postgresql/drizzle/feature/src/modules/auth/auth.service.ts | Same parse-guard in hybrid Postgres feature variant. |
| packages/templates/node/express/blueprint/hybrid-auth/mongodb/mongoose/mvc/src/services/auth.service.ts | Same parse-guard in hybrid Mongo MVC variant. |
| packages/templates/node/express/blueprint/hybrid-auth/mongodb/mongoose/feature/src/modules/auth/auth.service.ts | Same parse-guard in hybrid Mongo feature variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { | ||
| throw ApiError.badRequest("Invalid user data format"); | ||
| } |
There was a problem hiding this comment.
If JSON.parse(userData) fails here, OTP has already been consumed by OtpService.verifyOtp, but the Redis user:${email}:${hashCode} key is left behind until TTL. Consider best-effort deleting that key in the catch before throwing so corrupted/sensitive cached signup data doesn’t linger and the user can restart the flow cleanly.
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { | ||
| throw ApiError.badRequest("Invalid user data format"); | ||
| } |
There was a problem hiding this comment.
If JSON.parse(userData) fails here, OTP has already been consumed by OtpService.verifyOtp, but the Redis user:${email}:${hashCode} key is left behind until TTL. Consider best-effort deleting that key in the catch before throwing so corrupted/sensitive cached signup data doesn’t linger and the user can restart the flow cleanly.
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { |
There was a problem hiding this comment.
On JSON.parse failure, OTP has already been consumed, but the pending-signup keys remain (user:${email}:${hashCode} and user:pending:${email}) until TTL. That can leave a user stuck in “signup already in progress” with no usable OTP; consider best-effort deleting both keys in this catch before throwing.
| } catch { | |
| } catch (error) { | |
| await Promise.allSettled([ | |
| redisClient.del(`user:${email}:${hashCode}`), | |
| redisClient.del(`user:pending:${email}`) | |
| ]); |
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { | ||
| throw ApiError.badRequest("Invalid user data format"); | ||
| } |
There was a problem hiding this comment.
On JSON.parse failure, OTP has already been consumed, but the pending-signup keys remain (user:${email}:${hashCode} and user:pending:${email}) until TTL. That can leave a user stuck in “signup already in progress” with no usable OTP; consider best-effort deleting both keys in this catch before throwing.
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { |
There was a problem hiding this comment.
On JSON.parse failure, OTP has already been consumed, but the pending-signup keys remain (user:${email}:${hashCode} and user:pending:${email}) until TTL. That can leave a user stuck in “signup already in progress” with no usable OTP; consider best-effort deleting both keys in this catch before throwing.
| } catch { | |
| } catch (parseError) { | |
| // Best-effort cleanup to avoid leaving the user stuck with a pending signup | |
| try { | |
| await redisClient.del(`user:${email}:${hashCode}`); | |
| await redisClient.del(`user:pending:${email}`); | |
| } catch (cleanupError) { | |
| logger.error( | |
| cleanupError, | |
| "Failed to clean up pending signup keys after JSON parse error" | |
| ); | |
| } |
| let parsedData; | ||
| try { | ||
| parsedData = JSON.parse(userData); | ||
| } catch { |
There was a problem hiding this comment.
On JSON.parse failure, OTP has already been consumed, but the pending-signup keys remain (user:${email}:${hashCode} and user:pending:${email}) until TTL. That can leave a user stuck in “signup already in progress” with no usable OTP; consider best-effort deleting both keys in this catch before throwing.
| } catch { | |
| } catch { | |
| // Best-effort cleanup to avoid leaving the user stuck in "signup already in progress" | |
| await redisClient.del(`user:${email}:${hashCode}`); | |
| await redisClient.del(`user:pending:${email}`); |
Summary
JSON.parse(userData)in the auth serviceverifyEmailflow can throwSyntaxErrorif Redis stores corrupted/invalid JSON. While the null check validatesuserDataexists, it doesn't validate JSON format.Added try-catch with
ApiError.badRequestacross all 6 affected template variants (hybrid-auth and stateless-auth for MongoDB, PostgreSQL, MySQL).Test plan
Summary by CodeRabbit