feat(audit): add global backend audit trail - #114
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe backend adds a typed append-only audit contract with schema support, request/run correlation, validated structured details, transactional required writes, and best-effort denial logging across authentication, claims, items, uploads, reports, profiles, notifications, and scheduled jobs. ChangesAudit contract and persistence
Application workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as backend route
participant Prisma
participant Audit as auditLog utilities
Client->>Route: submit mutation or authentication request
Route->>Prisma: execute database mutation
Route->>Audit: create required or best-effort audit event
Audit->>Prisma: persist correlated audit record
Route-->>Client: return route response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/jobs/expireRetainedItems.ts (1)
48-63: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPrevent concurrent execution race conditions.
If multiple instances of this backend run simultaneously (e.g., in a multi-pod deployment), this cron job will execute on all instances concurrently. Because Prisma's
findManydoes not acquire row locks, all instances will fetch the exact sameeligibleItemsand wait on each other duringupdateMany. Even though the database will serialize theupdateManycalls (meaning only one instance will actually modify the rows), all instances will continue executing downstream logic—writing duplicate audit logs and fanning out duplicate notifications for those same items.Acquire a row lock on the candidates using
$queryRawat the start of the transaction to guarantee only one instance processes them, similar to the approach taken incleanupUnverifiedUsers.ts.🔒️ Proposed fix to add row locking
Ensure you import
Prismaat the top of the file:import { ClaimStatus, ItemStatus, MatchStatus, NotificationType, + Prisma, } from '`@prisma/client`';Then, add the lock before fetching
eligibleItems:const expiredCount = await prisma.$transaction(async (tx) => { + // Acquire row locks to prevent duplicate processing by concurrent cron instances + const lockedItems = await tx.$queryRaw<Array<{ itemId: string }>>(Prisma.sql` + SELECT "item_id" AS "itemId" + FROM "item" + WHERE "item_id" IN (${Prisma.join(candidateItemIds)}) + FOR UPDATE SKIP LOCKED + `); + + if (lockedItems.length === 0) { + return 0; + } + + const lockedItemIds = lockedItems.map((i) => i.itemId); + const eligibleItems = await tx.item.findMany({ where: { - itemId: { in: candidateItemIds }, + itemId: { in: lockedItemIds }, status: ItemStatus.stored, retentionExpiryDate: { lte: today },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/jobs/expireRetainedItems.ts` around lines 48 - 63, Update the transaction in the expire-retained-items job to acquire row-level locks on the candidate items with tx.$queryRaw before querying eligibleItems, following the locking approach used by cleanupUnverifiedUsers.ts. Import Prisma as needed for the raw query, and ensure the lock targets candidateItemIds so concurrent job instances cannot process the same rows or emit duplicate downstream effects.
🧹 Nitpick comments (10)
backend/tests/expireRetainedItems.test.ts (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude missing
statusin mocked claim data.The query in
expireDueItemsexplicitly selects thestatusfield, which is later passed into the audit log payload aspreviousStatus. To ensure the test accurately reflects runtime behavior and validates correctly iftoAuditDatastrictly enforces schema schemas, please include a valid status in this mock object.🛠️ Proposed fix
claim: { findMany: vi.fn().mockResolvedValue([ { claimId: '550e8400-e29b-41d4-a716-446655440000', studentId: 'student-1', itemName: 'iPhone 15', + status: 'submitted', }, ]), updateMany: vi.fn(), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/expireRetainedItems.test.ts` around lines 27 - 36, Update the mocked claim returned by claim.findMany in expireDueItems tests to include a valid status value, ensuring the fixture matches the query result and supports the audit payload’s previousStatus field.backend/src/routes/claims.ts (1)
548-614: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBatch the audit-log inserts in these transactions
backend/src/routes/claims.ts#L548-L614: replace the individualwriteAuditLog(..., tx)calls with a singlewriteAuditLogs([...], tx).backend/src/routes/claims.ts#L773-L805: do the same for the claim-created and notification-created logs.
Promise.allhere doesn’t add parallelism inside the interactive transaction, and batching matches the existingitems.tspattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/claims.ts` around lines 548 - 614, Replace the Promise.all of individual writeAuditLog calls in backend/src/routes/claims.ts lines 548-614 with one writeAuditLogs([...], tx) call containing the same entries. Apply the same batching change to the claim-created and notification-created audit logs at backend/src/routes/claims.ts lines 773-805, preserving all payloads and transaction usage.backend/src/routes/photoSessions.ts (1)
285-297: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
photo_upload_authorizedentity fields point at the session, not the uploaded image.
entityType: 'photo_session'/entityId: session.sessionIddescribes the session, while the actual subject of this event (the reserved image) is buried insidedetails.imageId. The siblingphoto_image_registeredevent a few lines later correctly models this asentityType: 'photo_session_image'/entityId: <imageId>. Keeping the taxonomy consistent makes the audit trail queryable by entity.♻️ Align entity fields with the image being authorized
await writeAuditLogBestEffort({ actorType: 'anonymous', action: 'photo_upload_authorized', - entityType: 'photo_session', - entityId: session.sessionId, + entityType: 'photo_session_image', + entityId: placeholder.imageId, outcome: 'success', details: { - imageId: placeholder.imageId, + sessionId: session.sessionId, contentType, sizeCategory: getUploadSizeCategory(fileSizeKb), }, ...auditContextFromRequest(req), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/photoSessions.ts` around lines 285 - 297, Update the photo_upload_authorized audit entry to identify the reserved image: use entityType 'photo_session_image' and placeholder.imageId as entityId, matching the photo_image_registered event while leaving the existing details and audit context unchanged.backend/tests/reportLinks.integration.test.ts (1)
179-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew audit events added to routes lack matching test assertions.
Both files mock the audit-log module so the new
writeAuditLog/writeAuditLogBestEffortcalls don't hit a real DB, but neither adds assertions for the new events themselves — unlikebackend/tests/uploads.integration.test.ts, which does assert on its newupload_authorizedcall and its payload shape.
backend/tests/reportLinks.integration.test.ts#L179-L180: extend assertions in the submit test to also verifyreport_created,item_created,report_link_consumed, andnotification_createdare called with the expectedentityId/details/txfor each.backend/tests/photoSessions.integration.test.ts#L68-L76: add assertions verifyingphoto_session_created,photo_upload_authorized,photo_image_registered,photo_session_access_denied, andphoto_image_deletedare invoked with the expected action/entity/outcome for their respective test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/reportLinks.integration.test.ts` around lines 179 - 180, The audit-log mocks lack assertions for the newly emitted events. In backend/tests/reportLinks.integration.test.ts:179-180, extend the submit test assertions for report_created, item_created, report_link_consumed, and notification_created, including expected entityId, details, and tx; in backend/tests/photoSessions.integration.test.ts:68-76, add case-specific assertions for photo_session_created, photo_upload_authorized, photo_image_registered, photo_session_access_denied, and photo_image_deleted, including expected action, entity, and outcome.backend/src/routes/reportLinks.ts (1)
700-759: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid
Promise.allfor these audit writes. Prisma serializestxcalls on one connection, so this adds no throughput and makes the transaction behavior harder to reason about. Use sequential awaits here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/reportLinks.ts` around lines 700 - 759, Replace the Promise.all wrapper around the four writeAuditLog calls in the report-link transaction with sequential awaits, preserving their existing order, arguments, and shared tx client.backend/src/utils/auditLog.ts (1)
12-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing Prisma's generated enums instead of duplicated literal unions.
actorType/outcomere-declareAuditActorType/AuditOutcomeas hand-written string literal unions. Importing the Prisma-generated enum types would keep this contract in sync by construction if the schema enums ever change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/utils/auditLog.ts` around lines 12 - 24, Update the AuditLogParams interface to use Prisma’s generated AuditActorType and AuditOutcome enum types for actorType and outcome instead of duplicating string literal unions. Import and reuse those generated enum types so the audit log contract remains synchronized with the schema.backend/prisma/migrations/20260720180000_harden_audit_log/migration.sql (2)
20-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack removal of the staged-rollout compatibility trigger.
The comment on lines 20-21 correctly notes this trigger only exists to keep old application code insert-compatible during rollout. There's no follow-up migration or tracked task here to drop
audit_log_fill_contract_defaults_trigger/function once all writers supply the new fields explicitly, so it risks becoming permanent dead logic that silently masks incomplete contract adoption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prisma/migrations/20260720180000_harden_audit_log/migration.sql` around lines 20 - 44, Track and schedule removal of the staged-rollout compatibility objects created by audit_log_fill_contract_defaults and audit_log_fill_contract_defaults_trigger once all writers explicitly provide actor_type and outcome. Add a follow-up migration or tracked cleanup task that drops the trigger and function, preserving their temporary rollout-only scope.
45-54: 🚀 Performance & Scalability | 🔵 TrivialLocking risk on an already-live
audit_logtable.Static analysis correctly flags production risk:
SET NOT NULLon lines 46-47 requires a full-table validation scan holding a blocking lock, and the fourCREATE INDEXstatements (51-54) run withoutCONCURRENTLY, blocking writes for their duration. Since this migration adds columns to an existing, presumably actively-writtenaudit_logtable, this could cause a noticeable stall on deploy. NoteCONCURRENTLYcan't be used inside a transaction anyway (which is how Prisma runs migrations by default), so mitigating this requires splitting the rollout into separate non-transactional migration steps (e.g., add nullable columns + backfill in one deploy, addNOT NULLvia a validatedCHECKconstraint in a later deploy, create indexes concurrently outside the transaction).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/prisma/migrations/20260720180000_harden_audit_log/migration.sql` around lines 45 - 54, The migration’s constraint validation and non-concurrent index creation can block writes on the live audit_log table. Split this rollout into separate non-transactional migration steps: safely add and backfill nullable fields, validate required values before enforcing NOT NULL, and create each index with concurrent creation outside Prisma’s default transaction; preserve the foreign-key removal in the appropriate step.Source: Linters/SAST tools
backend/src/middleware/requireRole.ts (1)
14-19: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUnauthenticated (401) denials aren't audited.
Only the role-mismatch (403) branch writes an
authorization_deniedaudit event; the unauthenticated branch returns 401 without any audit record. If the intent is a comprehensive audit trail for protected-route access denials, this may be a coverage gap; if it's intentional (to avoid logging noise from unauthenticated probing), consider noting that explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/middleware/requireRole.ts` around lines 14 - 19, Update the unauthenticated branch in requireRole to record an authorization_denied audit event before returning the 401 response, matching the existing role-mismatch audit behavior while preserving the current status and response payload.backend/src/utils/auditEvents.ts (1)
1-213: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
policymetadata isn't enforced anywhere.
policy: 'required' | 'best_effort'is defined per action but never checked against which write function (writeAuditLog/writeAuditLogsvswriteAuditLogBestEffort) actually persists it. A future call site could log arequiredaction viawriteAuditLogBestEffort, silently downgrading a compliance-critical event to best-effort with no compile-time or runtime signal. Consider a cheap runtime guard inwriteAuditLogBestEffort(see suggested diff onbackend/src/utils/auditLog.ts) that asserts the action's declared policy actually matches the function being used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/utils/auditEvents.ts` around lines 1 - 213, Enforce each action’s declared policy when using the best-effort audit writer: update writeAuditLogBestEffort to resolve the action metadata from auditEvents and reject or assert when its policy is required, while allowing only best_effort actions. Keep writeAuditLog and writeAuditLogs unchanged and preserve normal persistence for correctly matched calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/auth.ts`:
- Around line 226-246: Review writeAuditLog and its audit_log dependencies for
schema-stable, transaction-safe behavior at each listed auth mutation site.
Ensure required audit inserts do not rely on soft dependencies or failure-prone
side effects, while keeping writeAuditLog(..., tx) in the existing
prisma.$transaction so audit rows commit atomically with the primary mutation.
In `@backend/src/routes/photoSessions.ts`:
- Around line 384-397: Change the audit call in the photo image registration
handler from required `writeAuditLog` to the established best-effort logging
pattern used for `photo_upload_authorized` above. Preserve the existing audit
payload and ensure audit persistence failures are handled without propagating
through `next(err)` or turning the successful upload confirmation into a 500.
- Around line 449-458: Update the photoUploadSession.findUnique select used by
the access-denial path to include sessionId, then pass the selected sessionId as
entityId in the photo_session_access_denied audit log instead of null. Preserve
the existing ownership-mismatch handling and audit context.
In `@backend/src/routes/users.ts`:
- Around line 229-259: Update the studentNumber entry in changedFields to
compare the requested value with existing.studentNumber, rather than checking
only studentNumber !== undefined. Preserve the existing undefined guard and use
a bigint-aware comparison consistent with the types used by
existing.studentNumber.
In `@backend/src/utils/auditLog.ts`:
- Around line 137-182: Move the toAuditData(params) call inside the try block in
writeAuditLogBestEffort so contract-validation errors from assertSafeDetails,
assertRequiredDetails, or entity-type validation are caught and handled like
persistence failures. Preserve best-effort behavior by logging the failure and
resolving without rejecting; update the existing auditLog.test.ts expectation
that invalid contracts reject, while retaining coverage that detects invalid
contracts during tests.
---
Outside diff comments:
In `@backend/src/jobs/expireRetainedItems.ts`:
- Around line 48-63: Update the transaction in the expire-retained-items job to
acquire row-level locks on the candidate items with tx.$queryRaw before querying
eligibleItems, following the locking approach used by cleanupUnverifiedUsers.ts.
Import Prisma as needed for the raw query, and ensure the lock targets
candidateItemIds so concurrent job instances cannot process the same rows or
emit duplicate downstream effects.
---
Nitpick comments:
In `@backend/prisma/migrations/20260720180000_harden_audit_log/migration.sql`:
- Around line 20-44: Track and schedule removal of the staged-rollout
compatibility objects created by audit_log_fill_contract_defaults and
audit_log_fill_contract_defaults_trigger once all writers explicitly provide
actor_type and outcome. Add a follow-up migration or tracked cleanup task that
drops the trigger and function, preserving their temporary rollout-only scope.
- Around line 45-54: The migration’s constraint validation and non-concurrent
index creation can block writes on the live audit_log table. Split this rollout
into separate non-transactional migration steps: safely add and backfill
nullable fields, validate required values before enforcing NOT NULL, and create
each index with concurrent creation outside Prisma’s default transaction;
preserve the foreign-key removal in the appropriate step.
In `@backend/src/middleware/requireRole.ts`:
- Around line 14-19: Update the unauthenticated branch in requireRole to record
an authorization_denied audit event before returning the 401 response, matching
the existing role-mismatch audit behavior while preserving the current status
and response payload.
In `@backend/src/routes/claims.ts`:
- Around line 548-614: Replace the Promise.all of individual writeAuditLog calls
in backend/src/routes/claims.ts lines 548-614 with one writeAuditLogs([...], tx)
call containing the same entries. Apply the same batching change to the
claim-created and notification-created audit logs at
backend/src/routes/claims.ts lines 773-805, preserving all payloads and
transaction usage.
In `@backend/src/routes/photoSessions.ts`:
- Around line 285-297: Update the photo_upload_authorized audit entry to
identify the reserved image: use entityType 'photo_session_image' and
placeholder.imageId as entityId, matching the photo_image_registered event while
leaving the existing details and audit context unchanged.
In `@backend/src/routes/reportLinks.ts`:
- Around line 700-759: Replace the Promise.all wrapper around the four
writeAuditLog calls in the report-link transaction with sequential awaits,
preserving their existing order, arguments, and shared tx client.
In `@backend/src/utils/auditEvents.ts`:
- Around line 1-213: Enforce each action’s declared policy when using the
best-effort audit writer: update writeAuditLogBestEffort to resolve the action
metadata from auditEvents and reject or assert when its policy is required,
while allowing only best_effort actions. Keep writeAuditLog and writeAuditLogs
unchanged and preserve normal persistence for correctly matched calls.
In `@backend/src/utils/auditLog.ts`:
- Around line 12-24: Update the AuditLogParams interface to use Prisma’s
generated AuditActorType and AuditOutcome enum types for actorType and outcome
instead of duplicating string literal unions. Import and reuse those generated
enum types so the audit log contract remains synchronized with the schema.
In `@backend/tests/expireRetainedItems.test.ts`:
- Around line 27-36: Update the mocked claim returned by claim.findMany in
expireDueItems tests to include a valid status value, ensuring the fixture
matches the query result and supports the audit payload’s previousStatus field.
In `@backend/tests/reportLinks.integration.test.ts`:
- Around line 179-180: The audit-log mocks lack assertions for the newly emitted
events. In backend/tests/reportLinks.integration.test.ts:179-180, extend the
submit test assertions for report_created, item_created, report_link_consumed,
and notification_created, including expected entityId, details, and tx; in
backend/tests/photoSessions.integration.test.ts:68-76, add case-specific
assertions for photo_session_created, photo_upload_authorized,
photo_image_registered, photo_session_access_denied, and photo_image_deleted,
including expected action, entity, and outcome.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdcfb346-eec1-407b-837d-f070a0a75025
📒 Files selected for processing (37)
backend/README.mdbackend/prisma/migrations/20260720180000_harden_audit_log/migration.sqlbackend/prisma/schema.prismabackend/src/index.tsbackend/src/jobs/cleanupUnverifiedUsers.tsbackend/src/jobs/expireRetainedItems.tsbackend/src/lib/claimEmailNotifications.tsbackend/src/lib/matching/suggestions.tsbackend/src/lib/notifications.tsbackend/src/middleware/requestContext.tsbackend/src/middleware/requireRole.tsbackend/src/routes/auth.tsbackend/src/routes/claims.tsbackend/src/routes/items.tsbackend/src/routes/photoSessions.tsbackend/src/routes/reportLinks.tsbackend/src/routes/uploads.tsbackend/src/routes/users.tsbackend/src/types/express.d.tsbackend/src/utils/auditEvents.tsbackend/src/utils/auditLog.tsbackend/src/utils/uploadMetadata.tsbackend/tests/auditCoverage.test.tsbackend/tests/auditLog.test.tsbackend/tests/auditMigration.test.tsbackend/tests/auth.extra.integration.test.tsbackend/tests/auth.login.integration.test.tsbackend/tests/claims.integration.test.tsbackend/tests/cleanupUnverifiedUsers.test.tsbackend/tests/expireRetainedItems.test.tsbackend/tests/items.integration.test.tsbackend/tests/photoSessions.integration.test.tsbackend/tests/reportLinks.integration.test.tsbackend/tests/requestContext.test.tsbackend/tests/requireRole.test.tsbackend/tests/uploads.integration.test.tsbackend/tests/users.integration.test.ts
| const context = auditContextFromRequest(req); | ||
| await prisma.$transaction(async (tx) => { | ||
| await tx.refreshTokenLog.create({ | ||
| data: { | ||
| userId: user.userId, | ||
| tokenHash, | ||
| expiresAt: new Date(Date.now() + refreshDays * 24 * 60 * 60 * 1000), | ||
| }, | ||
| }); | ||
| await writeAuditLog( | ||
| { | ||
| actorId: user.userId, | ||
| actorType: 'user', | ||
| action: 'user_login', | ||
| entityType: 'user', | ||
| entityId: user.userId, | ||
| outcome: 'success', | ||
| ...context, | ||
| }, | ||
| tx | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Required audit write shares a transaction with the core mutation — an audit failure now fails login/registration/verification/refresh.
At each of these sites, writeAuditLog(..., tx) runs inside the same prisma.$transaction as the primary mutation (refresh-token creation, user creation, rollback deletion, email-verification update, token rotation). If the audit insert throws for any reason (e.g. a constraint issue, pool exhaustion), the whole transaction rolls back and the request 500s even though the credentials/token were valid. For the rollback path (421-435) this is especially bad: a failing audit write there also undoes the compensating tx.user.delete, leaving an un-cleaned-up user row and no rollback audit trail.
This matches the documented intent in backend/README.md ("Required audit rows commit with their database mutation"), so it may be an accepted trade-off — but it's worth double-checking that audit_log writes are resilient enough (schema-stable, no soft dependencies) that this coupling doesn't turn transient audit-table hiccups into full auth outages.
Also applies to: 366-397, 421-435, 509-542, 667-692
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/auth.ts` around lines 226 - 246, Review writeAuditLog and
its audit_log dependencies for schema-stable, transaction-safe behavior at each
listed auth mutation site. Ensure required audit inserts do not rely on soft
dependencies or failure-prone side effects, while keeping writeAuditLog(..., tx)
in the existing prisma.$transaction so audit rows commit atomically with the
primary mutation.
| await writeAuditLog({ | ||
| actorType: 'anonymous', | ||
| action: 'photo_image_registered', | ||
| entityType: 'photo_session_image', | ||
| entityId: image.imageId, | ||
| outcome: 'success', | ||
| details: { | ||
| sessionId: session.sessionId, | ||
| fileType, | ||
| sizeCategory: getUploadSizeCategory(fileSizeKb), | ||
| }, | ||
| ...auditContextFromRequest(req), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Required (non-best-effort) audit write with no accompanying mutation or transaction.
This handler only reads/validates an already-persisted image (created earlier in the presigned-url step) — no create/update happens here. Using writeAuditLog (which throws and propagates to next(err) on failure) means a transient audit-log DB error turns a legitimate, already-successful upload confirmation into a 500 for the caller, even though nothing needs to be rolled back. This is inconsistent with the best-effort pattern used for photo_upload_authorized just above, which is the same kind of "record an event, don't gate the response" case.
🔧 Use best-effort logging since there's no mutation to protect
- await writeAuditLog({
+ await writeAuditLogBestEffort({
actorType: 'anonymous',
action: 'photo_image_registered',
entityType: 'photo_session_image',
entityId: image.imageId,
outcome: 'success',
details: {
sessionId: session.sessionId,
fileType,
sizeCategory: getUploadSizeCategory(fileSizeKb),
},
...auditContextFromRequest(req),
});📝 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.
| await writeAuditLog({ | |
| actorType: 'anonymous', | |
| action: 'photo_image_registered', | |
| entityType: 'photo_session_image', | |
| entityId: image.imageId, | |
| outcome: 'success', | |
| details: { | |
| sessionId: session.sessionId, | |
| fileType, | |
| sizeCategory: getUploadSizeCategory(fileSizeKb), | |
| }, | |
| ...auditContextFromRequest(req), | |
| }); | |
| await writeAuditLogBestEffort({ | |
| actorType: 'anonymous', | |
| action: 'photo_image_registered', | |
| entityType: 'photo_session_image', | |
| entityId: image.imageId, | |
| outcome: 'success', | |
| details: { | |
| sessionId: session.sessionId, | |
| fileType, | |
| sizeCategory: getUploadSizeCategory(fileSizeKb), | |
| }, | |
| ...auditContextFromRequest(req), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/photoSessions.ts` around lines 384 - 397, Change the audit
call in the photo image registration handler from required `writeAuditLog` to
the established best-effort logging pattern used for `photo_upload_authorized`
above. Preserve the existing audit payload and ensure audit persistence failures
are handled without propagating through `next(err)` or turning the successful
upload confirmation into a 500.
| await writeAuditLogBestEffort({ | ||
| actorId: req.user!.user_id, | ||
| actorType: 'user', | ||
| action: 'photo_session_access_denied', | ||
| entityType: 'photo_session', | ||
| entityId: null, | ||
| outcome: 'denied', | ||
| reasonCode: 'session_ownership_mismatch', | ||
| ...auditContextFromRequest(req), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Denial audit log has no entityId because sessionId isn't selected.
The photoUploadSession.findUnique call above (lines 422-438) selects createdBy, expiresAt, and images, but never sessionId, so entityId: null is unavoidable here. Include sessionId in the select and use it, so this denial event can be correlated to the specific session it targeted.
🔧 Select sessionId and use it as entityId
const session = await prisma.photoUploadSession.findUnique({
where: { token },
select: {
+ sessionId: true,
createdBy: true,
expiresAt: true,
images: {
...
},
},
});
...
if (session.createdBy !== req.user!.user_id) {
await writeAuditLogBestEffort({
actorId: req.user!.user_id,
actorType: 'user',
action: 'photo_session_access_denied',
entityType: 'photo_session',
- entityId: null,
+ entityId: session.sessionId,
outcome: 'denied',
reasonCode: 'session_ownership_mismatch',
...auditContextFromRequest(req),
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/photoSessions.ts` around lines 449 - 458, Update the
photoUploadSession.findUnique select used by the access-denial path to include
sessionId, then pass the selected sessionId as entityId in the
photo_session_access_denied audit log instead of null. Preserve the existing
ownership-mismatch handling and audit context.
| const context = auditContextFromRequest(req); | ||
| const changedFields = [ | ||
| ...(existing.firstName !== firstName ? ['firstName'] : []), | ||
| ...(existing.lastName !== lastName ? ['lastName'] : []), | ||
| ...(isStudent && studentNumber !== undefined | ||
| ? ['studentNumber'] | ||
| : []), | ||
| ]; | ||
| updated = await prisma.$transaction(async (tx) => { | ||
| const profile = await tx.user.update({ | ||
| where: { userId }, | ||
| data, | ||
| select: { | ||
| ...userProfileSelect, | ||
| campus: { select: { campusName: true } }, | ||
| }, | ||
| }); | ||
| await writeAuditLog( | ||
| { | ||
| actorId: userId, | ||
| actorType: 'user', | ||
| action: 'user_profile_updated', | ||
| entityType: 'user', | ||
| entityId: userId, | ||
| outcome: 'success', | ||
| details: { changedFields }, | ||
| ...context, | ||
| }, | ||
| tx | ||
| ); | ||
| return profile; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
changedFields marks studentNumber as changed even when it's unchanged.
firstName/lastName are added to changedFields only via direct comparison against existing, but studentNumber is added whenever the field was explicitly present in the request (studentNumber !== undefined), with no comparison to existing.studentNumber. Resubmitting the same value logs a spurious "changed" entry in the audit trail.
🔧 Compare against existing value (bigint-aware)
const changedFields = [
...(existing.firstName !== firstName ? ['firstName'] : []),
...(existing.lastName !== lastName ? ['lastName'] : []),
- ...(isStudent && studentNumber !== undefined
- ? ['studentNumber']
- : []),
+ ...(isStudent &&
+ studentNumber !== undefined &&
+ (studentNumber === null
+ ? existing.studentNumber !== null
+ : existing.studentNumber !== BigInt(studentNumber))
+ ? ['studentNumber']
+ : []),
];📝 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.
| const context = auditContextFromRequest(req); | |
| const changedFields = [ | |
| ...(existing.firstName !== firstName ? ['firstName'] : []), | |
| ...(existing.lastName !== lastName ? ['lastName'] : []), | |
| ...(isStudent && studentNumber !== undefined | |
| ? ['studentNumber'] | |
| : []), | |
| ]; | |
| updated = await prisma.$transaction(async (tx) => { | |
| const profile = await tx.user.update({ | |
| where: { userId }, | |
| data, | |
| select: { | |
| ...userProfileSelect, | |
| campus: { select: { campusName: true } }, | |
| }, | |
| }); | |
| await writeAuditLog( | |
| { | |
| actorId: userId, | |
| actorType: 'user', | |
| action: 'user_profile_updated', | |
| entityType: 'user', | |
| entityId: userId, | |
| outcome: 'success', | |
| details: { changedFields }, | |
| ...context, | |
| }, | |
| tx | |
| ); | |
| return profile; | |
| const context = auditContextFromRequest(req); | |
| const changedFields = [ | |
| ...(existing.firstName !== firstName ? ['firstName'] : []), | |
| ...(existing.lastName !== lastName ? ['lastName'] : []), | |
| ...(isStudent && | |
| studentNumber !== undefined && | |
| (studentNumber === null | |
| ? existing.studentNumber !== null | |
| : existing.studentNumber !== BigInt(studentNumber)) | |
| ? ['studentNumber'] | |
| : []), | |
| ]; | |
| updated = await prisma.$transaction(async (tx) => { | |
| const profile = await tx.user.update({ | |
| where: { userId }, | |
| data, | |
| select: { | |
| ...userProfileSelect, | |
| campus: { select: { campusName: true } }, | |
| }, | |
| }); | |
| await writeAuditLog( | |
| { | |
| actorId: userId, | |
| actorType: 'user', | |
| action: 'user_profile_updated', | |
| entityType: 'user', | |
| entityId: userId, | |
| outcome: 'success', | |
| details: { changedFields }, | |
| ...context, | |
| }, | |
| tx | |
| ); | |
| return profile; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/routes/users.ts` around lines 229 - 259, Update the studentNumber
entry in changedFields to compare the requested value with
existing.studentNumber, rather than checking only studentNumber !== undefined.
Preserve the existing undefined guard and use a bigint-aware comparison
consistent with the types used by existing.studentNumber.
| export async function writeAuditLogBestEffort( | ||
| params: AuditLogParams | ||
| ): Promise<void> { | ||
| const data = toAuditData(params); | ||
| try { | ||
| const record = await prisma.auditLog.create({ data }); | ||
| logger.info( | ||
| { | ||
| logId: record.logId, | ||
| action: params.action, | ||
| actorId: params.actorId, | ||
| entityType: params.entityType, | ||
| entityId: params.entityId, | ||
| outcome: params.outcome ?? 'success', | ||
| requestId: params.requestId, | ||
| runId: params.runId, | ||
| }, | ||
| params.action | ||
| ); | ||
| } catch (error) { | ||
| const errorMetadata = | ||
| error instanceof Error | ||
| ? { | ||
| errorName: error.name, | ||
| errorCode: | ||
| 'code' in error && typeof error.code === 'string' | ||
| ? error.code | ||
| : undefined, | ||
| } | ||
| : { errorName: 'UnknownError' }; | ||
| logger.error( | ||
| { | ||
| ...errorMetadata, | ||
| action: params.action, | ||
| outcome: params.outcome ?? 'success', | ||
| actorType: resolveActorType(params), | ||
| entityType: params.entityType, | ||
| entityId: params.entityId, | ||
| reasonCode: params.reasonCode, | ||
| requestId: params.requestId, | ||
| runId: params.runId, | ||
| }, | ||
| 'audit_log_persistence_failed' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
writeAuditLogBestEffort can still throw on contract-validation errors, breaking its own guarantee.
toAuditData(params) on line 140 runs outside the try block, so assertSafeDetails/assertRequiredDetails/entity-type-mismatch errors are not caught, unlike persistence errors which are. Every best-effort call site (e.g. requireRole.ts's authorization_denied write before sending the 403, or the denial-logging paths in photoSessions.ts/uploads.ts) awaits this function with no surrounding try/catch, assuming it never rejects. A future typo in a details payload (a detail key not registered for that action, or a prohibited key) would throw synchronously and leave those security-denial responses unsent instead of degrading gracefully. This is confirmed intentional by auditLog.test.ts's "best-effort writes reject invalid event contracts before persistence" test, but that test asserts exactly the risky behavior described here.
🛡️ Proposed fix: make validation failures fail closed instead of propagating
export async function writeAuditLogBestEffort(
params: AuditLogParams
): Promise<void> {
- const data = toAuditData(params);
try {
+ const data = toAuditData(params);
const record = await prisma.auditLog.create({ data });If contract bugs should still fail CI/tests loudly, keep coverage in auditCoverage.test.ts/auditLog.test.ts as the safety net for catching them pre-merge, rather than at runtime in a security-critical path.
📝 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.
| export async function writeAuditLogBestEffort( | |
| params: AuditLogParams | |
| ): Promise<void> { | |
| const data = toAuditData(params); | |
| try { | |
| const record = await prisma.auditLog.create({ data }); | |
| logger.info( | |
| { | |
| logId: record.logId, | |
| action: params.action, | |
| actorId: params.actorId, | |
| entityType: params.entityType, | |
| entityId: params.entityId, | |
| outcome: params.outcome ?? 'success', | |
| requestId: params.requestId, | |
| runId: params.runId, | |
| }, | |
| params.action | |
| ); | |
| } catch (error) { | |
| const errorMetadata = | |
| error instanceof Error | |
| ? { | |
| errorName: error.name, | |
| errorCode: | |
| 'code' in error && typeof error.code === 'string' | |
| ? error.code | |
| : undefined, | |
| } | |
| : { errorName: 'UnknownError' }; | |
| logger.error( | |
| { | |
| ...errorMetadata, | |
| action: params.action, | |
| outcome: params.outcome ?? 'success', | |
| actorType: resolveActorType(params), | |
| entityType: params.entityType, | |
| entityId: params.entityId, | |
| reasonCode: params.reasonCode, | |
| requestId: params.requestId, | |
| runId: params.runId, | |
| }, | |
| 'audit_log_persistence_failed' | |
| ); | |
| } | |
| } | |
| export async function writeAuditLogBestEffort( | |
| params: AuditLogParams | |
| ): Promise<void> { | |
| try { | |
| const data = toAuditData(params); | |
| const record = await prisma.auditLog.create({ data }); | |
| logger.info( | |
| { | |
| logId: record.logId, | |
| action: params.action, | |
| actorId: params.actorId, | |
| entityType: params.entityType, | |
| entityId: params.entityId, | |
| outcome: params.outcome ?? 'success', | |
| requestId: params.requestId, | |
| runId: params.runId, | |
| }, | |
| params.action | |
| ); | |
| } catch (error) { | |
| const errorMetadata = | |
| error instanceof Error | |
| ? { | |
| errorName: error.name, | |
| errorCode: | |
| 'code' in error && typeof error.code === 'string' | |
| ? error.code | |
| : undefined, | |
| } | |
| : { errorName: 'UnknownError' }; | |
| logger.error( | |
| { | |
| ...errorMetadata, | |
| action: params.action, | |
| outcome: params.outcome ?? 'success', | |
| actorType: resolveActorType(params), | |
| entityType: params.entityType, | |
| entityId: params.entityId, | |
| reasonCode: params.reasonCode, | |
| requestId: params.requestId, | |
| runId: params.runId, | |
| }, | |
| 'audit_log_persistence_failed' | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/utils/auditLog.ts` around lines 137 - 182, Move the
toAuditData(params) call inside the try block in writeAuditLogBestEffort so
contract-validation errors from assertSafeDetails, assertRequiredDetails, or
entity-type validation are caught and handled like persistence failures.
Preserve best-effort behavior by logging the failure and resolving without
rejecting; update the existing auditLog.test.ts expectation that invalid
contracts reject, while retaining coverage that detects invalid contracts during
tests.
1 participant
Summary by CodeRabbit
X-Request-Idresponse header.