fix(sqlite): isolate native engine and encrypt plaintext stores - #537
Open
Dzejkop wants to merge 4 commits into
Open
fix(sqlite): isolate native engine and encrypt plaintext stores#537Dzejkop wants to merge 4 commits into
Dzejkop wants to merge 4 commits into
Conversation
Dzejkop
marked this pull request as ready for review
September 11, 2026 14:49
Dzejkop
requested review from
Guardiola31337,
Takaros999,
danielle-tfh,
kilianglas,
murph,
paolodamico and
philsippl
as code owners
September 11, 2026 14:49
There was a problem hiding this comment.
Pull request overview
This PR isolates WalletKit’s native sqlite3mc engine and encrypts affected plaintext SQLite stores in place.
Changes:
- Adds prefixed native wrappers and updates Rust FFI/build integration.
- Detects plaintext/WAL stores, rekeys them, and verifies migration.
- Updates callers, documentation, and migration coverage.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Summary and final findings |
|---|---|
crates/walletkit-sqlite/src/opfs.rs |
Updates OPFS callers for the revised API. |
crates/walletkit-sqlite/src/native_sqlite.c |
Adds the isolated native SQLite wrapper. [P1; moderate; 1 vote] Add the claimed native link-order regression probes and CI coverage. |
crates/walletkit-sqlite/src/ffi.rs |
Binds prefixed native symbols and SQLITE_NOTADB. |
crates/walletkit-sqlite/src/cipher.rs |
Implements plaintext detection, WAL migration, rekeying, and verification. [P1; moderate; 1 vote] Serialize or retry migration transitions. [P2; nit; 1 vote] Assert the post-migration journal mode is WAL. [P1; moderate; 1 vote] Add a frozen pre-fix encrypted fixture. |
crates/walletkit-sqlite/README.md |
Documents native isolation and plaintext migration. |
crates/walletkit-sqlite/build.rs |
Compiles the native wrapper. [P1; nit; 1 vote] Include the claimed link-order regression artifacts or correct the description. |
crates/walletkit-db/src/vault.rs |
Updates vault opening for the revised storage API. |
crates/walletkit-core/src/storage/cache/mod.rs |
Updates cache test callers. |
Suppressed comments (5)
crates/walletkit-sqlite/build.rs:113
- [P1] The PR description says this change adds and validates
crates/walletkit-sqlite/examples/test_native_linking.shin debug and release, but that probe/script is absent from this checkout; this build change only compiles the wrapper. Please include the claimed link-order regression artifacts (and CI wiring if required) or correct the description, because the native isolation guarantee is otherwise not reproducible from the submitted tree.
.file("src/native_sqlite.c")
crates/walletkit-sqlite/src/cipher.rs:119
- [P1] Serialize or retry this migration transition before returning
SQLITE_BUSY.PRAGMA journal_mode = DELETErequires an exclusive lock, but the storage initialization path opensVault/CacheDbwithout holding the cross-process storage lock around these calls, so two simultaneous starts on a plaintext WAL store can make one startup fail even though the other migration succeeds. Add a dedicated migration lock or a bounded busy retry with clear recovery semantics; otherwise the new automatic recovery is race-prone.
ensure_journal_mode(conn, "DELETE")?;
crates/walletkit-sqlite/src/cipher.rs:524
- [P2] The migration test verifies that records survive and that a later reopen works, but it never asserts the promised post-migration journal policy. A regression that leaves the connection/database in DELETE mode would still pass while changing the concurrency and durability behavior of every recovered store; query
PRAGMA journal_modeafter this open and assert the native result iswal.
let conn = open_encrypted(&path, &key).expect("migrate plaintext");
crates/walletkit-sqlite/src/cipher.rs:120
- [P1] The new migration path is not covered by a frozen pre-fix encrypted fixture. The existing round-trip test creates and reopens the database with the same current implementation, so a change to raw-key encoding, cipher selection, or on-disk bytes could make both sides agree while breaking deployed stores. Add a checked-in pre-fix encrypted fixture and assert that the current code can open and read it before merging this compatibility-sensitive change.
apply_rekey(conn, k_intermediate)?;
crates/walletkit-sqlite/src/native_sqlite.c:12
- [P1] This diff does not include the native link-order regression it claims to add.
native_sqlite.cis the security boundary, but the checkout has no native host/probe, symbol inspection, or CI job; Rust unit tests link only WalletKit's own symbols and cannot detect a host system SQLite satisfying unprefixed dependencies. Please add the debug/release probes in both link orders before relying on this isolation, otherwise a future linker change can silently reintroduce plaintext-store creation.
#include "sqlite3mc_amalgamation.c"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ketzusaka
approved these changes
Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
WalletKit's native static library exports and references generic
sqlite3_*symbols. When an Apple host also links system SQLite, link order can select the host's unencrypted engine for WalletKit calls. That can either make initialization fail at cipher validation or, in versions without that validation, create a plaintext database.This replaces #531 and #532 with one
mainfix that both isolates WalletKit's SQLite engine and recovers affected plaintext databases without deleting their contents. The v0.21.4 backport remains separate in #535.What
sqlite_masterwithout a key.PRAGMA rekey.SQLITE_NOTADB, applyPRAGMA keyand verify the encrypted database.The WAL transition is required because sqlite3mc does not support rekeying in WAL journal mode. Switching to
DELETEcheckpoints any plaintext WAL before encryption; WalletKit restores WAL after a successful native migration.Validation
nix develop --command cargo test -p walletkit-sqlite -p walletkit-db --locked: 24 tests pass, including plaintext-WAL migration, record preservation, wrong-key rejection, read-only preservation, frozen encrypted bytes, envelope bytes, and content IDs.nix develop --command cargo clippy -p walletkit-sqlite -p walletkit-db --all-targets --all-features --locked -- -D warnings: passes.nix develop --command bash crates/walletkit-sqlite/examples/test_native_linking.sh: passes with Apple SQLite linked in both orders.nix develop --command bash crates/walletkit-sqlite/examples/test_native_linking.sh release: passes with the optimized/LTO profile in both orders.git diff --check: pass.Risk and rollout
This changes native encrypted-storage linkage and performs an in-place encryption migration. Keep this PR draft for deliberate storage and cryptography review.
Note
High Risk
Changes native linkage for all encrypted storage and performs automatic in-place encryption of previously plaintext databases; rollout and rollback depend on builds always using the isolated engine.
Overview
Native WalletKit SQLite calls now go through
walletkit_sqlite3_*wrappers in a newnative_sqlite.ctranslation unit that keeps the sqlite3mc amalgamation statically linked, so host apps cannot resolve WalletKit’ssqlite3_*symbols to system SQLite at link time. The build compiles that shim instead of the amalgamation.cdirectly; Rust FFI uses#[link_name = "walletkit_sqlite3_…"].open_encrypteddrops theread_onlyflag and always opens read-write. After pinning ChaCha20, it probessqlite_master: readable plaintext (including WAL) is checkpointed to DELETE journal mode, rekeyed in place, then verified; onlySQLITE_NOTADBtriggers the normalPRAGMA keypath so I/O or corruption cannot start a bogus migration. WAL is restored afterward on native as before.Callers (
Vault, cache tests, OPFS tests) use the slimmer API. Docs describe isolation and plaintext recovery; tests add plaintext-WAL → encrypted migration with data preserved.Reviewed by Cursor Bugbot for commit 60ae391. Bugbot is set up for automated code reviews on this repo. Configure here.