fix: redact sensitive audit log output - #95
Conversation
📝 WalkthroughWalkthroughBackend changes rename the claim-approval trigger condition from ChangesClaim approval flow, logging, and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClaimsRoute
participant Prisma
participant AuditLog
Client->>ClaimsRoute: PATCH /api/claims/:claimId/status (approved)
ClaimsRoute->>Prisma: begin transaction
Prisma->>Prisma: update linked item to claimed
Prisma->>Prisma: update claim status
Prisma->>Prisma: create notification
Prisma->>AuditLog: createAuditLog(notification data)
AuditLog->>AuditLog: logger.info(logId, action, actorId, entityType, entityId)
Prisma-->>ClaimsRoute: commit transaction
ClaimsRoute-->>Client: updated claim detail
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/routes/claims.ts (1)
1011-1032: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard the item transition with a conditional write.
findUniqueplus an unconditionalupdateleaves a TOCTOU window here; two concurrent approvals can both observestoredand both flip the same item toclaimed. Use the sameupdateMany({ where: { itemId, status: ItemStatus.stored } })pattern already used elsewhere so only one approval can win.🤖 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 1011 - 1032, The item status check and update inside the prisma.$transaction block in claims.ts are vulnerable to a TOCTOU race because item.findUnique followed by item.update can let concurrent approvals both claim the same item. Update the approval flow in the claim status handling logic to use a conditional write on tx.item, matching the existing updateMany pattern used elsewhere, so the write only succeeds when itemId matches and status is ItemStatus.stored, and handle the zero-row result as the conflict case.
🧹 Nitpick comments (1)
backend/src/lib/logger.ts (1)
1-6: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding pino's built-in
redactoption for defense-in-depth.Redaction currently relies solely on
writeAuditLogmanually choosing which fields to pass tologger.info(excludingdetails/ipAddress). If any future caller logs the full audit params or record directly, PII (e.g.ipAddress,detailssuch asstudentFullName,contactNumber) could leak into logs. Configuring pino's nativeredactpaths on this shared logger would enforce redaction centrally rather than relying on every call site being careful.♻️ Suggested addition
export const logger = pino({ level: process.env.LOG_LEVEL || 'info', + redact: { + paths: ['ipAddress', 'details', '*.ipAddress', '*.details'], + censor: '[REDACTED]', + }, });🤖 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/lib/logger.ts` around lines 1 - 6, The shared logger in logger should enforce central PII protection instead of relying only on writeAuditLog to omit sensitive fields. Update the pino configuration in logger to use its built-in redact option so fields like ipAddress and nested details values such as studentFullName and contactNumber are always redacted even if future callers log full audit params or records. Keep the change localized to the logger setup so all consumers of logger inherit the safeguard automatically.
🤖 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/README.md`:
- Around line 267-269: The API status table is inconsistent with the implemented
`PATCH /api/items/:itemId/status` endpoint because it still shows the route as
Planned even though the docs describe a finished handler. Update the summary
table entry for `router.patch('/items/:itemId/status', ...)` to match the actual
implementation state, likely changing the Status from Planned to Done so the
README and endpoint details stay aligned.
In `@backend/src/routes/claims.ts`:
- Around line 1052-1071: The audit entry in the claim notification flow is being
written inside the transaction through writeAuditLog(..., tx), which can log
claim_notification_sent before the notification is actually committed. Update
the claim status notification path in backend/src/routes/claims.ts so the audit
log happens only after the tx has successfully committed, or make writeAuditLog
skip logger.info() when a transaction client is passed. Use the
createClaimStatusNotificationInput, notification.create, and writeAuditLog call
sites to locate the change.
---
Outside diff comments:
In `@backend/src/routes/claims.ts`:
- Around line 1011-1032: The item status check and update inside the
prisma.$transaction block in claims.ts are vulnerable to a TOCTOU race because
item.findUnique followed by item.update can let concurrent approvals both claim
the same item. Update the approval flow in the claim status handling logic to
use a conditional write on tx.item, matching the existing updateMany pattern
used elsewhere, so the write only succeeds when itemId matches and status is
ItemStatus.stored, and handle the zero-row result as the conflict case.
---
Nitpick comments:
In `@backend/src/lib/logger.ts`:
- Around line 1-6: The shared logger in logger should enforce central PII
protection instead of relying only on writeAuditLog to omit sensitive fields.
Update the pino configuration in logger to use its built-in redact option so
fields like ipAddress and nested details values such as studentFullName and
contactNumber are always redacted even if future callers log full audit params
or records. Keep the change localized to the logger setup so all consumers of
logger inherit the safeguard automatically.
🪄 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: 178c57b3-108b-4d1a-814f-6e3c5f8afc78
⛔ Files ignored due to path filters (1)
backend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
backend/README.mdbackend/package.jsonbackend/src/lib/logger.tsbackend/src/routes/claims.tsbackend/src/routes/items.tsbackend/src/utils/auditLog.ts
| Allowed targets: `expired`, `disposed`. Transitions: `stored → expired|disposed`, `expired → disposed`. Blocked when item is `claimed` or `disposed`. Returns updated item detail; rejects active linked claims in the same transaction. | ||
|
|
||
| A daily cron job (`expireRetainedItems`) also transitions `stored → expired` when `retentionExpiryDate` has passed (skips items with an approved claim awaiting pickup). Security should use `disposed` to record physical disposal after expiry. | ||
| A daily cron job (`expireRetainedItems`) also transitions `stored → expired` when `retentionExpiryDate` has passed, with a defensive skip for stored items that already have an approved claim. Security should use `disposed` to record physical disposal after expiry. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Items table still marks this endpoint "Planned" while this section documents a fully implemented endpoint.
This section now documents detailed allowed targets, transitions, blocking rules, and cron interaction for PATCH /api/items/:itemId/status, but the summary table row for that same route (unchanged, Line 234) still lists its Status as Planned. The referenced router.patch('/items/:itemId/status', ...) handler (per supplied context) appears fully implemented with validation, transaction, and audit logging — the status label should likely be updated to Done to avoid misleading API consumers.
🤖 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/README.md` around lines 267 - 269, The API status table is
inconsistent with the implemented `PATCH /api/items/:itemId/status` endpoint
because it still shows the route as Planned even though the docs describe a
finished handler. Update the summary table entry for
`router.patch('/items/:itemId/status', ...)` to match the actual implementation
state, likely changing the Status from Planned to Done so the README and
endpoint details stay aligned.
| const notification = await tx.notification.create({ | ||
| data: createClaimStatusNotificationInput(nextClaim, status), | ||
| }); | ||
|
|
||
| await writeAuditLog( | ||
| { | ||
| actorId: actor.userId, | ||
| action: 'claim_notification_sent', | ||
| entityType: 'notification', | ||
| entityId: notification.notificationId, | ||
| details: { | ||
| claimId: nextClaim.claimId, | ||
| recipientId: nextClaim.studentId, | ||
| claimStatus: status, | ||
| }, | ||
| ipAddress: req.ip, | ||
| }, | ||
| tx | ||
| ); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== claims route excerpt ==\n'
sed -n '1020,1085p' backend/src/routes/claims.ts
printf '\n== audit log utility ==\n'
sed -n '1,120p' backend/src/utils/auditLog.tsRepository: 86unj/Foundit
Length of output: 3164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== transaction surrounding the snippet ==\n'
sed -n '930,1095p' backend/src/routes/claims.ts
printf '\n== auditLog helper usages ==\n'
rg -n "writeAuditLog\\(" backend/src -g '!**/node_modules/**'
printf '\n== logger import and config in auditLog helper ==\n'
sed -n '1,120p' backend/src/lib/logger.tsRepository: 86unj/Foundit
Length of output: 6369
Defer the audit log until after commit backend/src/routes/claims.ts:1052-1071
writeAuditLog(..., tx) emits logger.info() before the surrounding transaction commits, so a late commit failure can leave a claim_notification_sent log for a rolled-back notification. Move that log outside the transaction boundary or skip logging when a transaction client is passed.
🤖 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 1052 - 1071, The audit entry in
the claim notification flow is being written inside the transaction through
writeAuditLog(..., tx), which can log claim_notification_sent before the
notification is actually committed. Update the claim status notification path in
backend/src/routes/claims.ts so the audit log happens only after the tx has
successfully committed, or make writeAuditLog skip logger.info() when a
transaction client is passed. Use the createClaimStatusNotificationInput,
notification.create, and writeAuditLog call sites to locate the change.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes