Repository navigation
🔒️ Only accept presence for the doc on its channel - #752
alecgibson wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 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.
798c1f2 to
0fb486e
Compare
Fixes #735 At the moment, the server relays a doc presence whose `v` is ahead of the doc unchanged, which lets one client make every subscriber fetch: 1. Submit presence with `v: 1e9` to a doc with 20 subscribers 2. Each subscriber calls `doc.fetch()`, since `v > doc.version` 3. The presence stays pending, so every later op fetches again This change has the `Agent` pass its tracked doc version through to the presence transform, which reads ops from the lower of that and `v`. The tracker can lag the database but never lead it, so a `v` past the last op read is ahead of the doc. Ops in that lag may have been deleted, though, so we check the snapshot version before rejecting. Doc presence must now carry an integer `v`, and so must ops that send one. A rejected presence restores the `Agent`'s previously cached request, so the leave it replays on disconnect isn't rejected too. Presence sent without `c`/`d` skips all of this, since the server can't tell it from generic presence, but clients ignore it since #752. We deliberately don't bound presence that is far *behind*: a `fetch` with `v: 0` already makes the server read the whole op history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
dawidreedsy
left a comment
There was a problem hiding this comment.
I do not have comments, but my review panel has few
| ### Example | ||
|
|
||
| ```js | ||
| backend.use('receivePresence', (context, next) => { | ||
| // agent.custom is usually set in the 'connect' hook | ||
| const userId = context.agent.custom.userId | ||
| const presence = context.presence | ||
| if (!presence.id.startsWith(`${userId}:`)) { | ||
| return next(new Error('Unauthorized')) | ||
| } | ||
| // This app only uses typed presence | ||
| if (!presence.c) { | ||
| return next(new Error('Unauthorized')) | ||
| } | ||
| // Let users clear their own presence, even if they've lost access | ||
| if (presence.p === null) return next() | ||
| if (!userCanChangeDoc(userId, presence.c, presence.d)) { | ||
| return next(new Error('Unauthorized')) | ||
| } | ||
| next() | ||
| }) | ||
|
|
||
| backend.use('sendPresence', (context, next) => { | ||
| const userId = context.agent.custom.userId | ||
| const presence = context.presence | ||
| if (!userCanReadDoc(userId, presence.c, presence.d)) { | ||
| return next(new Error('Unauthorized')) | ||
| } | ||
| next() | ||
| }) |
There was a problem hiding this comment.
[🤖 Security · should] F1: The example rejects updates in 'sendPresence' but never sets doNotForwardSendPresenceErrorsToClient. Anyone who copies it as-is keeps leaking presence IDs, and with the recommended ${userId}:${randomId} scheme that means user IDs, to clients that can't read the doc.
PoC with this exact middleware on a default Backend: Mallory is denied docs/secret in readSnapshots and sends a raw {a:'ps', ch:'docs.secret'}. On every update Alice sends, Mallory receives:
{"a":"p","ch":"docs.secret","id":"alice:abc","error":{"message":"Unauthorized"}}
This comes from agent.js:991. That tells Mallory who is on the doc and when, which is #738's eavesdropping case. The warning at L92 explains this, but it's a few paragraphs above the code people will paste.
Suggestion: Put the option in the example itself:
const backend = new Backend({presence: true, doNotForwardSendPresenceErrorsToClient: true})or add a comment on the 'sendPresence' handler saying it relies on that option.
There was a problem hiding this comment.
Fixed: the example now sets doNotForwardSendPresenceErrorsToClient, plus an errorHandler that skips the middleware's own PRESENCE_FORBIDDEN rejections. Otherwise every rejection lands in the default handler, which logs it. The warning above the example says so too.
| Presence.prototype._receiveUpdate = function(error, message) { | ||
| var isForwardedError = !!error && message.pv == null; | ||
| var localPresence = util.dig(this.localPresences, message.id); | ||
| if (localPresence) return localPresence._ack(error, message.pv); | ||
| if (localPresence && !isForwardedError) return localPresence._ack(error, message.pv); |
There was a problem hiding this comment.
[🤖 Intent + Correctness + Tests & conventions · question] F2: This closes the error route into a local presence with the same ID, but not the non-error route. A spoofed typed presence for the same doc, carrying the victim's presence ID, still reaches localPresence._ack(null, message.pv) and fires the victim's pending submit callback early. That's the remaining part of #738's hijack item.
The tests expert reproduced it on this head with a probe test (a scratch copy only, not pushed). It mirrors the test at test/client/presence/doc-presence.js:696, but with a typed message: connection 3 sends {ch:'books.northern-lights', c:'books', d:'northern-lights', t, id:'presence-1', pv:0}, and the victim's submit callback fires with null before its own presence comes back. pv starts at 0 and goes up by one (local-presence.js:67), so it's easy to guess.
This isn't a regression, and the docs' receivePresence ID-ownership check (docs/presence.md:128) blocks it when apps follow them, so it's arguably covered by "presence ID ownership is left to middleware". Was client-side hardening considered? The panel checked that it's viable and mixed-version safe:
- Your own success ack is the presence
_createPresencebuilds, whosesrcisagent._src(), which is the handshakeidthe client stores asconnection.id(connection.js:780). - Your own error echo is the original request, which has no
src. - A forger who sets their handshake
idto the victim'ssrcis dropped bypresence.src === this._src()(agent.js:978) before reaching the victim.
Suggestion: Either only ack a non-error message when message.src is missing or equal to this.connection.id, and add a typed version of the :696 test; or keep it as middleware's job and say so in the Presence IDs docs (see F5).
There was a problem hiding this comment.
Good catch, fixed. A local presence now only takes a message if it's one of its own replies: a success carries this connection's src, and an error echo carries the request's pv. Presence from another client with the same ID gets dropped, and so does a forwarded error, which goes to the Presence instead. There are tests for typed presence (doc-presence.js), untyped presence on a doc channel, and plain Presence (presence.js).
| }; | ||
|
|
||
| Presence.prototype._receiveUpdate = function(error, message) { | ||
| var isForwardedError = !!error && message.pv == null; |
There was a problem hiding this comment.
[🤖 Readability · nit] F3: The rule "an error with no pv is a forwarded error, not a reply to my own submission" is the core of the Copilot fix, but the code doesn't explain it. The reason lives in another file (agent.js:991 builds {a, ch, id, error} with no pv), so the next person to "simplify" this could easily bring the misrouting back.
Suggestion:
// Replies to our own submissions echo the request, so always have a pv. Errors
// forwarded from 'sendPresence' are about someone else's presence and have no
// pv, so they mustn't ack a local presence that happens to share its ID.
var isForwardedError = !!error && message.pv == null;There was a problem hiding this comment.
I've kept this without a comment, since our house style puts the why in the commit message. I have split it into two named booleans, isErrorForOwnRequest and isFromThisConnection, so the rule reads off the code. The forwarded-error test fails if someone "simplifies" it back.
| {: .warn } | ||
| Rejecting updates in `'sendPresence'` only stops them reaching the client if you set the [`doNotForwardSendPresenceErrorsToClient`]({{ site.baseurl }}{% link api/backend.md %}#options) option. Otherwise, the client is sent an error for each rejected update, including its presence ID. With the option set, each rejected update is passed to the `errorHandler` instead. | ||
|
|
||
| ### Sending presence |
There was a problem hiding this comment.
[🤖 Readability · nit] F4: The headings are written from the client's side, but the hooks are named from the server's. So "Receiving presence" (L85) tells you to use 'sendPresence', and "Sending presence" tells you to use 'receivePresence'. The body text names the right hook, but a skimmer could easily pick the wrong one.
Suggestion: Name the headings after what they control, e.g. ### Who can see presence / ### Who can send presence.
There was a problem hiding this comment.
Fixed: the headings are now "Who can see presence" and "Who can send presence".
|
|
||
| ### Presence IDs | ||
|
|
||
| Presence IDs are chosen by the client, and sent to every subscriber, so a client could use another client's presence ID to overwrite or clear its presence. |
There was a problem hiding this comment.
[🤖 Intent · nit] F5: A stolen presence ID does more than "overwrite or clear" another client's presence. #738 also lists that it can:
- suppress all of the victim's later updates by sending a high
pv, sinceRemotePresencedropspv < presenceVersion(remote-presence.js:14); - fire the victim's pending submit callbacks early (F2).
Spelling those out makes the ownership check below read as necessary rather than optional.
Suggestion: "...to overwrite or clear its presence, stop its later updates being applied, or trigger its pending submit callbacks."
There was a problem hiding this comment.
Fixed: the docs now say a stolen ID can also stop the victim's later updates being applied, and that older clients fire their pending submit callbacks early.
| // agent.custom is usually set in the 'connect' hook | ||
| const userId = context.agent.custom.userId | ||
| const presence = context.presence | ||
| if (!presence.id.startsWith(`${userId}:`)) { |
There was a problem hiding this comment.
[🤖 Security · nit] F7: startsWith(${userId}:) only proves ownership if user IDs can't contain :. User alice passes with alice:bob:xyz, which is also a valid presence ID for user alice:bob. And if agent.custom.userId isn't set, the prefix becomes undefined:, which anyone can use.
Suggestion: Reject a missing userId first (if (!userId) return next(new Error('Unauthorized'))), and note that the separator must not appear in user IDs.
There was a problem hiding this comment.
Fixed: the example now rejects a missing userId first, and the text says to use a separator that can't appear in user IDs.
|
|
||
| Subscribing to presence doesn't trigger any presence middleware, so any client can subscribe to any presence channel -- including the presence of a document it can't read. | ||
|
|
||
| To stop a client receiving presence, reject the update in the [`'sendPresence'`]({{ site.baseurl }}{% link middleware/actions.md %}#sendpresence) middleware. For typed presence, ShareDB makes sure that `presence.c` and `presence.d` match the presence channel. This middleware runs for every update about to be sent to every client, so keep the check cheap -- for example, by caching permissions on [`agent.custom`]({{ site.baseurl }}{% link api/agent.md %}#custom--object). |
There was a problem hiding this comment.
[🤖 Readability · nit] F8: "For typed presence, ShareDB makes sure that presence.c and presence.d match the presence channel." sits between two sentences about 'sendPresence' without saying why the reader should care: it's what lets that middleware trust c/d for its read check.
Suggestion: "...match the channel, so you can check read access with them."
There was a problem hiding this comment.
Fixed, using your wording.
|
|
||
| DocPresence.prototype._receiveUpdate = function(error, message) { | ||
| var isForThisDoc = message.c === this.collection && message.d === this.id; | ||
| if (!error && !isForThisDoc) return; |
There was a problem hiding this comment.
[🤖 Readability · nit] F9: Letting every error past the "is this for my doc" filter is deliberate, because server error frames have no c/d. Without a comment it looks like an oversight.
Suggestion:
// Error frames from the server have no c/d, so Presence._receiveUpdate routes them
if (!error && !isForThisDoc) return;There was a problem hiding this comment.
Same as F3: no comment, by house style. The error exemption is pinned by "emits a forwarded error for a presence with the same ID on the presence, not its local presence", which fails if errors get filtered here.
| return 'Invalid presence ID'; | ||
| } | ||
| if (isDocPresence(request) && !matchesDocPresenceChannel(request)) { | ||
| return 'Invalid presence channel'; |
There was a problem hiding this comment.
[🤖 Readability · nit] F10: This returns the same 'Invalid presence channel' as the existing check for a malformed ch (L446), so logs and clients can't tell which rule fired. No test matches this new message's text (the only assertion on it, test/client/presence/presence.js:547, is for the old check), so it's safe to change.
Suggestion: return 'Presence collection and ID do not match channel';
There was a problem hiding this comment.
Fixed: it's now "Presence collection and doc ID do not match channel", and the test asserts that text. While rebasing onto 7.0.0 I also moved the check after the c/ch type checks. Otherwise an unchecked c like {toString: 1} would hit DocPresence.channel(). There are new tests in test/agent.js for that order.
| }); | ||
|
|
||
| it('rejects a presence message with an empty collection', function(done) { | ||
| var localPresence1 = connection1.getDocPresence('', 'northern-lights').create('presence-1'); |
There was a problem hiding this comment.
[🤖 Tests & conventions · nit] F11: An empty collection has its own test, but an empty doc ID (getDocPresence('books', '')) doesn't. !!request.d only shows as covered because the null-d test hits it.
Rejecting empty c/d is the right call: on main such presence skipped _subscribeDocVersion, and with it the readSnapshots check, and was broadcast untransformed. It's worth pinning both halves.
Suggestion: Add a matching 'rejects a presence message with an empty doc ID' test.
There was a problem hiding this comment.
Added "rejects a presence message with an empty doc ID".
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. Another client's presence, or a forwarded `sendPresence` error, no longer reaches a local presence with the same ID either: the client's own replies carry its `src` if they succeed, or the request's `pv` if they fail, and forwarded errors carry neither. 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>
Fixes #735 At the moment, the server relays a doc presence whose `v` is ahead of the doc unchanged, which lets one client make every subscriber fetch: 1. Submit presence with `v: 1e9` to a doc with 20 subscribers 2. Each subscriber calls `doc.fetch()`, since `v > doc.version` 3. The presence stays pending, so every later op fetches again This change has the `Agent` pass its tracked doc version through to the presence transform, which reads ops from the lower of that and `v`. The tracker can lag the database but never lead it, so a `v` past the last op read is ahead of the doc. Ops in that lag may have been deleted, though, so we check the snapshot version before rejecting. Doc presence must now carry an integer `v`, and so must ops that send one. A rejected presence restores the `Agent`'s previously cached request, so the leave it replays on disconnect isn't rejected too. Presence sent without `c`/`d` skips all of this, since the server can't tell it from generic presence, but clients ignore it since #752. We deliberately don't bound presence that is far *behind*: a `fetch` with `v: 0` already makes the server read the whole op history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
b5df548 to
ecab047
Compare
Migration noteNot breaking. This can ship as a 7.x fix. It also carries #749, which was merged into this branch. The server now rejects requests that the bundled client never sends:
Clients now ignore some presence they used to apply:
The |
Fixes #735 At the moment, the server relays a doc presence whose `v` is ahead of the doc unchanged, which lets one client make every subscriber fetch: 1. Submit presence with `v: 1e9` to a doc with 20 subscribers 2. Each subscriber calls `doc.fetch()`, since `v > doc.version` 3. The presence stays pending, so every later op fetches again This change has the `Agent` pass its tracked doc version through to the presence transform, which reads ops from the lower of that and `v`. The tracker can lag the database but never lead it, so a `v` past the last op read is ahead of the doc. Ops in that lag may have been deleted, though, so we check the snapshot version before rejecting. Doc presence must now carry an integer `v`, and so must ops that send one. A rejected presence rolls the `Agent`'s cached request back to the newest earlier one that wasn't rejected, so the leave it replays on disconnect isn't rejected too. Presence sent without `c`/`d` skips all of this, since the server can't tell it from generic presence, but clients ignore it since #752. We deliberately don't bound presence that is far *behind*: a `fetch` with `v: 0` already makes the server read the whole op history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
ecab047 to
727b515
Compare
Fixes #735 At the moment, the server relays a doc presence whose `v` is ahead of the doc unchanged, which lets one client make every subscriber fetch: 1. Submit presence with `v: 1e9` to a doc with 20 subscribers 2. Each subscriber calls `doc.fetch()`, since `v > doc.version` 3. The presence stays pending, so every later op fetches again This change has the `Agent` pass its tracked doc version through to the presence transform, which reads ops from the lower of that and `v`. The tracker can lag the database but never lead it, so a `v` past the last op read is ahead of the doc. Ops in that lag may have been deleted, though, so we check the snapshot version before rejecting. A leave reads ops even when its `v` matches the tracked version: no client resends `null` presence, so a stale one would never be corrected. Doc presence must now carry an integer `v`, and so must ops that send one. A rejected presence rolls the `Agent`'s cached request back to the newest earlier one that wasn't rejected, so the leave it replays on disconnect isn't rejected too. Presence sent without `c`/`d` skips all of this, since the server can't tell it from generic presence, but clients ignore it since #752. We deliberately don't bound presence that is far *behind*: a `fetch` with `v: 0` already makes the server read the whole op history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
727b515 to
0e74cae
Compare
Fixes #735 At the moment, the server relays a doc presence whose `v` is ahead of the doc unchanged, which lets one client make every subscriber fetch: 1. Submit presence with `v: 1e9` to a doc with 20 subscribers 2. Each subscriber calls `doc.fetch()`, since `v > doc.version` 3. The presence stays pending, so every later op fetches again This change has the `Agent` pass its tracked doc version through to the presence transform, which reads ops from the lower of that and `v`. The tracker can lag the database but never lead it, so a `v` past the last op read is ahead of the doc. Ops in that lag may have been deleted, though, so we check the snapshot version before rejecting. A leave reads ops even when its `v` matches the tracked version: no client resends `null` presence, so a stale one would never be corrected. Doc presence must now carry an integer `v`, and so must ops that send one. A rejected presence rolls the `Agent`'s cached request back to the newest earlier one that wasn't rejected, so the leave it replays on disconnect isn't rejected too. Presence sent without `c`/`d` skips all of this, since the server can't tell it from generic presence, but clients ignore it since #752. We deliberately don't bound presence that is far *behind*: a `fetch` with `v: 0` already makes the server read the whole op history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
0e74cae to
c3be55e
Compare


Fixes #738
At the moment, the server checks read access against a presence's
candd, but broadcasts it on itsch:docs/secretinreadSnapshotsdocs.secretwith thec/dof a readable docdocs/secretsubscribers apply itThis change rejects doc presence whose
candddon't match its channel, andDocPresenceignores presence for any other doc, which also covers untyped presence and dotted-name channel collisions. Another client's presence, or a forwardedsendPresenceerror, no longer reaches a local presence with the same ID either: the client's own replies carry itssrcif they succeed, or the request'spvif they fail, and forwarded errors carry neither. 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