Resolve todo by implementing OAuth2 metadata caching

[PrivacyIsARight]

This change resolves a todo inside src/main/oauth2/oauth2.ts This todo dates back to cd7d54e, from January 19, 2025. Given it's been here for a bit, and my previous pr fixed an issue in this file, I thought I'd give a shot at resolving this todo.

Rundown

We first start by exporting TokenResponse, and AuthorizationServerMetadata. In the original code, this is defined implicitly. I've changed this to be an export. This is better TypeScript practice and it helps to clear up the guesswork more.

Next, we add a default max age and timeout constant. It's useful, and, if it ever needs to be changed it's very easy to do. FetchId is the clever bit here, because it's a counter which prevents any late network responses from overwriting newer, more accurate data.

The next section is the fetchAuthorizationServerMetadata() function. The first thing it does is check if Date.now() is after whenever the metadata expires at. And if so, it clears it. Deduplication is also in here, which makes sure only one request is actually sent, even if multiple try to send. This prevents what's colloquially known as the "thundering herd" problem. After this, fetchId comes back to help again, preventing the old, from overwriting the new.

The next section is the invalidateAuthorizationServerMetadataCache function. This one's pretty neat because it increments fetchId even if no fetch is running, which prevents any future attempts to use old data.

After this, there's the getCacheTtlMs function. Pretty much all this does is If the server says max-age=3600, you cache it for an hour. If the server says no-cache, you don't cache it at all. And If the server says nothing, you fall back to DEFAULT_METADATA_MAX_AGE_SECONDS.

The next section is the fetchMetadata function. This one's the most important obviously, because it performs the actual HTTP request. AbortSignal.timeout with the METADATA_FETCH_TIMEOUT_MS parameter is passed here to prevent it from hanging forever.

Lastly is the renewAccessToken function changes. It's been changed so that 429 or 500s are handled differently than 400. If the server is busy, we can just retry later. Deleting the user's login tokens is unnecessary here.

There are other changes in this pr, but most aren't super noteworthy.

AI / LLM usage statement:

Numerous different LLM's were used to try and find every issue wrong with the code changes. Presumably, this results in better quality code. LLM's used were Claude Sonnet 5 Max, Claude Sonnet 5 Thinking, Claude Fable 5, Gemini 3.1 Flash-Lite, Gemini 3.5 Flash, Gemini 3.5 Flash-Extended, Gemini 3.1 Pro, Gemini 3.1 Pro-Extended. Given the nature of code like this, I found it important to do this to ensure that no serious issues appeared.

[PrivacyIsARight]

Timeout handling was missing. This diff should cover it. Important incase that ever hangs.

@@ -27,6 +27,8 @@ export interface AuthorizationServerMetadata {
 const DEFAULT_METADATA_MAX_AGE_SECONDS = 3600;
 // Maximum time to wait for a response
 const METADATA_FETCH_TIMEOUT_MS = 10000;
+// Maximum time to wait for a response from the token endpoint
+const TOKEN_FETCH_TIMEOUT_MS = 10000;

 let cachedMetadata: AuthorizationServerMetadata | null = null;
 let activeFetchPromise: Promise<AuthorizationServerMetadata> | null = null;
@@ -170,6 +172,7 @@ export async function authenticate(): Promise<TokenResponse> {
         });
         const tokenResponse = await fetch(tokenUrl, {
             method: "POST",
+            signal: AbortSignal.timeout(TOKEN_FETCH_TIMEOUT_MS),
         });
         if (tokenResponse.status !== 200) {
             const responseText = await tokenResponse.text();
@@ -219,6 +222,7 @@ export async function renewAccessToken(): Promise<TokenResponse> {
     try {
         tokenResponse = await fetch(tokenUrl, {
             method: "POST",
+            signal: AbortSignal.timeout(TOKEN_FETCH_TIMEOUT_MS),
         });
     } catch (error) {
         log.error(`Failed to renew token (network error): ${error}`);

[PrivacyIsARight]

There's a lot of edge cases here I'm working on.

@@ -36,18 +36,23 @@ let metadataExpiresAt = 0;
 let fetchId = 0;

 export function fetchAuthorizationServerMetadata(): Promise<AuthorizationServerMetadata> {
-    if (cachedMetadata && Date.now() >= metadataExpiresAt) {
-        cachedMetadata = null;
+    // If we have a cache and it hasn't expired yet, use it
+    if (cachedMetadata && Date.now() < metadataExpiresAt) {
+        return Promise.resolve(cachedMetadata);
     }

+    // If it's expired, clear the cached payload
     if (cachedMetadata) {
-        return Promise.resolve(cachedMetadata);
+        cachedMetadata = null;
+        metadataExpiresAt = 0;
     }

+    // Return active fetch if one is already in-flight
     if (activeFetchPromise) {
         return activeFetchPromise;
     }

+    // Otherwise, begin a new fetch
     const currentFetchId = ++fetchId;
     activeFetchPromise = fetchMetadata()
         .then(({ metadata, expiresAt }) => {
@@ -267,9 +272,15 @@ export async function renewAccessToken(): Promise<TokenResponse> {
 }

 let tokenRenewer: NodeJS.Timeout | undefined;
+let isRenewing = false;
+
 export function startTokenRenewer(interval: number) {
     stopTokenRenewer();
     tokenRenewer = setInterval(() => {
+        // Prevents pile-ups if the interval is short or network is slow
+        if (isRenewing) return;
+        isRenewing = true;
+
         renewAccessToken()
             .then((value) => {
                 accountService.saveRefreshToken(value.refreshToken);
@@ -278,6 +289,9 @@ export function startTokenRenewer(interval: number) {
             })
             .catch((error) => {
                 log.error(`Failed to renew access token in background loop: ${error}`);
+            })
+            .finally(() => {
+                isRenewing = false;
             });
     }, interval);
 }
@@ -285,4 +299,5 @@ export function startTokenRenewer(interval: number) {
 export function stopTokenRenewer() {
     if (!tokenRenewer) return;
     clearInterval(tokenRenewer);
+    tokenRenewer = undefined;
 }

[PrivacyIsARight]

Enough wacky edge cases that I'll leave this as draft until im confident. The goal is that it should never break, ever.

@@ -232,7 +232,10 @@ export async function renewAccessToken(): Promise<TokenResponse> {
     } catch (error) {
         log.error(`Failed to renew token (network error): ${error}`);
         invalidateAuthorizationServerMetadataCache();
-        throw error;
+
+        const errObj = error instanceof Error ? error : new Error(String(error));
+        (errObj as any).isTransient = true;
+        throw errObj;
     }

     if (tokenResponse.status !== 200) {
@@ -251,14 +254,21 @@ export async function renewAccessToken(): Promise<TokenResponse> {
         if (tokenResponse.status === 400) {
             log.error(`400 Bad request: ${tokenUrl}`);
         }
-        throw new Error(error);
+
+        const errObj = new Error(error);
+        (errObj as any).isTransient = isTransient;
+        throw errObj;
     }
+
     const body = await tokenResponse.json();
     const { access_token, refresh_token, expires_in } = body;
     if (!access_token || !expires_in) {
         const error = "Invalid OAuth2 token response";
         log.error(`${error}: ${JSON.stringify(body)}`);
-        throw new Error(error);
+
+        const errObj = new Error(error);
+        (errObj as any).isTransient = false;
+        throw errObj;
     }
     if (!refresh_token) {
         log.info("No new refresh token in token response, keeping the current one.");
@@ -289,6 +299,11 @@ export function startTokenRenewer(interval: number) {
             })
             .catch((error) => {
                 log.error(`Failed to renew access token in background loop: ${error}`);
+
+                // The errors persistent, stop any background attempts
+                if (error && (error as any).isTransient === false) {
+                    stopTokenRenewer();
+                }
             })
             .finally(() => {
                 isRenewing = false;

[PrivacyIsARight]

Type safety is important.

@@ -12,6 +12,13 @@ import { stringify } from "node:querystring";

 const log = logger("oauth2-utils");

+export class OAuth2Error extends Error {
+    constructor(message: string, public isTransient: boolean) {
+        super(message);
+        this.name = "OAuth2Error";
+    }
+}
+
 export interface TokenResponse {
     token: string;
     refreshToken: string;
@@ -233,9 +240,8 @@ export async function renewAccessToken(): Promise<TokenResponse> {
         log.error(`Failed to renew token (network error): ${error}`);
         invalidateAuthorizationServerMetadataCache();

-        const errObj = error instanceof Error ? error : new Error(String(error));
-        (errObj as any).isTransient = true;
-        throw errObj;
+        const errorMessage = error instanceof Error ? error.message : String(error);
+        throw new OAuth2Error(errorMessage, true);
     }

     if (tokenResponse.status !== 200) {
@@ -255,9 +261,7 @@ export async function renewAccessToken(): Promise<TokenResponse> {
             log.error(`400 Bad request: ${tokenUrl}`);
         }

-        const errObj = new Error(error);
-        (errObj as any).isTransient = isTransient;
-        throw errObj;
+        throw new OAuth2Error(error, isTransient);
     }

     const body = await tokenResponse.json();
@@ -266,9 +270,7 @@ export async function renewAccessToken(): Promise<TokenResponse> {
         const error = "Invalid OAuth2 token response";
         log.error(`${error}: ${JSON.stringify(body)}`);

-        const errObj = new Error(error);
-        (errObj as any).isTransient = false;
-        throw errObj;
+        throw new OAuth2Error(error, false);
     }
     if (!refresh_token) {
         log.info("No new refresh token in token response, keeping the current one.");
@@ -301,7 +303,7 @@ export function startTokenRenewer(interval: number) {
                 log.error(`Failed to renew access token in background loop: ${error}`);

                 // The errors persistent, stop any background attempts
-                if (error && (error as any).isTransient === false) {
+                if (error instanceof OAuth2Error && !error.isTransient) {
                     stopTokenRenewer();
                 }
             })

[PrivacyIsARight]

Alright. I think it's fine now.

[PrivacyIsARight]

URL spoofing protection. The original code only checked

if (!url.startsWith("https://auth.barlobby.com")) return; 

The new code checks to prevent spoofs. Regardless of this pr, this change should be made. An attacker could register "auth.barlobby.com.attacker.xyz" and that would be valid because it only checks what it starts with. @p2004a you should probably look into this.

  if (url.origin !== new URL(getOAuthAuthorizationServerURL()).origin) {
       throw new Error("Refusing to open URL that does not match the expected OAuth2 authorization server");
   }

[p2004a]

That doesn't really matter and we can drop that particular check entirely. What is kind of relevant is verifying issuer field: right now even that isn't critical for us as we use only single authorization server in client, but it's a defense in depth just in case that would somehow change in the future.