security(collab): apply queued USER_CHANGES to the enqueue-time pad (cross-pad write TOCTOU)

[JohnMcLear]

Fixes GHSA-6mcx-x5h6-rpw2 (triage, medium — runtime-proven). Verified still present on develop: handleUserChanges re-reads sessioninfos[socket.id].padId at apply time (the write target), while the read-only gate and the padChannels key are taken at enqueue time.

The bug (CWE-362 → CWE-862)

Per-socket messages are processed concurrently and a thrown handler does not disconnect the socket. So a same-socket CLIENT_READY can swap the session's padId between a USER_CHANGES being authorized+enqueued and it being applied:

1. CLIENT_READY(A) — a writable pad → padId=A.
2. USER_CHANGES → passes the read-only gate on A, enqueues on channel A.
3. CLIENT_READY(B) — a read-only / unauthorized pad → swaps sessioninfos[socket.id].padId=B (and a denied CLIENT_READY still leaves it mutated, without disconnecting).
4. The queued handleUserChanges re-reads padId=B → writes to B.

A read-only share holder can thus overwrite the pad; the read-only enforcement and per-pad authorization are bypassed.

Fix

The padChannels channel key already is the enqueue-time padId. Thread it into handleUserChanges as authorizedPadId and use it for getPad(...) instead of re-reading the mutable session. The authorization gate and the write now refer to the same pad, closing the window. Non-racing flows are unchanged (session padId == enqueue-time padId).

Test

crossPadWriteToctou.ts invokes handleUserChanges with the session padId already swapped to a victim pad and asserts the change lands on the authorized (argument) pad and never the victim. Confirmed it fails when the vulnerable read is reintroduced (the victim gets modified). Full messages.ts socket suite (13) still passes; tsc --noEmit clean.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix TOCTOU cross-pad writes by binding USER_CHANGES to enqueue-time padId

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Prevent queued USER_CHANGES from re-targeting a different pad after concurrent CLIENT_READY.
• Thread enqueue-time padId into handleUserChanges and use it for pad writes.
• Add regression test that simulates swapped session padId and asserts no victim-pad mutation.

Diagram

graph TD
  A["Socket message"] --> B["Enqueue USER_CHANGES (padId key)"] --> C["padChannels per-pad queue"] --> D["handleUserChanges(authorizedPadId)"] --> E["padManager.getPad(authorizedPadId)"] --> F["appendRevision + updatePadClients"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Snapshot immutable session state at enqueue time

• ➕ Keeps handler signature unchanged by embedding a full session snapshot in the queued task
• ➕ Could protect other queued operations from similar session mutation issues
• ➖ Higher coupling and memory footprint (copies more state than needed)
• ➖ More surface area for subtle divergence between snapshot and live session behavior

2. Enforce per-socket sequential message processing

• ➕ Eliminates same-socket concurrent interleavings that enable this TOCTOU class
• ➖ Broader behavioral/performance impact across all message types
• ➖ Higher regression risk; may hide concurrency bugs rather than fixing authorization binding

3. Disconnect socket on handler errors / denied CLIENT_READY

• ➕ Reduces opportunity for a socket to keep operating in a partially invalid state
• ➖ Not sufficient alone (still possible to race if both messages are valid)
• ➖ Potential UX impact (more disconnects) and wider change scope

Recommendation: The chosen approach (threading the enqueue-time padId through padChannels into handleUserChanges) is the best minimal fix: it binds the authorization/read-only gate and the eventual write to the same pad, closing the TOCTOU window with low behavioral risk and a focused regression test.

Files changed (2) +104 / -4

Bug fix (1) +18 / -4

PadMessageHandler.tsBind queued USER_CHANGES to enqueue-time padId and export seam for test +18/-4

Bind queued USER_CHANGES to enqueue-time padId and export seam for test

• Pass the padChannels channel key (enqueue-time padId) into handleUserChanges as authorizedPadId and use it for padManager.getPad() instead of re-reading sessioninfos[socket.id].padId at apply time. Update error logging to reference authorizedPadId and export handleUserChanges to enable a targeted regression test.

src/node/handler/PadMessageHandler.ts

Tests (1) +86 / -0

crossPadWriteToctou.tsAdd deterministic regression test for cross-pad write TOCTOU +86/-0

Add deterministic regression test for cross-pad write TOCTOU

• Adds a backend test that simulates a swapped session padId and directly invokes handleUserChanges with an explicit authorizedPadId. Asserts the changeset modifies only the authorized pad and leaves the victim pad unchanged.

src/tests/backend/specs/crossPadWriteToctou.ts

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Pads not cleaned on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new crossPadWriteToctou regression test only removes the created pads at the end of the test
body, so any throw (including a failed assertion) before the cleanup will leave the pads in the test
DB. This can cause cross-test contamination and flaky/order-dependent backend test behavior.

Code

src/tests/backend/specs/crossPadWriteToctou.ts[R64-85]

+        const realUpdatePadClients = padMessageHandler.updatePadClients;
+        padMessageHandler.updatePadClients = async () => {}; // neutralise fan-out
+        try {
+          // authorizedPadId = the pad authorized at enqueue time (NOT the swapped
+          // session padId).
+          await padMessageHandler.handleUserChanges(socket, message, authorizedId);
+        } finally {
+          padMessageHandler.updatePadClients = realUpdatePadClients;
+          delete sessioninfos[socket.id];
+        }
+
+        const freshAuthorized = await padManager.getPad(authorizedId);
+        const freshVictim = await padManager.getPad(victimId);
+
+        assert.equal(freshVictim.text(), 'seed\n',
+            'the victim pad (swapped session padId) must NOT be modified');
+        assert.equal(freshAuthorized.text(), 'ZZseed\n',
+            'the change must land on the enqueue-time authorized pad');
+
+        await authorizedPad.remove();
+        await victimPad.remove();
+      });

Relevance

⭐⭐⭐ High

Team accepts ensuring test resources are cleaned up on failure to avoid cross-test
contamination/flakiness (cleanup in finally/afterEach).

PR-#7831
PR-#7798

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test’s only guaranteed cleanup is restoration of updatePadClients and deleting sessioninfos;
pad removal happens after assertions and will be skipped on early failure. Other backend specs use
afterEach-based cleanup to ensure pads are removed even when a test fails mid-execution.

src/tests/backend/specs/crossPadWriteToctou.ts[64-85]
src/tests/backend/specs/Pad.ts[32-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`crossPadWriteToctou.ts` creates two pads (`authorizedPad`, `victimPad`) but removes them only at the end of the test. If `handleUserChanges()` throws or any assertion fails before the final `remove()` calls, the pads persist and can pollute subsequent backend specs.

### Issue Context
There is already a `try/finally` block that restores `updatePadClients` and deletes `sessioninfos[socket.id]`, but pad deletion is outside that `finally`.

### Fix Focus Areas
- src/tests/backend/specs/crossPadWriteToctou.ts[64-85]

### Suggested fix
Wrap the entire test body (after pad creation) in a broader `try/finally` that always removes both pads (guarding for partial setup with null checks), or move the `authorizedPad.remove()` / `victimPad.remove()` calls into the existing `finally` (or an added outer `finally`) so they run even when assertions fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[Copilot]

Pull request overview

Fixes a TOCTOU cross-pad write vulnerability in the collaboration message pipeline by ensuring queued USER_CHANGES are applied to the pad that was authorized at enqueue time (rather than a potentially mutated session padId). Adds a targeted backend regression test that reproduces the cross-pad write scenario and verifies the fix.

Changes:

• Thread the per-pad queue channel key (enqueue-time pad ID) into handleUserChanges() and use it as the write target.
• Adjust failure logging to report the enqueue-time authorized pad ID.
• Add a backend regression test to ensure queued edits cannot be redirected to a “victim” pad via a swapped session padId.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/node/handler/PadMessageHandler.ts	Passes enqueue-time pad ID through the per-pad queue to prevent cross-pad writes in handleUserChanges().
src/tests/backend/specs/crossPadWriteToctou.ts	Adds a regression test that simulates a swapped session padId and asserts changes apply only to the authorized pad.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📜 Skill insights (0)

1. Enqueue padId still racy 🐞 Bug ⛨ Security ⭐ New

Description

padChannels now treats the queue key ch as the authorizedPadId, but ch is still taken from
thisSession.padId at enqueue time after multiple awaits in handleMessage(), so a concurrent
same-socket CLIENT_READY can mutate sessioninfos[socket.id].padId in-place between the
access/readonly gate and the enqueue. This leaves a remaining cross-pad write TOCTOU where a
USER_CHANGES authorized for pad A can still be queued/applied to pad B via a swapped channel key.

Code

src/node/handler/PadMessageHandler.ts[214]

+const padChannels = new Channels((ch, {socket, message}) => handleUserChanges(socket, message, ch));

Evidence

The PR change makes ch the authoritative write target, but handleMessage() still enqueues with
thisSession.padId after async access checks and hook calls, and CLIENT_READY mutates
padId/auth before access is checked and does not revert on denial. Because the disconnect check
only compares object identity, in-place mutation can slip through and change the channel key before
enqueue, reintroducing cross-pad mis-targeting.

src/node/handler/PadMessageHandler.ts[207-215]
src/node/handler/PadMessageHandler.ts[508-590]
src/node/handler/PadMessageHandler.ts[404-498]
src/node/handler/PadMessageHandler.ts[515-521]
src/node/handler/PadMessageHandler.ts[575-577]
src/node/handler/PadMessageHandler.ts[831-863]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `authorizedPadId` argument is derived from the `padChannels` channel key (`ch`), but the channel key is still computed from `thisSession.padId` late in `handleMessage()` after `await securityManager.checkAccess(...)` and plugin hook awaits. Because `CLIENT_READY` mutates `sessioninfos[socket.id].padId` in-place (and access denial does not revert/disconnect), the value used for enqueueing can diverge from the pad that was authorized/gated.

### Issue Context
This PR fixed the apply-time read in `handleUserChanges()` by using `authorizedPadId`, but it assumes the enqueue-time channel key always represents the pad that was authorized for the message.

### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[389-591]
- src/node/handler/PadMessageHandler.ts[207-215]
- src/node/handler/PadMessageHandler.ts[831-863]

### Suggested fix approach
1. Capture an immutable snapshot of the authorized pad identity for the current message **before any await that can yield** (or at least before the hook awaits), and use that snapshot consistently for:
  - readonly gating decision
  - `padChannels.enqueue(snapshotPadId, ...)`
  - the `authorizedPadId` passed into `handleUserChanges()` (either via the channel key or as an explicit field on the queued task)
2. Add a guard immediately before enqueue: if `sessioninfos[socket.id].padId` or `.auth` no longer matches the snapshot, drop/reject the message (treat it as a mid-flight pad switch).
3. Consider changing CLIENT_READY handling to replace the entire session object (or otherwise version it) so `sessioninfos[socket.id] !== thisSession` can detect in-place padId/auth mutation.

This ensures the pad used for authorization/readonly gating is the same pad used for queueing and applying edits.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Exported internal write handler 🐞 Bug ⚙ Maintainability ⭐ New

Description

handleUserChanges is now exported even though it relies on caller-established invariants
(readonly/access checks, pendingEdits accounting) and calls padManager.getPad() + appends
revisions. This increases the chance of accidental misuse by other in-process code/tests and makes
the module’s public surface harder to maintain safely.

Code

src/node/handler/PadMessageHandler.ts[R1022-1026]

+// Exported for the cross-pad TOCTOU regression test (GHSA-6mcx-x5h6-rpw2), which
+// verifies the write targets the enqueue-time `authorizedPadId` argument rather
+// than the mutable sessioninfos[socket.id].padId.
+exports.handleUserChanges = handleUserChanges;
+

Evidence

The PR adds a new export for handleUserChanges. The function mutates global stats (pendingEdits) and
applies edits without performing the readonly gate that is enforced in handleMessage() before
enqueue, so external in-process callers could violate its implicit preconditions.

src/node/handler/PadMessageHandler.ts[831-836]
src/node/handler/PadMessageHandler.ts[536-591]
src/node/handler/PadMessageHandler.ts[1022-1026]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`exports.handleUserChanges = handleUserChanges;` exposes an internal function that assumes the normal `handleMessage()` path has already performed access/readonly gating and incremented `pendingEdits`. Keeping it as a general export makes future internal callers more likely to bypass those preconditions.

### Issue Context
The export was added to support a regression test seam.

### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[831-1026]
- src/tests/backend/specs/crossPadWriteToctou.ts[1-86]

### Suggested fix approach
- Prefer a test-only export namespace, e.g. `exports._test = {handleUserChanges}` gated by `process.env.NODE_ENV === 'test'` (or similar), or move the core logic to a separate module that can be imported by both the handler and tests without expanding PadMessageHandler’s public API.
- If you keep the export, add explicit runtime precondition checks inside `handleUserChanges` (e.g., require `authorizedPadId` non-empty, require `thisSession` matches expected pad, and/or avoid touching `pendingEdits` when invoked outside the queue path).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Pads not cleaned on failure 🐞 Bug ☼ Reliability

Description

The new crossPadWriteToctou regression test only removes the created pads at the end of the test
body, so any throw (including a failed assertion) before the cleanup will leave the pads in the test
DB. This can cause cross-test contamination and flaky/order-dependent backend test behavior.

Code

src/tests/backend/specs/crossPadWriteToctou.ts[R64-85]

+        const realUpdatePadClients = padMessageHandler.updatePadClients;
+        padMessageHandler.updatePadClients = async () => {}; // neutralise fan-out
+        try {
+          // authorizedPadId = the pad authorized at enqueue time (NOT the swapped
+          // session padId).
+          await padMessageHandler.handleUserChanges(socket, message, authorizedId);
+        } finally {
+          padMessageHandler.updatePadClients = realUpdatePadClients;
+          delete sessioninfos[socket.id];
+        }
+
+        const freshAuthorized = await padManager.getPad(authorizedId);
+        const freshVictim = await padManager.getPad(victimId);
+
+        assert.equal(freshVictim.text(), 'seed\n',
+            'the victim pad (swapped session padId) must NOT be modified');
+        assert.equal(freshAuthorized.text(), 'ZZseed\n',
+            'the change must land on the enqueue-time authorized pad');
+
+        await authorizedPad.remove();
+        await victimPad.remove();
+      });

Relevance

⭐⭐⭐ High

Team accepts ensuring test resources are cleaned up on failure to avoid cross-test
contamination/flakiness (cleanup in finally/afterEach).

PR-#7831
PR-#7798

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test’s only guaranteed cleanup is restoration of updatePadClients and deleting sessioninfos;
pad removal happens after assertions and will be skipped on early failure. Other backend specs use
afterEach-based cleanup to ensure pads are removed even when a test fails mid-execution.

src/tests/backend/specs/crossPadWriteToctou.ts[64-85]
src/tests/backend/specs/Pad.ts[32-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`crossPadWriteToctou.ts` creates two pads (`authorizedPad`, `victimPad`) but removes them only at the end of the test. If `handleUserChanges()` throws or any assertion fails before the final `remove()` calls, the pads persist and can pollute subsequent backend specs.
### Issue Context
There is already a `try/finally` block that restores `updatePadClients` and deletes `sessioninfos[socket.id]`, but pad deletion is outside that `finally`.
### Fix Focus Areas
- src/tests/backend/specs/crossPadWriteToctou.ts[64-85]
### Suggested fix
Wrap the entire test body (after pad creation) in a broader `try/finally` that always removes both pads (guarding for partial setup with null checks), or move the `authorizedPad.remove()` / `victimPad.remove()` calls into the existing `finally` (or an added outer `finally`) so they run even when assertions fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Test decrements pendingEdits 🐞 Bug ◔ Observability ⭐ New

Description

The new regression test calls handleUserChanges() directly, but the handler unconditionally
decrements stats.counter('pendingEdits') on entry, leaving the global counter lower than expected
for the rest of the mocha run. This can pollute shared-suite metrics and make later counter-based
diagnostics misleading.

Code

src/tests/backend/specs/crossPadWriteToctou.ts[R64-73]

+        const realUpdatePadClients = padMessageHandler.updatePadClients;
+        padMessageHandler.updatePadClients = async () => {}; // neutralise fan-out
+        try {
+          // authorizedPadId = the pad authorized at enqueue time (NOT the swapped
+          // session padId).
+          await padMessageHandler.handleUserChanges(socket, message, authorizedId);
+        } finally {
+          padMessageHandler.updatePadClients = realUpdatePadClients;
+          delete sessioninfos[socket.id];
+        }

Evidence

The test invokes handleUserChanges directly, and the handler decrements pendingEdits immediately
upon entry; the corresponding increment normally happens in the USER_CHANGES path before enqueue,
which the test bypasses.

src/tests/backend/specs/crossPadWriteToctou.ts[64-73]
src/node/handler/PadMessageHandler.ts[831-836]
src/node/handler/PadMessageHandler.ts[588-591]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`handleUserChanges()` decrements the global `pendingEdits` counter, but the regression test invokes it directly instead of going through the normal enqueue path that increments the counter.

### Issue Context
This is test-only, but the backend suite runs in a single process and shares the stats collection.

### Fix Focus Areas
- src/tests/backend/specs/crossPadWriteToctou.ts[64-73]
- src/node/handler/PadMessageHandler.ts[831-836]

### Suggested fix approach
- In the test, increment the counter before calling `handleUserChanges()` (and/or restore it after), or stub the `stats.counter('pendingEdits')` methods for the duration of the test.
- Alternatively, drive the regression through the real enqueue path (padChannels.enqueue) so accounting matches production behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JohnMcLear]

Good catch @qodo-free-for-open-source-projects — you're right, the first cut wasn't sufficient. The channel key was still read from thisSession.padId at enqueue, i.e. after the awaits in handleMessage() (checkAccess + the security/message hooks), so a concurrent same-socket CLIENT_READY could still swap it in place between the read-only gate and the enqueue.

Fixed properly in the latest commit: the pad id + read-only flag are now pinned into a consistent snapshot (messagePadId/messageReadonly) alongside auth, before those awaits, and that snapshot drives the read-only gate, the queue key and the write. The regression test now drives a real USER_CHANGES over a socket and swaps the session padId mid-message via a handleMessageSecurity hook (the actual concurrent-CLIENT_READY race) — verified it fails when the pinned key is reverted. That also let me drop the handleUserChanges export and the direct-call test (your maintainability / pendingEdits / cleanup findings).

[qodo-free-for-open-source-projects]

Good catch @qodo-free-for-open-source-projects — you're right, the first cut wasn't sufficient. The channel key was still read from thisSession.padId at enqueue, i.e. after the awaits in handleMessage() (checkAccess + the security/message hooks), so a concurrent same-socket CLIENT_READY could still swap it in place between the read-only gate and the enqueue.

Fixed properly in the latest commit: the pad id + read-only flag are now pinned into a consistent snapshot (messagePadId/messageReadonly) alongside auth, before those awaits, and that snapshot drives the read-only gate, the queue key and the write. The regression test now drives a real USER_CHANGES over a socket and swaps the session padId mid-message via a handleMessageSecurity hook (the actual concurrent-CLIENT_READY race) — verified it fails when the pinned key is reverted. That also let me drop the handleUserChanges export and the direct-call test (your maintainability / pendingEdits / cleanup findings).

Thanks for the update. Pinning messagePadId and messageReadonly before the asynchronous checks, then using that snapshot for the gate, queue key, and write target, closes the cross-pad TOCTOU described here. The end-to-end regression test also provides appropriate coverage for the actual race.

[qodo-code-review]

Good catch @qodo-free-for-open-source-projects — you're right, the first cut wasn't sufficient. The channel key was still read from thisSession.padId at enqueue, i.e. after the awaits in handleMessage() (checkAccess + the security/message hooks), so a concurrent same-socket CLIENT_READY could still swap it in place between the read-only gate and the enqueue.

Fixed properly in the latest commit: the pad id + read-only flag are now pinned into a consistent snapshot (messagePadId/messageReadonly) alongside auth, before those awaits, and that snapshot drives the read-only gate, the queue key and the write. The regression test now drives a real USER_CHANGES over a socket and swaps the session padId mid-message via a handleMessageSecurity hook (the actual concurrent-CLIENT_READY race) — verified it fails when the pinned key is reverted. That also let me drop the handleUserChanges export and the direct-call test (your maintainability / pendingEdits / cleanup findings).

Thanks for the update. Pinning messagePadId and messageReadonly before the awaits, then using that snapshot for authorization, queueing, and the write, addresses finding 1 and closes the cross-pad TOCTOU. The end-to-end regression test also provides appropriate coverage for the actual race.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.