[Security] DurableState ETag-refresh race in upsert path

[pathosDev]

Severity / Size

• Severity: MEDIUM — narrow timing window between load() cache-refresh and the subsequent put(). Backend's ifMatch CAS does catch concurrent writers, so the data isn't actually corrupted. But the framework's etagCache can be left holding a stale value after the conflict, leading to spurious or repeated CAS errors on subsequent calls until the next legitimate refresh.
• Size: S (~1d).
• Threat model: not an attacker. This is a concurrency-correctness issue: two legitimate writers racing. Visible as confusing error chains in production where the same actor sees DurableStateConcurrencyError repeatedly even though it's the only writer (because the etagCache is stuck).

Affected files

• src/persistence/durable-state-stores/ObjectStorageDurableStateStore.ts:144-163 — the cache-refresh-then-put sequence. Window: between await this.load<S>(...) returning + etagCache.set(...) and the subsequent backend.put(..., ifMatch: refreshedEtag).
• src/persistence/durable-state-stores/ObjectStorageDurableStateStore.ts:177-183 — the CAS-error catch path. Throws DurableStateConcurrencyError(..., actualRevision: -1) without refreshing or invalidating the cache.

Background

upsert(pid, expectedRevision, state) is the CAS-write path. When the caller has the right expectedRevision but no cached etag (first call after restart, or actor restart wiped the in-memory cache), the code does an extra load() to populate the cache, then puts with ifMatch: cachedEtag.

The window:

1. cached === undefined → enter cache-refresh branch.
2. await this.load<S>(pid, options) — talks to backend. Backend at this moment shows {revision: N, etag: 'X'}. Cache is populated.
3. (GAP): between step 2 and step 4, another writer (different process, different actor instance, anywhere) successfully writes the same key. Backend now shows {revision: N+1, etag: 'Y'}.
4. await backend.put(..., ifMatch: 'X') — fails (server returns 412 Precondition Failed).
5. Catch path throws DurableStateConcurrencyError(pid, expectedRevision, -1). Cache still holds {etag: 'X', revision: N}.

After step 5, the caller probably catches + retries. On retry:

• New upsert(pid, expectedRevision=N, state) — but the real backend is at N+1.
• cached !== undefined && cached.revision === expectedRevision → CAS check passes locally.
• effectiveIfMatch = 'X' (still the stale cached etag).
• backend.put(..., ifMatch: 'X') → fails again.

Loop until the cache is invalidated by something else (e.g. another load() call from elsewhere).

Exploit walkthrough (concurrency, not adversarial)

Setup: two processes share a DurableState backend. Each runs the same actor at pid = 'order-42'. This is a misconfiguration (actors should be single-writer per pid), but it's a realistic operator error during a deployment / rollback / split-brain.

Step 1 — process A: load('order-42') → {revision: 5, etag: 'a'}. Cache: {etag: 'a', revision: 5}.

Step 2 — process A: upsert(expectedRevision=5, newState) → succeeds → backend now {revision: 6, etag: 'b'}. A's cache: {etag: 'b', revision: 6}.

Step 3 — process B (cold cache from a restart): upsert(expectedRevision=5, differentState) → enters refresh branch:

• load('order-42') → {revision: 6, etag: 'b'}. B's cache: {etag: 'b', revision: 6}.
• opt.value.revision (6) !== expectedRevision (5) → throws DurableStateConcurrencyError(5, 6). Cache stays {etag: 'b', revision: 6}.

Step 4 — process B retry: upsert(expectedRevision=6, ...differentState):

• cached.revision === expectedRevision (6) → passes local check.
• effectiveIfMatch = 'b'.

Step 5 — process A concurrently: upsert(expectedRevision=6, anotherState) → backend now {revision: 7, etag: 'c'}. A's cache: {etag: 'c', revision: 7}.

Step 6 — process B's put (from step 4): backend.put(..., ifMatch: 'b') → backend has etag 'c' now → 412 → caught → throws DurableStateConcurrencyError(6, -1). B's cache is still {etag: 'b', revision: 6}.

Step 7 — process B retry: same cache, same stale etag → fails again. Loop.

B exits the loop only when it does an explicit load() (refreshing the cache) or restarts. Operator sees confusing error chains.

How the 8 already-landed security fixes inform this

• Idempotency body-fingerprint (4cac92a): bound the response to a fingerprint of the request that produced it. Pattern: bind cached state to the exact version it came from. Apply here: invalidate cache on CAS failure so retry forces re-load.
• Snapshot seq integrity (99de741): made recovery loud-fail on suspicious state. Equivalent here: be loud about cache invalidation rather than silently retrying stale.

Fix design

Two complementary changes.

Track 1 — invalidate cache on CAS failure.

In the catch path:

} catch (e) {
  if (e instanceof ObjectStorageConcurrencyError) {
    // Invalidate the cache — its etag is stale by definition of a CAS
    // failure.  Next call's expectedRevision check + cache-refresh path
    // will fetch fresh state.
    this.etagCache.delete(pid);
    throw new DurableStateConcurrencyError(pid, expectedRevision, -1);
  }
  throw e;
}

After this fix, step 7 in the walkthrough becomes:

• B's cache is empty.
• upsert(expectedRevision=6, ...) → enters refresh branch → load() → gets {revision: 7, etag: 'c'} → opt.value.revision (7) !== expectedRevision (6) → throws with the truthful actualRevision=7.

Caller now has correct information.

Track 2 — also surface actualRevision from the backend when possible.

Currently the CAS catch throws with -1 because the backend doesn't tell us the colliding revision. But after Track 1's cache invalidation, the next call's refresh-load fetches the truth. We can collapse the two steps: on CAS failure, do an immediate load() and surface the real revision in this same error:

} catch (e) {
  if (e instanceof ObjectStorageConcurrencyError) {
    this.etagCache.delete(pid);
    let actualRevision = -1;
    try {
      const fresh = await this.load<S>(pid, options);
      actualRevision = fresh.isSome() ? fresh.value.revision : 0;
    } catch { /* swallow — best-effort enrichment */ }
    throw new DurableStateConcurrencyError(pid, expectedRevision, actualRevision);
  }
  throw e;
}

Caller sees the truthful conflict revision immediately, can decide whether to retry with the right expectedRevision.

Track 3 — counter metric.

durable_state_cas_conflict_total{pid} so operators see actual conflict rates. Helps distinguish "legitimate races" from "misconfigured multi-writer".

API surface

No public-API change. The DurableStateConcurrencyError(actualRevision: -1) semantics stay the same, just more often gets enriched with the real value.

Backward compatibility

Strictly an improvement. Callers that handled actualRevision === -1 as "unknown" continue to work; some calls will now have a real value where before they had -1.

Test plan

1. Exploit-equivalent test (tests/unit/persistence/durable-state-stores/etag-race.test.ts): reproduce the 7-step walkthrough using two ObjectStorageDurableStateStore instances against a shared InMemoryObjectStorage (yes, we'd need to add it, or use the filesystem backend). Pre-fix: process B loops indefinitely. Post-fix: process B's second retry succeeds (with the right expectedRevision) or fails with the truthful actualRevision.

2. Cache-invalidation defence: drive an intentional CAS conflict; verify etagCache.get(pid) returns undefined after the catch.

3. Enriched-error test: CAS conflict → DurableStateConcurrencyError carries the actual revision number (not -1).

4. Metric test: assert durable_state_cas_conflict_total increments.

5. Regression: existing ObjectStorageDurableStateStore tests pass; ETag-CAS contracts unchanged.

Acceptance criteria

• CAS-conflict catch invalidates etagCache.delete(pid).
• CAS-conflict catch attempts a best-effort load to enrich actualRevision.
• durable_state_cas_conflict_total metric exposed.
• Five new tests pass; existing DurableState tests green.
• Plan-doc + README "Known security caveats" updated on land.