Skip to content

fix(api): follow-up correctness issues in request.ts identified by PR #434 review #439

@mcmxcdev

Description

@mcmxcdev

Context

These issues were identified by CodeRabbit in the outside-diff-range comments of PR #434 and could not be posted inline. All findings are in packages/api/src/router/request.ts.


1. Hardcoded reviewedBy: "email" literal corrupts audit data

Lines ~991–999 and ~1016–1020

reviewedBy: "email" is hardcoded when calling applyDeleteRequest and applyUpdateRequest, even though the real reviewer email is already resolved earlier in the handler. This corrupts reviewer attribution in audit records.

-  reviewedBy: "email",
+  reviewedBy,

2. input.submittedBy used instead of validated submittedBy

Lines ~654–663

The delete-request insert persists input.submittedBy rather than the already-normalized submittedBy variable resolved from the auth context. In authenticated flows this can persist the wrong value or undefined.

-  submittedBy: input.submittedBy,
+  submittedBy,

3. Multi-step mutations not wrapped in a DB transaction

Lines ~1097–1113 and ~1143–1347

applyDeleteRequest and applyUpdateRequest perform multiple dependent writes (delete, update, insert) without a transaction. A mid-flight failure leaves the database in an inconsistent state.

Both functions should be wrapped in ctx.db.transaction(async (tx) => { ... }) with all ctx.db.* calls replaced by tx.*.


4. Raw payload logged via JSON.stringify, leaking sensitive data

Lines ~1200 and ~1279

console.log("inserting ao", JSON.stringify(updateRequest));
console.log("inserting event", JSON.stringify(updateRequest));

Full request payloads (including user-identifying fields) are emitted to server logs. Replace with structured logging of only non-sensitive metadata:

console.log("inserting ao", { regionId: updateRequest.regionId, aoName: updateRequest.aoName });
console.log("inserting event", { eventId: updateRequest.eventId, regionId: updateRequest.regionId });

5. INTERNAL_SERVER_ERROR thrown for missing event update target

Lines ~1272–1275

When .where(eq(schema.events.id, updateRequest.eventId)) returns no row, the error thrown is INTERNAL_SERVER_ERROR. This should be NOT_FOUND since the resource is simply absent, not an internal fault — consistent with the intent of PR #434.

-  throw new ORPCError("INTERNAL_SERVER_ERROR", { message: "Failed to update event" });
+  throw new ORPCError("NOT_FOUND", { message: "Event not found" });

Acceptance criteria

  • reviewedBy variable (not the "email" literal) is passed to applyDeleteRequest and applyUpdateRequest
  • submittedBy (validated) is used in the delete-request insert
  • applyDeleteRequest and applyUpdateRequest execute their writes inside a single ctx.db.transaction
  • console.log(JSON.stringify(...)) calls replaced with safe, structured logging
  • Missing event update target throws NOT_FOUND instead of INTERNAL_SERVER_ERROR

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status
    Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions