fix: harden server reliability and OAuth security

src/oauth.ts

@@ -1,12 +1,14 @@
 import { createHash, randomBytes, randomUUID } from "node:crypto";
-import { chmod, mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises";
+import { mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises";
 import path from "node:path";
 
 import type { OAuthRegisteredClientsStore } from "@modelcontextprotocol/sdk/server/auth/clients.js";
 import {
+  InvalidClientMetadataError,
   InvalidGrantError,
   InvalidScopeError,
   InvalidTargetError,
+  UnauthorizedClientError,
 } from "@modelcontextprotocol/sdk/server/auth/errors.js";
 import type {
   AuthorizationParams,
@@ -26,11 +28,12 @@ import type { AppConfig } from "./config.js";
 export const OAUTH_SCOPES = ["mcp:tools"] as const;
 
 interface StoredToken {
-  type: "access" | "refresh";
+  type: "access" | "refresh" | "used_refresh";
   clientId: string;
   scopes: string[];
   expiresAt: number;
   resource: string;
+  grantId?: string;
 }
 
 interface PersistedOAuthState {
@@ -65,18 +68,100 @@ function randomToken(): string {
   return randomBytes(32).toString("base64url");
 }
 
+const LOOPBACK_REDIRECT_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]);
+const SUPPORTED_CLIENT_AUTH_METHODS = new Set(["none", "client_secret_post"]);
+const SUPPORTED_GRANT_TYPES = new Set(["authorization_code", "refresh_token"]);
+
+function clientMetadataProblem(value: unknown): string | undefined {
+  if (!value || typeof value !== "object" || Array.isArray(value)) {
+    return "Client metadata must be an object";
+  }
+  const client = value as Partial<OAuthClientInformationFull>;
+  if (!Array.isArray(client.redirect_uris) || client.redirect_uris.length === 0) {
+    return "At least one redirect_uri is required";
+  }
+  for (const redirectUri of client.redirect_uris) {
+    if (typeof redirectUri !== "string") {
+      return "Every redirect_uri must be an absolute URL";
+    }
+    let parsed: URL;
+    try {
+      parsed = new URL(redirectUri);
+    } catch {
+      return "Every redirect_uri must be an absolute URL";
+    }
+    const isLoopback = LOOPBACK_REDIRECT_HOSTS.has(parsed.hostname);
+    if (parsed.protocol !== "https:" && !(parsed.protocol === "http:" && isLoopback)) {
+      return "Every redirect_uri must use HTTPS or HTTP on a loopback host";
+    }
+    if (parsed.hash || parsed.username || parsed.password) {
+      return "redirect_uris must not contain fragments or user credentials";
+    }
+  }
+
+  if (
+    client.token_endpoint_auth_method !== undefined &&
+    typeof client.token_endpoint_auth_method !== "string"
+  ) {
+    return "token_endpoint_auth_method must be a string";
+  }
+  const authMethod = client.token_endpoint_auth_method ?? "client_secret_post";
+  if (!SUPPORTED_CLIENT_AUTH_METHODS.has(authMethod)) {
+    return `Unsupported token_endpoint_auth_method: ${authMethod}`;
+  }
+
+  if (
+    client.grant_types !== undefined &&
+    (!Array.isArray(client.grant_types) ||
+      !client.grant_types.every((grantType) => typeof grantType === "string"))
+  ) {
+    return "grant_types must be an array of strings";
+  }
+  const grantTypes = client.grant_types ?? ["authorization_code"];
+  if (
+    grantTypes.length === 0 ||
+    !grantTypes.includes("authorization_code") ||
+    !grantTypes.every((grantType) => SUPPORTED_GRANT_TYPES.has(grantType))
+  ) {
+    return "grant_types must contain authorization_code and may contain refresh_token";
+  }
+
+  if (
+    client.response_types !== undefined &&
+    (!Array.isArray(client.response_types) ||
+      !client.response_types.every((responseType) => typeof responseType === "string"))
+  ) {
+    return "response_types must be an array of strings";
+  }
+  const responseTypes = client.response_types ?? ["code"];
+  if (responseTypes.length !== 1 || responseTypes[0] !== "code") {
+    return "Only the code response_type is supported";
+  }
+
+  if (client.scope !== undefined && typeof client.scope !== "string") {
+    return "scope must be a string";
+  }
+  const scopes = client.scope?.split(/\s+/).filter(Boolean) ?? [];
+  if (!scopes.every((scope) => OAUTH_SCOPES.includes(scope as (typeof OAUTH_SCOPES)[number]))) {
+    return "Only the mcp:tools scope is supported";
+  }
+  return undefined;
+}
+
 function isStoredToken(value: unknown): value is StoredToken {
   if (!value || typeof value !== "object") {
     return false;
   }
   const token = value as Partial<StoredToken>;
   return (
-    (token.type === "access" || token.type === "refresh") &&
+    (token.type === "access" || token.type === "refresh" || token.type === "used_refresh") &&
     typeof token.clientId === "string" &&
     Array.isArray(token.scopes) &&
     token.scopes.every((scope) => typeof scope === "string") &&
     typeof token.expiresAt === "number" &&
-    typeof token.resource === "string"
+    typeof token.resource === "string" &&
+    (token.grantId === undefined || typeof token.grantId === "string") &&
+    (token.type !== "used_refresh" || typeof token.grantId === "string")
   );
 }
 
@@ -86,8 +171,10 @@ function parseState(value: string): PersistedOAuthState {
     parsed.version !== 1 ||
     !parsed.clients ||
     typeof parsed.clients !== "object" ||
+    Array.isArray(parsed.clients) ||
     !parsed.tokens ||
     typeof parsed.tokens !== "object" ||
+    Array.isArray(parsed.tokens) ||
     !Object.values(parsed.tokens).every(isStoredToken)
   ) {
     throw new Error("Invalid OAuth state file format");
@@ -131,7 +218,6 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
   private async persist(): Promise<void> {
     const directory = path.dirname(this.stateFile);
     await mkdir(directory, { recursive: true, mode: 0o700 });
-    await chmod(directory, 0o700);
     const temporaryFile = `${this.stateFile}.${process.pid}.${randomBytes(6).toString("hex")}.tmp`;
     try {
       await writeFile(temporaryFile, `${JSON.stringify(this.state, null, 2)}\n`, {
@@ -149,10 +235,16 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
   private async mutate<T>(operation: () => T | Promise<T>): Promise<T> {
     await this.ensureLoaded();
     const pending = this.mutationQueue.then(async () => {
-      this.pruneExpired();
-      const result = await operation();
-      await this.persist();
-      return result;
+      const snapshot = structuredClone(this.state);
+      try {
+        this.pruneExpired();
+        const result = await operation();
+        await this.persist();
+        return result;
+      } catch (error) {
+        this.state = snapshot;
+        throw error;
+      }
     });
     this.mutationQueue = pending.then(
       () => undefined,
@@ -164,7 +256,11 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
   async getClient(clientId: string): Promise<OAuthClientInformationFull | undefined> {
     await this.ensureLoaded();
     await this.mutationQueue;
-    return this.state.clients[clientId];
+    const client = this.state.clients[clientId];
+    if (!client || client.client_id !== clientId || clientMetadataProblem(client)) {
+      return undefined;
+    }
+    return client;
   }
 
   async registerClient(
@@ -173,26 +269,48 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
     const supplied = client as Partial<OAuthClientInformationFull>;
     const registered: OAuthClientInformationFull = {
       ...client,
+      token_endpoint_auth_method:
+        client.token_endpoint_auth_method ?? "client_secret_post",
+      grant_types: client.grant_types ?? ["authorization_code"],
+      response_types: client.response_types ?? ["code"],
       client_id: supplied.client_id || randomUUID(),
       client_id_issued_at: supplied.client_id_issued_at || Math.floor(Date.now() / 1000),
     };
+    const problem = clientMetadataProblem(registered);
+    if (problem) {
+      throw new InvalidClientMetadataError(problem);
+    }
     return this.mutate(() => {
       this.state.clients[registered.client_id] = registered;
       return registered;
     });
   }
 
-  async issueTokenPair(clientId: string, scopes: string[], resource: string): Promise<OAuthTokens> {
-    return this.mutate(() => this.issueTokenPairWithoutPersist(clientId, scopes, resource));
+  async issueTokenPair(
+    clientId: string,
+    scopes: string[],
+    resource: string,
+    issueRefreshToken = true,
+  ): Promise<OAuthTokens> {
+    return this.mutate(() =>
+      this.issueTokenPairWithoutPersist(
+        clientId,
+        scopes,
+        resource,
+        randomUUID(),
+        issueRefreshToken,
+      ),
+    );
   }
 
   private issueTokenPairWithoutPersist(
     clientId: string,
     scopes: string[],
     resource: string,
+    grantId: string,
+    issueRefreshToken = true,
   ): OAuthTokens {
     const accessToken = randomToken();
-    const refreshToken = randomToken();
     const now = Date.now();
     this.state.tokens[tokenHash(accessToken)] = {
       type: "access",
@@ -200,21 +318,38 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
       scopes,
       expiresAt: now + this.accessTokenTtlSeconds * 1000,
       resource,
+      grantId,
     };
-    this.state.tokens[tokenHash(refreshToken)] = {
-      type: "refresh",
-      clientId,
-      scopes,
-      expiresAt: now + this.refreshTokenTtlSeconds * 1000,
-      resource,
-    };
-    return {
+    const tokens: OAuthTokens = {
       access_token: accessToken,
       token_type: "Bearer",
       expires_in: this.accessTokenTtlSeconds,
-      refresh_token: refreshToken,
       scope: scopes.join(" "),
     };
+    if (issueRefreshToken) {
+      const refreshToken = randomToken();
+      this.state.tokens[tokenHash(refreshToken)] = {
+        type: "refresh",
+        clientId,
+        scopes,
+        expiresAt: now + this.refreshTokenTtlSeconds * 1000,
+        resource,
+        grantId,
+      };
+      tokens.refresh_token = refreshToken;
+    }
+    return tokens;
+  }
+
+  private revokeGrantWithoutPersist(grantId: string, preserveReplayEvidence = false): void {
+    for (const [hash, token] of Object.entries(this.state.tokens)) {
+      if (
+        token.grantId === grantId &&
+        !(preserveReplayEvidence && token.type === "used_refresh")
+      ) {
+        delete this.state.tokens[hash];
+      }
+    }
   }
 
   async rotateRefreshToken(
@@ -228,21 +363,28 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
       const current = this.state.tokens[hash];
       if (
         !current ||
-        current.type !== "refresh" ||
         current.clientId !== clientId ||
         current.resource !== resource ||
         current.expiresAt <= Date.now()
       ) {
         return { status: "invalid" };
       }
+      if (current.type === "used_refresh") {
+        this.revokeGrantWithoutPersist(current.grantId!, true);
+        return { status: "invalid" };
+      }
+      if (current.type !== "refresh") {
+        return { status: "invalid" };
+      }
       const scopes = requestedScopes ?? current.scopes;
       if (!scopes.every((scope) => current.scopes.includes(scope))) {
         return { status: "invalid_scope" };
       }
-      delete this.state.tokens[hash];
+      const grantId = current.grantId ?? randomUUID();
+      this.state.tokens[hash] = { ...current, type: "used_refresh", grantId };
       return {
         status: "ok",
-        tokens: this.issueTokenPairWithoutPersist(clientId, scopes, resource),
+        tokens: this.issueTokenPairWithoutPersist(clientId, scopes, resource, grantId),
       };
     });
   }
@@ -254,14 +396,23 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore {
     if (!stored || stored.type !== "access" || stored.expiresAt <= Date.now()) {
       return undefined;
     }
+    const client = this.state.clients[stored.clientId];
+    if (!client || clientMetadataProblem(client)) {
+      return undefined;
+    }
     return stored;
   }
 
   async revoke(token: string, clientId: string): Promise<void> {
     await this.mutate(() => {
       const hash = tokenHash(token);
-      if (this.state.tokens[hash]?.clientId === clientId) {
-        delete this.state.tokens[hash];
+      const stored = this.state.tokens[hash];
+      if (stored?.clientId === clientId) {
+        if (stored.grantId) {
+          this.revokeGrantWithoutPersist(stored.grantId);
+        } else {
+          delete this.state.tokens[hash];
+        }
       }
     });
   }
@@ -350,7 +501,7 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
   private readonly authorizationCodes = new Map<string, AuthorizationCodeRecord>();
 
   constructor(private readonly config: AppConfig) {
-    if (!config.oauthIssuerUrl || !config.oauthResourceUrl || !config.authToken) {
+    if (!config.oauthIssuerUrl || !config.oauthResourceUrl || !config.oauthApprovalKey) {
       throw new Error("OAuth configuration is incomplete");
     }
     this.issuerUrl = new URL(config.oauthIssuerUrl);
@@ -391,9 +542,11 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
     params: AuthorizationParams,
     response: Response,
   ): Promise<void> {
+    if (!(client.grant_types ?? ["authorization_code"]).includes("authorization_code")) {
+      throw new UnauthorizedClientError("Client is not authorized for authorization_code");
+    }
     const resource = this.validateResource(params.resource);
     const scopes = this.validateScopes(params.scopes);
-    const redirectOrigin = new URL(params.redirectUri).origin;
     const request = response.req as Request;
     const accessKey =
       request.method === "POST" && typeof request.body?.access_key === "string"
@@ -402,12 +555,12 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
 
     response.set({
       "Content-Security-Policy":
-        `default-src 'none'; style-src 'unsafe-inline'; form-action 'self' ${redirectOrigin}; base-uri 'none'; frame-ancestors 'none'`,
+        "default-src 'none'; style-src 'unsafe-inline'; form-action 'self'; base-uri 'none'; frame-ancestors 'none'",
       "Referrer-Policy": "no-referrer",
       "X-Content-Type-Options": "nosniff",
     });
 
-    if (!accessKey || !tokensEqual(accessKey, this.config.authToken!)) {
+    if (!accessKey || !tokensEqual(accessKey, this.config.oauthApprovalKey!)) {
       response
         .status(accessKey ? 401 : 200)
         .type("html")
@@ -431,7 +584,7 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
     if (params.state !== undefined) {
       target.searchParams.set("state", params.state);
     }
-    response.redirect(302, target.href);
+    response.redirect(303, target.href);
   }
 
   async challengeForAuthorizationCode(
@@ -464,7 +617,19 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
       throw new InvalidGrantError("Invalid authorization code binding");
     }
     this.authorizationCodes.delete(authorizationCode);
-    return this.clientsStore.issueTokenPair(client.client_id, record.scopes, record.resource);
+    try {
+      return await this.clientsStore.issueTokenPair(
+        client.client_id,
+        record.scopes,
+        record.resource,
+        client.grant_types?.includes("refresh_token") ?? false,
+      );
+    } catch (error) {
+      if (record.expiresAt > Date.now() && !this.authorizationCodes.has(authorizationCode)) {
+        this.authorizationCodes.set(authorizationCode, record);
+      }
+      throw error;
+    }
   }
 
   async exchangeRefreshToken(
@@ -473,6 +638,9 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider {
     scopes?: string[],
     resource?: URL,
   ): Promise<OAuthTokens> {
+    if (!client.grant_types?.includes("refresh_token")) {
+      throw new UnauthorizedClientError("Client is not authorized for refresh_token");
+    }
     const resourceValue = this.validateResource(resource);
     const requestedScopes = scopes ? this.validateScopes(scopes) : undefined;
     const result = await this.clientsStore.rotateRefreshToken(