Skip to content

Commit e527f23

Browse files
Apply redirect_policy to every confirmed-leader address in _probe_one
The redirect-policy gate at cluster.py:1130 was inside the inner ``if not _addr_equiv(leader_address, node.address):`` branch, so a node that self-confirmed as leader (the probed address == the returned leader address) bypassed the policy unconditionally. The cached fast-path (cluster.py:867-869, 919) correctly gates every cached-leader consultation; the probe self-confirm arm was the lone asymmetric path. Canonical exposure: the user seeds the full cluster node list (typical) AND configures a narrower policy (regional pin / audit mode) AND the policy-excluded node is the current leader. That intersection IS the documented use case for an allowlist policy, so the silent bypass was a real invariant violation. Lift the _check_redirect call out of the inner conditional so it runs on every leader hit; keep _verify_redirect inside (the self-confirm path doesn't need a re-probe). _check_redirect is idempotent and inexpensive — single callable invocation. Update test_self_leader_bypasses_policy (which pinned the old silent-bypass behavior) to test_self_leader_does_not_bypass_policy asserting the policy now propagates on self-confirm. Add a dedicated pin file covering the four arms (excluded self-confirm raises; allowed self-confirm returns; redirect to excluded still rejected; no-policy self-confirm returns). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent db2a659 commit e527f23

3 files changed

Lines changed: 145 additions & 12 deletions

File tree

‎src/dqliteclient/cluster.py‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1119,15 +1119,27 @@ async def _probe_one(idx: int, node: _StoreNodeInfo) -> _LeaderHit | _ProbeMiss:
11191119
sem_acquired = False
11201120

11211121
if leader_address:
1122+
# Policy applies to EVERY confirmed-leader address,
1123+
# not just the redirect-different-from-probe case.
1124+
# The probed node's own address must pass the same
1125+
# gate as a redirect target — otherwise an
1126+
# allowlist-style policy is silently bypassed when
1127+
# the policy-excluded node happens to be the
1128+
# current leader (the canonical regional-pin use
1129+
# case). The cached fast-path (cluster.py:867-869,
1130+
# 919) already gates on every cached-leader
1131+
# consultation; this restores the probe-path
1132+
# symmetry. ``_check_redirect`` is idempotent and
1133+
# inexpensive (a single callable invocation).
1134+
# Re-raises ClusterPolicyError on rejection; the
1135+
# gather loop catches it and propagates.
1136+
self._check_redirect(leader_address, policy=policy)
11221137
# Only leader_address values that did NOT come from
11231138
# node.address itself need authorizing — those are
11241139
# real redirects. Compare via the canonical
11251140
# (host, port) tuple so an IPv6 bracketing
11261141
# difference does not look like a redirect.
11271142
if not _addr_equiv(leader_address, node.address):
1128-
# Re-raises ClusterPolicyError on rejection;
1129-
# the gather loop catches it and propagates.
1130-
self._check_redirect(leader_address, policy=policy)
11311143
# Re-probe the redirect target to confirm it
11321144
# self-identifies as leader. Stale-hint
11331145
# peers can hand back a node that no longer
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
"""Pin: ``_probe_one`` (inside ``find_leader``) applies the configured
2+
``redirect_policy`` to EVERY confirmed-leader address, including the
3+
self-confirm case where the probed node returns its OWN address as
4+
the leader.
5+
6+
Before the fix, ``_check_redirect`` was gated behind ``if not
7+
_addr_equiv(leader_address, node.address)``, so the policy was
8+
silently bypassed when the cluster's current leader happened to be
9+
the probed node. The cached fast-path correctly consults the policy
10+
(cluster.py:867-869, 919) — confirming the maintainer intent that
11+
redirect policy applies to ALL leader-resolution paths. The probe
12+
self-confirm arm was the lone asymmetric path.
13+
14+
Canonical exposure window: the user seeds the full cluster node list
15+
(typical) AND configures a narrower policy (regional pin / audit
16+
mode) AND the policy-excluded node is the current leader. That
17+
intersection IS the documented use case for an allowlist policy
18+
("regional pin"), so the bypass is a real silent invariant
19+
violation.
20+
"""
21+
22+
from __future__ import annotations
23+
24+
import asyncio
25+
from unittest.mock import AsyncMock, patch
26+
27+
import pytest
28+
29+
from dqliteclient.cluster import ClusterClient, allowlist_policy
30+
from dqliteclient.exceptions import ClusterError, ClusterPolicyError
31+
from dqliteclient.node_store import MemoryNodeStore
32+
33+
34+
def test_self_confirm_with_excluded_address_does_not_silently_return() -> None:
35+
"""A node that self-confirms as leader but is excluded by the
36+
allowlist policy must NOT be returned silently. This is the
37+
canonical regional-pin use case.
38+
39+
Setup: seed contains the excluded node only. Probing it returns
40+
its own address (self-confirm). Currently the policy gate is
41+
skipped on self-confirm and the excluded address is returned.
42+
After the fix, the policy gate runs and the call propagates
43+
ClusterPolicyError or fails the gather loop."""
44+
store = MemoryNodeStore(["10.0.0.99:9001"])
45+
cc = ClusterClient(
46+
store,
47+
timeout=5.0,
48+
# Policy excludes 10.0.0.99 — regional-pin shape (allowlist
49+
# mentions only 10.0.0.1, not the seed node).
50+
redirect_policy=allowlist_policy(["10.0.0.1:9001"]),
51+
)
52+
53+
async def probe(address: str, **_: object) -> str:
54+
return "10.0.0.99:9001"
55+
56+
with (
57+
patch.object(cc, "_query_leader", side_effect=probe),
58+
pytest.raises((ClusterPolicyError, ClusterError)),
59+
):
60+
asyncio.run(cc.find_leader())
61+
62+
63+
def test_self_confirm_with_allowed_address_returns_leader() -> None:
64+
"""Negative pin: a node that self-confirms AND is allowed by
65+
policy is returned unchanged. Confirms the fix doesn't over-
66+
reject."""
67+
store = MemoryNodeStore(["10.0.0.1:9001"])
68+
cc = ClusterClient(
69+
store,
70+
timeout=5.0,
71+
redirect_policy=allowlist_policy(["10.0.0.1:9001"]),
72+
)
73+
74+
with patch.object(cc, "_query_leader", new=AsyncMock(return_value="10.0.0.1:9001")):
75+
result = asyncio.run(cc.find_leader())
76+
assert result == "10.0.0.1:9001"
77+
78+
79+
def test_redirect_to_excluded_still_rejected_regression() -> None:
80+
"""Regression: the existing redirect-different-from-probe arm
81+
still rejects. The fix only HOISTS the policy check out of the
82+
inner ``if not _addr_equiv`` — it does not remove the original
83+
check site."""
84+
store = MemoryNodeStore(["10.0.0.1:9001"])
85+
cc = ClusterClient(
86+
store,
87+
timeout=5.0,
88+
redirect_policy=allowlist_policy(["10.0.0.1:9001"]),
89+
)
90+
91+
# Probe 10.0.0.1 → "attacker.com is leader" (redirect to
92+
# excluded address).
93+
async def probe(address: str, **_: object) -> str:
94+
return "attacker.com:9001"
95+
96+
with (
97+
patch.object(cc, "_query_leader", side_effect=probe),
98+
pytest.raises((ClusterPolicyError, ClusterError)),
99+
):
100+
asyncio.run(cc.find_leader())
101+
102+
103+
def test_no_policy_self_confirm_still_returns_leader() -> None:
104+
"""Regression: no policy configured → self-confirm path returns
105+
leader address (no over-reject when there's no policy)."""
106+
store = MemoryNodeStore(["10.0.0.1:9001"])
107+
cc = ClusterClient(store, timeout=5.0) # no redirect_policy
108+
with patch.object(cc, "_query_leader", new=AsyncMock(return_value="10.0.0.1:9001")):
109+
result = asyncio.run(cc.find_leader())
110+
assert result == "10.0.0.1:9001"

‎tests/test_redirect_policy.py‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -125,21 +125,32 @@ async def probe(address: str, **kw: object) -> str | None:
125125
f"probe attempts (expected 1)."
126126
)
127127

128-
def test_self_leader_bypasses_policy(self) -> None:
129-
"""If the queried node is the leader (returns its own address),
130-
the redirect policy doesn't apply — the address is already in the
131-
seed list by definition."""
128+
def test_self_leader_does_not_bypass_policy(self) -> None:
129+
"""Policy applies to EVERY confirmed-leader address, including
130+
the self-confirm case where the probed node returns its own
131+
address. The seed list and the policy are independent — an
132+
allowlist policy can legitimately exclude addresses present in
133+
the seed list (the canonical regional-pin use case). Pre-
134+
existing behavior silently bypassed the policy on self-confirm;
135+
the fix lifts the policy check out of the inner
136+
``if not _addr_equiv`` so it runs on every leader hit. See
137+
``test_probe_one_redirect_policy_on_self_confirm.py`` for the
138+
full pin set covering both arms."""
139+
from dqliteclient.exceptions import ClusterError, ClusterPolicyError
140+
132141
store = MemoryNodeStore(["10.0.0.1:9001"])
133-
# Policy that rejects everything — but the node returning its own
134-
# address isn't a real redirect, so it's accepted.
142+
# Policy rejects everything — the self-confirm path must
143+
# propagate the rejection, not silently return the address.
135144
cc = ClusterClient(
136145
store,
137146
timeout=5.0,
138147
redirect_policy=lambda _a: False,
139148
)
140-
with patch.object(cc, "_query_leader", new=AsyncMock(return_value="10.0.0.1:9001")):
141-
result = asyncio.run(cc.find_leader())
142-
assert result == "10.0.0.1:9001"
149+
with (
150+
patch.object(cc, "_query_leader", new=AsyncMock(return_value="10.0.0.1:9001")),
151+
pytest.raises((ClusterPolicyError, ClusterError)),
152+
):
153+
asyncio.run(cc.find_leader())
143154

144155
def test_redirect_rejection_emits_debug_log(self, caplog) -> None:
145156
"""Policy rejection must emit a DEBUG log so an SSRF-style

0 commit comments

Comments
 (0)