Validate client-supplied handshake id against malformed input

[anupamme]

What changed

Agent.prototype._checkRequest in lib/agent.js validates the handshake
id is a string, consistent with the type checks already applied to the
other client-supplied string fields (c, d, ch, and the presence id).

Update per review feedback: an earlier version of this change also
rejected id values like __proto__/constructor/hasOwnProperty via
util.isDangerousProperty, framed as closing a prototype-pollution gap. As
pointed out in review, that framing didn't hold up: agent.src (and the
op-level src it feeds, see lib/agent.js:25-29) is never used as an
object/map key anywhere in lib/ - only in === comparisons and plain
property assignments (this.src = src, {src: ...} on outgoing messages).
That's unlike collection/doc id/presence id/channel, which genuinely
are used as keys elsewhere (e.g. the subscribedDocs/subscribedPresences
maps), which is why isDangerousProperty is warranted for those.

So this has been narrowed to just the plain string-type check. It's not a
prototype-pollution or type-confusion fix - it's a protocol-hygiene
consistency change, rejecting the handshake with ERR_MESSAGE_BADLY_FORMED
/ 'Invalid client id' if id is set and isn't a string.

What this is not

An earlier version of this PR framed this as an authentication/CWE-287 fix,
claiming it prevented client-id impersonation. That framing was also wrong
and has been corrected:

• agent.src is a client-chosen correlation token, echoed back on
  ops/presence for self-echo suppression (_isOwnOp, _handlePresenceData)
  and used as a resubmission dedup key (lib/submit-request.js, lib/ot.js).
  It is never treated as a verified identity anywhere in lib/.
• ShareDB's actual, documented extension point for authenticating/authorizing
  a connection is the connect/receive middleware (lib/backend.js),
  which this change doesn't touch.
• So: this change does not, and architecturally cannot, prevent a client
  from claiming any (string) client id it likes - that was never something
  the handshake id field protected against.

Tests

test/agent.js's handshake suite:

• Rejects a handshake whose id is a non-string.
• Accepts a handshake with a normal string id (including values like
  __proto__, which are valid ids now that isDangerousProperty has been
  dropped for this field), and confirms it's set on agent.src and echoed
  back to the client.

🤖 Generated with Claude Code

[alecgibson]

Thanks for the submission! This probably looks like a sensible change, but a couple of notes:

• your PR title talks about "authentication", but this change has nothing to do with authentication?
• this change doesn't have any tests flexing the bug you're trying to fix
• this change doesn't actually address one of the concerns you raise in your PR description that an attacker can impersonate any client ID

Could you please address the above and resubmit for review? Thanks!

[anupamme]

Addressed. Pls review.

[alecgibson]

I don't think the request.id is actually ever used as an object key anywhere unless I'm missing something? What actual attack vector are you trying to guard against here?

Also I think we already validate the id type as a string both on client connection and during checkOp. Again, what exact bug are you trying to fix here?

[anupamme]

You're right, and thanks for pushing on this. I traced every use of agent.src/op.src across lib/ (agent.js, submit-request.js, ot.js, backend.js, db/index.js, client/*) and confirmed it's never used as an object/map key anywhere - only === comparisons and plain property assignment (this.src = src, {src: ...} on outgoing messages). That's unlike collection/doc id/presence id/channel, which genuinely are used as keys elsewhere (e.g. the subscribedDocs/subscribedPresences maps), which is why isDangerousProperty is warranted there.

So there's no real prototype-pollution vector here, and the type-confusion angle is also largely redundant: a string-type check for the handshake id already existed inline before this PR, and lib/ot.js's checkOp already rejects a non-string op.src before any op is persisted.

I've pushed a commit that drops the isDangerousProperty check for this field and keeps only the plain typeof === 'string' check, for consistency with the other fields _checkRequest validates - not framed as a security fix. Updated the PR description and removed the now-invalid __proto__/constructor/hasOwnProperty rejection tests accordingly (those are valid ids now).