fix: CSRF-safe test-email endpoint and bounded error log batch delete#289
fix: CSRF-safe test-email endpoint and bounded error log batch delete#289
Conversation
- Change /api/test-email from GET to POST to prevent CSRF and browser prefetch triggering (fixes #284) - Add .limit(500) to filter-based batch delete query in /api/admin/error-logs/batch-delete to prevent unbounded memory usage, with hasMore flag for client pagination (fixes #283) https://claude.ai/code/session_01P8grWDU2NXyfvqY3cxj18b
…ination - Add .orderBy(asc(errorLogs.id)) before .limit(500) for deterministic pagination when client loops on hasMore - Add test for hasMore=true when exactly 500 entries are returned - Add test verifying /api/test-email is registered as POST not GET https://claude.ai/code/session_01P8grWDU2NXyfvqY3cxj18b
- Add hasMore flag to restore and finalize endpoint responses for consistency with batch-delete - Update client batch-delete type to include hasMore and show toast when more entries remain to be deleted - Update test expectations for restore/finalize responses https://claude.ai/code/session_01P8grWDU2NXyfvqY3cxj18b
📝 WalkthroughWalkthroughAdded deterministic ordering and bounded pagination to admin error-log endpoints (batch-delete, restore, finalize), returning a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Client UI
participant Server as Server API
participant DB as Database
User->>Client: Trigger admin action (batch-delete / restore / finalize)
Client->>Server: POST /api/admin/error-logs/* (filters or ids)
Server->>DB: SELECT ... WHERE ... ORDER BY id ASC LIMIT 500
DB-->>Server: rows (n ≤ 500)
alt n == 500
Server->>Server: hasMore = true
else
Server->>Server: hasMore = false
end
Server-->>Client: { count, hasMore }
alt hasMore == true
Client->>User: Show toast: "More matching entries remain — repeat action"
else
Client->>User: Show toast: "Action complete"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/routes.ts (2)
1656-1656:⚠️ Potential issue | 🟡 MinorAdd deterministic ordering to restore query.
The batch-delete endpoint correctly uses
.orderBy(asc(errorLogs.id))for deterministic pagination, but the restore endpoint at line 1656 only uses.limit(500)without ordering.If
hasMore: trueand the user triggers restore again, the database may return rows in a different order, potentially causing entries to be skipped or processed twice.Suggested fix
- 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);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes.ts` at line 1656, The restore query uses db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500) without a deterministic order; modify the query that assigns softDeleted to include a stable ordering (e.g., .orderBy(asc(errorLogs.id))) before .limit(500) so pagination/hasMore behaves deterministically and rows aren't skipped or duplicated when restore is re-run.
1694-1694:⚠️ Potential issue | 🟡 MinorAdd deterministic ordering to finalize query.
Same issue as restore — the finalize endpoint should include
.orderBy(asc(errorLogs.id))for deterministic pagination whenhasMore: true.Suggested fix
- 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);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes.ts` at line 1694, The finalize endpoint's query that sets softDeleted uses a non-deterministic selection; update the query that builds softDeleted (the call to db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500)) to include deterministic ordering by adding .orderBy(asc(errorLogs.id)) so results are consistently paginated when hasMore is true; ensure the asc symbol is imported/available in the same scope if required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/pages/AdminErrors.tsx`:
- Around line 250-258: finalizeMutation and restoreMutation currently ignore the
backend's hasMore field whereas batchDeleteMutation handles it; update the
onSuccess handlers for finalizeMutation and restoreMutation to mirror
batchDeleteMutation by reading the response as { count: number; hasMore?:
boolean }, calling invalidateAll(), clearSelection(), showUndoToast(data.count)
and if (data.hasMore) call toast({ title: "More entries remain", description:
"Some matching entries were not processed. Repeat to process more." }) so the
user is informed when additional items remain.
---
Outside diff comments:
In `@server/routes.ts`:
- Line 1656: The restore query uses
db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500)
without a deterministic order; modify the query that assigns softDeleted to
include a stable ordering (e.g., .orderBy(asc(errorLogs.id))) before .limit(500)
so pagination/hasMore behaves deterministically and rows aren't skipped or
duplicated when restore is re-run.
- Line 1694: The finalize endpoint's query that sets softDeleted uses a
non-deterministic selection; update the query that builds softDeleted (the call
to db.select().from(errorLogs).where(isNotNull(errorLogs.deletedAt)).limit(500))
to include deterministic ordering by adding .orderBy(asc(errorLogs.id)) so
results are consistently paginated when hasMore is true; ensure the asc symbol
is imported/available in the same scope if required.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6fba32c-6539-4508-8a03-1c9452281b1c
📒 Files selected for processing (3)
client/src/pages/AdminErrors.tsxserver/routes.deleteErrorLog.test.tsserver/routes.ts
Address CodeRabbit review feedback: - Add .orderBy(asc(errorLogs.id)) to restore and finalize queries for deterministic pagination (matching batch-delete pattern) - Wire hasMore handling in finalizeMutation and restoreMutation client handlers so users are notified when more entries remain - Update test mock chains for restore/finalize to match new query shape https://claude.ai/code/session_01P8grWDU2NXyfvqY3cxj18b
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes.ts (1)
1617-1631:⚠️ Potential issue | 🟠 MajorApply ownership scoping before the 500-row limit.
These queries fetch the first 500 matching rows globally and only then filter by
monitorIdin memory. If a caller’s first authorized row is after 500 unauthorized rows, every retry reads the same untouched batch, socountstays0andhasMorestaystrueforever. That makes later authorized rows unreachable in batch-delete, restore, and finalize.This needs to move behind
IStorageand scope the SQL to authorized rows beforeorderBy(...).limit(500). The regression tests should also cover the non-owner starvation case.As per coding guidelines "Never put database queries or Drizzle ORM calls directly in route handlers — all database access must go through methods on the
IStorageinterface implemented inserver/storage.ts."Also applies to: 1656-1670, 1694-1708
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes.ts` around lines 1617 - 1631, The route handler is performing db.select(...).from(errorLogs)...limit(500) then filtering by monitorId in memory which can starve authorized rows; instead add a storage-layer method on IStorage (implemented in server/storage.ts) such as fetchAndDeleteAuthorizedErrorLogsBatch or getAuthorizedErrorLogIdsForDeletion that accepts userMonitorIds:Set<number> and isAppOwner:boolean plus limit/order, and move the ownership scoping into the SQL WHERE (e.g., WHERE (context->>'monitorId'::int IN (:userMonitorIds) OR :isAppOwner) or build a condition using inArray(errorLogs.id, ...) / json extraction depending on schema) so the orderBy(...).limit(500) is applied after filtering to authorized rows; update the route to call that IStorage method (use errorLogs, authorizedIds, deletedAt/now logic returned by storage) and add regression tests that simulate non-owner with authorized rows beyond 500 to ensure no starvation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/pages/AdminErrors.tsx`:
- Around line 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.
In `@server/routes.deleteErrorLog.test.ts`:
- Around line 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.
---
Outside diff comments:
In `@server/routes.ts`:
- Around line 1617-1631: The route handler is performing
db.select(...).from(errorLogs)...limit(500) then filtering by monitorId in
memory which can starve authorized rows; instead add a storage-layer method on
IStorage (implemented in server/storage.ts) such as
fetchAndDeleteAuthorizedErrorLogsBatch or getAuthorizedErrorLogIdsForDeletion
that accepts userMonitorIds:Set<number> and isAppOwner:boolean plus limit/order,
and move the ownership scoping into the SQL WHERE (e.g., WHERE
(context->>'monitorId'::int IN (:userMonitorIds) OR :isAppOwner) or build a
condition using inArray(errorLogs.id, ...) / json extraction depending on
schema) so the orderBy(...).limit(500) is applied after filtering to authorized
rows; update the route to call that IStorage method (use errorLogs,
authorizedIds, deletedAt/now logic returned by storage) and add regression tests
that simulate non-owner with authorized rows beyond 500 to ensure no starvation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1761f51e-8d1a-490f-a81e-3f4b52888b20
📒 Files selected for processing (3)
client/src/pages/AdminErrors.tsxserver/routes.deleteErrorLog.test.tsserver/routes.ts
| 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(); | ||
| }, |
There was a problem hiding this comment.
🛠️ 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.
| 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ 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.
2 participants
Summary
Fixes two reported bugs: the
/api/test-emailendpoint used HTTP GET for a state-changing operation (sending email), making it vulnerable to CSRF via image tags and browser prefetching (#284). The filter-based batch delete on/api/admin/error-logs/batch-deleteloaded all matching rows into memory with no upper bound, risking OOM on large tables (#283).Changes
Security — test-email endpoint (fixes #284)
app.get("/api/test-email", ...)toapp.post(...)inserver/routes.tsPerformance — bounded batch delete (fixes #283)
.orderBy(asc(errorLogs.id)).limit(500)to the filter-based query in the batch-delete endpointhasMoreboolean to the response so the client knows if additional requests are neededhasMorepattern to the restore and finalize endpoints for API consistencyAdminErrors.tsx) to readhasMoreand show a toast when more entries remainTests
routes.deleteErrorLog.test.tsto match the new.where().orderBy().limit()query shapehasMore: truewhen exactly 500 entries are returned/api/test-emailis registered as POST, not GEThasMoreHow to test
GET /api/test-emailreturns 404 (or method not allowed) andPOST /api/test-emailworks with a valid session{ hasMore: true }and only 500 are processednpm run check && npm run test— all 1834 tests passCloses #284
Closes #283
https://claude.ai/code/session_01P8grWDU2NXyfvqY3cxj18b
Summary by CodeRabbit
New Features
Bug Fixes