75 lines
3.4 KiB
Markdown
75 lines
3.4 KiB
Markdown
# Fix WS Control-Permission Bypass for Guests
|
|
|
|
Priority: **High** · Effort: **Trivial (one location)** · Risk: **Low**
|
|
|
|
## Problem
|
|
|
|
Guests are *intended* to be unable to control playback (`routes/helpers.ts:30`:
|
|
|
|
```ts
|
|
if (user.is_guest && permission === "control") return false;
|
|
```
|
|
|
|
) but the **WebSocket message handler** does not route through `userHasPermission`. It re-implements the check inline and **omits the guest guard**:
|
|
|
|
```ts
|
|
// websocket.ts:87-93
|
|
const canControl = user.is_admin
|
|
|| config.defaultPermissions?.includes("control")
|
|
|| hasPermission(userId, "channel", ws.data.channelId, "control");
|
|
if (!canControl) { ... return; }
|
|
```
|
|
|
|
Because the default config ships `defaultPermissions: ["listen", "control"]` (`config.json:5-8`), **any guest can `pause`, `unpause`, `seek`, and `jump` on any channel**, affecting all listeners.
|
|
|
|
The HTTP control endpoints (`routes/channels.ts:176, 196, 216, 261`) correctly use `userHasPermission`, so this is an inconsistency between the two code paths that reach the same `Channel` mutators.
|
|
|
|
## Affected Location
|
|
|
|
- `websocket.ts:86-93` — inline permission check, guest-unaware.
|
|
|
|
## Root Cause
|
|
|
|
Duplicated permission logic in two places. The WS handler was written before / diverged from `helpers.userHasPermission`.
|
|
|
|
## Implementation Plan
|
|
|
|
### Step 1 — Import `userHasPermission`
|
|
|
|
At the top of `websocket.ts`:
|
|
|
|
```ts
|
|
import { userHasPermission } from "./routes/helpers";
|
|
```
|
|
|
|
### Step 2 — Replace the inline check
|
|
|
|
Replace `websocket.ts:86-93` with:
|
|
|
|
```ts
|
|
if (!userHasPermission(user, "channel", ws.data.channelId, "control")) {
|
|
console.log("[WS] User lacks control permission:", user.username, "(guest=" + user.is_guest + ")");
|
|
return;
|
|
}
|
|
```
|
|
|
|
`userHasPermission` already encapsulates: admin bypass, the guest `control` denial (`helpers.ts:30`), the `defaultPermissions` config check, and the DB permission lookup. Routing through it makes WS and HTTP behavior identical.
|
|
|
|
### Step 3 — Verify no other call sites rely on the old behavior
|
|
|
|
`grep` the repo for the inline pattern (`config.defaultPermissions?.includes("control")`) — it should now appear only in `helpers.ts:33` (the canonical check). Any other duplicate should be replaced the same way.
|
|
|
|
## Validation
|
|
|
|
- **Guest cannot control**: sign in as a guest (or hit the server unauthenticated with `allowGuests: true`), open a channel WS, send `{"action":"pause"}`. Confirm the channel does **not** pause and the server logs the "lacks control permission" line.
|
|
- **Non-guest user still controls**: sign in as `test`/`testuser`, send the same action, confirm the channel pauses.
|
|
- **Admin controls**: sign in as admin, confirm control works on channels the admin does not own.
|
|
- **Guest still receives state**: confirm the guest client still gets `track`/state broadcasts (i.e. the *listen* path is unaffected — only `control` is denied).
|
|
- **Config matrix**: temporarily set `defaultPermissions` to `["listen"]` only and confirm a *non-guest* non-admin user is now also denied control via WS (matches the HTTP path).
|
|
|
|
## Risk / Rollback
|
|
|
|
- The only behavioral change is denying control to guests (and, if `defaultPermissions` lacks `"control"`, to non-admin users) — which is the documented intent. If any deployment currently relies on guests controlling playback, that was an unintentional privilege and should be re-granted explicitly via the permissions table, not by reverting.
|
|
- No schema or DB change.
|
|
- Rollback = revert the single edit.
|