From d52a3bfafeb5624a0bd35153ad202ae9a717ab88 Mon Sep 17 00:00:00 2001 From: Padreug Date: Mon, 22 Jun 2026 16:45:29 +0200 Subject: [PATCH 1/3] fix: guard every machine_npub deref against unpaired machines (None) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit machine_npub became nullable in #29/m011 (register-unpaired flow), but several consumers still assumed it's non-None and crashed `normalize_public_key(None)` with `AttributeError: 'NoneType' object has no attribute 'startswith'`. On the demo (which had an unpaired machine) this broke the platform-fee update (500) and spammed the cassette consumer with errors every 2s. The #29 create/pair paths were guarded; these were missed: - views_api `api_update_super_config`: the "republish fee to every active machine" loop → skip unpaired (they get their config at pairing). - cassette_transport `build_state_d_tags_for_machines`: skip unpaired (no state-beacon d-tag yet) — the cassette-consumer loop crash. - crud `get_machine_by_atm_pubkey_hex`: its `except (ValueError, AssertionError)` didn't catch the AttributeError; skip unpaired before normalize — the cassette event-handler crash. - bitspire `assert_nostr_attribution`: reject (SettlementAttributionError) an unpaired machine instead of crashing the payment listener. - views_api cassettes/publish endpoint: 400 (not paired) instead of crashing publish_to_atm. Verified on the dev stack: with an unpaired active machine present, the cassette consumer registers (skipping it) and runs clean — no AttributeError. --- bitspire.py | 8 ++++++++ cassette_transport.py | 6 ++++-- crud.py | 5 +++++ views_api.py | 14 ++++++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/bitspire.py b/bitspire.py index 9e0c70c..1609052 100644 --- a/bitspire.py +++ b/bitspire.py @@ -126,6 +126,14 @@ def assert_nostr_attribution(machine: Machine, extra: dict) -> None: "missing nostr_sender_pubkey on Payment.extra — invoice was not " "issued through the nostr-transport path" ) + if not machine.machine_npub: + # Unpaired machine (machine_npub None — nullable since #29/m011). It has + # no identity to attribute a settlement to; reject cleanly rather than + # let normalize_public_key(None) raise an uncaught AttributeError. + raise SettlementAttributionError( + f"machine {machine.id} is unpaired (no machine_npub); " + "a settlement cannot be attributed to it" + ) from lnbits.utils.nostr import normalize_public_key try: diff --git a/cassette_transport.py b/cassette_transport.py index 9f60d9a..e6517a1 100644 --- a/cassette_transport.py +++ b/cassette_transport.py @@ -141,8 +141,10 @@ def _state_d_tag(atm_pubkey_hex: str) -> str: def build_state_d_tags_for_machines(machines: list[Machine]) -> list[str]: """Bootstrap-consumer subscription filter helper: returns the full - `#d=[...]` list for all known ATMs an operator subscribes to.""" - return [_state_d_tag(_atm_hex_pubkey(m)) for m in machines] + `#d=[...]` list for all known PAIRED ATMs an operator subscribes to. + Unpaired machines (machine_npub is None — nullable since #29/m011) have no + state-beacon d-tag yet, so skip them rather than crash `_atm_hex_pubkey`.""" + return [_state_d_tag(_atm_hex_pubkey(m)) for m in machines if m.machine_npub] # ============================================================================= diff --git a/crud.py b/crud.py index 6c4ef53..6eda100 100644 --- a/crud.py +++ b/crud.py @@ -180,6 +180,11 @@ async def get_machine_by_atm_pubkey_hex(atm_pubkey_hex: str) -> Machine | None: target = atm_pubkey_hex.lower() machines = await list_all_active_machines() for m in machines: + # Unpaired machines (machine_npub is None — nullable since #29/m011) + # have no identity to match and would raise AttributeError in + # normalize_public_key (not caught below); skip them. + if not m.machine_npub: + continue try: if normalize_public_key(m.machine_npub).lower() == target: return m diff --git a/views_api.py b/views_api.py index 1a5bc62..35d35b2 100644 --- a/views_api.py +++ b/views_api.py @@ -1081,6 +1081,12 @@ async def api_update_super_config( ) if super_fractions_changed: for machine in await list_all_active_machines(): + # Unpaired machines (machine_npub is None — nullable since #29/m011) + # have no Nostr identity to publish a fee-config beacon to. Skip + # them; they pick up the current fee config when they pair + # (api_pair_machine publishes on success). + if not machine.machine_npub: + continue await publish_fee_config(machine, config, machine.operator_user_id) return config @@ -1147,6 +1153,14 @@ async def api_publish_machine_cassettes( 500 — anything else from the publish path """ machine = await _machine_owned_by(machine_id, user.id) + if not machine.machine_npub: + # Unpaired machine (machine_npub None — nullable since #29/m011) has no + # ATM identity to publish a cassette config to. Fail fast with a clean + # 400 instead of crashing publish_to_atm's normalize_public_key(None). + raise HTTPException( + HTTPStatus.BAD_REQUEST, + "machine is not paired — pair it before publishing cassette config", + ) existing = await list_cassette_configs_for_machine(machine_id) existing_positions = {row.position for row in existing} From 8dad72a00dc1c92d110299e7e49fa8646380c85e Mon Sep 17 00:00:00 2001 From: Padreug Date: Mon, 22 Jun 2026 16:53:45 +0200 Subject: [PATCH 2/3] fix: complete the unpaired-machine sweep + regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full sweep of every machine_npub deref found one more reachable crash: _record_rejected (tasks.py) logs machine_npub[:12], and the assert_nostr_attribution guard now routes an unpaired machine there, so None[:12] -> TypeError. Fall back to machine.id. Every other deref is safe by the attribution-gate invariant: a settlement only flows past assert_nostr_attribution (now rejecting unpaired) for a paired machine, so the downstream distribution / parse-path / "landed" logs can't see None; the collision-loop display already uses `(m.machine_npub or m.id)`. Adds tests/test_unpaired_machine_guards.py: attribution rejects an unpaired machine with the domain SettlementAttributionError (not AttributeError), and build_state_d_tags skips it. New tests + every guard-affected suite pass. (Two pre-existing test_pair_endpoint failures — #29 drift: fake_pair lacks bunker_relay, and the test DB lacks super_config — are out of scope; filed separately.) --- tasks.py | 5 ++- tests/test_unpaired_machine_guards.py | 57 +++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 tests/test_unpaired_machine_guards.py diff --git a/tasks.py b/tasks.py index eb874de..e3865d2 100644 --- a/tasks.py +++ b/tasks.py @@ -235,7 +235,10 @@ async def _record_rejected(payment: Payment, machine: Machine, exc: Exception) - return logger.error( f"spirekeeper: rejected settlement {rejected.id} " - f"(machine={machine.machine_npub[:12]}..., " + # An unpaired machine (machine_npub None) reaches here now that + # assert_nostr_attribution rejects it — fall back to the id so the + # log line doesn't crash on None[:12]. + f"(machine={(machine.machine_npub or machine.id)[:12]}..., " f"payment_hash={payment.payment_hash[:12]}...): {exc}" ) diff --git a/tests/test_unpaired_machine_guards.py b/tests/test_unpaired_machine_guards.py new file mode 100644 index 0000000..78ae196 --- /dev/null +++ b/tests/test_unpaired_machine_guards.py @@ -0,0 +1,57 @@ +""" +Regression: `machine_npub` is nullable (#29/m011 register-unpaired flow), so +every consumer that derives a Nostr identity from it must handle `None` rather +than crash `normalize_public_key(None)` (AttributeError: 'NoneType' has no +'startswith') or `machine_npub[:12]` (TypeError). See PR #33 — an unpaired +machine on the demo broke the platform-fee update (500) and the cassette +consumer. + +These cover the pure-function guards; the DB-backed loops +(get_machine_by_atm_pubkey_hex, the super-config republish loop) are exercised +on the dev stack with an unpaired active machine. +""" + +from datetime import datetime, timezone + +import pytest + +from ..bitspire import SettlementAttributionError, assert_nostr_attribution +from ..cassette_transport import build_state_d_tags_for_machines +from ..models import Machine + +_PAIRED_HEX = "82341f882b6eabcbd6b1c2da5cd14df14b8e91dd0e6da41a72b78ad8f3a7d3b9" + + +def _machine(npub: str | None) -> Machine: + now = datetime.now(timezone.utc) + return Machine( + id="unpaired1", + operator_user_id="op1", + machine_npub=npub, + wallet_id="w1", + name="unpaired", + location=None, + fiat_code="EUR", + is_active=True, + created_at=now, + updated_at=now, + ) + + +def test_attribution_rejects_unpaired_machine_cleanly(): + """An unpaired machine must raise the domain SettlementAttributionError + (which the listener records as 'rejected'), not an uncaught AttributeError + from normalize_public_key(None).""" + with pytest.raises(SettlementAttributionError): + assert_nostr_attribution( + _machine(None), + {"source": "bitspire", "nostr_sender_pubkey": _PAIRED_HEX}, + ) + + +def test_cassette_d_tags_skip_unpaired_machine(): + """build_state_d_tags_for_machines must skip unpaired machines rather than + crash _atm_hex_pubkey on a None npub — the cassette-consumer loop crash.""" + tags = build_state_d_tags_for_machines([_machine(_PAIRED_HEX), _machine(None)]) + assert len(tags) == 1 # only the paired machine contributes a d-tag + assert all("None" not in t for t in tags) From b55fc8bc1cba852f488f8707cca98674dc533064 Mon Sep 17 00:00:00 2001 From: Padreug Date: Mon, 22 Jun 2026 17:18:24 +0200 Subject: [PATCH 3/3] fix(pairing): default bunker_relay to the spire's public event relay, not localhost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The seed minted via the Pair UI baked an unreachable bunker relay into bunker_url. The UI form has no bunker_relay field, so pair_spire fell back to its default `settings.lnbits_nsec_bunker_url` — which on a deployed instance is the INTERNAL relay lnbits uses to reach the co-located bunker (e.g. ws://127.0.0.1:5000/nostrrelay/demo). The remote ATM can't reach localhost, so connectNewSeed hangs -> BunkerTimeoutError "Signer Unreachable". (Flagged by bitspire on the demo; the localhost-relay /pair gotcha the coord thread called out.) Default bunker_relay to the spire's own public event relay (relays[0]) instead: the bunker lives on the same operator nostrrelay the spire publishes its events to, so that URL is machine-reachable. An explicit `bunker_relay` still overrides for split-relay deploys. An empty override now falls back to the same default rather than raising. Regression test: with no (or empty) bunker_relay, bunker_url embeds relays[0] and contains no 127.0.0.1. NOTE: relays[0] is a pragmatic default; whether the seed should carry multiple relays / be sourced from the operator's nostrclient relay is a follow-up. --- pairing.py | 32 +++++++++++++++----------------- tests/test_pairing.py | 22 +++++++++++++++++----- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/pairing.py b/pairing.py index be7a6a4..c2d923f 100644 --- a/pairing.py +++ b/pairing.py @@ -179,26 +179,20 @@ async def pair_spire( connection lifecycle out of the orchestration so this is unit-testable with a fake client. - `bunker_relay` / `keystore_passphrase` default to the lnbits bunker - settings; injectable for tests. `relays` are the relays the spire will - use for its *own* events (kind-21000/30078) — typically the operator's - nostrrelay; supplied by the API layer. + `relays` are the relays the spire uses for its *own* events + (kind-21000/30078) — typically the operator's public nostrrelay; supplied by + the API layer. `bunker_relay` (the relay baked into `bunker_url`, where the + spire reaches the bunker) defaults to `relays[0]`; `keystore_passphrase` + defaults to the lnbits bunker setting. Both injectable for tests. Raises PairingError on any bunker failure; no state is persisted here (the API layer persists on success). """ - relay = ( - bunker_relay if bunker_relay is not None else settings.lnbits_nsec_bunker_url - ) passphrase = ( keystore_passphrase if keystore_passphrase is not None else settings.lnbits_nsec_bunker_keystore_passphrase ) - if not relay: - raise PairingError( - "LNBITS_NSEC_BUNKER_URL is not set — cannot build a spire bunker connection" - ) if not passphrase: raise PairingError( "LNBITS_NSEC_BUNKER_KEYSTORE_PASSPHRASE is not set — " @@ -206,6 +200,14 @@ async def pair_spire( ) if not relays: raise PairingError("at least one relay is required for the seed URL") + # The relay baked into `bunker_url` is where the *spire* (the remote ATM) + # reaches the bunker, so it must be a machine-reachable public URL — NOT + # `settings.lnbits_nsec_bunker_url`, which is how the co-located lnbits + # reaches the bunker (typically ws://127.0.0.1, unreachable from the ATM — + # the localhost-relay /pair gotcha bitspire flagged). Default to the spire's + # own event relay (the bunker lives on the same operator relay the spire + # publishes to); an explicit `bunker_relay` overrides for split-relay deploys. + relay = bunker_relay if bunker_relay else relays[0] key_name = spire_key_name(machine.id) client_name = f"spire-client-{machine.id}" @@ -250,9 +252,7 @@ async def pair_spire( ) -async def revoke_spire( - machine: Machine, *, admin_client: NsecBunkerAdminClient -) -> int: +async def revoke_spire(machine: Machine, *, admin_client: NsecBunkerAdminClient) -> int: """Revoke a spire's bunker access (the "Revoke spire access" UX, aiolabs/spirekeeper#9/#12). @@ -274,6 +274,4 @@ async def revoke_spire( except NsecBunkerNotConfiguredError as exc: raise PairingError(f"nsecbunkerd is not configured: {exc}") from exc except NsecBunkerError as exc: - raise PairingError( - f"bunker admin RPC failed during revoke: {exc}" - ) from exc + raise PairingError(f"bunker admin RPC failed during revoke: {exc}") from exc diff --git a/tests/test_pairing.py b/tests/test_pairing.py index 080f5f9..e3d2999 100644 --- a/tests/test_pairing.py +++ b/tests/test_pairing.py @@ -218,17 +218,29 @@ def test_malformed_token_raises(): _pair(bunker) -def test_missing_relay_or_passphrase_raises(): - with pytest.raises(PairingError, match="LNBITS_NSEC_BUNKER_URL"): - asyncio.run( +def test_bunker_relay_defaults_to_spire_event_relay(): + """No explicit bunker_relay -> the relay baked into bunker_url is the spire's + own public event relay (relays[0]), NOT lnbits's internal bunker URL. This + is the localhost-relay /pair gotcha: a UI-minted seed (the form has no + bunker_relay field) must embed a machine-reachable relay, not ws://127.0.0.1. + An empty bunker_relay falls back to the same default.""" + from urllib.parse import quote + + for empty in (None, ""): + result = asyncio.run( pair_spire( _machine(), relays=_RELAYS, - admin_client=FakeBunker(), - bunker_relay="", + admin_client=FakeBunker(token_secret="s"), # pragma: allowlist secret + bunker_relay=empty, keystore_passphrase=_PASSPHRASE, ) ) + assert f"relay={quote(_RELAYS[0], safe='')}" in result.bunker_url + assert "127.0.0.1" not in result.bunker_url + + +def test_missing_relay_or_passphrase_raises(): with pytest.raises(PairingError, match="PASSPHRASE"): asyncio.run( pair_spire(