Skip to content

fix(storage): preserve malformed Keychain authority - #196

Merged
johnny4young merged 7 commits into
mainfrom
fix/preserve-malformed-store-key
Oct 8, 2026
Merged

johnny4young merged 7 commits into
mainfrom
fix/preserve-malformed-store-key

Conversation

@johnny4young

@johnny4young johnny4young commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Summary

A Keychain item that exists but is not a 64-hex database key now fails closed with Failure.malformedKey instead of being deleted and regenerated. On main that path deleted with kSecAttrSynchronizableAny, so a malformed local item also destroyed a valid synchronizable key and pushed the next open into fresh-key recovery.

Review follow-ups

  • d8df4e0 — docs(storage): reattach passphrase store documentation to its type. The new PassphraseKeychainOperations protocol had been inserted between the long KeychainPassphraseStore doc comment and the struct, so Quick Help showed the store's documentation on the protocol. Pure move of declarations/comments; no behaviour change. CI green on this head.

Decisions taken

  • Failing closed leaves the app on its existing ephemeral-store warning until the item is repaired; no silent reset.

Known limitations & follow-ups

  • Follow-up (product feature, not in this PR): an explicit "Reset encrypted store" recovery action in Settings that deletes the malformed item after confirmation, so this state is recoverable without Keychain Access.

Merge order

Independent; merge before the v0.10.0 release PR #193.


Original description (state at 36d9553):

Problem

A successful but malformed preferred database-key read deleted both Keychain scopes, risking reachable encryption authority and unintended fresh-store recovery.

Repair

Fail closed with a content-free malformed-key error. Do not add, delete, regenerate or fall through to another scope when a preferred item exists but is malformed. Accept documented 64 ASCII-hex authority verbatim, including uppercase legacy encodings. Inject Security read/add/delete operations so regressions never need the host Keychain.

Regression coverage added

Malformed local plus valid synchronizable item; non-Data successful read; invalid encoding; unchanged local/synchronizable values; true absence creates once; duplicate-add contender reads the winner; preferred/fallback behavior and legacy bytes. Existing native Keychain round-trip tests remain intact.

Compatibility

No key rotation, schema or blob-framing change. Arbitrary passphrases remain available to explicit store initializers; the Keychain-managed item uses the historically generated ASCII-hex format.

Exact-head validation

Published head: 36d9553
Uploaded/local staged tree: b88b7f6e54017298b44ab36188d1dce2c4dc9845 (verified identical before publication)

  • git diff --check passed locally
  • make format, make lint and make test were attempted locally but Swift/Xcode is absent from this Linux executor
  • Exact-head native CI 37381147952 passed strict formatter/SwiftLint, 1504 package tests in 233 suites, 83.9% production-source coverage above the unchanged80% gate, StoreKit and macOS/iOS builds. The injected malformed-authority and duplicate-create regression suite passed
  • Fake-Keychain regression tests passed natively; models from the separate persistence lanes do not qualify Keychain behavior

Combined source review

All five repair trees plus unmerged MCP PR195 at c90e07d combine without textual conflicts in a disposable local tree. Native combined execution remains unverified. MCP optional binary content may become nil after legitimate deletion/GC; the captured row hash is never replaced by a newer body lookup. No stronger availability promise for deleted content or new public-stack dependency is inferred.

Ready for human review only. No merge/release, live Keychain, clipboard, real user library or backup access.

Human-review readiness

Scoped implementation and regression coverage are complete at unchanged head 36d9553. Fresh verification confirms both required native jobs and GitGuardian successful, the expected non-blocking beta-canary skip, and no pending discussion comments, submitted reviews or unresolved inline review threads. Review readiness does not authorize merge, release or deployment.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn


Generated by Claude Code

@johnny4young
johnny4young marked this pull request as ready for review October 6, 2026 03:47
The injectable Keychain operations protocol was inserted between the
KeychainPassphraseStore doc comment and the struct, so Quick Help showed
the store's documentation on the protocol and none on the store. Move the
protocol and its system implementation above the comment and give the
protocol its own short description.

@johnny4young johnny4young left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: A Keychain item that exists but is not a 64-hex database key now throws Failure.malformedKey instead of being deleted and regenerated. Keychain I/O is made injectable (PassphraseKeychainOperations) so this path can be tested.

Verdict: ready. This also fixes a serious latent data-loss path. On main, the "unreadable item" branch called deleteKey(), which deletes with kSecAttrSynchronizableAny. A malformed local item therefore also wiped a perfectly valid synchronizable key, and the next open generated a fresh key and archived the user's database as unreadable. Failing closed is the right call.

Checked:

  • isStoredKey matches generateKey(), which produces 64 lowercase hex characters, and also accepts uppercase. Both production callers (GRDBClipboardStore.encrypted and StoreContentKey.load) use the default service/account, so no other secret format flows through this store.
  • On failure, StoreBootstrap.liveOpener turns the throw into the existing ephemeral-store warning. Nothing is reset silently.

Fixed and pushed (d8df4e0): 🟡 the new protocol and its system implementation sat between the long KeychainPassphraseStore doc comment and the struct. As a result, Quick Help attached the store's documentation to PassphraseKeychainOperations and showed none on the store. I moved both declarations above the comment and gave the protocol a two-line description. It is a pure move of comments and declarations with no code change. It was not compiled locally because there is no Swift toolchain here; CI will confirm.

Question for the owner (not a blocker): after this change, a malformed item leaves the user on the ephemeral fallback indefinitely, with no in-app way out. Do you want a "Reset encrypted store" recovery action that explicitly deletes the malformed item, so this state is recoverable without Keychain Access?


Generated by Claude Code

claude added 2 commits October 8, 2026 03:58
…sync keys

- Store fake adds in the local or synchronizable slot the query names, so a
  regression that writes the key into the wrong scope fails the suite
- Cover a malformed synchronizable key with no local key: it fails closed,
  reads both scopes and never adds or deletes
- Spell the accepted hex ranges with ASCII literals and correct the doc: keys
  are generated lowercase and uppercase is tolerated
@johnny4young
johnny4young merged commit 379a6d8 into main Oct 8, 2026
4 checks passed
@johnny4young
johnny4young deleted the fix/preserve-malformed-store-key branch October 8, 2026 05:38
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.

2 participants