Support CIMD as preferred OAuth client registration for thv run by amirejaz · Pull Request #5085 · stacklok/toolhive

amirejaz · 2026-04-28T10:50:08Z

Summary

Implements Phase 1 of RFC-0071 — CIMD (Client ID Metadata Document) support for `thv run` as an OAuth client.

When a remote authorization server advertises `client_id_metadata_document_supported: true` in its discovery document, `thv run` now presents `https://toolhive.dev/oauth/client-metadata.json\` as its `client_id` instead of performing a Dynamic Client Registration (DCR) round-trip. Falls back to DCR gracefully if the AS rejects the CIMD `client_id`.

Draft until `https://toolhive.dev/oauth/client-metadata.json\` is live. Tracked in stacklok/infra#4549. The code is complete and reviewed; the PR can merge once the URL is reachable.

Changes

File	Change
`pkg/oauthproto/cimd.go`	New: `ToolHiveClientMetadataDocumentURL` constant and `IsClientIDMetadataDocumentURL` helper
`pkg/oauthproto/discovery.go`	Added `ClientIDMetadataDocumentSupported` to `AuthorizationServerMetadata`
`pkg/auth/discovery/discovery.go`	Added `ClientIDMetadataDocumentSupported` to `AuthServerInfo`; `applyDiscoveryPreCheck` helper caches discovered endpoints and applies CIMD URL before the DCR gate; `SkipCIMD` field on `OAuthFlowConfig` prevents pre-check re-applying CIMD on fallback retry
`pkg/auth/remote/config.go`	Added `CachedCIMDClientID`; added clear to `ClearCachedClientCredentials`
`pkg/auth/remote/handler.go`	CIMD `client_id` set in `buildOAuthFlowConfig`; DCR fallback in `performOAuthFlow` via `isCIMDRejectionError` (triggers on `invalid_client`/`unauthorized_client` only; `SkipCIMD=true` prevents retry loop)
`test/e2e/oidc_mock.go`	Added `client_id_metadata_document_supported: true` to mock OIDC discovery document for future E2E tests

PKCE

PKCE S256 is already enforced unconditionally for all flows in `createOAuthConfig` (discovery.go lines 659/675) — CIMD flows inherit it without additional changes.

Test plan

Unit tests: `pkg/oauthproto`, `pkg/auth/remote`, `pkg/auth/discovery` — all pass
`TestIsCIMDRejectionError` covers all RFC-specified retry/no-retry cases
E2E: pending `https://toolhive.dev/oauth/client-metadata.json\` going live (infra PR above)

Closes #4826

Generated with Claude Code

When a remote authorization server advertises client_id_metadata_document_supported in its discovery document, thv run now presents https://toolhive.dev/oauth/client-metadata.json as its client_id instead of performing a DCR round-trip. Falls back to DCR gracefully if the AS rejects the CIMD client_id. The CIMD check runs inside PerformOAuthFlow before the DCR gate so it works regardless of which issuer discovery path was taken (configured issuer, realm-derived, or resource metadata). Includes hack/mock-cimd-server for local E2E testing. Closes #4826 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

- Move cimd.go and cimd_test.go to pkg/oauthproto, update package declaration - Update imports from pkg/oauth to pkg/oauthproto in handler.go and handler_test.go - Fix CodeQL SSRF alert in mock-cimd-server: validate redirect_uri is localhost before making outbound request; use io.Discard to drain response body - Fix revive lint: unused parameter, redefined builtin min - Fix errcheck lint: handle resp.Body.Close error Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

jhrozek

Mostly RFC-0071 alignment feedback. One concern I'd flag as a blocker (the DCR fallback retry doesn't actually fall back), several spec-conformance gaps around the trigger conditions, logging, and tests called out by name in the RFC. The rest is cleanup. Happy to pair on any of this.

- Remove hack/mock-cimd-server: was added for a manual test session but has no E2E test coverage and does not belong in the final PR - Remove toolhive-client-metadata.json: the authoritative copy is in the infra repo (stacklok/infra#4549) where it gets deployed to https://toolhive.dev/oauth/client-metadata.json via CloudFront - Add client_id_metadata_document_supported: true to test/e2e/oidc_mock.go discovery document so the existing E2E mock server is CIMD-capable for future integration tests Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

- cimd.go: collapse duplicate godoc; expand IsClientIDMetadataDocumentURL comment with rationale for broad HTTPS check and Phase 2 TODO - config.go: clear CachedCIMDClientID in ClearCachedClientCredentials; note in doc comment that the restart-skip reader is deferred (#2728) - discovery.go: extract applyDiscoveryPreCheck helper to stay under cyclomatic complexity limit; populate discovered endpoints on config to prevent double-fetch on the DCR path; add SkipCIMD to OAuthFlowConfig; emit WARN when AS discovery fetch fails instead of silently falling back - handler.go: fix retry-loop bug — set SkipCIMD=true on retry so the pre-check does not re-apply the CIMD URL; narrow retry trigger to invalid_client / unauthorized_client only via isCIMDRejectionError - handler_test.go: add TestIsCIMDRejectionError covering all error cases the RFC specifies (invalid_client/unauthorized_client retry; invalid_request/other errors propagate) PKCE is already enforced unconditionally in createOAuthConfig (lines 659/675 of discovery.go) for all flows including CIMD — no change needed. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

codecov · 2026-04-28T14:43:36Z

Codecov Report

❌ Patch coverage is 53.19149% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.32%. Comparing base (f6118bc) to head (a5ddb28).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/remote/handler.go	44.44%	20 Missing ⚠️
pkg/auth/oauth/oidc.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5085      +/-   ##
==========================================
- Coverage   67.37%   67.32%   -0.06%     
==========================================
  Files         598      599       +1     
  Lines       60573    60613      +40     
==========================================
- Hits        40814    40809       -5     
- Misses      16666    16709      +43     
- Partials     3093     3095       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Move explicit priority chain (P1/P2/P3 with comments) to performOAuthFlow; remove duplicate CIMD block from buildOAuthFlowConfig so it is a pure config builder with no registration logic - Fix Priority 1 and Priority 2 in discoverIssuerAndScopes: fetch AS discovery document even when issuer is pre-configured or realm-derived, so ClientIDMetadataDocumentSupported is populated and CIMD detection works on those paths - Remove TestBuildOAuthFlowConfig_CIMD: CIMD behaviour is now tested via TestIsCIMDRejectionError and the priority chain in performOAuthFlow Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

- discovery.go: restore precise port-resolution comment explaining why order matters (DCR allows fallback, pre-registered/CIMD clients require exact port); consolidate CIMD note into the else-branch comment - handler.go: remove incorrect RFC 6749 §5.2 reference from isCIMDRejectionError — §5.2 covers token endpoint errors but CIMD rejection happens at the authorization endpoint; use plain RFC 6749 reference without a specific section Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Two test cases under Label("remote", "auth", "cimd"): 1. AS advertises CIMD support: verifies thv run presents ToolHiveClientMetadataDocumentURL as client_id, includes PKCE code_challenge, and never calls /oauth/register (DCR skipped). 2. AS does not advertise CIMD: verifies thv run falls back to DCR and does not use the CIMD metadata URL as client_id. Uses a minimal httptest-based mock AS (cimdMockAuthServer) rather than OIDCMockServer because Fosite's GetClient rejects HTTPS client IDs that are not pre-registered — CIMD requires accepting any valid HTTPS URL as client_id. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

jhrozek

Two follow-ups after the rework — both readability/robustness, neither blocking.

…ror check CachedCIMDClientID: - Re-introduce the field with a clear doc comment explaining why it is kept separate from CachedClientID (independent lifecycle — DCR credential rotation must not clear the stable CIMD URL) - Guard clientCredentialsPersister so CIMD URLs are NOT stored in CachedClientID; they go to CachedCIMDClientID instead - Implement the reader in resolveClientCredentials: CIMD URL is returned as client_id for token refresh on restart, checked before DCR credentials; this was the deferred reader Jakub flagged - ClearCachedClientCredentials does not clear CachedCIMDClientID (documented in its comment) shouldUseCIMD: - Extract the four-condition CIMD predicate into a named helper, mirroring shouldDynamicallyRegisterClient (jhrozek non-blocking) isCIMDRejectionError: - Replace strings.Contains with errors.As(*oauth2.RetrieveError) to avoid substring collisions and format coupling (jhrozek non-blocking) - Update TestIsCIMDRejectionError to use real *oauth2.RetrieveError values matching what golang.org/x/oauth2 actually returns Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

CIMD rejection surfaces from two distinct stages: - Authorization endpoint: AS redirects callback with error=invalid_client; flow.go formats this as "OAuth error: <code> - <description>" (plain error, not *oauth2.RetrieveError). - Token endpoint: *oauth2.RetrieveError with ErrorCode set. The previous errors.As-only check missed auth-endpoint rejections entirely, meaning the DCR fallback never triggered when the AS rejected the CIMD URL at the authorize step. Fix by checking errors.As first (precise, for token endpoint), then strings.HasPrefix for the auth-endpoint format. Update TestIsCIMDRejectionError to cover both error sources. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

…upport

Brings in changes from main (v0.25.0 release cycle) and regenerates the swagger documentation to match, fixing the Docs CI check.

Some authorization servers (e.g. Granola) advertise client_id_metadata_document_supported only in their RFC 8414 /.well-known/oauth-authorization-server document, not in the OIDC /.well-known/openid-configuration document. When discoverOIDCEndpointsWithClientAndValidation falls back to the OAuth AS metadata to fetch a missing registration_endpoint, it now also merges ClientIDMetadataDocumentSupported so CIMD detection works correctly for servers that split their metadata across the two well-known endpoints. Tested against Granola (https://mcp-auth.granola.ai): CIMD URL now appears as client_id in the authorize request instead of a DCR-issued opaque identifier. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

jhrozek

LGTM — thanks for the careful iteration on the rejection-error handling. Good catch that the auth-callback path needed the HasPrefix branch alongside errors.As; the structured-first, string-fallback hybrid is the right shape.

github-advanced-security AI found potential problems Apr 28, 2026

View reviewed changes

Comment thread hack/mock-cimd-server/main.go Fixed

amirejaz and others added 2 commits April 28, 2026 16:08

amirejaz force-pushed the cimd-thv-run-client-support branch from 28b5295 to 2668957 Compare April 28, 2026 11:55

github-actions Bot added the size/M Medium PR: 300-599 lines changed label Apr 28, 2026

github-advanced-security AI found potential problems Apr 28, 2026

View reviewed changes

Comment thread hack/mock-cimd-server/main.go Fixed

jhrozek reviewed Apr 28, 2026

View reviewed changes

amirejaz and others added 2 commits April 28, 2026 19:14

github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 28, 2026

amirejaz changed the title ~~Support CIMD as preferred OAuth client registration for thv run~~ WIP: Support CIMD as preferred OAuth client registration for thv run Apr 28, 2026

github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 28, 2026

github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 28, 2026

github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 28, 2026

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 29, 2026

jhrozek reviewed Apr 29, 2026

View reviewed changes

Comment thread pkg/auth/remote/handler.go Outdated

Comment thread pkg/auth/remote/handler.go Outdated

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Apr 29, 2026

amirejaz changed the title ~~WIP: Support CIMD as preferred OAuth client registration for thv run~~ Support CIMD as preferred OAuth client registration for thv run Apr 29, 2026

amirejaz marked this pull request as ready for review April 29, 2026 13:40

amirejaz requested review from ChrisJBurns, JAORMX, lujunsan, rdimitrov and yrobla as code owners April 29, 2026 13:40

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Apr 29, 2026

amirejaz mentioned this pull request Apr 29, 2026

Add follow-up CIMD E2E and unit tests #5130

Merged

amirejaz added 2 commits April 29, 2026 20:24

Merge remote-tracking branch 'origin/main' into cimd-thv-run-client-s…

660ba88

…upport

Merge main and regenerate swagger docs

279bbec

Brings in changes from main (v0.25.0 release cycle) and regenerates the swagger documentation to match, fixing the Docs CI check.

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Apr 29, 2026

amirejaz requested a review from jhrozek April 29, 2026 20:40

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Apr 29, 2026

Merge branch 'main' into cimd-thv-run-client-support

a5ddb28

github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Apr 30, 2026

jhrozek approved these changes Apr 30, 2026

View reviewed changes

amirejaz merged commit ac8fe8e into main Apr 30, 2026
48 checks passed

amirejaz deleted the cimd-thv-run-client-support branch April 30, 2026 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support CIMD as preferred OAuth client registration for thv run#5085

Support CIMD as preferred OAuth client registration for thv run#5085
amirejaz merged 13 commits into
mainfrom
cimd-thv-run-client-support

amirejaz commented Apr 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jhrozek left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 28, 2026 •

edited

Loading

Uh oh!

jhrozek left a comment

Uh oh!

Uh oh!

Uh oh!

jhrozek left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

amirejaz commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

PKCE

Test plan

Uh oh!

Uh oh!

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amirejaz commented Apr 28, 2026 •

edited

Loading

codecov Bot commented Apr 28, 2026 •

edited

Loading