Skip to content

fix(storage): repair config Apply, backup add, and add runtime primary swap - #45

Merged
sirdeggen merged 8 commits into
bsv-blockchain:masterfrom
sambabbage:fix/storage-config-and-runtime-swap
May 12, 2026
Merged

fix(storage): repair config Apply, backup add, and add runtime primary swap#45
sirdeggen merged 8 commits into
bsv-blockchain:masterfrom
sambabbage:fix/storage-config-and-runtime-swap

Conversation

@sambabbage

Copy link
Copy Markdown
Contributor

Summary

Three closely-related fixes to the Settings → Storage flow, plus one feature addition. All surface together when actually using the storage panel against a self-hosted (or any non-default) storage server. Tested end-to-end against a fresh server: onboarding remote, swapping primary↔backup, adding https://storage.babbage.systems as a backup.

Bug fixes

1. WalletConfig — Apply now persists across panel close

The toggle/close path always re-applied backupConfig (the pre-edit snapshot), silently reverting the user's just-applied configuration. After a successful Apply we now clear backupConfig so the close path becomes a no-op, and in controlled mode (Settings route) we also onToggle?.() to auto-close — so users don't have to click Back themselves only to have the close clobber their config.

2. WalletService.addBackupStorageUrl — no longer throws "Originator is required for permission checks"

_managers.wallet is the WalletPermissionsManager-wrapped wallet, but the BRC-103 handshake inside StorageClient.makeAvailable() calls wallet.createHmac directly with no originator. The permissions wrapper rejects that, so adding any remote backup at runtime fails:

Error: Originator is required for permission checks.
  at _WalletPermissionsManager.prepareOriginator
  at _WalletPermissionsManager.ensureProtocolPermission
  at _WalletPermissionsManager.createHmac
  at async createNonce
  at async _Peer.initiateHandshake

We now stash the raw wallet as underlyingWallet in _managers (alongside the permissioned wallet) and use it for the StorageClient constructor. Internal-only field; never exposed to apps.

3. Settings — silent failures on Add Backup are now visible

The catch block had a comment claiming the service layer toasted errors, but WalletService.addBackupStorageUrl only toasts on success. Failed adds are now surfaced via toast.error (i18n'd) and console.error. This is what made the underlying-wallet bug above so hard to find.

New feature: Make Primary swap

WalletService.setPrimaryStorage(target) — switches the active (primary) storage to one of the currently-configured backups. Calls WalletStorageManager.setActive, which syncs pending writes to the target then atomically flips the active store. The wallet object is not rebuilt; subsequent reads/writes route through the new active store transparently. The old primary is demoted to a backup. useRemoteStorage / storageUrl / backupStorageUrls are updated together so the swap survives reload.

Wired through WalletContext and useWalletService. New Make Primary button on each backup row in Settings, using the existing sync-progress dialog for live progress logging.

i18n

backup_storage_make_primary_button and backup_error_add_failed added across all 12 supported languages.

Test plan

  • Self-hosted storage server (p2ppsr/storage) running on a Linux box, mainnet, port 8080
  • Onboard with Remote Storage → linuxbox URL: confirmed 500 sat balance loads
  • Settings → Make Primary on Local Electron → swap completes, Active Storage updates, linuxbox journal traffic drops
  • Settings → Make Primary on linuxbox → swaps back, traffic resumes
  • Settings → Add Backup → https://storage.babbage.systems: now succeeds (previously failed silently with the originator error)
  • Apply storage config from Settings → no longer reverts on Back
  • tsc --noEmit clean

…y swap

Bundles closely-related fixes and one feature addition for the storage
configuration flow. All surface together when actually using the Settings
storage panel against a self-hosted storage server.

WalletConfig — Apply now persists across panel close
  Previously the toggle/close path always re-applied `backupConfig` (the
  pre-edit snapshot), silently reverting the user's just-applied config.
  We now clear `backupConfig` on a successful Apply, and in controlled mode
  also signal the parent via `onToggle()` so the panel auto-closes.

WalletService — `addBackupStorageUrl` no longer throws "Originator is required"
  `_managers.wallet` is the `WalletPermissionsManager`-wrapped wallet, but
  the BRC-103 handshake inside `StorageClient.makeAvailable()` calls
  `wallet.createHmac` directly with no originator. The permissions wrapper
  rejects that. We now stash the raw `wallet` as `underlyingWallet` in
  `_managers` (alongside the permissioned `wallet`) and use it for the
  StorageClient constructor. Internal-only; never exposed to apps.

Settings — silent failures on Add Backup are now visible
  The catch block had a comment claiming the service layer toasted errors
  itself, but the service only toasts on success. Failed adds are now
  surfaced via `toast.error` and `console.error`.

Feature: Make Primary button on each backup row
  New `WalletService.setPrimaryStorage(target)` calls
  `WalletStorageManager.setActive` — which syncs pending writes to the
  target then atomically flips the active store — and updates
  `useRemoteStorage` / `storageUrl` / `backupStorageUrls` in the snapshot
  together so the swap survives reload. Wired through `WalletContext` and
  `useWalletService`. New "Make Primary" button on each backup row in
  Settings.

i18n: `backup_storage_make_primary_button` and `backup_error_add_failed`
added across all 12 supported languages.

@sambabbage sambabbage left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One observed issue when syncing after increasing balance with a local storage set:

"Received HTTP 502 from https://store-us-1.bsvb.tech/ without valid BSV authentication (missing headers: x-bsv-auth-version, x-bsv-auth-identity-key, x-bsv-auth-signature) - body preview:"

sambabbage added 2 commits May 8, 2026 11:53
The dialog's footer button reads 'Cancel' while syncComplete is false and
'Close' once it flips true. handleMakePrimary only set syncComplete in the
success path of the try block, so on failure (e.g., a transient 502 from
one of the backup writers during the underlying setActive's updateBackups
phase) the button stayed on 'Cancel' even though the operation had finished.

Move setSyncComplete(true) into finally to mirror handleSyncBackupStorage.
Also log the error to the console for parity with handleAddBackupStorage.
… state

If the snapshot's view of the primary (`useRemoteStorage`/`storageUrl`)
drifts out of sync with `WalletStorageManager.getStores()` — for example
when a previous swap flipped the manager but failed to persist to
localStorage, or a reload happened mid-flight — the prior implementation
would early-return on `targetStore.isActive === true` with an
"Already the primary storage" toast, but never write back the corrected
snapshot. The Settings UI continued to show the stale primary.

Always reconcile the snapshot from the manager's store list after the
operation, including on the no-op path. `useRemoteStorage`,
`storageUrl`, and `backupStorageUrls` are re-derived from `getStores()`,
which is the authoritative source. The toast now distinguishes the
genuine no-op case (target was already active AND visible primary
matched) from the heal case (visible primary changed).
@sambabbage
sambabbage marked this pull request as draft May 8, 2026 19:23
…he provider

`WalletStorageManager` exposes no runtime API to detach a registered
storage provider. The previous implementation acknowledged this in its
toast — "It will be disconnected on next restart" — but left the live
manager attached to the just-removed URL, which meant Sync All Backups
and setPrimaryStorage would still iterate the removed provider. Worse,
`setActive`'s internal updateBackups loop could leave a user-removed
store as a sync target, or fail mid-flight against a flaky removed
backup that the user thought was already gone.

Persist the updated snapshot, then trigger a renderer reload. The
existing auto-init path reads the snapshot and rebuilds the wallet
without the removed provider — no bespoke teardown logic to maintain.
Snapshot save errors are now surfaced via toast.error and rethrown
rather than silently swallowed, so the user sees a real failure rather
than a misleading success.
@sambabbage
sambabbage marked this pull request as ready for review May 8, 2026 19:25
@sirdeggen

Copy link
Copy Markdown
Contributor

Very nice work thank you. Existing issue looks like:

AuthFetch.fetch (sdk auth/clients/AuthFetch.js:222) retries stale sessions only on 401:

  const isStaleSession = error.message.includes('Session not found for nonce') ||
      (error.message.includes('without valid BSV authentication') &&
          peerToUse.identityKey != null &&
          error.details?.status === 401);  // <-- 400 falls through

  Sequence: first sync handshake fresh → 0 inserts both backups. Between syncs, peer nonce went stale server-side (TTL/restart). Second sync re-used cached peers[baseURL] → server rejected nonce → nginx
  swallowed 400 → SDK didn't recognize stale-session pattern → reject.

  StorageClient state: authClient.peers keyed by baseURL. makeAvailable() no-ops after first call (settings cached) — doesn't reset peer. No public reset API.

@sirdeggen sirdeggen self-assigned this May 11, 2026
@sirdeggen

Copy link
Copy Markdown
Contributor

This run:

  • Fresh page load → fresh authClient.peers (already empty, my reset = no-op)
  • During _buildWallet, backupClient.makeAvailable() rpcCall succeeded on eu-1 (got settings back, BRC-103 handshake worked)
  • Moments later, syncBackupStorage → processSyncChunk rpcCall fails 400 on same authClient, same endpoint

So peer cache was never the problem here. Auth handshake works for makeAvailable (tiny body) but fails for processSyncChunk (large body containing sync state). nginx 400 returns 400 Bad Request
generic page — no app code touched.

Likely cause: nginx rejecting processSyncChunk request specifically. Suspects:

  • client_max_body_size exceeded (default 1MB) — though nginx normally returns 413 not 400
  • client_header_buffer_size exceeded — possible, BSV auth + x-bsv-auth-request-id could be large
  • nginx-level path/method block specific to eu-1's config

@sirdeggen

Copy link
Copy Markdown
Contributor

It's probably just body size for our particular services are being limited.

@sirdeggen

Copy link
Copy Markdown
Contributor

All fixed. For the record it was wallet-infra. here was the issue:
BRC-100 wallet sync — ModSecurity body-inspection incident

Symptom

POST / to store-*.bsvb.tech from bsv-desktop's WalletStorageManager.updateBackups → first call succeeded (or empty chunks), subsequent calls failed:

  • HTTP 400 from nginx (nginx generic page) — first failure mode
  • HTTP 502 Bad Gateway — after first WAF tweak

Both surfaced in bsv-desktop as Received HTTP ... without valid BSV authentication (missing headers: x-bsv-auth-version, ...) because the proxy-level error page returned no BRC-103 auth headers. Misleading —
looked like an auth-session problem.

Stack

bsv-desktop (AuthFetch / StorageClient)
→ Cloudflare DNS → AWS NLB → Traefik
→ Traefik modsecurity plugin (HTTP forward)
→ modsecurity sidecar (owasp/modsecurity-crs:4-nginx)
→ wallet-infra Express (port 8080)

ENABLE_NGINX=false on wallet-infra, so the nginx-style 400/502 pages came from the modsecurity sidecar's nginx (CRS image), not the wallet pod.

Root cause

processSyncChunk JSON-RPC bodies pack many fields per chunk (transactions, signatures, output records, basket/tag tables). ModSecurity walks the JSON to enforce SecArgumentsLimit (configured at 10000). With
~100 KB chunks the ARGS count exceeded the limit.

Two failure modes from the same condition:

  1. Rule 200007 "Failed to fully parse request body due to large argument count" — explicit deny when ARGS ≥ limit → returns 400 with nginx generic page.
  2. Rule 200002 "Failed to parse request body" — fires when the JSON parser callback cancels (REQBODY_ERROR=1) after hitting the limit. Disabling 200007 alone left 200002 to fire on the same condition →
    still 400.

Even with both 200002 and 200007 excluded, ModSecurity still walks the JSON internally to enforce the limit. The walk on 100KB+ bodies exceeds the Traefik modsec-plugin client timeout → traefik returns 502
with fail to send HTTP request to modsec: context deadline exceeded. The wallet sync request never reaches wallet-infra; backend logs show only the small leading RPCs (makeAvailable, findOrInsertUser,
findOrInsertSyncStateAuth) and never processSyncChunk.

Misleading SDK error message

SimplifiedFetchTransport.send throws createUnauthenticatedResponseError whenever the response lacks x-bsv-auth-* headers, regardless of the upstream status code. Generic proxy error pages (400/502/etc.)
carry no BSV headers, so the SDK reports them as "missing BSV auth" — easy to misdiagnose as a session/handshake bug.

AuthFetch retries stale sessions only on HTTP 401; status 400/502 with the same symptom doesn't trigger retry.

Fix

infrastructure/controllers/modsecurity.yaml — modsecurity-rules ConfigMap, RESPONSE-999-EXCLUSION-RULES-AFTER-CRS.conf:

SecRule REQUEST_HEADERS:X-Forwarded-Host "@rx ^store-[a-z]+-[0-9]+.bsvb.tech$"
"id:1000004,phase:1,pass,nolog,ctl:requestBodyAccess=Off"

Phase 1, host-scoped via Traefik-set X-Forwarded-Host. Disables request-body access entirely on store-*.bsvb.tech only — modsec doesn't parse the JSON, no ARGS walk, no timeout. Same pattern previously used
for Teranode binary endpoints (rule 1000003).

Annotation configmap-hash: store-body-access-off-2026-05-11 on the modsec Deployment to force pod rollout when the ConfigMap changes (subPath mounts don't hot-reload).

Why it's safe: BRC-100 storage endpoints enforce BRC-103 mutual auth at the app layer — every request is signature-verified against the wallet identity before processing. Modsec body inspection adds no
marginal security here. Global ModSecurity coverage (incl. SecArgumentsLimit) still applies to every other app on the cluster.

@sirdeggen sirdeggen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

underlyingWallet means very little to me,

I propose we call the fully functional wallet wallet and the one protected by permissionsManager guardedWallet to differentiate.

@sirdeggen

Copy link
Copy Markdown
Contributor

putting forward the changes in a commit now

@sirdeggen

Copy link
Copy Markdown
Contributor

I think in my haste to patch the security problem of the previous release, I replaced wallet with permissionsManager more broadly than intended. I suspect that's why the config wasn't persisting, because that's not behavior I'd noticed.

sirdeggen added 2 commits May 11, 2026 21:25
Signed-off-by: Deggen <d.kellenschwiler@bsvassociation.org>
Signed-off-by: Deggen <d.kellenschwiler@bsvassociation.org>
@sirdeggen
sirdeggen dismissed their stale review May 12, 2026 03:59

commit fixed

sirdeggen added 2 commits May 11, 2026 23:08
Signed-off-by: Deggen <d.kellenschwiler@bsvassociation.org>
@sirdeggen
sirdeggen merged commit 49b4b63 into bsv-blockchain:master May 12, 2026
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