fix: enforce guild-scoped auth for audit log websocket#265
Merged
BillChirico merged 1 commit intoVolvoxLLC:mainfrom Mar 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request enhances WebSocket ticket authentication with guild-scoped security. The HMAC ticket format is extended to include guildId, server-side tenant scoping is enforced, and backward compatibility is maintained for legacy tickets while tests are extended to validate cross-guild isolation. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
| Filename | Overview |
|---|---|
| src/api/ws/auditStream.js | Core security fix: validateTicket now returns { valid, guildId } from a 4-part ticket; ws.guildId is stored on auth; handleFilter rejects cross-guild filter attempts; matchesFilter enforces guild isolation on both the no-filter and filtered paths. Logic is sound with one minor defense-in-depth gap in the filter-present path. |
| src/api/ws/logStream.js | Backward-compatible ticket parsing added — accepts both legacy 3-part (nonce.expiry.hmac) and new 4-part (nonce.expiry.guildId.hmac) tickets. Log stream itself intentionally does not enforce guild scoping. Logic and HMAC payload derivation are correct for both formats. |
| tests/api/ws/auditStream.test.js | Good new cross-guild isolation tests added (no-filter drop + mismatched filter rejection). However, the pre-existing "should NOT broadcast to client with non-matching guildId filter" test (line 278) is broken by the new guild enforcement: the cross-guild filter is now rejected, leaving ws.auditFilter null, so a matching-guild entry is broadcast and the timeout expectation fails. |
| web/src/app/api/log-stream/ws-ticket/route.ts | createTicket updated to accept and bind guildId in the signed HMAC payload, matching the new 4-part ticket format. guildId is validated from query params and guild-admin authorization is checked before the ticket is issued. No issues found. |
| CLAUDE.md | Session notes appended documenting the security changes made in this PR. No code impact. |
Sequence Diagram
sequenceDiagram
participant C as Dashboard Client
participant R as ws-ticket Route
participant A as auditStream.js
C->>R: GET /api/log-stream/ws-ticket?guildId=G1
R->>R: authorizeGuildAdmin(request, G1)
R->>R: createTicket(secret, G1)<br/>HMAC over nonce.expiry.G1
R-->>C: { wsUrl, ticket: "nonce.expiry.G1.hmac" }
C->>A: WS connect /ws/audit-log
A->>A: handleConnection()<br/>ws.guildId = null
C->>A: { type: "auth", ticket }
A->>A: validateTicket()<br/>verify HMAC, extract guildId
A->>A: ws.authenticated = true<br/>ws.guildId = "G1"
A-->>C: { type: "auth_ok" }
C->>A: { type: "filter", guildId: "G2" }
A->>A: G2 ≠ ws.guildId (G1) → reject
A-->>C: { type: "error", message: "Guild filter does not match..." }
Note over A: broadcastAuditEntry(entry)
A->>A: matchesFilter(filter, entry, ws.guildId)<br/>no filter → entry.guild_id === "G1"?
A-->>C: { type: "entry", entry } only if entry.guild_id === "G1"
Comments Outside Diff (1)
-
tests/api/ws/auditStream.test.js, line 278-298 (link)Existing test broken by new guild enforcement
This test will now fail due to the new
handleFilterrejection logic introduced in this PR.Here's the sequence of events with the updated code:
makeTicket()defaults toguildId = 'guild1', sows.guildId = 'guild1'after auth.- The filter message sends
guildId: 'other-guild', which differs fromws.guildId.handleFilternow rejects this with{ type: 'error', ... }—ws.auditFilteris never set and staysnull. await q.next()on line 283 consumes the error response (notfilter_ok), but that's fine since the result is discarded.broadcastAuditEntry({ guild_id: 'guild1', ... })is called.matchesFilter(null, { guild_id: 'guild1' }, 'guild1')now hits the new!filterbranch and returnsentry.guild_id === authenticatedGuildId→'guild1' === 'guild1'→true.- The entry is broadcast, so
q.next(500)resolves instead of timing out — therejects.toThrow('Message timeout')expectation fails.
The test needs to be updated to use an entry from a different guild (e.g.
guild_id: 'other-guild') to verify it is not broadcast, which aligns with the new cross-guild isolation behavior already covered by the new test added in this PR:
Last reviewed commit: 2ffc2b6
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens multi-tenant security for the audit log WebSocket by binding authentication tickets (and downstream filtering/broadcast) to a specific guild context.
Changes:
- Updated audit-log WS auth to require guild-bound tickets and persist the authenticated
guildIdon the connection. - Enforced guild scoping on the server: mismatched filter requests are rejected and broadcasts are constrained to the authenticated guild even with no filter.
- Updated ticket generation/validation logic and expanded WS audit stream tests for cross-guild isolation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/app/api/log-stream/ws-ticket/route.ts | Includes guildId in the signed WS ticket payload returned to the client. |
| src/api/ws/auditStream.js | Requires guild-bound tickets, stores ws.guildId, enforces guild scoping in filters and broadcasts. |
| src/api/ws/logStream.js | Adds backward-compatible parsing for both legacy 3-part and new 4-part tickets. |
| tests/api/ws/auditStream.test.js | Updates ticket helper format and adds tests covering cross-guild isolation and mismatched filter rejection. |
| CLAUDE.md | Documents session notes describing the security change and test updates. |
Comments suppressed due to low confidence (1)
src/api/ws/auditStream.js:17
- The protocol header comment lists server→client messages as
auth_ok,entry, anderror, but the implementation also sendsfilter_okinhandleFilter(). Since this block was touched while updating the ticket format, please update it to includefilter_ok(and ideally note that guildId in filters is constrained to the authenticated guild).
* Protocol (JSON messages):
* Client → Server:
* { type: 'auth', ticket: '<nonce>.<expiry>.<guildId>.<hmac>' }
* { type: 'filter', guildId: '...', action: '...', userId: '...' }
*
* Server → Client:
* { type: 'auth_ok' }
* { type: 'entry', entry: { ... } }
* { type: 'error', message: '...' }
*/
💡 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/ws/logStream.js`:
- Around line 205-220: validateTicket() currently accepts 4-part tickets but
discards the guildId and only returns a boolean; update validateTicket() to
return an object like { valid: boolean, guildId: string|null } and ensure it
extracts and returns the verified guildId (use the existing nonce/expiry/hmac
logic but include guildId in the returned payload when parts.length === 4). In
handleAuth(), capture the returned guildId and set it on the WebSocket (e.g.
ws.guildId = guildId) so subsequent logic can scope the session. Use that
ws.guildId when calling queryLogs({ limit: HISTORY_LIMIT }) to filter history
and ensure wsTransport.addClient(ws) / WebSocketTransport.passesFilter() receive
or read the guild context so broadcasts are guild-scoped (pass guildId into
filter calls or rely on ws.guildId inside passesFilter()). Ensure tests/usage of
validateTicket(), handleAuth(), queryLogs(), wsTransport.addClient(), and
WebSocketTransport.passesFilter() are updated to expect and handle the guildId.
In `@tests/api/ws/auditStream.test.js`:
- Around line 373-386: The earlier test "should NOT broadcast to client with
non-matching guildId filter" conflicts with the new rejection behavior; update
that test so it expects an error when sending a mismatched guildId instead of
awaiting 'filter_ok'. Specifically, in that test (the one using sendJson(ws, {
type: 'filter', guildId: 'other-guild', ... }) and consuming messages via
q.next() / createMessageQueue), change the assertion that awaited 'filter_ok' to
assert msg.type === 'error' and the message contains the same 'Guild filter does
not match authenticated guild' text (or adjust the guildId to match the
authenticated ticket if you want to preserve the original flow), ensuring you
update the sendJson/q.next assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ee32807-1d28-4555-9c8c-59b7650c5a3f
📒 Files selected for processing (5)
CLAUDE.mdsrc/api/ws/auditStream.jssrc/api/ws/logStream.jstests/api/ws/auditStream.test.jsweb/src/app/api/log-stream/ws-ticket/route.ts
📜 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). (2)
- GitHub Check: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes — No double quotes except in JSON
Semicolons are always required
Files:
src/api/ws/auditStream.jsweb/src/app/api/log-stream/ws-ticket/route.tstests/api/ws/auditStream.test.jssrc/api/ws/logStream.js
**/*.{js,ts,tsx,jsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indent — Biome enforced
Files:
src/api/ws/auditStream.jsweb/src/app/api/log-stream/ws-ticket/route.tsCLAUDE.mdtests/api/ws/auditStream.test.jssrc/api/ws/logStream.js
src/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,tsx}: Use Winston logger fromsrc/logger.js, NEVERconsole.*
Use safe Discord message methods —safeReply()/safeSend()/safeEditReply()
Use parameterized SQL — Never string interpolation in queries
Files:
src/api/ws/auditStream.jssrc/api/ws/logStream.js
tests/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests required with 80% coverage threshold, never lower it
Files:
tests/api/ws/auditStream.test.js
🧠 Learnings (7)
📚 Learning: 2026-03-08T04:37:50.316Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.316Z
Learning: Read `CLAUDE.md` before doing anything else and update it after completing infrastructure work with technical decisions and session notes
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T02:15:42.321Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T02:15:42.321Z
Learning: Applies to web/src/routes/dashboard/**/*.{ts,tsx} : Server-rendered dashboard entry pages should include static metadata for SEO and consistency with client-side title updates
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T02:15:42.321Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T02:15:42.321Z
Learning: Applies to web/src/lib/page-titles.ts : Dashboard browser titles should sync with route changes using centralized title helpers with the canonical app title `Volvox.Bot - AI Powered Discord Bot`
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T02:15:42.321Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T02:15:42.321Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard shell layout component should mount `DashboardTitleSync` to sync `document.title` on pathname changes for client-rendered pages
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T04:37:50.316Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.316Z
Learning: Applies to web/src/app/dashboard/**/*.{ts,tsx} : New dashboard routes must add a matcher entry to `dashboardTitleMatchers` in `web/src/lib/page-titles.ts` with exact equality for leaf routes and subtree check for subtrees
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T02:15:42.321Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T02:15:42.321Z
Learning: Applies to web/tests/components/dashboard/**/*.test.{ts,tsx} : Web dashboard config editor tests should cover category switching, search functionality, dirty badges, and explicit manual-save workspace behavior instead of autosave assumptions
Applied to files:
CLAUDE.md
📚 Learning: 2026-03-08T04:37:50.316Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.316Z
Learning: Applies to web/src/app/dashboard/page.{ts,tsx} : Dashboard SSR entry points must export `metadata` using `createPageMetadata()` from `web/src/lib/page-titles.ts`
Applied to files:
CLAUDE.md
🔇 Additional comments (1)
src/api/ws/auditStream.js (1)
157-168: Server-side guild binding closes the no-filter bypass.Binding
ws.auditFilter.guildIdtows.guildIdand then checking the authenticated guild again inbroadcastAuditEntry()is the right place to enforce tenant isolation, even when the client omits a filter.Also applies to: 245-267
BillChirico
pushed a commit
that referenced
this pull request
Mar 10, 2026
Thread 1: Pass guildId query param to ws-ticket endpoint - useLogStream() now accepts guildId as first param - fetchTicket appends ?guildId=<id> to the ws-ticket request - Connection is skipped until guildId is non-null - logs/page.tsx uses useGuildSelection() to get guildId and passes it Thread 2: Update JSDoc for matchesFilter in auditStream.js - Add @param for authenticatedGuildId - Clarify 'no filter' behavior (falls back to guild_id === authenticatedGuildId) Thread 3-5: Fix contradicting test in auditStream.test.js - 'should NOT broadcast to client with non-matching guildId filter' was sending guildId:'other-guild' and expecting filter_ok — but the new server logic returns an error for mismatched guild filters - Replaced with 'should NOT broadcast cross-guild entries to a guild1-authenticated client' which broadcasts an entry from 'other-guild' and verifies the guild1-authenticated client does NOT receive it
|
This was referenced Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Please describe your changes in detail.
Issue ticket number and link
What type of PR is this? (check all applicable)