feat(meet): add tags mom versioning and room music#189
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!token || !repository) { | ||
| return NextResponse.json( | ||
| { | ||
| error: | ||
| "MoM versioning is not configured. Set MOM_GITHUB_TOKEN and MOM_GITHUB_REPOSITORY.", |
There was a problem hiding this comment.
This public handler accepts caller-supplied roomId and Markdown, then writes them with the server's GitHub token without checking a session or room-host role. Any caller who can reach the route can create or replace that day's minutes for another room.
| type="button" | ||
| onClick={() => { | ||
| finalizeTokenRef.current += 1; | ||
| setFinalizeState({ status: "aborted" }); |
There was a problem hiding this comment.
This action only invalidates the local completion token; it never cancels the in-flight request. If GitHub completes the write afterward, the UI reports that finalization was aborted even though the minutes were committed.
| } | ||
| return { | ||
| id: `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, | ||
| query, | ||
| title: query, | ||
| url: query, | ||
| requestedByUserId: options.userId, | ||
| requestedByDisplayName: options.displayName, | ||
| startedAt: Date.now(), | ||
| }; |
There was a problem hiding this comment.
A direct /play request accepts any HTTP or HTTPS URL and broadcasts it as an audio source to every participant. When room music is enabled, a participant can make attendees' browsers request private-network or state-changing endpoints; direct and resolved stream URLs need a compulsory default-deny host and network policy.
| const resolved = await resolveMusicTrack({ | ||
| query: playMatch[1] ?? "", | ||
| userId: sender.id, | ||
| displayName, | ||
| }); | ||
| if ("error" in resolved) { | ||
| rejectMessage(resolved.error); | ||
| return; | ||
| } | ||
| room.setCurrentMusicTrack(resolved); |
There was a problem hiding this comment.
Permission is checked before the awaited lookup but not before the resulting track is committed. If a host turns music off while lookup is pending, this continuation installs and broadcasts the track after the off transition, so music can start despite the current room policy.
| if (Array.isArray(response.participantTags)) { | ||
| for (const entry of response.participantTags) { | ||
| if (!entry?.userId || !Array.isArray(entry.tags)) continue; | ||
| dispatchParticipants({ | ||
| type: "UPDATE_TAGS", | ||
| userId: entry.userId, | ||
| tags: entry.tags, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The join snapshot dispatches UPDATE_TAGS before existing peers are guaranteed to have participant entries. Because the reducer ignores updates for missing users, a newly joined client can permanently omit existing participants' tags until another tag change is broadcast.
Artifacts
Repro: focused executable join snapshot ordering test
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: verbose state-transition output and failing assertion
- Keeps the command output available without making the summary code-heavy.
| const departingUserKey = this.userKeysById.get(clientId); | ||
| this.userKeysById.delete(clientId); | ||
| this.handRaisedByUserId.delete(clientId); | ||
| this.participantTagsByUserId.delete(clientId); |
There was a problem hiding this comment.
Once the disconnect grace period expires, this deletes every host-assigned tag for the participant. If the same member rejoins later, the server has no identity-keyed assignment to restore, so tag-based moderation silently omits that member.
Artifacts
Repro: executable Room lifecycle harness
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: verbose before, disconnect, and same-identity rejoin output
- Keeps the command output available without making the summary code-heavy.
theg1239
left a comment
theg1239
left a comment
There was a problem hiding this comment.
Review: participant tags, MoM versioning, and room music
Thanks for the PR. All three features are genuinely useful, and the SFU-side plumbing is in good shape: permissions are enforced server-side through the existing ensureAdminRoom and applyRoomPolicyUpdate patterns, tag normalization is solid, and room teardown resets all the new state. Every finding below was verified against the PR head (113afd6), so none of it is speculative.
That said, there are two security issues and four state bugs that need to be resolved before this merges.
Must fix: security
1. /api/conclave/mom/finalize is completely unauthenticated
apps/web/src/app/api/conclave/mom/finalize/route.ts:21
Any caller who can reach the deployment can commit arbitrary markdown (up to 512 KB) to the configured GitHub repo using the server's token. There is no session check, no room membership check, and no rate limit. Concretely, an anonymous caller can:
- overwrite any room's minutes, since
roomIdis caller-supplied - spam commits into the repo history, which is not reversible
- exhaust the token's API quota
Two aggravators:
- The raw
roomId(notsafeRoomId) is interpolated into the commit message, so crafted or multiline commit messages are possible. - GitHub error bodies are echoed back to the anonymous caller (
detail: text.slice(0, 500)), leaking repo and config details.
The app already has better-auth installed, and /api/unfurl demonstrates the project's rate-limit pattern; both belong here. Since a web session alone does not prove the caller is in the room, the strongest fix is an SFU-attested token (there is precedent in the conclave:authorize signed-token flow). The Finalize button should also be host-gated in the UI: right now canExport is just hasExportContent && !isViewOnly (TranscriptPanel.tsx:989), so any non-view-only participant can trigger a commit.
2. /play broadcasts arbitrary URLs that every participant's browser fetches
packages/sfu/server/socket/handlers/chatHandlers.ts:988 plus the <audio src preload="auto"> in MeetingMusicPlayer.tsx
With music set to "everyone", any participant can make every attendee's browser request an attacker-chosen URL: tracking, intranet probing from each attendee's network position, or bandwidth burn. Also, allowing http: is pure downside: the app is served over HTTPS, so browsers block those as mixed content and the player just shows a misleading "Tap to start".
Minimum fix: require https:. Better: restrict direct-URL playback to hosts, or check URLs against an allowlist of known audio hosts, while keeping free-text search (which resolves through the configured Piped instance) available to everyone.
Must fix: correctness
3. A pending /play resurrects music after the host turns it off
Permission is checked (chatHandlers.ts:990-1008), then the code awaits resolveMusicTrack(...), then calls room.setCurrentMusicTrack(resolved) unconditionally (line 1019). setMusicPermission("off") clears the track, but an in-flight lookup re-installs and re-broadcasts it afterwards. Re-check room.musicPermission after the await, and also confirm the sender is still in the room, since a kicked user's pending request currently still lands.
4. The initial tag snapshot is dropped for every new joiner
In useMeetSocket.ts, the join handler dispatches UPDATE_TAGS (head line 4700) before applyDisplayNameSnapshot creates the participant entries (line 4740), and the reducer ignores updates for unknown users (participant-reducer.ts: if (!participant) return state). A joining client therefore shows no tags for existing members until some later tag change happens to broadcast. Move the participantTags dispatch after applyDisplayNameSnapshot, or make UPDATE_TAGS upsert.
5. Tags do not survive disconnect and rejoin
Room.removeClient deletes the user's entry in participantTagsByUserId once the grace period expires (Room.ts:336), and tags are keyed by the transient client id. The room already maintains a stable identity for exactly this situation: userKeysById, which is how displayNamesByKey survives. Key tags by userKey so that "Remove all @jc" still covers a member who refreshed five minutes ago. As written, the moderation feature silently loses its targets.
6. /play inside a DM goes room-wide
The TTS branch guards with !directMessageIntent, but the play branch matches on messageContent, which for a DM is the private message body (chatHandlers.ts:988). A private "/play x" starts music for the whole room and posts a public "Music: X played ..." chat line. Add the same !directMessageIntent guard.
Related edge case: /play sent with an image attachment marks the image attached (line 978) and then drops it from the emitted message, orphaning the asset until room teardown.
7. Abort is cosmetic
TranscriptPanel.tsx:1262: Abort bumps the local token and shows "aborted", but the fetch and the GitHub commit proceed. Wire an AbortController into the fetch, and since the server-side PUT can still complete after the client disconnects, consider wording it "stopped waiting" rather than "aborted".
Recommended
- Default tags are duplicated three times:
["host","jc","sc","boards","guest"]appears inRoom.ts(DEFAULT_ROOM_TAGS),useMeetState.ts, and as aParticipantsPaneldefault parameter. Single-source it inmeeting-core. Also notejc/sc/boardsare org-specific values hardcoded into generic room code; per-room config would be cleaner if this is meant to generalize. - No playback position sync:
startedAtis stored but never used, so late joiners and autoplay-blocked users start at 0:00, andloopmakes the drift permanent. Fine for v1, but worth a known-limitation note since the PR description says "shared music playback". - "Remove @tag" buttons always render, even when nobody holds the tag, and the returned
removedCountis never surfaced, so a click can look like a no-op. availableParticipantTagsonly grows: every tag ever assigned joins the room's set with no removal path. Bounded by host behavior, but trivial to cap.- Mobile parity:
apps/mobilekeeps its own chat-command list (no/playautocomplete) and has no music player or tags UI, so native users hear nothing while web users hear music. Worth a follow-up issue if not handled here. - The async-IIFE wrap of the whole
sendMessagehandler without re-indenting keeps this diff small, but the file's indentation now misrepresents scope and the formatter will churn it on the next touch.
What's done well
- Music permission and tag mutations are enforced server-side and follow the established admin-handler conventions, including no-op suppression and control-plane snapshot exposure.
- Server-side tag normalization (lowercase, restricted charset, 24-char cap) and a full state reset on room teardown.
- Slow mode is server-enforced with admins exempt; Piped resolution validates stream URLs and picks by bitrate, with user-friendly errors.
- The join-callback dependency array fix (adding previously missing setters) is a nice drive-by correctness improvement.
- The reducer's sorted-equality check avoids no-op re-renders.
Tests and verification
No tests were added. packages/meeting-core already has chat-commands.test.ts, so /play parsing and the UPDATE_TAGS reducer branch are cheap, high-value additions. The PR notes that web lint and typecheck never ran locally (pnpm install timeouts). I spot-checked the risky imports (Volume2, Loader2, CheckCircle2, and the type re-exports) and they resolve, but CI should confirm compilation before merge.
Suggested path to merge
Items 1-4 are must-fix (two security, two correctness). Item 5 is strongly recommended since tag persistence is the feature's core promise. Items 6-7 are quick fixes. The three features are independent, so if the MoM auth story needs design time, splitting it out would let tags and music land sooner.
🤖 Generated with Claude Code
|
Superseded by #195, which is rebased on latest main and scoped to MoM versioning + room music only. |
2 participants
Summary
/playhandling, slow mode, shared music playback, and speech ducking to 35%Configuration
MUSIC_PIPED_API_BASEenables open-source Piped-compatible music search; direct audio URLs work without itMOM_GITHUB_TOKEN/GITHUB_TOKEN,MOM_GITHUB_REPOSITORY, optionalMOM_GITHUB_BRANCH, and optionalMOM_GITHUB_BASE_PATHenable MoM commitsVerification
git diff --check --cachedpassedeslintpassed for modified shared/SFU filespnpm installrepeatedly timed out fetching registry packages (dashjs,next,@next/swc-win32-x64-msvc), leavingeslint-plugin-react-hooksunavailable locallyspent a lot of water and tokens to review your slop
Greptile Summary
This PR adds participant groups, shared room music, and GitHub-backed meeting minutes. The main changes are:
/playresolution, slow mode, playback, and speech ducking.Confidence Score: 2/5
The repository-write authorization and arbitrary playback URL paths need fixes before merging.
The MoM route lets reachable callers write with a shared GitHub credential. Music URLs can trigger requests from every participant browser without a host boundary. Pending lookups can restore music after it is disabled. Tag synchronization can lose initial or rejoined participant assignments.
apps/web/src/app/api/conclave/mom/finalize/route.ts, packages/sfu/server/socket/handlers/chatHandlers.ts, apps/web/src/app/hooks/useMeetSocket.ts, packages/sfu/config/classes/Room.ts
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant Web participant SFU participant Peers participant MoM as MoM API participant GitHub User->>SFU: /play URL or search SFU->>SFU: Check permission SFU->>SFU: Resolve track asynchronously SFU->>Peers: musicStateChanged Peers->>Peers: Load audio URL User->>Web: Finalize minutes Web->>MoM: POST roomId and Markdown MoM->>GitHub: Read current file SHA MoM->>GitHub: Commit Markdown GitHub-->>Web: Commit result%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant Web participant SFU participant Peers participant MoM as MoM API participant GitHub User->>SFU: /play URL or search SFU->>SFU: Check permission SFU->>SFU: Resolve track asynchronously SFU->>Peers: musicStateChanged Peers->>Peers: Load audio URL User->>Web: Finalize minutes Web->>MoM: POST roomId and Markdown MoM->>GitHub: Read current file SHA MoM->>GitHub: Commit Markdown GitHub-->>Web: Commit resultReviews (1): Last reviewed commit: "feat(meet): add tags mom versioning and ..." | Re-trigger Greptile