Skip to content

feat: upgrade otplib to version 13.4.0 and refactor OTP handling methods #1793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Artuomka merged 2 commits into from
May 20, 2026
Merged

feat: upgrade otplib to version 13.4.0 and refactor OTP handling methods #1793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e704c7
feat: upgrade otplib to version 13.4.0 and refactor OTP handling methods
Artuomka May 20, 2026
6271585
Merge branch 'main' into backend_up_npm_libs
Artuomka May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion backend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"node-sql-parser": "^5.4.0",
"nodemailer": "^8.0.7",
"nunjucks": "^3.2.4",
"otplib": "^12.0.1",
"otplib": "^13.4.0",
"p-queue": "9.3.0",
"pg-connection-string": "^2.13.0",
"qrcode": "^1.5.4",
Expand Down
4 changes: 2 additions & 2 deletions backend/src/entities/user/use-cases/disable-otp.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
InternalServerErrorException,
NotFoundException,
} from '@nestjs/common';
import { authenticator } from 'otplib';
import { verifySync } from 'otplib';
import AbstractUseCase from '../../../common/abstract-use.case.js';
import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js';
import { BaseType } from '../../../common/data-injection.tokens.js';
Expand Down Expand Up @@ -41,7 +41,7 @@ export class DisableOtpUseCase extends AbstractUseCase<VerifyOtpDS, OtpDisabling
throw new BadRequestException(Messages.OTP_NOT_ENABLED);
}
try {
const isValid = authenticator.check(otpToken, otpSecretKey);
const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid;
if (isValid) {
foundUser.isOTPEnabled = false;
foundUser.otpSecretKey = null;
Expand Down
4 changes: 2 additions & 2 deletions backend/src/entities/user/use-cases/generate-otp-use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common';
import { authenticator } from 'otplib';
import { generateSecret } from 'otplib';
import AbstractUseCase from '../../../common/abstract-use.case.js';
import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js';
import { BaseType } from '../../../common/data-injection.tokens.js';
Expand Down Expand Up @@ -37,7 +37,7 @@ export class GenerateOtpUseCase extends AbstractUseCase<string, OtpSecretDS> imp
);
}

const otpSecretKey = authenticator.generateSecret();
const otpSecretKey = generateSecret();
foundUser.otpSecretKey = otpSecretKey;
await this._dbContext.userRepository.saveUserEntity(foundUser);
const { otpauth, qrCode } = await generateQRCode(foundUser.email, otpSecretKey);
Expand Down
4 changes: 2 additions & 2 deletions backend/src/entities/user/use-cases/otp-login-use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BadRequestException, Inject, Injectable, NotFoundException } from '@nestjs/common';
import { authenticator } from 'otplib';
import { verifySync } from 'otplib';
import AbstractUseCase from '../../../common/abstract-use.case.js';
import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js';
import { BaseType } from '../../../common/data-injection.tokens.js';
Expand Down Expand Up @@ -28,7 +28,7 @@ export class OtpLoginUseCase extends AbstractUseCase<VerifyOtpDS, IToken> implem
if (!foundUser) {
throw new NotFoundException(Messages.USER_NOT_FOUND);
}
const isValid = authenticator.check(otpToken, foundUser.otpSecretKey);
const isValid = verifySync({ token: otpToken, secret: foundUser.otpSecretKey }).valid;
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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:

🏁 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 -50

Repository: 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 -20

Repository: 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 -40

Repository: 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 3

Repository: 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.

Suggested change
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.

if (!isValid) {
await this.recordSignInAudit(
foundUser.email,
Expand Down
4 changes: 2 additions & 2 deletions backend/src/entities/user/use-cases/verify-otp-use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common';
import { authenticator } from 'otplib';
import { verifySync } from 'otplib';
import AbstractUseCase from '../../../common/abstract-use.case.js';
import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js';
import { BaseType } from '../../../common/data-injection.tokens.js';
Expand Down Expand Up @@ -48,7 +48,7 @@ export class VerifyOtpUseCase extends AbstractUseCase<VerifyOtpDS, OtpValidation
);
}
try {
const isValid = authenticator.check(otpToken, otpSecretKey);
const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid;
if (isValid) {
foundUser.isOTPEnabled = true;
await this._dbContext.userRepository.saveUserEntity(foundUser);
Expand Down
4 changes: 2 additions & 2 deletions backend/src/entities/user/utils/generate-qr-code.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { authenticator } from 'otplib';
import { generateURI } from 'otplib';
import QRCode from 'qrcode';

export async function generateQRCode(userEmail: string, secretKey: string) {
const service = 'Rocketadmin';
const otpauth = authenticator.keyuri(userEmail, service, secretKey);
const otpauth = generateURI({ issuer: service, label: userEmail, secret: secretKey });
const qrCode = await QRCode.toDataURL(otpauth);
return { qrCode, otpauth };
}
94 changes: 53 additions & 41 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading