Skip to content

Commit 9de2b4f

Browse files
fix(webhooks): reject rewrite_to values that normalize to no host
Review follow-up. The guard rejected a structurally unsafe value but still let three shapes through to an empty host, which then assembled to `https://:9000/hook` -- an authority with a port and no host, exactly the shape @target-uri canonicalization rejects, produced by the hook meant to keep the authority well-formed. Each reached the empty host past a different check: - `"[]"` is non-empty until the brackets come off. - `"."` is non-empty until the trailing root dot comes off. - `".."` survived a single-dot strip as `"."`, because the strip used rstrip('.') and ate every dot rather than the one root dot canonicalize_host removes. Rejection is now stated as 'no empty label', which also covers an interior empty label (`"a..b"`). Refs #991.
1 parent 979bbb4 commit 9de2b4f

2 files changed

Lines changed: 42 additions & 1 deletion

File tree

src/adcp/webhook_transport_hooks.py

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@ def __post_init__(self) -> None:
127127
# Accept an already-bracketed IPv6 literal by unwrapping it first;
128128
# the brackets are re-applied below from the canonical form.
129129
inner = value[1:-1] if value.startswith("[") and value.endswith("]") else value
130+
if not inner:
131+
# `"[]"` survives the non-empty check above and empties here. An
132+
# empty host is the one outcome this guard exists to prevent: it
133+
# assembles to `https://:9000/hook`, an authority with a port and
134+
# no host -- exactly the shape @target-uri canonicalization rejects.
135+
raise ValueError(
136+
f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP "
137+
f"literal; got {value!r}, which has no host"
138+
)
130139

131140
# Structural rejection comes FIRST and is the actual point of this
132141
# guard: `rewrite_to` is interpolated straight into the netloc, so any
@@ -171,7 +180,21 @@ def __post_init__(self) -> None:
171180
# refusing them here would break the case this class exists for.
172181
# Structural safety is already established above; anything further
173182
# is the resolver's business, not ours.
174-
object.__setattr__(self, "rewrite_to", inner.lower().rstrip("."))
183+
# One trailing root dot, matching `canonicalize_host` -- `rstrip`
184+
# would eat every dot, so `"."` and `".."` normalized to the empty
185+
# host rather than being rejected.
186+
ascii_host = inner.lower()
187+
if ascii_host.endswith("."):
188+
ascii_host = ascii_host[:-1]
189+
if not ascii_host or any(label == "" for label in ascii_host.split(".")):
190+
# Catches `"."`, `".."` and `"a..b"`. An empty label is not a
191+
# host, and `".."` in particular survives a single-dot strip as
192+
# `"."` -- non-empty, but still no host.
193+
raise ValueError(
194+
f"DockerLocalhostRewrite(rewrite_to=...) must be a hostname or IP "
195+
f"literal; got {value!r}, which normalizes to an empty label"
196+
)
197+
object.__setattr__(self, "rewrite_to", ascii_host)
175198
return
176199

177200
# Non-ASCII: convert to A-labels so the netloc is wire-legal. Failure

tests/conformance/signing/test_webhook_transport_hooks.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,24 @@ def test_rewrite_to_hostname_and_ipv4_still_accepted() -> None:
124124
)
125125

126126

127+
@pytest.mark.parametrize("bad", ["[]", "[.]", ".", "..", "...", "a..b"])
128+
def test_rewrite_to_rejects_values_that_normalize_to_no_host(bad: str) -> None:
129+
"""An empty host is the outcome this guard exists to prevent.
130+
131+
These reach the empty host by two different routes the earlier checks each
132+
miss: ``"[]"`` is non-empty until the brackets come off, and ``"."`` /
133+
``".."`` are non-empty until the trailing root dot is stripped. Both used
134+
to assemble to ``https://:9000/hook`` — an authority with a port and no
135+
host, which is precisely the shape ``@target-uri`` canonicalization
136+
rejects, produced by the hook meant to keep the authority well-formed.
137+
138+
``"a..b"`` is here because the fix is stated as "no empty label" rather
139+
than "not empty", and an interior empty label is the same defect.
140+
"""
141+
with pytest.raises(ValueError, match="rewrite_to"):
142+
DockerLocalhostRewrite(rewrite_to=bad)
143+
144+
127145
@pytest.mark.parametrize(
128146
"name", ["my_service", "host_gateway", "docker_host.local", "_dns-sd._udp.local"]
129147
)

0 commit comments

Comments
 (0)