DragonX/ObsidianDragon
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 4 Wiki Activity

fix(lite): address adversarial audit findings in session's lite work

Browse Source 
Re-audited this session's lite-wallet changes (originally written at medium
effort) and fixed the genuine issues found:

- walletReady (open path): litelib_initialize_existing returns the bare string
  "OK", which is NOT valid JSON, so the previous `json::accept(value)` check
  marked a *successful* open as not-ready. Key off a non-empty success response
  instead (the bridge already maps "Error:"/null to failure). Drops the now
  unused nlohmann include.
- sync progress: while the detached sync thread is still running, syncDone_ is
  authoritative — don't surface the backend's transient idle syncstatus
  ({"syncing":"false"} -> parser progress=1.0/complete=true) as a misleading
  100%/done. Force complete=false and zero the bogus 1.0 in the progress model.
- per-address balance: also exclude `pending` outputs (notes/utxos from an
  unconfirmed received tx) so per-address figures match confirmed/available.
- secret wiping: the settings page left the page-local request copies
  (input.request.*Request.{passphrase,seedPhrase}) unwiped, and the
  validation-only fallback path wiped nothing. Replace the single-path memzero
  with an RAII scrubber that wipes both the UI char buffers and the request
  string copies on every return path.
- concurrency: document that concurrent bridge->execute() is intentionally
  unguarded — litelib serializes wallet access internally via
  Arc<RwLock<LightWallet>>, so a C++ mutex is unnecessary and would defeat the
  sync/syncstatus concurrency the design relies on. syncLaunched_ -> atomic.

Tests: fake backend now returns the real init shapes (seed object for
create/restore, bare "OK" for open) and a new open-path case guards the
walletReady regression. Removed an unreliable alloc==freed leak assert from the
thread-bearing controller test (kept in the thread-free bridge test). Also fixed
a stray CMake indent and removed ~220MB of untracked build/debug scratch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
DanS
2026-06-05 11:28:37 -05:00
parent 2daea67a1e
commit 043cdc7128
7 changed files with 76 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
CMakeLists.txt
View File

@@ -981,7 +981,7 @@ if(BUILD_TESTING)
src/wallet/lite_bridge_runtime.cpp
src/wallet/lite_client_bridge.cpp
src/wallet/lite_connection_service.cpp
src/wallet/lite_wallet_controller.cpp
src/wallet/lite_wallet_controller.cpp
src/wallet/lite_result_parsers.cpp
src/wallet/lite_sync_service.cpp
src/wallet/lite_wallet_gateway.cpp

24
src/ui/pages/settings_page.cpp
View File

@@ -236,6 +236,24 @@ static void evaluateLiteLifecycleRequestFromPageState(App* app) {
break;
}
// Wipe ALL secret material when leaving this function, on every path (real execution,
// validation-only fallback, or early return): the UI char buffers AND the std::string
// copies inside `input.request.*Request`. The controller wipes its own by-value request
// copy, but these page-local copies are separate; leaving them would defeat the wipe.
struct LiteSecretScrubber {
wallet::LiteWalletLifecycleUiExecutionInput& in;
~LiteSecretScrubber() {
sodium_memzero(s_settingsState.lite_lifecycle_passphrase,
sizeof(s_settingsState.lite_lifecycle_passphrase));
sodium_memzero(s_settingsState.lite_restore_seed,
sizeof(s_settingsState.lite_restore_seed));
wallet::secureWipeLiteSecret(in.request.createRequest.passphrase);
wallet::secureWipeLiteSecret(in.request.openRequest.passphrase);
wallet::secureWipeLiteSecret(in.request.restoreRequest.seedPhrase);
wallet::secureWipeLiteSecret(in.request.restoreRequest.passphrase);
}
} liteSecretScrubber{input};
// When a linked lite backend is present, execute the operation for real through the
// App-owned controller. Otherwise fall back to the validation-only adapter.
if (auto* lite = app->liteWallet()) {
@@ -252,11 +270,7 @@ static void evaluateLiteLifecycleRequestFromPageState(App* app) {
break;
}
// Secrets have been consumed; wipe the UI buffers (and the request copies) so they
// do not linger in memory after the attempt.
sodium_memzero(s_settingsState.lite_lifecycle_passphrase, sizeof(s_settingsState.lite_lifecycle_passphrase));
sodium_memzero(s_settingsState.lite_restore_seed, sizeof(s_settingsState.lite_restore_seed));
// (Secret wiping is handled unconditionally by liteSecretScrubber at function exit.)
s_settingsState.lite_lifecycle_summary = result.bridgeResponseRedacted;
if (result.walletReady) {
s_settingsState.lite_lifecycle_status = "Wallet ready";

17
src/wallet/lite_wallet_controller.cpp
View File

@@ -41,11 +41,13 @@ void applyLiteRefreshModelToWalletState(const LiteWalletAppRefreshModel& model,
if (model.hasAddresses) {
// Per-address balances from unspent notes/utxos (when the notes command succeeded).
// Sum the value of outputs at each address that are neither spent nor unconfirmed-spent.
// Sum the value of confirmed, unspent outputs at each address: skip anything spent,
// unconfirmed-spent, or still pending (notes/utxos from an unconfirmed received tx) so
// the per-address figures match the confirmed balance the wallet treats as available.
std::unordered_map<std::string, std::uint64_t> perAddressZatoshis;
if (model.hasSpendableOutputs) {
for (const auto& output : model.spendableOutputs) {
if (output.spent || output.unconfirmedSpent) continue;
if (output.spent || output.unconfirmedSpent || output.pending) continue;
perAddressZatoshis[output.address] += output.valueZatoshis;
}
}
@@ -155,8 +157,7 @@ void LiteWalletController::onLifecycleResult(const LiteWalletLifecycleResult& re
void LiteWalletController::startSync()
{
if (syncLaunched_) return;
syncLaunched_ = true;
if (syncLaunched_.exchange(true)) return;
syncStarted_ = true;
// The backend `sync` command is a blocking, uninterruptible full chain scan, so run it on
// a detached thread. Capture shared refs (not the controller) so it is safe to outlive us.
@@ -183,8 +184,12 @@ std::optional<LiteWalletAppRefreshModel> LiteWalletController::refreshModel()
model.hasSyncStatus = true;
model.sync.walletHeight = syncResult.syncStatus.syncedBlocks;
model.sync.chainHeight = syncResult.syncStatus.totalBlocks;
model.sync.progress = syncResult.syncStatus.progress;
model.sync.complete = syncResult.syncStatus.complete;
// syncDone_ is authoritative: the detached sync thread is still running, so we are NOT
// complete regardless of what syncstatus reports. The backend briefly returns the idle
// shape ({"syncing":"false"} -> parser progress=1.0, complete=true) before the scan
// starts publishing in-progress status; don't surface that as a misleading 100%/done.
model.sync.complete = false;
model.sync.progress = syncResult.syncStatus.complete ? 0.0 : syncResult.syncStatus.progress;
return model;
}

10
src/wallet/lite_wallet_controller.h
View File

@@ -125,6 +125,14 @@ private:
// The bridge is shared (not just owned) so the detached, uninterruptible sync thread can
// safely outlive the controller: it holds a ref, so the underlying bridge is destroyed
// (and litelib_shutdown called) only once BOTH the controller and a running sync release it.
//
// Concurrent execute() is intentionally NOT guarded by a C++ mutex here. The design relies
// on overlapping calls (the detached sync thread runs the long `sync` while the refresh
// worker polls `syncstatus`, and the UI thread may call `new`). litelib serializes wallet
// access internally: LightClient.wallet is an Arc<RwLock<LightWallet>> (do_new_address takes
// write(), do_balance takes read(), do_sync_internal takes read()/write() and writes
// sync_status separately), so concurrent execute() calls cannot corrupt state. A coarse
// mutex would instead serialize sync against syncstatus polling and defeat the design.
std::shared_ptr<LiteClientBridge> bridge_;
LiteWalletLifecycleService lifecycle_;
LiteWalletGateway gateway_;
@@ -136,7 +144,7 @@ private:
// Detached background sync (backend `sync` is a blocking, uninterruptible full scan).
std::thread syncThread_;
bool syncLaunched_ = false;
std::atomic<bool> syncLaunched_{false}; // startSync() guard (set on the main thread)
std::shared_ptr<std::atomic<bool>> syncDone_ = std::make_shared<std::atomic<bool>>(false);
// Joinable background refresh worker (fast iterations: syncstatus, plus data once synced).

14
src/wallet/lite_wallet_lifecycle_service.cpp
View File

@@ -7,8 +7,6 @@
#include <cctype>
#include <utility>
#include <nlohmann/json.hpp>
namespace dragonx {
namespace wallet {
@@ -384,11 +382,13 @@ LiteWalletLifecycleResult LiteWalletLifecycleService::bridgeResult(
result.ok = true;
result.bridgeAccepted = true;
// The bridge already classifies "Error:"-prefixed responses as failures (ok=false).
// A successful init returns a JSON document (seed info for create, wallet info for
// open/restore); treat a well-formed JSON response as a genuinely ready wallet rather
// than discarding it. (Richer field extraction lands with the sync slice.)
result.walletReady = nlohmann::json::accept(bridgeCall.value);
// The bridge already classifies "Error:"-prefixed responses (and null/empty returns) as
// failures (ok=false), so reaching here means the backend genuinely succeeded. The success
// payloads are NOT uniformly JSON: create/restore return a seed object
// ({"seed":..,"birthday":..}), but open (litelib_initialize_existing) returns the bare
// string "OK". A JSON-validity test would therefore wrongly mark a successful open as
// not-ready, so treat any non-empty success response as a ready wallet.
result.walletReady = !bridgeCall.value.empty();
result.status = successStatus;
return result;
}

12
tests/fake_lite_backend.h
View File

@@ -51,13 +51,19 @@ inline char* liteFakeDup(const char* s)
}
inline bool liteFakeWalletExists(const char*) { return g_liteFakeWalletExists; }
inline char* liteFakeInitNew(bool, const char*) { return liteFakeDup("{\"result\":\"created\"}"); }
// Match the real litelib_* return shapes so tests exercise the production walletReady path:
// create/restore return a seed object ({"seed":..,"birthday":..}); open returns the bare
// string "OK" (NOT JSON) — see litelib_initialize_existing.
inline char* liteFakeInitNew(bool, const char*)
{
return liteFakeDup("{\"seed\":\"fake seed phrase words\",\"birthday\":0}");
}
inline char* liteFakeInitFromPhrase(bool, const char*, const char*,
unsigned long long, unsigned long long, bool)
{
return liteFakeDup("{\"result\":\"restored\"}");
return liteFakeDup("{\"seed\":\"fake seed phrase words\",\"birthday\":0}");
}
inline char* liteFakeInitExisting(bool, const char*) { return liteFakeDup("{\"result\":\"opened\"}"); }
inline char* liteFakeInitExisting(bool, const char*) { return liteFakeDup("OK"); }
inline char* liteFakeExecute(const char* command, const char* args)
{
// new-address generation returns a JSON array with the new address (type from args: zs/R).

22
tests/test_phase4.cpp
View File

@@ -4458,7 +4458,10 @@ void testLiteWalletControllerLifecycle()
const auto liteCaps = makeWalletCapabilities(WalletBuildKind::Lite, /*embeddedDaemon*/ false, /*liteBackendLinked*/ true);
const LiteConnectionSettings conn = defaultLiteConnectionSettings();
// create -> wallet ready, walletOpen() true, persist callback fires once, no leak
// create -> wallet ready, walletOpen() true, persist callback fires once.
// (No alloc==freed leak assert here: a ready wallet launches the detached sync thread +
// refresh worker, which are still in flight at end-of-scope and legitimately hold owned
// strings. The leak/double-free invariant is checked in the thread-free bridge test.)
{
dragonx::test::resetLiteFakeCounters();
int persistCount = 0;
@@ -4474,7 +4477,22 @@ void testLiteWalletControllerLifecycle()
EXPECT_TRUE(result.operation == LiteWalletLifecycleOperation::CreateNew);
EXPECT_TRUE(controller.walletOpen());
EXPECT_EQ(persistCount, 1);
EXPECT_EQ(dragonx::test::g_liteFakeAlloc, dragonx::test::g_liteFakeFreed);
}
// open existing -> ready. Regression guard: the real backend's open path
// (litelib_initialize_existing) returns the bare string "OK", which is NOT valid JSON, so
// walletReady must key off a non-empty success response, not JSON validity.
{
dragonx::test::resetLiteFakeCounters();
dragonx::test::g_liteFakeWalletExists = true;
LiteWalletController controller(liteCaps, conn, LiteClientBridge::fromApi(dragonx::test::makeFakeLiteApi()));
LiteWalletOpenRequest req;
req.passphrase = "hunter2";
const auto result = controller.openWallet(req);
EXPECT_TRUE(result.ok);
EXPECT_TRUE(result.walletReady);
EXPECT_TRUE(result.operation == LiteWalletLifecycleOperation::OpenExisting);
EXPECT_TRUE(controller.walletOpen());
}
// restore-from-seed round-trips to ready