[Security] PersistentFSM stateTimeout race on rapid transitions

[pathosDev]

Severity / Size

• Severity: LOW (downgraded from MEDIUM after code review — see "Caveat")
• Size: S
• Threat model: closed-group cluster — risk is state corruption under rapid state transitions. An attacker that can drive the FSM to transition A→B→A within a single afterMs window can cause a stale timer to fire prematurely after the second A entry.

Affected files

• src/fsm/PersistentFSM.ts:352-371 — armTimerForCurrentState cancels previous + arms new timer + captures stateAtArm.
• src/fsm/PersistentFSM.ts:373-378 — cancelTimer calls cancel() on the scheduler-returned Cancellable.
• src/fsm/PersistentFSM.ts:386-425 — fireTimeoutTransition checks curr.state !== stateAtArm; if so, refuses and re-arms.
• src/fsm/PersistentFSM.ts:286-298 — onReceive intercepts the __fsm_state_timeout__ self-tell.

Caveat — audit framing vs reality

The audit lists this as "rapid transitions can leave an old timer armed when a new state is entered → spurious timeout fires in the wrong state". Code inspection shows the framework already has two layers of defence:

1. armTimerForCurrentState calls cancelTimer() first (line 353). On every transition, the previous timer is cancelled before the new one is armed.
2. fireTimeoutTransition checks stateAtArm (line 388). If a state-name mismatch is detected (i.e., a stale timer's fire arrived after a transition out), the fire is refused.

So the framework correctly handles:

• Race window: timer fires while in a different state: stateAtArm check catches it → refuse.
• Race window: timer fires while in the same state name but for a different "incarnation" of that state: stateAtArm sees only the state name, not the incarnation — so a stale fire after A→B→A produces a fire with stateAtArm = A matching the second A's curr.state = A. It fires. The fire is valid for the current state, but uses a timer that was armed for the previous incarnation of A. The user expected afterMs countdown from the second A entry; they get a fire earlier.

That's the actual residual race — the audit was right that there's a race, but wrong about its consequence. The fire isn't "in the wrong state"; it's "in the right state but with an unexpectedly-short countdown".

Background

PersistentFSM's _timeout config is per-state: enter state X → arm timer for X's afterMs → either a command transitions out (cancel) or the timer fires (transition to X's next).

The race shape:

1. FSM enters state A. armTimerForCurrentState() schedules t1 for afterMs = 1000.
2. After 500ms, command moves A → B. armTimerForCurrentState() cancels t1, schedules t2 for B's afterMs.
3. After 100ms in B, command moves B → A. armTimerForCurrentState() cancels t2, schedules t3 for A's afterMs = 1000.
4. But: t1's callback was already in the macrotask queue before step 2's cancel() ran. clearTimeout on a queued callback in Node doesn't unqueue it.
5. ~500ms after step 3 (= ~600ms after step 2), t1's callback fires: this.self.tell({ kind: '__fsm_state_timeout__', stateAtArm: 'A' }).
6. onReceive calls fireTimeoutTransition('A'). Check: curr.state ('A') !== stateAtArm ('A') is false → fires.
7. The user wanted a fire 1000ms after step 3. They get a fire 600ms after step 3.

Result: the FSM transitions out of A (1000 - 600) = 400ms earlier than the user's contract. For an "order timeout 5 minutes" FSM this is mostly harmless; for a "rate-limit cooldown 30 seconds" FSM it's a real bug (cooldown shortened).

Exploit walkthrough

Step 1 — App models payment with a stateTimeout for "abandoned cart" cleanup:

transitions = {
  pending: {
    pay:    { event: { kind: 'paid' }, next: 'paid' },
    _timeout: {
      afterMs: 5 * 60 * 1000,  // 5 min idle → mark abandoned
      event: { kind: 'abandoned' },
      next: 'abandoned',
    },
  },
  paid: {
    refund: { event: { kind: 'refunded' }, next: 'pending' },  // re-enters pending
  },
  abandoned: { /* terminal */ },
};

Step 2 — Customer flow:

• t=0: enter pending. Timer t1 armed for t=5min.
• t=1min: customer pays. Transition pending → paid. t1 cancelled.
• t=1min+ε: customer disputes, requests refund. Transition paid → pending. Timer t3 armed for t=6min.
• t=5min: t1's queued callback finally fires (it was queued at scheduling time; Node's event-loop fairness can hold callbacks across many state transitions if the actor is busy).
• t=5min: fireTimeoutTransition('pending') is called with stateAtArm = 'pending'. curr.state === 'pending'. It fires. Cart marked abandoned 1 minute after the customer's last interaction — but they're still actively using it.

Step 3 — Customer sees "cart abandoned" in their UI mid-checkout. Files a support ticket: "I was using your site, why did my cart disappear?"

Realistic worst case: stateTimeout fires afterMs later than the original arm — not the re-entry. Bug surface is "this timer fires too early after a re-entry within afterMs".

How the 8 already-landed security fixes inform this

• Snapshot seq integrity — added a strict identity check on incoming data. Same shape: add an incarnation counter to the timer fire so stale fires can be identified beyond just state name.
• Hello-handshake hijack defence — used an opaque token (the hello sentinel) to prove "this packet matches the handshake we just did". Same shape: use a generation counter that increments on every arm.
• idempotency body-fingerprint — tied a cached result to the request body's hash; mismatches caught explicitly. Same shape: tie a timer fire to the arm-time's generation; mismatches rejected.

Fix design

Track 1 — Generation counter (primary). Add a generation counter that increments on every armTimerForCurrentState. The timer's fire payload carries the generation; fireTimeoutTransition checks both state AND generation:

interface FsmTimeoutFire<SName extends string> {
  readonly kind: '__fsm_state_timeout__';
  readonly stateAtArm: SName;
  readonly generation: number;   // NEW
}

private _timeoutGeneration = 0;
private _timeoutTimer: Cancellable | null = null;
private _timeoutCurrentGeneration: number | null = null;  // NEW

private armTimerForCurrentState(): void {
  this.cancelTimer();
  const state = this.currentFsmState;
  const timeout = this.transitions[state]?.[FSM_TIMEOUT_KEY];
  if (!timeout) return;
  const generation = ++this._timeoutGeneration;
  this._timeoutCurrentGeneration = generation;
  this._timeoutTimer = this.system.scheduler.scheduleOnceFn(
    timeout.afterMs,
    () => {
      this._timeoutTimer = null;
      const fire: FsmTimeoutFire<SName> = {
        kind: '__fsm_state_timeout__',
        stateAtArm: state,
        generation,
      };
      (this.self as ActorRef<unknown>).tell(fire);
    },
  );
}

private cancelTimer(): void {
  if (this._timeoutTimer) {
    this._timeoutTimer.cancel();
    this._timeoutTimer = null;
  }
  this._timeoutCurrentGeneration = null;
}

private async fireTimeoutTransition(stateAtArm: SName, generation: number): Promise<void> {
  if (generation !== this._timeoutCurrentGeneration) {
    // Stale fire from a previously-cancelled timer.  Drop silently.
    this.log.debug(
      `PersistentFSM: stale stateTimeout fire (generation=${generation}, current=${this._timeoutCurrentGeneration}) — dropped`,
    );
    return;
  }
  // ... existing fire logic (state-match check still applies as defence-in-depth)
}

Track 2 — Defence-in-depth: keep the existing state-name check. Even with the generation counter, the state-name check from the current code is kept as a second-layer guard.

Track 3 — Metric. fsm_timeout_stale_fires_total Counter — counts the (now-correctly-suppressed) stale fires. Lets ops see how often this race actually triggers.

Track 4 — Same fix on FSM (the non-persistent variant). FSM has the same shape; same race, same fix.

API surface

No public API change. The FsmTimeoutFire interface is internal (declared interface not export interface).

Backward compatibility

Non-breaking. Existing user code unaffected. Behaviour change: stateTimeout fires are now correctly scoped to their specific arm-time, not just the state name.

Test plan

1. Race-reproduction (exploit) — manually orchestrate: enter A → 100ms later enter B → 100ms later re-enter A → wait for A's original afterMs. Verify (without the fix) that the stale fire happens early; (with the fix) that it's suppressed and the second-arm's timer fires at the correct moment.
2. Generation counter monotonicity — re-arm 1000 times, verify generation increments and stale fires are all dropped.
3. State-name + generation both checked — even if generation matches by accident (shouldn't happen, but defence-in-depth), state-name mismatch still rejects.
4. Metric correctness — fsm_timeout_stale_fires_total increments on stale drops.
5. Same fix on FSM — non-persistent FSM behaves identically.
6. Cancel-then-no-arm — transition to a state with no _timeout cancels current timer, sets _timeoutCurrentGeneration = null; any stale fire is dropped.
7. Regression — existing PersistentFSM stateTimeout tests pass.

Acceptance criteria

• PersistentFSM._timeoutGeneration + check in fireTimeoutTransition.
• Same fix applied to FSM (non-persistent variant).
• FsmTimeoutFire payload carries generation.
• fsm_timeout_stale_fires_total metric emitted.
• Test suite covers all 7 cases above.
• CHANGELOG entry under "FSM stateTimeout race fix (generation-counter guard)".