[Bug]: device-key-ed25519.json rewritten non-atomically in ClearStoredTokens → torn write silently rotates device identity

[KrasimirKralev]

Summary

DeviceIdentityStore.ClearStoredTokens rewrites the authoritative device-identity file device-key-ed25519.json with a non-atomic File.WriteAllBytes. A crash, power-loss, or forced process-kill mid-write can leave a torn / zero-byte / truncated key file. On the next startup, DeviceIdentity.LoadExisting() treats an unparseable/empty file as invalid and silently regenerates a brand-new keypair — rotating the device identity and discarding the (non-regenerable) Ed25519 private key and any device tokens. The node then has to re-pair/re-enroll with the gateway.

This is the exact failure mode the codebase already guards against for this same file: the canonical writer DeviceIdentity.AtomicWriteKeyFile stages to a sibling temp file and File.Moves it into place specifically "so a process-kill or power-loss mid-write cannot leave a torn/zero-byte key file that the next LoadOrCreate would silently rotate the identity over." ClearStoredTokens is the one writer of this file that bypasses that protection.

Bug class: non-atomic persisted write of authoritative, non-regenerable security material (data-loss / auth).

Affected code

Non-atomic write of the identity file:

• src/OpenClaw.Connection/DeviceIdentityStore.cs:60 — File.WriteAllBytes(keyPath, ms.ToArray()); inside ClearStoredTokens, after a read-modify (strip DeviceToken/DeviceTokenScopes/NodeDeviceToken/NodeDeviceTokenScopes).

The recovery-less read path that turns a torn write into identity loss:

• src/OpenClaw.Shared/DeviceIdentity.cs:179-208 — LoadExisting() catches any exception (and the data == null || PrivateKeyBase64 empty case) and calls GenerateNew(), which overwrites the file with a fresh keypair.

The canonical atomic writer for the very same file (the in-repo correct pattern):

• src/OpenClaw.Shared/DeviceIdentity.cs:523-543 — AtomicWriteKeyFile (temp + ACL-restrict + File.Move(overwrite:true), with temp cleanup).
• src/OpenClaw.Shared/DeviceIdentity.cs:106-155 — TryClearDeviceTokenForRole performs the same logical operation (read → null out a role's token fields → keep the keypair → write back) but routes through AtomicWriteKeyFile. ClearStoredTokens is its drifted, non-atomic twin.

Reachable, authoritative callers (this is not dead code):

• src/OpenClaw.Connection/GatewayConnectionManager.cs:570 and :623 (token clear on disconnect / logout / role switch), plus ConnectionStatusWindow and ConnectionPage.

A second, related non-atomic write of the same file exists at src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs:2750 (File.WriteAllText(identityKeyPath, identityBackup) on the direct-connect rollback-restore path) — worth funneling through the same helper in one pass.

Steps to reproduce (analysis-derived, not captured on a live machine)

1. Pair the Windows node so device-key-ed25519.json holds a keypair + an operator/node token.
2. Trigger a token clear (disconnect / logout / switch gateway → GatewayConnectionManager calls ClearStoredTokens).
3. Interrupt the process while File.WriteAllBytes at DeviceIdentityStore.cs:60 is mid-write (power-loss, OS kill, disk-full at the wrong moment). The file is left truncated/partial.
4. Restart the app → DeviceIdentity.Initialize() → LoadExisting() throws on the truncated JSON → GenerateNew().

Expected: clearing a token must never be able to destroy the keypair; an interrupted clear leaves the previous, intact identity file in place (atomic replace), so the device keeps its identity.

Actual (by code path): an interrupted clear can truncate the file; the next launch silently rotates the entire device identity, discarding the private key and all tokens. The device de-enrolls and must be re-paired/re-approved on the gateway.

Why this is a real defect, not intended behavior

• The same file has a dedicated atomic writer with a comment naming this precise hazard (AtomicWriteKeyFile, DeviceIdentity.cs:514-520).
• A behaviorally-identical sibling that clears tokens while preserving the keypair (TryClearDeviceTokenForRole) already does it atomically. ClearStoredTokens does the same job non-atomically — classic duplicate-implementation drift, not a deliberate design choice.
• The blast radius is non-regenerable security material (Ed25519 private key + device tokens), and the read path silently rotates rather than surfacing an error, so the loss is invisible until pairing breaks.

(The torn-write window itself is timing-dependent and was reasoned from the source, not reproduced on hardware — flagged honestly as analysis, not an observed crash.)

Proposed fix options

• Option A (recommended, smallest, matches in-repo pattern): make ClearStoredTokens write through the same atomic temp-file + File.Move(overwrite:true) + ACL-restrict path as DeviceIdentity.AtomicWriteKeyFile. Best done by reusing one helper rather than a third copy — e.g. expose the atomic writer (or a DeviceIdentity.ClearAllStoredTokens(dataPath) that nulls all four token fields and calls AtomicWriteKeyFile) and have ClearStoredTokens delegate to it. This also de-duplicates the two clear-token implementations.
• Option B: keep ClearStoredTokens where it is but replace the raw File.WriteAllBytes(keyPath, …) with a local atomic temp+rename mirroring AtomicWriteKeyFile (preserve the file's existing ACL/permissions on the rename, per the existing helper's TryRestrictSensitiveFileAcl). Apply the same to the ConnectionPage.xaml.cs:2750 rollback-restore write.
• Option C (defense-in-depth, complementary): in LoadExisting(), before GenerateNew() on a parse failure, distinguish "file present but unreadable/torn" from "file absent" and fail loud / preserve rather than silently rotating, so a corrupt identity is recoverable (e.g. back up the bad file and require explicit re-enroll) instead of silently destroyed.

Option A keeps the change Cold-shaped and consistent with the existing contract; C is a separate hardening that could ship independently.

What was / was not tested

• Verified by source-tracing on main @ 87dc3dc: the non-atomic write site, the recovery-less LoadExisting → GenerateNew path, the atomic sibling that proves intent, and the live callers.
• Not tested: no live Windows reproduction and no failing-then-passing unit test — File.Move atomicity is NTFS-specific and this is a Windows-targeted .NET app, which can't be exercised in a Linux sandbox. Filed as an investigated issue rather than a PR because converting a non-atomic write to temp+rename changes failure semantics on protected/permission-sensitive destinations (and this is auth material), so the maintainer should choose the storage/permission contract and add the Windows-side regression coverage.

---

Filed with AI assistance (automated source-pattern mining + analysis). Findings were source-confirmed against the cited code on main; the reproduction is analysis-derived, not captured on hardware. No fix has been pushed — happy for a maintainer to pick the contract in Option A/B/C.

---

Generated by Claude Code

[clawsweeper]

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed June 29, 2026, 6:32 AM ET / 10:32 UTC.

Summary
Keep open: current main and v0.6.3 still rewrite the per-gateway device identity file non-atomically in ClearStoredTokens, while the shared identity code already documents and uses an atomic writer for this exact torn-write hazard.

Reproducibility: yes. for the source-level path, but not with a live crash harness: a paired identity can enter ClearStoredTokens, an interrupted File.WriteAllBytes can leave invalid JSON, and LoadExisting() then regenerates the keypair.

Next step
A fix is concrete, but it should be maintainer-handled because it changes security-sensitive identity storage and Windows atomic/ACL behavior.

Security
Needs attention: The issue concerns non-atomic writes to an Ed25519 private-key and token file, and source inspection confirms this auth material can be corrupted before startup regenerates identity.

Review details

Best possible solution:

Reuse or expose the shared DeviceIdentity atomic key-file path for all token-clearing and rollback identity rewrites, with focused regression coverage that preserves identity fields, tokens cleared as intended, ACL behavior, and no leftover temp files.

Do we have a high-confidence way to reproduce the issue?

Yes for the source-level path, but not with a live crash harness: a paired identity can enter ClearStoredTokens, an interrupted File.WriteAllBytes can leave invalid JSON, and LoadExisting() then regenerates the keypair.

Is this the best way to solve the issue?

Yes: routing the duplicate token-clear and restore writes through the existing atomic identity writer is the narrowest maintainable fix; fail-loud corrupt-file handling would be useful defense-in-depth but can be separate.

AGENTS.md: found and applied where relevant.

Remaining risk / open question:

• The torn-write failure is source-derived rather than live reproduced on Windows with a crash or power-loss harness.
• A fix touches security-sensitive identity material and should preserve the existing ACL restriction behavior of the atomic key-file writer.

Codex review notes: model internal, reasoning high; reviewed against 87dc3dc79d7c.

Label changes

Label changes:

• add P2: This is a normal-priority but real auth/data-loss bug: the failure is timing-dependent, but the affected path can force re-pairing by losing device identity.
• add impact:data-loss: An interrupted write can corrupt the only stored Ed25519 private key and device tokens for a paired gateway.
• add impact:security: The affected file stores security-sensitive device identity material and credentials.
• add impact:auth-provider: The bug affects device-token credential precedence and gateway pairing/re-enrollment behavior.
• add issue-rating: 🦞 diamond lobster: Current issue advisory state selects this label.
• add clawsweeper:source-repro: Current issue advisory state selects this label.
• add clawsweeper:no-new-fix-pr: Current issue advisory state selects this label.
• add clawsweeper:needs-maintainer-review: Current issue advisory state selects this label.
• add clawsweeper:needs-security-review: Current issue advisory state selects this label.

Label justifications:

• P2: This is a normal-priority but real auth/data-loss bug: the failure is timing-dependent, but the affected path can force re-pairing by losing device identity.
• impact:data-loss: An interrupted write can corrupt the only stored Ed25519 private key and device tokens for a paired gateway.
• impact:security: The affected file stores security-sensitive device identity material and credentials.
• impact:auth-provider: The bug affects device-token credential precedence and gateway pairing/re-enrollment behavior.

Evidence reviewed

Security concerns:

• [medium] Use atomic writes for device identity token clears — src/OpenClaw.Connection/DeviceIdentityStore.cs:60
ClearStoredTokens writes directly over the identity file even though the shared identity code has an ACL-preserving atomic writer for this security-sensitive file.
  Confidence: 0.86

What I checked:

• Current non-atomic writer: DeviceIdentityStore.ClearStoredTokens preserves non-token fields but finishes with File.WriteAllBytes(keyPath, ms.ToArray()) against the authoritative device-key-ed25519.json. (src/OpenClaw.Connection/DeviceIdentityStore.cs:60, 87dc3dc79d7c)
• Identity load silently regenerates on invalid file: LoadExisting() treats null/missing private-key data or any caught exception as invalid and calls GenerateNew(), which creates a fresh keypair. (src/OpenClaw.Shared/DeviceIdentity.cs:186, 87dc3dc79d7c)
• Existing atomic writer documents the same hazard: AtomicWriteKeyFile writes to a sibling temp file, restricts ACLs, then File.Move(..., overwrite: true); its comment says this avoids torn or zero-byte key files that would silently rotate identity. (src/OpenClaw.Shared/DeviceIdentity.cs:523, 87dc3dc79d7c)
• Reachable connection paths: GatewayConnectionManager calls ClearStoredTokens when applying setup codes and shared tokens, so the writer is on active credential replacement paths. (src/OpenClaw.Connection/GatewayConnectionManager.cs:570, 87dc3dc79d7c)
• Rollback restore has a second raw write: The direct-connect rollback path restores the identity backup with File.WriteAllText(identityKeyPath, identityBackup), matching the issue's related non-atomic write concern. (src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs:2750, 87dc3dc79d7c)
• Latest release still has the same writer: The v0.6.3 tag still contains the File.WriteAllBytes implementation in DeviceIdentityStore.ClearStoredTokens, so this is not fixed in the latest release. (src/OpenClaw.Connection/DeviceIdentityStore.cs:60, 85445c78066b)

Likely related people:

• AlexAlves87: Blame shows the current ClearStoredTokens implementation, the atomic key writer, the load/regenerate path, and the main connection callers all come from commit 3e00346. (role: introduced current behavior; confidence: high; commits: 3e003463c9e2; files: src/OpenClaw.Connection/DeviceIdentityStore.cs, src/OpenClaw.Shared/DeviceIdentity.cs, src/OpenClaw.Connection/GatewayConnectionManager.cs)
• Vincent Koc: Recent history modified DeviceIdentity key handling and adjacent pairing/startup state after the initial connection implementation. (role: recent shared identity contributor; confidence: medium; commits: ae61c99bd858, 260fb90c6dc7; files: src/OpenClaw.Shared/DeviceIdentity.cs, src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs)
• Barbara Kudiess: Recent commits touched ConnectionPage.xaml.cs and gateway connection state/error recovery, including the area containing the direct-connect rollback restore path. (role: recent connection UI/state contributor; confidence: medium; commits: 1fb28ee19072, 977648cc9c84, 9ac8cfb24f35; files: src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs, src/OpenClaw.Connection/GatewayConnectionManager.cs)

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.