mirror of
https://github.com/Shik3i/KoalaSync.git
synced 2026-08-18 15:23:13 +00:00
docs+test: move KNOWN_LIMITATIONS to docs/, add everyone-mode regression test, co-host plan
- Move KNOWN_LIMITATIONS.md → docs/KNOWN_LIMITATIONS.md (+ fix SECURITY.md link). - WS test: in default 'everyone' mode a non-host guest can still drive play/pause/seek/force-sync/episode-lobby — proves host-control OFF == unchanged behavior. - docs/host-control-mode-COHOST-PLAN.md: plan for multi-controller (owner grants drive rights to several co-hosts), built on the capabilities hook; covers the gate generalization, roles, backwards-compat, edge cases, and a separate large-room (510-peer) scaling track. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
KoalaDev
@@ -0,0 +1,103 @@
|
||||
# Known Limitations & Accepted Non-Issues (NOFIX)
|
||||
|
||||
This file records design decisions that are **intentional** and **out of scope** for
|
||||
KoalaSync's threat model. They are frequently re-discovered by reviewers, linters, and
|
||||
AI agents and re-reported as "bugs" or "vulnerabilities." They are not. Please read the
|
||||
threat model below before flagging any of them.
|
||||
|
||||
> **For reviewers & agents:** Do **not** re-report the NOFIX items below. They are
|
||||
> deliberate. If you think the *threat model itself* should change (e.g. KoalaSync
|
||||
> starts handling accounts, payments, or sensitive data), raise **that** discussion
|
||||
> first — these items only stand because the threat model holds.
|
||||
|
||||
---
|
||||
|
||||
## Threat model
|
||||
|
||||
KoalaSync is a **privacy-preserving watch-party sync tool**, not a security product.
|
||||
|
||||
- **Rooms are ephemeral.** They exist for a few hours and are auto-reaped. There are no
|
||||
accounts, no persistent storage, no money, and no sensitive content on the relay.
|
||||
- **The relay is a dumb, stateless message bus.** It forwards play/pause/seek between
|
||||
peers who *chose* to watch together and joined via an invite link shared out-of-band.
|
||||
- **Participants are invited.** Anyone in a room was let in. The social contract is
|
||||
"we're watching a video together," not "mutually distrusting parties."
|
||||
|
||||
### What we DO defend against
|
||||
- **Accidental disruption** — the entire point of Host Control Mode.
|
||||
- **Spam / DoS** that degrades the relay for everyone — rate limits, 4 KB payload cap,
|
||||
server-side gating, lazy-cached health responses.
|
||||
- **Resource exhaustion / memory leaks** — bounded maps, periodic cleanup, room/peer reaping.
|
||||
- **Crashes from malformed input** — strict sanitization and clamping of every field.
|
||||
- **Genuine boundary breaches** — admin-metrics auth (constant-time), CORS, WSS upgrade,
|
||||
invite-hash isolation, strict CSP. Reports here are very welcome (see `SECURITY.md`).
|
||||
|
||||
### What we explicitly DO NOT defend against
|
||||
A **determined participant who modifies their own client to misbehave inside a room they
|
||||
were invited to.** The worst they achieve is sending playback commands or seizing the
|
||||
"host" role in a temporary room they could already disrupt by other means. That is a
|
||||
**social** problem, solved socially: kick them, or start a new room. Engineering real
|
||||
identity/auth to prevent it would destroy the account-less, frictionless, privacy-first
|
||||
design — a bad trade for an ad-hoc movie night.
|
||||
|
||||
---
|
||||
|
||||
## NOFIX entries
|
||||
|
||||
### NOFIX-1 — `peerId` is unauthenticated; a crafted client can impersonate or seize the host
|
||||
**Flag:** `peerId` is client-asserted and broadcast to every peer (in `ROOM_DATA` /
|
||||
`PEER_STATUS`). A modified client can join with the host's `peerId`, dedupe-kick the real
|
||||
host, and become host — controlling or locking `host-only` mode.
|
||||
|
||||
**Why NOFIX:** Requires a *modified client* + an *invited* participant + a `peerId` that is
|
||||
only meaningful inside that one *temporary* room. The payoff is sending play/pause or
|
||||
locking a room the attacker is already in — pure trolling, instantly reversible (kick /
|
||||
new room). Cryptographic per-user identity is wildly disproportionate for an ad-hoc,
|
||||
account-less, ephemeral watch party. **Out of threat model.**
|
||||
Do **not** "fix" with accounts, signed peerIds, or per-user tokens — that breaks the
|
||||
core design.
|
||||
|
||||
### NOFIX-2 — Room-password comparison is not constant-time
|
||||
**Flag:** room password hashes are compared with `!==` (`server/index.js`), so the compare
|
||||
is theoretically timing-attackable.
|
||||
|
||||
**Why NOFIX:** The compared value is an **HMAC-SHA256 hash that never leaves the server** —
|
||||
an attacker cannot observe it to mount a timing attack. Even a hypothetical success only
|
||||
lets someone join a *temporary* room to send playback commands. Not worth defending.
|
||||
(The admin-metrics bearer token — a real boundary — **does** use `crypto.timingSafeEqual`.
|
||||
That is the line we actually guard.)
|
||||
|
||||
### NOFIX-3 — `OFFICIAL_SERVER_TOKEN` is public in the repo
|
||||
**Flag:** the connection token in `shared/constants.js` is committed, so anyone can connect.
|
||||
|
||||
**Why NOFIX:** It is a **coarse filter** to keep random scanners off the relay, **not
|
||||
authentication**. The relay is a public message bus by design; rate limits and per-room
|
||||
behavior are the real protections.
|
||||
|
||||
### NOFIX-4 — Room IDs are enumerable via `GET_ROOMS`
|
||||
**Flag:** any connected client can list all room IDs (and whether each has a password).
|
||||
|
||||
**Why NOFIX:** This is the intended **"Public Rooms"** feature. Rooms wanting privacy set a
|
||||
password; listing the IDs of password-less rooms only lets someone join a watch party —
|
||||
the same as being handed the invite link.
|
||||
|
||||
### NOFIX-5 — A pause/seek can only be reverted, not prevented
|
||||
**Flag:** in `host-only` mode a guest's pause still fires locally before the extension can
|
||||
react, so there is a brief flicker before snap-back.
|
||||
|
||||
**Why NOFIX:** A content script cannot intercept a `<video>` event before the element
|
||||
acts. Reacting (snap-back) is the only option and is by design; the ~½s flicker is
|
||||
acceptable. Not a bug.
|
||||
|
||||
---
|
||||
|
||||
## Not NOFIX — just deferred (may be revisited)
|
||||
|
||||
These are *not* accepted-forever; they are scoped out of v1 and tracked separately
|
||||
(see the host-control-mode design docs in `docs/`):
|
||||
|
||||
- **Host grace on a long disconnect (EC-10).** A brief reconnect/second-tab keeps the host
|
||||
(handled), but a long real disconnect still falls back to `everyone`. A ~30s host-reserve
|
||||
grace could be added later.
|
||||
- **Intent-classifier / snap-back tuning.** Thresholds are first-pass; real-device testing
|
||||
may adjust them.
|
||||
@@ -0,0 +1,118 @@
|
||||
# Co-Host (Multi-Controller) — Implementation Plan
|
||||
|
||||
Branch base: `feature/host-control-mode` (builds directly on it).
|
||||
Goal: let the room owner grant **playback control to several peers** (co-hosts), not
|
||||
just one — e.g. 4 of N people in a room may drive play/pause/seek, the rest are guests.
|
||||
|
||||
This is the second server-gated feature the `capabilities` hook was designed for
|
||||
(`CAPABILITIES.CO_HOST = 'co-host'`, already stubbed in `shared/constants.js`).
|
||||
|
||||
---
|
||||
|
||||
## 1. Roles
|
||||
|
||||
| Role | Can drive (play/pause/seek/force-sync/episode-lobby) | Can promote/demote + toggle mode |
|
||||
|------|------|------|
|
||||
| **Owner** (room creator, = today's "host") | yes (always a controller) | **yes** |
|
||||
| **Controller** (co-host) | yes (in `host-only` mode) | no |
|
||||
| **Guest** | only in `everyone` mode | no |
|
||||
|
||||
The single-host feature is just the special case `controllers = { owner }`.
|
||||
|
||||
## 2. Data model
|
||||
|
||||
### Server (`room` object)
|
||||
- `ownerPeerId` — the creator / manager. Keep `hostPeerId` as an **alias** (= ownerPeerId)
|
||||
so older clients keep working.
|
||||
- `controllers: Set<peerId>` — peers allowed to drive. **Always contains ownerPeerId.**
|
||||
- `controlMode: 'everyone' | 'host-only'` — unchanged wire values (`'host-only'` now means
|
||||
"restricted to controllers", not "single host").
|
||||
- `MAX_CONTROLLERS` cap (e.g. 10) to bound the set + payload.
|
||||
|
||||
### Shared constants
|
||||
- `CAPABILITIES.CO_HOST = 'co-host'` (un-stub it) → add to `SERVER_CAPABILITIES`.
|
||||
- New events:
|
||||
- `SET_PEER_ROLE` (client→server): `{ peerId, controller: boolean }` — owner promotes/demotes.
|
||||
- Extend `CONTROL_MODE` (server→client) payload: `{ controlMode, ownerPeerId, hostPeerId, controllers: [peerId...] }`.
|
||||
- `ROOM_DATA` gains `ownerPeerId` + `controllers`.
|
||||
|
||||
## 3. Gate generalization (the core change)
|
||||
|
||||
Today the gate compares against a single `hostPeerId`. Generalize to set membership:
|
||||
|
||||
- **Server relay gate** (`server/index.js`): `controlMode === 'host-only' && !room.controllers.has(mapping.peerId)` → drop. (Was `mapping.peerId !== room.hostPeerId`.)
|
||||
- **Background gates** (sender + receiver): replace `amHost()` / `senderId !== hostPeerId`
|
||||
with controller-set membership: `controllers.includes(myPeerId)` / `senderId ∈ controllers`.
|
||||
- **Helpers:** split `amHost()` into `amOwner()` (manage rights) and `amController()`
|
||||
(drive rights). The desync/snap-back path keys on `!amController()` instead of `!amHost()`.
|
||||
|
||||
`SET_PEER_ROLE` handler (server): validate sender is owner, target is a current peer in the
|
||||
room, enforce `MAX_CONTROLLERS`, always keep owner in the set, then broadcast `CONTROL_MODE`
|
||||
with the new `controllers`.
|
||||
|
||||
## 4. Client + UI
|
||||
|
||||
- **Owner** sees the peer list with a per-peer **"Controller" toggle** (promote/demote) plus
|
||||
the existing mode toggle.
|
||||
- **Controllers** see a "Controller" badge and are NOT locked out of the remote-control buttons.
|
||||
- **Guests** see "Guest" + the host-only notice (unchanged).
|
||||
- The promote UI + co-host badges render only when the relay advertises the `co-host`
|
||||
capability (feature detection, same pattern as `hostControlSupported`).
|
||||
- i18n: new keys (`ROLE_CONTROLLER`, `BTN_PROMOTE`, `BTN_DEMOTE`, …) across all locales.
|
||||
|
||||
## 5. Backwards compatibility
|
||||
|
||||
- **New client + old server** (host-control only, no `co-host` capability): no co-host UI;
|
||||
behaves as today's single-host. ✓
|
||||
- **Old client + new server**: ignores `controllers` / `SET_PEER_ROLE`. An old client that
|
||||
the owner promotes still **gates itself** (its sender-gate only knows `!amHost`), so it
|
||||
can't drive — it degrades to a guest. Co-host requires a client that understands
|
||||
`controllers`. Document this; not a crash. ✓
|
||||
- No `PROTOCOL_VERSION` bump needed — purely additive, same as host-control.
|
||||
|
||||
## 6. Edge cases
|
||||
- **Controller leaves** → `removePeerFromRoom` also does `room.controllers.delete(peerId)`.
|
||||
- **Owner leaves** → fallback: promote the earliest remaining **controller** to owner (prefer
|
||||
a controller over a random peer); if none, earliest peer; keep the rest of the set. Reuse
|
||||
the `peerJoinLocks` guard so a reconnect/second-tab doesn't demote (same fix as host).
|
||||
- **Promote a peer not in the room** → server rejects (target must be a live peer).
|
||||
- **Promote beyond `MAX_CONTROLLERS`** → server rejects, re-syncs the owner's UI.
|
||||
- **`everyone` mode** → the `controllers` set is still maintained (so flipping to `host-only`
|
||||
keeps the chosen co-hosts), it just isn't enforced while in `everyone`.
|
||||
- **peerId spoofing** → unchanged accepted limitation (see `docs/KNOWN_LIMITATIONS.md`);
|
||||
co-host doesn't widen it materially (still bounded to a temporary room).
|
||||
|
||||
## 7. Scale: the "4 of 510 people" part — read this
|
||||
|
||||
The role change above is moderate. **Putting 510 people in one room is a separate, larger
|
||||
problem** and should be its own track:
|
||||
|
||||
- `MAX_PEERS_PER_ROOM` is **25** today. 510 needs a large raise + load testing.
|
||||
- **The real bottleneck at scale is heartbeat fan-out, not control events.** Every peer
|
||||
heartbeats and the relay broadcasts each to all peers → O(N²) per interval. At 510 that's
|
||||
~510×509 / 15s ≈ **17k msg/s just for heartbeats** — the scaling wall.
|
||||
- **Co-host actually *helps* the control-event side:** in `host-only` mode only the few
|
||||
controllers emit play/pause/seek, so event *sources* drop from N to K (e.g. 4). Restricting
|
||||
who can drive is synergistic with big rooms.
|
||||
- Large rooms therefore need (independent of co-host):
|
||||
- **Heartbeat fan-out reduction** — e.g. only relay controller/owner heartbeats to everyone,
|
||||
relay guest heartbeats only to the owner/controllers (for the UI), or server-side
|
||||
aggregation into periodic snapshots instead of per-peer relay.
|
||||
- **`ROOM_DATA` payload trimming** — a 510-entry peer list is large; send counts + controller
|
||||
details, lazy-load the full roster.
|
||||
- Possibly the **socket.io Redis adapter** for horizontal scaling, and broadcast tuning.
|
||||
|
||||
## 8. Effort estimate
|
||||
- **Co-host roles** (server gate generalization + `SET_PEER_ROLE` + owner-leave fallback +
|
||||
client gates + promote UI + i18n), at the current ≤25-peer scale: **~3–4 dev days** (same
|
||||
shape as host-control itself — mostly generalizing host→controller-set).
|
||||
- **Large-room scaling (510)**: a **separate ~1–2 week** track (heartbeat redesign + payload
|
||||
trimming + cap raise + load testing), independent of co-host. Recommend shipping co-host at
|
||||
the current cap first, then scaling rooms as its own project.
|
||||
|
||||
## 9. Suggested sequencing
|
||||
1. `CAPABILITIES.CO_HOST` + `controllers`/`ownerPeerId` in room state + `ROOM_DATA` (additive).
|
||||
2. Server `SET_PEER_ROLE` + gate generalization + owner-leave fallback + WS tests.
|
||||
3. Background: controller-set membership in both gates + `amOwner`/`amController`.
|
||||
4. Popup: promote/demote toggles (owner) + Controller badge + i18n.
|
||||
5. (Separate track) large-room scaling.
|
||||
Reference in New Issue
Block a user