fix: address final review nits N1-N4

- N1: Add session cookie failFlow test — validates the hasSession
  branch triggers failFlow on OAuth error callback
- N2: Replace setTimeout(50) with setImmediate for microtask drain
- N3: Add 'unknown client' attribution to isClientRejection JSDoc
- N4: Remove dead getFlowState mock from failFlow tests
This commit is contained in:
Danny Avila 2026-04-03 21:59:39 -04:00
parent c20266c4f9
commit 0a77eb2a9f
3 changed files with 33 additions and 5 deletions

View file

@ -315,7 +315,6 @@ describe('MCP Routes', () => {
it('should fail the flow when OAuth error is received with valid CSRF cookie', async () => {
const flowId = 'test-user-id:test-server';
const mockFlowManager = {
getFlowState: jest.fn().mockResolvedValue({ status: 'PENDING', createdAt: Date.now() }),
failFlow: jest.fn().mockResolvedValue(true),
};
@ -342,10 +341,38 @@ describe('MCP Routes', () => {
);
});
it('should fail the flow when OAuth error is received with valid session cookie', async () => {
const flowId = 'test-user-id:test-server';
const mockFlowManager = {
failFlow: jest.fn().mockResolvedValue(true),
};
getLogStores.mockReturnValueOnce({});
require('~/config').getFlowStateManager.mockReturnValueOnce(mockFlowManager);
MCPOAuthHandler.resolveStateToFlowId.mockResolvedValueOnce(flowId);
const sessionToken = generateTestCsrfToken('test-user-id');
const response = await request(app)
.get('/api/mcp/test-server/oauth/callback')
.set('Cookie', [`oauth_session=${sessionToken}`])
.query({
error: 'invalid_client',
state: flowId,
});
const basePath = getBasePath();
expect(response.status).toBe(302);
expect(response.headers.location).toBe(`${basePath}/oauth/error?error=invalid_client`);
expect(mockFlowManager.failFlow).toHaveBeenCalledWith(
flowId,
'mcp_oauth',
'invalid_client',
);
});
it('should NOT fail the flow when OAuth error is received without cookies (DoS prevention)', async () => {
const flowId = 'test-user-id:test-server';
const mockFlowManager = {
getFlowState: jest.fn().mockResolvedValue({ status: 'PENDING', createdAt: Date.now() }),
failFlow: jest.fn(),
};

View file

@ -495,7 +495,8 @@ export class MCPConnectionFactory {
/**
* Checks whether an error indicates the OAuth client registration was rejected.
* Includes RFC 6749 §5.2 standard codes (`invalid_client`, `unauthorized_client`)
* and known vendor-specific patterns (Okta: `client_id mismatch`, Auth0: `client not found`).
* and known vendor-specific patterns (Okta: `client_id mismatch`, Auth0: `client not found`,
* generic: `unknown client`).
*/
static isClientRejection(error: unknown): boolean {
if (!error || typeof error !== 'object') {

View file

@ -354,8 +354,8 @@ describe('MCPConnectionFactory', () => {
await oauthRequiredHandler!({ serverUrl: 'https://api.example.com' });
// Wait for the background .catch() handler to run
await new Promise((r) => setTimeout(r, 50));
// Drain microtasks so the background .catch() handler completes
await new Promise((r) => setImmediate(r));
// deleteClientRegistration should have been called via clearStaleClientIfRejected
expect(mockMCPTokenStorage.deleteClientRegistration).toHaveBeenCalledWith(