[Security] PersistentActor accepts out-of-order events from snapshot store

[pathosDev]

Severity / Size

• Severity: MEDIUM — recovery applies events in delivery order; if the journal returns events out of seq order (whether because of journal bug, attacker tampering, or backend misbehaviour), the actor's state silently diverges from any legitimate replay. No exception thrown; the bug is invisible until later cross-actor inconsistency is observed.
• Size: S (~1d).
• Threat model: backend that returns events in wrong order — could be a buggy custom Journal implementation, a corrupted backing store, or an attacker with write access to the journal (shared SQLite file, compromised Cassandra coordinator, S3-like backend with stale-read semantics). Companion to the snapshot-seq-corruption fix (99de741).

Affected files

• src/persistence/PersistentActor.ts:193-198 — the replay loop. No per-event seq validation. Trusts journal.read() to return events in monotonically-increasing seq order.
• src/persistence/Journal.ts — interface contract. Documents read() as returning events in (fromSeq, ..., toSeq] order but doesn't enforce it.

Background

PersistentActor.recover() already validates the snapshot's seq against the journal (snapshot-seq integrity fix 99de741). But the per-event seq numbers inside journal.read()'s return are trusted blindly: the loop walks them in delivery order and applies onEvent to each.

The contract says read(pid, fromSeq, toSeq?) returns events in [fromSeq, ..., toSeq] order. In-memory and SQLite journals respect this naturally — they ORDER BY seq. But:

• A custom Journal impl might forget the ORDER BY clause.
• A distributed backend (Cassandra) might return rows in a different order if the partition key isn't seq-bound.
• An attacker with row-level write access could shuffle the journal's storage representation.
• A read replica with eventually-consistent visibility could deliver events that were committed in order but are visible in a different order during the replay window.

The pattern matches 99de741: don't trust the persistence-layer return; validate at the recovery boundary.

Exploit walkthrough

Setup: PersistentActor account-42 has journal events:

• seq=1: {kind: 'created', balance: 0}
• seq=2: {kind: 'deposit', amount: 100}
• seq=3: {kind: 'withdraw', amount: 50}

Legitimate state after recovery: {balance: 50}.

Adversarial scenario — a buggy custom journal returns the events in order [1, 3, 2]:

Step 1 — recovery starts. Snapshot loaded (if any) at seq < 1 → starts from seq=1.

Step 2 — replay loop:

• event 1 (seq=1): apply → state = {balance: 0} (created). this._seq = 1.
• event 2 (seq=3): apply → state = {balance: -50} (withdraw before deposit → negative). this._seq = 3.
• event 3 (seq=2): apply → state = {balance: 50} (deposit lands). this._seq = 2.

After replay, this._seq = 2 (last applied seq). The next persist() will use expectedSeq = 2 + 1 = 3 — colliding with the existing seq=3 event in the journal. Append throws JournalConcurrencyError. The actor sees a phantom concurrency error on its first persist after recovery.

State during the negative-balance window may have triggered side-effects (logging, metrics, alerts). The final state happens to match — but only by coincidence (idempotent CRDT-like ops); for non-commutative event semantics, the final state is wrong.

The harder failure mode: events that are non-commutative — final state depends on order. Example: a setPassword event followed by disableAccount. If reordered: disabled-then-password-set → account is enabled but with the old password. Privilege escalation or unintended visibility.

How the 8 already-landed security fixes inform this

• Snapshot seq integrity (99de741): set the precedent — validate persistence-layer claims at the recovery boundary. This issue extends the same defense to per-event seqs.
• Gossip version-cap (709431b): pattern of "monotonic invariant must hold, reject otherwise". Apply to event seqs during replay.

Fix design

One-track defense, mirroring the snapshot-seq validation.

Track 1 — per-event seq monotonicity check.

In the replay loop:

let expectedSeq = this._seq + 1;
for (const ev of events) {
  if (!Number.isInteger(ev.sequenceNr) || ev.sequenceNr < expectedSeq) {
    throw new Error(
      `[persistence] '${this.persistenceId}' journal returned event with sequenceNr=${ev.sequenceNr}, ` +
      `expected ${expectedSeq}+ (events must be strictly monotonically increasing from ${this._seq + 1})`,
    );
  }
  if (ev.sequenceNr > expectedSeq) {
    // Gap detection: ev.sequenceNr > expected means events between
    // `expected` and `ev.sequenceNr` are missing.  Decision: reject.
    // Recovery on a missing-event journal would produce wrong state.
    throw new Error(
      `[persistence] '${this.persistenceId}' journal has gap: expected seq ${expectedSeq}, got ${ev.sequenceNr}`,
    );
  }
  const decoded = decodeEvent<Event>(ev.event, evAdapter);
  this._state = this.onEvent(this._state, decoded);
  this._seq = ev.sequenceNr;
  expectedSeq = ev.sequenceNr + 1;
}

Three properties enforced:

• Integer non-negative: seq must be a sane number.
• Monotonic non-decreasing: each event's seq ≥ expected, never goes backwards.
• No gaps: each event's seq exactly matches expected.

The "no gaps" choice is the strict reading. An alternative is to allow gaps (skip-and-warn) for journals that legitimately have holes from deleted-events / compaction. But this contradicts the documented Journal.delete(pid, toSeq) semantics — deletion compacts the prefix, not the middle. So holes in read() output indicate a real issue.

Track 2 — counter metric.

persistent_actor_replay_rejected_total{reason: 'out_of_order' | 'gap'} for operator visibility.

Track 3 — diagnostic context in the error.

Include the actor's persistenceId, the expected seq, and the offending event's seq in the error message. Operators investigating a "phantom CAS error" can trace back to the bad journal read.

API surface

No public-API change. Internal recovery loop hardened.

Backward compatibility

• Legitimate journals (InMemory, SQLite) already return monotonic events — unchanged behaviour.
• Cassandra journal: verify that the events_by_persistence_id query orders by seq (it should, by the CQL primary-key design). Add a regression test against FakeCassandraClient to confirm.
• Custom user journals: any impl that returns events out of order will now fail loudly at recovery instead of silently corrupting state. Loud failure is an improvement.

Test plan

1. Exploit-equivalent test (tests/unit/persistence/PersistentActor-replay-order.test.ts): use a custom stub Journal that returns events [seq=1, seq=3, seq=2]; recovery throws with a clear "out-of-order" message; actor is terminated by supervisor.

2. Gap-detection test: Journal returns events [seq=1, seq=2, seq=4] (missing seq=3); recovery throws "gap" error.

3. Boundary test: Journal returns events [seq=0] while expectedSeq=0 → accepted (legit edge case); [seq=-1] rejected.

4. Backward-compat test: in-memory + SQLite journal replay produces correctly-ordered events for all existing PersistentActor tests; none of them break.

5. Regression: existing PersistentActor.test.ts + snapshot-integrity.test.ts (the existing seq-tampering test) all pass.

Acceptance criteria

• Replay loop validates each event's sequenceNr is finite, non-negative integer, equal to expected (no gaps, no out-of-order).
• persistent_actor_replay_rejected_total metric exposed with reason label.
• Error messages include persistenceId + expected/actual seq.
• Five new tests pass; existing PersistentActor tests green.
• Plan-doc + README "Known security caveats" updated on land.

[pathosDev]

Re-derived independently in the production-readiness review tracked in #913, which folded a contiguity finding into this issue.

The folded finding proposed asserting that the replayed stream has no gaps, alongside the ordering validation this issue already proposes. Worth recording is how much that one assertion is load-bearing, because every gap it would catch is reachable from framework code rather than from a hostile store:

• CassandraJournal.ts:157-165 claim-then-write with a best-effort release — the JSDoc calls it "the documented crash window".
• D1Client.ts:166-168 — withTransaction with no transaction, so RelationalJournal.append's per-event inserts can leave a prefix.
• MongoJournal.ts:55-60 — a mid-batch infrastructure failure.
• CachedSnapshotStore.ts:76-84 — a stale entry (5-minute TTL) surviving a compaction.

Today replayState folds whatever journal.read returns with no check (src/persistence/Replay.ts:93-97), so each of those becomes silently wrong state and a "recovery complete" line at debug level. With the assertion they become a loud recovery failure. That is roughly twenty lines for four crash windows, which is the best ratio in the persistence layer.