Skip to content

fix: CSRF-safe test-email endpoint and bounded error log batch delete #289

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
bd73-com merged 4 commits into from
Mar 27, 2026
Merged

fix: CSRF-safe test-email endpoint and bounded error log batch delete #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c290e43
fix: change test-email to POST and bound filter-based error log deletion
claude Mar 26, 2026
39801d5
fix: add ORDER BY and hasMore test for deterministic batch delete pag…
claude Mar 26, 2026
4490f27
fix: add hasMore to restore/finalize endpoints and wire client handler
claude Mar 26, 2026
8506d29
fix: add orderBy to restore/finalize queries, wire hasMore in client
claude Mar 26, 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
19 changes: 14 additions & 5 deletions client/src/pages/AdminErrors.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,34 +155,40 @@ export default function AdminErrors() {
}, []);

const finalizeMutation = useMutation({
mutationFn: async () => {
mutationFn: async (): Promise<{ count: number; hasMore?: boolean }> => {
const res = await fetch("/api/admin/error-logs/finalize", {
method: "POST",
credentials: "include",
});
if (!res.ok) throw new Error("Failed to finalize deletion");
return res.json();
},
Comment on lines +158 to 165
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Promote the new error-log action contract into shared/routes.ts.

{ count, hasMore } is now a shared server/client contract, but it is duplicated inline three times here next to hardcoded endpoint strings. The next server-side change can drift silently. Define these request/response shapes and route constants in shared/routes.ts and import them from @shared/routes.

As per coding guidelines "All types, schemas, and constants shared between client and server must live in the shared/ directory and be imported using the @shared/ path alias. Never duplicate shared types in client or server code." and "Define route constants in the api object in shared/routes.ts with method, path, responses, and optional input. Never hardcode route path strings like '/api/monitors' in server or client code."

Also applies to: 179-186, 245-256

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@client/src/pages/AdminErrors.tsx` around lines 158 - 165, Add a shared
route+contract for the finalize error-log action in shared/routes.ts (e.g., add
an api.admin.errorLogs.finalize entry with method: "POST", path:
"/api/admin/error-logs/finalize" and responses describing { count: number;
hasMore?: boolean }) and export its request/response types; then update
client/src/pages/AdminErrors.tsx to import the route constant and response type
from `@shared/routes` and replace the three hardcoded fetch calls and inline `{
count; hasMore }` types (including the mutationFn) to use the imported
route.path for the fetch URL and the shared response type for typings.

onSuccess: () => {
onSuccess: (data) => {
invalidateAll();
clearSelection();
if (data.hasMore) {
toast({ title: "More entries remain", description: "Some soft-deleted entries were not finalized. Repeat to finalize more." });
}
},
onError: () => {
toast({ title: "Error", description: "Failed to permanently delete entries", variant: "destructive" });
},
});

const restoreMutation = useMutation({
mutationFn: async () => {
mutationFn: async (): Promise<{ count: number; hasMore?: boolean }> => {
const res = await fetch("/api/admin/error-logs/restore", {
method: "POST",
credentials: "include",
});
if (!res.ok) throw new Error("Failed to restore entries");
return res.json();
},
onSuccess: () => {
onSuccess: (data) => {
invalidateAll();
if (data.hasMore) {
toast({ title: "More entries remain", description: "Some soft-deleted entries were not restored. Repeat to restore more." });
}
},
onError: () => {
toast({ title: "Error", description: "Failed to restore entries", variant: "destructive" });
Expand Down Expand Up @@ -247,12 +253,15 @@ export default function AdminErrors() {
const err = await res.json().catch(() => null);
throw new Error(err?.message || "Failed to delete entries");
}
return res.json() as Promise<{ count: number }>;
return res.json() as Promise<{ count: number; hasMore?: boolean }>;
},
onSuccess: (data) => {
invalidateAll();
clearSelection();
showUndoToast(data.count);
if (data.hasMore) {
toast({ title: "More entries remain", description: "Some matching entries were not deleted. Repeat to delete more." });
}
},
onError: (error: Error) => {
toast({ title: "Error", description: error.message, variant: "destructive" });
Expand Down
66 changes: 47 additions & 19 deletions server/routes.deleteErrorLog.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,11 +490,15 @@ describe("POST /api/admin/error-logs/batch-delete", () => {
await ensureRoutes();
vi.clearAllMocks();

// For batch endpoints, the select chain ends at .where() (no .limit/.orderBy)
// so mockSelectWhereFn must resolve to data directly.
// For batch endpoints:
// - ID path: .where() resolves directly (no .limit/.orderBy)
// - Filter path: .where().orderBy().limit(500)
// Default to filter chain; ID-based tests override with mockResolvedValue.
mockLimitFn.mockResolvedValue([]);
mockOrderByFn.mockReturnValue({ limit: mockLimitFn });
mockSelectWhereFn.mockReturnValue({ orderBy: mockOrderByFn, limit: mockLimitFn });
mockSelectFromFn.mockReturnValue({ where: mockSelectWhereFn });
mockDbSelect.mockReturnValue({ from: mockSelectFromFn });
mockSelectWhereFn.mockResolvedValue([]);

mockUpdateWhereFn.mockResolvedValue(undefined);
mockUpdateSetFn.mockReturnValue({ where: mockUpdateWhereFn });
Expand Down Expand Up @@ -607,7 +611,7 @@ describe("POST /api/admin/error-logs/batch-delete", () => {
it("soft-deletes entries matching filters for app owner", async () => {
mockGetUser.mockResolvedValue({ tier: "power" });
mockGetMonitors.mockResolvedValue([]);
mockSelectWhereFn.mockResolvedValue([
mockLimitFn.mockResolvedValue([
{ id: 1, context: null },
{ id: 2, context: null },
{ id: 3, context: null },
Expand All @@ -616,14 +620,14 @@ describe("POST /api/admin/error-logs/batch-delete", () => {
const req = { user: { claims: { sub: "owner-123" } }, body: { filters: { level: "error" } } };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "3 entries deleted", count: 3 });
expect(res._json).toEqual({ message: "3 entries deleted", count: 3, hasMore: false });
expect(mockDbUpdate).toHaveBeenCalled();
});

it("soft-deletes with filter and excludeIds", async () => {
mockGetUser.mockResolvedValue({ tier: "power" });
mockGetMonitors.mockResolvedValue([]);
mockSelectWhereFn.mockResolvedValue([
mockLimitFn.mockResolvedValue([
{ id: 1, context: null },
{ id: 3, context: null },
]);
Expand All @@ -634,7 +638,7 @@ describe("POST /api/admin/error-logs/batch-delete", () => {
};
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "2 entries deleted", count: 2 });
expect(res._json).toEqual({ message: "2 entries deleted", count: 2, hasMore: false });
});

it("rejects empty filters object", async () => {
Expand All @@ -659,15 +663,29 @@ describe("POST /api/admin/error-logs/batch-delete", () => {
it("applies ownership filtering with filters mode", async () => {
mockGetUser.mockResolvedValue({ tier: "power" });
mockGetMonitors.mockResolvedValue([{ id: 10 }]);
mockSelectWhereFn.mockResolvedValue([
mockLimitFn.mockResolvedValue([
{ id: 1, context: { monitorId: 10 } },
{ id: 2, context: null },
]);

const req = { user: { claims: { sub: "not-the-owner" } }, body: { filters: { level: "error" } } };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "1 entries deleted", count: 1 });
expect(res._json).toEqual({ message: "1 entries deleted", count: 1, hasMore: false });
});

it("returns hasMore true when filter query hits the 500-row limit", async () => {
mockGetUser.mockResolvedValue({ tier: "power" });
mockGetMonitors.mockResolvedValue([]);
// Simulate exactly 500 rows returned (the limit)
const entries = Array.from({ length: 500 }, (_, i) => ({ id: i + 1, context: null }));
mockLimitFn.mockResolvedValue(entries);

const req = { user: { claims: { sub: "owner-123" } }, body: { filters: { level: "error" } } };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json.count).toBe(500);
expect(res._json.hasMore).toBe(true);
});

it("rejects filters with only invalid values", async () => {
Expand Down Expand Up @@ -758,9 +776,10 @@ describe("POST /api/admin/error-logs/restore", () => {
await ensureRoutes();
vi.clearAllMocks();

// restore uses .where(...).limit(500), so chain through mockLimitFn
// restore uses .where(...).orderBy(...).limit(500), so chain through mockOrderByFn/mockLimitFn
mockLimitFn.mockResolvedValue([]);
mockSelectWhereFn.mockReturnValue({ limit: mockLimitFn });
mockOrderByFn.mockReturnValue({ limit: mockLimitFn });
mockSelectWhereFn.mockReturnValue({ orderBy: mockOrderByFn, limit: mockLimitFn });
mockSelectFromFn.mockReturnValue({ where: mockSelectWhereFn });
mockDbSelect.mockReturnValue({ from: mockSelectFromFn });

Expand Down Expand Up @@ -795,7 +814,7 @@ describe("POST /api/admin/error-logs/restore", () => {
const req = { user: { claims: { sub: "owner-123" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "2 entries restored", count: 2 });
expect(res._json).toEqual({ message: "2 entries restored", count: 2, hasMore: false });
expect(mockDbUpdate).toHaveBeenCalled();
expect(mockUpdateSetFn).toHaveBeenCalled();
});
Expand All @@ -812,7 +831,7 @@ describe("POST /api/admin/error-logs/restore", () => {
const req = { user: { claims: { sub: "not-the-owner" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "1 entries restored", count: 1 });
expect(res._json).toEqual({ message: "1 entries restored", count: 1, hasMore: false });
});

it("returns count 0 when no soft-deleted entries exist", async () => {
Expand All @@ -823,7 +842,7 @@ describe("POST /api/admin/error-logs/restore", () => {
const req = { user: { claims: { sub: "owner-123" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "0 entries restored", count: 0 });
expect(res._json).toEqual({ message: "0 entries restored", count: 0, hasMore: false });
expect(mockDbUpdate).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -851,9 +870,10 @@ describe("POST /api/admin/error-logs/finalize", () => {
await ensureRoutes();
vi.clearAllMocks();

// finalize uses .where(...).limit(500), so chain through mockLimitFn
// finalize uses .where(...).orderBy(...).limit(500), so chain through mockOrderByFn/mockLimitFn
mockLimitFn.mockResolvedValue([]);
mockSelectWhereFn.mockReturnValue({ limit: mockLimitFn });
mockOrderByFn.mockReturnValue({ limit: mockLimitFn });
mockSelectWhereFn.mockReturnValue({ orderBy: mockOrderByFn, limit: mockLimitFn });
mockSelectFromFn.mockReturnValue({ where: mockSelectWhereFn });
mockDbSelect.mockReturnValue({ from: mockSelectFromFn });

Expand Down Expand Up @@ -887,7 +907,7 @@ describe("POST /api/admin/error-logs/finalize", () => {
const req = { user: { claims: { sub: "owner-123" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "2 entries finalized", count: 2 });
expect(res._json).toEqual({ message: "2 entries finalized", count: 2, hasMore: false });
expect(mockDbDelete).toHaveBeenCalled();
expect(mockDeleteWhereFn).toHaveBeenCalled();
});
Expand All @@ -904,7 +924,7 @@ describe("POST /api/admin/error-logs/finalize", () => {
const req = { user: { claims: { sub: "not-the-owner" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "1 entries finalized", count: 1 });
expect(res._json).toEqual({ message: "1 entries finalized", count: 1, hasMore: false });
});

it("returns count 0 when no soft-deleted entries exist", async () => {
Expand All @@ -915,7 +935,7 @@ describe("POST /api/admin/error-logs/finalize", () => {
const req = { user: { claims: { sub: "owner-123" } }, body: {} };
const res = await callHandler("post", ENDPOINT, req);
expect(res._status).toBe(200);
expect(res._json).toEqual({ message: "0 entries finalized", count: 0 });
expect(res._json).toEqual({ message: "0 entries finalized", count: 0, hasMore: false });
expect(mockDbDelete).not.toHaveBeenCalled();
});

Expand All @@ -932,3 +952,11 @@ describe("POST /api/admin/error-logs/finalize", () => {
errorSpy.mockRestore();
});
});

describe("POST /api/test-email", () => {
it("is registered as POST, not GET", async () => {
await ensureRoutes();
expect(registeredRoutes["post"]?.["/api/test-email"]).toBeDefined();
expect(registeredRoutes["get"]?.["/api/test-email"]).toBeUndefined();
});
});
Comment on lines +956 to +962
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Assert CSRF rejection, not just POST registration.

This only proves the route moved from GET to POST. It will still pass if /api/test-email is later exempted in server/middleware/csrf.ts or the CSRF middleware stops wrapping /api/*, so the security fix itself is untested. Add a request-level test that sends a POST without a valid Origin and expects rejection.

As per coding guidelines "Security-related tests: verify that SSRF, CSRF, auth bypass, and rate limiting tests exist and are thorough."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@server/routes.deleteErrorLog.test.ts` around lines 956 - 962, The test
currently only asserts route registration (ensureRoutes and registeredRoutes for
"post" "/api/test-email") but must also assert actual CSRF enforcement; add a
request-level test that performs a POST to "/api/test-email" through the test
server (the same app used by other integration tests) with no or an invalid
Origin header and expects the CSRF middleware to reject the request (HTTP
403/CSRF error), ensuring the behavior implemented in server/middleware/csrf.ts
remains effective; update the spec to send the POST and assert rejection rather
than relying solely on route registration.

19 changes: 11 additions & 8 deletions server/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { TIER_LIMITS, TAG_LIMITS, TAG_ASSIGNMENT_LIMITS, BROWSERLESS_CAPS, RESEN
import { startScheduler } from "./services/scheduler";
import * as cheerio from "cheerio";
import { getUncachableStripeClient, getStripePublishableKey } from "./stripeClient";
import { sql, desc, eq, and, isNull, isNotNull, inArray, notInArray } from "drizzle-orm";
import { sql, asc, desc, eq, and, isNull, isNotNull, inArray, notInArray } from "drizzle-orm";
import { db } from "./db";
import { sendNotificationEmail } from "./services/email";
import { ErrorLogger } from "./services/logger";
Expand Down Expand Up @@ -217,7 +217,7 @@ export async function registerRoutes(
});

// Test Email Endpoint - verifies Resend email delivery
app.get("/api/test-email", isAuthenticated, emailUpdateRateLimiter, async (req: any, res) => {
app.post("/api/test-email", isAuthenticated, emailUpdateRateLimiter, async (req: any, res) => {
try {
const userId = req.user.claims.sub;
const user = await authStorage.getUser(userId);
Expand Down Expand Up @@ -1614,7 +1614,7 @@ export async function registerRoutes(
conditions.push(notInArray(errorLogs.id, excludeList));
}

const entries = await db.select().from(errorLogs).where(and(...conditions));
const entries = await db.select().from(errorLogs).where(and(...conditions)).orderBy(asc(errorLogs.id)).limit(500);

const authorized = entries.filter((log: any) => {
const ctx = log.context as Record<string, unknown> | null;
Expand All @@ -1627,7 +1627,8 @@ export async function registerRoutes(
const authorizedIds = authorized.map((e: any) => e.id);
await db.update(errorLogs).set({ deletedAt: now }).where(inArray(errorLogs.id, authorizedIds));
}
res.json({ message: `${authorized.length} entries deleted`, count: authorized.length });
const hasMore = entries.length === 500;
res.json({ message: `${authorized.length} entries deleted`, count: authorized.length, hasMore });
}
} catch (error: any) {
console.error("Error batch deleting error logs:", error);
Expand All @@ -1652,7 +1653,7 @@ export async function registerRoutes(
(await storage.getMonitors(userId)).map((m: any) => m.id)
);

const softDeleted = await db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500);
const softDeleted = await db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).orderBy(asc(errorLogs.id)).limit(500);

const authorized = softDeleted.filter((log: any) => {
const ctx = log.context as Record<string, unknown> | null;
Expand All @@ -1665,7 +1666,8 @@ export async function registerRoutes(
const authorizedIds = authorized.map((e: any) => e.id);
await db.update(errorLogs).set({ deletedAt: null }).where(inArray(errorLogs.id, authorizedIds));
}
res.json({ message: `${authorized.length} entries restored`, count: authorized.length });
const hasMore = softDeleted.length === 500;
res.json({ message: `${authorized.length} entries restored`, count: authorized.length, hasMore });
} catch (error: any) {
console.error("Error restoring error logs:", error);
res.status(500).json({ message: "Failed to restore error logs" });
Expand All @@ -1689,7 +1691,7 @@ export async function registerRoutes(
(await storage.getMonitors(userId)).map((m: any) => m.id)
);

const softDeleted = await db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500);
const softDeleted = await db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).orderBy(asc(errorLogs.id)).limit(500);

const authorized = softDeleted.filter((log: any) => {
const ctx = log.context as Record<string, unknown> | null;
Expand All @@ -1702,7 +1704,8 @@ export async function registerRoutes(
const authorizedIds = authorized.map((e: any) => e.id);
await db.delete(errorLogs).where(inArray(errorLogs.id, authorizedIds));
}
res.json({ message: `${authorized.length} entries finalized`, count: authorized.length });
const hasMore = softDeleted.length === 500;
res.json({ message: `${authorized.length} entries finalized`, count: authorized.length, hasMore });
} catch (error: any) {
console.error("Error finalizing error logs:", error);
res.status(500).json({ message: "Failed to finalize error logs" });
Expand Down
Loading