mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 13:28:57 +00:00
xarmian
bcef802335
* fix(security): gate collab WebSocket writes on editor role (TASK-265)
The collab WebSocket (GET /api/v1/collab/{itemID}) is mounted outside
the /{slug} subrouter, so RequireWorkspaceAccess never runs on it.
authorizeCollabAccess gated admission on membership + item visibility
but NOT edit role, so a plain workspace VIEWER was admitted and could
WRITE: every inbound Yjs sync frame persisted to item_yjs_updates
(room.go) and got canonicalized into items.content when a co-present
editor's authorized flush ran. The REST write path blocks viewers via
requireEditPermission; this closes the equivalent gap on the collab
relay.
Fix — non-editors become READ-ONLY participants (not hard-rejected, so
live view + presence stay intact):
- authorizeCollabAccess now returns a collabAccess{canWrite} alongside
the admission decision. canWrite is computed once via
store.ResolveUserPermission (the same predicate requireEditPermission
falls back to): owner/editor membership grants write; a viewer/guest
gets write only through a collection/item edit grant.
- RoomManager.Join takes a canWrite flag stored per-connection as an
atomic.Bool. room.go's readLoop drops a read-only conn's inbound sync
frames (not persisted via AppendYjsUpdate, not rebroadcast); awareness
(presence) frames still relay so the viewer's cursor stays visible,
and outbound broadcasts from editors still reach the viewer.
- The handler's periodic revalidation pushes mid-session write-permission
changes via a new RoomManager.SetConnWritable, so an editor demoted to
viewer becomes read-only without a reconnect (complements the existing
CloseConn-on-revocation path).
No SCHEMA_VERSION / DefaultSchemaVersion bump: this is an authorization
/ behavioral change, not a ProseMirror/Y.Doc node-spec change, so the
op-log must not be pruned.
Tests: TestCollabViewerIsReadOnly (viewer admitted 101, receives an
editor's broadcast, but its own sync frame is neither persisted nor
broadcast while the editor's is) and TestAuthorizeCollabAccessCanWrite
(viewer→canWrite=false, editor→canWrite=true). Verified the E2E test
fails with the gate removed.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): close 4 collab read-only gaps from orchestrator review (TASK-265)
Independent Codex pass on the collab editor-role gate found four gaps:
[P1] Read-only conns were still eligible designated APPLIERS. A viewer
(or an editor demoted mid-session) could be elected to apply an
external content edit; its resulting sync frames were dropped by the
new gate, yet its applier_ack was accepted → ApplyExternalContent
reported success → the PATCH handler skipped its direct-write fallback
→ the external edit was silently lost. Fix: pickApplier now skips
non-writers, and handleControlMessage ignores applier_ack from a conn
whose canWrite is false (belt-and-suspenders so the fallback fires).
[P2] Demotion TOCTOU. readLoop read canWrite=true, then could block on
appendMu and persist AFTER SetConnWritable(false) returned. Fix: the
canWrite check now runs INSIDE the appendMu critical section, and
SetConnWritable stores the flag under the same appendMu — so a frame
racing a demotion is either fully persisted before the flip or dropped.
[P2] Revalidation could run before the conn was registered. The first
jittered tick could fire while Join was still setting up; SetConnWritable
would no-op against the unregistered conn and Join then installed the
stale canWrite=true until a later tick. Fix: Join takes an onRegistered
callback invoked right after addConn; the handler gates the reval loop
on it so the first SetConnWritable always finds the conn.
[P2] canWrite didn't mirror REST for editors/owners. It was computed
purely from ResolveUserPermission, which resolves item/collection
GRANTS before membership role — so an editor/owner holding an
incidental `view` grant was wrongly made read-only. Fix: mirror
requireEditPermission exactly — editor/owner MEMBER short-circuits to
canWrite=true BEFORE grant resolution; viewers/guests still fall back
to ResolveUserPermission so grants can override an insufficient role.
Tests: TestApplyExternalContentSkipsReadOnlyApplier (verified failing
without the pickApplier gate), TestHandleControlMessageIgnoresAckFromReadOnlyConn,
TestCollabDemotionMakesConnReadOnly (mid-session demotion → read-only
without reconnect), and two new TestAuthorizeCollabAccessCanWrite cases
(editor+incidental view grant → true; viewer+edit grant → true).
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): safe no-applier direct write for read-only-only collab rooms (TASK-265)
Codex round 2 found a P1 introduced by excluding viewers from applier
election: in a room whose only peers are read-only, an external content
update (PATCH) hits ErrNoApplierAvailable, then PruneAndApply refused to
prune because live conns existed (len(r.conns) > 0). After the retry
budget the PATCH handler fell through to an UNLOCKED, UN-PRUNED direct
write — items.content was updated but the stale op-log survived, so a
fresh editor replaying it (or a viewer promoted to editor flushing its
stale in-memory Y.Doc) would silently overwrite the external update.
Fix: PruneAndApply now blocks only on a live WRITER peer — a read-only
peer can never persist, so it doesn't force the unsafe fallback. After
the prune + write succeeds it evicts the read-only peers
(Room.closeReadOnlyConns: WriteControl close frame + Close, concurrency-
safe with writeLoop) so their now-stale Y.Doc can't linger; they
reconnect and lazy-seed from the fresh items.content (their old resume
cursor is below the pruned op-log's MIN → force_refresh). Mixed rooms
(an editor present) are unaffected — the editor is still elected applier
and PruneAndApply is never reached.
Tests: TestPruneAndApplyEvictsReadOnlyRoom (read-only-only room prunes +
evicts, applyFn runs) and TestPruneAndApplyBlockedByLiveWriter (a live
writer still yields ErrRoomActiveDuringPrune).
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): fence PruneAndApply read-only eviction under appendMu (TASK-265)
Codex round 3 P1: PruneAndApply classified writers, ran applyFn (prune +
write), and evicted read-only conns WITHOUT holding room.appendMu. A
concurrent viewer→editor revalidation could set canWrite=true after the
writer check, append a stale frame during the prune/write, and — now a
writer — evade closeReadOnlyConns, racing the prune and leaving a live
stale Y.Doc that overwrites the external update.
Fix: PruneAndApply now holds room.appendMu across the ENTIRE sequence
(writer classification + applyFn + eviction). appendMu is the same lock
readLoop takes across its canWrite-check+persist and SetConnWritable
takes when flipping canWrite, so a promotion can no longer interleave
with the classification/prune. Lock order is itemLock → appendMu →
room.mu; no path takes room.mu → appendMu, so no inversion.
Also closes the residual "frame already read, blocked on appendMu, then
promoted after release" window: roomConn gains a terminal `evicted`
atomic flag set by closeReadOnlyConns (under appendMu) and checked in
readLoop's persist gate alongside canWrite, so an evicted read-only
conn's in-flight frame is dropped even if a racing revalidation promotes
it in the same instant.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* refactor(collab): descope read-only eviction; keep writer-aware prune guard (TASK-265)
Per orchestrator scope decision, remove the read-only EVICTION machinery
added during review (over-engineering for TASK-265's security goal):
- Room.closeReadOnlyConns and its call in PruneAndApply.
- roomConn.evicted flag and its check in readLoop's persist gate.
- appendMu held across PruneAndApply + the force-close socket I/O.
PruneAndApply reverts to no appendMu / no socket I/O, keeping only the
LOAD-BEARING writer-aware guard: it blocks (ErrRoomActiveDuringPrune)
only on a live WRITER peer, not any conn. An all-viewer room's external
edit therefore still prunes + direct-writes safely (op-log pruned, so a
fresh editor lazy-seeds from the new items.content) instead of erroring.
The residual — a connected read-only peer keeps a possibly-stale Y.Doc
until reconnect/refresh, and a viewer promoted to editor before re-sync
could push stale content — is a low-severity lost-update edge (a
promoted viewer is a legitimate editor), consistent with the pre-existing
direct-write contract. Documented on PruneAndApply and tracked in
BUG-2103 (proposed fix: proactive re-seed/refresh of remaining read-only
peers).
Kept unchanged: the authorizeCollabAccess canWrite editor/owner role
short-circuit, dropping read-only inbound sync frames under appendMu +
the SetConnWritable demotion fence + registration ordering, and the
pickApplier / applier_ack read-only exclusions.
Tests: replace TestPruneAndApplyEvictsReadOnlyRoom with
TestPruneAndApplyAllowsReadOnlyRoom (read-only-only room -> applyFn
runs); keep TestPruneAndApplyBlockedByLiveWriter.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): enforce token write-scope + fence prune promotion on collab (TASK-265)
Two logic gaps from the orchestrator's final pass:
[P1] canWrite ignored BEARER-TOKEN SCOPE. The collab upgrade is a GET,
so a read-scoped PAT/OAuth token passes TokenAuth's method-keyed
tokenScopeAllows check, then rode the user's editor role (or the legacy
workspace-token grant) to canWrite=true and could persist Yjs mutations
over the socket — a read-only-principal-writes bypass via token scope
instead of role. REST DOES enforce write-scope (TokenAuth →
tokenScopeAllows blocks read-scoped tokens from PATCH/POST/DELETE); the
collab GET simply slips the method gate. Fix mirrors REST: TokenAuth now
stashes the token scopes (WithTokenScopes, as MCPBearerAuth already
does), and authorizeCollabAccess re-applies the write-capability half —
canWrite is downgraded to read-only when the caller's token scope
doesn't permit writes (http.MethodPost representative verb). Applied to
both the legacy workspace-token path and the member/grant path.
Non-token principals (cookie / CLI session, fresh install) carry empty
scopes → unrestricted → unaffected. Test: an editor with a read /
pad:read token gets canWrite=false; with write / * gets canWrite=true.
[P2] PruneAndApply's writer-scan was not serialized with SetConnWritable,
so a viewer promoted during applyFn could append a stale frame while the
op-log is pruned + content written (persist/prune ordering race, distinct
from BUG-2103's async residual). Fix: hold room.appendMu across the
writer-scan AND applyFn. Safe now that eviction/socket-I/O is gone —
applyFn is a pure store op (PruneYjsUpdatesBefore + UpdateItemWithParentLink,
the only caller) that never re-enters itemLock / appendMu / room.mu, so no
inversion or re-entrant deadlock. Lock order: itemLock → appendMu → room.mu.
Gates: build, gofmt, vet, golangci-lint (0 issues), go test
./internal/server/ ./internal/collab/, and go test -race
./internal/collab/ all pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra
* fix(security): honor token write-scope in collab fresh-install branch (TASK-265)
The zero-user (pre-bootstrap) branch of authorizeCollabAccess returned
canWrite=true unconditionally. A legacy workspace token still carries a
scope on a fresh instance, so a read-scoped token could persist Yjs
mutations over the collab GET upgrade — inconsistent with REST, whose
method gate blocks a read token's mutation. Route the branch through
collabTokenWriteScopeAllowed, which returns true for the anonymous
(no-token) setup caller (empty scopes = unrestricted) and false for a
read-scoped token. Adds TestAuthorizeCollabAccessFreshInstallTokenScope.
Found by the orchestrator's independent Codex pass.
Claude-Session: https://claude.ai/code/session_01EZ6yr6pAUFb1uffan912ra