Skip to content

fix-forward #2633: the dead-credential 404 fires on existing off-allowlist routes and the garbage-credential control is inert (tsk-sonaie) - #2792

Closed
jaylfc wants to merge 4 commits into
devfrom
exec/tsk-iqk2bn
Closed

fix-forward #2633: the dead-credential 404 fires on existing off-allowlist routes and the garbage-credential control is inert (tsk-sonaie)#2792
jaylfc wants to merge 4 commits into
devfrom
exec/tsk-iqk2bn

Conversation

@jaylfc

@jaylfc jaylfc commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

SUPERSEDED — do not merge. Close this.

Replaced by #2799, cut fresh from dev.

This PR was built on exec/tsk-iqk2bn, whose parent commits (4b27a6f9b,
f53531eac, 7bd12228a) never reached dev. While that line sat closed, dev
solved the same problem independently via ae71bb203 (fix-forward tsk-okf4cz,
PR #2716 on top of #2698/#2702), which introduced _any_route_matches and
consults the router before answering 404.

Verified on clean origin/dev @ b8f7726ea:

$ python -m pytest tests/test_a2a_bus_agent_auth.py::TestBusAgentAuth::test_skeleton_key_guard_agent_token_rejected_off_allowlist \
    tests/test_agent_scope_requests.py::test_agent_cannot_approve_its_own_request \
    tests/test_agent_scope_requests.py::test_agent_cannot_deny_its_own_request \
    "tests/test_auth_middleware.py::TestRegistryJwtRouteResolution" -q -p no:cacheprovider
8 passed in 25.67s
  • Card item 1 (the 404 firing on existing off-allowlist routes) is already fixed
    on dev, and dev's version also handles {x:path} converters, which this
    branch does not.
  • Card item 2 (the inert garbage-credential control) does not apply: dev has no
    _looks_like_registry_jwt.

Rebasing this branch would have re-landed a duplicate, dead second 404 branch
alongside dev's — the section-2 branch fires first for any registry JWT, so
everything this PR adds in section 4 would be unreachable.

The one piece of the chain that IS still a live defect on dev — identity-only
auth ignoring the token_min_iat rotation cutoff, so rotate-tokens revokes
nothing on the scope-request, decisions, container-request and auth-request
surfaces — is fixed in #2799 with its own RED-first evidence.

jaylfc added 4 commits August 28, 2026 23:40
@jaylfc
Fix auth middleware to return 404 for authenticated callers on unknow…
4b27a6f 
…n routes

The auth middleware runs BEFORE routing, so it reports a routing failure as an auth failure.
The fix allows a validated credential (local token) to continue to routing rather than
immediately returning, letting unknown routes return 404 for authenticated callers.

This preserves anti-enumeration for anonymous callers while removing ambiguity for
authenticated ones, so a wrong URL is indistinguishable from dead credentials.

Fixes: Unknown routes for valid local tokens now return 404 instead of 401.

Docs-Reviewed: The fix preserves the anti-enumeration property by letting routing handle
  the response for authenticated calls. Unauthenticated callers still get 401 on unknown
  routes, while authenticated calls get proper 404 for unknown routes or correct handling
  for known routes. The agent-token route allowlist in auth_middleware.py is unchanged,
  and no docs/agent-coordination.md modifications are needed.

Acceptance:
1. VALID token + unknown route -> 404 (fixed)
2. NO token + unknown route -> 401 (anti-enumeration preserved)
3. VALID token + real route -> 200 (control passes)
@jaylfc
fix auth middleware returns 404 for valid registry JWT on unlisted ro…
f53531e 
…utes

A valid credential on an unlisted route previously hit the terminal 401
fallthrough in _dispatch, making a wrong URL byte-identical to dead
credentials. The fix verifies the registry JWT signature (signature only,
no scope check) when a Bearer header is present but no other credential
path matched; if the signature is valid and the route is not in the closed
agent-token allowlist the middleware returns 404 directly, keeping routing
unreached (no skeleton key) and keeping the anonymous 401 unchanged.

Four acceptance tests added: valid JWT+unknown path -> 404; no credential
-> 401; garbage bearer -> 401; valid JWT on an existing non-allowlisted
route -> 404 and call_next not awaited.

Docs-Reviewed: the agent-token route allowlist is unchanged; the new 404
response for unlisted routes is a middleware-layer anti-enumeration
enhancement, not an API surface change.
@jaylfc
fix dead registry JWTs returning 404 instead of 401, strengthen garba…
7bd1222 
…ge-credential test

Docs-Reviewed: auth_middleware.py change only affects dead-credential detection (revoked/rotated JWTs), not the agent-token route allowlist
@jaylfc
fix(auth): 404 only for paths no route serves, not off-allowlist rout…
6b33ad7 
…es (tsk-sonaie)

The middleware's wrong-URL 404 fired for any live registry JWT that reached
the session gate, so a route that EXISTS but is deliberately off the
agent-token allowlist -- GET /api/agents/registry, and the scope-request
approve/deny routes -- answered 404 instead of the gate's 401. That is the
mirror image of the bug this chain exists to kill: the URL is correct and the
credential is simply not authorised for it, yet the caller was told the URL
was wrong. Three pre-existing guard tests were red at the previous head.

The 404 branch now consults the router first (Match.PARTIAL counts: a wrong
verb on a real path is still a real path) and only answers 404 when nothing
matches. Routing is still never invoked -- call_next is not awaited on either
arm -- so the allowlist stays closed.

Also wires state.agent_registry on test_garbage_bearer_unknown_path_returns_401.
Without it `await registry.get(...)` raised TypeError on a bare MagicMock and
the liveness check returned False for a reason unrelated to the signature, so
the control could not see signature verification being deleted. It now can.
@qodo-code-review

qodo-code-review Bot commented Sep 5, 2026

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: c1e3eff7-b49f-421f-936d-d5900038ee19

📥 Commits

Reviewing files that changed from the base of the PR and between b8f7726 and 6b33ad7.

📒 Files selected for processing (7)
  • changelog.d/tsk-hbzm7l-fix-route-enumeration.md
  • changelog.d/tsk-iqk2bn-dead-credential-404.md
  • changelog.d/tsk-sonaie-404-only-for-unrouted-paths.md
  • changelog.d/tsk-vylg2y-registry-jwt-unknown-route-404.md
  • docs/agent-coordination.md
  • tests/test_auth_middleware.py
  • tinyagentos/auth_middleware.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Sep 5, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@jaylfc jaylfc mentioned this pull request Sep 5, 2026
Merged
@jaylfc

jaylfc commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Lead review 2026-09-06 — blocked, superseded by tsk-etzofb

This branch and dev fixed the #2633 chain independently. dev shipped check_agent_identity + _any_route_matches at the unknown-route decision point in dispatch() (with ~15 tests patching that call site); this PR adds a parallel _path_is_routed / _route_list_matches / _looks_like_registry_jwt path at the same point. Merging origin/dev in leaves the two shadowing each other — 8 tests flip red depending on which runs first (4 of dev's, 4 of this PR's). That is not a mechanical conflict, so the fold pass stopped here and the branch is untouched at 6b33ad7.

Decision: one mechanism, dev's. Card tsk-etzofb ports this PR's TestRegistryJwtUnknownRouteDispatch (8 tests) onto dev as the red, fixes whatever is still red inside dev's mechanism, and carries the doc paragraph. #2799 (rotation cutoff inside check_agent_identity) lands first, which likely covers the rotated-token case already. This PR stays open lead-blocked and will be closed when the superseding PR merges.

@jaylfc jaylfc added the lead-blocked Lead has blocked this PR; gate_merge.sh refuses at exit 10. label Sep 6, 2026
@jaylfc jaylfc mentioned this pull request Sep 6, 2026
Merged
jaylfc added a commit that referenced this pull request Sep 6, 2026
@jaylfc
Merge pull request #2828 from jaylfc/exec/tsk-etzofb
9cbb425 
Supersede #2792 (tsk-iqk2bn): registry-JWT unknown-route + off-allowlist hardening must extend dev's shipped check_agent_identity/_any_route_matches mechanism, not ship a second one
@jaylfc

jaylfc commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #2828 (tsk-etzofb), merged 2026-09-06: the registry-JWT unknown-route hardening now extends dev's shipped check_agent_identity/_any_route_matches mechanism instead of shipping a second one. Closing without merge.

@jaylfc jaylfc closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

lead-blocked Lead has blocked this PR; gate_merge.sh refuses at exit 10.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jaylfc