fix: add global error handler + try/catch in recover & email-change routes (BUG-112)
All checks were successful
Build & Deploy to Staging / Build & Deploy to Staging (push) Successful in 19m57s

This commit is contained in:
OpenClaw Subagent 2026-03-17 17:10:36 +01:00
parent 2dfb0ac784
commit a3bba8f0d5
6 changed files with 469 additions and 166 deletions

View file

@ -171,3 +171,97 @@ describe("POST /v1/email-change/verify", () => {
);
});
});
describe("POST /v1/email-change - Database failure handling", () => {
it("returns 500 when validateApiKey DB query fails", async () => {
const { queryWithRetry } = await import("../services/db.js");
vi.mocked(queryWithRetry).mockRejectedValue(new Error("Connection pool exhausted"));
const res = await request(app).post("/v1/email-change").send({
apiKey: "df_pro_xxx",
newEmail: "new@example.com"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
it("returns 500 when email existence check fails", async () => {
const { queryWithRetry } = await import("../services/db.js");
let callCount = 0;
vi.mocked(queryWithRetry).mockImplementation((async (sql: string) => {
callCount++;
// First call (validateApiKey) succeeds
if (callCount === 1 && sql.includes("SELECT") && sql.includes("key =")) {
return { rows: [{ key: "df_pro_xxx", email: "old@example.com", tier: "pro" }], rowCount: 1 };
}
// Second call (email check) fails
throw new Error("DB connection lost");
}) as any);
const res = await request(app).post("/v1/email-change").send({
apiKey: "df_pro_xxx",
newEmail: "new@example.com"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
it("returns 500 when createPendingVerification fails", async () => {
const { createPendingVerification } = await import("../services/verification.js");
vi.mocked(createPendingVerification).mockRejectedValue(new Error("DB insert failed"));
const res = await request(app).post("/v1/email-change").send({
apiKey: "df_pro_xxx",
newEmail: "new@example.com"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
});
describe("POST /v1/email-change/verify - Database failure handling", () => {
it("returns 500 when validateApiKey DB query fails", async () => {
const { queryWithRetry } = await import("../services/db.js");
vi.mocked(queryWithRetry).mockRejectedValue(new Error("Connection timeout"));
const res = await request(app).post("/v1/email-change/verify").send({
apiKey: "df_pro_xxx",
newEmail: "new@example.com",
code: "123456"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
it("returns 500 when UPDATE query fails", async () => {
const { queryWithRetry } = await import("../services/db.js");
let callCount = 0;
vi.mocked(queryWithRetry).mockImplementation((async (sql: string) => {
callCount++;
// First call (validateApiKey) succeeds
if (callCount === 1 && sql.includes("SELECT") && sql.includes("key =")) {
return { rows: [{ key: "df_pro_xxx", email: "old@example.com", tier: "pro" }], rowCount: 1 };
}
// Second call (UPDATE) fails
throw new Error("UPDATE failed - constraint violation");
}) as any);
const res = await request(app).post("/v1/email-change/verify").send({
apiKey: "df_pro_xxx",
newEmail: "new@example.com",
code: "123456"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
});

View file

@ -0,0 +1,118 @@
import { describe, it, expect, vi } from "vitest";
import express, { Request, Response, NextFunction } from "express";
import request from "supertest";
const mockLogger = { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() };
vi.mock("../services/logger.js", () => ({
default: mockLogger,
}));
describe("Global error handler", () => {
it("returns 500 JSON for unhandled errors in API routes", async () => {
const app = express();
// Add request ID middleware (like in main app)
app.use((req, _res, next) => {
(req as any).requestId = "test-req-id";
next();
});
// Add a test route that throws an error
app.get("/v1/test-error", (_req: Request, _res: Response) => {
throw new Error("Test unhandled error");
});
// Add global error handler (same as in src/index.ts)
app.use((err: unknown, req: Request, res: Response, _next: NextFunction) => {
const reqId = (req as any).requestId || "unknown";
mockLogger.error({ err, requestId: reqId, method: req.method, path: req.path }, "Unhandled route error");
if (!res.headersSent) {
const isApi = req.path.startsWith("/v1/") || req.path.startsWith("/health");
if (isApi) {
res.status(500).json({ error: "Internal server error" });
} else {
res.status(500).send("Internal server error");
}
}
});
const res = await request(app).get("/v1/test-error");
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
expect(res.headers["content-type"]).toMatch(/json/);
expect(mockLogger.error).toHaveBeenCalledWith(
expect.objectContaining({
method: "GET",
path: "/v1/test-error",
}),
"Unhandled route error"
);
});
it("returns 500 text for unhandled errors in non-API routes", async () => {
const app = express();
// Add request ID middleware
app.use((req, _res, next) => {
(req as any).requestId = "test-req-id";
next();
});
// Add a test route that throws an error
app.get("/test-error-page", (_req: Request, _res: Response) => {
throw new Error("Test page error");
});
// Add global error handler
app.use((err: unknown, req: Request, res: Response, _next: NextFunction) => {
const reqId = (req as any).requestId || "unknown";
mockLogger.error({ err, requestId: reqId, method: req.method, path: req.path }, "Unhandled route error");
if (!res.headersSent) {
const isApi = req.path.startsWith("/v1/") || req.path.startsWith("/health");
if (isApi) {
res.status(500).json({ error: "Internal server error" });
} else {
res.status(500).send("Internal server error");
}
}
});
const res = await request(app).get("/test-error-page");
expect(res.status).toBe(500);
expect(res.text).toBe("Internal server error");
expect(res.headers["content-type"]).toMatch(/text/);
expect(mockLogger.error).toHaveBeenCalled();
});
it("does not send response if headers already sent", async () => {
const app = express();
app.use(express.json());
app.get("/v1/test-headers-sent", (_req: Request, res: Response, next: NextFunction) => {
res.status(200).json({ ok: true });
next(new Error("Too late"));
});
// Add global error handler
app.use((err: unknown, req: Request, res: Response, _next: NextFunction) => {
const reqId = (req as any).requestId || "unknown";
mockLogger.error({ err, requestId: reqId, method: req.method, path: req.path }, "Unhandled route error");
if (!res.headersSent) {
const isApi = req.path.startsWith("/v1/") || req.path.startsWith("/health");
if (isApi) {
res.status(500).json({ error: "Internal server error" });
} else {
res.status(500).send("Internal server error");
}
}
});
const res = await request(app).get("/v1/test-headers-sent");
// Should get the 200 response, error handler does nothing
expect(res.status).toBe(200);
expect(res.body).toEqual({ ok: true });
});
});

View file

@ -2,6 +2,11 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
import express from "express";
import request from "supertest";
vi.mock("../services/db.js");
vi.mock("../services/logger.js", () => ({
default: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() },
}));
let app: express.Express;
beforeEach(async () => {
@ -13,6 +18,7 @@ beforeEach(async () => {
const { createPendingVerification, verifyCode } = await import("../services/verification.js");
const { sendVerificationEmail } = await import("../services/email.js");
const { getAllKeys } = await import("../services/keys.js");
const { queryWithRetry } = await import("../services/db.js");
vi.mocked(createPendingVerification).mockResolvedValue({ email: "test@test.com", code: "654321", createdAt: "", expiresAt: "", attempts: 0 });
vi.mocked(verifyCode).mockResolvedValue({ status: "ok" });
@ -20,6 +26,8 @@ beforeEach(async () => {
vi.mocked(getAllKeys).mockReturnValue([
{ key: "existing-key", tier: "pro" as const, email: "found@test.com", createdAt: "2025-01-01" },
]);
// Default DB mock: returns empty (no user found in DB fallback)
vi.mocked(queryWithRetry).mockResolvedValue({ rows: [], rowCount: 0 } as any);
const { recoverRouter } = await import("../routes/recover.js");
app = express();
@ -94,3 +102,40 @@ describe("POST /recover/verify", () => {
expect(res.body.apiKey).toBeUndefined();
});
});
describe("POST /recover - Database failure handling", () => {
it("returns 500 when queryWithRetry throws in DB fallback", async () => {
const { queryWithRetry } = await import("../services/db.js");
const { getAllKeys } = await import("../services/keys.js");
// Mock cache miss + DB failure
vi.mocked(getAllKeys).mockReturnValue([]);
vi.mocked(queryWithRetry).mockRejectedValue(new Error("Connection pool exhausted"));
const res = await request(app).post("/recover").send({ email: "test@example.com" });
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
});
describe("POST /recover/verify - Database failure handling", () => {
it("returns 500 when queryWithRetry throws in DB fallback", async () => {
const { queryWithRetry } = await import("../services/db.js");
const { getAllKeys } = await import("../services/keys.js");
const { verifyCode } = await import("../services/verification.js");
// Mock cache miss + verifyCode success + DB failure
vi.mocked(getAllKeys).mockReturnValue([]);
vi.mocked(verifyCode).mockResolvedValue({ status: "ok" });
vi.mocked(queryWithRetry).mockRejectedValue(new Error("Unexpected DB schema error"));
const res = await request(app).post("/recover/verify").send({
email: "test@example.com",
code: "123456"
});
expect(res.status).toBe(500);
expect(res.body).toEqual({ error: "Internal server error" });
});
});

View file

@ -1,4 +1,4 @@
import express, { Request, Response } from "express";
import express, { Request, Response, NextFunction } from "express";
import { randomUUID } from "crypto";
import { AuthenticatedRequest } from "./types.js";
import "./types.js"; // Augments Express.Request with requestId, acquirePdfSlot, releasePdfSlot
@ -221,7 +221,29 @@ app.use((req, res) => {
}
});
// Global error handler — must be after all routes
app.use((err: unknown, req: Request, res: Response, _next: NextFunction) => {
const reqId = (req as any).requestId || "unknown";
// Check if this is a JSON parse error from express.json()
if (err instanceof SyntaxError && 'status' in err && (err as any).status === 400 && 'body' in err) {
logger.warn({ err, requestId: reqId, method: req.method, path: req.path }, "Invalid JSON body");
if (!res.headersSent) {
res.status(400).json({ error: "Invalid JSON in request body" });
}
return;
}
logger.error({ err, requestId: reqId, method: req.method, path: req.path }, "Unhandled route error");
if (!res.headersSent) {
const isApi = req.path.startsWith("/v1/") || req.path.startsWith("/health");
if (isApi) {
res.status(500).json({ error: "Internal server error" });
} else {
res.status(500).send("Internal server error");
}
}
});
async function start() {
// Initialize PostgreSQL

View file

@ -71,48 +71,54 @@ async function validateApiKey(apiKey: string) {
* description: Too many attempts
*/
router.post("/", emailChangeLimiter, async (req: Request, res: Response) => {
const { apiKey, newEmail } = req.body || {};
try {
const { apiKey, newEmail } = req.body || {};
if (!apiKey || typeof apiKey !== "string") {
res.status(400).json({ error: "apiKey is required." });
return;
if (!apiKey || typeof apiKey !== "string") {
res.status(400).json({ error: "apiKey is required." });
return;
}
if (!newEmail || typeof newEmail !== "string") {
res.status(400).json({ error: "newEmail is required." });
return;
}
const cleanEmail = newEmail.trim().toLowerCase();
if (!EMAIL_RE.test(cleanEmail)) {
res.status(400).json({ error: "Invalid email format." });
return;
}
const keyRow = await validateApiKey(apiKey);
if (!keyRow) {
res.status(403).json({ error: "Invalid API key." });
return;
}
// Check if email is already taken by another key
const existing = await queryWithRetry(
`SELECT key FROM api_keys WHERE email = $1 AND key != $2`,
[cleanEmail, apiKey]
);
if (existing.rows.length > 0) {
res.status(409).json({ error: "This email is already associated with another account." });
return;
}
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send email change verification");
});
res.json({ status: "verification_sent", message: "A verification code has been sent to your new email address." });
} catch (err: unknown) {
const reqId = (req as any).requestId || "unknown";
logger.error({ err, requestId: reqId }, "Unhandled error in POST /email-change");
res.status(500).json({ error: "Internal server error" });
}
if (!newEmail || typeof newEmail !== "string") {
res.status(400).json({ error: "newEmail is required." });
return;
}
const cleanEmail = newEmail.trim().toLowerCase();
if (!EMAIL_RE.test(cleanEmail)) {
res.status(400).json({ error: "Invalid email format." });
return;
}
const keyRow = await validateApiKey(apiKey);
if (!keyRow) {
res.status(403).json({ error: "Invalid API key." });
return;
}
// Check if email is already taken by another key
const existing = await queryWithRetry(
`SELECT key FROM api_keys WHERE email = $1 AND key != $2`,
[cleanEmail, apiKey]
);
if (existing.rows.length > 0) {
res.status(409).json({ error: "This email is already associated with another account." });
return;
}
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send email change verification");
});
res.json({ status: "verification_sent", message: "A verification code has been sent to your new email address." });
});
/**
@ -161,43 +167,49 @@ router.post("/", emailChangeLimiter, async (req: Request, res: Response) => {
* description: Too many failed attempts
*/
router.post("/verify", async (req: Request, res: Response) => {
const { apiKey, newEmail, code } = req.body || {};
try {
const { apiKey, newEmail, code } = req.body || {};
if (!apiKey || !newEmail || !code) {
res.status(400).json({ error: "apiKey, newEmail, and code are required." });
return;
}
const cleanEmail = newEmail.trim().toLowerCase();
const cleanCode = String(code).trim();
const keyRow = await validateApiKey(apiKey);
if (!keyRow) {
res.status(403).json({ error: "Invalid API key." });
return;
}
const result = await verifyCode(cleanEmail, cleanCode);
switch (result.status) {
case "ok": {
await queryWithRetry(
`UPDATE api_keys SET email = $1 WHERE key = $2`,
[cleanEmail, apiKey]
);
logger.info({ apiKey: apiKey.slice(0, 10) + "...", newEmail: cleanEmail }, "Email changed");
res.json({ status: "ok", newEmail: cleanEmail });
break;
if (!apiKey || !newEmail || !code) {
res.status(400).json({ error: "apiKey, newEmail, and code are required." });
return;
}
case "expired":
res.status(410).json({ error: "Verification code has expired. Please request a new one." });
break;
case "max_attempts":
res.status(429).json({ error: "Too many failed attempts. Please request a new code." });
break;
case "invalid":
res.status(400).json({ error: "Invalid verification code." });
break;
const cleanEmail = newEmail.trim().toLowerCase();
const cleanCode = String(code).trim();
const keyRow = await validateApiKey(apiKey);
if (!keyRow) {
res.status(403).json({ error: "Invalid API key." });
return;
}
const result = await verifyCode(cleanEmail, cleanCode);
switch (result.status) {
case "ok": {
await queryWithRetry(
`UPDATE api_keys SET email = $1 WHERE key = $2`,
[cleanEmail, apiKey]
);
logger.info({ apiKey: apiKey.slice(0, 10) + "...", newEmail: cleanEmail }, "Email changed");
res.json({ status: "ok", newEmail: cleanEmail });
break;
}
case "expired":
res.status(410).json({ error: "Verification code has expired. Please request a new one." });
break;
case "max_attempts":
res.status(429).json({ error: "Too many failed attempts. Please request a new code." });
break;
case "invalid":
res.status(400).json({ error: "Invalid verification code." });
break;
}
} catch (err: unknown) {
const reqId = (req as any).requestId || "unknown";
logger.error({ err, requestId: reqId }, "Unhandled error in POST /email-change/verify");
res.status(500).json({ error: "Internal server error" });
}
});

View file

@ -57,41 +57,47 @@ const recoverLimiter = rateLimit({
* description: Too many recovery attempts
*/
router.post("/", recoverLimiter, async (req: Request, res: Response) => {
const { email } = req.body || {};
try {
const { email } = req.body || {};
if (!email || typeof email !== "string" || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) {
res.status(400).json({ error: "A valid email address is required." });
return;
}
const cleanEmail = email.trim().toLowerCase();
const keys = getAllKeys();
const userKey = keys.find(k => k.email === cleanEmail);
if (!userKey) {
// DB fallback: cache may be stale in multi-replica setups
const dbResult = await queryWithRetry(
"SELECT key FROM api_keys WHERE email = $1 LIMIT 1",
[cleanEmail]
);
if (dbResult.rows.length > 0) {
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send recovery email");
});
logger.info({ email: cleanEmail }, "recover: cache miss, sent recovery via DB fallback");
if (!email || typeof email !== "string" || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email)) {
res.status(400).json({ error: "A valid email address is required." });
return;
}
const cleanEmail = email.trim().toLowerCase();
const keys = getAllKeys();
const userKey = keys.find(k => k.email === cleanEmail);
if (!userKey) {
// DB fallback: cache may be stale in multi-replica setups
const dbResult = await queryWithRetry(
"SELECT key FROM api_keys WHERE email = $1 LIMIT 1",
[cleanEmail]
);
if (dbResult.rows.length > 0) {
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send recovery email");
});
logger.info({ email: cleanEmail }, "recover: cache miss, sent recovery via DB fallback");
}
res.json({ status: "recovery_sent", message: "If an account exists for this email, a verification code has been sent." });
return;
}
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send recovery email");
});
res.json({ status: "recovery_sent", message: "If an account exists for this email, a verification code has been sent." });
return;
} catch (err: unknown) {
const reqId = (req as any).requestId || "unknown";
logger.error({ err, requestId: reqId }, "Unhandled error in POST /recover");
res.status(500).json({ error: "Internal server error" });
}
const pending = await createPendingVerification(cleanEmail);
sendVerificationEmail(cleanEmail, pending.code).catch(err => {
logger.error({ err, email: cleanEmail }, "Failed to send recovery email");
});
res.json({ status: "recovery_sent", message: "If an account exists for this email, a verification code has been sent." });
});
/**
@ -141,66 +147,72 @@ router.post("/", recoverLimiter, async (req: Request, res: Response) => {
* description: Too many failed attempts
*/
router.post("/verify", recoverLimiter, async (req: Request, res: Response) => {
const { email, code } = req.body || {};
try {
const { email, code } = req.body || {};
if (!email || !code) {
res.status(400).json({ error: "Email and code are required." });
return;
}
const cleanEmail = email.trim().toLowerCase();
const cleanCode = String(code).trim();
const result = await verifyCode(cleanEmail, cleanCode);
switch (result.status) {
case "ok": {
const keys = getAllKeys();
let userKey = keys.find(k => k.email === cleanEmail);
// DB fallback: cache may be stale in multi-replica setups
if (!userKey) {
logger.info({ email: cleanEmail }, "recover verify: cache miss, falling back to DB");
const dbResult = await queryWithRetry(
"SELECT key, tier, email, created_at, stripe_customer_id FROM api_keys WHERE email = $1 LIMIT 1",
[cleanEmail]
);
if (dbResult.rows.length > 0) {
const row = dbResult.rows[0];
userKey = {
key: row.key,
tier: row.tier as "free" | "pro",
email: row.email,
createdAt: row.created_at instanceof Date ? row.created_at.toISOString() : row.created_at,
stripeCustomerId: row.stripe_customer_id || undefined,
};
}
}
if (userKey) {
res.json({
status: "recovered",
apiKey: userKey.key,
tier: userKey.tier,
message: "Your API key has been recovered. Save it securely — it is shown only once.",
});
} else {
res.json({
status: "recovered",
message: "No API key found for this email.",
});
}
break;
if (!email || !code) {
res.status(400).json({ error: "Email and code are required." });
return;
}
case "expired":
res.status(410).json({ error: "Verification code has expired. Please request a new one." });
break;
case "max_attempts":
res.status(429).json({ error: "Too many failed attempts. Please request a new code." });
break;
case "invalid":
res.status(400).json({ error: "Invalid verification code." });
break;
const cleanEmail = email.trim().toLowerCase();
const cleanCode = String(code).trim();
const result = await verifyCode(cleanEmail, cleanCode);
switch (result.status) {
case "ok": {
const keys = getAllKeys();
let userKey = keys.find(k => k.email === cleanEmail);
// DB fallback: cache may be stale in multi-replica setups
if (!userKey) {
logger.info({ email: cleanEmail }, "recover verify: cache miss, falling back to DB");
const dbResult = await queryWithRetry(
"SELECT key, tier, email, created_at, stripe_customer_id FROM api_keys WHERE email = $1 LIMIT 1",
[cleanEmail]
);
if (dbResult.rows.length > 0) {
const row = dbResult.rows[0];
userKey = {
key: row.key,
tier: row.tier as "free" | "pro",
email: row.email,
createdAt: row.created_at instanceof Date ? row.created_at.toISOString() : row.created_at,
stripeCustomerId: row.stripe_customer_id || undefined,
};
}
}
if (userKey) {
res.json({
status: "recovered",
apiKey: userKey.key,
tier: userKey.tier,
message: "Your API key has been recovered. Save it securely — it is shown only once.",
});
} else {
res.json({
status: "recovered",
message: "No API key found for this email.",
});
}
break;
}
case "expired":
res.status(410).json({ error: "Verification code has expired. Please request a new one." });
break;
case "max_attempts":
res.status(429).json({ error: "Too many failed attempts. Please request a new code." });
break;
case "invalid":
res.status(400).json({ error: "Invalid verification code." });
break;
}
} catch (err: unknown) {
const reqId = (req as any).requestId || "unknown";
logger.error({ err, requestId: reqId }, "Unhandled error in POST /recover/verify");
res.status(500).json({ error: "Internal server error" });
}
});