Skip to content

🔒️ Only accept presence for the doc on its channel - #752

Draft
alecgibson wants to merge 1 commit into
mainfrom
check-doc-presence-channel
Draft

alecgibson wants to merge 1 commit into
mainfrom
check-doc-presence-channel

Conversation

@alecgibson

@alecgibson alecgibson commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #738

At the moment, the server checks read access against a presence's c and d, but broadcasts it on its ch:

  1. Deny docs/secret in readSnapshots
  2. Send presence on docs.secret with the c/d of a readable doc
  3. docs/secret subscribers apply it

This change rejects doc presence whose c and d don't match its channel, and DocPresence ignores presence for any other doc, which also covers untyped presence and dotted-name channel collisions. Forwarded sendPresence errors no longer reach a local presence with the same ID either: unlike replies to the client's own submissions, they carry no pv. Subscribing, write access and presence ID ownership are left to middleware, which the new "Access control" docs cover.

The server check only holds once every instance is upgraded, and the client filter only protects upgraded clients.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@coveralls

coveralls commented Oct 6, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 97.949% (+0.03%) from 97.916% — check-doc-presence-channel into main

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Mismatched presence errors can bypass the client filter and reach another local presence with the same ID.

1 open finding
What changed in this PR

Hardens document presence channel validation and documents presence access-control responsibilities.

Changes:

  • Validates document presence channels server-side and filters mismatched client updates.
  • Adds security regression tests.
  • Documents middleware-based access control and presence ID ownership.
File Description
lib/​agent.js Validates document presence channels.
lib/​client/​presence/​doc-presence.js Filters updates for other documents.
test/​client/​presence/​doc-presence.js Tests channel validation and filtering.
docs/​presence.md Adds access-control guidance.
docs/​middleware/​actions.md Adds presence permission warnings.
docs/​api/​presence.md Documents presence ID ownership.
docs/​api/​backend.md Documents presence error-forwarding configuration.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/client/presence/doc-presence.js
@alecgibson
alecgibson force-pushed the check-doc-presence-channel branch from 9014faf to 798c1f2 Compare October 13, 2026 16:35
Fixes #738

At the moment, the server checks read access against a presence's `c`
and `d`, but broadcasts it on its `ch`:

 1. Deny `docs/secret` in `readSnapshots`
 2. Send presence on `docs.secret` with the `c`/`d` of a readable doc
 3. `docs/secret` subscribers apply it

This change rejects doc presence whose `c` and `d` don't match its
channel, and `DocPresence` ignores presence for any other doc, which
also covers untyped presence and dotted-name channel collisions.
Forwarded `sendPresence` errors no longer reach a local presence with
the same ID either: unlike replies to the client's own submissions,
they carry no `pv`. Subscribing, write access and presence ID
ownership are left to middleware, which the new "Access control" docs
cover.

The server check only holds once every instance is upgraded, and the
client filter only protects upgraded clients.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation, tests, and documentation consistently address the reported presence-routing vulnerabilities.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document that presence has no built-in access control (eavesdrop/inject/hijack via channels)

3 participants