Skip to content

refactor: Security audit fixes (certs, webview, RNG) + drop 5 dependencies - #2557

Merged
ethicnology merged 11 commits into
developfrom
audit-security-deps
Aug 5, 2026
Merged

refactor: Security audit fixes (certs, webview, RNG) + drop 5 dependencies#2557
ethicnology merged 11 commits into
developfrom
audit-security-deps

Conversation

@ethicnology

@ethicnology ethicnology commented Aug 4, 2026

Copy link
Copy Markdown
Member

A codebase audit (security + dependencies) found three vulnerabilities and five
removable dependencies:

  1. Electrum probes accepted any TLS certificate. onBadCertificate: (_) => true
    applied to all servers, letting a network attacker MITM the connectivity probe
    and make a malicious server look healthy.
  2. The exchange auth webview matched URLs by prefix. startsWith('https://accounts')
    would also match lookalike hosts such as https://accounts.evil.com.
  3. Log attachment filenames used a non-secure RNG.

decimal, flutter_animate, font_awesome_flutter and sliver_tools were each used
in a single file; http was unused.

What changes

  • Electrum: ServerStatusPort.checkElectrum takes a required allowSelfSigned.
    Default servers now enforce strict CA validation; self-signed certs are only
    tolerated for user-configured custom servers (personal nodes).
  • Exchange auth: exact host comparison against the configured auth URL;
    non-HTTPS navigation is blocked outright.
  • Log attachments: Random.secure().
  • Dependencies: each removal replaces the usage with a stdlib/local equivalent —
    bolt11 amounts via exact rational arithmetic (same truncation semantics, covered by
    a new unit test), a plain Duration, a local GitHub SVG tinted from the theme, and
    a Stack overlay instead of SliverStack + pinned SliverAppBar (behaviourally
    identical — see code comment).

What deliberately does NOT change

  • decimal and http stay in the lockfile as transitive deps (other packages
    need them); only the direct dependency is dropped.
  • Self-signed certs remain accepted for the user's own custom Electrum servers.
  • gap is kept: used in 244 files and already owned and re-exported by bull_ui.
    Migrating the remaining root imports to the bull_ui barrel belongs to the
    design-system migration, not this PR.
  • No behaviour change on the exchange home screen (buttons, app bar actions,
    pull-to-refresh untouched).

Security review notes

Touches TLS validation (Electrum probes) and the exchange auth webview navigation
policy — worth a careful look. No key material, signing, or backup/recovery code
involved.

Commit reading order

1–3: the three security fixes (independent, individually revertable).
4–8: one commit per dropped dependency, each carrying its pubspec.yaml + pubspec.lock
hunk so every intermediate state resolves under --enforce-lockfile.

Test plan

Automated (already in this PR):

  • make analyze — no issues found (matches CI: --fatal-warnings --fatal-infos)
  • make unit-test — all green, including 4 new Swap.amountSat tests
    (test/core_test/swaps/swap_entity_test.dart): exact bolt11 decode
    (2500u → 250 000 sats), empty invoice → 0, unparseable invoice → 0,
    chain swap passthrough

Manual QA:

Electrum (cert validation):

  • Default servers (mainnet + testnet) still show online/connected in
    Settings → Electrum server
  • Adding a custom server with a self-signed cert still succeeds and shows online
  • Toggling between default and custom servers updates the status indicator correctly

Exchange auth webview:

  • Exchange login flow completes end-to-end (auth host navigation still allowed)
  • Terms / privacy links open from the webview
  • Navigating to any unrelated URL inside the webview stays blocked

@claude

This comment was marked as resolved.

@ethicnology ethicnology self-assigned this Aug 4, 2026
@ethicnology
ethicnology force-pushed the audit-security-deps branch from db9edcc to 540a4ea Compare August 4, 2026 14:41
@ethicnology

Copy link
Copy Markdown
Member Author

1. App-bar drag regression (exchange home) — accepted as a trade-off rather
than reverted: the hit area is limited to the bar's four icon buttons, and
losing drag-to-scroll from exactly those buttons is a small price for dropping
sliver_tools. The code comment no longer claims behavioural identity and now
documents this difference explicitly, the stale "sliver app bar" comment in
lib/router.dart is fixed, and I've added a drag-to-scroll item to the manual
QA list in the PR description. (commit bfad60fce)

2. Exact-host match narrowing + silent blocks — both prevent paths now
emit a log.warning with the host only (never the full URL — query params
may carry tokens), so a legitimate navigation blocked in the field, like a
cross-host redirect between numbered .dev auth instances, is diagnosable from
user logs. Keeping the exact-match policy as-is: silently following redirects
across accounts* hosts is precisely the class of thing this fix is meant to
make visible, and testnet login is on the QA list. (commit 0c65abf69)

3. allowSelfSigned naming — renamed to skipCertValidation across the
port, adapter and both call sites, with the doc comment now stating plainly
that it disables ALL certificate checks (chain, expiry, hostname) and that an
active MITM is indistinguishable from the user's node on custom servers.
TOFU-pinning the custom server's certificate (store SHA-256 at add time,
compare on later probes) is the right follow-up to give personal nodes real
MITM resistance — out of scope here, I'll file an issue for it. (commit
f43b55662)

@ethicnology

Copy link
Copy Markdown
Member Author

@wired-pasteque can you give it a manually QA please?

@wired-pasteque

Copy link
Copy Markdown
Collaborator

@ethicnology, manual QA - all good ✅

On the Electrum side, default servers (mainnet + testnet) still show online/connected, adding a custom server with a self-signed cert still succeeds and shows online, and toggling between default and custom updates the status indicator correctly.

Screen.Recording.2026-08-04.at.11.43.51.AM.mov

For the exchange auth webview, the login flow completes end-to-end, terms/privacy links open fine, also navigating inside is working as usual. No regressions on my end.

# Conflicts:
#	lib/features/settings/ui/screens/all_settings_screen.dart
The transaction datasource always opened a CA-validated TLS socket built from the bare host and port, ignoring both the url scheme and the user's validateDomain setting. A personal node reached over tcp://, or serving a self-signed certificate, therefore synced fine through BDK/LWK but failed on every transaction-detail fetch.

ElectrumConnection already carries the resolved url, validateDomain and timeout — its doc even names the electrum-client repository as a consumer — so pass it down instead of a lone url string. The socket now follows the scheme, validates certificates exactly like the sync path, and applies the configured timeout, which was missing altogether and could hang the fallback loop.

Bare host:port urls, how Liquid servers are persisted, keep defaulting to TLS rather than being read as a scheme by Uri.parse.
The status probe decided its own certificate policy — lax for custom servers, strict for defaults — instead of asking the setting the BDK/LWK sync obeys. The two could therefore disagree: a personal node serving a self-signed certificate passed the probe, was saved, and only then failed to sync, with nothing pointing at the certificate.

checkElectrum now takes validateDomain and forwards it to onBadCertificate, so "online" means the wallet can really use the server. ElectrumConnectivityAdapter reads it from the electrum settings of the network being checked, which also replaces a SettingsRepository fetch whose result was being discarded; AddCustomServerUsecase reads it before probing and propagates a load failure rather than probing with a guess.

Consequence worth knowing: while validateDomain is on (the default), adding a self-signed server now fails instead of succeeding and syncing badly. The message is still the generic "unreachable" one — telling the user to turn domain validation off is a separate change.
@ethicnology
ethicnology merged commit 0e4bf66 into develop Aug 5, 2026
2 checks passed
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