Merge branch 'dr-transport-auth-fixes' into 'main'

fix(phase-03): address post-merge recheck findings

See merge request ai-adoption/mcp/mcp4openapi!6

Author: Růžička, David

src/auth/client-auth-gate.test.ts

@@ -71,6 +71,27 @@ describe('ClientAuthGate (API key path only)', () => {
     );
 
     await expect(gate.validate('wrong-key')).rejects.toBeInstanceOf(ClientAuthGateError);
+    await expect(gate.validate('wrong-key')).rejects.toThrow(/no valid identity resolved/);
+  });
+
+  it('returns principal when mode=optional and valid key is presented', async () => {
+    const gate = new ClientAuthGate(
+      'profile-a',
+      {
+        mode: 'optional',
+        api_keys: {
+          type: 'inline',
+          keys: [{ key_from_env: ENV_VAR, subject: 'service-a', scopes: ['read'] }],
+        },
+      },
+      makeLogger(),
+    );
+
+    const principal = await gate.validate(VALID_KEY);
+    expect(principal).not.toBeNull();
+    expect(principal!.authType).toBe('token');
+    expect(principal!.subject).toBe('service-a');
+    expect(principal!.scopes).toEqual(['read']);
   });
 
   it('returns null when API key is invalid and mode=optional', async () => {
@@ -121,6 +142,7 @@ describe('ClientAuthGate (API key path only)', () => {
     );
 
     await expect(gate.validate(undefined)).rejects.toBeInstanceOf(ClientAuthGateError);
+    await expect(gate.validate(undefined)).rejects.toThrow(/no token presented/);
   });
 
   it('defaults mode to "required" when omitted (closed by default)', async () => {
@@ -137,6 +159,10 @@ describe('ClientAuthGate (API key path only)', () => {
 
     await expect(gate.validate(undefined)).rejects.toBeInstanceOf(ClientAuthGateError);
     await expect(gate.validate('wrong-key')).rejects.toBeInstanceOf(ClientAuthGateError);
+    // Happy path: valid key still resolves a principal even when mode was omitted.
+    const principal = await gate.validate(VALID_KEY);
+    expect(principal).not.toBeNull();
+    expect(principal!.authType).toBe('token');
   });
 
   it('returns null when mode=optional and no api_keys store is configured', async () => {
@@ -150,14 +176,17 @@ describe('ClientAuthGate (API key path only)', () => {
     expect(await gate.validate('any-token')).toBeNull();
   });
 
-  it('mode takes precedence over mode_from_env when both are set', async () => {
+  it('mode_from_env field is ignored when mode is already set (constructor receives pre-resolved config)', async () => {
+    // Constructor precondition: config.mode is always a resolved literal by the time
+    // ClientAuthGate is constructed (resolveClientAuthGateConfig runs first in the
+    // transport). mode_from_env resolution is tested in client-auth-gate-validator.test.ts.
     const modeEnv = 'CLIENT_AUTH_GATE_TEST_MODE';
-    process.env[modeEnv] = 'optional'; // would make gate optional if read
+    process.env[modeEnv] = 'optional'; // would flip mode if the constructor read it
     try {
       const gate = new ClientAuthGate(
         'profile-a',
         {
-          mode: 'required', // explicit mode wins — mode_from_env must be ignored
+          mode: 'required', // explicit resolved mode wins; mode_from_env is ignored
           mode_from_env: modeEnv,
           api_keys: {
             type: 'inline',
@@ -172,55 +201,6 @@ describe('ClientAuthGate (API key path only)', () => {
     }
   });
 
-  it('resolves mode from mode_from_env when env var is set to optional', async () => {
-    const modeEnv = 'CLIENT_AUTH_GATE_TEST_MODE';
-    process.env[modeEnv] = 'optional';
-    try {
-      const gate = new ClientAuthGate(
-        'profile-a',
-        {
-          mode_from_env: modeEnv,
-          api_keys: {
-            type: 'inline',
-            keys: [{ key_from_env: ENV_VAR, subject: 'service-a' }],
-          },
-        },
-        makeLogger(),
-      );
-      expect(await gate.validate('wrong-key')).toBeNull();
-    } finally {
-      delete process.env[modeEnv];
-    }
-  });
-
-  it('throws ClientAuthGateError in constructor when mode_from_env env var is not set', () => {
-    expect(
-      () =>
-        new ClientAuthGate(
-          'profile-a',
-          { mode_from_env: 'CLIENT_AUTH_GATE_TEST_MODE_UNSET' },
-          makeLogger(),
-        ),
-    ).toThrow(ClientAuthGateError);
-  });
-
-  it('throws ClientAuthGateError in constructor when mode_from_env env var has invalid value', () => {
-    const modeEnv = 'CLIENT_AUTH_GATE_TEST_MODE';
-    process.env[modeEnv] = 'superuser';
-    try {
-      expect(
-        () =>
-          new ClientAuthGate(
-            'profile-a',
-            { mode_from_env: modeEnv },
-            makeLogger(),
-          ),
-      ).toThrow(ClientAuthGateError);
-    } finally {
-      delete process.env[modeEnv];
-    }
-  });
-
   it('throws ClientAuthGateError when token is provided, no api_keys store, and mode=required', async () => {
     // Pins the fall-through path: token present, apiKeyStore absent, mode=required
     // → must throw rather than silently returning null.

src/auth/client-auth-gate.ts

@@ -39,22 +39,18 @@ export class ClientAuthGate {
 
   constructor(profileId: string, config: ClientAuthGateConfig, logger: Logger) {
     this.logger = logger;
-    let resolvedMode = config.mode;
-    if (!resolvedMode && config.mode_from_env) {
-      const envValue = process.env[config.mode_from_env];
-      if (!envValue) {
-        throw new ClientAuthGateError(
-          `client_auth_gate.mode_from_env: env var '${config.mode_from_env}' is not set`,
-        );
-      }
-      if (envValue !== 'required' && envValue !== 'optional') {
-        throw new ClientAuthGateError(
-          `client_auth_gate.mode must be 'required' or 'optional', got '${envValue}'`,
-        );
-      }
-      resolvedMode = envValue;
+    // Precondition: config.mode must already be a resolved literal ('required' |
+    // 'optional'). mode_from_env resolution belongs in resolveClientAuthGateConfig(),
+    // which is always called before constructing ClientAuthGate in the transport.
+    // Keeping resolution outside the constructor avoids duplicating the env-var
+    // lookup and ensures a single source of truth for mode normalization.
+    const mode = config.mode ?? 'required';
+    if (mode !== 'required' && mode !== 'optional') {
+      throw new ClientAuthGateError(
+        `client_auth_gate.mode must be 'required' or 'optional', got '${mode}'`,
+      );
     }
-    this.resolvedMode = resolvedMode ?? 'required';
+    this.resolvedMode = mode;
     if (config.api_keys) {
       this.apiKeyStore = createApiKeyStore(config.api_keys, profileId, logger);
     }

src/transport/http-transport-client-auth.test.ts

@@ -216,11 +216,19 @@ describe('Client auth gate (Phase 3) — session init integration', () => {
     };
     transport.setProfileContextProvider(async () => profileContext);
 
+    const logger = (transport as unknown as { logger: { warn: ReturnType<typeof vi.fn> } }).logger;
+    const warnSpy = vi.spyOn(logger, 'warn');
+
     const req = makeReq();
     const res = makeRes();
     await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
 
     expect(res.statusCode).toBe(401);
+    expect(res.body).toEqual(expect.objectContaining({ error: 'Unauthorized', message: 'Client authentication failed' }));
+    expect(warnSpy).toHaveBeenCalledWith(
+      'Client auth gate rejected session init',
+      expect.objectContaining({ errorType: 'ClientAuthGateError', profileId: 'default' }),
+    );
   });
 
   // Scenario 5
@@ -269,11 +277,19 @@ describe('Client auth gate (Phase 3) — session init integration', () => {
     };
     transport.setProfileContextProvider(async () => profileContext);
 
+    const logger = (transport as unknown as { logger: { warn: ReturnType<typeof vi.fn> } }).logger;
+    const warnSpy = vi.spyOn(logger, 'warn');
+
     const req = makeReq();
     const res = makeRes();
     await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
 
     expect(res.statusCode).toBe(401);
+    expect(res.body).toEqual(expect.objectContaining({ error: 'Unauthorized', message: 'Client authentication failed' }));
+    expect(warnSpy).toHaveBeenCalledWith(
+      'Client auth gate rejected session init',
+      expect.objectContaining({ errorType: 'ClientAuthGateError', profileId: 'default' }),
+    );
   });
 
   // Scenario 7
@@ -339,10 +355,14 @@ describe('Client auth gate (Phase 3) — session init integration', () => {
     await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
 
     expect(res.statusCode).toBe(401);
-    expect(res.body).toEqual(expect.objectContaining({ error: 'Unauthorized' }));
+    expect(res.body).toEqual(expect.objectContaining({ error: 'Unauthorized', message: 'Client authentication failed' }));
     expect(warnSpy).toHaveBeenCalledWith(
       'Client auth gate rejected session init',
-      expect.objectContaining({ errorType: 'unknown' }),
+      expect.objectContaining({
+        errorType: 'unknown',
+        profileId: 'default',
+        error: 'upstream store unreachable',
+      }),
     );
   });
 
@@ -370,10 +390,81 @@ describe('Client auth gate (Phase 3) — session init integration', () => {
     await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
 
     expect(res.statusCode).toBe(401);
+    expect(res.body).toEqual(expect.objectContaining({ error: 'Unauthorized', message: 'Client authentication failed' }));
     expect(warnSpy).toHaveBeenCalledWith(
       'Client auth gate rejected session init',
-      expect.objectContaining({ errorType: 'ClientAuthGateError' }),
+      expect.objectContaining({
+        errorType: 'ClientAuthGateError',
+        profileId: 'default',
+      }),
+    );
+  });
+
+  // Scenario 11 — mode=optional + valid key → principal populated
+  it('mode=optional + valid key -> 200 + session.clientPrincipal populated', async () => {
+    // Pins that optional mode does NOT degrade all requests to anonymous — a client
+    // that presents a valid key still receives a populated principal for audit/policy.
+    const profileContext: HttpProfileContext = {
+      profileId: 'default',
+      client_auth_gate: {
+        mode: 'optional',
+        api_keys: {
+          type: 'inline',
+          keys: [{ key_from_env: VALID_KEY_ENV, subject: SUBJECT, scopes: ['read'] }],
+        },
+      } satisfies ClientAuthGateConfig,
+    };
+    transport.setProfileContextProvider(async () => profileContext);
+
+    const req = makeReq(`Bearer ${VALID_KEY}`);
+    const res = makeRes();
+    await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
+
+    expect(res.statusCode).toBe(200);
+    const sessionId = res.headers['Mcp-Session-Id'];
+    expect(sessionId).toBeTruthy();
+    const session = getSession(transport, 'default', sessionId!) as
+      | { clientPrincipal?: { authType: string; subject: string; scopes: string[] } }
+      | undefined;
+    expect(session).toBeDefined();
+    expect(session!.clientPrincipal).toBeDefined();
+    expect(session!.clientPrincipal!.authType).toBe('token');
+    expect(session!.clientPrincipal!.subject).toBe(SUBJECT);
+    expect(session!.clientPrincipal!.scopes).toEqual(['read']);
+  });
+
+  // Scenario 12 — gate-initialized log fires with correct fields
+  it('gate construction logs "Client auth gate initialized" with mode and hasApiKeys', async () => {
+    // Pins that the initialization log (used by ops to confirm gate config at startup)
+    // includes the correct structured fields.
+    const profileContext: HttpProfileContext = {
+      profileId: 'default',
+      client_auth_gate: {
+        mode: 'required',
+        api_keys: {
+          type: 'inline',
+          keys: [{ key_from_env: VALID_KEY_ENV, subject: SUBJECT }],
+        },
+      },
+    };
+    transport.setProfileContextProvider(async () => profileContext);
+
+    const logger = (transport as unknown as { logger: { info: ReturnType<typeof vi.fn> } }).logger;
+    const infoSpy = vi.spyOn(logger, 'info');
+
+    // Drive a request to trigger getProfileState() and gate construction.
+    const req = makeReq(`Bearer ${VALID_KEY}`);
+    const res = makeRes();
+    await (transport as unknown as { handlePost: (r: unknown, s: unknown) => Promise<void> }).handlePost(req, res);
+
+    const initCall = infoSpy.mock.calls.find(
+      (c) => typeof c[0] === 'string' && c[0].includes('Client auth gate initialized'),
     );
+    expect(initCall).toBeDefined();
+    const logFields = initCall![1] as Record<string, unknown>;
+    expect(logFields['mode']).toBe('required');
+    expect(logFields['hasApiKeys']).toBe(true);
+    expect(logFields['profileId']).toBe('default');
   });
 
   // Scenario 10 — session-creation log includes clientSubject + clientAuthType

src/transport/http-transport.ts

@@ -533,13 +533,24 @@ export class HttpTransport {
       // api_keys env vars. In the profile-loader path this is a no-op (validator
       // already ran at load time and mode is a literal string). For direct
       // HttpTransport construction the call provides the same fail-fast guarantees.
-      const gateConfig = resolveClientAuthGateConfig(context.client_auth_gate);
-      state.clientAuthGate = new ClientAuthGate(profileId, gateConfig, this.logger);
-      this.logger.info('Client auth gate initialized', {
-        profileId,
-        mode: gateConfig.mode,
-        hasApiKeys: !!gateConfig.api_keys,
-      });
+      // Wrap in try/catch so a misconfigured gate produces a clear error log and a
+      // descriptive 500 (via the handlePost outer catch) rather than a generic one.
+      try {
+        const gateConfig = resolveClientAuthGateConfig(context.client_auth_gate);
+        state.clientAuthGate = new ClientAuthGate(profileId, gateConfig, this.logger);
+        this.logger.info('Client auth gate initialized', {
+          profileId,
+          mode: gateConfig.mode,
+          hasApiKeys: !!gateConfig.api_keys,
+        });
+      } catch (err) {
+        this.logger.error(
+          'Client auth gate configuration error — profile will be unavailable',
+          err instanceof Error ? err : new Error(String(err)),
+          { profileId },
+        );
+        throw err;
+      }
     }
 
     this.profileStates.set(profileId, state);
@@ -2791,6 +2802,7 @@ export class HttpTransport {
                 profileId: requestProfileId,
                 error: err instanceof Error ? err.message : String(err),
                 errorType: isClientAuthGateError ? 'ClientAuthGateError' : 'unknown',
+                ...(isClientAuthGateError ? {} : { errorStack: err instanceof Error ? err.stack : undefined }),
               });
               res.status(HTTP_STATUS.UNAUTHORIZED).json({
                 error: 'Unauthorized',
@@ -3020,6 +3032,10 @@ export class HttpTransport {
         status = HTTP_STATUS.TOO_MANY_REQUESTS;
         errorLabel = 'Too Many Requests';
         message = `Rate limit exceeded: ${error.message} (correlation ID: ${correlationId})`;
+      } else if (error instanceof ClientAuthGateError) {
+        status = 500;
+        errorLabel = 'Gateway Configuration Error';
+        message = `Client auth gate misconfigured: ${error.message} (correlation ID: ${correlationId})`;
       }
 
       res.status(status).json({ error: errorLabel, message, correlationId });