Repository navigation
fix(a2a): set each refusal reason at its raise site, not by matching prose (ent#397) - #2182
Conversation
…omparison (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
`_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
…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
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.
…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.
vybe
left a comment
There was a problem hiding this comment.
Validated via /validate-pr, plus merge-integration work.
The fix is right and the reasoning in the description holds up. Deriving a machine-readable code by substring-matching a human message that interpolates an attacker-influenced hostname is the fragility A2AEndpointUrlError's own docstring warns against; PublicUrlRefusal(kind, message) set at all nine raise sites, mapped through one table with no string inspection, is the correct shape. Three details I checked specifically:
PublicUrlRefusalsubclassesValueError, so the template-registry validator — which lets it propagate — is unchanged in both type and message text.credentials→endpoint_invalidpreserves the code that refusal has always carried, so no route's HTTP mapping moves under a bug fix.- The bare-
ValueErrorfallback still answersendpoint_invalidrather than guessing from prose, which is the honest answer.
Verified at runtime that _A2A_REASON_BY_KIND covers every declared kind and that its values, A2A_URL_REASONS and the router's status map all agree.
Security clean, no packaging gaps, 3 files.
Three things needed to land it:
-
Retargeted to
devand merged dev in.url_validation.pyandmcp.mdwere add-vs-nothing (kept ours; FR-3a sits after #2181's FR-4b, no duplication).test_ent399_ipv6_origin.pycame through add/add because #2181 landed as a squash, so the stacked commit and the merged one are different objects — took dev's copy verbatim, since this PR doesn't touch that file. -
Un-xfailed F3 in
test_736_a2a_outbound_edges.py— #2178 pins itstrict=True, so this fix made it XPASS and CI would have gone red. Per the file's own convention: marker deleted, tracker row FIXED. -
H1 needed the same treatment, and this one is worth a look.
_endpoint_url_reasons()regex-scansurl_validation.pyfor literalA2AEndpointUrlError("...")raise sites — which is precisely the shape this PR removes. With two reasons now produced via the table, the literals-only scan under-reported and H1 declaredendpoint_private_addressa dead map entry while the router still maps it correctly. I taught the helper to read both production sites, so it measures what the module emits rather than how it spells it. Flagging because it's the sort of guard that looks like it caught a regression when it actually caught its own assumption.
Green locally: 1366 passed across the a2a / url-validation / registry / template suites; lint 169/240, no new violations.
Note: Related to trinity-enterprise#397 is cross-tracker, so ent#397 won't auto-promote — bumping it manually.
| because an operator debugging a typo needs to see which name failed.""" | ||
| with pytest.raises(A2AEndpointUrlError) as exc: | ||
| validate("https://typo.example.com/a2a") | ||
| assert "typo.example.com" in str(exc.value) |
|
Resolve by running |
… spelling (#2175 F5b) `upsert_endpoint` documents three credential paths — omit (leave), set, `clear_credential` (remove). A whitespace-only value was a fourth: it skipped the header-safety check (`if credential.strip() and …`) and then took the `elif credential:` branch, because `" "` is truthy, writing `""`. So `None` and `""` preserved the stored secret while `" "` silently destroyed it. Not reachable over HTTP — `A2AOutboundEndpointUpsert._validate_credential` normalises a blank `SecretStr` to `None`, so the API surface is honest. This is a public module function, the value is a partner secret the caller may hold no other copy of, and "blank clears it" is the wrong default for that. One normalisation (`clean_credential`), applied after the validity checks and before either write path, so a future third write site cannot reintroduce the fourth path. What does NOT change: `clear_credential` is still the only removal path; the length cap is still measured on the raw value, so the bound an operator is told about does not move when their secret has surrounding whitespace; and the header-safety guard still refuses a credential with an interior space — only an entirely blank one means "leave it alone". Verified on real code: updating a stored `real-secret` with `" "` or `"\t"` destroys it on dev and preserves it on this branch; `None` and `""` are unchanged on both. Tests: tests/unit/test_2175_blank_credential.py — 14 cases (every blank spelling, omission, create-path blank storing no key at all, a real credential still set / replaced / stripped, clear_credential precedence, the header-safety guard, the raw length cap, and that no empty-string credential can reach the caller as an empty Bearer header). F1/F2/F3 of this issue are fixed separately — ent#399/#2181, ent#398/#2180, ent#397/#2182. Related to #2175
… spelling (#2175 F5b) (#2183) * fix(a2a): a blank credential leaves the stored secret alone, in every spelling (#2175 F5b) `upsert_endpoint` documents three credential paths — omit (leave), set, `clear_credential` (remove). A whitespace-only value was a fourth: it skipped the header-safety check (`if credential.strip() and …`) and then took the `elif credential:` branch, because `" "` is truthy, writing `""`. So `None` and `""` preserved the stored secret while `" "` silently destroyed it. Not reachable over HTTP — `A2AOutboundEndpointUpsert._validate_credential` normalises a blank `SecretStr` to `None`, so the API surface is honest. This is a public module function, the value is a partner secret the caller may hold no other copy of, and "blank clears it" is the wrong default for that. One normalisation (`clean_credential`), applied after the validity checks and before either write path, so a future third write site cannot reintroduce the fourth path. What does NOT change: `clear_credential` is still the only removal path; the length cap is still measured on the raw value, so the bound an operator is told about does not move when their secret has surrounding whitespace; and the header-safety guard still refuses a credential with an interior space — only an entirely blank one means "leave it alone". Verified on real code: updating a stored `real-secret` with `" "` or `"\t"` destroys it on dev and preserves it on this branch; `None` and `""` are unchanged on both. Tests: tests/unit/test_2175_blank_credential.py — 14 cases (every blank spelling, omission, create-path blank storing no key at all, a real credential still set / replaced / stripped, clear_credential precedence, the header-safety guard, the raw length cap, and that no empty-string credential can reach the caller as an empty Bearer header). F1/F2/F3 of this issue are fixed separately — ent#399/#2181, ent#398/#2180, ent#397/#2182. Related to #2175 * test(a2a): retire the strict xfail that F5b's fix turns into an XPASS `test_G4_a_whitespace_only_credential_does_not_silently_clear_the_stored_one` documented the F5b bug with `@pytest.mark.xfail(strict=True)`. This branch fixes the bug, so the test now passes — and a strict XPASS is a hard failure, which would have taken the suite red on merge. The assertion was already written as the correct behaviour, so dropping the marker converts it in place from a bug-documenting xfail into a live regression test. Caught locally because CI never ran on this PR (no checks reported). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: trinity-ability <noreply@anthropic.com>
# Conflicts: # docs/memory/requirements/mcp.md
What this fixes
validate_a2a_endpoint_urlderived its machine-readablereasonby substring-matching itsown human message — the fragility
A2AEndpointUrlError's docstring explicitly warns about.The DNS-failure message interpolates the hostname, and
canonical_hostpasses hostscontaining spaces, so
https://an internal address.example/a2a— a host that merely failedto resolve — was reported
endpoint_private_address.No bypass: both refusals map to the same HTTP status. The cost is diagnostic, and structural
— the operator is told the opposite of what happened, any consumer branching on
reasonbranches wrongly, and a code reconstructed from prose is not machine-readable.
Changes
PublicUrlRefusal(kind, message), raised at all nine refusal sites in_validate_public_https_url:invalid,not_https,credentials,private_address,dns_failure._A2A_REASON_BY_KIND) with nostring inspection.
credentialskeeps mapping toendpoint_invalid— the code it hasalways carried; inventing a new one would change the route's HTTP mapping under a bug fix.
PublicUrlRefusalsubclassesValueError, so the template-registry validator (whichlets it propagate) is unchanged in type and message text — pinned by test.
ValueErrorwithout a kind still becomesendpoint_invalid— honest about notknowing which refusal it was, rather than guessing from the prose.
Verification on real code
Only the defective cell moves.
Tests
tests/unit/test_ent397_refusal_reasons.py— 23 cases:test_D12_a_dns_failure_is_never_misreported_as_a_private_address— the AC-named test,plain. The sibling pin is on the PR fix(security): mapped-CGNAT bypassed the SSRF destination predicate (ent#393) #2178 branch, not on
dev; it must be un-xfailedwhen fix(security): mapped-CGNAT bypassed the SSRF destination predicate (ent#393) #2178 lands. This file holds the property meanwhile.
must use HTTPS.example,could not be resolved.example, …)http://, non-httpsscheme, embedded credentials, junk port, out-of-range port, missing host, unparseable
resolver record, empty resolver result
_validate_public_https_urlreverts to a bareValueError— the defect's return pathendpoint_invaliddefault by accident)test_refusal_message_never_echoes_the_resolved_address,named in the AC) plus the counterpart: the DNS message still names the host the operator
typed, since that interpolation is the vector, not the defect
Green: 23 new, 1198 across every A2A / URL-validation / registry / skills-library / template
suite.
Related observation (not fixed here)
https:// peer.example.com/a2a— an authority with a leading space — is accepted ondevand on this branch:
canonical_hoststrips it, so the connection goes to the right hostwhile the stored URL keeps the space. That is the same "
canonical_hostpasses hostscontaining spaces" property this issue names as its vector, but tightening it is a separate
behaviour change and out of scope here. Happy to file it.
Related to trinity-enterprise#397