Repository navigation
Conversation
…ent#393) `_is_internal_address` gated its RFC 6598 clause on `ip.version == 4`. The IPv4-MAPPED form `::ffff:100.64.0.1` reports version 6, so the clause never reached it — and CGNAT is precisely the range Python's `ipaddress` answers False to for all six of its own predicates, which is why ent#14 S3 added the clause in the first place. RFC 6598 space was therefore accepted as a PUBLIC destination by both validators built on `_validate_public_https_url`: `validate_a2a_endpoint_url`, which then sends a credentialed Bearer POST (#736), and `validate_template_registry_url`. Every OTHER internal range survives its mapped form for free, because CPython short-circuits `is_private`/`is_reserved` through `ipv4_mapped` before consulting its IPv6 tables. CGNAT is the one range whose refusal is Trinity's own code, so it is the one that had to do that delegation itself — the clause now resolves the v4 view once and tests membership against it. The fix is COMPLETE, not one shape in a queue: `::ffff:0:0/96` is the only v4-in-v6 embedding CPython can inherit an IPv4 verdict through. `::/8` (IPv4-compatible RFC 4291, IPv4-translated RFC 2765), `64:ff9b::/96` + `64:ff9b:1::/48` (NAT64), `2002::/16` (6to4) and `2001::/32` (Teredo) are refused wholesale by prefix regardless of the address they carry. That is now pinned, because the refusal is the interpreter's and not ours (the #1891 class) — an interpreter change could reopen it with the suite still green. A narrowing security fix needs its widening guard too: `test_C1b` asserts the mapped rows either side of the /10 stay public, since refusing every mapped address would be worse than the leak it fixes. Also lands the `/edge-cases` sweep that found this (F0). Its five remaining strict-xfails are real defects, now each tracked and named from its marker: F1 ent#399, F2 ent#398, F3 ent#397, F4 ent#395, F5b ent#396. The sibling gap in `mcp_validator`, which carries no CGNAT clause at all, is ent#394 — left out deliberately: it is not in this fix's blast radius and is vendored byte-identically under Invariant #5. Fixes Abilityai/trinity-enterprise#393 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reopening under the |
…uard (#2174) PR #2178 landed `tests/unit/test_736_a2a_outbound_edges.py` — the /edge-cases sweep's own file — with this defect pinned as a strict-xfail (G7). My branch had added a same-path file, which would have conflicted at merge and duplicated the coverage. The sweep's file is canonical; this merges it in, removes the G7 xfail, and keeps only the cases it does not already cover in a separate file. The prototyped create-time guard (a new endpoint may not be named after an existing id) is dropped. It invalidated G7's own setup — the sweep creates the collision through `upsert_endpoint`, which the guard refuses — and, being create-time, it does nothing for a collision already stored, which is the case actually at risk. The delete fix stands alone; the guard can be filed separately if wanted. Related to #2174
|
Heads-up on a collision with what just landed on
Two pins in this PR will XPASS-strict once rebased, because both defects are now fixed on
Happy to do the rebase + un-xfail work on this branch if that is easier than you carrying it. |
…omparison (ent#398) (#2180) Port `:0` meant three different things along one outbound call path: `_validate_public_https_url` coalesced it to the scheme default (`parsed.port or 443` — `0` is falsy), `_pinned_url` dropped it and so connected to 443, and `_same_origin` compared it literally as port 0. A `:0` endpoint therefore validated, would have connected correctly, and was then permanently refused `card_origin_mismatch` against any card declaring the ordinary form. Fail-closed, so nothing leaks or misroutes — the damage is a permanently broken endpoint with a reason code that points at the wrong thing. The hazard worth fixing is the three independent normalisations of one field: today they disagree harmlessly, and an edit to any one of them is what turns that into something else. - `utils.url_validation.effective_port(port, scheme)` + `SCHEME_DEFAULT_PORTS`: `0` and absent both mean the scheme default (port 0 is not a connectable destination; browsers refuse `:0` rather than dialing it), an explicit port is preserved, an unknown scheme yields None for the caller to interpret. - All three sites consume it. `_same_origin` keeps `-1` for "unknown scheme, no default", so two such URLs stay comparable without matching a real port. The junk-port read stays inside its except clause, so a bad authority is still a named refusal rather than a traceback. - `_pinned_url` now omits a port equal to the scheme default rather than spelling it out — same destination, and consistent with the default-port equivalence the card comparison is built on. The `Host` header still preserves what the operator typed, which is the one place an explicit `:443` is observable to a peer. Default-port equivalence (Trinity's own card emits no port, so `https://h` and `https://h:443` must be one origin, #736/#738) is preserved and pinned by test. Verified against the live instance's own code: on dev, a `:0` endpoint validates to port 443, pins to the bare IP, and `_same_origin` returns False — refused card_origin_mismatch; on this branch the same input returns True and the call would proceed. Tests: tests/unit/test_ent398_port_normalisation.py — 21 cases (the normalisation truth table, the three consumers agreeing, the end-to-end card comparison, the Host header, default-port equivalence both ways, non-default ports surviving pinning, junk ports still named refusals, unknown-scheme comparison). The sibling pin `test_736_a2a_outbound_edges.py::test_D7_…` lives on the PR #2178 branch and must be un-xfailed when that lands; this file holds the same property on dev meanwhile. Related to trinity-enterprise#398
Retargeted from the merged fix/ent398-port-normalisation to dev.
Two conflicts, both adjacent-line churn rather than real disagreement:
* a2a_client.py import block — dev never touched the line; took ours
(`canonical_origin_host`). Verified `_canonical_host` has no remaining
caller in the file, so dropping the alias breaks nothing.
* requirements/mcp.md — pure add-vs-nothing; kept FR-4b, which now sits
correctly after #2180's FR-4a with no duplication.
The integration finding: #2178 landed on dev while this PR was stacked, and
it carries F1 as a `strict=True` xfail. ent#399 fixes that defect, so the
marker XPASSed and CI would have gone red on merge — exactly the un-xfail
obligation this PR's own description flagged. Removed the F1 marker, flipped
its tracker row to FIXED and updated the surrounding prose, per the file's
stated convention that a fix "cannot land without deleting the marker".
F3 (ent#397, #2182) and F5b (ent#396, #2183) stay xfail — each is its own
PR's obligation.
Green locally: 906 passed across the a2a / url-validation / registry suites.
…99) (#2181) * fix(a2a): one port normalisation for validation, pinning and origin comparison (ent#398) Port `:0` meant three different things along one outbound call path: `_validate_public_https_url` coalesced it to the scheme default (`parsed.port or 443` — `0` is falsy), `_pinned_url` dropped it and so connected to 443, and `_same_origin` compared it literally as port 0. A `:0` endpoint therefore validated, would have connected correctly, and was then permanently refused `card_origin_mismatch` against any card declaring the ordinary form. Fail-closed, so nothing leaks or misroutes — the damage is a permanently broken endpoint with a reason code that points at the wrong thing. The hazard worth fixing is the three independent normalisations of one field: today they disagree harmlessly, and an edit to any one of them is what turns that into something else. - `utils.url_validation.effective_port(port, scheme)` + `SCHEME_DEFAULT_PORTS`: `0` and absent both mean the scheme default (port 0 is not a connectable destination; browsers refuse `:0` rather than dialing it), an explicit port is preserved, an unknown scheme yields None for the caller to interpret. - All three sites consume it. `_same_origin` keeps `-1` for "unknown scheme, no default", so two such URLs stay comparable without matching a real port. The junk-port read stays inside its except clause, so a bad authority is still a named refusal rather than a traceback. - `_pinned_url` now omits a port equal to the scheme default rather than spelling it out — same destination, and consistent with the default-port equivalence the card comparison is built on. The `Host` header still preserves what the operator typed, which is the one place an explicit `:443` is observable to a peer. Default-port equivalence (Trinity's own card emits no port, so `https://h` and `https://h:443` must be one origin, #736/#738) is preserved and pinned by test. Verified against the live instance's own code: on dev, a `:0` endpoint validates to port 443, pins to the bare IP, and `_same_origin` returns False — refused card_origin_mismatch; on this branch the same input returns True and the call would proceed. Tests: tests/unit/test_ent398_port_normalisation.py — 21 cases (the normalisation truth table, the three consumers agreeing, the end-to-end card comparison, the Host header, default-port equivalence both ways, non-default ports surviving pinning, junk ports still named refusals, unknown-scheme comparison). The sibling pin `test_736_a2a_outbound_edges.py::test_D7_…` lives on the PR #2178 branch and must be un-xfailed when that lands; this file holds the same property on dev meanwhile. Related to trinity-enterprise#398 * fix(a2a): compare IP-literal origins as addresses, not as text (ent#399) `_same_origin`'s docstring claimed "IPv6 bracket forms compared after normalisation"; the code compared `urlsplit().hostname` textually. `canonical_host` leaves a literal untouched — `idna.encode` rejects it, the ASCII fallback returns it verbatim — so `[2606:4700:4700::1111]` and `[2606:4700:4700:0:0:0:0:1111]`, one address written two ways, were refused `card_origin_mismatch`. Fail-closed, but an IPv6-literal endpoint was unusable whenever its peer's card and its registration spelled the address differently, which they have no reason to agree on. The false docstring is the other half of the defect: it is what the next reader builds on. - `utils.url_validation.canonical_origin_host` — an IP literal is parsed through `ipaddress` and compared canonically; a name still goes through `canonical_host` (UTS-46 IDNA, SV-7); anything neither path canonicalises keeps the existing textual comparison, so an unusual host stays comparable to itself rather than becoming un-callable. - The scope id is part of the key: `fe80::1%eth0` and `fe80::1%eth1` stay the different destinations they are. - Ambiguous IPv4 spellings (leading zeros, integer forms) are REFUSED by `ipaddress`, not folded, so this can never equate two addresses a resolver would treat differently. - The docstring now states what the code does. The same-origin property the card check exists for is unweakened: a card declaring a different address, port or scheme is still refused — verified against a real peer that listens at ::1111 while its card declares ::2222. Verified live against a real IPv6 TLS peer (card declaring the expanded form): dev refuses the compressed and mixed-group registrations `card_origin_mismatch` and accepts only the byte-identical spelling; this branch completes all of them. Tests: tests/unit/test_ent399_ipv6_origin.py — 29 cases, incl. the AC-named test_E6_ipv6_literal_origins_compare_equal_across_spellings (the sibling pin lives on the PR #2178 branch and must be un-xfailed when that lands), a symmetric equivalence table, the must-stay-different table (address, port, scheme, scope id, v4-vs-mapped, literal-vs-name), the ambiguous-spelling guard, the IDN clause, and the textual fallback. Related to trinity-enterprise#399 * test(ent#399): use the linter's sanctioned _STUBBED_MODULE_NAMES spelling tests/lint_sys_modules.py recognises the snapshot/restore exemption by the exact top-level name `_STUBBED_MODULE_NAMES` paired with an autouse `_restore_sys_modules` fixture. This file had the whole pattern right — pre-stub snapshot, autouse restore, pop-vs-reassign — but spelled the list `_STUBBED`, so the AST check did not match it and counted three new violations against the ratcheted baseline. Never surfaced before because the check runs on a `dev` base only, and this PR was stacked on fix/ent398-port-normalisation until now. Lint: 169 violations in 56 files, baseline 240, no new. Tests unchanged. --------- Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com>
…able
Retargeted from the merged fix/ent399-ipv6-origin-normalisation to dev.
Conflicts, all add-vs-nothing or squash artefacts:
* url_validation.py and requirements/mcp.md — both sides append at the same
point; kept ours (PublicUrlRefusal, FR-3a). FR-3a now sits after #2181's
FR-4b with no duplication.
* test_ent399_ipv6_origin.py — add/add, because #2181 landed as a SQUASH so
the stacked commit and the merged one are different objects. Took dev's
copy verbatim; this PR does not touch that file, and dev's version carries
#2181's `_STUBBED_MODULE_NAMES` lint fix.
Un-xfailed F3 in test_736_a2a_outbound_edges.py — #2178 pins it strict, so
ent#397's fix made it XPASS and CI would have gone red. Tracker row flipped to
FIXED, prose updated, per the file's own convention. F5b stays for #2183.
H1's `_endpoint_url_reasons()` needed the same treatment for a subtler reason.
It regex-scans url_validation.py for literal `A2AEndpointUrlError("...")` raise
sites — which IS the shape ent#397 deliberately removes. With two reasons now
produced through `_A2A_REASON_BY_KIND`, the literals-only scan under-reported
and H1 called `endpoint_private_address` a dead map entry while the router still
maps it correctly. The helper now reads both production sites, so it measures
what the module emits rather than how it spells it. Verified at runtime that the
table's values, `A2A_URL_REASONS` and the router's status map all agree, and
that the table covers every declared kind.
Also carried the `_STUBBED` -> `_STUBBED_MODULE_NAMES` rename in
test_ent397_refusal_reasons.py, the same lint gap #2181 hit — invisible until
retargeting, since that check only runs on a `dev` base.
Green locally: 1366 passed across a2a / url-validation / registry / template
suites; lint 169/240, no new violations.
…prose (ent#397) (#2182) * fix(a2a): one port normalisation for validation, pinning and origin comparison (ent#398) Port `:0` meant three different things along one outbound call path: `_validate_public_https_url` coalesced it to the scheme default (`parsed.port or 443` — `0` is falsy), `_pinned_url` dropped it and so connected to 443, and `_same_origin` compared it literally as port 0. A `:0` endpoint therefore validated, would have connected correctly, and was then permanently refused `card_origin_mismatch` against any card declaring the ordinary form. Fail-closed, so nothing leaks or misroutes — the damage is a permanently broken endpoint with a reason code that points at the wrong thing. The hazard worth fixing is the three independent normalisations of one field: today they disagree harmlessly, and an edit to any one of them is what turns that into something else. - `utils.url_validation.effective_port(port, scheme)` + `SCHEME_DEFAULT_PORTS`: `0` and absent both mean the scheme default (port 0 is not a connectable destination; browsers refuse `:0` rather than dialing it), an explicit port is preserved, an unknown scheme yields None for the caller to interpret. - All three sites consume it. `_same_origin` keeps `-1` for "unknown scheme, no default", so two such URLs stay comparable without matching a real port. The junk-port read stays inside its except clause, so a bad authority is still a named refusal rather than a traceback. - `_pinned_url` now omits a port equal to the scheme default rather than spelling it out — same destination, and consistent with the default-port equivalence the card comparison is built on. The `Host` header still preserves what the operator typed, which is the one place an explicit `:443` is observable to a peer. Default-port equivalence (Trinity's own card emits no port, so `https://h` and `https://h:443` must be one origin, #736/#738) is preserved and pinned by test. Verified against the live instance's own code: on dev, a `:0` endpoint validates to port 443, pins to the bare IP, and `_same_origin` returns False — refused card_origin_mismatch; on this branch the same input returns True and the call would proceed. Tests: tests/unit/test_ent398_port_normalisation.py — 21 cases (the normalisation truth table, the three consumers agreeing, the end-to-end card comparison, the Host header, default-port equivalence both ways, non-default ports surviving pinning, junk ports still named refusals, unknown-scheme comparison). The sibling pin `test_736_a2a_outbound_edges.py::test_D7_…` lives on the PR #2178 branch and must be un-xfailed when that lands; this file holds the same property on dev meanwhile. Related to trinity-enterprise#398 * fix(a2a): compare IP-literal origins as addresses, not as text (ent#399) `_same_origin`'s docstring claimed "IPv6 bracket forms compared after normalisation"; the code compared `urlsplit().hostname` textually. `canonical_host` leaves a literal untouched — `idna.encode` rejects it, the ASCII fallback returns it verbatim — so `[2606:4700:4700::1111]` and `[2606:4700:4700:0:0:0:0:1111]`, one address written two ways, were refused `card_origin_mismatch`. Fail-closed, but an IPv6-literal endpoint was unusable whenever its peer's card and its registration spelled the address differently, which they have no reason to agree on. The false docstring is the other half of the defect: it is what the next reader builds on. - `utils.url_validation.canonical_origin_host` — an IP literal is parsed through `ipaddress` and compared canonically; a name still goes through `canonical_host` (UTS-46 IDNA, SV-7); anything neither path canonicalises keeps the existing textual comparison, so an unusual host stays comparable to itself rather than becoming un-callable. - The scope id is part of the key: `fe80::1%eth0` and `fe80::1%eth1` stay the different destinations they are. - Ambiguous IPv4 spellings (leading zeros, integer forms) are REFUSED by `ipaddress`, not folded, so this can never equate two addresses a resolver would treat differently. - The docstring now states what the code does. The same-origin property the card check exists for is unweakened: a card declaring a different address, port or scheme is still refused — verified against a real peer that listens at ::1111 while its card declares ::2222. Verified live against a real IPv6 TLS peer (card declaring the expanded form): dev refuses the compressed and mixed-group registrations `card_origin_mismatch` and accepts only the byte-identical spelling; this branch completes all of them. Tests: tests/unit/test_ent399_ipv6_origin.py — 29 cases, incl. the AC-named test_E6_ipv6_literal_origins_compare_equal_across_spellings (the sibling pin lives on the PR #2178 branch and must be un-xfailed when that lands), a symmetric equivalence table, the must-stay-different table (address, port, scheme, scope id, v4-vs-mapped, literal-vs-name), the ambiguous-spelling guard, the IDN clause, and the textual fallback. Related to trinity-enterprise#399 * fix(a2a): set each refusal reason at its raise site, not by matching prose (ent#397) `validate_a2a_endpoint_url` derived its machine-readable `reason` by substring-matching the human message — the exact fragility `A2AEndpointUrlError`'s own docstring warns against. The DNS-failure message interpolates the hostname and `canonical_host` passes hosts containing spaces, so `https://an internal address.example/a2a` — a host that merely failed to resolve — was reported `endpoint_private_address`. No bypass: both refusals map to the same HTTP status, so nothing is admitted that should not be. The cost is diagnostic, and structural: a machine-readable code reconstructed from prose is not machine-readable. The message must stay free to be reworded; the code must not be free to change meaning. - `PublicUrlRefusal(kind, message)` — raised at all nine refusal sites in `_validate_public_https_url` (`invalid`, `not_https`, `credentials`, `private_address`, `dns_failure`). - The A2A wrapper maps kind → reason through one table (`_A2A_REASON_BY_KIND`), with no string inspection. `credentials` keeps mapping to `endpoint_invalid`, the code it has always carried — inventing a new code would change the route's HTTP mapping under a bug fix. - `PublicUrlRefusal` subclasses `ValueError`, so the template-registry validator, which lets it propagate, is unchanged in both type and message text. - A `ValueError` raised without a kind still becomes `endpoint_invalid`: honest about not knowing which refusal it was, rather than guessing from the prose. Verified live on real code: a DNS-failing host whose NAME contains "internal address" reports `endpoint_private_address` on dev and `endpoint_dns_failure` on this branch; every other refusal keeps the code it had. Tests: tests/unit/test_ent397_refusal_reasons.py — 23 cases, incl. the AC-named test_D12_a_dns_failure_is_never_misreported_as_a_private_address (the sibling pin lives on the PR #2178 branch and must be un-xfailed when that lands), every phrase the old matcher keyed on spelled as a hostname, one case per refusal path, an AST guard that no raise site in the shared helper reverts to a bare ValueError, a kind→reason coverage check, the template-registry caller's unchanged behaviour, and the topology-oracle property (a refusal never echoes the resolved address). Related to trinity-enterprise#397 --------- Co-authored-by: trinity-ability <309458136+trinity-ability@users.noreply.github.com> Co-authored-by: trinity-ability <trinity@ability.ai>
Fixes abilityai/trinity-enterprise#393
Summary
_is_internal_addressgated its RFC 6598 CGNAT clause onip.version == 4. The IPv4-mapped form::ffff:100.64.0.1reports version 6, so the clause never ran — and CGNAT is exactly the range Python'sipaddressanswersFalseto for all six of its own predicates, which is why ent#14 S3 added the clause in the first place._validate_public_https_url:validate_a2a_endpoint_url, which then sends a credentialedAuthorization: BearerPOST (feat: add call_a2a_agent MCP tool for outbound A2A protocol calls #736), andvalidate_template_registry_url.ipv4_mappedwhen v6) and tests100.64.0.0/10membership against that.Why only this one range drifted
Every other internal range survives its mapped form for free, because CPython short-circuits
is_private/is_reservedthroughipv4_mappedbefore consulting its IPv6 tables. CGNAT is the one range whose refusal is Trinity's own code, so it is the one that had to do that delegation itself.The suite had already anticipated the evasion generically — it parametrised
::ffff:127.0.0.1and::ffff:10.0.0.1— but those rows pass on borrowed delegation, so they proved nothing about the one clause that performs none. That is the durable lesson recorded indocs/memory/learnings.md.The fix is complete, not one shape in a queue
::ffff:0:0/96is the only v4-in-v6 embedding CPython can inherit an IPv4 verdict through. Verified empirically across every transition mechanism:ipv4_mapped?::ffff:0:0/96IPv4-mapped::/8IPv4-compatible (RFC 4291) / IPv4-translated (RFC 2765)64:ff9b::/96+64:ff9b:1::/48NAT642002::/166to42001::/32TeredoThose refusals are the interpreter's, not ours, so they are now pinned — an interpreter change could otherwise reopen them with the whole suite still green (the #1891 class, which a version-parity check structurally cannot catch).
A narrowing fix needs its widening guard
test_C1basserts the mapped rows either side of the /10 stay public. Refusing every mapped address would be a strictly worse bug than the leak — it breaks working endpoints instead of leaking to unreachable ones.Changes
src/backend/utils/url_validation.pyipv4_mappedis the complete answertests/unit/test_736_a2a_outbound_edges.py(new)/edge-casessweep that found this; F0's strict-xfail removed,test_C1badded, 4 completeness rowstests/unit/test_736_a2a_outbound_properties.py(new)INTERNAL_ADDRESSESlisttests/unit/test_ent14_registry_url_ssrf.pytests/unit/test_736_a2a_url_validation.pydocs/memory/learnings.mdAcceptance criteria
_is_internal_addressrefuses::ffff:100.64.0.1,::ffff:100.127.255.254and any address whoseipv4_mappedform falls in100.64.0.0/10validate_a2a_endpoint_urlreturnsendpoint_private_addressfor a host resolving only to mapped CGNATvalidate_template_registry_urlrefuses the sametest_C1_the_mapped_form_of_cgnat_is_refused_like_every_other_mapped_formis removed and the test passes plainlytest_internal_destinations_are_refused's parametrize listTest plan
tests/unit: 10,173 passedFailure triple per
learnings.md2026-08-13 (a differing failure set is not by itself a regression):88a4e2f7test_pat_propagation_properties::TestKnownGaps×3 fails in all three — pre-existing, order-independent. Everything else moves between runs:test_1081_physical_meterandtest_1966_ops_reset_auditfail on base and pass here;test_subscription_auto_switch_pingpongerrors only in the full run and passes 29/29 standalone. This change introduces zero failures.Known defects landing with the sweep (tracked, not silent)
This PR lands the edge-case file whose five remaining strict-xfails are real, unfixed defects.
strict=Truemeans none can rot silently in code, and each marker now names its tracker issue:_same_origincompares IPv6 literals textually (fail-closed):0normalised three ways along one call path (fail-closed)reasonreverse-engineered from its own messageremove_endpointcan delete two endpoints and both credentialsAlso filed: abilityai/trinity-enterprise#394 —
mcp_validator._resolves_to_private_ipcarries no CGNAT clause at all, in either form. Deliberately out of scope here: it is not in this fix's blast radius, has a different threat model (config hygiene, fails closed on DNS failure, no credential on the wire), and is vendored byte-identically under Invariant #5, so fixing it is 2 files plus a parity assertion.Notes for review
test_refusal_message_never_echoes_the_resolved_address) is unchanged.architecture.md'surl_validation.pyentry remains accurate as written.🤖 Generated with Claude Code