fix(fleet): defer plugin-view auth to the member so proxied views stop 401ing (#1890)#1894
Conversation
…p 401ing (#1890) A plugin view page is auth-exempt public chrome on the instance that serves it (a browser iframe navigation can't carry the operator bearer). Through a fleet hub the same page loads at `/agents/<slug>/plugins/<id>/…` — a prefix the hub's own public list can't match (it's built from the hub's manifests, and the member may run plugins the hub doesn't). Under a token-gated hub the headerless navigation fell through to the bearer check and returned the reported 401. The member is the authority on what it serves anonymously, so the hub defers to it: every instance now serves its live auth-exempt prefix list on the public `/.well-known/protoagent/public-paths` endpoint (the listed paths are already anonymously reachable, so this discloses nothing new). The auth gate checks slug-prefixed plugin-namespace paths against the *member's* list via an injected async resolver (`graph/fleet/member_public.py`, TTL-cached, fail-closed), and stamps `request.state.member_public` so the proxy forwards those requests anonymously — never lending a stored remote bearer to an unauthenticated caller (same posture as the remote-WS refusal). Consulted only under a real credential and only inside a plugin namespace; `/agents/<slug>/api/…` stays fully gated. Bonus (same rule as #1752 — any init-time registry wiring must re-apply on reload): the hot-reload commit now re-pushes plugin public prefixes to the live auth gate, so a hot-enabled plugin's view no longer stays 401 until a restart. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
QA Audit — PR #1894 | fix(fleet): defer plugin-view auth to the member so proxied views stop 401ing (#1890)
VERDICT: WARN (CI pending — auto-promotes to APPROVE on terminal-green)
CI Status
⏳ 8 checks still running: build, Verify workspace config, Fleet integration, Python tests, A2A live smoke, Web E2E smoke, Lint, gitleaks. No terminal failures observed.
Diff Review
- Core fix in
a2a_impl/auth.py: after_is_publicbypass, checks slug-prefixed plugin-namespace paths against the member's public list via an injected async resolver. Scoped tightly — only when hub is token-gated AND path matches/agents/<slug>/plugins/<id>/…. Fail-closed on resolver errors. - New
graph/fleet/member_public.py: fetches member's/.well-known/protoagent/public-paths, TTL-cached (30s/5s), re-validates prefixes with same_PLUGIN_NS_RE, never sends stored remote credentials. graph/fleet/proxy.py+2:forward_todrops stored remote bearer whenrequest.state.member_publicis set — no credential lending.- Hot-reload fix:
set_public_prefixesre-applied on reload (same #1752 rule). CHANGELOG + docs updated. - Tests: +8 in
test_a2a_auth.py, newtest_fleet_member_public.py, +2 intest_fleet_proxy.py, reload test updated.
Observations
- CLAWPATCH/LOW: Global httpx client in
graph/fleet/proxy.pyis never closed on shutdown — pre-existing, not introduced here. - CLAWPATCH/LOW: Several
server/__init__.pyhandlers (_root_to_console,_serve_sw,_prometheus_metrics,_serve_favicon) outside the diff excerpt — pre-existing, not introduced here. - The
_MEMBER_PUBLIClist-of-one pattern for the resolver slot is functional for async FastAPI but could be a module-level callable for clarity — not a blocker.
No HIGH or CRITICAL findings. The approve-on-green policy promotes this review to APPROVED automatically once every check settles terminal-green.
— Quinn, QA Engineer
|
Submitted COMMENT review on #1894. |
There was a problem hiding this comment.
CI terminal-green, no blockers on prior review — auto-approving on green (#748).
|
✅ CI went terminal-green with no blockers on the prior review — promoting it to APPROVED per the approve-on-green policy (#748). |
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 0da6cc3e8855 · shadow — comment-only
[review-synthesizer completed: workflow code-review-structural:report]
Verification Summary
Both findings confirmed. No refutations, no demotions — the verifier validated the core factual claims and only flagged a minor layering caveat (not independently verified) that doesn't weaken the finding.
Prose Brief
Overall risk is low: this PR adds a thin well-known discovery endpoint and consumer-side fetch logic, both backed by existing primitives. The fix-first item is the regex duplication in member_public.py — importing from a2a_impl/auth.py eliminates a genuine drift risk at trivial cost. The panel had no disagreement (only two findings, both confirmed). Verification changed nothing in severity or count; it only clarified that the unverified "graph/ may import from a2a_impl/" layering claim doesn't affect the core duplication defect. No gaps were reported by the panel (no structural pass was skipped — the code-review-structural engine's findings are already merged into these two).
[
{
"file": "graph/fleet/member_public.py",
"line": 41,
"severity": "minor",
"category": "conventions",
"claim": "The _PLUGIN_NS_RE regex is duplicated identically from a2a_impl/auth.py instead of importing the canonical definition.",
"evidence": "Both files define `_PLUGIN_NS_RE = re.compile(r\"^/(?:api/)?plugins/[^/]+/\")`. The new file's own comment acknowledges it: 'Same namespace shape the auth gate enforces on its own list (a2a_impl/auth.py).' Since graph/ may import from a2a_impl/ under the repo's layering contract, this is a copy-paste that could drift.",
"source": "conventions",
"verdict": "confirmed",
"note": "Both files define identical regex; comment in member_public.py:37 acknowledges the duplication ('Same namespace shape... a2a_impl/auth.py'). The layering claim (graph/ → a2a_impl/) not independently verified but the core factual claim holds."
},
{
"file": "server/__init__.py",
"line": 830,
"severity": "minor",
"category": "tests",
"claim": "The new /.well-known/protoagent/public-paths FastAPI endpoint has no direct HTTP-level test.",
"evidence": "The route `@fastapi_app.get(_member_public.WELL_KNOWN_PATH, include_in_schema=False)` is registered but never exercised by a test that hits the endpoint over HTTP. The underlying `public_prefixes()` getter is unit-tested in test_a2a_auth.py, and the consumer-side fetch is tested via FakeClient in test_fleet_member_public.py, but the actual server-side route wiring is untested.",
"source": "tests",
"verdict": "confirmed",
"note": "test_a2a_auth.py uses Starlette TestClient (no FastAPI well-known route); test_fleet_member_public.py tests consumer-side member_public_prefixes() via FakeClient only. No test hits the FastAPI route at /.well-known/protoagent/public-paths. public_prefixes() getter itself is a trivial list-return wrapper, not separately tested (but _PLUGIN_PUBLIC is exercised via set_public_prefixes tests)."
}
]
Closes #1890.
Problem
On a token-gated fleet hub, viewing another member's plugin views 401s:
{"detail": "Unauthorized: expected 'Authorization: Bearer <token>'"}(observed on a protoContent instance — Jon viewing Matt's plugins).
A plugin view page is deliberately auth-exempt public chrome (
_view_public_paths): a browser iframe navigation can't carry anAuthorizationheader, so the page loads unauthenticated and then receives the bearer via the postMessage init handshake for its gated/api/plugins/<id>/*data calls.Through the hub the same page loads at
/agents/<slug>/plugins/<id>/view. The hub'sA2AAuthMiddleware._is_publicdoes plainstartswithmatching against prefixes like/plugins/<id>/view— the/agents/<slug>prefix defeats the match (and the hub's public list is built from the hub's manifests anyway; the member may run plugins the hub doesn't). The headerless navigation falls through to the bearer check → 401. It only surfaces under a bearer-gated hub (tailnet/homelab); open-mode desktop hubs pass everything.Fix
The member is the authority on what it serves anonymously, so the hub defers to it:
GET /.well-known/protoagent/public-paths(the listed paths are already anonymously reachable — this discloses nothing new).A2AAuthMiddlewarechecks a slug-prefixed plugin-namespace path against the member's list through an injected async resolver (graph/fleet/member_public.py— TTL-cached, fail-closed), and stampsrequest.state.member_public.proxy.forward_tosees that stamp and forwards the request anonymously — it does not attach a stored remote bearer, so an unauthenticated caller can never borrow the remote's credential (same posture as the existing remote-WS refusal inforward_ws).Scoped tightly: consulted only when a credential actually gates the hub, and only inside a
/plugins/<id>/namespace —/agents/<slug>/api/…(incl./api/plugins/install) stays fully gated. A member can only ever open its own/plugins/<id>/…subtree on the hub, exactly as far as it already opens it to direct callers. Resolver error / unreachable member / bad payload all fail closed to normal bearer auth.Bonus fix (same class as #1752)
The hot-reload commit now re-pushes plugin public prefixes to the live auth gate (
set_public_prefixes) — without it, a hot-enabled plugin's view page stayed 401 until a full restart. Same rule as #1752: any init-time registry wiring must re-apply on reload.Tests
tests/test_a2a_auth.py(+8): member-public path passes without a bearer and is stamped; false → 401; not consulted outside a plugin namespace or in open mode; resolver error fails closed; the console's bearer-attached probe is unaffected;public_prefixes()getter.tests/test_fleet_member_public.py(new): fetch + prefix match + TTL cache; non-conforming member prefixes dropped; unreachable / non-200 / bad-shape / unresolvable-slug all fail closed; cache expiry refetch; never sends stored remote credentials to the well-known endpoint.tests/test_fleet_proxy.py(+2):forward_todrops the stored remote bearer for amember_publicrequest, keeps it for normal authed traffic.tests/test_reload_rebuild_deps.py: fakeLoadedPluginsgainspublic_paths; reload no-op'd for the newset_public_prefixescall.Gates green in-worktree: full pytest (3437 passed / 5 skipped), ruff, lint-imports (3 contracts kept), live_smoke. Layering held — the resolver is injected from the server bootstrap so
a2a_impl/graphstay host-free.🤖 Generated with Claude Code