📚 Document websocket subscription revocation

The behavior that made GHSA-m53j-2766-6jqw easy to get wrong is
non-obvious: the connection registry is local to one backend instance
while the message bus is shared, so a revocation has to travel over the
bus to reach the instance that holds the socket. The advisory's own
suggested fix closes the connection from the RPC handler, which only
works in a single-backend deployment.

Records, so the next reader does not have to rediscover it:

- the registry shape and that `state` is shadowed by `::ws/state` in
  every handler, so only the lifecycle hooks touch it
- the two revocation mechanisms (announcement, interval re-check) and
  why the decision is always a fresh permission check
- which mutations announce, and why `delete-share-link` does not
- that the topic and publish helpers live in `app.rpc.notifications`,
  since the command namespaces cannot require the watcher
- three Promesa traps that cost real debugging time here: `alts!` over
  the data channel, the argument order of `ct/diff`, and the singular
  unit keywords of `ct/duration`
- how the tests drive the real msgbus, and that watcher-based tests
  must register their connection

Closes #12155

AI-assisted-by: space-bunny-free
This commit is contained in:
Andrey Antukh 2026-10-07 11:07:33 +00:00
parent d608f3b898
commit 5a25da8d6d
2 changed files with 42 additions and 0 deletions

View File

@ -9,6 +9,7 @@ Backend: JVM Clojure; Integrant; PostgreSQL; Redis/Valkey; RPC; HTTP; storage; m
- Embedded Ladybug graph experiment, projection, incremental sync, console, and risks: `mem:backend/graph-experiment`
- Session lifetime config, token `:exp`, and idle/absolute GC: `mem:backend/session-expiration`.
- Auth flows, permission model, teams, projects, invitations, comments, webhooks, audit: `mem:backend/auth-permissions-product-domains`
- WebSocket subscriptions, the connection registry (local per backend, unlike the bus), and how a lost permission gets revoked: `mem:backend/websocket-subscriptions`.
- Audit-log event collection (RPC wrapper, frontend ingestion), telemetry duality, webhook fan-out, error reporters, Nexus archival and retention: `mem:audit-log` (guía completa, también para perfiles junior; el stub `mem:backend/audit-log` solo redirige aquí)
- Services, task-queue/Pub-Sub topology constraints -> `mem:prod-infra/core`.

View File

@ -0,0 +1,41 @@
# Backend WebSocket Subscriptions
## Registry
- `app.http.websocket/state` holds `{:connections {conn-id → wsp} :by-profile {profile-id → #{conn-id}}}` in **one** atom, so a single `swap!` keeps both consistent; `register-connection`/`unregister-connection` maintain them together and drop a profile from the index once its last connection leaves.
- Each connection's own `::ws/state` atom holds its subscriptions (`::file-subscription`, `::team-subscription`). `state` is shadowed by `::ws/state` in every `handle-message` destructuring, so only `on-connect`, `on-disconnect` and the `repl-*` helpers touch the registry — reading the registry from a handler is a bug.
- `::profile-id` and `::session-id` on a connection live in the `app.http.websocket` namespace, while `::ws/id`, `::ws/state`, `::ws/output-ch`, `::ws/session-id` live in `app.util.websocket`. Mixing them up yields silently `nil` values (a `nil` `session-id` makes the channel's `:xf` drop **every** message).
- The registry is local to one backend instance. The message bus is shared (`mem:prod-infra/core`). Anything that must reach connections owned by another instance has to go through the bus — closing a connection from an RPC handler only ever works in a single-backend deployment.
## Revocation model
A subscription is authorized once, at `:subscribe-file`/`:subscribe-team`, and that decision used to be trusted for the whole connection life (GHSA-m53j-2766-6jqw, medium). Two independent mechanisms now bound it:
- **Announcement (primary).** A mutation that can change access publishes on `app.rpc.notifications/internal-revocation-topic` (`notify-permissions-changed`, `notify-team-permissions-changed`). The `::revocation-watcher` Integrant component in `app.http.websocket` subscribes and calls `revalidate-profile-subscriptions`, which looks the profile up in `:by-profile` and re-checks every subscription with the same `has-read-permissions?` predicates the subscribe handlers authorize with. The event is only the trigger; the fresh check is the decision, which is what keeps a downgrade to `viewer` (still readable) and a non-member organization owner (read-only via `get-organization-owner-permissions`) from being mistaken for a revocation.
- **Interval (safety net).** `start-relay` arms a `px/schedule` task per subscription that re-checks every `subscription-revalidation-interval` (default `default-revalidation-interval`, 5 min, a code constant in `app.http.websocket` and **not** in the `config.clj` default map, same rule as session lifetimes). It covers what no announcement can: Nitrate transferring organization ownership (an HTTP call to another service) and rows deleted straight over SQL. The re-check is a scheduled task, not a timer in the relay loop, because a loop-driven timer would be pushed away forever by steady edit traffic — exactly the busiest files. `still-authorized?` is the single fail-open point: a transient DB error logs and keeps the subscription.
Which mutations announce: `delete-team-member`, `leave-team`, `update-team-member-role` (member only), `delete-file`, `delete-project`, `move-files`, `move-project` (whole source team). `delete-share-link` deliberately does not: a share-link visitor never passes the websocket subscribe check, because `check-read-permissions!` is called without a `share-id` and that arity only considers team membership.
`close-file-subscription`/`close-team-subscription` are idempotent: no-op on a mismatched id, and they `dissoc` the subscription so a repeat call and `:pointer-update` do not publish to a dead topic. Closing the channel is what ends the relay (`take!` on a closed channel returns `nil`).
## Topic and layering
- `internal-revocation-topic` is a plain string, not a uuid, so it cannot collide with the `uuid/zero` system topic — that one is `sp/pipe`d straight into the client output and anything published there reaches every client. No client can subscribe to the internal topic: `:subscribe-file`/`:subscribe-team` only accept ids that already passed a permission check, and `:broadcast`/`:pointer-update` only republish to ids the server already holds.
- The topic and the publish helpers live in `app.rpc.notifications`, not in `app.http.websocket`: the command namespaces cannot require the watcher, because `app.http.websocket` already requires `app.rpc.commands.files`/`teams`. The watcher keeps the registry, which is the part it owns.
## Promesa traps in this namespace
- `sp/alts!` over the **data** channel silently stopped delivering in practice; `sp/take!` is the proven path here (`app.util.websocket/start-io-loop!` uses `alts!` with a ping channel, which is a different case).
- `ct/diff a b` returns **b − a** (`Duration/between`), so "time until a future instant" is `(inst-ms (ct/diff (ct/now) future))`.
- `ct/duration` unit keywords are singular: `{:millis 150}`, `{:minutes 5}`, never `:milliseconds`/`:days`.
## Tests
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes.
## See also
- `mem:backend/auth-permissions-product-domains` for the permission predicates and the not-found semantics that the re-checks reuse.
- `mem:prod-infra/core` for why the bus crosses backends and the registry does not.
- `mem:frontend/routing-app-shell-subtleties` for the client side: the frontend already reacts to `team-membership-change` by navigating away, which is why revocation needs no new client message.
- `.agents/plans/2026-10-07-websocket-subscription-revocation.md` for the original plan, and the sibling `.agents/plans/2026-10-05-leave-team-file-access-cleanup.md` whose hardened permission queries this re-verification depends on (without live team membership required in `sql:file-permissions`, the re-check would keep reporting access for a member who already left).