Skip to content

Require explicit opt-in for legacy OAuth metadata fallback - #1952

Open
halter73 wants to merge 2 commits into
mainfrom
halter73-harden-oauth-metadata-fallback
Open

halter73 wants to merge 2 commits into
mainfrom
halter73-harden-oauth-metadata-fallback

Conversation

@halter73

@halter73 halter73 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Align main with the focused no-metadata fallback adaptation in #1949, without stacking on or importing the release/1.x backport. That PR is unchanged.

Main already includes #1700's PKCE metadata verification and #1605/#1726's issuer/state validation. This change only disables the automatic synthetic OAuth metadata fallback unless explicitly enabled:

  • When protected resource metadata is unavailable (resourceUri is null) and no authorization server metadata document is discovered, reject authentication before requesting authorization or exchanging tokens by default.
  • Restore only that original eligible fallback when the process-wide ModelContextProtocol.Authentication.AllowLegacyOAuthMetadataFallback AppContext switch is explicitly true. The switch value is not cached; using the fallback logs a warning.
  • Preserve /authorize, /token, and /register defaults and existing S256 challenge/verifier behavior. Discovered metadata that omits S256 or advertises only plain remains rejected, even when the switch is enabled; a later eligible discovery endpoint can still succeed.
  • Leave discovered-document PKCE/issuer rules, callback APIs, obsoletion, and state validation unchanged.

Compatibility tightening and escape hatch

Intentional compatibility impact: metadata-free legacy OAuth deployments that previously relied on synthetic endpoint defaults now fail by default. Deployments already authenticating through valid discovered metadata are unaffected by this incremental change. This is not a new provider compatibility rule for discovered metadata.

A trusted legacy deployment can explicitly opt in at application startup:

AppContext.SetSwitch(
    "ModelContextProtocol.Authentication.AllowLegacyOAuthMetadataFallback",
    true);

The switch is disabled when absent or false. Enabling it is not assurance of PKCE support. Hosts must independently establish that their trusted authorization server correctly enforces PKCE, including downgrade defenses. The switch affects every OAuth client in the process, not a per-server allowlist; do not enable it alongside untrusted authorization servers or toggle it around individual requests. Prefer fixing discovery and advertising S256.

The API remarks, transport guide, and protected-client sample document this tightening and escape hatch. The conformance client opts in only for its trusted auth/2025-03-26-oauth-endpoint-fallback scenario; other scenarios retain secure defaults. Existing conformance expectations remain unchanged.

Rationale

RFC 9700 §2.1.1 and §4.7.1 allow deployment-specific assurance of PKCE support, but synthesizing S256 after discovery fails is not assurance. State validation protects response correlation/CSRF; it does not prevent direct redemption of an intercepted authorization code. PKCE remains necessary, particularly for native public clients. Main also retains the obsolete code-only callback, whose behavior this PR does not change.

This is a narrow secure-default alignment, not a claim of an established exploit or an emergency affecting all existing clients.

Validation

  • dotnet build --no-restore --nologo --verbosity quiet: zero warnings/errors.
  • Focused OAuth and client conformance coverage: 170 passed on each of .NET 8, 9, and 10 with ordinary test parallelization.
  • Full non-manual ASP.NET Core test assembly after the cleanup: 666 passed, 33 skipped, zero failures on each of .NET 8, 9, and 10.
  • Before the test-only cleanup, the complete non-manual solution suite passed with 11,458 passed, 397 skipped, zero failures across .NET 8/9/10 and .NET Framework 4.7.2. Production code is unchanged by the cleanup.
  • Default-state tests retain no-metadata rejection, discovered missing/plain-only PKCE rejection (including the legacy path), alternate discovery success, the protected-resource-metadata guard, metadata-issuer checks, and no authorization/token/callback activity assertions on rejected paths. AuthTests no longer mutates AppContext or disables parallelization; redundant absent/false/true permutations and dedicated in-process opted-in success/warning assertions were removed rather than skipped.
  • The unchanged auth/2025-03-26-oauth-endpoint-fallback conformance scenario exercises successful opt-in in a subprocess. Explicit-false permutations, warning emission, and invalid-metadata rejection while opted in are no longer separately asserted in-process. No new subprocess infrastructure or changes to conformance expectations were introduced.
  • Direct execution of the unchanged legacy fallback conformance scenario confirms the compatibility boundary: default-off fails its three required OAuth activity checks; the trusted fixture opt-in passes all six checks. No expectations were changed or skipped.

Validation used pinned npm dependencies and a test-process-only current-directory PATH entry for Windows process tests; no machine settings or repository test behavior were changed.

Note

This PR and its description were generated with GitHub Copilot.

Align main with the secure-default metadata-free fallback adaptation from release/1.x without changing discovered metadata validation or callback behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
tarekgh
tarekgh previously approved these changes Oct 23, 2026

@tarekgh tarekgh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Remove in-process AppContext mutations and class-wide serialization. Retain secure-default regression coverage and existing subprocess conformance opt-in.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@tarekgh

tarekgh commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

The markdown failure will be fixed in another PR. I think we are good to merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants