OAuth: use authorization_servers URL from resource metadata

[BobDickinson]

Summary

Fixed issue from main PR #1133 on the v1.5 branch.

Also, per comment on the PR, put fix into getAuthorizationServerUrl() which is now called from both places.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• Test updates

Changes Made

Scope discovery and metadata discovery no longer always use the MCP server URL for the OAuth authorization server. When the server returns resource metadata with authorization_servers, we use that URL (e.g. authorization_servers[0]) for both discovering OAuth metadata and discovering scopes.

Testing

• Added/updated automated tests
• Manual testing performed

Note: I did not have a test server that presented an auth server in resource metadata to test against (Keycloak, Entra ID, etc). But the PR functionality and tests match the main PR #1133.

Checklist

• Code follows the style guidelines (ran npm run prettier-fix)
• Self-review completed

Breaking Changes

No

[BobDickinson]

@claude review

[claude]

Claude finished @BobDickinson's task in 1m 19s —— View job

---

Code Review

• Gather context and read changed files
• Review core/auth/discovery.ts
• Review core/auth/state-machine.ts
• Review core/__tests__/auth/discovery.test.ts
• Compile and post final review

---

Summary: The fix is correct and well-structured. The core bug was that discoverScopes always called discoverAuthorizationServerMetadata with new URL("/", serverUrl), ignoring any authorization_servers in resource metadata. The solution extracts a shared getAuthorizationServerUrl() utility and applies it consistently in both call sites.

core/auth/discovery.ts

The new getAuthorizationServerUrl() function is clean and correct:

const first = resourceMetadata?.authorization_servers?.[0];
return first ? new URL(first) : new URL("/", serverUrl);

• Optional chaining on ?.[0] correctly returns undefined for empty arrays, falling back to serverUrl as expected.
• The truthy check means an empty string "" also falls back to serverUrl — correct, and the comment explains it.
• Accepting null in the signature (OAuthProtectedResourceMetadata | null) matches what the state machine passes after catching errors.

Minor: { fetchFn } when fetchFn is undefined passes { fetchFn: undefined } to discoverAuthorizationServerMetadata. The tests at lines 203 and 234 assert exactly this. No behavior change from the original, just worth confirming the SDK handles undefined fetchFn gracefully (it should).

core/auth/state-machine.ts

The refactoring simplifies the original defensive pattern:

// Before: verbose inline logic
let authServerUrl: URL = new URL("/", context.serverUrl);
// ...
if (resourceMetadata?.authorization_servers?.length) {
  const firstServer = resourceMetadata.authorization_servers[0];
  if (firstServer) {
    authServerUrl = new URL(firstServer);
  }
}

// After: centralized
const authServerUrl = getAuthorizationServerUrl(context.serverUrl, resourceMetadata);

The behavior is equivalent — both fall back to the MCP server origin when authorization_servers is absent, empty, or has an empty-string entry. The resourceMetadata can be null here (when resource metadata discovery fails/throws), and getAuthorizationServerUrl accepts null, so error handling is preserved correctly.

authServerUrl is now saved into state via context.updateState({ ..., authServerUrl, ... }). This is good — downstream steps could reference it if needed, and it aids debugging.

core/__tests__/auth/discovery.test.ts

Good test coverage for the new behavior. Highlights:

• ✅ Different domain auth server (https://auth-server.com/)
• ✅ Auth server with path component (Keycloak-style: https://auth-server.com/realms/my-realm/)
• ✅ Empty authorization_servers falls back to serverUrl
• ✅ fetchFn is forwarded to discoverAuthorizationServerMetadata

Suggestions:

1. Missing direct tests for getAuthorizationServerUrl: The function is only tested indirectly through discoverScopes. Since it's now exported and used in two places, consider adding a small describe("getAuthorizationServerUrl", ...) block covering: null resourceMetadata, undefined resourceMetadata, empty string as authorization_servers[0], and a valid URL. This would make regressions easier to spot. Fix this →

2. Missing test for empty string in authorization_servers[0]: The comment in getAuthorizationServerUrl says "empty string falls back to serverUrl" but there's no test exercising this specific edge case.

3. No test coverage for state-machine's use of getAuthorizationServerUrl: The metadata_discovery transition now uses the shared function, but its tests (if any exist) likely don't verify the authorization_servers URL is used for auth server metadata discovery.

Spec compliance note

Using only authorization_servers[0] is consistent with the main PR #1133 and matches the typical implementation — the spec (RFC 9728) says to use the first entry. No fallback-to-next-server logic is needed here.

Verdict

Approved with minor suggestions. The core fix is correct, the centralization is a genuine improvement, and the key test cases are covered. The suggestions above are non-blocking improvements.

PR description typo: "put fix into getAuthorizationServerUrl() which is not called from both places" — should likely be "which is now called from both places".