[Fixes] Multiple - #23
Merged
Merged
Conversation
…DropAll Security review finding H5 (high): the DropAll test helper hardcoded DROP TABLE statements for 15 unqualified table names, including tables belonging to a different application (users, psbts, vault_*, _sqlx_migrations). Running the test suite with a mispointed DATABASE_TEST_URL silently and irreversibly destroyed those tables, while never actually cleaning the crate's own schema-qualified bdk_wallet.* tables - which is why tests contaminated each other and required --test-threads=1. - Delete the DropAll trait/impl and the dead _drop_tables helper; the test suite no longer issues a single DROP TABLE. - create_test_stores now creates a uniquely named bdk_sqlx_test_* database per store, so tests are isolated, parallel-safe, and repeatable. Leftover test databases are reaped opportunistically, never while any session is connected, with creation/cleanup serialized against races. - Use Store::new_with_url(None, ..) for the sqlite in-memory store so the pool is correctly limited to a single connection. - Also fixes L8 (redundant url.clone()). - README: document the new test database behavior; drop --test-threads=1. Fixes a broken baseline: 3 of 5 tests failed on a shared database due to leftover wallet data. Full suite now passes in parallel and on repeat runs.
Security review finding H1 (high): anchor_tx declared FKs to block and tx with no ON DELETE clause. On a reorg, BDK's local_chain changeset carries (height, None) and the store deletes the block row; while any anchor_tx row still references it (the normal case for an anchored tx), the database rejected the DELETE and the entire persist transaction aborted - after which the wallet could never persist again. anchor_tx rows were never deleted anywhere, so any reorg over an anchored block triggered this. Reproduced before the fix: postgres error 23503 on anchor_tx_wallet_name_block_hash_fkey, sqlite FOREIGN KEY constraint failed - matching the report's PoC evidence. - sqlite: migration 02 rebuilds anchor_tx with ON DELETE CASCADE on both FKs (sqlite cannot alter FK clauses in place); migration 01 is untouched to preserve sqlx checksums for existing databases. Verified against a database with existing anchor rows: data survives the rebuild and the reorg delete now cascades. - postgres: hardcoded schema gains ON DELETE CASCADE, plus an idempotent DO block that upgrades the constraints of databases created with the old schema. Verified idempotent and cascading on an old-schema database. - migrations/postgres/01 (currently unused by code) kept in sync. - New regression test reorged_out_anchored_block_can_be_deleted covers both backends: persist anchored txs, disconnect the anchored block, assert persistence continues and anchors are dropped with the block.
…load Security review finding H2 (high) + L3: the read path decoded stored rows with error-swallowing patterns (if let Ok(..) = consensus_decode / serde_json::from_value), so corrupted whole_tx or anchor rows were silently skipped: the wallet loaded successfully with less history and a wrong balance, and nothing was ever reported. The integrity policy was also inconsistent - other columns of the same tables already hard-failed the load. Both backends now apply one uniform fail-loud policy: - Undecodable whole_tx bytes return BdkSqlxError::Consensus. - consensus::deserialize replaces consensus_decode, so trailing bytes after a valid transaction are rejected too (L3). - The decoded transaction's computed txid is cross-checked against the stored txid column (new TxidMismatch error). - Unparseable anchor JSON returns BdkSqlxError::SerdeJson. - The anchor payload's block hash is cross-checked against the stored block_hash column (new AnchorBlockHashMismatch error). New regression test corrupt_rows_error_on_load covers all five scenarios on both backends and verifies the store loads cleanly again once the corruption is repaired.
Security review finding H3 (high) + L10: every instrumented function in the sqlite backend used bare #[tracing::instrument], which records all arguments into the span - full descriptors (xpub form today: public keys, derivation structure), whole changesets, wallet names, and pool internals - and emitted them at INFO, exactly the verbosity the shipped example enables (RUST_LOG=bdk_sqlx=debug). The postgres module already used skip_all/skip; the sqlite module now matches it. - #[tracing::instrument(skip_all)] on every instrumented fn in sqlite.rs and on the TestStore persister shims in test.rs. - sqlite info! events downgraded to trace! to match the postgres backend's noise level (L10). - New regression test tracing_output_contains_no_descriptor_material captures tracing output at TRACE while creating, persisting, and loading a wallet, and asserts no descriptor/xkey/pubkey material appears. Before the fix it failed with XPub material in the captured spans. Note bdk_wallet 1.2 strips xprv->xpub at Wallet::create, so what leaked today was watch-only surveillance data, not private keys - but spans record whatever a future changeset carries, so the missing skip was latent-critical.
Security review finding H4 (high): easy_backup ran an unscoped SELECT * FROM keychain - no wallet_name filter - and pretty-printed every wallet's rows, including full descriptor strings, to stdout, where CI logs, journald, and container log drivers capture them. One call exposed all tenants' descriptors (xpub form today; latent-critical for anything secret-bearing stored later). Both copies were also dead code: pub fns inside private modules, never re-exported and never constructed (the compiler flagged KeychainEntry as never constructed), so removal breaks no user. This also makes cargo clippy --all-targets -- -Dwarnings pass again, which was failing on master because of exactly this dead code. A real backup facility, if wanted later, should require a wallet_name scope, write to a caller-provided sink instead of stdout, and document the sensitivity of descriptor data.
Security review finding M1 (medium): the process-global NETWORK OnceLock
existed for 'network validation', yet the read path never compared the
DB-stored network against it - InvalidNetwork only fired on unparseable
strings, and that error path called get_network().unwrap(), which would
panic if the global was unset.
- postgres: after parsing the stored network, it is now checked against
the configured global network and the load fails with InvalidNetwork on
mismatch. The unwrap is gone; when no network is configured the error
falls back to a descriptive placeholder instead of panicking.
- sqlite: an unparseable stored network string returned via
.expect("parse Network") - a panic on corrupt data. It now returns
InvalidNetwork like the postgres backend.
- New regression test mismatched_network_errors_on_load covers both the
wrong-network and unparseable-network cases.
Security review finding M2 (medium): values crossing the DB boundary were converted with unchecked 'as' casts in both backends - value as u64, height as u32, vout as u32, last_seen as u64, last_revealed as u32 on the read path (and the mirror-image casts on the write path). A negative or oversized stored value wrapped silently: value=-1 became ~18.4 quintillion sats, height=-1 became height 4294967295. All casts are replaced with a checked_conv helper that returns the new BdkSqlxError::IntOutOfRange error naming the offending column and value. New regression test out_of_range_values_error_on_load verifies that negative txout values and block heights error on load on both backends instead of wrapping.
…updates
Security review finding M3 (medium): insert_descriptor and insert_network
were bare INSERTs - unlike every other writer in the crate and unlike
upstream BDK stores - so re-persisting a merged changeset that carried the
descriptor or network again aborted with a unique-constraint violation.
update_last_revealed silently updated 0 rows when the keychain row was
missing, which would lose derivation state and lead to address reuse.
- insert_descriptor/insert_network now upsert (ON CONFLICT DO UPDATE) in
both backends.
- update_last_revealed returns QueryError{keychain, RowNotFound} when no
row matched, in both backends.
- New regression test repersisting_full_changeset_is_idempotent covers
both behaviors on both backends.
Security review finding M4 (medium): the postgres backend hand-rolled its
schema with CREATE TABLE IF NOT EXISTS strings in migrate(), wrote a
version row it never read back, and had no way to alter existing
databases - blocking schema fixes like H1's FK change. The sqlite backend
already used sqlx::migrate!(); both backends now share one scheme.
- migrate() now runs sqlx::migrate!("./migrations/postgres").
- The H1 constraint upgrade moves from the hardcoded query list into
versioned migration 02.
- Databases created by earlier releases (no _sqlx_migrations bookkeeping)
are adopted transparently: migration 01 is pure IF NOT EXISTS, and
migration 02 upgrades pre-cascade anchor_tx constraints in place.
Verified against a database built with the old hand-rolled schema and
live rows: both migrations apply, and a reorg block delete cascades.
Security review finding M5 (medium): - Remove sqlx-postgres-tester: it was an unused [dependencies] entry that pulled in the obsolete sqlx-core 0.6.3 (flagged by cargo's future-incompatibility report) into every consumer's tree. - Move bdk_wallet's test-utils feature to [dev-dependencies] so test helpers are no longer compiled into release builds of consumers. - Document the postgres TLS default (sslmode=prefer silently falls back to plaintext) on build_with_url and in a new README security-notes section, along with least-privilege role guidance. - Add #![forbid(unsafe_code)]. - Add a rustsec cargo-audit job to CI.
Security review finding L5 (low): the block table's only uniqueness was (wallet_name, hash), so a reorg that replaced the block at a height with a different hash (local_chain changeset entry (height, Some(new_hash))) inserted a second row at the same height and left the old one behind - subsequent loads picked one of the two rows nondeterministically. - Persisting a block now first deletes any row at the same height with a different hash (its stale anchors cascade away via the H1 FKs), then upserts, in both backends. - Migration 03 (both backends) deduplicates existing rows and adds a UNIQUE index on (wallet_name, height) so the invariant is enforced by the database from now on. - New regression test reorg_replaced_block_leaves_single_row verifies a replaced block leaves exactly one row, the new hash wins the load, and the stale anchors are gone - on both backends.
Security review finding L2 (low): read() opened a transaction for its multi-query snapshot but never committed it, relying on implicit rollback at drop. Commit it explicitly on the success path in both backends.
Security review finding L1 (low): Store::<Sqlite>::new accepts any pool, but a multi-connection pool on :memory: gives every connection its own private database - reads and writes silently diverge and per-connection PRAGMAs don't apply pool-wide. Document the constraint and point callers at new_with_url(None, ..), which configures a single-connection pool. The test suite already switched to that constructor in the H5 commit.
Security review finding L7 (low): fix 'bitoin' typo in the HexToArray error message, and reword MissingPool - it fired when the builder had no pool configured, not when a postgres connection failed to initialize. The sqlite/postgres builder API asymmetry noted in the same finding (no SqliteStoreBuilder) is a feature addition and is left as a follow-up.
Security review finding L9 (low): the sqlx re-export is part of the public API surface, so a sqlx major version bump is a breaking change for this crate. Document that, and steer consumers to import sqlx types through the re-export to stay version-aligned.
Security review finding L6 (low): CI ran five identical postgres jobs, one per test, because tests could not share a database. Now that every test isolates itself in its own database (H5 fix), a single job runs the whole suite in parallel. The manual psql database-creation steps are gone too - tests create their own databases against the service's default postgres database.
A hardening pass over the whole store, each defect pinned by an
always-on regression test (src/test.rs, tests/builder_network.rs):
- tx.last_seen for a tx not yet stored was silently dropped (the UPDATE
affected 0 rows); the write now upserts a stub row (whole_tx is
nullable).
- Reads anchored on the network row, so rows persisted by a changeset
that carried no network were written but never read back; tx, block
and keychain tables are now read unconditionally.
- A changeset mapping the same block hash to several heights silently
collapsed to one block row, losing checkpoints; such changesets are
now rejected with DuplicateBlockHash.
- The postgres write path did not validate changeset.network against
the configured network, letting a foreign network overwrite the row
and wedge all subsequent reads; the write is now rejected with
InvalidNetwork.
- keychain.last_revealed INTEGER DEFAULT 0 made a wallet persisted
before its first address reveal reload with index 0 marked used,
skipping it forever. New rows now store NULL explicitly and
migration 04 drops the default. Existing rows are deliberately
untouched: a stored 0 is ambiguous ('revealed index 0' vs 'never
revealed') and rewriting it could cause address reuse.
- update_last_revealed was a plain UPDATE, letting a stale/replayed
changeset move the derivation index backwards and silently reuse
addresses; the update now never decreases the stored value.
- Store::<Postgres>::read ran at READ COMMITTED (per-statement
snapshots), so a concurrent writer could produce a mixed-generation
changeset; the read transaction now uses REPEATABLE READ.
- initialize_network had a check-then-set race that failed concurrent
same-network builds spuriously with SetNetworkFailure; a lost race
now re-validates instead. The race regression test lives in its own
integration-test binary (tests/builder_network.rs) because the
configured network is process-global.
- Store derived Clone with a DB: Clone bound that sqlx's Postgres/
Sqlite marker types do not satisfy, making the impl unusable; a
manual bound-free impl is provided.
- The sqlite backend had no network validation at all: its constructor
took no network and any stored or incoming network was accepted. It
now takes the network at construction (shared process-global with
the postgres backend) and applies the same read/write guards.
Store::<Sqlite>::new and new_with_url therefore take a network
argument.
- insert_descriptor's conflict update kept the stored last_revealed
unconditionally, so replacing a descriptor under the same
(wallet_name, keychainkind) made the new descriptor inherit the old
derivation index and silently skip those addresses on load. The keep
is now conditional on the descriptor being unchanged.
- A keychainkind value outside 'External'/'Internal' was silently
ignored on load, dropping a keychain; corrupt rows now fail with
InvalidKeychainKind.
- Reorgs left duplicate block rows per height behind and made loads
nondeterministic; migration 03 dedupes and enforces one row per
(wallet_name, height), and the write path removes replaced blocks so
their anchors cascade away.
- Corrupt stored data (undecodable tx bytes, txid/anchor payload
mismatches, negative or overflowing integers at the database
boundary) now fails the load loudly via TxidMismatch,
AnchorBlockHashMismatch and checked_conv instead of being silently
skipped or wrapping.
- Migration 05 drops the dead version table and the redundant
idx_block_height index on both backends.
Also: tokio and tracing-subscriber move to dev-dependencies (library
consumers should not pay for test-only deps), and the README gains a
Resolved defects section and security notes (TLS sslmode, least-
privilege roles, descriptor sensitivity).
…parity Resolves the remaining findings of a full review of the crate. Each fix is pinned by an always-on regression test; verified with fmt, clippy -Dwarnings, and the full suite (58 tests) against postgres and sqlite. - fix(postgres): two writers persisting different block hashes for the same previously unoccupied height raced the block table's two unique indexes. The loser's DELETE could not see the winner's uncommitted row, and its INSERT then violated idx_block_wallet_height -- an index the upsert's (wallet_name, hash) conflict target does not cover -- aborting the loser's whole changeset with a raw 23505. Reproduced before the fix (duplicate key value violates unique constraint "idx_block_wallet_height"). Block writes are now serialized per wallet with a transaction-scoped advisory lock (pg_advisory_xact_lock(hashtext(wallet))): the loser waits for the winner to commit, then sees and replaces its row, last-writer-wins, exactly as if the writes had been issued sequentially. The lock is released by commit/rollback and keyed per wallet, so different wallets never block each other. sqlite needs no equivalent: its single-writer lock already serializes the same interleaving. Regression test concurrent_block_writes_at_same_height_both_land races two writers over ten heights on both backends and was verified to fail without the lock. - fix(both): the tx.last_seen upsert overwrote unconditionally, so a stale or replayed changeset moved the timestamp backwards, contradicting bdk_chain's own Merge (last_seen only ever increases). The conflict update now keeps the maximum on both backends, matching the monotonic guarantee update_last_revealed already enforces for derivation state. Regression test last_seen_never_regresses covers both backends. - fix(sqlite): statement failures propagated raw BdkSqlxError::Sqlx while postgres wrapped them in BdkSqlxError::QueryError with table context, so callers could not match one error kind for 'the write failed at the database'. sqlite now wraps with the same table labels as postgres; the two tests that encoded the asymmetry (unmigrated_store_errors, failed_persist_rolls_back_everything) now assert QueryError on both backends. - feat(sqlite): SqliteStoreBuilder mirroring PgStoreBuilder (new(wallet_name).network(..).migrate(..).pool(..).build() / build_with_url(..); build_with_url(None) builds the single-connection in-memory store), closing the constructor/builder API asymmetry between backends. Store::<Sqlite>::new_with_url now delegates to it, so pool construction lives in exactly one place. - feat(sqlite): Store::<Sqlite>::migrate(), mirroring Store::<Postgres>::migrate; sqlite_migrate_is_idempotent covers it. - test: bare 'cargo test' without DATABASE_TEST_URL no longer fails 40+ tests with .expect panics; postgres-backend tests skip gracefully with a one-time notice while the sqlite backend still runs. CI sets the variable, so full coverage always runs there. - style: #[tracing::instrument] on persist-path helpers is uniformly skip_all on both backends (one rule: no span records arguments); module-internal free functions demoted from unreachable pub to pub(crate); get_test_minisicript_with_change_desc typo renamed; README's Resolved defects section moved to CHANGELOG.md. Deliberately unchanged: the process-global network (OnceLock) is a documented design decision -- one process, one network, validated on every read and write -- and remains as-is.
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
1 participant
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.
Summary
Brings
masterup to date withdevelop: 18 commits remediating every finding of a full security/robustness review of the crate (H1–H5, M1–M5, L1–L10), plus a final hardening pass over both backends. Each fix is pinned by an always-on regression test; the suite (58 tests) passes against both postgres and sqlite, withclippy -Dwarningsandfmtclean.High severity
f4fc697):anchor_txFKs had noON DELETEclause, so deleting a reorged-out block aborted the whole persist transaction and the wallet could never persist again. FKs now cascade (sqlite via table-rebuild migration, postgres via schema change + idempotent upgrade of existing databases).6cde51b): undecodablewhole_txbytes and unparseable anchor JSON were skipped, so a wallet loaded with less history and a wrong balance. Both backends now fail loudly (Consensus,SerdeJson), reject trailing bytes, and cross-check decoded txid / anchor block hash against the stored columns (TxidMismatch,AnchorBlockHashMismatch).8686af9): bare#[tracing::instrument]in the sqlite backend captured full descriptors and changesets at INFO. Nowskip_alleverywhere; noisyinfo!downgraded totrace!. Regression test asserts no descriptor material in TRACE output.easy_backupdumped every tenant's keychain to stdout (195d02f): unscopedSELECT * FROM keychainpretty-printed all wallets' descriptors to stdout (CI logs, journald). Removed; it was dead code.DropAlltest helper (052f978): hardcodedDROP TABLEon 15 unqualified names (including other applications' tables) while never cleaning the crate's own schema-qualified tables. Replaced with per-test uniquely namedbdk_sqlx_test_*databases — tests are isolated, parallel-safe, and repeatable;--test-threads=1is gone.Medium severity
fd05b5e): postgres now checks the stored network against the configured network and fails withInvalidNetworkon mismatch; the panic-on-corrupt-data.expectin sqlite is gone too.5218db7):value=-1wrapped to ~18.4 quintillion sats. Allascasts replaced with checked conversions returningIntOutOfRangenaming the column and value.55305b2): re-persisting a merged changeset aborted on unique-constraint violations; both are now upserts.update_last_revealederrors on a missing keychain row instead of silently updating 0 rows (address-reuse risk).720ee4b): adoptsqlx::migrate!()versioned migrations for postgres, matching sqlite; databases created by earlier releases are adopted transparently.ba5d90f): removed unusedsqlx-postgres-tester(pulled in obsolete sqlx-core 0.6.3), movedbdk_wallet/test-utilsto dev-deps, documented the postgres TLSsslmode=preferplaintext fallback, added#![forbid(unsafe_code)]and acargo-auditCI job.Low severity
57eab29): document that multi-connection pools on sqlite:memory:give every connection its own private database; point callers at the single-connection constructor.31ff671):read()'s snapshot transaction is now explicitly committed instead of relying on implicit rollback at drop.f7571db): a reorg replacing a block at a height left two rows and nondeterministic loads; writes now delete the replaced row (anchors cascade), and migration 03 dedupes + enforces one row per(wallet_name, height).0843f8f): five copy-pasted per-test CI jobs collapsed into one parallel test-suite job.e09a0d7): fixbitointypo; reword misleadingMissingPooldescription.f92edef): document thatpub use sqlxcouples this crate's API stability to sqlx's major version.Final hardening pass (
ad02915)pg_advisory_xact_lock), last-writer-wins, with no cross-wallet blocking. sqlite needs no equivalent (single-writer lock).tx.last_seenupsert now keeps the maximum, matchingbdk_chain's monotonic merge guarantee instead of letting stale changesets move time backwards.BdkSqlxError::QueryErrorwith table context, matching postgres, so callers can match one error kind.SqliteStoreBuildermirroringPgStoreBuilder, andStore::<Sqlite>::migrate()mirroring postgres.cargo testwithoutDATABASE_TEST_URLno longer fails 40+ tests; postgres tests skip gracefully while sqlite still runs. CI keeps full coverage.skip_allinstrumentation,pub(crate)demotions, typo fixes; README's Resolved defects moved toCHANGELOG.md.Deliberately unchanged: the process-global network (
OnceLock) remains a documented design decision — one process, one network, validated on every read and write.Test plan
cargo clippy --all-targets -- -Dwarningscargo fmt --check