Skip to content

Commit 5a52405

Browse files
Validate node-store addresses syntactically via parse_address at the store boundary
``MemoryNodeStore`` validated whitespace + non-empty + non- duplicate but never called ``parse_address``. Malformed entries (unbracketed IPv6 like ``::1:9001``, non-numeric port, hostname with no port, credentials shape) leaked ``ValueError`` through the ``ClusterClient._find_leader_impl`` sweep, whose narrow except tuple (``DqliteConnectionError``, ``ProtocolError``, ``OperationalError``, ``OSError``) does NOT include ``ValueError``. Result: a single bad seed aborted the entire sweep with the wrong exception class and no per-node attribution. Apply the same syntactic validation in both ``__init__`` and ``set_nodes``: parse via the existing ``connection._parse_address`` helper, wrap the result with a named-entry diagnostic. Local imports avoid the ``cluster <-> node_store`` import cycle (cluster already imports ``NodeStore`` from node_store). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 726e74f commit 5a52405

3 files changed

Lines changed: 84 additions & 1 deletion

File tree

src/dqliteclient/node_store.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,23 @@ def __init__(
116116
addr = raw.strip()
117117
if not addr:
118118
raise ValueError("NodeStore addresses must be non-empty 'host:port' strings")
119+
# Syntactic validation via the same parser
120+
# ``_query_leader`` will use later. Without this,
121+
# malformed entries (unbracketed IPv6, non-numeric
122+
# port, IDN, credentials shape) leak ``ValueError``
123+
# through the ``find_leader`` sweep's narrow except
124+
# tuple — aborting the entire sweep with the wrong
125+
# exception class and no per-node attribution.
126+
# Local import to avoid cluster <-> node_store
127+
# circular import (cluster imports NodeStore).
128+
from dqliteclient.connection import _parse_address as _parse_addr_validator
129+
130+
try:
131+
_parse_addr_validator(addr)
132+
except ValueError as e:
133+
raise ValueError(
134+
f"NodeStore address {addr!r} is not a valid 'host:port': {e}"
135+
) from e
119136
if addr in seen:
120137
continue
121138
seen.add(addr)
@@ -152,6 +169,16 @@ async def set_nodes(self, nodes: Sequence[NodeInfo]) -> None:
152169
addr = node.address.strip()
153170
if not addr:
154171
raise ValueError("NodeInfo.address must be a non-empty 'host:port' string")
172+
# Same syntactic validation as ``__init__``; see rationale
173+
# there. Local import to avoid the cluster import cycle.
174+
from dqliteclient.connection import _parse_address as _parse_addr_validator
175+
176+
try:
177+
_parse_addr_validator(addr)
178+
except ValueError as e:
179+
raise ValueError(
180+
f"NodeInfo.address {addr!r} is not a valid 'host:port': {e}"
181+
) from e
155182
if addr in seen:
156183
continue
157184
seen.add(addr)
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
"""Pin: ``MemoryNodeStore`` syntactically validates each address
2+
via the same ``parse_address`` helper that ``_query_leader`` will
3+
use later. Without this, malformed entries (unbracketed IPv6,
4+
non-numeric port, IDN, credentials shape) leak ``ValueError``
5+
through the ``find_leader`` sweep's narrow except tuple — aborting
6+
the entire sweep with the wrong exception class.
7+
"""
8+
9+
from __future__ import annotations
10+
11+
import pytest
12+
13+
from dqliteclient.node_store import MemoryNodeStore, NodeInfo
14+
from dqlitewire import NodeRole
15+
16+
17+
@pytest.mark.parametrize(
18+
"bad",
19+
[
20+
"no-port",
21+
"host:abc",
22+
"host:99999",
23+
"::1:9001", # unbracketed IPv6
24+
],
25+
)
26+
def test_init_rejects_malformed_address(bad: str) -> None:
27+
with pytest.raises(ValueError, match="not a valid"):
28+
MemoryNodeStore([bad])
29+
30+
31+
def test_init_accepts_well_formed_addresses() -> None:
32+
store = MemoryNodeStore(["localhost:9001", "[::1]:9002"])
33+
nodes = list(store._nodes)
34+
assert len(nodes) == 2
35+
assert nodes[0].address == "localhost:9001"
36+
assert nodes[1].address == "[::1]:9002"
37+
38+
39+
@pytest.mark.asyncio
40+
async def test_set_nodes_rejects_malformed_address() -> None:
41+
store = MemoryNodeStore()
42+
with pytest.raises(ValueError, match="not a valid"):
43+
await store.set_nodes([NodeInfo(node_id=1, address="bogus:notaport", role=NodeRole.VOTER)])
44+
45+
46+
@pytest.mark.asyncio
47+
async def test_set_nodes_accepts_well_formed_addresses() -> None:
48+
store = MemoryNodeStore()
49+
await store.set_nodes(
50+
[
51+
NodeInfo(node_id=1, address="localhost:9001", role=NodeRole.VOTER),
52+
NodeInfo(node_id=2, address="[::1]:9002", role=NodeRole.STANDBY),
53+
]
54+
)
55+
nodes = await store.get_nodes()
56+
assert len(nodes) == 2

tests/test_pool_acquire_signal_propagation.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ async def test_baseexception_during_cleanup_propagates() -> None:
8484
``BaseException`` from the cancelled-task drain must propagate.
8585
The cleanup may not silently swallow them — only ``CancelledError``
8686
is meant to be drained."""
87-
pool = ConnectionPool("localhost:9001", min_size=0, max_size=1, timeout=5.0) # type: ignore[arg-type]
87+
pool = ConnectionPool(["localhost:9001"], min_size=0, max_size=1, timeout=5.0)
8888
_force_at_capacity(pool, _SentinelBaseException("signal"))
8989

9090
with pytest.raises(_SentinelBaseException, match="signal"):

0 commit comments

Comments
 (0)