feat: upgrade otplib to version 13.4.0 and refactor OTP handling methods#1793
Conversation
📝 WalkthroughWalkthroughThis PR upgrades the Changesotplib v13.4.0 migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the backend OTP library (otplib) to v13.4.0 and updates the backend’s OTP secret generation, verification, and QR-code URI creation to use the new otplib APIs.
Changes:
- Bump
otplibfrom^12.0.1to^13.4.0and refreshpnpm-lock.yamlwith new otplib v13 transitive dependencies. - Refactor OTP enrollment QR code generation to build otpauth URIs via
generateURI. - Refactor OTP verification and login flows to use
verifySync(...).valid, and secret generation to usegenerateSecret().
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile for otplib v13; introduces new transitive dependencies (including @noble/hashes@2.2.0 with a Node engine constraint). |
| backend/package.json | Bumps otplib dependency version to ^13.4.0. |
| backend/src/entities/user/utils/generate-qr-code.ts | Switches QR-code otpauth URI generation from authenticator.keyuri to generateURI. |
| backend/src/entities/user/use-cases/verify-otp-use.case.ts | Updates OTP verification to use the new otplib verification API. |
| backend/src/entities/user/use-cases/otp-login-use.case.ts | Updates OTP login verification to use the new otplib verification API. |
| backend/src/entities/user/use-cases/generate-otp-use.case.ts | Updates OTP secret generation to use the new otplib secret generator. |
| backend/src/entities/user/use-cases/disable-otp.use.case.ts | Updates OTP disabling verification to use the new otplib verification API. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '@noble/hashes@2.2.0': | ||
| resolution: {integrity: sha512-IYqDGiTXab6FniAgnSdZwgWbomxpy9FtYvLKs7wCUs2a8RkITG+DFGO1DM9cr+E3/RgADRpFjrKVaJ1z6sjtEg==} | ||
| engines: {node: '>= 20.19.0'} |
![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 (2)
backend/src/entities/user/use-cases/disable-otp.use.case.ts (1)
44-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn 400 for invalid OTP instead of 500.
When Line 44 evaluates to
isValid === false, execution reaches Line 56 and returnsInternalServerErrorException. That path is a normal invalid-token case and should map toBadRequestException.Proposed fix
try { const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid; - if (isValid) { - foundUser.isOTPEnabled = false; - foundUser.otpSecretKey = null; - await this._dbContext.userRepository.saveUserEntity(foundUser); - return { - disabled: true, - }; - } + if (!isValid) { + throw new BadRequestException(Messages.OTP_DISABLING_FAILED_INVALID_TOKEN); + } + foundUser.isOTPEnabled = false; + foundUser.otpSecretKey = null; + await this._dbContext.userRepository.saveUserEntity(foundUser); + return { + disabled: true, + }; } catch (_error) { throw new BadRequestException(Messages.OTP_DISABLING_FAILED_INVALID_TOKEN); } - throw new InternalServerErrorException(Messages.OTP_DISABLING_FAILED); + throw new InternalServerErrorException(Messages.OTP_DISABLING_FAILED);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/user/use-cases/disable-otp.use.case.ts` around lines 44 - 56, The current flow treats a false OTP verification as an internal server error; update the logic in disable-otp.use.case.ts around verifySync({ token: otpToken, secret: otpSecretKey }) so that when isValid is false you explicitly throw new BadRequestException(Messages.OTP_DISABLING_FAILED_INVALID_TOKEN) (instead of falling through), and reserve the catch block for unexpected exceptions (re-throw as InternalServerErrorException(Messages.OTP_DISABLING_FAILED)); keep the existing success branch that sets foundUser.isOTPEnabled = false, foundUser.otpSecretKey = null and calls this._dbContext.userRepository.saveUserEntity(foundUser).backend/src/entities/user/use-cases/verify-otp-use.case.ts (1)
51-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid OTP path currently returns 500.
If Line 51 yields
isValid === false, the code falls through to Line 67 and throwsINTERNAL_SERVER_ERROR. This should be treated as a client validation failure.Proposed fix
try { const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid; - if (isValid) { - foundUser.isOTPEnabled = true; - await this._dbContext.userRepository.saveUserEntity(foundUser); - return { - validated: true, - }; - } + if (!isValid) { + throw new HttpException( + { message: Messages.OTP_VALIDATION_FAILED }, + HttpStatus.BAD_REQUEST, + ); + } + foundUser.isOTPEnabled = true; + await this._dbContext.userRepository.saveUserEntity(foundUser); + return { + validated: true, + }; } catch (_error) { throw new HttpException( { message: Messages.OTP_VALIDATION_FAILED, }, HttpStatus.BAD_REQUEST, ); } - throw new HttpException( - { - message: Messages.OTP_VALIDATION_FAILED, - }, - HttpStatus.INTERNAL_SERVER_ERROR, - ); + throw new HttpException( + { + message: Messages.OTP_VALIDATION_FAILED, + }, + HttpStatus.INTERNAL_SERVER_ERROR, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/user/use-cases/verify-otp-use.case.ts` around lines 51 - 72, The code currently falls through to an INTERNAL_SERVER_ERROR when verifySync returns isValid === false; change the control flow so an invalid OTP returns a client error: immediately after computing isValid from verifySync({ token: otpToken, secret: otpSecretKey }) throw an HttpException with HttpStatus.BAD_REQUEST and Messages.OTP_VALIDATION_FAILED if isValid is false (or replace the final INTERNAL_SERVER_ERROR throw with the BAD_REQUEST variant), keeping the existing catch block for unexpected exceptions and preserving the success path that sets foundUser.isOTPEnabled and calls this._dbContext.userRepository.saveUserEntity(foundUser).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/entities/user/use-cases/otp-login-use.case.ts`:
- Line 31: The OTP verification in otp-login-use.case.ts calls verifySync({
token: otpToken, secret: foundUser.otpSecretKey }) without checking that
foundUser.otpSecretKey is present or catching verification errors; update the
logic in the OTP login use-case to first validate that foundUser.otpSecretKey is
not null/undefined and throw a clear error if missing, then wrap the verifySync
call in a try/catch and handle verification failures by throwing the appropriate
domain error (instead of letting exceptions bubble), replacing the current
direct assignment to isValid with this guarded, error-handled flow.
---
Outside diff comments:
In `@backend/src/entities/user/use-cases/disable-otp.use.case.ts`:
- Around line 44-56: The current flow treats a false OTP verification as an
internal server error; update the logic in disable-otp.use.case.ts around
verifySync({ token: otpToken, secret: otpSecretKey }) so that when isValid is
false you explicitly throw new
BadRequestException(Messages.OTP_DISABLING_FAILED_INVALID_TOKEN) (instead of
falling through), and reserve the catch block for unexpected exceptions
(re-throw as InternalServerErrorException(Messages.OTP_DISABLING_FAILED)); keep
the existing success branch that sets foundUser.isOTPEnabled = false,
foundUser.otpSecretKey = null and calls
this._dbContext.userRepository.saveUserEntity(foundUser).
In `@backend/src/entities/user/use-cases/verify-otp-use.case.ts`:
- Around line 51-72: The code currently falls through to an
INTERNAL_SERVER_ERROR when verifySync returns isValid === false; change the
control flow so an invalid OTP returns a client error: immediately after
computing isValid from verifySync({ token: otpToken, secret: otpSecretKey })
throw an HttpException with HttpStatus.BAD_REQUEST and
Messages.OTP_VALIDATION_FAILED if isValid is false (or replace the final
INTERNAL_SERVER_ERROR throw with the BAD_REQUEST variant), keeping the existing
catch block for unexpected exceptions and preserving the success path that sets
foundUser.isOTPEnabled and calls
this._dbContext.userRepository.saveUserEntity(foundUser).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 617bdb82-774d-432e-b6e3-78bc3a546aa2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
backend/package.jsonbackend/src/entities/user/use-cases/disable-otp.use.case.tsbackend/src/entities/user/use-cases/generate-otp-use.case.tsbackend/src/entities/user/use-cases/otp-login-use.case.tsbackend/src/entities/user/use-cases/verify-otp-use.case.tsbackend/src/entities/user/utils/generate-qr-code.ts
| throw new NotFoundException(Messages.USER_NOT_FOUND); | ||
| } | ||
| const isValid = authenticator.check(otpToken, foundUser.otpSecretKey); | ||
| const isValid = verifySync({ token: otpToken, secret: foundUser.otpSecretKey }).valid; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In otplib v13.4.0, under what conditions can verifySync throw, and does it return { valid: false } for invalid tokens?
💡 Result:
In otplib v13.4.0, the verifySync function is designed to return a result object rather than throwing exceptions for failed token validations [1][2]. Conditions for verifySync to throw: The verifySync function will throw an error if it is used with a crypto plugin that does not support synchronous HMAC operations [1][2]. For example, attempting to use the WebCryptoPlugin, which is inherently asynchronous, with verifySync will cause it to throw [1][2]. Return value for invalid tokens: For invalid tokens, verifySync returns an object in the format { valid: false } [1][2]. Successful verifications return an object containing { valid: true,... } along with additional properties such as delta, epoch, or timeStep [1][2].
Citations:
- 1: https://npmx.dev/package/@otplib/totp/v/13.4.0
- 2: https://otplib.yeojz.dev/api/@otplib/totp/functions/verifySync.html
🏁 Script executed:
# First, let's read the file to understand the context
cat -n backend/src/entities/user/use-cases/otp-login-use.case.ts | head -50Repository: rocket-admin/rocketadmin
Length of output: 2683
🏁 Script executed:
# Check if there's a crypto plugin configuration anywhere in the backend
find backend -type f \( -name "*.ts" -o -name "*.js" -o -name "*.json" \) | xargs rg -l "WebCrypto|crypto.*plugin|otplib" | head -20Repository: rocket-admin/rocketadmin
Length of output: 379
🏁 Script executed:
# Look for how otplib is initialized/imported
rg "import.*otplib|from.*otplib" backend/Repository: rocket-admin/rocketadmin
Length of output: 548
🏁 Script executed:
# Check the backend package.json for otplib version
cat backend/package.json | grep -A 5 -B 5 "otplib"Repository: rocket-admin/rocketadmin
Length of output: 350
🏁 Script executed:
# Check if otpSecretKey could be undefined by looking at user entity/type
rg "otpSecretKey" backend/src/entities/ -B 2 -A 2 | head -40Repository: rocket-admin/rocketadmin
Length of output: 2906
🏁 Script executed:
# Check if there's any crypto plugin configuration or initialization
rg -i "webcrypto|crypto.*plugin" backend/Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
# Look at other verifySync usages to see if they use try/catch
rg "verifySync" backend/ -B 3 -A 3Repository: rocket-admin/rocketadmin
Length of output: 4121
Add null validation and error handling for OTP verification.
Line 31 calls verifySync directly with foundUser.otpSecretKey without validating it first or wrapping in error handling. Since otpSecretKey can be null (defined with default: null in the user entity), this will cause unexpected behavior. Additionally, similar OTP use-cases in the codebase (disable-otp.use.case.ts, verify-otp-use.case.ts) both validate the secret and wrap verifySync in try/catch, making this inconsistent.
Proposed fix
- const isValid = verifySync({ token: otpToken, secret: foundUser.otpSecretKey }).valid;
+ const otpSecretKey = foundUser.otpSecretKey;
+ let isValid = false;
+ if (otpSecretKey) {
+ try {
+ isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid;
+ } catch (_error) {
+ isValid = false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isValid = verifySync({ token: otpToken, secret: foundUser.otpSecretKey }).valid; | |
| const otpSecretKey = foundUser.otpSecretKey; | |
| let isValid = false; | |
| if (otpSecretKey) { | |
| try { | |
| isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid; | |
| } catch (_error) { | |
| isValid = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/entities/user/use-cases/otp-login-use.case.ts` at line 31, The
OTP verification in otp-login-use.case.ts calls verifySync({ token: otpToken,
secret: foundUser.otpSecretKey }) without checking that foundUser.otpSecretKey
is present or catching verification errors; update the logic in the OTP login
use-case to first validate that foundUser.otpSecretKey is not null/undefined and
throw a clear error if missing, then wrap the verifySync call in a try/catch and
handle verification failures by throwing the appropriate domain error (instead
of letting exceptions bubble), replacing the current direct assignment to
isValid with this guarded, error-handled flow.
Summary by CodeRabbit
Bug Fixes
Chores