fix: require OTP verification for recovery unlock#115
Merged
Junman140 merged 2 commits intoPi-Defi-world:mainfrom Mar 29, 2026
Merged
fix: require OTP verification for recovery unlock#115Junman140 merged 2 commits intoPi-Defi-world:mainfrom
Junman140 merged 2 commits intoPi-Defi-world:mainfrom
Conversation
|
@Yusufolosun Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughImplements a two-step recovery unlock: Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller
participant Service
participant Database
participant RabbitMQ
participant JWT
rect rgba(100, 150, 255, 0.5)
Note over Client,JWT: Step 1 — Unlock Request
Client->>Controller: POST /recovery/unlock\n{ identifier, passcode }
Controller->>Service: unlockApp({ identifier, passcode })
Service->>Database: Find user by identifier
Service->>Service: Verify passcode hash
Service->>Service: Generate 6-digit OTP
Service->>Database: Store hashed OTP challenge (10-min expiry)
Service->>JWT: Sign challenge token
Service->>RabbitMQ: Publish OTP to queue (email/sms)
Service-->>Controller: { challenge_token, channel }
Controller-->>Client: 200 OK\n{ challenge_token, channel }
end
rect rgba(150, 200, 100, 0.5)
Note over Client,JWT: Step 2 — OTP Verification
Client->>Controller: POST /recovery/unlock/verify\n{ challenge_token, code }
Controller->>Service: verifyRecoveryOtp({ challenge_token, code })
Service->>JWT: Verify & decode challenge token
Service->>Database: Lookup latest unused, non-expired OTP challenge
Service->>Service: Verify OTP code hash
Service->>Database: Mark OTP challenge as used
Service->>Service: Issue new API key
Service-->>Controller: { api_key, user_id }
Controller-->>Client: 200 OK\n{ api_key, user_id }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/recoveryController.ts`:
- Around line 87-93: Normalize the error thrown by the challenge verification so
the controller can reliably map it to a 401: update verifyChallengeToken in the
recoveryService (and any calls in src/utils/jwt.ts) to throw a standardized
error (e.g., throw new AppError("Invalid or expired challenge", 401)) for all
challenge-related failures, or alternatively expand the recoveryController.ts
check around the e.message (the block with e.message === "Invalid or expired
challenge") to treat any message containing the word "challenge"
(case-insensitive) or the specific messages emitted by verifyChallengeToken/jwt
utilities as a 401; reference verifyChallengeToken, recoveryService, and the jwt
utility when making the change so all challenge-failure paths are consistently
mapped to next(new AppError(..., 401)).
In `@src/services/recovery/recoveryService.ts`:
- Line 12: OTP_EXPIRY_MINUTES is set to 10 but the challenge JWT in
src/utils/jwt.ts is hardcoded to "5m", causing the challenge token to expire
before the OTP row; update the code so both expiries come from the same source:
either export OTP_EXPIRY_MINUTES (or a derived constant like OTP_TTL) and use it
to build the JWT expiry in the jwt helper instead of "5m", or modify the
recovery service to pass the OTP TTL into the JWT creation function; adjust all
uses (e.g., OTP_EXPIRY_MINUTES, the JWT helper function that issues the
challenge token, and any calling code in the recovery/unlock flow) so they read
the same TTL value.
- Around line 138-160: The OTP redeem flow is raceable because you read with
prisma.otpChallenge.findFirst then unconditionally call
prisma.otpChallenge.update, allowing two concurrent requests to both succeed;
change the claim to an atomic conditional update: replace the
prisma.otpChallenge.update call with an atomic operation (e.g.,
prisma.otpChallenge.updateMany or an equivalent) that includes usedAt: null (and
still validates expiresAt if desired) in the WHERE clause, then check the
returned count/affectedRows and throw “Invalid or expired code” if it is not
exactly 1 so only the caller that actually claimed the row proceeds to issue API
keys; keep the bcrypt.compare step and only attempt the atomic claim after a
successful password match, and abort if the conditional update did not affect
exactly one row.
- Around line 34-36: The generateOtpCode function uses Math.random which is not
cryptographically secure; replace it with the Node crypto CSPRNG by importing
randomInt from "crypto" and using randomInt to produce a 6-digit number (e.g.,
randomInt(100000, 1000000)) inside generateOtpCode so it returns a securely
generated 6-digit string; update the import and the body of generateOtpCode
accordingly.
In `@tests/recovery.test.ts`:
- Around line 107-125: The test seeds an OTP row via prisma.otpChallenge.create
before calling unlockApp(), but verifyRecoveryOtp() (in
src/services/recovery/recoveryService.ts) always loads the latest unused
challenge for the user, so the OTP "123456" doesn't match the challenge tied to
the challengeToken returned by unlockApp(); fix by obtaining the actual
OTP/challenge produced by the RabbitMQ mock or by making OTP generation
deterministic and reusing the same challengeToken for both verification attempts
(i.e., stop creating a separate prisma.otpChallenge entry and instead capture
the OTP/challenge emitted by the RabbitMQ mock or force unlockApp()/challenge
creation to use the seeded value so verifyRecoveryOtp() validates against the
correct challengeToken).
- Around line 136-145: The test uses a handcrafted JWT which only proves
rejection of malformed tokens; replace it with a legitimately-signed challenge
token so expiry is actually exercised: call signChallengeToken (or the same
token-creation helper used in production) to create a token with a short TTL or
explicitly set exp to a past timestamp, then advance/freeze time (or set exp) so
the token is expired and call verifyRecoveryOtp({ challenge_token: <that token>,
code: otpCode }) expecting a rejection; reference signChallengeToken and
verifyRecoveryOtp to locate where to create and verify the real expired token.
🪄 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: 90c2a064-8727-4cf6-8814-b2d945619dd7
📒 Files selected for processing (4)
src/controllers/recoveryController.tssrc/routes/recoveryRoutes.tssrc/services/recovery/recoveryService.tstests/recovery.test.ts
Comment on lines
+87
to
+93
| if ( | ||
| e.message === "Invalid or expired code" || | ||
| e.message === "Invalid code" | ||
| ) | ||
| return next(new AppError(e.message, 401)); | ||
| if (e.message === "Invalid or expired challenge") | ||
| return next(new AppError(e.message, 401)); |
There was a problem hiding this comment.
Bad challenge tokens will miss this 401 mapping.
src/services/recovery/recoveryService.ts, Line 135, forwards verifyChallengeToken() failures verbatim, but this block only matches "Invalid or expired challenge". Even the wrong-purpose case from src/utils/jwt.ts, Lines 33-35, misses this mapping, so bad challenge tokens can fall through to the generic error path instead of returning the intended 401. Normalize the service error or widen this branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controllers/recoveryController.ts` around lines 87 - 93, Normalize the
error thrown by the challenge verification so the controller can reliably map it
to a 401: update verifyChallengeToken in the recoveryService (and any calls in
src/utils/jwt.ts) to throw a standardized error (e.g., throw new
AppError("Invalid or expired challenge", 401)) for all challenge-related
failures, or alternatively expand the recoveryController.ts check around the
e.message (the block with e.message === "Invalid or expired challenge") to treat
any message containing the word "challenge" (case-insensitive) or the specific
messages emitted by verifyChallengeToken/jwt utilities as a 401; reference
verifyChallengeToken, recoveryService, and the jwt utility when making the
change so all challenge-failure paths are consistently mapped to next(new
AppError(..., 401)).
| import { signChallengeToken, verifyChallengeToken } from "../../utils/jwt"; | ||
| import { getRabbitMQChannel, QUEUES } from "../../config/rabbitmq"; | ||
|
|
||
| const OTP_EXPIRY_MINUTES = 10; |
There was a problem hiding this comment.
The challenge token expires five minutes before the OTP does.
OTP_EXPIRY_MINUTES is 10 here, but src/utils/jwt.ts, Line 8, hardcodes the challenge JWT to 5m. After minute 5 the OTP row is still valid while /recovery/unlock/verify can no longer redeem it, which cuts the advertised recovery window in half. Drive both expiries from the same constant or pass the OTP TTL into the JWT helper.
Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/recovery/recoveryService.ts` at line 12, OTP_EXPIRY_MINUTES is
set to 10 but the challenge JWT in src/utils/jwt.ts is hardcoded to "5m",
causing the challenge token to expire before the OTP row; update the code so
both expiries come from the same source: either export OTP_EXPIRY_MINUTES (or a
derived constant like OTP_TTL) and use it to build the JWT expiry in the jwt
helper instead of "5m", or modify the recovery service to pass the OTP TTL into
the JWT creation function; adjust all uses (e.g., OTP_EXPIRY_MINUTES, the JWT
helper function that issues the challenge token, and any calling code in the
recovery/unlock flow) so they read the same TTL value.
Comment on lines
+34
to
+36
| function generateOtpCode(): string { | ||
| return String(Math.floor(100000 + Math.random() * 900000)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/services/recovery/recoveryService.ts | head -50Repository: Pi-Defi-world/acbu-backend
Length of output: 1814
🏁 Script executed:
cat package.json | grep -A 2 -B 2 '"engines"'Repository: Pi-Defi-world/acbu-backend
Length of output: 52
🏁 Script executed:
rg "Math\.random|crypto\.randomInt|randomInt" src/services/recovery/Repository: Pi-Defi-world/acbu-backend
Length of output: 172
🏁 Script executed:
cat package.json | grep -E '"node"|"engines"' || echo "Node version info not found in root package.json"Repository: Pi-Defi-world/acbu-backend
Length of output: 118
🏁 Script executed:
ls -la | grep -E "\.nvmrc|\.node-version" || echo "No node version files found"Repository: Pi-Defi-world/acbu-backend
Length of output: 97
🏁 Script executed:
# Check if crypto.randomInt is used elsewhere in the codebase
rg "randomInt|crypto\.(random|secure)" --type ts --type jsRepository: Pi-Defi-world/acbu-backend
Length of output: 318
Use a CSPRNG for recovery OTPs.
Math.random() is not cryptographically secure; predictability weakens the second factor. Replace with crypto.randomInt() to generate 6-digit OTPs. The codebase already uses crypto for secure random values elsewhere (e.g., crypto.randomBytes() in auth middleware and user controller).
Suggested fix
function generateOtpCode(): string {
- return String(Math.floor(100000 + Math.random() * 900000));
+ return String(randomInt(100000, 1_000_000));
}Add to imports:
import { randomInt } from "crypto";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/recovery/recoveryService.ts` around lines 34 - 36, The
generateOtpCode function uses Math.random which is not cryptographically secure;
replace it with the Node crypto CSPRNG by importing randomInt from "crypto" and
using randomInt to produce a 6-digit number (e.g., randomInt(100000, 1000000))
inside generateOtpCode so it returns a securely generated 6-digit string; update
the import and the body of generateOtpCode accordingly.
Comment on lines
+138
to
+160
| const challenge = await prisma.otpChallenge.findFirst({ | ||
| where: { | ||
| userId: payload.userId, | ||
| expiresAt: { gt: now }, | ||
| usedAt: null, | ||
| }, | ||
| orderBy: { createdAt: "desc" }, | ||
| }); | ||
|
|
||
| if (!challenge) { | ||
| throw new Error("Invalid or expired code"); | ||
| } | ||
|
|
||
| const match = await bcrypt.compare(code, challenge.codeHash); | ||
| if (!match) { | ||
| logger.warn("Recovery: invalid OTP", { userId: payload.userId }); | ||
| throw new Error("Invalid code"); | ||
| } | ||
|
|
||
| await prisma.otpChallenge.update({ | ||
| where: { id: challenge.id }, | ||
| data: { usedAt: now }, | ||
| }); |
There was a problem hiding this comment.
Redeeming the OTP is raceable.
This is a read-then-write flow, and the update() at Lines 157-160 only matches by id. Two concurrent verify requests can both read the same unused challenge before either write happens, then both update it and both issue API keys. That breaks the single-use guarantee from the PR. Claim the row atomically and only continue when exactly one row was updated.
🧵 Proposed fix
- await prisma.otpChallenge.update({
- where: { id: challenge.id },
- data: { usedAt: now },
- });
+ const claimed = await prisma.otpChallenge.updateMany({
+ where: {
+ id: challenge.id,
+ usedAt: null,
+ expiresAt: { gt: now },
+ },
+ data: { usedAt: now },
+ });
+ if (claimed.count !== 1) {
+ throw new Error("Invalid or expired code");
+ }📝 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 challenge = await prisma.otpChallenge.findFirst({ | |
| where: { | |
| userId: payload.userId, | |
| expiresAt: { gt: now }, | |
| usedAt: null, | |
| }, | |
| orderBy: { createdAt: "desc" }, | |
| }); | |
| if (!challenge) { | |
| throw new Error("Invalid or expired code"); | |
| } | |
| const match = await bcrypt.compare(code, challenge.codeHash); | |
| if (!match) { | |
| logger.warn("Recovery: invalid OTP", { userId: payload.userId }); | |
| throw new Error("Invalid code"); | |
| } | |
| await prisma.otpChallenge.update({ | |
| where: { id: challenge.id }, | |
| data: { usedAt: now }, | |
| }); | |
| const challenge = await prisma.otpChallenge.findFirst({ | |
| where: { | |
| userId: payload.userId, | |
| expiresAt: { gt: now }, | |
| usedAt: null, | |
| }, | |
| orderBy: { createdAt: "desc" }, | |
| }); | |
| if (!challenge) { | |
| throw new Error("Invalid or expired code"); | |
| } | |
| const match = await bcrypt.compare(code, challenge.codeHash); | |
| if (!match) { | |
| logger.warn("Recovery: invalid OTP", { userId: payload.userId }); | |
| throw new Error("Invalid code"); | |
| } | |
| const claimed = await prisma.otpChallenge.updateMany({ | |
| where: { | |
| id: challenge.id, | |
| usedAt: null, | |
| expiresAt: { gt: now }, | |
| }, | |
| data: { usedAt: now }, | |
| }); | |
| if (claimed.count !== 1) { | |
| throw new Error("Invalid or expired code"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/recovery/recoveryService.ts` around lines 138 - 160, The OTP
redeem flow is raceable because you read with prisma.otpChallenge.findFirst then
unconditionally call prisma.otpChallenge.update, allowing two concurrent
requests to both succeed; change the claim to an atomic conditional update:
replace the prisma.otpChallenge.update call with an atomic operation (e.g.,
prisma.otpChallenge.updateMany or an equivalent) that includes usedAt: null (and
still validates expiresAt if desired) in the WHERE clause, then check the
returned count/affectedRows and throw “Invalid or expired code” if it is not
exactly 1 so only the caller that actually claimed the row proceeds to issue API
keys; keep the bcrypt.compare step and only attempt the atomic claim after a
successful password match, and abort if the conditional update did not affect
exactly one row.
tests/recovery.test.ts
Outdated
Comment on lines
+136
to
+145
| it("should reject expired challenge token", async () => { | ||
| const expiredToken = | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VySWQiOiJ0ZXN0IiwicHVycG9zZSI6InNpZ25pbl8yZmEiLCJpYXQiOjE2MDk0NTkyMDAsImV4cCI6MTYwOTQ1OTIwMH0.test"; | ||
|
|
||
| await expect( | ||
| verifyRecoveryOtp({ | ||
| challenge_token: expiredToken, | ||
| code: otpCode, | ||
| }), | ||
| ).rejects.toThrow(); |
There was a problem hiding this comment.
This test is not actually proving the expiry path.
expiredToken is a hand-written string, not one minted by signChallengeToken(), so this only shows that invalid challenge tokens are rejected. If you want expiry coverage here, generate a real challenge token and advance time past its TTL, or sign one with an already-expired exp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/recovery.test.ts` around lines 136 - 145, The test uses a handcrafted
JWT which only proves rejection of malformed tokens; replace it with a
legitimately-signed challenge token so expiry is actually exercised: call
signChallengeToken (or the same token-creation helper used in production) to
create a token with a short TTL or explicitly set exp to a past timestamp, then
advance/freeze time (or set exp) so the token is expired and call
verifyRecoveryOtp({ challenge_token: <that token>, code: otpCode }) expecting a
rejection; reference signChallengeToken and verifyRecoveryOtp to locate where to
create and verify the real expired token.
Member
|
@Yusufolosun cl failed |
Replace DB-coupled recovery tests with isolated unit tests so CI does not depend on a live database for this suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
@Junman140 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/recovery.test.ts (1)
83-85: Strengthen OTP creation assertion in unlock test.Line 83 only checks call count. Asserting key fields (
userId,channel,expiresAt) will better protect against silent security regressions.🔍 Stronger assertion example
- expect(mockPrismaOtpCreate).toHaveBeenCalledTimes(1); + expect(mockPrismaOtpCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + userId: "user-1", + channel: "email", + expiresAt: expect.any(Date), + }), + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/recovery.test.ts` around lines 83 - 85, The test currently only checks mockPrismaOtpCreate call count; update it to assert the OTP creation payload contains the correct userId and channel and a valid future expiresAt. Replace or augment the call-count assertion for mockPrismaOtpCreate with a call-argument check (e.g. expect(mockPrismaOtpCreate).toHaveBeenCalledWith/expect.objectContaining) that verifies userId === "user-1", channel === the expected channel (e.g. "sms" or "email" used in the test), and that expiresAt is a Date/number in the future (compare to Date.now() or use expect.any(Date) plus a > check). Keep mockSignChallengeToken assertion as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/recovery.test.ts`:
- Around line 101-137: Add two regression tests for verifyRecoveryOtp: one that
verifies a previously used OTP is rejected by mocking mockPrismaOtpFindFirst to
return a record with usedAt set (e.g., usedAt: new Date()) and asserting
verifyRecoveryOtp rejects, mockPrismaOtpUpdate is not called, and
mockGenerateApiKey is not called; and another that verifies expired/no-active
challenges are rejected by making mockVerifyChallengeToken return null or throw
(simulating an expired challenge) and asserting verifyRecoveryOtp rejects and
mockGenerateApiKey/mockPrismaOtpUpdate are not called. Ensure you reference the
existing helpers (verifyRecoveryOtp, mockVerifyChallengeToken,
mockPrismaOtpFindFirst, mockPrismaOtpUpdate, mockGenerateApiKey) when adding
these tests.
---
Nitpick comments:
In `@tests/recovery.test.ts`:
- Around line 83-85: The test currently only checks mockPrismaOtpCreate call
count; update it to assert the OTP creation payload contains the correct userId
and channel and a valid future expiresAt. Replace or augment the call-count
assertion for mockPrismaOtpCreate with a call-argument check (e.g.
expect(mockPrismaOtpCreate).toHaveBeenCalledWith/expect.objectContaining) that
verifies userId === "user-1", channel === the expected channel (e.g. "sms" or
"email" used in the test), and that expiresAt is a Date/number in the future
(compare to Date.now() or use expect.any(Date) plus a > check). Keep
mockSignChallengeToken assertion as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+101
to
+137
| describe("verifyRecoveryOtp", () => { | ||
| it("issues API key on valid OTP", async () => { | ||
| mockVerifyChallengeToken.mockReturnValue({ userId: "user-1" }); | ||
| mockPrismaOtpFindFirst.mockResolvedValue({ | ||
| id: "otp-1", | ||
| codeHash: await bcrypt.hash("111111", 10), | ||
| }); | ||
| mockGenerateApiKey.mockResolvedValue("api-key-1"); | ||
|
|
||
| const out = await verifyRecoveryOtp({ | ||
| challenge_token: "challenge-token", | ||
| code: "111111", | ||
| }); | ||
|
|
||
| expect(out).toEqual({ api_key: "api-key-1", user_id: "user-1" }); | ||
| expect(mockPrismaOtpUpdate).toHaveBeenCalledWith({ | ||
| where: { id: "otp-1" }, | ||
| data: { usedAt: expect.any(Date) }, | ||
| }); | ||
| expect(mockGenerateApiKey).toHaveBeenCalledWith("user-1", []); | ||
| }); | ||
|
|
||
| it("rejects invalid OTP", async () => { | ||
| mockVerifyChallengeToken.mockReturnValue({ userId: "user-1" }); | ||
| mockPrismaOtpFindFirst.mockResolvedValue({ | ||
| id: "otp-1", | ||
| codeHash: await bcrypt.hash("111111", 10), | ||
| }); | ||
|
|
||
| await expect( | ||
| verifyRecoveryOtp({ | ||
| challenge_token: "challenge-token", | ||
| code: "222222", | ||
| }), | ||
| ).rejects.toThrow("Invalid code"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing regression tests for OTP single-use and expiry enforcement.
Lines 101-137 only validate success/invalid-code paths. They don’t assert that a used OTP cannot be reused and that expired/no-active challenges are rejected, which are core security guarantees for this flow.
✅ Suggested test additions
+ it("rejects OTP reuse", async () => {
+ mockVerifyChallengeToken.mockReturnValue({ userId: "user-1" });
+ const codeHash = await bcrypt.hash("111111", 10);
+ mockPrismaOtpFindFirst
+ .mockResolvedValueOnce({ id: "otp-1", codeHash }) // first verify
+ .mockResolvedValueOnce(null); // second verify should see no active challenge
+ mockGenerateApiKey.mockResolvedValue("api-key-1");
+
+ await verifyRecoveryOtp({ challenge_token: "challenge-token", code: "111111" });
+ await expect(
+ verifyRecoveryOtp({ challenge_token: "challenge-token", code: "111111" }),
+ ).rejects.toThrow();
+ });
+
+ it("rejects expired/missing active OTP challenge", async () => {
+ mockVerifyChallengeToken.mockReturnValue({ userId: "user-1" });
+ mockPrismaOtpFindFirst.mockResolvedValue(null);
+
+ await expect(
+ verifyRecoveryOtp({ challenge_token: "challenge-token", code: "111111" }),
+ ).rejects.toThrow();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/recovery.test.ts` around lines 101 - 137, Add two regression tests for
verifyRecoveryOtp: one that verifies a previously used OTP is rejected by
mocking mockPrismaOtpFindFirst to return a record with usedAt set (e.g., usedAt:
new Date()) and asserting verifyRecoveryOtp rejects, mockPrismaOtpUpdate is not
called, and mockGenerateApiKey is not called; and another that verifies
expired/no-active challenges are rejected by making mockVerifyChallengeToken
return null or throw (simulating an expired challenge) and asserting
verifyRecoveryOtp rejects and mockGenerateApiKey/mockPrismaOtpUpdate are not
called. Ensure you reference the existing helpers (verifyRecoveryOtp,
mockVerifyChallengeToken, mockPrismaOtpFindFirst, mockPrismaOtpUpdate,
mockGenerateApiKey) when adding these tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #105
The
/recovery/unlockendpoint allowed account recovery with only identifier and passcode, bypassing two-factor authentication. This created a security vulnerability where stolen or weak passcodes enabled account takeover.Root Cause
The
unlockAppfunction immediately issued an API key after verifying identifier and passcode, without requiring a second authentication factor (OTP).Fix Implemented
Implemented two-factor authentication for recovery unlock following the existing 2FA pattern from
authService.ts:Changes:
POST /recovery/unlock: Verify identifier + passcode, generate OTP, return challenge tokenPOST /recovery/unlock/verify: Verify challenge token + OTP, issue API keyOtpChallengetable, RabbitMQ queue, JWT challenge tokensNew Flow:
/recovery/unlock{ challenge_token, channel }/recovery/unlock/verifyTesting Performed
Added comprehensive test suite in
tests/recovery.test.ts:CI Confirmation
All changes follow existing code patterns and TypeScript typing. The implementation:
authService.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests