Skip to content

Commit cdb0ed4

Browse files
KonstantinMirinbokelley
authored andcommitted
fix(signing): validate the port per RFC 3986 instead of trusting int()
Review follow-up. `_malformed_authority_reason` judged the host but never the port, so `portstr` went straight into `int()` -- far more permissive than the grammar `port = *DIGIT`, in three distinct ways: - `int("-80")` produced the authority `host:-80`, which is not an authority. - `int("8_0")` is 80: Python accepts underscore digit separators. - `int("٨٠")` is also 80: `int()` accepts non-ASCII digits, so `host:٨٠` and `host:80` collapsed to the SAME canonical authority. That is a raw-vs-canonical differential -- a peer that does not fold Arabic-Indic digits derives a different @authority from identical bytes and the signature fails for a reason neither side can see in its own logs. `str.isdigit()` does NOT close the third case -- `"٨٠".isdigit()` is True -- so the gate tests ASCII digits specifically. An empty port is NOT rejected. RFC 3986 §3.2.3 makes it legal (`*DIGIT`), meaning "default", and directs normalizers to drop the port and its colon, so `https://host:/p` normalizes to `host`. `urlsplit` agrees (`port=None`). Rejecting it would refuse a valid URI; it previously raised a bare `ValueError` from `int("")` carrying no code -- the same "passes on someone else's exception" failure `_split_or_reject` exists to prevent, one frame lower. Refs #978.
1 parent d0c626a commit cdb0ed4

2 files changed

Lines changed: 77 additions & 2 deletions

File tree

src/adcp/signing/canonical.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,10 +236,10 @@ def _canon_authority(netloc: str, scheme: str) -> str:
236236
host = netloc[: end + 1]
237237
tail = netloc[end + 1 :]
238238
if tail.startswith(":"):
239-
port = int(tail[1:])
239+
port = _port_or_reject(tail[1:], netloc)
240240
elif ":" in netloc:
241241
host, portstr = netloc.rsplit(":", 1)
242-
port = int(portstr)
242+
port = _port_or_reject(portstr, netloc)
243243
else:
244244
host = netloc
245245
host = _canon_host(host, netloc)
@@ -248,6 +248,39 @@ def _canon_authority(netloc: str, scheme: str) -> str:
248248
return host
249249

250250

251+
_ASCII_DIGITS = frozenset("0123456789")
252+
253+
254+
def _port_or_reject(portstr: str, netloc: str) -> int | None:
255+
"""Parse a port per RFC 3986 §3.2.3 (`port = *DIGIT`), or reject.
256+
257+
The port used to go straight into `int()`, which is far more permissive
258+
than the grammar and produced three distinct problems:
259+
260+
* `int("-80")` gave the authority `host:-80`, which is not an authority.
261+
* `int("8_0")` is 80 -- Python accepts underscore digit separators.
262+
* `int("٨٠")` is also 80 -- `int()` accepts non-ASCII digits, so
263+
`host:٨٠` and `host:80` collapsed to the SAME canonical authority. A
264+
peer that does not fold Arabic-Indic digits derives a different
265+
`@authority` from identical bytes, and the signature fails for a reason
266+
neither side can see in its own logs.
267+
268+
`str.isdigit()` does not close that last one -- `"٨٠".isdigit()` is True --
269+
so the test is ASCII digits specifically.
270+
271+
An EMPTY port is legal and means "default": the grammar is `*DIGIT`, and
272+
§3.2.3 says a normalizer should drop the port and its colon when empty. So
273+
`https://host:/p` normalizes to `host` rather than being rejected.
274+
"""
275+
if not portstr:
276+
return None
277+
if not all(ch in _ASCII_DIGITS for ch in portstr):
278+
raise TargetUriMalformedError(
279+
netloc, f"the port {portstr!r} is not a sequence of ASCII digits"
280+
)
281+
return int(portstr)
282+
283+
251284
def _canon_host(host: str, netloc: str) -> str:
252285
"""Lower-case an ASCII host, or convert a U-label to its A-label form.
253286

tests/conformance/signing/test_canonicalization.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,3 +228,45 @@ def test_authority_that_empties_after_normalization_is_rejected(url: str) -> Non
228228
assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed"
229229
with pytest.raises(ValueError):
230230
canonicalize_authority(url)
231+
232+
233+
@pytest.mark.parametrize(
234+
("url", "expected"),
235+
[
236+
# RFC 3986 §3.2.3: `port = *DIGIT`, so an EMPTY port is legal and means
237+
# "default" -- normalizers SHOULD drop it and its colon. Rejecting it
238+
# would refuse a valid URI.
239+
("https://host:/p", "host"),
240+
("https://[::1]:/p", "[::1]"),
241+
# ...and a real port still survives.
242+
("https://host:8443/p", "host:8443"),
243+
],
244+
ids=["empty-port", "empty-port-ipv6", "real-port"],
245+
)
246+
def test_empty_port_is_normalized_away_not_rejected(url: str, expected: str) -> None:
247+
assert canonicalize_authority(url) == expected
248+
249+
250+
@pytest.mark.parametrize(
251+
"url",
252+
["https://host:-80/p", "https://host:8_0/p", "https://host:٨٠/p", "https://host:8a/p"],
253+
ids=["negative", "underscore-separator", "arabic-indic-digits", "alphanumeric"],
254+
)
255+
def test_non_digit_port_is_rejected_with_the_spec_code(url: str) -> None:
256+
"""The port was never validated -- it went straight into `int()`.
257+
258+
Three distinct failures came out of that. `int("-80")` yielded the
259+
authority ``host:-80``, which is not a valid authority at all. `int("8_0")`
260+
is 80, because Python accepts underscore digit separators. And
261+
`int("٨٠")` is also 80, because `int()` accepts non-ASCII digits -- so
262+
``host:٨٠`` and ``host:80`` collapsed to the SAME canonical authority.
263+
That last one is a raw-vs-canonical differential: a peer that does not
264+
fold Arabic-Indic digits computes a different `@authority` for the same
265+
bytes, and the signature fails for a reason neither side can see.
266+
267+
Note `str.isdigit()` alone does not close this -- `"٨٠".isdigit()` is
268+
True. The gate has to be ASCII digits specifically.
269+
"""
270+
with pytest.raises(ValueError) as excinfo:
271+
canonicalize_authority(url)
272+
assert getattr(excinfo.value, "code", None) == "request_target_uri_malformed"

0 commit comments

Comments
 (0)