Skip to content

Commit 9fec3d4

Browse files
Pin cycle 22's _sanitize_display_text guard at the three find_leader aggregate-error sites
Cycle 22 added ``_sanitize_display_text(node.address)`` at the three error-branch interpolations in ``ClusterClient._find_leader_impl``: the no-leader-known arm, the timeout arm, and the transport-error arm. Without these tests, a refactor that consolidates the three error lines into a helper (and forgets one of the sanitise calls) silently re-introduces the log-injection vector cycle 22 was meant to close. Three pinning tests, each driving a different error branch with a hostile address carrying CR + injected log-line text. Each asserts CR is sanitized (LF is deliberately preserved per the wire-layer ``_sanitize_server_text`` contract — multi-line server diagnostics still render). The hostile text remains in the message so operators can still triage; only the CR control character is escaped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent edcc2c7 commit 9fec3d4

1 file changed

Lines changed: 104 additions & 0 deletions

File tree

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
"""Pin: ``ClusterClient._find_leader_impl``'s three
2+
aggregate-error interpolation sites all run
3+
``_sanitize_display_text(node.address)`` so a hostile
4+
allowlist entry (or a redirected node carrying CR /
5+
U+2028 / U+2029 / control chars) cannot inject log lines
6+
through the user-facing ``ClusterError`` message.
7+
8+
Per the wire-layer ``_sanitize_server_text`` contract,
9+
LF and TAB are deliberately preserved so multi-line
10+
server diagnostics still render. The sanitiser replaces
11+
CR, U+2028, U+2029, and other control / bidi / invisible
12+
characters with ``?``.
13+
14+
Cycle 22 added the sanitisation but no test pins the
15+
contract. A refactor that consolidates the three error
16+
lines into a helper and forgets one of the sanitise calls
17+
would silently re-introduce the log-injection vector.
18+
19+
Three sites (one per error branch):
20+
21+
1. ``no leader known yet`` arm (``_query_leader`` returned None).
22+
2. ``timed out`` arm (``TimeoutError``).
23+
3. transport-error arm (``DqliteConnectionError`` / ``OSError`` /
24+
``OperationalError``).
25+
"""
26+
27+
from __future__ import annotations
28+
29+
from unittest.mock import AsyncMock
30+
31+
import pytest
32+
33+
from dqliteclient.cluster import ClusterClient
34+
from dqliteclient.exceptions import ClusterError, DqliteConnectionError
35+
from dqliteclient.node_store import MemoryNodeStore, NodeInfo
36+
from dqlitewire import NodeRole
37+
38+
39+
def _hostile_store(address: str) -> MemoryNodeStore:
40+
"""Bypass the store's own validation (which strips CR / LF)
41+
by injecting directly. The test exercises the find_leader-side
42+
sanitise guard, not the store-side one."""
43+
store = MemoryNodeStore()
44+
object.__setattr__(
45+
store,
46+
"_nodes",
47+
(NodeInfo(node_id=1, address=address, role=NodeRole.VOTER),),
48+
)
49+
return store
50+
51+
52+
@pytest.mark.asyncio
53+
async def test_aggregate_error_sanitises_no_leader_known_branch() -> None:
54+
hostile = "evil:9001\r\nINJECTED-LOG-LINE"
55+
cc = ClusterClient(_hostile_store(hostile), timeout=0.1)
56+
cc._query_leader = AsyncMock(return_value=None)
57+
58+
with pytest.raises(ClusterError) as exc_info:
59+
await cc.find_leader()
60+
61+
msg = str(exc_info.value)
62+
# CR is sanitized (replaced with ``?``); LF is deliberately
63+
# preserved per the wire-layer contract (multi-line server
64+
# diagnostics).
65+
assert "\r" not in msg, f"CR leaked into aggregate error: {msg!r}"
66+
# The original hostile text is retained (just CR-escaped) so
67+
# operators can still triage.
68+
assert "INJECTED-LOG-LINE" in msg
69+
70+
71+
@pytest.mark.asyncio
72+
async def test_aggregate_error_sanitises_timeout_branch() -> None:
73+
hostile = "evil:9002\r\nINJECTED-FROM-TIMEOUT"
74+
cc = ClusterClient(_hostile_store(hostile), timeout=0.1)
75+
76+
async def _raise_timeout(*a: object, **kw: object) -> None:
77+
raise TimeoutError()
78+
79+
cc._query_leader = AsyncMock(side_effect=_raise_timeout)
80+
81+
with pytest.raises(ClusterError) as exc_info:
82+
await cc.find_leader()
83+
84+
msg = str(exc_info.value)
85+
assert "\r" not in msg
86+
assert "INJECTED-FROM-TIMEOUT" in msg
87+
88+
89+
@pytest.mark.asyncio
90+
async def test_aggregate_error_sanitises_transport_error_branch() -> None:
91+
hostile = "evil:9003\r\nINJECTED-FROM-TRANSPORT"
92+
cc = ClusterClient(_hostile_store(hostile), timeout=0.1)
93+
94+
async def _raise_transport(*a: object, **kw: object) -> None:
95+
raise DqliteConnectionError("connection refused")
96+
97+
cc._query_leader = AsyncMock(side_effect=_raise_transport)
98+
99+
with pytest.raises(ClusterError) as exc_info:
100+
await cc.find_leader()
101+
102+
msg = str(exc_info.value)
103+
assert "\r" not in msg
104+
assert "INJECTED-FROM-TRANSPORT" in msg

0 commit comments

Comments
 (0)