Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
60 changes: 55 additions & 5 deletions src/controllers/recoveryController.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
import { Response, NextFunction } from "express";
import { z } from "zod";
import { Request } from "express";
import { unlockApp } from "../services/recovery/recoveryService";
import {
unlockApp,
verifyRecoveryOtp,
} from "../services/recovery/recoveryService";
import { AppError } from "../middleware/errorHandler";

const unlockAppSchema = z.object({
identifier: z.string().min(1, "identifier is required"), // email or E.164 phone
identifier: z.string().min(1, "identifier is required"),
passcode: z.string().min(1, "passcode is required"),
});

const verifyRecoveryOtpSchema = z.object({
challenge_token: z.string().min(1, "challenge_token is required"),
code: z.string().min(1, "code is required"),
});

/**
* POST /recovery/unlock
* Body: { identifier: string (email or E.164 phone), passcode: string }
* On success returns { api_key, user_id }. No auth header required (user is recovering access).
* Returns { challenge_token, channel }.
*/
export async function postUnlock(
req: Request,
Expand All @@ -26,8 +34,8 @@ export async function postUnlock(
passcode: body.passcode,
});
res.status(200).json({
api_key: result.api_key,
user_id: result.user_id,
challenge_token: result.challenge_token,
channel: result.channel,
});
} catch (e) {
if (e instanceof z.ZodError) {
Expand All @@ -41,6 +49,48 @@ export async function postUnlock(
return next(new AppError(e.message, 401));
if (e.message.includes("identifier"))
return next(new AppError(e.message, 400));
if (e.message === "Recovery channel not configured")
return next(new AppError(e.message, 400));
if (e.message === "OTP delivery unavailable")
return next(new AppError(e.message, 503));
}
next(e);
}
}

/**
* POST /recovery/unlock/verify
* Body: { challenge_token: string, code: string }
* Returns { api_key, user_id }.
*/
export async function postUnlockVerify(
req: Request,
res: Response,
next: NextFunction,
): Promise<void> {
try {
const body = verifyRecoveryOtpSchema.parse(req.body);
const result = await verifyRecoveryOtp({
challenge_token: body.challenge_token,
code: body.code,
});
res.status(200).json({
api_key: result.api_key,
user_id: result.user_id,
});
} catch (e) {
if (e instanceof z.ZodError) {
const msg = e.errors.map((x) => x.message).join("; ");
return next(new AppError(msg, 400));
}
if (e instanceof Error) {
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));
Comment on lines +87 to +93
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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)).

}
next(e);
}
Expand Down
6 changes: 5 additions & 1 deletion src/routes/recoveryRoutes.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { Router } from "express";
import { postUnlock } from "../controllers/recoveryController";
import {
postUnlock,
postUnlockVerify,
} from "../controllers/recoveryController";
import { standardRateLimiter } from "../middleware/rateLimiter";

const router: ReturnType<typeof Router> = Router();

router.use(standardRateLimiter);

router.post("/unlock", postUnlock);
router.post("/unlock/verify", postUnlockVerify);

export default router;
123 changes: 115 additions & 8 deletions src/services/recovery/recoveryService.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,62 @@
/**
* Recovery Tier 1: unlock app via email/phone + passcode.
* On success, issues a new API key and returns it. Does not touch Stellar keys.
* Recovery Tier 1: unlock app via email/phone + passcode + OTP.
* Two-step flow: 1) verify passcode, send OTP; 2) verify OTP, issue API key.
*/
import bcrypt from "bcryptjs";
import { prisma } from "../../config/database";
import { generateApiKey } from "../../middleware/auth";
import { logger } from "../../config/logger";
import { signChallengeToken, verifyChallengeToken } from "../../utils/jwt";
import { getRabbitMQChannel, QUEUES } from "../../config/rabbitmq";

const OTP_EXPIRY_MINUTES = 10;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


export interface UnlockAppParams {
identifier: string; // email or E.164 phone
passcode: string;
}

export interface UnlockAppResult {
challenge_token: string;
channel: "email" | "sms";
}

export interface VerifyRecoveryOtpParams {
challenge_token: string;
code: string;
}

export interface VerifyRecoveryOtpResult {
api_key: string;
user_id: string;
}

function generateOtpCode(): string {
return String(Math.floor(100000 + Math.random() * 900000));
}
Comment on lines +34 to +36
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/services/recovery/recoveryService.ts | head -50

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

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


async function publishOtpToQueue(payload: {
channel: string;
to: string;
code: string;
}): Promise<void> {
try {
const ch = getRabbitMQChannel();
await ch.assertQueue(QUEUES.OTP_SEND, { durable: true });
ch.sendToQueue(QUEUES.OTP_SEND, Buffer.from(JSON.stringify(payload)), {
persistent: true,
});
logger.debug("Recovery OTP published to queue", {
channel: payload.channel,
});
} catch (e) {
logger.error("Failed to publish recovery OTP to RabbitMQ", e);
throw new Error("OTP delivery unavailable");
}
}

/**
* Unlock app: verify identifier + passcode, then issue and return a new API key.
* identifier is email or phone_e164 (E.164).
* Step 1: Verify identifier + passcode, generate OTP, return challenge token.
*/
export async function unlockApp(
params: UnlockAppParams,
Expand All @@ -40,7 +77,12 @@ export async function unlockApp(

const user = await prisma.user.findFirst({
where,
select: { id: true, passcodeHash: true },
select: {
id: true,
passcodeHash: true,
email: true,
phoneE164: true,
},
});
if (!user || !user.passcodeHash) {
logger.warn("Recovery: user not found or no passcode set", {
Expand All @@ -55,10 +97,75 @@ export async function unlockApp(
throw new Error("Invalid passcode");
}

const apiKey = await generateApiKey(user.id, []);
logger.info("Recovery: app unlocked, new key issued", { userId: user.id });
const channel = isEmail ? "email" : "sms";
const to = isEmail ? user.email : user.phoneE164;
if (!to) {
throw new Error("Recovery channel not configured");
}

const code = generateOtpCode();
const codeHash = await bcrypt.hash(code, 10);
await prisma.otpChallenge.create({
data: {
userId: user.id,
codeHash,
channel,
expiresAt: new Date(Date.now() + OTP_EXPIRY_MINUTES * 60 * 1000),
},
});

await publishOtpToQueue({ channel, to, code });

const challenge_token = signChallengeToken(user.id);
logger.info("Recovery: passcode verified, OTP sent", {
userId: user.id,
channel,
});

return { challenge_token, channel };
}

/**
* Step 2: Verify challenge token + OTP code, issue API key.
*/
export async function verifyRecoveryOtp(
params: VerifyRecoveryOtpParams,
): Promise<VerifyRecoveryOtpResult> {
const { challenge_token, code } = params;
const payload = verifyChallengeToken(challenge_token);

const now = new Date();
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 },
});
Comment on lines +138 to +160
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.


const apiKey = await generateApiKey(payload.userId, []);
logger.info("Recovery: OTP verified, new key issued", {
userId: payload.userId,
});

return {
api_key: apiKey,
user_id: user.id,
user_id: payload.userId,
};
}
Loading