Skip to content

[M11] Fix TOCTOU race in device-alias Notecard swap - #48

Open
bsatrom wants to merge 1 commit into
mainfrom
fix/infra/m11-alias-toctou
Open

bsatrom wants to merge 1 commit into
mainfrom
fix/infra/m11-alias-toctou

Conversation

@bsatrom

@bsatrom bsatrom commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

handleDeviceAlias (lambda/shared/device-lookup.ts) read the alias record, decided a Notecard swap had occurred, then issued an unconditional UpdateCommand. Two events for the same serial_number arriving concurrently (a real possibility with Notehub batch delivery + Lambda fan-out) could both read the same device_uid and race — the later write silently clobbering the earlier one, and list_append blindly duplicating uids in previous_device_uids.

Changes (lambda/shared/device-lookup.ts)

  • updateAliasOnSwap now takes the caller's observed previousDeviceUids and:
    • guards the write with ConditionExpression: 'device_uid = :expected_old' so it only applies when device_uid is still what the caller read (closes the TOCTOU window),
    • rebuilds previous_device_uids as a de-duplicated set (Set) that excludes the new current uid, instead of unconditional list_append.
  • handleDeviceAlias wraps the read→swap in a retry loop (max 3 attempts). On ConditionalCheckFailedException it re-reads and retries; if a concurrent writer already set our target device_uid, the equality check returns a clean no-op. The new-device path still relies on createAlias's existing attribute_not_exists condition.

Tests (lambda/shared/device-lookup.test.ts)

  • Updated the existing swap test for the deduped-history shape.
  • New: asserts the ConditionExpression + :expected_old value.
  • New: dedup test — history containing both old and new uids collapses correctly.
  • New: conditional path — first UpdateCommand rejects with ConditionalCheckFailedException, retry succeeds (2 update calls).
  • New: lost race → no-op — after the conditional failure the re-read sees the target uid already applied and returns { isSwap: false }.

npm run build (tsc) and npm test (vitest) both pass — 455 tests, 32 files.

Closes review finding M11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant