Skip to content

POST /documents/:slug/ops: route-level DB reapply (source: 'rest-ops') can replay stale markdown into live collab rooms #63

Description

@devYRPauli

Summary

POST /documents/:slug/ops performs a route-level broad reapply of the persisted DB row into the live collab room after non-rewrite.apply mutations, using source: 'rest-ops'. Unlike every other server-side write source, rest-ops is not in shouldBlockLegacyLiveApplySource, so this reapply is not blocked when a live shared doc is present. A point-in-time DB snapshot pushed into a live room can replay stale markdown over concurrent edits that are not yet persisted.

The repo's own collab-security test already asserts this reapply should be gone, but the corresponding refactor was never landed, so the test fails on main.

Where

server/routes.ts, in the /documents/:slug/ops success path (around line 1729-1743):

if (op !== 'rewrite.apply') {
  try {
    const collabRuntime = getCollabRuntime();
    if (collabRuntime.enabled) {
      const updatedDoc = getDocumentBySlug(slug);
      if (updatedDoc) {
        const applyOptions = {
          markdown: typeof updatedDoc.markdown === 'string' ? updatedDoc.markdown : undefined,
          marks: parseJson(updatedDoc.marks),
          source: 'rest-ops',
        };
        await applyCanonicalDocumentToCollab(slug, applyOptions);
        ...

The guard that is supposed to stop legacy reverse-flow writes onto live docs is shouldBlockLegacyLiveApplySource in server/collab.ts:

function shouldBlockLegacyLiveApplySource(source: string): boolean {
  if (!source) return false;
  return source === 'rest-put'
    || source === 'engine'
    || source === 'engine-suggestion-accept'
    || source === 'library'
    || source.startsWith('agent')
    || source.startsWith('rewrite:');
}

rest-ops is absent here, while its REST sibling rest-put (PUT /documents/:slug) is present. applyCanonicalDocumentToCollabInner only blocks-and-marks-stale when shouldBlockLegacyLiveApplySource(origin) is true, so a rest-ops apply proceeds onto a live room.

Impact

A non-rewrite.apply /ops mutation that lands while a client is live can overwrite the live fragment with the DB row markdown. Any concurrent client edit that has not yet been persisted to the row is at risk of being clobbered (and the agent op can land against shifted content), returning 200 success.

Evidence

src/tests/collab-security.test.ts encodes the intended end state:

assert(
  routesSource.includes('Route-level reapplication from the DB can replay stale markdown')
    && !routesSource.includes("source: 'rest-ops'"),
  'Expected /documents/:slug/ops to avoid broad DB reapply after engine-backed mutations',
);

On current main, routes.ts still contains source: 'rest-ops' and does not contain that comment, so this assertion fails. The intended refactor (route stops reapplying; non-rewrite ops propagate through the canonical Yjs path the way rewrite.apply does) appears to have never been completed.

Question / possible directions

Is the rest-ops route-level reapply intended, or is the collab-security guard the intended target state? Two directions, depending on the answer:

  1. Complete the refactor the test anticipates: have non-rewrite.apply /ops mutations commit through the canonical rewrite/Yjs path and drop the route-level DB reapply, so the live room is never re-seeded from a stale row.
  2. Minimal hardening for parity: add rest-ops to shouldBlockLegacyLiveApplySource so it is treated like rest-put (block on live docs, mark the projection stale, let clients resync from canonical).

Happy to send a PR for whichever direction you prefer.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions