Audit update - #119
Conversation
Adds a per-action summary-sentence registry (auditSummaries.ts) and wires it into toAuditData() alongside two new AuditLogParams fields (actorRole, entityLabel). Both are kept as separate params rather than caller-supplied `details` keys, so they never need to pass through the per-action detailKeys allowlist and no catalog changes are required.
… and summary Threads actorRole through login/refresh success writes and entityLabel (target account's role) through denied-auth events that already have a loaded user record. refresh_token_denied's user_missing branch gets neither, since no account was resolved. requireRole.ts needs no change -- its existing actualRole detail already feeds the new summary.
…e and summary Threads actorRole/entityLabel through direct item registration, status updates, walk-in release, and the report-submission chain (report, item, link-consumption, notification share one entity label). Also fixes item_created's missing source key and item_status_denied's missing previousStatus on the approved-claim-conflict path. Also fixes the summary generator's array formatting (comma+space) so changed-field lists read naturally in the generated summary sentence.
Threads actorRole through authenticated profile, upload-authorization, and photo-session writes; the anonymous walk-in photo routes get an entity label but no actorRole, since there's no authenticated actor. Also records the actual session id (previously null) and a label on the two access-denied writes that used to carry empty details. Updates the item_updated summary test expectation to match the comma+space list formatting fixed alongside U5.
…mary Adds entityLabel (item name/campus/retention date) to expireRetainedItems' claim and item audit rows and the missing itemId detail; no actorRole is ever set since these are system-authored runs. unverified_user_deleted now gets a non-empty summary for free via the U1 registry, so only its test needed updating. Fixes the expireRetainedItems test to check entityLabel inside details (where the enrichment actually places it) rather than as a top-level row field.
…d helpers Threads actorRole and an item/claim entityLabel through every claims.ts audit write, including applyMatchConfirmation (now takes role + an optional item label used across its four writes) and both of its call sites (item-link confirm, match-suggestion confirm). Fixes claim_status_updated's missing itemId detail. Widens fanOutToCampusSecurity, refreshClaimMatchSuggestions, and deliverStudentClaimEmail's audit/context params to accept actorRole (and entityLabel where relevant), threaded from the caller's already-loaded actor -- system-triggered calls (cron batch refresh) leave actorRole unset. Strengthens existing claims.integration.test.ts assertions to cover AE1 (rejection carries actorRole/entityLabel/summary) and the claim-created/email-notification paths' new fields. Known gap: applyMatchConfirmation's two call sites (item-link confirm, match-suggestion confirm) have no pre-existing integration test coverage in this file (prisma.item is not mocked there), so the role/ label threading through that specific path is verified by typecheck and code review only, not a dedicated test.
Adds a short section to the audit-event registry describing the three new fields and why they bypass the per-action detailKeys allowlist. Adds a coverage test asserting every catalog action has a matching auditSummaries entry, so a future action added without one fails loudly.
notification_fanout_created writes for claim creation and cancellation had no entityLabel despite the claim's item name being available at both call sites. The item-disposal auto-rejection loop in items.ts had the same gap for its per-claim claim_status_updated writes despite the item's title/category already being in scope one line above.
|
Warning Review limit reached
Next review available in: 46 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 (1)
📝 WalkthroughWalkthroughAudit logging now centrally enriches persisted details with actor roles, entity labels, and action-specific summaries. Routes, jobs, notifications, and email flows supply additional context, while unit and integration tests verify coverage, formatting, and sensitive-data exclusion. ChangesAudit enrichment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Route
participant toAuditData
participant auditSummaries
participant AuditLog
Route->>toAuditData: Submit audit action and context
toAuditData->>auditSummaries: Build action-specific summary
auditSummaries-->>toAuditData: Return summary text
toAuditData->>AuditLog: Persist enriched details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 4
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/photoSessions.ts (1)
551-560: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the available session label to deletion audits.
photo_image_deletedhassession.sessionIdin scope but omitsentityLabel, unlike the adjacent access-denied event. AddentityLabel: \photo session ${session.sessionId}`` so successful deletions retain the same audit context.🤖 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 551 - 560, Add the session label to the successful photo deletion audit object for action photo_image_deleted, using entityLabel with the value “photo session ${session.sessionId}” alongside the existing sessionId detail and deleteContext fields.
🧹 Nitpick comments (1)
backend/src/utils/auditSummaries.ts (1)
92-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
claim_match_suggestion_revieweddoesn't surfaceentityLabel, unlike sibling claim summaries.Every other claim/item summary wraps its subject with
withLabel(...)so the persistedentityLabelshows up in the human-readable text (e.g.claim_status_updated,claim_transition_denied,item_status_updated). This one only readstransition(ctx), so a caller-suppliedentityLabelfor this action is silently dropped from the summary even though it's still stored separately indetails.entityLabel.♻️ Proposed fix
claim_match_suggestion_reviewed: (ctx) => - `${actorPhrase(ctx)} reviewed a match suggestion${transition(ctx)}.`, + `${actorPhrase(ctx)} reviewed ${withLabel('a match suggestion', ctx)}${transition(ctx)}.`,🤖 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/auditSummaries.ts` around lines 92 - 93, Update the claim_match_suggestion_reviewed summary in the audit summary definitions to wrap its match-suggestion subject with the existing withLabel(...) helper, preserving the current actorPhrase(ctx) and transition(ctx) content while including any supplied entityLabel.
🤖 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/docs/audit-events.md`:
- Around line 11-15: The audit serialization flow around toAuditData() and
enrichDetails() must validate entityLabel before injecting it into details,
using the existing centralized sensitive-value and length validation applied to
caller details. Ensure request-derived labels such as payload.itemName cannot
persist emails, phone numbers, or other PII, and update the entityLabel
documentation to state this enforced validation contract.
In `@backend/src/jobs/expireRetainedItems.ts`:
- Around line 220-224: The item_auto_expired label in the expire-retained-items
flow currently uses the opaque item.campusId. Update the surrounding job logic
to derive a safe human-readable item title or category, using the campus name
only when campus context is needed, and use that value in entityLabel while
preserving the retention-expiry suffix. Update the corresponding test
expectation to match the new label.
In `@backend/src/routes/claims.ts`:
- Around line 521-523: Update applyMatchConfirmation to accept separate
claimLabel and itemLabel parameters. Use claimLabel for claim audit events and
itemLabel for item audit events, including direct-link and suggestion-review
callers so each entityType receives its correct label.
In `@backend/src/utils/auditSummaries.ts`:
- Around line 18-25: Update actorPhrase to handle ctx.actorType === 'unknown'
explicitly, returning wording that indicates the actor could not be resolved;
keep the existing role, system, anonymous, and generic user mappings unchanged.
---
Outside diff comments:
In `@backend/src/routes/photoSessions.ts`:
- Around line 551-560: Add the session label to the successful photo deletion
audit object for action photo_image_deleted, using entityLabel with the value
“photo session ${session.sessionId}” alongside the existing sessionId detail and
deleteContext fields.
---
Nitpick comments:
In `@backend/src/utils/auditSummaries.ts`:
- Around line 92-93: Update the claim_match_suggestion_reviewed summary in the
audit summary definitions to wrap its match-suggestion subject with the existing
withLabel(...) helper, preserving the current actorPhrase(ctx) and
transition(ctx) content while including any supplied entityLabel.
🪄 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: c24e4351-195c-4bf5-b70b-22d8ed78f036
📒 Files selected for processing (24)
backend/docs/audit-events.mdbackend/src/jobs/expireRetainedItems.tsbackend/src/lib/claimEmailNotifications.tsbackend/src/lib/matching/suggestions.tsbackend/src/lib/notifications.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/utils/auditLog.tsbackend/src/utils/auditSummaries.tsbackend/tests/auditCoverage.test.tsbackend/tests/auditLog.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/uploads.integration.test.tsbackend/tests/users.integration.test.ts
| - `details.actorRole` (`student` | `security` | `admin`) — present only when the call site supplies an authenticated actor's role. System and anonymous writes never fabricate one. | ||
| - `details.entityLabel` — a short human-readable name for the entity the record concerns (item title/category, claim item name, campus, etc.), supplied by the call site from data already in scope. Never a URL, object key, filename, or personal field. | ||
| - `details.summary` — a one-line generated sentence describing the event, present on every record. | ||
|
|
||
| These three keys bypass each action's own `detailKeys` allowlist by design (they are supplied as dedicated `AuditLogParams` fields, not as caller-supplied `details` entries), so adding them required no changes to the ~40 action definitions above. A dedicated test (`tests/auditCoverage.test.ts`) fails if a future action is added without a matching entry in the summary registry. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate entityLabel before the allowlist bypass.
toAuditData() validates only caller details before enrichDetails() injects entityLabel; claim creation passes request-derived payload.itemName as that label. This can persist emails, phone numbers, or other PII despite the stated exclusion policy. Apply central sensitive-value/length validation to entityLabel before enrichment, then document that enforced contract.
🤖 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/docs/audit-events.md` around lines 11 - 15, The audit serialization
flow around toAuditData() and enrichDetails() must validate entityLabel before
injecting it into details, using the existing centralized sensitive-value and
length validation applied to caller details. Ensure request-derived labels such
as payload.itemName cannot persist emails, phone numbers, or other PII, and
update the entityLabel documentation to state this enforced validation contract.
| entityLabel: `campus ${item.campusId}${ | ||
| item.retentionExpiryDate | ||
| ? `, retention expired ${item.retentionExpiryDate.toISOString().slice(0, 10)}` | ||
| : '' | ||
| }`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use the opaque campus ID as the item label.
item_auto_expired stores campus ${item.campusId}, which is not a human-readable item identifier when campusId is a UUID. Return a safe item title/category for this job and use it as the label; if campus context is needed, use the campus name instead. Update the corresponding test expectation.
🤖 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 220 - 224, The
item_auto_expired label in the expire-retained-items flow currently uses the
opaque item.campusId. Update the surrounding job logic to derive a safe
human-readable item title or category, using the campus name only when campus
context is needed, and use that value in entityLabel while preserving the
retention-expiry suffix. Update the corresponding test expectation to match the
new label.
| role: UserRole, | ||
| context: { requestId: string; ipAddress?: string }, | ||
| itemLabel?: string |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Carry separate claim and item labels.
applyMatchConfirmation applies one itemLabel to both claim and item audit events. The direct-link flow passes the claim name, mislabeling item_status_updated; the suggestion-review flow passes the stored-item label, mislabeling claim events. Accept claimLabel and itemLabel separately, and select the one matching each event’s entityType.
🤖 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 521 - 523, Update
applyMatchConfirmation to accept separate claimLabel and itemLabel parameters.
Use claimLabel for claim audit events and itemLabel for item audit events,
including direct-link and suggestion-review callers so each entityType receives
its correct label.
| function actorPhrase(ctx: AuditSummaryContext): string { | ||
| if (ctx.actorRole === 'student') return 'A student'; | ||
| if (ctx.actorRole === 'security') return 'Security staff'; | ||
| if (ctx.actorRole === 'admin') return 'An admin'; | ||
| if (ctx.actorType === 'system') return 'A system job'; | ||
| if (ctx.actorType === 'anonymous') return 'An anonymous requester'; | ||
| return 'A user'; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
actorPhrase doesn't handle actorType === 'unknown' explicitly.
Falls through to the 'A user' default, which misrepresents the audit trail when the actor could not actually be resolved (resolveActorType returns 'unknown' when neither actorId nor actorType is supplied).
🐛 Proposed fix
function actorPhrase(ctx: AuditSummaryContext): string {
if (ctx.actorRole === 'student') return 'A student';
if (ctx.actorRole === 'security') return 'Security staff';
if (ctx.actorRole === 'admin') return 'An admin';
if (ctx.actorType === 'system') return 'A system job';
if (ctx.actorType === 'anonymous') return 'An anonymous requester';
+ if (ctx.actorType === 'unknown') return 'An unidentified actor';
return 'A user';
}📝 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.
| function actorPhrase(ctx: AuditSummaryContext): string { | |
| if (ctx.actorRole === 'student') return 'A student'; | |
| if (ctx.actorRole === 'security') return 'Security staff'; | |
| if (ctx.actorRole === 'admin') return 'An admin'; | |
| if (ctx.actorType === 'system') return 'A system job'; | |
| if (ctx.actorType === 'anonymous') return 'An anonymous requester'; | |
| return 'A user'; | |
| } | |
| function actorPhrase(ctx: AuditSummaryContext): string { | |
| if (ctx.actorRole === 'student') return 'A student'; | |
| if (ctx.actorRole === 'security') return 'Security staff'; | |
| if (ctx.actorRole === 'admin') return 'An admin'; | |
| if (ctx.actorType === 'system') return 'A system job'; | |
| if (ctx.actorType === 'anonymous') return 'An anonymous requester'; | |
| if (ctx.actorType === 'unknown') return 'An unidentified actor'; | |
| return 'A user'; | |
| } |
🤖 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/auditSummaries.ts` around lines 18 - 25, Update actorPhrase
to handle ctx.actorType === 'unknown' explicitly, returning wording that
indicates the actor could not be resolved; keep the existing role, system,
anonymous, and generic user mappings unchanged.
Fixes the CI Prettier Check failure -- an import statement was wrapped across multiple lines when it fits on one.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests