feat(audit): complete audit log system#152
Conversation
|
Claude finished @BillChirico's task in 2m 37s —— View job Reviewing PR #152 — Audit Log System
Found 3 warnings, 1 nitpick. Review submitted with request-changes.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughImplements an audit logging system for tracking admin actions, including a database migration, Express middleware to capture mutating requests, an API endpoint with filtering, a web dashboard page, and automatic retention policy management. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
2 critical, 3 warnings found.
🔴 Critical
-
Audit middleware is dead code (
src/api/index.js:62): Placed after all route handlers, so it never executes. Express stops processing layers once a route sends a response. Must be moved before the route mounts. -
Sensitive data logged in plain text (
src/api/middleware/auditLog.js:176-178): Rawreq.bodyis stored in the audit log. Config updates containing API keys (triage.classifyApiKey,triage.respondApiKey) are stored unmasked.
🟡 Warning
-
Config diff includes unmasked secrets (
src/api/middleware/auditLog.js:182-185):computeConfigDiffoperates on full config including sensitive fields. UsemaskSensitiveFields()before diffing. -
React Fragment missing key (
web/src/app/dashboard/audit-log/page.tsx:361): Shorthand<>in.map()doesn't support keys. Use<Fragment key={entry.id}>. -
Documentation not updated (
migrations/015_audit_logs.cjs): AGENTS.md Key Files table, Database Tables section, and README config reference need updating per project conventions.
Prompt to fix all issues
Fix the following issues in the volvox-bot repo on branch feat/audit-log:
1. FILE: src/api/index.js
ISSUE: The audit log middleware on line 62 is placed AFTER all route handlers, making it dead code.
FIX: Move `router.use(auditLogMiddleware());` to BEFORE the first authenticated route mount (line 33, before the `/config` route). Keep it after the public routes (health, community, auth). Remove the old placement at line 60-62 and its comment.
2. FILE: src/api/middleware/auditLog.js
ISSUE: Lines 176-178 store raw req.body in audit log, which can include sensitive API keys.
FIX: Import `SENSITIVE_FIELDS` and `MASK` from `../utils/configAllowlist.js`. Before assigning `details.body = req.body`, deep-clone the body and mask any fields matching SENSITIVE_FIELDS paths. Use the same masking logic as `maskSensitiveFields()` in configAllowlist.js.
3. FILE: src/api/middleware/auditLog.js
ISSUE: Lines 182-185 compute config diff on unmasked config objects, leaking sensitive field values.
FIX: Import `maskSensitiveFields` from `../utils/configAllowlist.js`. Apply it to both beforeConfig and afterConfig before calling `computeConfigDiff()`:
`const diff = computeConfigDiff(maskSensitiveFields(beforeConfig), maskSensitiveFields(afterConfig));`
Also apply it to the beforeConfig capture on line 162: `beforeConfig = maskSensitiveFields(structuredClone(config));`
4. FILE: web/src/app/dashboard/audit-log/page.tsx
ISSUE: Line 361 uses shorthand Fragment `<>` inside `.map()` which doesn't support keys. React will warn.
FIX: Add `import { Fragment } from 'react';` to the imports (line 5). Replace `<>` on line 361 with `<Fragment key={entry.id}>` and `</>` on line 406 with `</Fragment>`. Remove the `key={entry.id}` from the inner TableRow on line 362.
5. FILES: AGENTS.md, README.md
ISSUE: New files and database table not documented.
FIX: In AGENTS.md, add to Key Files table:
- `src/api/middleware/auditLog.js` | Audit log middleware — intercepts mutating requests, records non-blocking audit entries with config diffs
- `src/api/routes/auditLog.js` | Audit log API route — paginated, filterable audit log retrieval
- `web/src/app/dashboard/audit-log/page.tsx` | Audit log dashboard page — searchable table with filters and expandable details
- `web/src/app/api/guilds/[guildId]/audit-log/route.ts` | Next.js API route — proxies audit log requests to bot API
- `migrations/015_audit_logs.cjs` | Migration — creates `audit_logs` table
Add to Database Tables section:
- `audit_logs` | Admin action audit trail — guild_id, user_id, action, target, details (JSONB), IP, timestamp. Indexed on (guild_id, created_at DESC)
In README.md, add auditLog to the config reference section with `enabled` (boolean) and `retentionDays` (number, default 90).
There was a problem hiding this comment.
Pull request overview
Implements an end-to-end audit logging feature for admin actions in the bot API and surfaces it in the web dashboard (API endpoint + UI page), including retention cleanup and configuration toggles.
Changes:
- Adds
audit_logspersistence (migration), a read API (GET /guilds/:id/audit-log), and a Next.js proxy route for the dashboard. - Introduces an Express audit logging middleware intended to record mutating admin requests, plus scheduler-based retention purging.
- Adds a new dashboard page and navigation entry to browse/filter audit log entries.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/api/index.js |
Wires in the audit log router and attempts to apply the audit middleware. |
src/api/middleware/auditLog.js |
Adds middleware to derive actions/targets and write audit log entries. |
src/api/routes/auditLog.js |
Adds paginated/filterable audit log listing endpoint. |
src/modules/scheduler.js |
Adds periodic cleanup to purge expired audit log entries. |
src/api/utils/configAllowlist.js |
Adds auditLog to the safe config allowlist. |
migrations/015_audit_logs.cjs |
Creates audit_logs table + index. |
web/src/app/api/guilds/[guildId]/audit-log/route.ts |
Proxies audit log reads from the dashboard to the bot API. |
web/src/app/dashboard/audit-log/page.tsx |
Adds the audit log table UI with filters, expansion, and pagination. |
web/src/components/layout/sidebar.tsx |
Adds “Audit Log” navigation item. |
tests/api/routes/auditLog.test.js |
Adds tests for the new audit log API route (auth/filtering/pagination/errors). |
tests/api/middleware/auditLog.test.js |
Adds unit tests for the audit middleware/action derivation/diff behavior. |
config.json |
Adds auditLog config and includes unrelated string/emoji encoding changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/015_audit_logs.cjs`:
- Around line 29-31: The existing index idx_audit_logs_guild_created on
audit_logs(guild_id, created_at DESC) doesn’t help queries that delete by
created_at alone; add a standalone index on created_at (e.g., CREATE INDEX IF
NOT EXISTS idx_audit_logs_created_at ON audit_logs(created_at DESC)) to support
the scheduled retention purge that deletes by created_at without guild_id so it
avoids full-table scans.
In `@src/api/middleware/auditLog.js`:
- Around line 139-140: The audit middleware is calling getConfig() without a
guild ID, causing configDiff to be computed from the global snapshot instead of
the guild-merged snapshot; update all places where getConfig() is used in this
file (e.g., the calls feeding configDiff and any comparisons around configDiff)
to call getConfig(guildId) using the guild id from the request params or
payload, and when applying runtime updates use setConfigValue(path, value,
guildId) so the change targets the guild-scoped config; ensure variables like
configDiff and any subsequent diff logic operate on the returned guild-specific
config object.
- Around line 176-178: The middleware currently assigns raw req.body into
details.body which can leak secrets; instead deep-clone req.body, walk its
object tree (handling arrays) and redact values for a blacklist of sensitive
keys (e.g., "password", "token", "apiKey", "authorization", "access_token",
"refresh_token", "secret") using case-insensitive matching, replacing values
with a constant like "[REDACTED]", then set details.body = redactedClone; ensure
you do not mutate the original req.body and perform this logic where
details.body is assigned in the auditLog middleware.
In `@src/modules/scheduler.js`:
- Around line 221-227: retentionDays from config is used directly in pool.query
with make_interval which can cause SQL errors if non-numeric; validate and
coerce it before use (e.g., parseInt/Number and ensure
Number.isInteger(retentionDays) && retentionDays > 0), fall back to the default
(90) or return early if invalid, and then pass the validated numeric
retentionDays into pool.query for the DELETE on audit_logs to guarantee the SQL
parameter is always a positive integer.
In `@tests/api/middleware/auditLog.test.js`:
- Around line 111-240: Add two regression tests in the existing middleware
suite: one that verifies getConfig is invoked with the guild ID when handling a
guild config update (spy/mock the getConfig function used by auditLogMiddleware
and assert it was called with '123' for a request to
'/api/v1/guilds/123/config'), and another that ensures sensitive keys in
details.body are redacted before the DB insert (configure req.body with keys
like password, token, apiKey, invoke the middleware, trigger the 'finish'
listener, and assert req.app.locals.dbPool.query was called with a
parameter/argument where details.body has those sensitive values
replaced/removed or replaced with a redaction marker). Ensure both tests use the
existing helpers (createMockReq/createMockRes), call auditLogMiddleware(),
simulate response finish via res._listeners.finish(), and assert against
getConfig and dbPool.query calls accordingly.
In `@tests/api/routes/auditLog.test.js`:
- Around line 173-189: Add a test that asserts invalid date query params are
ignored by the audit-log handler: call authed(request(app).get(...)) with
startDate=not-a-date (and optionally endDate invalid), mock mockPool.query to
return count and rows, assert res.status is 200, then inspect
mockPool.query.mock.calls[0] to ensure the SQL does NOT contain 'created_at >='
or 'created_at <=' (same approach as the existing 'should filter by date range'
test), referencing the existing test helpers authed, request, app and
mockPool.query to locate where to add the new case.
In `@web/src/app/dashboard/audit-log/page.tsx`:
- Line 88: Change the searchTimerRef declaration to use an explicit timer type
instead of inferring from an undefined initial value: update the symbol
searchTimerRef to e.g. useRef<NodeJS.Timeout | undefined>(undefined) (or, if you
prefer browser numeric IDs, useRef<number | null>(null)) and keep subsequent
clearTimeout/calls compatible with that type.
- Around line 358-408: The fragment that wraps the pair of rows inside
entries.map (the JSX surrounding TableRow and the conditional details TableRow)
needs a unique key to satisfy React list keys: replace the shorthand fragment
<>...</> with a keyed fragment (e.g., <Fragment key={entry.id}>...</Fragment>)
or use <React.Fragment key={entry.id}> if you prefer, and add the necessary
import for Fragment from 'react' (or reference React). Update the fragment
around the main row and the expanded details row in the entries.map callback so
the outermost element has key={entry.id}.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
config.jsonmigrations/015_audit_logs.cjssrc/api/index.jssrc/api/middleware/auditLog.jssrc/api/routes/auditLog.jssrc/api/utils/configAllowlist.jssrc/modules/scheduler.jstests/api/middleware/auditLog.test.jstests/api/routes/auditLog.test.jsweb/src/app/api/guilds/[guildId]/audit-log/route.tsweb/src/app/dashboard/audit-log/page.tsxweb/src/components/layout/sidebar.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: Docker Build Validation
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — import/export, never require()
Always use node: protocol for Node.js builtin imports (e.g., import { readFileSync } from 'node:fs')
Files:
src/api/utils/configAllowlist.jstests/api/routes/auditLog.test.jssrc/api/routes/auditLog.jstests/api/middleware/auditLog.test.jssrc/modules/scheduler.jssrc/api/middleware/auditLog.jssrc/api/index.js
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Always use semicolons
Use single quotes — enforced by Biome
Use 2-space indentation — enforced by Biome
Files:
src/api/utils/configAllowlist.jstests/api/routes/auditLog.test.jssrc/api/routes/auditLog.jsweb/src/app/dashboard/audit-log/page.tsxtests/api/middleware/auditLog.test.jssrc/modules/scheduler.jsweb/src/components/layout/sidebar.tsxsrc/api/middleware/auditLog.jssrc/api/index.jsweb/src/app/api/guilds/[guildId]/audit-log/route.ts
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER use console.log, console.warn, console.error, or any console.* method in src/ files — always use Winston logger instead: import { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logger: info('Message processed', { userId, channelId })
Use custom error classes from src/utils/errors.js and always log errors with context before re-throwing
Use getConfig(guildId?) from src/modules/config.js to read config; use setConfigValue(path, value, guildId?) to update at runtime
Use safeSend() utility for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Add tests for all new code with mandatory 80% coverage threshold on statements, branches, functions, and lines; run pnpm test before every commit
Files:
src/api/utils/configAllowlist.jssrc/api/routes/auditLog.jssrc/modules/scheduler.jssrc/api/middleware/auditLog.jssrc/api/index.js
web/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use next/image Image component with appropriate layout and sizing props in Next.js components
Files:
web/src/app/dashboard/audit-log/page.tsxweb/src/components/layout/sidebar.tsxweb/src/app/api/guilds/[guildId]/audit-log/route.ts
web/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use Zustand store (zustand) for state management in React components; implement fetch-on-demand pattern in stores
Files:
web/src/app/dashboard/audit-log/page.tsxweb/src/components/layout/sidebar.tsxweb/src/app/api/guilds/[guildId]/audit-log/route.ts
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js by importing handler functions and calling client.on() with config parameter
Check config.yourModule.enabled before processing in module event handlers
Prefer per-request getConfig() pattern in new modules over reactive onConfigChange() wiring; only add onConfigChange() listeners for stateful resources that cannot re-read config on each use
Files:
src/modules/scheduler.js
🧠 Learnings (3)
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/db.js : Database connection requires PostgreSQL (via pg driver with raw SQL, no ORM); the bot works without a database but config persistence requires PostgreSQL
Applied to files:
migrations/015_audit_logs.cjs
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/index.js : The tempban scheduler runs on a 60s interval and catches up on missed unbans after restart; started in index.js startup and stopped in graceful shutdown
Applied to files:
src/modules/scheduler.js
📚 Learning: 2026-02-27T16:24:15.054Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T16:24:15.054Z
Learning: Applies to src/**/*.js : NEVER use console.log, console.warn, console.error, or any console.* method in src/ files — always use Winston logger instead: import { info, warn, error } from '../logger.js'
Applied to files:
src/modules/scheduler.js
🧬 Code graph analysis (5)
tests/api/routes/auditLog.test.js (1)
src/api/server.js (1)
createApp(27-87)
tests/api/middleware/auditLog.test.js (1)
src/api/middleware/auditLog.js (3)
deriveAction(20-58)computeConfigDiff(78-92)auditLogMiddleware(131-220)
src/modules/scheduler.js (2)
src/modules/config.js (2)
err(94-94)getConfig(282-313)src/db.js (1)
getPool(142-147)
src/api/middleware/auditLog.js (4)
src/api/routes/members.js (1)
after(163-163)src/commands/memory.js (1)
targetId(350-350)src/logger.js (1)
info(230-232)src/modules/config.js (2)
err(94-94)getConfig(282-313)
src/api/index.js (1)
src/api/middleware/auditLog.js (1)
auditLogMiddleware(131-220)
🪛 GitHub Check: CodeQL
src/api/routes/auditLog.js
[failure] 41-41: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
src/api/index.js
[failure] 55-55: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
This route handler performs authorization, but is not rate-limited.
🔇 Additional comments (12)
web/src/components/layout/sidebar.tsx (1)
56-60: Nice sidebar integration.The new Audit Log entry cleanly reuses the existing navigation structure and active-route behavior.
src/api/utils/configAllowlist.js (1)
28-28: Allowlist update looks correct.Adding
'auditLog'toSAFE_CONFIG_KEYSis consistent with the new audit configuration surface.src/modules/scheduler.js (1)
202-205: Good maintenance cadence hook.Running audit-log retention cleanup on a throttled scheduler interval is a solid non-intrusive approach.
config.json (1)
264-267: Audit log defaults are sensible.The new
auditLog.enabledandauditLog.retentionDaysdefaults are clear and operationally reasonable.web/src/app/api/guilds/[guildId]/audit-log/route.ts (1)
36-44: Proxy parameter forwarding is well-scoped.Restricting forwarded query params to an allowlist keeps the proxy surface tight and predictable.
src/api/routes/auditLog.js (2)
41-41: Rate limiting is correctly applied — static analysis false positive.The CodeQL warning about missing rate limiting is incorrect. The
auditRateLimitmiddleware (defined at line 16) is properly applied to this route handler as the first middleware in the chain.
1-110: Well-structured audit log API implementation.Clean implementation with proper parameterized queries, parallel database calls for efficiency, and correct input validation. The rate limiter, auth checks, and error handling are all in place.
src/api/index.js (2)
60-62: Audit middleware placement is correct.Placing
auditLogMiddleware()after all route mounts ensures it intercepts mutating requests across all authenticated routes. The middleware checksres.on('finish')so it captures outcomes without blocking.
53-55: Comment is factually incorrect —auditLogRouteris mounted afterguildsRouter, not before.The comment states "mounted before guilds catch-all" but the actual mounting order is:
- Line 48:
guildsRouterunder/guilds- Line 55:
auditLogRouterunder/guildsAdditionally,
guildsRouterdoes not have a catch-all route. Its routes are all explicit (e.g.,/:id,/:id/channels,/:id/roles, etc.). The current mount order works becauseguildsRouterlacks a route matching/:id/audit-log, allowingauditLogRouterto handle it. However, the comment's explanation of why this ordering matters is misleading.tests/api/routes/auditLog.test.js (1)
1-231: Comprehensive test coverage for the audit log route.Good coverage of authentication, pagination, filtering, and error handling scenarios. The mock setup is clean and tests properly verify SQL query construction.
web/src/app/dashboard/audit-log/page.tsx (2)
94-101: Good cleanup patterns for debounce timer and abort controller.Proper cleanup of the timeout in the debounce effect and aborting in-flight requests on unmount prevents memory leaks and race conditions.
Also applies to: 103-107
265-271: Good accessibility implementation.The input has proper
aria-label, and table rows support keyboard navigation with Enter/Space handlers. This ensures the expandable rows are accessible to keyboard users.Also applies to: 362-372
|
| Filename | Overview |
|---|---|
| migrations/015_audit_logs.cjs | Creates audit_logs table with proper indexes for guild_id and created_at queries |
| src/api/middleware/auditLog.js | Non-blocking audit middleware with action derivation and config diff tracking; insufficient sensitive field masking |
| src/api/routes/auditLog.js | Paginated audit log endpoint with proper filtering, auth, rate limiting, and error handling |
| src/api/utils/configAllowlist.js | Added auditLog to SAFE_CONFIG_KEYS; maskSensitiveFields only covers two triage API keys |
| web/src/app/dashboard/audit-log/page.tsx | Filterable audit log dashboard with proper locale handling, debounced search, and expandable rows |
Sequence Diagram
sequenceDiagram
participant User
participant Dashboard
participant NextAPI as Next.js API
participant BotAPI as Bot API
participant Middleware as Audit Middleware
participant DB as PostgreSQL
User->>Dashboard: View audit log page
Dashboard->>NextAPI: GET /api/guilds/:id/audit-log
NextAPI->>NextAPI: authorizeGuildAdmin
NextAPI->>BotAPI: GET /guilds/:id/audit-log
BotAPI->>BotAPI: requireGuildAdmin
BotAPI->>DB: SELECT with filters
DB-->>BotAPI: Return entries + total
BotAPI-->>NextAPI: JSON response
NextAPI-->>Dashboard: Proxy response
Dashboard-->>User: Display audit table
Note over User,DB: Admin performs mutating action
User->>Dashboard: Update config/member/etc
Dashboard->>NextAPI: PUT/POST/PATCH/DELETE
NextAPI->>BotAPI: Forward request
BotAPI->>Middleware: Intercept mutating request
Middleware->>Middleware: Derive action, extract guild
Middleware->>BotAPI: Call next() (non-blocking)
BotAPI->>BotAPI: Process request
BotAPI-->>Middleware: Response finished
Middleware->>Middleware: maskSensitiveFields
Middleware->>DB: INSERT audit_logs (fire-and-forget)
BotAPI-->>NextAPI: Response
NextAPI-->>Dashboard: Response
Note over DB: Scheduler cleanup (every 6 hours)
DB->>DB: DELETE old entries > retentionDays
Last reviewed commit: d2ab87f
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
5 warnings, 1 nitpick remaining after the fixes in 4dbcdc2.
The middleware ordering, React Fragment key, and rate limiting issues from the earlier round are fixed. However, several issues remain that should be addressed before merge.
🟡 Warnings
-
Query string pollutes action derivation and target extraction (
src/api/middleware/auditLog.js:168, 191, 217-220):req.originalUrlincludes the query string.deriveAction()and the target extraction split on/without stripping?..., producing corrupted action names (e.g.,config?limit=25.update) and target IDs. Usereq.pathfor all path-parsing; keepreq.originalUrlonly indetails.path. -
getConfig()called without guild ID (src/api/middleware/auditLog.js:154, 201): Per project conventions,getConfig(guildId)should be used to check guild-specificauditLog.enabledoverrides and to capture correct before/after config snapshots for diffs. -
Missing standalone
created_atindex (migrations/015_audit_logs.cjs:28-31): The retention purge query deletes bycreated_atalone withoutguild_id. The composite index(guild_id, created_at DESC)won't be used, causing sequential scans. -
retentionDaysnot validated (src/modules/scheduler.js:221): Non-numeric config values would cause repeated SQL errors every 6-hour cleanup cycle. Coerce and validate before passing tomake_interval. -
Missing
auditLogvalidation schema (src/api/utils/configValidation.js): TheauditLogkey is inSAFE_CONFIG_KEYS(writable via API) but has no type validation inCONFIG_SCHEMA, allowing invalid values to be persisted. -
Documentation not updated (AGENTS.md, README): Per project convention "Keep docs up to date — this is non-negotiable", AGENTS.md Key Files, Database Tables, and README config reference need updating.
🔵 Nitpick
maskSensitiveFieldsis config-specific (src/api/middleware/auditLog.js:195): Only handles known config paths. General-purpose sensitive field names (password,token,secret) in non-config request bodies are stored unredacted.
Prompt to fix all issues
Fix the following issues in the volvox-bot repo on branch feat/audit-log:
1. FILE: src/api/middleware/auditLog.js
ISSUE: req.originalUrl includes query strings, corrupting action names and target IDs.
FIX: On line 167-168, change `extractGuildId(req.originalUrl || req.path)` to `extractGuildId(req.path)` and `deriveAction(req.method, req.originalUrl || req.path)` to `deriveAction(req.method, req.path)`.
On lines 217-220, change `(req.originalUrl || req.path)` to `req.path` for pathSegments extraction.
Keep `req.originalUrl` only on line 191 in `details.path` for display purposes.
2. FILE: src/api/middleware/auditLog.js
ISSUE: getConfig() called without guild ID on lines 154 and 201.
FIX: Extract guildId earlier (before the config check) using `const guildId = extractGuildId(req.path);`. Then change line 154 from `const config = getConfig();` to `const config = getConfig(guildId || undefined);`. Change line 201 from `const afterConfig = getConfig();` to `const afterConfig = getConfig(guildId || undefined);`. Move the existing guildId assignment on line 167 to use the already-extracted value.
3. FILE: migrations/015_audit_logs.cjs
ISSUE: Missing standalone created_at index for retention purge.
FIX: After the existing CREATE INDEX statement (line 31), add:
await pool.query(`CREATE INDEX IF NOT EXISTS idx_audit_logs_created_at ON audit_logs(created_at);`);
4. FILE: src/modules/scheduler.js
ISSUE: retentionDays not validated before SQL use.
FIX: Change line 221 from `const retentionDays = config.auditLog?.retentionDays ?? 90;` to:
const parsed = Number(config.auditLog?.retentionDays ?? 90);
const retentionDays = Number.isFinite(parsed) && parsed > 0 ? parsed : 90;
5. FILE: src/api/utils/configValidation.js
ISSUE: Missing auditLog schema in CONFIG_SCHEMA.
FIX: Add to CONFIG_SCHEMA (after the last entry, before the closing brace):
auditLog: {
type: 'object',
properties: {
enabled: { type: 'boolean' },
retentionDays: { type: 'number' },
},
},
6. FILES: AGENTS.md, README.md
ISSUE: New files and database table not documented.
FIX: In AGENTS.md Key Files table, add entries for:
- src/api/middleware/auditLog.js | Audit log middleware — intercepts mutating requests, records non-blocking audit entries
- src/api/routes/auditLog.js | Audit log API route — paginated, filterable audit log retrieval
- web/src/app/dashboard/audit-log/page.tsx | Audit log dashboard page with filters, pagination, expandable details
- web/src/app/api/guilds/[guildId]/audit-log/route.ts | Next.js API route — proxies audit log requests to bot API
- migrations/015_audit_logs.cjs | Migration — creates audit_logs table
In AGENTS.md Database Tables section, add:
- audit_logs | Admin action audit trail — guild_id, user_id, action, target, details (JSONB), IP, timestamp
In README.md config reference section, add auditLog with enabled (boolean) and retentionDays (number, default 90).
Additional Comments (1)
|
Mount audit log route and middleware in the API router. Add 'auditLog' to SAFE_CONFIG_KEYS for dashboard configurability. Add auditLog section to config.json with enabled:true and retentionDays:90. Refs #123
Purge audit log entries older than configured retentionDays every 6 hours (360th scheduler tick). Uses make_interval for safe parameterized date arithmetic. Refs #123
Add audit log dashboard page with searchable table, action/user/date filters, expandable detail rows, and pagination. Add Next.js API proxy route. Add 'Audit Log' entry with ClipboardList icon to sidebar nav. Refs #123
API route tests cover auth, pagination, filtering (action, userId, date range), combined filters, 503 on missing DB, and 500 on DB error. Middleware tests cover action derivation, config diff computation, non-blocking behavior, response status gating, and enable/disable. Refs #123
…ing, React key prop, rate limiting
- Move auditLogMiddleware before routes so it actually executes (was dead code at the bottom)
- Add req._auditLogAttached flag to prevent duplicate execution across multiple /guilds mounts
- Import maskSensitiveFields from configAllowlist.js; mask req.body and configDiff before storing
- Reuse already-fetched config snapshot for beforeConfig (eliminates extra getConfig() call)
- Guard insertAuditEntry pool.query() with thenable check to prevent unhandled rejections in tests
- Fix React Fragment missing key prop: switch <> to <React.Fragment key={entry.id}> in .map()
- Note: rate limiting already applied at route level (auditRateLimit in auditLog.js)
- Update guilds test: account for one getConfig() call consumed by auditLogMiddleware
- Run pnpm format (fixes pre-existing formatting in configValidation.js, welcomeOnboarding.js)
- Extract cleanPath once (split on '?') to prevent query strings from corrupting action names, guild IDs, and target IDs - Use getConfig(guildId) instead of global getConfig() for before/after config diff snapshots so the diff reflects the actual guild config
BillChirico
force-pushed
the
feat/audit-log
branch
from
8c9f37b to
70a98ae
Compare
…purgeExpiredAuditLogs
Additional Comments (1)
|
Audit Log System
Complete implementation of audit logging for all admin actions in the web dashboard with attribution.
What's Included
Migration (
015_audit_logs.cjs): Createsaudit_logstable with guild_id, user_id, action, target_type, target_id, details (jsonb), ip_address, created_at. Indexed on (guild_id, created_at DESC).Audit Middleware (
src/api/middleware/auditLog.js): Non-blocking Express middleware that intercepts POST/PUT/PATCH/DELETE on authenticated routes. Captures user, action, target, IP, and request body. Computes before/after config diffs on config updates. Fire-and-forget DB insert.API Route (
src/api/routes/auditLog.js):GET /api/v1/guilds/:id/audit-log— paginated (limit/offset), filterable by action, userId, startDate, endDate. Rate-limited, guild admin auth required.API Router Integration: Mounted audit log route and middleware in
src/api/index.js.Dashboard Page (
web/src/app/dashboard/audit-log/page.tsx): Searchable table with action/user/date filters, expandable detail rows, and pagination.Dashboard API Proxy (
web/src/app/api/guilds/[guildId]/audit-log/route.ts): Next.js API route proxying to bot API.Sidebar Navigation: Added "Audit Log" entry with ClipboardList icon.
Config: Added
auditLog: { enabled: true, retentionDays: 90 }to config.json. Added'auditLog'toSAFE_CONFIG_KEYS.Retention Cleanup: Integrated into scheduler — purges entries older than retentionDays every 6 hours (360th tick).
Tests: 29 tests covering API route (auth, pagination, filtering, error handling) and middleware (action derivation, config diff, non-blocking behavior, enable/disable).
Test Results
Closes #123