refactor(auth): align authMiddleware with SDK AuthInfo and fix httpTransport type issue

cyanheads · cyanheads · commit 10013c68bc8d · 2025-05-25T20:11:47.000-07:00
- Refactors `authMiddleware.ts` to conform to the MCP SDK's `AuthInfo` type.
  This involves updated JWT claim handling for `clientId` (from `cid` or `client_id`) and `scopes` (from `scp` or `scope`).
  Missing `clientId` is now an error, and invalid/missing `scopes` default to an empty array.
  Logging and JSDoc have also been improved.
- Adds a workaround in `httpTransport.ts` to sanitize `req.auth` for SDK compatibility by setting it to undefined if it's a string or devMode object, addressing potential type mismatches when passing to the SDK's `transport.handleRequest`.
diff --git a/src/mcp-server/transports/authentication/authMiddleware.ts b/src/mcp-server/transports/authentication/authMiddleware.ts
@@ -1,163 +1,230 @@
 /**
- * MCP Authentication Middleware: Bearer Token Validation (JWT).
+ * @fileoverview MCP Authentication Middleware for Bearer Token Validation (JWT).
  *
  * This middleware validates JSON Web Tokens (JWT) passed via the 'Authorization' header
  * using the 'Bearer' scheme (e.g., "Authorization: Bearer <your_token>").
  * It verifies the token's signature and expiration using the secret key defined
- * in the configuration (MCP_AUTH_SECRET_KEY).
+ * in the configuration (`config.mcpAuthSecretKey`).
  *
- * If the token is valid, the decoded payload is attached to `req.auth` for potential
- * use in downstream authorization logic (e.g., checking scopes or permissions).
+ * If the token is valid, an object conforming to the MCP SDK's `AuthInfo` type
+ * (expected to contain `token`, `clientId`, and `scopes`) is attached to `req.auth`.
  * If the token is missing, invalid, or expired, it sends an HTTP 401 Unauthorized response.
  *
- * --- Scope and Relation to MCP Authorization Spec (2025-03-26) ---
- * - This middleware handles the *validation* of an already obtained Bearer token,
- *   as required by Section 2.6 of the MCP Auth Spec.
- * - It does *NOT* implement the full OAuth 2.1 authorization flows (e.g., Authorization
- *   Code Grant with PKCE), token endpoints (/token), authorization endpoints (/authorize),
- *   metadata discovery (/.well-known/oauth-authorization-server), or dynamic client
- *   registration (/register) described in the specification. It assumes the client
- *   obtained the JWT through an external process compliant with the spec or another
- *   agreed-upon mechanism.
- * - It correctly returns HTTP 401 errors for invalid/missing tokens as per Section 2.8.
- *
- * --- Implementation Details & Requirements ---
- * - Requires the 'jsonwebtoken' package (`npm install jsonwebtoken @types/jsonwebtoken`).
- * - The `MCP_AUTH_SECRET_KEY` environment variable MUST be set to a strong, secret value
- *   in production. The middleware includes a startup check for this.
- * - In non-production environments, if the secret key is missing, authentication checks
- *   are bypassed for development convenience (a warning is logged). THIS IS INSECURE FOR PRODUCTION.
- * - The structure of the JWT payload (e.g., containing user ID, scopes) depends on the
- *   token issuer and is not dictated by this middleware itself, but the payload is made
- *   available on `req.auth`.
- *
  * @see {@link https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/docs/specification/2025-03-26/basic/authorization.mdx | MCP Authorization Specification}
+ * @module src/mcp-server/transports/authentication/authMiddleware
  */
 
-import { NextFunction, Request, Response } from 'express';
-import jwt from 'jsonwebtoken';
-// Import config, environment constants, and logger
-import { config, environment } from '../../../config/index.js';
-import { logger } from '../../../utils/index.js';
+import { NextFunction, Request, Response } from "express";
+import jwt from "jsonwebtoken";
+import { AuthInfo } from "@modelcontextprotocol/sdk/server/auth/types.js"; // Import from SDK
+import { config, environment } from "../../../config/index.js";
+import { logger, requestContextService } from "../../../utils/index.js";
 
 // Extend the Express Request interface to include the optional 'auth' property
-// This allows attaching the decoded JWT payload to the request object.
+// using the imported AuthInfo type from the SDK.
 declare global {
   // eslint-disable-next-line @typescript-eslint/no-namespace
   namespace Express {
     interface Request {
-      /** Decoded JWT payload if authentication is successful, or a development mode indicator. */
-      auth?: jwt.JwtPayload | string | { devMode: boolean; warning: string };
+      /** Authentication information derived from the JWT, conforming to MCP SDK's AuthInfo. */
+      auth?: AuthInfo;
     }
   }
 }
 
-// --- Startup Validation ---
-// Validate secret key presence on module load (fail fast principle).
-// This prevents the server starting insecurely in production without the key.
-if (environment === 'production' && !config.security.mcpAuthSecretKey) {
-  logger.fatal('CRITICAL: MCP_AUTH_SECRET_KEY is not set in production environment. Authentication cannot proceed securely.');
-  // Throwing an error here will typically stop the Node.js process.
-  throw new Error('MCP_AUTH_SECRET_KEY must be set in production environment for JWT authentication.');
+// Startup Validation: Validate secret key presence on module load.
+if (environment === "production" && !config.security.mcpAuthSecretKey) {
+  logger.fatal(
+    "CRITICAL: MCP_AUTH_SECRET_KEY is not set in production environment. Authentication cannot proceed securely.",
+  );
+  throw new Error(
+    "MCP_AUTH_SECRET_KEY must be set in production environment for JWT authentication.",
+  );
 } else if (!config.security.mcpAuthSecretKey) {
-    // Log a clear warning if running without a key in non-production environments.
-    logger.warning('MCP_AUTH_SECRET_KEY is not set. Authentication middleware will bypass checks (DEVELOPMENT ONLY). This is insecure for production.');
+  logger.warning(
+    "MCP_AUTH_SECRET_KEY is not set. Authentication middleware will bypass checks (DEVELOPMENT ONLY). This is insecure for production.",
+  );
 }
 
 /**
- * Express middleware function for verifying JWT Bearer token authentication.
- * Checks the `Authorization` header, verifies the token, and attaches the payload to `req.auth`.
- *
- * @param {Request} req - Express request object.
- * @param {Response} res - Express response object.
- * @param {NextFunction} next - Express next middleware function.
+ * Express middleware for verifying JWT Bearer token authentication.
  */
-export function mcpAuthMiddleware(req: Request, res: Response, next: NextFunction): void {
-  // Establish context for logging associated with this middleware execution.
-  const context = { operation: 'mcpAuthMiddleware', method: req.method, path: req.path };
-  logger.debug('Running MCP Authentication Middleware (Bearer Token Validation)...', context);
+export function mcpAuthMiddleware(
+  req: Request,
+  res: Response,
+  next: NextFunction,
+): void {
+  const context = requestContextService.createRequestContext({
+    operation: "mcpAuthMiddleware",
+    method: req.method,
+    path: req.path,
+  });
+  logger.debug(
+    "Running MCP Authentication Middleware (Bearer Token Validation)...",
+    context,
+  );
 
-  // --- Development Mode Bypass ---
-  // If the secret key is missing (and not in production), bypass authentication.
+  // Development Mode Bypass
   if (!config.security.mcpAuthSecretKey) {
-    // Double-check environment for safety, although the startup check should prevent this in prod.
-    if (environment !== 'production') {
-      logger.warning('Bypassing JWT authentication: MCP_AUTH_SECRET_KEY is not set (DEVELOPMENT ONLY).', context);
-      // Attach a dummy auth object to indicate bypass for potential downstream checks.
-      req.auth = { devMode: true, warning: 'Auth bypassed due to missing secret key' };
-      return next(); // Proceed without authentication.
+    if (environment !== "production") {
+      logger.warning(
+        "Bypassing JWT authentication: MCP_AUTH_SECRET_KEY is not set (DEVELOPMENT ONLY).",
+        context,
+      );
+      // Populate req.auth strictly according to SDK's AuthInfo
+      req.auth = {
+        token: "dev-mode-placeholder-token",
+        clientId: "dev-client-id",
+        scopes: ["dev-scope"],
+      };
+      // Log dev mode details separately, not attaching to req.auth if not part of AuthInfo
+      logger.debug("Dev mode auth object created.", {
+        ...context,
+        authDetails: req.auth,
+      });
+      return next();
     } else {
-      // Defensive coding: Should be caught by startup check, but handle anyway.
-      logger.error('FATAL: MCP_AUTH_SECRET_KEY is missing in production. Cannot bypass auth.', context);
-      // Send a server error response as this indicates a critical configuration issue.
-      res.status(500).json({ error: 'Server configuration error: Authentication key missing.' });
-      return; // Halt processing.
+      logger.error(
+        "FATAL: MCP_AUTH_SECRET_KEY is missing in production. Cannot bypass auth.",
+        context,
+      );
+      res.status(500).json({
+        error: "Server configuration error: Authentication key missing.",
+      });
+      return;
     }
   }
 
-  // --- Standard JWT Bearer Token Verification ---
   const authHeader = req.headers.authorization;
-  logger.debug(`Authorization header present: ${!!authHeader}`, context);
-
-  // Check for the presence and correct format ('Bearer <token>') of the Authorization header.
-  if (!authHeader || !authHeader.startsWith('Bearer ')) {
-    logger.warning('Authentication failed: Missing or malformed Authorization header (Bearer scheme required).', context);
-    // Respond with 401 Unauthorized as per RFC 6750 (Bearer Token Usage).
-    res.status(401).json({ error: 'Unauthorized: Missing or invalid authentication token format.' });
-    return; // Halt processing.
+  if (!authHeader || !authHeader.startsWith("Bearer ")) {
+    logger.warning(
+      "Authentication failed: Missing or malformed Authorization header (Bearer scheme required).",
+      context,
+    );
+    res.status(401).json({
+      error: "Unauthorized: Missing or invalid authentication token format.",
+    });
+    return;
   }
 
-  // Extract the token part from the "Bearer <token>" string.
-  const token = authHeader.split(' ')[1];
-  // Avoid logging the token itself for security reasons.
-  logger.debug('Extracted token from Bearer header.', context);
-
-  // Check if a token was actually present after the split.
-  if (!token) {
-    logger.warning('Authentication failed: Token missing after Bearer split (Malformed header).', context);
-    res.status(401).json({ error: 'Unauthorized: Malformed authentication token.' });
-    return; // Halt processing.
+  const tokenParts = authHeader.split(" ");
+  if (tokenParts.length !== 2 || tokenParts[0] !== "Bearer" || !tokenParts[1]) {
+    logger.warning("Authentication failed: Malformed Bearer token.", context);
+    res
+      .status(401)
+      .json({ error: "Unauthorized: Malformed authentication token." });
+    return;
   }
+  const rawToken = tokenParts[1];
 
   try {
-    // Verify the token's signature and expiration using the configured secret key.
-    // `jwt.verify` throws errors for invalid signature, expiration, etc.
-    const decoded = jwt.verify(token, config.security.mcpAuthSecretKey);
-    // Avoid logging the decoded payload directly unless necessary for specific debugging,
-    // as it might contain sensitive information.
-    logger.debug('JWT verified successfully.', { ...context });
+    const decoded = jwt.verify(rawToken, config.security.mcpAuthSecretKey);
+
+    if (typeof decoded === "string") {
+      logger.warning(
+        "Authentication failed: JWT decoded to a string, expected an object payload.",
+        context,
+      );
+      res
+        .status(401)
+        .json({ error: "Unauthorized: Invalid token payload format." });
+      return;
+    }
+
+    // Extract and validate fields for SDK's AuthInfo
+    const clientIdFromToken =
+      typeof decoded.cid === "string"
+        ? decoded.cid
+        : typeof decoded.client_id === "string"
+          ? decoded.client_id
+          : undefined;
+    if (!clientIdFromToken) {
+      logger.warning(
+        "Authentication failed: JWT 'cid' or 'client_id' claim is missing or not a string.",
+        { ...context, jwtPayloadKeys: Object.keys(decoded) },
+      );
+      res.status(401).json({
+        error: "Unauthorized: Invalid token, missing client identifier.",
+      });
+      return;
+    }
+
+    let scopesFromToken: string[];
+    if (
+      Array.isArray(decoded.scp) &&
+      decoded.scp.every((s) => typeof s === "string")
+    ) {
+      scopesFromToken = decoded.scp as string[];
+    } else if (
+      typeof decoded.scope === "string" &&
+      decoded.scope.trim() !== ""
+    ) {
+      scopesFromToken = decoded.scope.split(" ").filter((s) => s);
+      if (scopesFromToken.length === 0 && decoded.scope.trim() !== "") {
+        // handles case " " -> [""]
+        scopesFromToken = [decoded.scope.trim()];
+      } else if (scopesFromToken.length === 0 && decoded.scope.trim() === "") {
+        // If scope is an empty string, treat as no scopes rather than erroring, or use a default.
+        // Depending on strictness, could also error here. For now, allow empty array if scope was empty string.
+        logger.debug(
+          "JWT 'scope' claim was an empty string, resulting in empty scopes array.",
+          context,
+        );
+      }
+    } else {
+      // If scopes are strictly mandatory and not found or invalid format
+      logger.warning(
+        "Authentication failed: JWT 'scp' or 'scope' claim is missing, not an array of strings, or not a valid space-separated string. Assigning default empty array.",
+        { ...context, jwtPayloadKeys: Object.keys(decoded) },
+      );
+      scopesFromToken = []; // Default to empty array if scopes are mandatory but not found/invalid
+      // Or, if truly mandatory and must be non-empty:
+      // res.status(401).json({ error: "Unauthorized: Invalid token, missing or invalid scopes." });
+      // return;
+    }
 
-    // Attach the decoded payload (which can be an object or string based on JWT content)
-    // to the request object (`req.auth`) for use in subsequent middleware or route handlers
-    // (e.g., for fine-grained authorization checks based on payload claims like user ID or scopes).
-    req.auth = decoded;
+    // Construct req.auth with only the properties defined in SDK's AuthInfo
+    // All other claims from 'decoded' are not part of req.auth for type safety.
+    req.auth = {
+      token: rawToken,
+      clientId: clientIdFromToken,
+      scopes: scopesFromToken,
+    };
 
-    // Authentication successful, proceed to the next middleware or the main route handler.
+    // Log separately if other JWT claims like 'sub' (sessionId) are needed for app logic
+    const subClaimForLogging =
+      typeof decoded.sub === "string" ? decoded.sub : undefined;
+    logger.debug("JWT verified successfully. AuthInfo attached to request.", {
+      ...context,
+      mcpSessionIdContext: subClaimForLogging,
+      clientId: req.auth.clientId,
+      scopes: req.auth.scopes,
+    });
     next();
   } catch (error: unknown) {
-    // Handle errors thrown by `jwt.verify`.
-    let errorMessage = 'Invalid token'; // Default error message.
+    let errorMessage = "Invalid token";
     if (error instanceof jwt.TokenExpiredError) {
-      // Specific error for expired tokens.
-      errorMessage = 'Token expired';
-      // After instanceof check, 'error' is typed as TokenExpiredError
-      logger.warning('Authentication failed: Token expired.', { ...context, expiredAt: error.expiredAt }); // Log specific details here
+      errorMessage = "Token expired";
+      logger.warning("Authentication failed: Token expired.", {
+        ...context,
+        expiredAt: error.expiredAt,
+      });
     } else if (error instanceof jwt.JsonWebTokenError) {
-      // General JWT errors (e.g., invalid signature, malformed token).
-      // After instanceof check, 'error' is typed as JsonWebTokenError
-      errorMessage = `Invalid token: ${error.message}`; // Include specific JWT error message
-      logger.warning(`Authentication failed: ${errorMessage}`, { ...context }); // Log specific details here
+      errorMessage = `Invalid token: ${error.message}`;
+      logger.warning(`Authentication failed: ${errorMessage}`, { ...context });
     } else if (error instanceof Error) {
-        // Handle other standard JavaScript errors
-        errorMessage = `Verification error: ${error.message}`;
-        logger.error('Authentication failed: Unexpected error during token verification.', { ...context, error: error.message }); // Log specific details here
+      errorMessage = `Verification error: ${error.message}`;
+      logger.error(
+        "Authentication failed: Unexpected error during token verification.",
+        { ...context, error: error.message },
+      );
     } else {
-      // Handle non-Error exceptions
-      errorMessage = 'Unknown verification error';
-      logger.error('Authentication failed: Unexpected non-error exception during token verification.', { ...context, error });
+      errorMessage = "Unknown verification error";
+      logger.error(
+        "Authentication failed: Unexpected non-error exception during token verification.",
+        { ...context, error },
+      );
     }
-    // Respond with 401 Unauthorized for any token validation failure.
     res.status(401).json({ error: `Unauthorized: ${errorMessage}.` });
-    // Do not call next() - halt processing for this request.
   }
 }
diff --git a/src/mcp-server/transports/httpTransport.ts b/src/mcp-server/transports/httpTransport.ts
@@ -379,6 +379,15 @@ export async function startHttpTransport(
       logger.debug(`Processing POST request content for session ${currentSessionId}...`, { ...basePostContext, sessionId: currentSessionId, isInitReq });
       // Delegate the actual handling (parsing, routing, response/SSE generation) to the SDK transport instance.
       // The SDK transport handles returning 202 for notification/response-only POSTs internally.
+      
+      // --- Type modification for req.auth compatibility ---
+      const tempReqPost = req as any; // Allow modification
+      if (tempReqPost.auth && (typeof tempReqPost.auth === 'string' || (typeof tempReqPost.auth === 'object' && 'devMode' in tempReqPost.auth))) {
+        logger.debug('Sanitizing req.auth for SDK compatibility (POST)', { ...basePostContext, sessionId: currentSessionId, originalAuthType: typeof tempReqPost.auth });
+        tempReqPost.auth = undefined;
+      }
+      // --- End modification ---
+
       await transport.handleRequest(req, res, req.body);
       logger.debug(`Finished processing POST request content for session ${currentSessionId}.`, { ...basePostContext, sessionId: currentSessionId });
 
@@ -435,6 +444,15 @@ export async function startHttpTransport(
       // MCP Spec (GET): Client SHOULD include Last-Event-ID for resumption. Resumption handling depends on SDK transport.
       // MCP Spec (DELETE): Client SHOULD send DELETE to terminate. Server MAY respond 405 if not supported.
       // This implementation supports DELETE via the SDK transport's handleRequest.
+
+      // --- Type modification for req.auth compatibility ---
+      const tempReqSession = req as any; // Allow modification
+      if (tempReqSession.auth && (typeof tempReqSession.auth === 'string' || (typeof tempReqSession.auth === 'object' && 'devMode' in tempReqSession.auth))) {
+        logger.debug(`Sanitizing req.auth for SDK compatibility (${method})`, { ...baseSessionReqContext, sessionId, originalAuthType: typeof tempReqSession.auth });
+        tempReqSession.auth = undefined;
+      }
+      // --- End modification ---
+      
       await transport.handleRequest(req, res);
       logger.info(`Successfully handled ${method} request for session ${sessionId}`, { ...baseSessionReqContext, sessionId });
       // Note: For DELETE, the transport's handleRequest should trigger the 'onclose' handler for cleanup.
