fix: reject base58 address with unknown version byte in parse_address
What the developer wrote
Authored by kkdao
fix: reject base58 address with unknown version byte in parse_address
address_to_scriptpubkey returns None (no exception) for a base58 address
with a valid checksum but a version byte matching no network. The final
parse_address block only caught exceptions, so such an address was
returned as valid. Capture the return and require a Script. Per review
on #898.
What changed, and why it matters
This fix closes a hole where a Bitcoin address that looks valid (correct checksum) but belongs to no known network could be accepted by Krux's address parser. Before the patch, the parser only caught thrown errors; because the underlying library quietly returns nothing for an unknown address version, such an address slipped through as valid. The patch now checks that a real script output was produced and rejects the address otherwise. This could matter if a user scanned a malformed or wrong-network address and the device treated it as valid.
Review whether any other callers of address_to_scriptpubkey() in the codebase assume it always returns a Script or raises; add similar None checks if needed. Confirm the new regression test covers mainnet/testnet version mismatches as well as entirely unknown version bytes.
Security signals we found
Input validation bypass fixed
Base58 address version byte not validated before fix
Library silent failure (None return) not handled by caller
Regression test added for malformed-but-checksum-valid address
Evidence from the diff
In src/krux/wallet.py parse_address(), the code calls embit’s address_to_scriptpubkey() for base58 addresses. That function returns None for a base58check address whose version byte does not match any known network p2pkh/p2sh version, rather than raising EmbitError. The original code only caught exceptions, so None was ignored and the address was returned as valid. The patch assigns the return value to sc and adds an isinstance(sc, Script) check, raising ValueError(‘invalid address’) when the result is not a Script. A regression test constructs a valid base58check address with version byte 0xFF and asserts it is rejected.
Changed components
src/krux/wallet.pyparse_address()address_to_scriptpubkey() integrationInspect captured patch +20 / −1
diff --git a/src/krux/wallet.py b/src/krux/wallet.py
index 663ceaa..9f1b18b 100644
--- a/src/krux/wallet.py
+++ b/src/krux/wallet.py
@@ -508,9 +508,13 @@ def parse_address(address_data):
if not isinstance(sc, Script):
try:
- address_to_scriptpubkey(addr)
+ sc = address_to_scriptpubkey(addr)
except EmbitError:
raise ValueError("invalid address")
+ # A base58 address with a valid checksum but an unknown version byte
+ # returns None here instead of raising, so verify a Script came back.
+ if not isinstance(sc, Script):
+ raise ValueError("invalid address")
return addr
diff --git a/tests/test_wallet.py b/tests/test_wallet.py
index 4d1873d..245c8ef 100644
--- a/tests/test_wallet.py
+++ b/tests/test_wallet.py
@@ -1772,6 +1772,21 @@ def test_parse_address_raises_errors(mocker, m5stickv, tdata):
parse_address(case)
+def test_parse_address_rejects_unknown_base58_version(m5stickv):
+ """A base58 address with a valid checksum but an unknown version byte must be
+ rejected. address_to_scriptpubkey returns None (no exception) for such an
+ address, so parse_address must check the returned Script, not only catch
+ errors.
+ """
+ from embit import base58
+ from krux.wallet import parse_address
+
+ # Valid base58check payload; version byte 0xFF matches no network p2pkh/p2sh
+ unknown_version_address = base58.encode_check(b"\xff" + b"\x00" * 20)
+ with pytest.raises(ValueError):
+ parse_address(unknown_version_address)
+
+
def test_parse_address_propagates_keyboardinterrupt(mocker, m5stickv):
"""KeyboardInterrupt must propagate: never swallowed by the bech32-uppercase
fallback, nor relabeled 'invalid address' by the final attempt.
Why this scored 62/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.