Skip to content

Fix atomic device-key token clearing - #889

Merged
shanselman merged 3 commits into
openclaw:mainfrom
christineyan4:christineyan4-fix-atomic-device-key-write
Jun 29, 2026
Merged

Fix atomic device-key token clearing#889
shanselman merged 3 commits into
openclaw:mainfrom
christineyan4:christineyan4-fix-atomic-device-key-write

Conversation

@christineyan4

@christineyan4 christineyan4 commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Route DeviceIdentityStore.ClearStoredTokens through a shared atomic token-clear helper instead of rewriting device-key-ed25519.json with File.WriteAllBytes
  • Preserve unknown identity JSON fields while stripping DeviceToken, DeviceTokenScopes, NodeDeviceToken, and NodeDeviceTokenScopes
  • Add an atomic raw device-key writer and use it for the ConnectionPage rollback-restore path
  • Preserve best-effort cleanup semantics for malformed/unexpected identity file cases

Fixes #888

Validation

  • ./build.ps1
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore
  • Targeted OpenClaw.Connection.Tests DeviceIdentityStore tests

Manual smoke

  • Launched the branch with an isolated OPENCLAW_TRAY_DATA_DIR
  • Paired/connected, captured identity snapshot before token clear
  • Disconnected via the tray app, captured identity snapshot after token clear
  • Compared snapshots to confirm DeviceId, PublicKeyBase64, and PrivateKeyBase64 stayed unchanged while token fields were cleared

Real behavior proof

Manual smoke run against this branch using an isolated OPENCLAW_TRAY_DATA_DIR.

image

Proof screenshot shows:

  • deviceIdSame=True
  • publicKeySame=True
  • privateKeySame=True
  • beforeHadDeviceToken=True
  • afterHasDeviceToken=False

This verifies token clearing removed the stored device token while preserving the device identity/keypair.

@christineyan4 christineyan4 mentioned this pull request Jun 29, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 4:31 PM ET / 20:31 UTC.

Summary
The PR routes all-token clearing and direct-connect rollback identity restoration through a shared atomic raw device-key writer and adds non-object JSON regression coverage.

Reproducibility: yes. for the source-level path: current main rewrites the identity file non-atomically, and invalid/torn JSON causes LoadExisting() to generate a new keypair. I did not run a live crash or power-loss harness in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 4 files, +98/-22. The patch is compact but touches persisted identity storage and direct-connect rollback behavior.
  • Real proof artifacts: 1 terminal screenshot. The posted proof directly shows identity fields preserved and the stored device token removed after the smoke run.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #888
Summary: This PR is the candidate fix for the canonical open issue about non-atomic device-key token clearing.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] The PR changes writes to device-key-ed25519.json, which stores Ed25519 identity material and gateway device tokens; an incorrect storage change could affect credentials or force re-pairing.
  • [P1] The posted proof shows identity fields preserved and token fields cleared, while Windows ACL and rename semantics are inherited from the existing helper rather than independently proven in this PR.

Maintainer options:

  1. Accept the existing atomic-writer contract (recommended)
    Because the PR routes new writes through the same temp-file, ACL-restrict, and rename helper already used for device-key writes, maintainers can merge once they are comfortable with that storage contract.
  2. Ask for ACL-focused Windows proof
    If maintainers want more evidence, ask for redacted Windows output showing the identity file ACL remains restricted after token clear and rollback restore.
  3. Pause for broader corrupt-file recovery
    Pause this PR only if maintainers want fail-loud recovery for already-corrupt identity files bundled with the atomic clear fix instead of tracked separately.

Next step before merge

  • No automated repair is needed; maintainers should decide whether the existing atomic-writer and ACL contract plus posted proof are sufficient for this security-sensitive identity-storage change.

Security
Cleared: The diff touches sensitive identity storage but routes writes through the existing ACL-restricting temp-file and rename path; I found no concrete supply-chain or security regression.

Review details

Best possible solution:

Land this narrow atomic-writer fix after maintainer security/storage review, then let the linked canonical issue close when the PR merges.

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

Yes for the source-level path: current main rewrites the identity file non-atomically, and invalid/torn JSON causes LoadExisting() to generate a new keypair. I did not run a live crash or power-loss harness in this read-only review.

Is this the best way to solve the issue?

Yes: the PR uses the existing atomic device-key write pattern and keeps the cleanup path best-effort. The remaining decision is maintainer acceptance of the sensitive storage contract, not a narrower code fix.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority fix for a real but timing-dependent identity/token persistence bug with limited affected surface.
  • merge-risk: 🚨 security-boundary: The patch changes writes to a file containing Ed25519 private-key material and device tokens, so CI alone does not settle storage-boundary behavior.
  • merge-risk: 🚨 auth-provider: The patch changes how paired gateway device tokens are cleared and restored, so an incorrect merge could force re-pairing or break credential behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body/comment include terminal screenshot proof from an isolated OPENCLAW_TRAY_DATA_DIR smoke run showing the identity key material preserved and the device token removed.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body/comment include terminal screenshot proof from an isolated OPENCLAW_TRAY_DATA_DIR smoke run showing the identity key material preserved and the device token removed.
Evidence reviewed

What I checked:

Likely related people:

  • AlexAlves87: Blame and symbol history show the current ClearStoredTokens implementation, identity load/regenerate behavior, atomic key writer, and main connection callers trace primarily to one merged connection implementation commit. (role: introduced current behavior; confidence: high; commits: 3e003463c9e2; files: src/OpenClaw.Connection/DeviceIdentityStore.cs, src/OpenClaw.Shared/DeviceIdentity.cs, src/OpenClaw.Connection/GatewayConnectionManager.cs)
  • vincentkoc: Recent merged 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)
  • bkudiess: Recent merged commits touched ConnectionPage and gateway connection state/error recovery around the direct-connect rollback and connection UI paths. (role: recent connection UI/state contributor; confidence: medium; commits: 1fb28ee19072, 977648cc9c84, f51a8618ed7b; files: src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs, src/OpenClaw.Connection/GatewayConnectionManager.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 29, 2026
Christine Yan and others added 3 commits June 29, 2026 15:41
Christine Yan
Fix non-atomic device-key write in ClearStoredTokens
5e336be 
ClearStoredTokens in DeviceIdentityStore used File.WriteAllBytes to
rewrite device-key-ed25519.json. A crash/power-loss mid-write could leave
a torn file, causing LoadExisting to silently regenerate a new keypair
and rotate the device identity.

Fix: Add DeviceIdentity.TryClearAllDeviceTokens that strips all token
fields using raw JSON filtering and writes atomically via temp-file +
File.Move (same pattern as AtomicWriteKeyFile). ClearStoredTokens now
delegates to it.

Also add AtomicWriteKeyFileRaw for the ConnectionPage rollback-restore
path that previously used File.WriteAllText directly.

Fixes openclaw#888

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Christine Yan
Handle non-object identity JSON during token clear
13c8dca 
Guard TryClearAllDeviceTokens before enumerating JSON object properties so valid-but-non-object JSON remains a best-effort no-op instead of throwing through ClearStoredTokens.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Christine Yan
Preserve best-effort token clear behavior
e6171f2 
Keep DeviceIdentityStore.ClearStoredTokens as a best-effort operation by logging unexpected failures from the shared atomic token-clear helper instead of allowing them to escape connection cleanup flows.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@christineyan4
christineyan4 force-pushed the christineyan4-fix-atomic-device-key-write branch from cc46e12 to e6171f2 Compare June 29, 2026 19:47
@christineyan4

christineyan4 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author
image

Adding screenshot proof from the manual smoke test.

The smoke test used an isolated OPENCLAW_TRAY_DATA_DIR and invoked the production token-clear path. The compare output shows:

  • deviceIdSame=True
  • publicKeySame=True
  • privateKeySame=True
  • beforeHadDeviceToken=True
  • afterHasDeviceToken=False

So the stored device token was cleared while the device identity and Ed25519 keypair were preserved.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 29, 2026
@shanselman
shanselman merged commit 7463a34 into openclaw:main Jun 29, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

2 participants

@christineyan4 @shanselman