fix(oauth): harden OIDC discovery (timeout, retry, serve-stale)

2026-07-16 17:56:19 +02:00
parent 511f9e5195
commit cb5d754113
2 changed files with 168 additions and 21 deletions
@@ -46,7 +46,8 @@ describe('oauth/discovery', () => {
    expect(result).toEqual(VALID_METADATA);
    expect(fetch).toHaveBeenCalledTimes(1);
    expect(fetch).toHaveBeenCalledWith(
-      'https://mail.example.com/.well-known/oauth-authorization-server'
+      'https://mail.example.com/.well-known/oauth-authorization-server',
+      expect.objectContaining({ signal: expect.any(AbortSignal) }),
    );
  });

@@ -64,7 +65,8 @@ describe('oauth/discovery', () => {
    expect(fetch).toHaveBeenCalledTimes(2);
    expect(fetch).toHaveBeenNthCalledWith(
      2,
-      'https://fallback.example.com/.well-known/openid-configuration'
+      'https://fallback.example.com/.well-known/openid-configuration',
+      expect.objectContaining({ signal: expect.any(AbortSignal) }),
    );
  });

@@ -176,4 +178,83 @@ describe('oauth/discovery', () => {
    expect(second).toEqual(VALID_METADATA);
    expect(fetch).toHaveBeenCalledTimes(1);
  });
+
+  it('bounds each discovery fetch with an AbortSignal timeout (no hang on unresponsive IdP)', async () => {
+    vi.spyOn(console, 'error').mockImplementation(() => {});
+    const fetchMock = vi.fn().mockRejectedValue(
+      Object.assign(new Error('The operation timed out'), { name: 'TimeoutError' }),
+    );
+    vi.stubGlobal('fetch', fetchMock);
+
+    const result = await discoverOAuth('https://unresponsive.example.com', { validateEndpoint });
+
+    expect(result).toBeNull();
+    // Every discovery fetch must carry an AbortSignal so an unresponsive IdP is
+    // aborted (DISCOVERY_TIMEOUT_MS) instead of hanging the request - and, with
+    // it, the login page's SSO button.
+    expect(fetchMock.mock.calls.length).toBeGreaterThan(0);
+    for (const call of fetchMock.mock.calls) {
+      expect(call[1]).toEqual(expect.objectContaining({ signal: expect.any(AbortSignal) }));
+    }
+  });
+
+  it('retries once when the first attempt fails, then succeeds', async () => {
+    // Attempt 1: both well-known URLs fail. Attempt 2: first URL succeeds.
+    vi.stubGlobal('fetch', vi.fn()
+      .mockResolvedValueOnce({ ok: false, status: 503 })
+      .mockResolvedValueOnce({ ok: false, status: 503 })
+      .mockResolvedValueOnce({ ok: true, json: () => Promise.resolve(VALID_METADATA) }));
+
+    const result = await discoverOAuth('https://flaky.example.com', { validateEndpoint });
+
+    expect(result).toEqual(VALID_METADATA);
+    // 2 failures (attempt 1) + 1 success (attempt 2 retry).
+    expect(fetch).toHaveBeenCalledTimes(3);
+  });
+
+  it('serves stale cached metadata when a refresh fails (keeps the SSO button up)', async () => {
+    vi.useFakeTimers();
+    try {
+      const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
+
+      // First call succeeds and caches the metadata.
+      vi.stubGlobal('fetch', vi.fn().mockResolvedValueOnce({
+        ok: true,
+        json: () => Promise.resolve(VALID_METADATA),
+      }));
+      const first = await discoverOAuth('https://stale.example.com', { validateEndpoint });
+      expect(first).toEqual(VALID_METADATA);
+
+      // Expire the cache (positive TTL is 10 min).
+      vi.advanceTimersByTime(10 * 60 * 1000 + 1);
+
+      // Refresh now fails on every URL/attempt: the stale-but-usable value must
+      // be returned instead of null so the SSO button keeps rendering.
+      vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('network down')));
+      const pending = discoverOAuth('https://stale.example.com', { validateEndpoint });
+      await vi.advanceTimersByTimeAsync(1000); // fire the retry backoff timer
+      const second = await pending;
+
+      expect(second).toEqual(VALID_METADATA);
+      expect(warnSpy).toHaveBeenCalled();
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  it('negative-caches a total failure (no cached value) to avoid hammering the IdP', async () => {
+    vi.spyOn(console, 'error').mockImplementation(() => {});
+    const fetchMock = vi.fn().mockRejectedValue(new Error('network down'));
+    vi.stubGlobal('fetch', fetchMock);
+
+    const first = await discoverOAuth('https://down.example.com', { validateEndpoint });
+    const callsAfterFirst = fetchMock.mock.calls.length;
+    const second = await discoverOAuth('https://down.example.com', { validateEndpoint });
+
+    expect(first).toBeNull();
+    expect(second).toBeNull();
+    // The immediate second call is short-circuited by the negative cache, so no
+    // additional fetches are made.
+    expect(fetchMock).toHaveBeenCalledTimes(callsAfterFirst);
+  });
 });
@@ -21,6 +21,26 @@ const CACHE_TTL_MS = 10 * 60 * 1000;
 const CACHE_MAX_ENTRIES = 64;
 const metadataCache = new Map<string, { metadata: OAuthMetadata; expiresAt: number }>();

+// --- Discovery hardening ---------------------------------------------------
+// The login page's "Sign in with SSO" button is gated on OIDC discovery
+// succeeding. An un-timed, un-retried fetch that dropped its cached value on
+// failure let a single transient blip to the IdP silently hide the button.
+// A per-fetch timeout, one retry, and serving stale-but-usable metadata on
+// failure keep the button up through a transient blip.
+const DISCOVERY_TIMEOUT_MS = 4000;
+const DISCOVERY_RETRIES = 1;
+const DISCOVERY_RETRY_DELAY_MS = 300;
+// When discovery fails, remember the outcome briefly so repeated login-page
+// loads during an outage don't hammer the IdP. Also throttles re-discovery
+// while serving stale metadata. Kept short so recovery is fast.
+const DISCOVERY_FAILURE_TTL_MS = 15 * 1000;
+
+// Records recent failures for serverUrls that have no cached metadata to serve.
+const negativeCache = new Map<string, number>();
+
+const sleep = (ms: number): Promise<void> =>
+  new Promise((resolve) => setTimeout(resolve, ms));
+
 function rememberMetadata(serverUrl: string, metadata: OAuthMetadata): void {
  // Bound the cache so callers that can supply arbitrary serverUrl values
  // (e.g. unauthenticated routes that fall back to user input) cannot
@@ -33,6 +53,16 @@ function rememberMetadata(serverUrl: string, metadata: OAuthMetadata): void {
  metadataCache.set(serverUrl, { metadata, expiresAt: Date.now() + CACHE_TTL_MS });
 }

+function rememberFailure(serverUrl: string): void {
+  // Bound like metadataCache: a user-supplied serverUrl must not grow this map
+  // without limit.
+  if (negativeCache.size >= CACHE_MAX_ENTRIES) {
+    const oldest = negativeCache.keys().next().value;
+    if (oldest !== undefined) negativeCache.delete(oldest);
+  }
+  negativeCache.set(serverUrl, Date.now() + DISCOVERY_FAILURE_TTL_MS);
+}
+
 // Endpoints come from an attacker-controllable JSON document when callers pass
 // a user-supplied serverUrl (e.g. /api/auth/totp-token-exchange under
 // allowCustomJmapEndpoint). Without a validator, a malicious metadata document
@@ -52,24 +82,18 @@ async function endpointsArePublic(
  return true;
 }

-export async function discoverOAuth(
-  serverUrl: string,
-  options?: DiscoverOAuthOptions,
+// One pass over the well-known documents. Returns usable metadata, or null
+// (pushing diagnostics into `errors`) when neither URL yields a public,
+// complete document. Each fetch is bounded by a timeout so an unresponsive IdP
+// can never hang the request (and, with it, the login page's SSO button).
+async function attemptDiscovery(
+  urls: string[],
+  validate: EndpointValidator | undefined,
+  errors: string[],
 ): Promise<OAuthMetadata | null> {
-  const cached = metadataCache.get(serverUrl);
-  if (cached && cached.expiresAt > Date.now()) return cached.metadata;
-  if (cached) metadataCache.delete(serverUrl);
-
-  const urls = [
-    `${serverUrl}/.well-known/oauth-authorization-server`,
-    `${serverUrl}/.well-known/openid-configuration`,
-  ];
-
-  const errors: string[] = [];
-
  for (const url of urls) {
    try {
-      const response = await fetch(url);
+      const response = await fetch(url, { signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS) });
      if (!response.ok) {
        errors.push(`${url} returned HTTP ${response.status}`);
        continue;
@@ -82,20 +106,18 @@ export async function discoverOAuth(
          data.token_endpoint,
          data.revocation_endpoint,
          data.end_session_endpoint,
-        ], options?.validateEndpoint);
+        ], validate);
        if (!allPublic) {
          errors.push(`${url} returned non-public or invalid endpoint URL`);
          continue;
        }
-        const metadata: OAuthMetadata = {
+        return {
          issuer: data.issuer,
          authorization_endpoint: data.authorization_endpoint,
          token_endpoint: data.token_endpoint,
          revocation_endpoint: data.revocation_endpoint,
          end_session_endpoint: data.end_session_endpoint,
        };
-        rememberMetadata(serverUrl, metadata);
-        return metadata;
      }
      errors.push(`${url} response missing required endpoints`);
    } catch (err) {
@@ -103,7 +125,51 @@ export async function discoverOAuth(
      continue;
    }
  }
+  return null;
+}

+export async function discoverOAuth(
+  serverUrl: string,
+  options?: DiscoverOAuthOptions,
+): Promise<OAuthMetadata | null> {
+  const cached = metadataCache.get(serverUrl);
+  if (cached && cached.expiresAt > Date.now()) return cached.metadata;
+  // A stale entry is deliberately retained (not deleted) so it can be served
+  // as a fallback below if the refresh fails - this keeps the SSO button up
+  // through a transient IdP blip.
+
+  // Nothing to serve and we failed recently: skip hammering the IdP.
+  if (!cached) {
+    const retryAfter = negativeCache.get(serverUrl);
+    if (retryAfter !== undefined && retryAfter > Date.now()) return null;
+  }
+
+  const urls = [
+    `${serverUrl}/.well-known/oauth-authorization-server`,
+    `${serverUrl}/.well-known/openid-configuration`,
+  ];
+
+  const errors: string[] = [];
+  for (let attempt = 0; attempt <= DISCOVERY_RETRIES; attempt++) {
+    if (attempt > 0) await sleep(DISCOVERY_RETRY_DELAY_MS);
+    const metadata = await attemptDiscovery(urls, options?.validateEndpoint, errors);
+    if (metadata) {
+      rememberMetadata(serverUrl, metadata);
+      negativeCache.delete(serverUrl);
+      return metadata;
+    }
+  }
+
+  // Every attempt failed. Prefer stale-but-usable metadata over nothing so the
+  // login page keeps rendering the SSO button during the outage; throttle the
+  // next re-discovery so we don't retry on every request.
+  if (cached) {
+    cached.expiresAt = Date.now() + DISCOVERY_FAILURE_TTL_MS;
+    console.warn(`[OAuth] Discovery refresh failed for ${serverUrl}; serving stale metadata: ${errors.join('; ')}`);
+    return cached.metadata;
+  }
+
+  rememberFailure(serverUrl);
  console.error(`[OAuth] Discovery failed for ${serverUrl}: ${errors.join('; ')}`);
  return null;
 }