Skip to content

Fix DCR failure for authorization servers with non-root issuer paths#5357

Merged
amirejaz merged 2 commits into
mainfrom
worktree-fix-dcr-non-root-issuer
May 21, 2026
Merged

Fix DCR failure for authorization servers with non-root issuer paths#5357
amirejaz merged 2 commits into
mainfrom
worktree-fix-dcr-non-root-issuer

Conversation

@amirejaz
Copy link
Copy Markdown
Contributor

Summary

Authorization servers whose issuer has a non-root path (e.g. https://example.com/oauth) may serve their RFC 8414 metadata exclusively at the path-insertion URL (scheme://host/.well-known/oauth-authorization-server/path), not at the OIDC issuer-suffix URL ({issuer}/.well-known/openid-configuration). After upgrading to v0.28.0, thv run would fail for these servers before opening the browser — the workload never entered running state.

  • PR Migrate CLI OAuth flow to pkg/auth/dcr resolver #5250 introduced a re-fetch of AS metadata inside resolveDCRCredentials to get code_challenge_methods_supported for the S256 PKCE gate. It constructed a single OIDC issuer-suffix URL for that fetch and passed it to FetchAuthorizationServerMetadataFromURL, which fetches exactly one URL with no fallback. Servers that only serve the RFC 8414 path-insertion URL returned HTML or a 404, producing the unexpected content-type "text/html" error from the issue.
  • The pre-existing v0.27.x code (discoverOIDCEndpointsWithClientAndValidation) tried both URL forms and fell back gracefully; that fallback was lost in the migration.
  • Fix routes the re-fetch through oauthproto.FetchAuthorizationServerMetadata, which tries all three well-known URL forms in priority order (RFC 8414 path-insertion → OIDC issuer-suffix → bare RFC 8414). The fetched code_challenge_methods_supported is forwarded to the resolver via a new dcr.Request.CodeChallengeMethodsSupported field so the S256 PKCE gate fires without a second round-trip inside the resolver.

Fixes #5356.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

New and updated tests:

  • TestHandleDynamicRegistration_NonRootIssuerRFC8414PathInsertion — regression test: mounts metadata only at /.well-known/oauth-authorization-server/oauth, verifies DCR succeeds end-to-end (would have failed before the fix).
  • TestHandleDynamicRegistration_PreDiscoveredPathNonRootIssuer — same scenario on the pre-discovered path (endpoints already in OAuthFlowConfig), which is the more common production case.
  • TestHandleDynamicRegistration_SynthesisesEndpointWhenMetadataOmitsIt — verifies the ErrRegistrationEndpointMissing synthesis branch in resolveDCRCredentials routes to {issuer}/register.
  • TestResolveDCRCredentials_CodeChallengeMethodsSupportedFieldEnablesPublicClient — pins the new dcr.Request.CodeChallengeMethodsSupported field: S256 present allows public client, plain-only or absent rejects it, absent is fine for confidential clients.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change
pkg/auth/dcr/request.go Add CodeChallengeMethodsSupported []string to Request; active on the RegistrationEndpoint branch so callers can supply already-discovered capability fields.
pkg/auth/dcr/resolver.go Thread req.CodeChallengeMethodsSupported into dcrEndpoints on the RegistrationEndpoint branch so the S256 gate fires from the caller-supplied field.
pkg/auth/discovery/discovery.go Replace the single OIDC URL construction in resolveDCRCredentials with oauthproto.FetchAuthorizationServerMetadata (multi-URL fallback); forward CodeChallengeMethodsSupported to the resolver. Update function doc.
pkg/auth/discovery/dcr_resolver_test.go Add three tests (regression, pre-discovered path, endpoint synthesis).
pkg/auth/dcr/resolver_test.go Add table test for CodeChallengeMethodsSupported field on RegistrationEndpoint branch.

Does this introduce a user-facing change?

DCR now succeeds for authorization servers with non-root issuer paths (e.g. https://example.com/oauth) that serve RFC 8414 metadata at the path-insertion URL. These servers were broken in v0.28.0; this restores v0.27.x behaviour.

Special notes for reviewers

The architectural root cause (#5250 doc comment, "known limitation of the option (b) migration") is that code_challenge_methods_supported is not threaded through AuthServerInfo to the CLI call site. That follow-up (threading the field through ValidateAndDiscoverAuthServer and all its callers) would eliminate the re-fetch entirely on the pre-discovered path and remains open. This PR takes the minimal targeted fix: use the multi-URL fallback for the re-fetch and forward the result via the new field, without touching AuthServerInfo or its callers.

Generated with Claude Code

amirejaz and others added 2 commits May 21, 2026 15:21
resolveDCRCredentials constructed a single OIDC issuer-suffix URL
({issuer}/.well-known/openid-configuration) and passed it to the DCR
resolver, which fetched exactly that URL via
FetchAuthorizationServerMetadataFromURL. Authorization servers that
serve their RFC 8414 metadata exclusively at the path-insertion URL
(scheme://host/.well-known/oauth-authorization-server/{path}) — such as
Gleean's AS with issuer https://example.com/oauth — received a 404 or
HTML response and the CLI failed with "unexpected content-type text/html"
before opening the browser.

Replace the single-URL construction with oauthproto.FetchAuthorizationServerMetadata,
which tries the three well-known URL forms in priority order (RFC 8414
path-insertion, OIDC issuer-suffix, bare RFC 8414), restoring the
fallback behaviour that existed in v0.27.x via
discoverOIDCEndpointsWithClientAndValidation. The fetched
code_challenge_methods_supported is forwarded to the resolver through a
new dcr.Request.CodeChallengeMethodsSupported field, so the S256 PKCE
gate fires without a second discovery round-trip inside the resolver.

Regression test added: TestHandleDynamicRegistration_NonRootIssuerRFC8414PathInsertion
mounts metadata only at /.well-known/oauth-authorization-server/oauth
and verifies DCR succeeds end-to-end.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Three gaps in the test suite left by the #5356 fix:

1. pkg/auth/dcr: TestResolveDCRCredentials_CodeChallengeMethodsSupportedFieldEnablesPublicClient
   pins the new Request.CodeChallengeMethodsSupported field on the
   RegistrationEndpoint-direct branch. Without a DiscoveryURL the S256
   gate has no metadata to read from; the field is the only input.
   Four cases: S256 allows public client, plain rejects, absent rejects,
   absent is fine for confidential clients.

2. pkg/auth/discovery: TestHandleDynamicRegistration_PreDiscoveredPathNonRootIssuer
   covers the pre-discovered path (config.RegistrationEndpoint/AuthorizeURL/
   TokenURL all pre-set) with a non-root issuer. getDiscoveryDocument
   short-circuits on this path and returns a synthesised document with
   empty code_challenge_methods_supported; the multi-URL re-fetch in
   resolveDCRCredentials must still reach the path-insertion URL correctly.

3. pkg/auth/discovery: TestHandleDynamicRegistration_SynthesisesEndpointWhenMetadataOmitsIt
   verifies that when FetchAuthorizationServerMetadata returns
   ErrRegistrationEndpointMissing (valid metadata, no registration_endpoint),
   resolveDCRCredentials synthesises {issuer}/register and routes the
   registration there — the nanobot/Hydra convention also applied by
   the resolver's DiscoveryURL branch.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label May 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.40%. Comparing base (9a5d0c2) to head (db32247).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/auth/discovery/discovery.go 64.70% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5357      +/-   ##
==========================================
+ Coverage   68.36%   68.40%   +0.03%     
==========================================
  Files         622      624       +2     
  Lines       63371    63459      +88     
==========================================
+ Hits        43326    43410      +84     
  Misses      16809    16809              
- Partials     3236     3240       +4     

☔ 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.

Copy link
Copy Markdown
Contributor

@tgrunnagle tgrunnagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix correctly addresses #5356 — the multi-URL fallback restores v0.27.x behavior and the regression test mounts metadata only at the RFC 8414 path-insertion URL, which is exactly the failure scenario from the issue. Doc comments are updated cleanly and the deferred architectural follow-up (threading capability fields through AuthServerInfo) is appropriately scoped out. One non-blocking note inline about synthesis duplication.

Comment thread pkg/auth/discovery/discovery.go
@amirejaz amirejaz merged commit 4c2e32c into main May 21, 2026
76 of 77 checks passed
@amirejaz amirejaz deleted the worktree-fix-dcr-non-root-issuer branch May 21, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v0.28.0 regression: DCR fails for issuers with non-root paths

2 participants