From dff92b72a72fd6739f38d2d292f7cb5653564333 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:20:49 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=9D=20Document=20how=20to=20stop=20cli?= =?UTF-8?q?ents=20spoofing=20op=20`src`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/share/sharedb/issues/733 At the moment, a client can submit an op with another client's `src` and next `seq`, and the server will then drop that client's real op as a duplicate, while still acking it. We can't enforce ownership of `src` in the library without a protocol change, since the handshake `id` is also chosen by the client. Instead, this change documents that `src` is client-chosen, and adds a middleware recipe that scopes it to the authenticated user. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- docs/api/agent.md | 7 +++ docs/middleware/op-submission.md | 51 +++++++++++++++++ lib/agent.js | 3 +- test/middleware.js | 96 ++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 2 deletions(-) diff --git a/docs/api/agent.md b/docs/api/agent.md index 1a614bb68..7265e76f5 100644 --- a/docs/api/agent.md +++ b/docs/api/agent.md @@ -27,6 +27,13 @@ If the `Connection` was created through [`backend.connect()`]({{ site.baseurl }} {: .info } The `agent.custom` is passed onto the `options` field in [database adapter]({{ site.baseurl }}{% link adapters/database.md %}) calls as `options.agentCustom`. This allows further customisation at the database level e.g. [in `sharedb-mongo` middleware](https://github.com/share/sharedb-mongo#middlewares). +### `clientId` -- string + +> The ID that ShareDB generates for the client. The client uses it as the `src` of its ops, unless it is reconnecting and keeps the ID from its previous connection instead. + +{: .info } +The `clientId` can be set in [`'connect'`]({{ site.baseurl }}{% link middleware/actions.md %}#connect) middleware, for example to [restrict op sources]({{ site.baseurl }}{% link middleware/op-submission.md %}#restricting-op-sources). This doesn't work for clients older than ShareDB v1.2, which are sent their ID before `'connect'` runs. + ### `backend` -- [Backend]({{ site.baseurl }}{% link api/backend.md %}) > The [`Backend`]({{ site.baseurl }}{% link api/backend.md %}) instance that created this `Agent` diff --git a/docs/middleware/op-submission.md b/docs/middleware/op-submission.md index 10f6e2b0b..cad4fa520 100644 --- a/docs/middleware/op-submission.md +++ b/docs/middleware/op-submission.md @@ -141,6 +141,57 @@ backend.on('submitRequestEnd', () => { {: .info :} Since `'submitRequestEnd'` is an event -- not a middleware hook -- it provides no callback, and no way to return an error to the client. It is purely informational. +## Restricting op sources + +Every op carries a `src` and a `seq`, which ShareDB uses to recognise an op that a client resends after reconnecting, so that it isn't applied twice. The `src` is chosen by the client: a reconnecting client reclaims its previous [`clientId`]({{ site.baseurl }}{% link api/agent.md %}#clientid--string) in its handshake, and it can also set `src` on an individual op. + +{: .warn :} +Every op is broadcast with its `src` and `seq`, so a client can submit an op with another client's `src` and that client's next `seq`. ShareDB will then mistake that client's real op for a duplicate, and drop it while telling the client it succeeded. + +If your users must not be able to do this to each other, make each `src` name its user. The `'connect'` hook below gives each new client a `clientId` that names its user. The [`'receive'`]({{ site.baseurl }}{% link middleware/actions.md %}#receive) hook drops a reclaimed ID that names someone else, so that the client is given a new one, and rejects ops whose `src` names someone else: + +```js +const crypto = require('crypto') +const ACTIONS = require('sharedb').MESSAGE_ACTIONS + +function srcFor(userId) { + return userId + ':' + crypto.randomUUID() +} + +function isSrcOf(userId, src) { + if (typeof src !== 'string') return false + const separator = src.lastIndexOf(':') + return separator !== -1 && src.slice(0, separator) === String(userId) +} + +backend.use('connect', (context, next) => { + const userId = authenticatedUserId(context.req) + context.agent.custom.userId = userId + context.agent.clientId = srcFor(userId) + next() +}) + +backend.use('receive', (context, next) => { + const message = context.data + const userId = context.agent.custom.userId + if (message.a === ACTIONS.handshake && message.id != null && !isSrcOf(userId, message.id)) { + delete message.id + } + if (message.a === ACTIONS.op && message.src != null && !isSrcOf(userId, message.src)) { + return next(new Error('Op src belongs to another user')) + } + next() +}) +``` + +The user ID is sent to collaborators and stored with every op as part of its `src`, so use an opaque ID rather than something personal like an email address. + +{: .info :} +A client that connected before you add these hooks, or whose `Connection` is reused after a different user logs in, is given a new ID when it next reconnects. An op it was still sending under its old ID is rejected. + +{: .warn :} +These hooks need clients from ShareDB v1.2 or later. Older clients are sent their ID before `'connect'` runs, so they never learn the new one. + ## Mutating ops Ops may be amended in the `apply` middleware using the special `request.$fixup()` method: diff --git a/lib/agent.js b/lib/agent.js index 9060741f1..d5d8ef0a4 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -25,8 +25,7 @@ function Agent(backend, stream) { // src is a client-configurable "id" which the client will set in its handshake, // and attach to its ops. This should take precedence over clientId if set. // Only legacy clients, or new clients connecting for the first time will use the - // Agent-provided clientId. Ideally we'll deprecate clientId in favour of src - // in the next breaking change. + // Agent-provided clientId. this.src = null; this.connectTime = Date.now(); diff --git a/test/middleware.js b/test/middleware.js index 77cb80a59..f984aec01 100644 --- a/test/middleware.js +++ b/test/middleware.js @@ -5,6 +5,7 @@ var types = require('../lib/types'); var errorHandler = util.errorHandler; var ShareDBError = require('../lib/error'); var sinon = require('sinon'); +var crypto = require('crypto'); var ACTIONS = require('../lib/message-actions').ACTIONS; var ERROR_CODE = ShareDBError.CODES; @@ -63,6 +64,101 @@ describe('middleware', function() { done(); }); }); + + it('uses a clientId set on connect as the connection id and op src', function(done) { + var backend = this.backend; + backend.use('connect', function(request, next) { + request.agent.clientId = 'alice:1'; + next(); + }); + + var srcs = []; + backend.use('submit', function(request, next) { + srcs.push(request.op.src); + next(); + }); + + backend.connect(null, null, function(connection) { + expect(connection.id).to.equal('alice:1'); + connection.get('dogs', 'fido').create({age: 3}, function(error) { + if (error) return done(error); + expect(srcs).to.eql(['alice:1']); + done(); + }); + }); + }); + }); + + describe('restricting op sources', function() { + beforeEach(function() { + function srcFor(userId) { + return userId + ':' + crypto.randomUUID(); + } + + function isSrcOf(userId, src) { + if (typeof src !== 'string') return false; + var separator = src.lastIndexOf(':'); + return separator !== -1 && src.slice(0, separator) === String(userId); + } + + this.backend.use('connect', function(context, next) { + var userId = context.req.userId; + context.agent.custom.userId = userId; + context.agent.clientId = srcFor(userId); + next(); + }); + + this.backend.use('receive', function(context, next) { + var message = context.data; + var userId = context.agent.custom.userId; + if (message.a === ACTIONS.handshake && message.id != null && !isSrcOf(userId, message.id)) { + delete message.id; + } + if (message.a === ACTIONS.op && message.src != null && !isSrcOf(userId, message.src)) { + return next(new Error('Op src belongs to another user')); + } + next(); + }); + }); + + it('keeps a reclaimed id that names the same user', function(done) { + var backend = this.backend; + backend.connect(null, {userId: 42}, function(connection) { + var id = connection.id; + expect(id).to.match(/^42:/); + connection.close(); + backend.connect(connection, {userId: 42}, function() { + expect(connection.id).to.equal(id); + done(); + }); + }); + }); + + it('replaces a reclaimed id that names another user', function(done) { + var backend = this.backend; + backend.connect(null, {userId: 42}, function(connection) { + connection.close(); + connection.id = '7:stale'; + backend.connect(connection, {userId: 42}, function() { + expect(connection.id).to.match(/^42:/); + expect(connection.id).to.equal(connection.agent.clientId); + done(); + }); + }); + }); + + it('rejects an op whose src names another user', function(done) { + this.backend.connect(null, {userId: 42}, function(connection) { + connection.get('dogs', 'fido').on('error', function(error) { + expect(error.message).to.equal('Op src belongs to another user'); + done(); + }); + connection.send({ + a: ACTIONS.op, c: 'dogs', d: 'fido', v: 0, src: '7:1', seq: 1, + create: {type: 'json0', data: {}} + }); + }); + }); }); describe('readSnapshots', function() {