mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 06:55:40 +00:00
xarmian
2945ee27dd
* feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254)
WebSocket entry point for Yjs-based collaborative editing on a
single item under PLAN-1248. Bare-bones in this PR by design:
upgrade + log connect/disconnect + drain reads. Protocol logic
(forwarding to OpBus, persisting to op-log, awareness fan-out)
arrives in TASK-1255 (room manager).
Authorisation mirrors RequireWorkspaceAccess but keyed on the
item's workspace ID rather than a {slug} URL param — the WS URL
only carries itemID. Implementation re-uses the same access
ladder:
fresh-install escape hatch (no users)
→ grant
legacy workspace-scoped API token, no user
→ grant if token's workspace matches the item's workspace
OAuth token allow-list (TASK-953)
→ reject when workspace not on consented list
authenticated user
→ admin OR member OR has guest grants
User is re-fetched from the store on each upgrade (not trusted
from session-context cache) so a mid-session admin demotion or
member removal closes the upgrade path immediately. Mirrors
sseSubscriberStillHasAccess. Periodic per-connection
revalidation lives in TASK-1256.
Route registered alongside SSE (outside the jsonContentType
middleware group, but inside the auth middleware chain). Promotes
github.com/gorilla/websocket from indirect to direct dep and
bumps to v1.5.3 (latest stable; v1.5.0 was already in
go.mod transitively via another package).
Tests cover:
- fresh-install escape hatch grants the upgrade
- bootstrapped server rejects unauthenticated upgrade with 401
- non-member with valid session is rejected with 403
(NOT 401 — confirms the access path runs after auth, not before)
- unknown item surfaces as 404 (not 401/403 leak)
- empty itemID segment doesn't match the route
Test infrastructure note: dialCollab takes an explicit User-Agent
because pad's session-binding middleware hashes the UA at
CreateSession time and re-checks on every request — the dialer
must match what was stored, otherwise the cookie is rejected
before the workspace check fires (and we'd see a misleading 401
where 403 was expected).
Parent: PLAN-1248. Phase 1 — Backend foundation.
* style: gofmt handlers_collab_test.go per Codex review (round 1)
* fix(server): SetReadLimit + nginx upgrade headers for collab WS per Codex review (round 2)
P-MEDIUM #1: handleCollab.ReadMessage had no per-message size cap, so an
authenticated client could send an arbitrarily large frame and force
unbounded server-side buffering — the HTTP body limit applied by the
auth chain doesn't apply once the connection is upgraded. Set
SetReadLimit(1 MiB), generous for keystroke-rate Yjs ops and large
enough for a typical initial-sync state. ReadMessage returns an error
when exceeded, which the existing read loop handles as a normal close.
P-MEDIUM #2: deploy/nginx.conf routed /api/v1/collab/ through the
default `location /` block, which sets `Connection ""` (cleared so HTTP
keepalive works) — that strips the Upgrade header, so WebSocket
upgrades silently fail behind the documented nginx deployment. Add a
dedicated location block with proxy_set_header Upgrade $http_upgrade /
Connection "upgrade", same 24h read/send timeouts as SSE so an idle
editor tab does not get cut off mid-session.
* fix(server): enforce per-item visibility in collab WS upgrade per Codex review (round 3)
P2: authorizeCollabAccess granted upgrade to any workspace member or
guest-with-grants without checking whether THIS specific item was
visible to that user. A restricted member (collection_access=specific)
or a guest with grants on item A could upgrade /api/v1/collab/{itemID}
for an item B in a different collection — they'd see live edits to a
document they have no right to read.
Restructure the access ladder:
1. Workspace-level gate stays as-is: "any access at all?" If no
membership AND no grants → 403 (unchanged).
2. Item-level visibility check added on top, mirroring requireItemVisible
without depending on middleware-set request context (the WS path
doesn't go through RequireWorkspaceAccess):
- VisibleCollectionIDs nil → "all" access → grant.
- Item's collection in the visible set → grant.
- Item-level grant on this exact item → grant (covers guests
given access to a single item rather than a whole collection).
- Else → 404, mirroring requireItemVisible's "don't leak
existence" pattern.
Admin path returns nil before this check, so no change there.
Legacy workspace-scoped API tokens grant editor-equivalent access
on workspace match (predates the grants design); that branch is
untouched since legacy tokens don't have a user identity to scope
per-item grants against.
Test added: TestCollabUpgradeRejectsRestrictedMemberForeignCollection
— member with specific access to collA tries to upgrade for an item
in collB → 404. Existing 5 tests still pass.
* fix(server): strict per-item visibility check + sibling-grant test per Codex review (round 4)
P1 (round 4): VisibleCollectionIDs is broader than full-collection
access — it includes collections "anchored" by an item-level grant
(so the nav can still surface the parent collection of a granted
item). Round 3's check treated every visible collection as full
access; a guest with grant `item:A` could upgrade
/api/v1/collab/{B} for a sibling B in the same collection.
Tighten by mirroring guestResourceFilter / requireItemVisible:
1. Coarse stage stays — collection must be in the visible set.
2. NEW strict stage when the user has item-level grants:
(a) full collection grant on this collection → grant
(b) member's "specific" access list including this collection
→ grant
(c) item grant on THIS exact item → grant
Else → 404 (the visible-set hit was anchored by a sibling's
grant, not by full collection access).
When the user has NO item grants, the coarse-only check is
sufficient — visibility came from full collection access (member's
"specific" list, full collection grant, or "all" access).
Test added: TestCollabUpgradeRejectsGuestWithSiblingItemGrantOnly
— guest with item:A grant tries to upgrade for sibling B in the
same collection → 404 (the bug being regression-tested) AND verifies
the granted item A still upgrades cleanly to 101 Switching Protocols.