Repository navigation
Conversation
…missive resolution, verify cross-domain claims A sales agent syncing adagents.json for publisher domain D had three authorization decisions left to it, and each implementation made them differently (#1697): - properties_for_publisher_domain(properties, D) answers "which of these authorized properties are D's" for shared and network files. A declared publisher_domain wins; otherwise a type=domain identifier naming D or any subdomain (www., m., shop., *.D) attributes the property. Domainless properties (bundle-only apps, venues) are excluded unless include_domainless=True. www.D is treated as D; hosts compare in A-label form; D must be registrable (an IP, single label or public suffix such as co.uk or github.io raises). - resolve_properties_for_agent(mode="permissive") now requires publisher_domain=, the domain the document was fetched for. The bare-entry fallback keeps only properties whose identifiers are all type=domain under that domain, so a file on attacker.example can no longer bare-list an agent and claim another publisher's site, app bundle or DOOH venue. A property whose own publisher_domain names another domain, or that carries a bundle, store or venue identifier, is dropped. allow_any_domain=True keeps the SDK 9 result. Typed selectors and strict mode are unchanged. - verify_cross_domain_properties(properties, source_domain=A, agent_url=...) keeps a claim about another publisher's domain B only when B vouches: the document governing B, fetched with fetch_adagents (so B's authoritative_location, or its ads.txt MANAGERDOMAIN when B has no file, is followed), authorizes the agent under strict resolution for every identifier of the property. A domain identifier is vouched for by its registrable domain; a declared publisher_domain by that domain. It fails closed: every named publisher must vouch for the whole property, any fetch failure or bare entry vouches for nothing, unusable domain values are invalid_domain, and at most max_domains (50) distinct domains are fetched. The fetcher is injectable. Tests are written from the attacker's side: lookalike and suffix domains, mixed-domain and relabelled properties, bundles bolted onto a vouched site, a relay publisher vouching for a victim, private-suffix tenants, fetch failures, fan-out bounds, and delegation through authoritative_location and MANAGERDOMAIN with real fetch_adagents over a mock transport. BREAKING CHANGE: resolve_properties_for_agent(mode="permissive") raises ValueError unless publisher_domain= (or allow_any_domain=True) is passed, and with publisher_domain its bare-entry fallback returns only domain properties under that domain. See MIGRATION_v9_to_v10.md. Closes #1697 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean review, no blocking or medium findings.
Checked:
- Security gate (fail-closed):
_only_names_domain(adagents.py L371-384) drops bare-entry properties unless every identifier istype:"domain"under the fetched domain and a declaredpublisher_domainnames it;[D, victim.com]and[D, bundle]are correctly rejected._vouching_domains(L474-505) returns the offending raw value on any unusable host and reduces to eTLD+1, so a relay file cannot vouch for a victim's registrable domain._claim_covered(L643-650) requires every named publisher to cover every identifier under strict resolution. - Bounds/ops: fan-out bounded via
to_fetch[:max_domains](L598); every fetch failure returnsfetch_failed(L606-608), not a verified claim; default fetcher has a 10s timeout;max_concurrencysemaphore caps parallelism;_authorizedswallows Exception → None soasyncio.gathercannot propagate. - Semver signal: breaking API change (permissive mode raises without
publisher_domain=) carriesfeat(adagents)!:, a BREAKING CHANGE footer, and a MIGRATION_v9_to_v10.md entry — the mandatory public-surface semver gate is satisfied. - Public surface: new exports wired consistently through all three
src/adcp/__init__.pysites; they are utility functions (not ADCPHandler methods), so the AGENTS.md handler-table requirement does not apply.
high_risk is true only because src/adcp/__init__.py is modified, but the reviewer found no medium-or-higher concerns on it, so the modification is presumed safe (row 5 does not fire without a medium finding). gated_paths is false; no author-team gate; no prior decision. No rows 1–8 trigger — falls through to row 9.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass is clean, net improvement over prior.
Delta since the prior approve is a single file: the generated tests/fixtures/public_api_snapshot.json, recording six purely-additive public names (AdagentsFetcher, CrossDomainFailureReason, CrossDomainVerification, UnverifiedProperty, properties_for_publisher_domain, verify_cross_domain_properties). No removals, renames, or signature changes — the snapshot gate stays armed. The breaking behavior change (resolve_properties_for_agent(mode="permissive") now requiring publisher_domain=) is correctly carried by the feat(adagents)!: commit and documented in MIGRATION_v9_to_v10.md, satisfying the semver-signal rule.
Decision-table walk: no critical/high/medium findings (row 1 N/A). gated_paths is false (row 2 N/A). high_risk is true via src/adcp/__init__.py (modified), but no medium-or-higher finding on it, so rows 3–5 do not fire — a modified high-risk file with no concern is presumed safe. Prior decision was approve, not escalate (row 6 N/A). No no-auto-approve team match (row 7 N/A). Zero medium findings (row 8 N/A). Falls through to row 9 → approve.
…pace Its snapshot fingerprint differs on Python 3.10, where typing.Any is a _SpecialForm. It stays importable from adcp.adagents. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — delta-only pass since the prior approve, clean with no new findings.
What I checked:
- The only meaningful change since the prior approve is un-exporting the
AdagentsFetchertype alias from the publicadcp.*surface (lazy map,__all__, TYPE_CHECKING block, and the API snapshot fixture). The reviewer confirms removal is complete with no dangling references and the alias remains defined in adagents.py for thefetcher=annotation. - No semver concern: the alias was introduced by this same unreleased PR, so trimming it before cut is not a public-surface break under the repo's semver-signal rule.
- high_risk is true only because
src/adcp/__init__.pyis (modified). Per the decision rules, a modification with no medium-or-higher finding is presumed safe — not an escalation trigger (row 5 requires a medium finding; none exist). - gated_paths is false and review_decision is APPROVED, so no hard gate applies. No no-auto-approve team match.
Decision table: rows 1–8 do not fire (no critical/high findings, gated_paths false, no deletions, no medium findings, prior decision was approve, no team gate). Falls through to row 9 → approve.
|
Feedback from the Scope3 sales agent, which syncs publishers' adagents.json files and will adopt this. The attribution helper and the shape of 1. Let the permissive gate report what it excludesWith Many sellers are onboarding while publisher files are still incomplete. For them, the right production behavior is to keep an unauthorized claim and mark it, so the seller and the buyer can see the status and the operator can decide when to enforce. Two ways to support that:
One case to note either way: 2. Neutral wording for
|
Problem
get_properties_by_agent/resolve_properties_for_agentsay which properties an agent is authorized for in one adagents.json. A sales agent syncing that file for publisher domainDthen had to make three more authorization decisions itself, and every implementation made them differently (#1697).Fix
1.
properties_for_publisher_domain(properties, D, *, include_domainless=False)This picks out D's properties from a shared or network file:
publisher_domainwins when it namesD. Identifiers are not consulted.type: "domain"identifier namingDor any subdomain counts (www.,m.,shop.,*.D).include_domainless=True.www.Dis treated asD, exceptwww.com-style names. Hosts compare in A-label form.Dmust be registrable: an IP, a single label or a public suffix (co.uk,github.io) raises.2. Permissive resolution is gated (breaking)
resolve_properties_for_agent(mode="permissive")now requirespublisher_domain=(the domain the document was fetched for), or an explicitallow_any_domain=True.type: "domain"under that domain.publisher_domainnames another domain, and properties carrying bundle, store or venue identifiers. A domain can't vouch for those.3.
verify_cross_domain_properties(properties, *, source_domain, agent_url, fetcher=None, ...)A claim in A's file about another publisher's domain B is kept only when B vouches. Vouching means the document governing B, fetched with
fetch_adagents, authorizes the agent (strict resolution, B's revocations honored) for every identifier of the property. Becausefetch_adagentsfollows B'sauthoritative_location, and B's ads.txtMANAGERDOMAINwhen B has no file, the same check covers "B authorizes the agent" and "B delegates to A".publisher_domainis vouched for by that domain.verified,unverified(withdomainand a reason:not_vouched,fetch_failed,invalid_domainordomain_limit) anddomainless.Conservative choices to review
These are the places where I picked the stricter option. None of them weakens an existing check.
allow_any_domain=Trueis the explicit escape hatch for the SDK 9 behavior.[D, victim.com]or[D, bundle:com.victim]through.publisher_domaincontradicts D is dropped rather than ignored.max_domains=50.MANAGERDOMAINcounts only when B publishes no file, perfetch_adagents. If B's own file doesn't authorize the agent, that is B's answer.verify_cross_domain=Trueflag on the resolver. I implemented the async helper with an injectable fetcher instead, since the resolver is sync.Tests
tests/test_adagents_publisher_domain.pyhas 49 tests, written from the attacker's side:authoritative_locationandMANAGERDOMAINwith realfetch_adagentsover a mock transportExisting permissive tests now pass
allow_any_domain=True(or the fetched domain). Alltests/test_adagents*.pypass locally (379). ruff, black and mypy are clean on the touched modules. I'm relying on CI for the full suite: the shared VM is saturated by parallel workstreams, and a local full run timed out.BREAKING CHANGE: resolve_properties_for_agent with mode permissive raises ValueError unless publisher_domain or allow_any_domain=True is passed, and with publisher_domain its bare-entry fallback returns only domain properties under that domain. See MIGRATION_v9_to_v10.md.
Closes #1697
🤖 Generated with Claude Code