Improve orchestrated auth mode #219
Conversation
…r handling and service configuration
…r handling and service configuration
📝 WalkthroughWalkthroughAdds a CIMD subsystem (types, validator, errors, cache backends, service), federated/orchestrated multi‑provider auth flows and stores, test utilities/mocks (MockOAuthServer, MockCimdServer), E2E apps/tests, SDK DI/context wiring, and extensive docs. Changes are additive and include new public APIs and tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
participant AuthorizeFlow
participant CimdService
participant Cache
participant Validator
participant HTTP as Fetch
end
Client->>AuthorizeFlow: GET /oauth/authorize?client_id=https://...
AuthorizeFlow->>CimdService: resolveClientMetadata(client_id)
CimdService->>Cache: get(client_id)
alt cache hit
Cache-->>CimdService: cached metadata
else cache miss
CimdService->>Validator: validateClientIdUrl(client_id)
Validator-->>CimdService: validated URL
CimdService->>HTTP: GET CIMD document (If-None-Match/If-Modified-Since?)
HTTP-->>CimdService: 200/304 + headers/body
CimdService->>CimdService: validateDocument(document)
CimdService->>Cache: set(client_id, metadata, headers)
end
CimdService-->>AuthorizeFlow: metadata
AuthorizeFlow->>AuthorizeFlow: validateRedirectUri(redirect_uri, metadata)
AuthorizeFlow-->>Client: render login/consent (client_name, logo_uri)
sequenceDiagram
rect rgba(255,240,200,0.5)
participant User
participant FrontMCP
participant FederatedSession
participant TokenStore
participant Provider1 as GitHub
participant Provider2 as Slack
end
User->>FrontMCP: Start auth (providers: github, slack)
FrontMCP->>FederatedSession: createSession([github, slack])
FrontMCP->>FrontMCP: startNextProvider(github) (PKCE/state)
FrontMCP->>Provider1: Redirect to provider authorize
User->>Provider1: Authenticate
Provider1->>FrontMCP: /oauth/provider/github/callback?code=...
FrontMCP->>Provider1: exchangeCode(code)
FrontMCP->>TokenStore: storeTokens(authId, 'github', tokens)
FrontMCP->>FederatedSession: completeCurrentProvider(github)
FrontMCP->>FrontMCP: startNextProvider(slack)
FrontMCP->>Provider2: Redirect to Slack authorize
User->>Provider2: Authenticate
Provider2->>FrontMCP: /oauth/provider/slack/callback?code=...
FrontMCP->>Provider2: exchangeCode(code)
FrontMCP->>TokenStore: storeTokens(authId, 'slack', tokens)
FrontMCP->>FederatedSession: completeCurrentProvider(slack)
FrontMCP->>FrontMCP: issue FrontMCP authorization code
FrontMCP-->>User: Redirect back with code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@apps/e2e/demo-e2e-cimd/src/main.ts`:
- Around line 4-5: The port parsing currently uses parseInt(process.env['PORT']
?? '3150', 10) which can produce NaN for non-numeric PORT values; update the
logic that sets the port variable so that after parsing process.env['PORT'] you
detect NaN (e.g., Number.isNaN(parsed)) and fall back to the default 3150,
ensuring the final port value assigned to the port constant is always a valid
number; reference the existing parseInt call and the port constant when making
the change.
In `@apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-repos.tool.ts`:
- Around line 10-64: The execute method of GitHubReposTool ignores the input
`limit` (defined in inputSchema) and always returns the full mock repos list;
update execute to read the parsed limit from its parameter (currently named
_input, or destructure as { limit }) and apply it to the returned repos array
(e.g., slice the mock array to limit) so the output.repos respects the input
contract while leaving providerId/token fields unchanged.
In `@apps/e2e/demo-e2e-orchestrated/src/main-multi-provider.ts`:
- Around line 13-15: The port parsing using parseInt(process.env['PORT'] ??
'3122', 10) can yield NaN for non-numeric PORT values; update the initialization
of the port variable so that after parsing process.env['PORT'] (used in
main-multi-provider.ts) you check Number.isFinite/Number.isNaN (or isNaN) and
default to 3122 (or throw a clear error) when the parsed value is invalid;
specifically adjust the code that sets const port to validate the result of
parseInt(process.env['PORT'] ?? '3122', 10) and fallback to 3122 (or raise a
descriptive error) before using port to bind the server.
In `@libs/auth/src/cimd/cimd.cache.ts`:
- Around line 89-105: The TTL calculation applies the Age header subtraction to
cacheControl['max-age'] but not to cacheControl['s-maxage'], so s-maxage can
incorrectly ignore Age; update the TTL logic (in the block that inspects
cacheControl['s-maxage']) to mirror the Age subtraction used for max-age: read
cacheControl['s-maxage'] into a local seconds variable, if headers.age exists
parse and clamp it, subtract it from the s-maxage seconds (not going below 0),
and then set ttlMs = adjustedSeconds * 1000 so s-maxage still takes precedence
but respects the Age header.
In `@libs/auth/src/cimd/cimd.service.ts`:
- Around line 291-295: In readResponseWithLimit, the current fallback to
response.text() bypasses the maxBytes limit when response.body?.getReader() is
undefined; change the fallback so you still enforce maxBytes by calling
response.text() (or response.arrayBuffer()) and then measuring the byte length
(e.g., Buffer.byteLength(text, 'utf8') or arrayBuffer.byteLength) and if it
exceeds maxBytes throw a controlled error (include clientId for context) or
truncate according to your policy; update the readResponseWithLimit
implementation to perform this size check when getReader() is not available so
oversized responses are rejected/handled consistently.
In `@libs/auth/src/session/authorization.store.ts`:
- Around line 64-65: The exported authorizationCodeRecordSchema is missing
fields defined on the public AuthorizationCodeRecord interface (e.g.,
pendingAuthId plus consent/federated-login related fields), causing a
schema/interface mismatch; update authorizationCodeRecordSchema so it declares
all 22 fields from AuthorizationCodeRecord (including pendingAuthId and the
consent/federated login fields) with matching types and optionality, ensuring
the public schema faithfully represents the interface used by consumers and
preventing future validation/stripping issues.
In `@libs/sdk/src/auth/cimd/index.ts`:
- Around line 1-63: The docs reference and main SDK exports are inconsistent:
the new subpath module '@frontmcp/sdk/auth/cimd' and the DI token
CimdServiceToken are not surfaced in the main SDK entry, causing examples that
import from '@frontmcp/sdk' to break; fix by either (A) re-exporting the CIMD
module and CimdServiceToken from the SDK's main index so '@frontmcp/sdk'
includes the same symbols as '@frontmcp/sdk/auth/cimd', or (B) add an explicit
package.json "exports" subpath for '@frontmcp/sdk/auth/cimd' and update docs to
show imports from '@frontmcp/sdk/auth/cimd' (or from '@frontmcp/auth' for
upstream symbols like errors), and update the docs page
docs/draft/docs/authentication/cimd.mdx to clearly show the correct import paths
and include CimdServiceToken in examples.
In `@libs/sdk/src/auth/cimd/README.md`:
- Around line 371-386: The markdown tables under the "IPv4 Blocked Ranges" and
"IPv6 Blocked Ranges" sections violate markdownlint MD058/MD040 by not having
blank lines before/after the tables and the example fenced blocks lack language
identifiers; fix by adding a blank line above and below each table (e.g., around
the "IPv4 Blocked Ranges" and "IPv6 Blocked Ranges" tables) and annotate all
fenced code blocks with the appropriate language (for example ```http) as shown
in the provided example; apply the same changes to the other affected block
ranges noted (lines referenced in the comment).
- Around line 45-47: Replace bare URL and email occurrences with Markdown
autolinks or link text: wrap the metadata URL
`https://mcpclient.ai/oauth/metadata.json` as
<https://mcpclient.ai/oauth/metadata.json> or a markdown link (e.g., `[Claude
MCP Client](https://mcpclient.ai/oauth/metadata.json)`), and wrap the email
`john@example.com` as `<john@example.com>` or a mailto link. Update the README
lines mentioning "Claude wants to create a GitHub issue", "Claude MCP Client",
and "John (john@example.com) must approve the access" to use those linked forms;
apply the same change to the other occurrences flagged (the other similar lines
in the file).
In `@libs/sdk/src/auth/flows/oauth.callback.flow.ts`:
- Around line 311-328: The code is using an unsafe cast (sessionStore as any).
Replace the any by declaring or importing a SessionStore interface matching the
createSession signature and type the sessionStore variable accordingly (or
implement a type guard isSessionStore(sessionStore): sessionStore is
SessionStore and narrow before calling createSession), then call
sessionStore.createSession(...) to create federatedSession; reference the
createSession method and the federatedSession variable when updating types to
ensure the created object shape matches expected fields (pendingAuthId,
clientId, redirectUri, scopes, state, resource, userInfo, frontmcpPkce,
providerIds).
- Around line 387-396: Replace the direct use of browser/node crypto in
generatePkceVerifier with the helper from `@frontmcp/utils`: locate the
generatePkceVerifier method and stop calling crypto.getRandomValues; instead
import and call the appropriate random/PKCE utility from `@frontmcp/utils` (or its
PKCE verifier generator if available) to produce the secure random bytes/PKCE
string, preserve the same character set/length behavior, and update the import
list to reference `@frontmcp/utils`.
In `@libs/sdk/src/auth/flows/well-known.oauth-authorization-server.flow.ts`:
- Around line 106-123: The isOrchestrated flag is inverted: currently set as
isOrchestrated: !metadata.auth which treats configured auth as non-orchestrated;
update the value to use the actual orchestrated detection (call
isOrchestratedMode with metadata.auth) so that in the
wellKnownAsStateSchema.parse payload isOrchestrated reflects the real mode;
locate the block that builds the object passed to wellKnownAsStateSchema.parse
in well-known.oauth-authorization-server.flow (uses variables metadata and
baseUrl) and replace the isOrchestrated assignment with isOrchestrated:
isOrchestratedMode(metadata.auth) (or equivalent) so redirects/metadata serving
behave correctly.
In `@libs/testing/src/auth/mock-oauth-server.ts`:
- Around line 859-872: The wildcard-based regex construction using the
user-controlled variable pattern (see variables pattern, regexPattern,
redirectUri) is vulnerable to ReDoS; replace the dynamic RegExp approach with a
safe, linear-time wildcard match: split the pattern on '*' and validate each
literal segment against redirectUri using indexOf/startsWith/endsWith checks in
sequence, and enforce a maximum wildcard count (e.g., <= 2) and a max segment
length to reject overly complex patterns; if the pattern exceeds limits, log an
error and skip it.
🧹 Nitpick comments (30)
libs/sdk/src/context/frontmcp-context.ts (1)
386-410: LGTM! Well-designed internal API for context token management.The implementation correctly:
- Uses
@internalannotations to restrict usage to SDK internals- Returns
ReadonlyMapto prevent external mutation- Provides type-safe registration via the generic
<T>parameterOptional consideration: If tokens need cleanup during context lifecycle (e.g., for long-lived sessions), a
deleteContextToken(token: unknown): booleanmethod might be useful. This can be deferred if not immediately needed.apps/e2e/demo-e2e-remote/e2e/remote.e2e.test.ts (1)
30-44: Consider cleanup on partial startup failure.If the mock Mintlify server starts successfully but the local MCP server fails to start (lines 47-65), the mock Mintlify server may remain running since
afterAllwon't execute ifbeforeAllthrows. This could leave orphaned processes during test development.♻️ Optional: Add cleanup on partial failure
} catch (error) { console.error('[E2E] Failed to start local MCP server:', error); + // Clean up mock Mintlify server if it was started + if (mockMintlifyServer) { + await mockMintlifyServer.stop(); + mockMintlifyServer = null; + } throw error; }apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-user.tool.ts (1)
10-36: Explicitly type ToolContext generics for consistency and IDE support.ToolContext has well-defined generic parameters with proper defaults (not
any), so the current code is correctly typed. However, explicitly specifyingToolContext<Input, Output>aligns with other demo tools and improves readability.♻️ Suggested update
const inputSchema = z.object({}); const outputSchema = z.object({ error: z.string().optional(), }); +type Input = z.infer<typeof inputSchema>; +type Output = z.infer<typeof outputSchema>; + -export class GitHubUserTool extends ToolContext { - async execute(_input: z.infer<typeof inputSchema>): Promise<z.infer<typeof outputSchema>> { +export class GitHubUserTool extends ToolContext<Input, Output> { + async execute(_input: Input): Promise<Output> {libs/testing/src/fixtures/test-fixture.ts (1)
73-104: Preserve the original error type when startup fails.Wrapping in a generic
ErrordiscardsServerStartErrortyping and the original stack. Consider throwingServerStartErrorwith the original error as the cause to keep diagnostics intact.♻️ Suggested change
-import { TestServer } from '../server/test-server'; +import { TestServer } from '../server/test-server'; +import { ServerStartError } from '../errors'; } catch (error) { // Re-throw with additional context const errMsg = error instanceof Error ? error.message : String(error); - throw new Error( + throw new ServerStartError( `Failed to start test server.\n\n` + `Server entry: ${currentConfig.server}\n` + `Command: ${serverCommand}\n\n` + `Error: ${errMsg}`, + error instanceof Error ? error : undefined, ); }docs/draft/docs/authentication/cimd.mdx (1)
48-50: Minor style: vary repeated phrasing in the scenario bullets.Line 48 repeats “wants to”; consider a small rephrase for variety.
libs/sdk/src/auth/session/__tests__/orchestrated-token-store.test.ts (1)
129-149: Consider adding cleanup() method test coverage.The token expiration tests verify that expired tokens return
nullon retrieval, but there's no explicit test for thecleanup()method that's called periodically. Consider adding a test to verifycleanup()removes expired records from the store.💡 Suggested test for cleanup()
it('should remove expired tokens during cleanup', async () => { await store.storeTokens(authId, providerId, { accessToken: 'gho_xxxx', expiresAt: Date.now() - 1000, // Already expired }); expect(store.size).toBe(1); await store.cleanup(); expect(store.size).toBe(0); });libs/testing/src/auth/mock-cimd-server.ts (2)
307-322: Remove unusedclientIdvariable.The
clientIdvariable on line 313 is computed but never used, making it dead code.🧹 Remove unused variable
registerInvalidDocument(path: string, document: unknown): void { if (!this._info) { throw new Error('Mock CIMD server is not running'); } const fullPath = path.startsWith('/') ? path : `/${path}`; - const clientId = `${this._info.baseUrl}${fullPath}`; // Store as a special "invalid" client that bypasses validation this.clients.set(fullPath, { path: fullPath, document: document as CimdDocument, }); this.log(`Registered invalid document at ${fullPath}`); }
385-392: Avoid non-null assertion; use a guard instead.Per coding guidelines, avoid non-null assertions (
!). The code already checkscustomResponses.has(url), but TypeScript's control flow analysis doesn't narrow theget()result.♻️ Use guard pattern
// Check for custom error responses first - if (this.customResponses.has(url)) { - const customResponse = this.customResponses.get(url)!; + const customResponse = this.customResponses.get(url); + if (customResponse) { res.writeHead(customResponse.status, { 'Content-Type': 'application/json' }); res.end(customResponse.body ? JSON.stringify(customResponse.body) : ''); this.log(`Returned custom error response: ${customResponse.status}`); return; }libs/sdk/src/auth/session/orchestrated-token.store.ts (2)
124-143: Consider makingderiveKeyForRecordsynchronous.The
hkdfSha256function appears to be synchronous based on the implementation, but this method is markedasync. While not incorrect, it adds unnecessary Promise overhead.💡 Make method synchronous
- private async deriveKeyForRecord(compositeKey: string): Promise<Uint8Array> { + private deriveKeyForRecord(compositeKey: string): Uint8Array { if (!this.encryptionKey) { throw new Error('Encryption key not configured'); } // Check cache const cached = this.derivedKeys.get(compositeKey); if (cached) { return cached; } // Derive key using HKDF const info = new TextEncoder().encode(`orchestrated-token:${compositeKey}`); const salt = new TextEncoder().encode('frontmcp-token-store'); const derivedKey = hkdfSha256(this.encryptionKey, salt, info, 32); this.derivedKeys.set(compositeKey, derivedKey); return derivedKey; }Note: This would require updating callers to remove
await.
288-296: Safe iteration pattern for Map deletion.Collecting keys first before deletion is the correct approach. However, for
deleteAllForAuthorization, you're iterating and deleting in the same loop which can be problematic in some JavaScript engines.♻️ Collect keys before deletion
async deleteAllForAuthorization(authorizationId: string): Promise<void> { const prefix = `${authorizationId}:`; + const keysToDelete: string[] = []; for (const key of this.tokens.keys()) { if (key.startsWith(prefix)) { - this.tokens.delete(key); - this.derivedKeys.delete(key); + keysToDelete.push(key); } } + for (const key of keysToDelete) { + this.tokens.delete(key); + this.derivedKeys.delete(key); + } }libs/auth/src/cimd/cimd.types.ts (1)
132-150: Consider adding cross-field validation for TTL constraints.The schema allows configurations where
minTtlMs > defaultTtlMsordefaultTtlMs > maxTtlMs, which could cause unexpected behavior at runtime.♻️ Suggested refinement with cross-field validation
export const cimdCacheConfigSchema = z.object({ defaultTtlMs: z.number().min(0).default(3600_000), maxTtlMs: z.number().min(0).default(86400_000), minTtlMs: z.number().min(0).default(60_000), -}); +}).refine( + (data) => data.minTtlMs <= data.defaultTtlMs && data.defaultTtlMs <= data.maxTtlMs, + { message: 'TTL values must satisfy: minTtlMs <= defaultTtlMs <= maxTtlMs' } +);libs/sdk/src/auth/session/federated-auth.session.ts (2)
259-269: Safe iteration pattern for cleanup, but consider collecting IDs first.Deleting from a Map while iterating over it is safe in JavaScript (per ES6 spec), but collecting expired IDs first can be clearer and avoids potential confusion.
♻️ Suggested clearer pattern
async cleanup(): Promise<void> { const now = Date.now(); + const expiredIds: string[] = []; for (const [id, record] of this.sessions) { if (now > record.expiresAt) { - this.sessions.delete(id); + expiredIds.push(id); } } + for (const id of expiredIds) { + this.sessions.delete(id); + } }
374-390: Non-null assertion on line 384 is safe but could use type guard.The
shift()!on line 384 is preceded by an empty queue check, so it's safe. However, per coding guidelines, prefer explicit error handling over non-null assertions.♻️ Suggested explicit handling
// Pop from queue and set as current - const providerId = session.providerQueue.shift()!; + const providerId = session.providerQueue.shift(); + if (!providerId) { + // This should never happen due to the check above, but satisfies type checker + throw new Error('No more providers in queue'); + } session.currentProviderId = providerId;libs/sdk/src/auth/session/__tests__/federated-auth-session.test.ts (2)
181-197: Add explicitinstanceofcheck for Map restoration.Per coding guidelines, include
instanceofchecks in tests to verify proper type hierarchy. Line 194 checksinstanceof Map- good. The test could also verify the restored session matches the original more comprehensively.♻️ Suggested enhancement
it('should serialize and deserialize session', () => { const session = createSession(); session.completedProviders.set('github', { providerId: 'github', tokens: { accessToken: 'token' }, completedAt: Date.now(), }); const record = toSessionRecord(session); expect(Array.isArray(record.completedProviders)).toBe(true); const restored = fromSessionRecord(record); expect(restored.completedProviders instanceof Map).toBe(true); expect(restored.completedProviders.get('github')).toBeDefined(); + expect(restored.id).toBe(session.id); + expect(restored.clientId).toBe(session.clientId); + expect(restored.providerQueue).toEqual(session.providerQueue); });
337-385: Consider adding test for cleanup() method behavior.While expiration is tested via
get(), explicitly testing thecleanup()method would improve coverage of the timer-based cleanup path.♻️ Suggested additional test
describe('cleanup', () => { it('should remove expired sessions on cleanup', async () => { const session = store.createSession({ pendingAuthId: '1', clientId: 'c', redirectUri: 'http://localhost/cb', scopes: [], userInfo: {}, frontmcpPkce: { challenge: 'c', method: 'S256' }, providerIds: [], }); session.expiresAt = Date.now() - 1000; await store.store(session); expect(store.size).toBe(1); await store.cleanup(); expect(store.size).toBe(0); }); });libs/sdk/src/common/types/options/auth/orchestrated.schema.ts (1)
81-87: Clarify JSDoc for CIMD default behavior.The JSDoc says
@default { enabled: true }but the field isoptional(), meaningcimdisundefinedby default. The defaultenabled: trueonly applies when acimdobject is explicitly provided. Consider clarifying this nuance.📝 Suggested JSDoc clarification
/** * CIMD (Client ID Metadata Documents) configuration. * Enables OAuth clients to use HTTPS URLs as client identifiers. - * `@default` { enabled: true } + * When provided, defaults to { enabled: true }. + * If omitted, CIMD support depends on service defaults. */ cimd: cimdConfigSchema.optional(),libs/testing/jest.config.ts (1)
1-7: Consider consistency with project conventions for file system operations.The coding guidelines recommend using
@frontmcp/utilsfor file system operations. While Jest config files are typically exempt from such rules (being build-time configuration rather than library code), consider whether aligning with the project's fs utilities would be preferable for consistency.That said, using
require('fs')directly in Jest config is a common and acceptable pattern since Jest configs run in a Node.js context before any module resolution customization takes effect.libs/sdk/src/scope/flows/http.request.flow.ts (1)
199-231: Consider loggingauth:verifyfailures (currently silent).
The catch block suppresses all errors, which can mask real auth verification failures in orchestrated mode. A debug log would help troubleshooting without changing behavior.💡 Suggested tweak
- } catch { - // auth:verify may not be registered, ignore - } + } catch (err) { + // auth:verify may not be registered, ignore + this.logger.debug(`[${this.requestId}] auth:verify failed`, { error: err }); + }libs/sdk/src/auth/flows/oauth.authorize.flow.ts (1)
484-669: Use CIMD client_name on the federated login page.
clientDisplayNameis computed but the federated login UI still usesclientId, so CIMD-friendly names won’t surface in multi-provider flows. Consider threading the display name through.♻️ Proposed refactor
- const federatedLoginHtml = this.renderFederatedLoginPage({ - pendingAuthId, - detection, - clientId: validatedRequest.client_id, - redirectUri: validatedRequest.redirect_uri, - }); + const federatedLoginHtml = this.renderFederatedLoginPage({ + pendingAuthId, + detection, + clientId: validatedRequest.client_id, + clientName: clientDisplayName, + redirectUri: validatedRequest.redirect_uri, + });- private renderFederatedLoginPage(params: { - pendingAuthId: string; - detection: AuthProviderDetectionResult; - clientId: string; - redirectUri: string; - }): string { - const { pendingAuthId, detection, clientId } = params; + private renderFederatedLoginPage(params: { + pendingAuthId: string; + detection: AuthProviderDetectionResult; + clientId: string; + clientName?: string; + redirectUri: string; + }): string { + const { pendingAuthId, detection, clientId, clientName } = params;- clientName: clientId, + clientName: clientName ?? clientId,libs/auth/src/cimd/__tests__/cimd.service.test.ts (3)
248-266: Avoidas anytype assertions.The coding guidelines specify avoiding
anytypes without strong justification. Theseas anycasts could hide type mismatches between test data and the expectedClientMetadataDocumenttype.♻️ Suggested fix
Import the proper type and ensure the test data conforms to it:
+import type { ClientMetadataDocument } from '../cimd.types'; + +// At line 20, type the sample document properly: -const validCimdDocument = { +const validCimdDocument: ClientMetadataDocument = { client_id: 'https://example.com/oauth/client-metadata.json', // ... rest of fields }; -expect(() => service.validateRedirectUri('https://example.com/callback', validCimdDocument as any)).not.toThrow(); +expect(() => service.validateRedirectUri('https://example.com/callback', validCimdDocument)).not.toThrow();
167-173: Addinstanceofchecks for error classes.Per coding guidelines, tests should include
instanceofchecks to verify proper error hierarchy. This helps ensure errors are correctly constructed and can be caught by type.♻️ Suggested approach
it('should throw CimdFetchError on network error', async () => { mockFetch.mockRejectedValueOnce(new Error('Network error')); - await expect(service.resolveClientMetadata('https://example.com/oauth/client-metadata.json')).rejects.toThrow( - CimdFetchError, - ); + await expect(service.resolveClientMetadata('https://example.com/oauth/client-metadata.json')).rejects.toSatisfy( + (error) => error instanceof CimdFetchError, + ); });Apply similar pattern for
CimdValidationError,CimdClientIdMismatchError, andRedirectUriMismatchErrortests throughout the file.
297-321: UsemockResolvedValueOnceinstead ofmockResolvedValuefor isolation.Using
mockResolvedValue(line 299) sets a persistent mock that could leak into other tests if assertions fail early. The pattern used elsewhere in this file (mockResolvedValueOnce) is safer.♻️ Suggested fix
it('should clear all cache', async () => { // Populate cache - mockFetch.mockResolvedValue({ + mockFetch.mockResolvedValueOnce({ ok: true, // ... });libs/testing/src/auth/__tests__/mock-oauth-server.test.ts (1)
128-140: Consider adding null check before using non-null assertion.Line 135 uses
location!which could cause confusing failures if the mock server behavior changes. Adding a guard assertion improves test diagnostics.♻️ Suggested improvement
const location = response.headers.get('location'); expect(location).toBeDefined(); +expect(location).not.toBeNull(); const redirectUrl = new URL(location!);Or use a type guard:
const location = response.headers.get('location'); expect(location).toBeDefined(); if (!location) throw new Error('Location header missing'); const redirectUrl = new URL(location);libs/auth/src/cimd/__tests__/cimd.validator.test.ts (1)
76-92: Addinstanceofchecks to verify error hierarchy.Per coding guidelines, error class tests should include
instanceofchecks. This ensures the error classes are properly constructed and maintain the expected inheritance chain.♻️ Example fix
it('should throw InvalidClientIdUrlError for HTTP URL', () => { - expect(() => validateClientIdUrl('http://example.com/client')).toThrow(InvalidClientIdUrlError); + try { + validateClientIdUrl('http://example.com/client'); + fail('Expected InvalidClientIdUrlError to be thrown'); + } catch (error) { + expect(error).toBeInstanceOf(InvalidClientIdUrlError); + expect((error as InvalidClientIdUrlError).message).toMatch(/HTTPS/); + } expect(() => validateClientIdUrl('http://example.com/client')).toThrow(/HTTPS/); });Or use Jest's
toSatisfy:expect(() => validateClientIdUrl('http://example.com/client')).toThrow( expect.objectContaining({ constructor: InvalidClientIdUrlError }) );libs/sdk/src/auth/flows/oauth.callback.flow.ts (1)
356-361: Silent fallback when provider not configured may mask configuration errors.When
providerConfigis not found (line 357), the code logs a warning and returns silently (line 360), falling through to normal auth. This could mask configuration issues in production.Consider whether this should be an error in non-development environments, or at least emit a more prominent warning:
if (!providerConfig) { - // Provider not configured yet - for now, fall back to normal auth - this.logger.warn(`Provider ${firstProviderId} not configured, falling back to normal auth`); - return; + // In production, this indicates a misconfiguration + this.logger.error(`Provider ${firstProviderId} not configured - federated login cannot proceed`); + this.respond( + httpRespond.html( + this.renderErrorPage('server_error', `Authentication provider not configured: ${firstProviderId}`), + 500, + ), + ); + return; }libs/testing/src/auth/mock-oauth-server.ts (1)
176-197: Mutating options via type assertion is fragile.Lines 180, 187, 194-196 cast
this.optionstoMockOAuthServerOptionsand mutate it directly. Sincethis.optionsis already typed asMockOAuthServerOptions, the cast is redundant. If the intent is to allow runtime modification, consider making these fields mutable in the interface or using a separate mutable state object.♻️ Suggested approach
+/** Mutable runtime state separate from initial options */ +private runtimeConfig: { + autoApprove: boolean; + testUser?: MockTestUser; + validRedirectUris: string[]; +}; constructor(tokenFactory: TestTokenFactory, options: MockOAuthServerOptions = {}) { this.tokenFactory = tokenFactory; this.options = options; + this.runtimeConfig = { + autoApprove: options.autoApprove ?? false, + testUser: options.testUser, + validRedirectUris: [...(options.validRedirectUris ?? [])], + }; // ... } setAutoApprove(enabled: boolean): void { - (this.options as MockOAuthServerOptions).autoApprove = enabled; + this.runtimeConfig.autoApprove = enabled; }libs/auth/src/cimd/cimd.validator.ts (2)
207-257: Comprehensive IPv4 SSRF protection with good coverage of common private ranges.The function correctly blocks loopback, RFC 1918 private ranges, link-local, current network, broadcast, and multicast addresses. For additional hardening, consider also blocking documentation addresses (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24) and shared address space (100.64.0.0/10), though these are edge cases.
262-297: Good IPv6 SSRF protection covering common private/reserved ranges.The function correctly blocks loopback, unspecified, link-local, and unique local addresses. The IPv4-mapped address handling via regex delegation to
checkIpv4is a good approach. Note that the regex (::ffff:n.n.n.n) may not catch hex-encoded IPv4-mapped addresses (e.g.,::ffff:7f00:1), though these are uncommon in practice.libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts (2)
50-64: Replacez.any()with proper typed schemas for flow state.The state schema uses
z.any()forfederatedSession,providerTokens, andproviderUserInfo, which bypasses type safety. Per coding guidelines, avoidanytypes without strong justification.Suggested improvement
Consider defining proper schemas or using
z.unknown()with runtime type guards:const stateSchema = z.object({ // From URL params providerId: z.string().optional(), // From query params code: z.string().optional(), error: z.string().optional(), errorDescription: z.string().optional(), providerState: z.string().optional(), // Federated session federatedSessionId: z.string().optional(), - federatedSession: z.any().optional(), // FederatedAuthSession + federatedSession: z.unknown().optional(), // FederatedAuthSession - validated at runtime // Provider tokens - providerTokens: z.any().optional(), // ProviderTokens - providerUserInfo: z.any().optional(), // ProviderUserInfo + providerTokens: z.unknown().optional(), // ProviderTokens - validated at runtime + providerUserInfo: z.unknown().optional(), // ProviderUserInfo - validated at runtime });Based on coding guidelines: "Use
unknowninstead ofanyfor generic type parameter defaults."
479-488: Consider using@frontmcp/utilsfor PKCE verifier generation.Per coding guidelines, cryptographic operations should use
@frontmcp/utilsinstead of direct crypto APIs. The file already importsrandomUUIDandsha256Base64urlfrom@frontmcp/utils(line 46).Suggested approach
Check if
@frontmcp/utilsprovides a PKCE verifier generator orrandomBytesthat can be used here:+import { randomBytes } from '@frontmcp/utils'; + private generatePkceVerifier(): string { const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~'; let verifier = ''; - const randomValues = new Uint8Array(64); - crypto.getRandomValues(randomValues); + const randomValues = randomBytes(64); for (const value of randomValues) { verifier += chars[value % chars.length]; } return verifier; }Based on coding guidelines: "Use
@frontmcp/utilsfor cryptographic operations instead ofnode:cryptodirectly."
apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-repos.tool.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@libs/sdk/src/auth/flows/oauth.callback.flow.ts`:
- Around line 295-301: The type guard is checking for 'createSession' on
sessionStore but the flow actually calls sessionStore.store() and
sessionStore.update(); change the guard to verify the actual methods used (check
'store' in sessionStore && 'update' in sessionStore) or assert the correct
SessionStore interface for the sessionStore variable, and update the error
log/message to reflect "Federated session store not configured (missing
store/update)"; ensure references to sessionStore.store and sessionStore.update
remain valid after the fix.
In `@libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts`:
- Around line 50-64: The stateSchema currently uses z.any() for
federatedSession, providerTokens, and providerUserInfo which bypasses
type-safety; update stateSchema to replace z.any() with either z.unknown() for
those three fields or, preferably, define and reference explicit Zod schemas
(e.g., FederatedAuthSessionSchema, ProviderTokensSchema, ProviderUserInfoSchema)
and use them instead so stateSchema.federatedSession,
stateSchema.providerTokens, and stateSchema.providerUserInfo are strongly typed.
- Around line 140-145: The logger currently emits the full providerState (which
contains a session ID) in the oauth provider callback flow; in the method
handling the callback (look for providerState, stateParts and the
this.logger.warn call in oauth.provider-callback.flow.ts) replace the full value
with a masked or truncated form before logging (e.g., show only last 4 chars or
replace middle with ****), so change the this.logger.warn to log a
sanitizedProviderState variable instead while keeping the rest of the control
flow (respond, renderErrorPage) unchanged.
In `@libs/testing/src/auth/mock-oauth-server.ts`:
- Line 1014: The form posts to /oauth/authorize/submit but handleRequest lacks a
route for that path, causing 404s; add a POST route case in handleRequest for
"/oauth/authorize/submit" and implement a new handler function
handleAuthorizeSubmit that parses the POST body (username/password or selected
consent), validates/creates the authorization code, stores any necessary state,
and returns a redirect to the client callback (including code and state)
matching the existing authorize flow; ensure you reuse existing helpers for code
generation/storage and follow the same query/redirect parameter names used by
the authorize endpoint.
🧹 Nitpick comments (15)
libs/testing/src/auth/mock-oauth-server.ts (3)
176-197: Unnecessary type casts onthis.options.The casts
(this.options as MockOAuthServerOptions)are redundant sincethis.optionsis already typed asMockOAuthServerOptions(line 148).✨ Simplified implementation
setAutoApprove(enabled: boolean): void { - (this.options as MockOAuthServerOptions).autoApprove = enabled; + this.options.autoApprove = enabled; } setTestUser(user: MockTestUser): void { - (this.options as MockOAuthServerOptions).testUser = user; + this.options.testUser = user; } addValidRedirectUri(uri: string): void { - const uris = (this.options as MockOAuthServerOptions).validRedirectUris ?? []; + const uris = this.options.validRedirectUris ?? []; uris.push(uri); - (this.options as MockOAuthServerOptions).validRedirectUris = uris; + this.options.validRedirectUris = uris; }
786-814: UserInfo endpoint doesn't validate the access token.The endpoint checks for a Bearer token prefix but doesn't verify the token's validity or extract the user from it—it always returns the configured
testUser. This is likely intentional for a mock server, but consider adding a comment clarifying this behavior for future maintainers.📝 Documentation suggestion
private async handleUserInfoEndpoint(req: IncomingMessage, res: ServerResponse): Promise<void> { // Extract Bearer token from Authorization header const authHeader = req.headers['authorization']; if (!authHeader?.startsWith('Bearer ')) { res.writeHead(401, { 'WWW-Authenticate': 'Bearer error="invalid_token"' }); res.end(JSON.stringify({ error: 'invalid_token', error_description: 'Missing or invalid Authorization header' })); return; } - // For mock server, we'll return the test user if configured + // NOTE: For simplicity, the mock server does not validate the token. + // It always returns the configured testUser regardless of token content. + // This is sufficient for E2E testing where token issuance is controlled. const testUser = this.options.testUser;
49-49: Replacenode:cryptoimports with@frontmcp/utilsfor consistency.The coding guidelines require using
@frontmcp/utilsfor cryptographic operations. Replace thecreateHashandrandomBytesimports from'crypto'with equivalents from@frontmcp/utils:
randomBytes→ directly available from@frontmcp/utilscreateHash('sha256').update(data).digest('base64url')→ usesha256Base64url()from@frontmcp/utilslibs/sdk/src/auth/flows/well-known.oauth-authorization-server.flow.ts (1)
104-104: Use a specific error class with MCP error code instead of generic Error.Per coding guidelines for
libs/sdk, this should use a specific error class with an MCP error code for proper error handling and client diagnostics.♻️ Suggested improvement
- if (!request) throw new Error('Request is undefined'); + if (!request) throw new InvalidRequestError('Request is undefined');Import the appropriate error class from the SDK's error module.
libs/auth/src/cimd/cimd-redis.cache.ts (3)
54-57: Use a specific error class instead of genericError.Per coding guidelines for
libs/{sdk,adapters}/**, use specific error classes with MCP error codes instead of generic errors. Consider using or creating a CIMD-specific configuration error.Suggested fix
+import { CimdError } from './cimd.errors'; + +// In constructor: - if (!config.redis) { - throw new Error('Redis configuration is required for RedisCimdCache'); - } + if (!config.redis) { + throw new CimdError( + 'Redis configuration is required for RedisCimdCache', + 'CIMD_CONFIG_ERROR', + 500, + ); + }
290-315: Consider batching cleanup operations for better performance.The
cleanup()method fetches all keys, then individually gets and potentially deletes each one. For large caches, this creates O(n) Redis round-trips. Consider using a pipeline or batching approach.Suggested optimization using mget for batching
async cleanup(): Promise<number> { const now = Date.now(); let removed = 0; const keys = await this.redis.keys(`${this.keyPrefix}*`); + if (keys.length === 0) return 0; - for (const key of keys) { - const data = await this.redis.get(key); - if (!data) continue; - - try { - const entry = JSON.parse(data) as SerializedCimdCacheEntry; - // Remove entries that are very old (2x max TTL past expiration) - if (entry.expiresAt + this.config.maxTtlMs * 2 < now) { - await this.redis.delete(key); - removed++; - } - } catch { - // Invalid JSON, remove it - await this.redis.delete(key); - removed++; - } - } + const keysToDelete: string[] = []; + // Process in batches to reduce memory pressure + const BATCH_SIZE = 100; + for (let i = 0; i < keys.length; i += BATCH_SIZE) { + const batchKeys = keys.slice(i, i + BATCH_SIZE); + for (const key of batchKeys) { + const data = await this.redis.get(key); + if (!data) continue; + try { + const entry = JSON.parse(data) as SerializedCimdCacheEntry; + if (entry.expiresAt + this.config.maxTtlMs * 2 < now) { + keysToDelete.push(key); + } + } catch { + keysToDelete.push(key); + } + } + } + if (keysToDelete.length > 0) { + await this.redis.mdelete(keysToDelete); + removed = keysToDelete.length; + } return removed; }
116-131: Silent error handling may hide issues in production.The catch block silently deletes entries with invalid JSON but doesn't log this, which could make debugging difficult in production. Consider logging at debug level.
Suggested improvement
} catch { // Invalid JSON, delete and return undefined + // Note: Consider adding a logger parameter to RedisCimdCache for debugging await this.redis.delete(key); return undefined; }libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts (2)
424-424: Track skipped providers properly.Line 424 has a TODO comment indicating skipped providers aren't tracked properly. This could affect audit logging and user consent tracking in orchestrated auth flows.
Would you like me to open an issue to track implementing proper skipped provider tracking? This would involve:
- Adding a
skippedProvidersfield toFederatedAuthSession- Updating
storeProviderTokensto track declined providers- Including skipped providers in the authorization code data
468-474: Consider using the full SHA-256 hash for user sub or document the collision trade-off.The 16-character truncation (96 bits) provides only ~48 bits of collision resistance via birthday attack, with collisions becoming likely after ~2.8×10^14 generates. This is weaker than modern recommendations (128–256 bits). Additionally,
oauth.callback.flow.tsuses the full hash without truncation, creating inconsistency. Either standardize on the full hash for better collision resistance or add a comment explaining why truncation is acceptable for this use case.libs/sdk/src/auth/session/federated-auth.session.ts (2)
424-426: Avoid non-null assertion; use explicit guard instead.Per coding guidelines, avoid non-null assertions (
!). Although there's a length check on line 420, the assertion on line 425 can be replaced with proper handling.Suggested fix
// Pop from queue and set as current - const providerId = session.providerQueue.shift()!; + const providerId = session.providerQueue.shift(); + if (!providerId) { + throw new Error('No more providers in queue'); + } session.currentProviderId = providerId;
284-311: Duplicate session creation logic betweencreateSessionmethod andcreateFederatedAuthSessionfunction.The
InMemoryFederatedAuthSessionStore.createSessionmethod (lines 284-311) and the standalonecreateFederatedAuthSessionfunction (lines 337-367) contain nearly identical logic. Consider having one delegate to the other or extracting a shared helper.Suggested refactor
createSession(params: { pendingAuthId: string; clientId: string; redirectUri: string; scopes: string[]; state?: string; resource?: string; userInfo: { email?: string; name?: string; sub?: string }; frontmcpPkce: { challenge: string; method: 'S256' }; providerIds: string[]; }): FederatedAuthSession { - const now = Date.now(); - return { - id: randomUUID(), - pendingAuthId: params.pendingAuthId, - clientId: params.clientId, - redirectUri: params.redirectUri, - scopes: params.scopes, - state: params.state, - resource: params.resource, - userInfo: params.userInfo, - frontmcpPkce: params.frontmcpPkce, - providerQueue: [...params.providerIds], - completedProviders: new Map(), - createdAt: now, - expiresAt: now + this.sessionTtlMs, - }; + return createFederatedAuthSession(params, this.sessionTtlMs); }Also applies to: 337-367
libs/auth/src/cimd/__tests__/cimd.service.test.ts (2)
167-173: Addinstanceofchecks for error class verification.Per coding guidelines for test files, tests should include
instanceofchecks for error classes to verify proper error hierarchy. The error tests only check that specific errors are thrown but don't verify the error class hierarchy.Suggested enhancement for one of the error tests
it('should throw CimdFetchError on network error', async () => { mockFetch.mockRejectedValueOnce(new Error('Network error')); - await expect(service.resolveClientMetadata('https://example.com/oauth/client-metadata.json')).rejects.toThrow( - CimdFetchError, - ); + try { + await service.resolveClientMetadata('https://example.com/oauth/client-metadata.json'); + fail('Expected CimdFetchError to be thrown'); + } catch (error) { + expect(error).toBeInstanceOf(CimdFetchError); + expect(error).toBeInstanceOf(Error); + expect((error as CimdFetchError).code).toBe('CIMD_FETCH_ERROR'); + } });Also applies to: 175-186, 188-215, 217-244
248-266: Avoidas anytype assertions in tests.Per coding guidelines, avoid
anytypes. ThevalidCimdDocument as anyassertions can be replaced with proper typing by ensuring the mock document matches theClientMetadataDocumenttype.Suggested fix
+import type { ClientMetadataDocument } from '../cimd.types'; // Sample valid CIMD document -const validCimdDocument = { +const validCimdDocument: ClientMetadataDocument = { client_id: 'https://example.com/oauth/client-metadata.json', client_name: 'Test Client', redirect_uris: ['https://example.com/callback', 'http://localhost:3000/callback'], token_endpoint_auth_method: 'none', grant_types: ['authorization_code'], response_types: ['code'], }; // Then remove `as any` assertions: - expect(() => service.validateRedirectUri('https://example.com/callback', validCimdDocument as any)).not.toThrow(); + expect(() => service.validateRedirectUri('https://example.com/callback', validCimdDocument)).not.toThrow();libs/sdk/src/auth/flows/oauth.callback.flow.ts (2)
356-361: Orphaned session on provider config fallback.If the provider is not configured, the code returns early (line 360) but leaves the federated session already stored in the session store (line 331). This orphaned session will persist until TTL expiration. Consider deleting the session on fallback to normal auth.
♻️ Suggested improvement
const providerConfig = localAuth.getProviderConfig(firstProviderId); if (!providerConfig) { // Provider not configured yet - for now, fall back to normal auth this.logger.warn(`Provider ${firstProviderId} not configured, falling back to normal auth`); + // Clean up the orphaned federated session + await sessionStore.delete(federatedSession.id); return; }
369-378: Session cleanup missing on error path.Similar to the fallback case, if
buildProviderAuthorizeUrlfails, the federated session remains in the store. Add cleanup for consistency.♻️ Suggested improvement
if (!redirectUrl) { this.logger.error(`Failed to build authorize URL for provider: ${firstProviderId}`); + // Clean up the orphaned federated session + await sessionStore.delete(federatedSession.id); this.respond( httpRespond.html( this.renderErrorPage('server_error', `Failed to initiate auth with provider: ${firstProviderId}`), 500, ), ); return; }
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/testing/src/auth/mock-oauth-server.ts (1)
508-538:clientSecretis declared but never enforced.
MockOAuthServerOptions.clientSecretand the metadata advertiseclient_secret_basic/client_secret_post, yet the token endpoint never validates a client secret. This can break confidential-client tests or mislead callers. Either implement secret validation (basic/post) or remove it from the advertised auth methods/options.
🤖 Fix all issues with AI agents
In `@libs/sdk/src/auth/flows/oauth.callback.flow.ts`:
- Around line 283-328: The code currently trusts selectedProviders from the
query params; before creating the federated session (createFederatedAuthSession)
validate that the pending auth request (this.state.required.pendingAuth) exists
and has federatedLogin === true and an allowed provider list, then ensure
selectedProviders is a subset of that allowedProviderIds; if the checks fail,
log via this.logger.error and return an appropriate error response instead of
proceeding (also use this.state.required.pendingAuthId when present).
- Around line 355-377: The federated session is left orphaned when
getProviderConfig(firstProviderId) returns falsy or
buildProviderAuthorizeUrl(...) fails; before each early return add cleanup to
remove or mark the federated session as failed in your session store (use the
flow's session variable and the existing session store API — e.g., call
sessionStore.delete(session.id) or sessionStore.update(session.id, { status:
'failed' })). Do this in the branches after
localAuth.getProviderConfig(firstProviderId) check and after the redirectUrl
falsy check (before calling this.respond / renderErrorPage) so the session isn't
left in storage.
- Around line 65-68: The plan constant (typed as FlowPlan<string>) for the
/oauth/callback now includes handleFederatedAuth in execute alongside
handleIncrementalAuth, createAuthorizationCode, and redirectToClient but this
behavior is undocumented; update the draft authentication docs to describe the
callback flow steps, enumerate the plan stages (pre: parseInput,
validatePendingAuth; execute: handleIncrementalAuth, handleFederatedAuth,
createAuthorizationCode, redirectToClient), and explain how handleFederatedAuth
interacts with incremental auth and the generation of an authorization code when
FlowRunOptions exposes the public FlowPlan<string>; include expected
inputs/outputs, ordering guarantees, and any error/redirect behavior callers
should expect.
In `@libs/testing/src/auth/mock-oauth-server.ts`:
- Around line 822-869: handleAuthorizeSubmit currently accepts any client_id and
performs the deny redirect before validating redirect_uri; reorder and tighten
validation so redirect_uri is validated first (using isValidRedirectUri)
immediately after parsing params, then validate client_id against configured
clients (if your server enforces client registration) before acting on action;
ensure the deny branch uses redirectWithError only after redirect_uri and
client_id validations pass so you don't redirect to an unvalidated URI or accept
unknown clients.
- Around line 438-454: The current flow validates client_id before redirect_uri
causing a potential redirect to an untrusted location; update the order in the
authorize handler so you call isValidRedirectUri(redirectUri) first and return a
400 JSON error if it fails, and only then validate this.options.clientId against
clientId and call redirectWithError(res, redirectUri, ...) when the redirectUri
is known-valid; ensure the redirectWithError, options.clientId, clientId and
redirectUri checks remain but are executed after successful isValidRedirectUri.
- Around line 56-120: The new exported types and class (MockTestUser,
MockOAuthServerOptions, MockOAuthServerInfo, and MockOAuthServer) are public SDK
APIs but lack documentation; add corresponding docs under
docs/draft/docs/testing/ (either update api-reference.mdx or create a dedicated
OAuth testing guide) that document each exported interface and the
MockOAuthServer class, include descriptions of all fields (e.g., sub, email,
claims, port, issuer, autoApprove, clientId, accessTokenTtlSeconds,
refreshTokenTtlSeconds), example usage for spinning up the mock server, and any
behavioral notes (auto-approve flow, expected client validation, redirect URI
wildcard semantics) so the public API surface is fully covered.
- Around line 48-50: Replace direct Node crypto usage in this file: remove
imports of createHash and randomBytes from 'crypto' and instead import
randomBytes, base64urlEncode, and sha256Base64url from '@frontmcp/utils'; then
update all uses where you generate code/verifier and challenge so that any
randomBytes(32).toString('base64url') becomes base64urlEncode(randomBytes(32))
and any createHash('sha256').update(verifier).digest('base64url') becomes
sha256Base64url(verifier) (look for usages around functions/variables that
create the PKCE verifier/challenge and where random tokens are generated, e.g.,
the code that constructs the PKCE verifier and token values).
♻️ Duplicate comments (2)
libs/sdk/src/auth/cimd/README.md (2)
371-390: Add blank lines around the IPv4/IPv6 tables to satisfy MD058.markdownlint expects a blank line before and after tables.
🔧 Suggested fix
**IPv4 Blocked Ranges:** + | Range | Description | |-------|-------------| | `127.0.0.0/8` | Loopback addresses | | `10.0.0.0/8` | Private Class A | | `172.16.0.0/12` | Private Class B (172.16.x.x - 172.31.x.x) | | `192.168.0.0/16` | Private Class C | | `169.254.0.0/16` | Link-local addresses | | `0.0.0.0/8` | Current network | | `224.0.0.0/4` | Multicast (224.x.x.x - 239.x.x.x) | | `255.255.255.255` | Broadcast | + **IPv6 Blocked Ranges:** + | Range | Description | |-------|-------------| | `::1` | Loopback | | `::` | Unspecified | | `fe80::/10` | Link-local | | `fc00::/7` | Unique local (ULA) | | `::ffff:x.x.x.x` | IPv4-mapped (checked against IPv4 rules) | +
432-448: Specify languages on fenced code blocks (MD040).Add a language tag to the Cache-Control and conditional request examples.
🔧 Suggested fix
-``` +```http Cache-Control: max-age=3600 → Cache for 1 hour Cache-Control: s-maxage=7200 → Cache for 2 hours (shared cache) Cache-Control: no-cache → Use minimum TTL Cache-Control: no-store → Use minimum TTL@@
-+http
If-None-Match: "abc123" → Server returns 304 if unchanged
If-Modified-Since: Wed, 01 Jan... → Server returns 304 if unchanged
🧹 Nitpick comments (1)
docs/draft/docs/authentication/cimd.mdx (1)
401-406: Clarifyno-storecaching semantics to avoid confusion.
Cache-Control: no-storetypically means “do not cache at all,” but the doc currently maps it to “Use minimum TTL.” If this is intentional behavior, consider explicitly noting that it deviates from standard semantics; otherwise, update the text to reflect non-caching to avoid misinterpretation.✏️ Suggested wording tweak
-Cache-Control: no-store → Use minimum TTL +Cache-Control: no-store → Do not cache (or note if implementation maps to min TTL intentionally)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/draft/docs/authentication/local.mdx`:
- Around line 81-93: Minor style issue: add a period to "etc" in the table cell.
Edit the OAuth Callback Flow table in docs/draft/docs/authentication/local.mdx
and update the `parseInput` description text from "Extract query parameters
(email, pending_auth_id, etc)" to use "etc." (i.e., append a period). Ensure
only the punctuation changes and other symbols like `parseInput`,
`validatePendingAuth`, `handleIncrementalAuth`, `handleFederatedAuth`,
`createAuthorizationCode`, and `redirectToClient` remain unchanged.
In `@libs/testing/tsconfig.lib.json`:
- Around line 9-14: Add a wildcard path mapping for the `@frontmcp/sdk` package so
its sub-path exports (e.g., ./transport) resolve during TS compilation: update
the "paths" block in the tsconfig (the JSON that currently lists
"@frontmcp/sdk": ["libs/sdk/dist/index.d.ts"]) to include a mapping for
"@frontmcp/sdk/*" that points to the built declarations (for example,
"libs/sdk/dist/*/index.d.ts") so sub-path imports resolve correctly.
♻️ Duplicate comments (3)
libs/testing/src/auth/mock-oauth-server.ts (2)
56-120: Add documentation for new public API types.The new exported interfaces (
MockTestUser,MockOAuthServerOptions,MockOAuthServerInfo) and theMockOAuthServerclass with its new public methods (setAutoApprove,setTestUser,addValidRedirectUri,clearStoredTokens) are public SDK APIs. Per coding guidelines forlibs/**, add matching documentation updates todocs/draft/docs/testing/.
853-869: Addclient_idvalidation to align withhandleAuthorizeEndpoint.
handleAuthorizeEndpointvalidatesclient_idagainstthis.options.clientId(lines 450-454), buthandleAuthorizeSubmitdoes not. This inconsistency means the manual login flow accepts anyclient_ideven when the server is configured to require a specific one.🔧 Suggested fix
// Validate redirect_uri FIRST (before any redirects to prevent open redirect) if (!this.isValidRedirectUri(redirectUri)) { res.writeHead(400, { 'Content-Type': 'application/json' }); res.end( JSON.stringify({ error: 'invalid_request', error_description: 'Invalid redirect_uri', }), ); return; } + // Validate client_id if configured (safe to redirect now that redirect_uri is validated) + if (this.options.clientId && clientId !== this.options.clientId) { + this.redirectWithError(res, redirectUri, 'unauthorized_client', 'Invalid client_id', state); + return; + } + // Handle deny action (safe to redirect now that redirect_uri is validated) if (action === 'deny') {libs/sdk/src/auth/flows/oauth.callback.flow.ts (1)
191-201: Block federated flow when it wasn’t enabled (and require a provider selection).Right now, a request can set
federated=trueeven if the pending auth didn’t includefederatedLogin, or pass an empty provider list. That enables a federated path without server-side intent. Reject both cases early.🔒 Proposed fix
- if (isFederated && pendingAuth.federatedLogin) { + if (isFederated && !pendingAuth.federatedLogin) { + this.logger.warn('Federated login not enabled for this authorization request'); + this.respond(httpRespond.html(this.renderErrorPage('invalid_request', 'Federated login not enabled'), 400)); + return; + } + + if (isFederated && pendingAuth.federatedLogin) { + if (!selectedProviders || selectedProviders.length === 0) { + this.logger.warn('No federated providers selected'); + this.respond(httpRespond.html(this.renderErrorPage('invalid_request', 'No federated providers selected'), 400)); + return; + } const allProviders = pendingAuth.federatedLogin.providerIds; - const selected = selectedProviders || []; + const selected = selectedProviders;
🧹 Nitpick comments (2)
libs/testing/src/auth/mock-oauth-server.ts (2)
179-197: Remove redundant type casts.The type casts
(this.options as MockOAuthServerOptions)are unnecessary sincethis.optionsis already typed asMockOAuthServerOptions.♻️ Suggested fix
setAutoApprove(enabled: boolean): void { - (this.options as MockOAuthServerOptions).autoApprove = enabled; + this.options.autoApprove = enabled; } setTestUser(user: MockTestUser): void { - (this.options as MockOAuthServerOptions).testUser = user; + this.options.testUser = user; } addValidRedirectUri(uri: string): void { - const uris = (this.options as MockOAuthServerOptions).validRedirectUris ?? []; + const uris = this.options.validRedirectUris ?? []; uris.push(uri); - (this.options as MockOAuthServerOptions).validRedirectUris = uris; + this.options.validRedirectUris = uris; }Note: This requires changing
readonly optionsto allow mutation, or restructuring to use a separate mutable config object.
788-816: Consider validating the access token for higher-fidelity testing.The endpoint extracts the Bearer token but doesn't validate it against issued tokens. While acceptable for a mock server, tests won't catch scenarios where the wrong or expired token is sent. If higher fidelity is needed later, consider storing issued access tokens and validating them.
…and enhance OAuth server validation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-repos.tool.ts`:
- Around line 37-52: In execute, distinguish unauthenticated vs unauthorized:
use this.getAuthInfo() / user (the existing getAuthInfo and user variables) to
first check if authInfo or user is missing and return a clear unauthenticated
response (e.g., success:false, tokenReceived:false, error:'User not
authenticated') and only afterwards evaluate hasGitHub (the
federatedClaims.selectedProviders check) and return the existing 'GitHub
provider not authorized' when the user is authenticated but github was not
selected; update the execute function's early-return logic accordingly to match
github-user tool semantics.
In `@libs/sdk/src/auth/flows/oauth.callback.flow.ts`:
- Around line 318-324: The type guard for the federated session store currently
checks sessionStore.store and sessionStore.update but not sessionStore.delete;
update the guard in oauth.callback.flow.ts to also verify that
sessionStore.delete is a function (e.g., include typeof sessionStore.delete ===
'function') before proceeding, and keep the existing error log/response
(this.logger.error('Federated session store not configured') /
this.respond(...)) if the guard fails so subsequent calls to sessionStore.delete
in this flow are safe.
♻️ Duplicate comments (2)
docs/draft/docs/authentication/local.mdx (1)
81-93: Clear documentation of the OAuth callback flow stages.The callback flow documentation is well-structured and provides a useful reference for developers. The "etc." style fix from the previous review has been addressed.
libs/testing/src/auth/mock-oauth-server.ts (1)
56-70: Add documentation for the new publicMockTestUserinterface.This is a newly exported public API type from a publishable SDK library. Per coding guidelines for
libs/**, when public APIs change, ensure there is a matching documentation update indocs/draft/docs/. Consider adding JSDoc descriptions for each property beyond the brief inline comments.
🧹 Nitpick comments (3)
docs/draft/docs/authentication/local.mdx (1)
94-106: Consider clarifying required vs optional parameters.The parameter table is helpful. Currently only
nameis marked as optional—consider adding similar annotations to other optional parameters (e.g.,incremental,federated,providers,tools) to help developers understand which combinations are valid for different flows.libs/testing/src/auth/mock-oauth-server.ts (1)
788-804: Consider documenting that token validation is not performed.The userinfo endpoint accepts any Bearer token and returns the configured test user without validating the token. This is acceptable for a mock server, but consider adding a comment to clarify this behavior for test authors who might expect token validation.
📝 Suggested documentation addition
private async handleUserInfoEndpoint(req: IncomingMessage, res: ServerResponse): Promise<void> { - // Extract Bearer token from Authorization header + // Extract Bearer token from Authorization header + // Note: Token is not validated - any Bearer token returns the configured test user const authHeader = req.headers['authorization'];apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-repos.tool.ts (1)
10-12: Constrainlimitto valid values.
A negative or fractional limit will silently return empty/odd slices. Consider enforcing an integer minimum (and optional max) at the schema level.♻️ Proposed update
-const inputSchema = z.object({ - limit: z.number().optional().default(10).describe('Maximum number of repos to list'), -}); +const inputSchema = z.object({ + limit: z + .number() + .int() + .min(1) + .optional() + .default(10) + .describe('Maximum number of repos to list'), +});
apps/e2e/demo-e2e-orchestrated/src/apps/github/tools/github-repos.tool.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/testing/src/auth/mock-oauth-server.ts (1)
508-783: EnforceclientSecretwhen configured.
MockOAuthServerOptions.clientSecretis exposed but never checked, so tests can pass even if confidential-client auth is broken. Add a simple validation before issuing tokens.🔧 Suggested fix
private async handleTokenEndpoint(req: IncomingMessage, res: ServerResponse): Promise<void> { // Parse request body const body = await this.readBody(req); const params = new URLSearchParams(body); + const authHeader = req.headers['authorization']; + let clientSecret = params.get('client_secret') ?? undefined; + if (!clientSecret && authHeader?.startsWith('Basic ')) { + const decoded = Buffer.from(authHeader.slice(6), 'base64').toString('utf8'); + const [, secret] = decoded.split(':', 2); + clientSecret = secret; + } + if (this.options.clientSecret && clientSecret !== this.options.clientSecret) { + res.writeHead(401, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'invalid_client', error_description: 'Invalid client_secret' })); + return; + } const grantType = params.get('grant_type');
🤖 Fix all issues with AI agents
In `@libs/sdk/src/auth/flows/oauth.callback.flow.ts`:
- Around line 283-412: The handleFederatedAuth flow leaves request state marked
as federated when a provider is missing; when
localAuth.getProviderConfig(firstProviderId) is falsy you must clear federated
indicators (e.g. this.state.isFederated = false and this.state.selectedProviders
= undefined or empty) or respond with an error before returning so downstream
stages don't treat it as federated; update the branch that logs the warn/await
sessionStore.delete(...) to also clear the federated flags on this.state (and/or
call this.respond with an error page) prior to returning.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/testing/src/auth/mock-oauth-server.ts`:
- Around line 516-520: The Basic auth parsing for clientSecret (in the block
using authHeader, Buffer.decode and the variable clientSecret) incorrectly uses
split(':', 2) which truncates secrets that contain colons; change the logic to
decode the header, find the first ':' via indexOf on the decoded string, then
take everything after that first colon as the secret (handle missing colon
safely) so clientSecret correctly preserves any additional colons.
♻️ Duplicate comments (1)
libs/testing/src/auth/mock-oauth-server.ts (1)
56-120: External documentation still needed for public API types.The public interfaces (
MockTestUser,MockOAuthServerOptions,MockOAuthServerInfo) and theMockOAuthServerclass are exported SDK APIs. Per coding guidelines forlibs/**, add matching documentation todocs/draft/docs/testing/.
…s with client secret validation
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/testing/src/auth/mock-oauth-server.ts (1)
355-369: Align advertised response/grant types with actual support.
The mock only supportsresponse_type=codeand does not implementclient_credentials, yet metadata advertises bothtokenandclient_credentials. This can mislead clients and tests.🔧 Suggested fix
- response_types_supported: ['code', 'token'], + response_types_supported: ['code'], @@ - grant_types_supported: ['authorization_code', 'refresh_token', 'client_credentials', 'anonymous'], + grant_types_supported: ['authorization_code', 'refresh_token', 'anonymous'],- response_types_supported: ['code', 'token'], + response_types_supported: ['code'], @@ - grant_types_supported: ['authorization_code', 'refresh_token', 'client_credentials', 'anonymous'], + grant_types_supported: ['authorization_code', 'refresh_token', 'anonymous'],Also applies to: 376-386
🤖 Fix all issues with AI agents
In `@docs/draft/docs/testing/api-reference.mdx`:
- Around line 544-551: The documented MockTestUser interface is inconsistent
with the implementation: update the docs' MockTestUser shape to match the code
by replacing the index signature with a single optional property claims?:
Record<string, unknown>, while keeping required properties like sub and optional
email, name, picture; ensure the docs reference the claims property name exactly
as used in the implementation (claims) and describe its purpose as a container
for additional claims.
- Around line 619-627: The docs list fields on MockOAuthServerInfo that the
actual type doesn't expose; update either the implementation or the docs: either
add the missing properties authorizationEndpoint, tokenEndpoint, and
userinfoEndpoint to the MockOAuthServerInfo type/definition (ensuring types and
any initialization in the mock server code populate
jwksUrl/authorizationEndpoint/tokenEndpoint/userinfoEndpoint accordingly) or
remove those three properties from the docs so MockOAuthServerInfo matches the
real type; reference the MockOAuthServerInfo interface and the specific property
names (authorizationEndpoint, tokenEndpoint, userinfoEndpoint) when making the
change.
- Around line 529-538: The docs incorrectly state refreshTokenTtlSeconds default
as 86400; update the MockOAuthServerOptions documentation to reflect the actual
default of 2592000 seconds (30 days) by changing the inline comment for
refreshTokenTtlSeconds to 2592000s; ensure the identifier refreshTokenTtlSeconds
in MockOAuthServerOptions is the one you update so it matches the code default.
In `@libs/testing/src/auth/mock-oauth-server.ts`:
- Around line 642-666: The PKCE branch in the token handler must reject
unsupported code_challenge_method values instead of silently treating them as
"plain"; before calling computeCodeChallenge, check
codeRecord.codeChallengeMethod and if it is not 'S256' or 'plain' return a 400
JSON error (e.g., error: 'invalid_request' / error_description: 'unsupported
code_challenge_method') so misconfigured clients are flagged; update the logic
around codeRecord.codeChallengeMethod, codeVerifier, and computeCodeChallenge in
mock-oauth-server.ts to enforce this allowed-set check.
…d auth configuration
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/auth/authorization/orchestrated.authorization.ts (1)
247-281: AvoidauthorizedProviderIdsbeing overwritten by...projections.If callers pass
authorizedProviderIds: undefined, it lands inprojectionsand (via spread order) can overridefinalAuthorizedProviderIds, nullifying the fallback to provider states. Consider extracting it fromctxand applying...projectionsbefore the explicit value.🔧 Suggested fix
- const { - token, - user, - scopes = [], - claims, - expiresAt, - primaryProviderId, - tokenStore, - onTokenRefresh, - providers = {}, - ...projections - } = ctx; + const { + token, + user, + scopes = [], + claims, + expiresAt, + primaryProviderId, + tokenStore, + onTokenRefresh, + providers = {}, + authorizedProviderIds, + ...projections + } = ctx; - const finalAuthorizedProviderIds = - projections.authorizedProviderIds ?? (providerIdsFromState.length > 0 ? providerIdsFromState : undefined); + const finalAuthorizedProviderIds = + authorizedProviderIds ?? (providerIdsFromState.length > 0 ? providerIdsFromState : undefined); return new OrchestratedAuthorization({ id, isAnonymous: false, user, claims, expiresAt, scopes, token, primaryProviderId, tokenStore, onTokenRefresh, providerStates, authorizedProviders, - authorizedProviderIds: finalAuthorizedProviderIds, - ...projections, + ...projections, + authorizedProviderIds: finalAuthorizedProviderIds, });libs/sdk/src/auth/flows/auth.verify.flow.ts (1)
403-452: Don’t drop federated provider selections when the token store is empty/unavailable.When
tokenStoreexists,authorizedProviderIdsis set toproviderIdsFromStore ?? [], which ignoresfederatedClaims?.selectedProviderseven if the store fails or is empty. This can incorrectly strip provider authorizations in federated flows. Use federated claims as a fallback if the store yields none.🔧 Suggested fix
- authorizedProviderIds: tokenStore ? (providerIdsFromStore ?? []) : federatedClaims?.selectedProviders, + authorizedProviderIds: + providerIdsFromStore && providerIdsFromStore.length > 0 + ? providerIdsFromStore + : federatedClaims?.selectedProviders,
🤖 Fix all issues with AI agents
In `@docs/draft/docs/testing/api-reference.mdx`:
- Around line 668-670: The example references
oauthServer.info.authorizationEndpoint but MockOAuthServerInfo (used for
oauthServer.info) lacks that property; update MockOAuthServerInfo to include the
standard endpoint URL fields (e.g., authorizationEndpoint, tokenEndpoint,
jwksUri or revocationEndpoint as needed) so the example can reference
oauthServer.info.authorizationEndpoint, or alternatively change the example
comment to use only documented properties like oauthServer.info.baseUrl or
oauthServer.info.issuer—pick one approach and make the corresponding change in
MockOAuthServerInfo or the example comment to keep the API consistent.
In `@libs/sdk/src/auth/authorization/orchestrated.authorization.ts`:
- Around line 153-154: The `authorizedProviderIds` field in the
`OrchestratedAuthorizationCreateCtx` interface lacks documentation coverage for
a public SDK API. Add JSDoc comments to the `authorizedProviderIds` field to
describe its purpose, and update the authentication documentation files in
`docs/draft/docs/authentication/` (such as modes.mdx) to include information
about the `OrchestratedAuthorizationCreateCtx` interface and its fields,
including when and how `authorizedProviderIds` should be used.
In `@libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts`:
- Around line 176-189: The logger.warn in oauth.provider-callback.flow.ts
exposes full state values (session.currentProviderState and providerState);
change it to log masked/truncated versions (e.g., first N chars + "...", or a
hashed/obfuscated snippet) before calling this.logger.warn so the message shows
only a non-sensitive portion, leaving the rest redacted; update the code paths
in getStateValidation/where session.currentProviderState is read and ensure the
same masked variables are used in the log call while keeping the existing
respond(this.renderErrorPage(...)) behavior intact.
In `@libs/sdk/src/common/types/options/auth/index.ts`:
- Line 18: The documentation for orchestration mode is missing an entry for the
new public type FederatedAuthConfig; update the authentication modes MDX
document to add a table row for FederatedAuthConfig alongside ConsentConfig and
IncrementalAuthConfig in the orchestrated mode configuration section, following
the same phrasing and link pattern used for the other config types and
referencing the FederatedAuthConfig type so the public API docs match the code.
In `@libs/sdk/src/common/types/options/auth/interfaces.ts`:
- Around line 212-221: The stateValidation field in the FederatedAuthConfig
interface is marked as optional with the ? modifier, but it must be required to
match the Zod schema definition in shared.schemas.ts where stateValidation is
defined with a default value, making it a required field in the inferred type.
Remove the ? from the stateValidation field declaration in the
FederatedAuthConfig interface to make it required and keep the interface in sync
with its corresponding Zod schema.
🧹 Nitpick comments (8)
libs/testing/src/auth/__tests__/mock-oauth-server.test.ts (3)
11-68: Consider adding test forserver.infoaccess before start.Per the relevant code snippet, accessing
server.infobefore the server is started should throw'Mock OAuth server is not running'. Adding a test for this error path would improve coverage:it('should throw when accessing info before start', () => { const uninitializedServer = new MockOAuthServer(tokenFactory); expect(() => uninitializedServer.info).toThrow('Mock OAuth server is not running'); });Also consider adding a test for
getTokenFactory()to verify it returns the injected factory.
135-140: Replace non-null assertions with proper guards or explicit assertions.Per coding guidelines, avoid non-null assertions (
!). Use type guards or explicit assertions with meaningful error messages instead.Suggested pattern for handling nullable values
- const redirectUrl = new URL(location!); + expect(location).toBeDefined(); + const redirectUrl = new URL(location as string);Or use a helper assertion:
function assertDefined<T>(value: T | null | undefined, message: string): asserts value is T { expect(value).toBeDefined(); } // Usage: assertDefined(location, 'Expected location header'); const redirectUrl = new URL(location);This pattern applies to other non-null assertions throughout the file (lines 152, 177-178, 209-210, 276, 305, 337, 370, 418).
392-454: Consider adding test for invalid refresh token.The refresh token flow tests the happy path well and verifies token rotation. Consider adding a negative test for invalid or revoked refresh tokens:
it('should reject invalid refresh token', async () => { const response = await fetch(`${server.info.baseUrl}/oauth/token`, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: new URLSearchParams({ grant_type: 'refresh_token', refresh_token: 'invalid-refresh-token', client_id: 'refresh-client', }).toString(), }); expect(response.ok).toBe(false); const error = await response.json(); expect(error.error).toBe('invalid_grant'); });libs/auth/src/cimd/cimd.service.ts (1)
145-156: Consider returning entry metadata fromcache.set()to avoid the extra lookup.The pattern of
cache.set()followed immediately bycache.get()creates an unnecessary round-trip to retrieve entry metadata (expiresAt, etag, lastModified). This could be optimized by havingset()return the stored entry.♻️ Potential optimization
// Cache the result - await this.cache.set(clientId, document, headers); - const entry = await this.cache.get(clientId); + const entry = await this.cache.set(clientId, document, headers); return { isCimdClient: true, metadata: document, fromCache: false, expiresAt: entry?.expiresAt, etag: entry?.etag, lastModified: entry?.lastModified, };This would require updating the
CimdCacheBackend.set()interface to return the stored entry.libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts (3)
302-314: Consider structured error logging to avoid leaking sensitive details.Lines 303 and 310 log caught errors directly using template literals. This could expose stack traces or internal error details. Use structured logging or extract only the message.
Suggested fix
} catch (err) { - this.logger.warn(`Failed to get user info from provider ${providerId}: ${err}`); + this.logger.warn(`Failed to get user info from provider ${providerId}`, { error: err instanceof Error ? err.message : 'Unknown error' }); // Continue without user info } } this.logger.info(`Successfully exchanged code for tokens with provider: ${providerId}`); } catch (err) { - this.logger.error(`Provider token exchange error: ${err}`); + this.logger.error('Provider token exchange error', { error: err instanceof Error ? err.message : 'Unknown error' }); this.respond(
345-354: Skipped providers are not being tracked.When a provider is declined (
tokensis null), the code clears the current provider fields but doesn't record it as "skipped". This connects to the TODO on line 440. Consider adding to askippedProvidersset/array in the session for proper tracking.Would you like me to help implement skipped provider tracking, or should I open an issue to track this?
152-152: Consider adding a type guard forLocalPrimaryAuthaccess.The repeated pattern
this.scope.auth as LocalPrimaryAuth(lines 152, 266, 328, 374, 436) assumes the auth instance type without verification. Consider adding a helper method that validates and returns the properly typed instance, or add a runtime check.Suggested helper method
private getLocalAuth(): LocalPrimaryAuth { const auth = this.scope.auth; if (!(auth instanceof LocalPrimaryAuth)) { throw new Error('OauthProviderCallbackFlow requires LocalPrimaryAuth'); } return auth; }Then replace
this.scope.auth as LocalPrimaryAuthwiththis.getLocalAuth()throughout.libs/sdk/src/auth/flows/auth.verify.flow.ts (1)
509-512: Consider reusing a shared authorization-id derivation helper.
deriveAuthorizationIdduplicates the logic inOrchestratedAuthorization.generateAuthorizationId. To avoid drift, consider moving this into a shared utility (or expose a shared helper) and reuse it here.
…ation ID handling
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/draft/docs/testing/api-reference.mdx`:
- Around line 1214-1224: The exports list is missing the CIMD mock server types;
add MockCimdServer, MockCimdServerOptions, MockCimdServerInfo, and
MockCimdClientOptions to the same exports block as the OAuth server types (near
MockOAuthServer, MockOAuthServerOptions, MockOAuthServerInfo, MockTestUser) so
the API reference includes the CIMD testing components; ensure the four symbols
are appended in the same comma-separated style and ordering as the surrounding
token factory and OAuth type entries.
In `@libs/sdk/src/auth/authorization/orchestrated.authorization.ts`:
- Around line 48-61: Add documentation for the new TokenStore public API methods
getProviderIds and migrateTokens: update the SDK docs under docs/draft/docs to
document TokenStore (or Orchestrated TokenStore) additions, describing method
signatures getProviderIds(authorizationId: string): Promise<string[]> and
migrateTokens(fromAuthId: string, toAuthId: string): Promise<void>, expected
behavior (what provider IDs are returned, and that migrateTokens moves stored
tokens from one authorizationId to another), any error/edge cases, and note that
InMemoryOrchestratedTokenStore implements these methods; ensure examples and
links to InMemoryOrchestratedTokenStore and related auth flows are included.
In `@libs/sdk/src/auth/flows/oauth.provider-callback.flow.ts`:
- Around line 446-493: skippedProviderIds is always empty because we never read
skipped providers from the FederatedAuthSession; update completeFederatedAuth to
pull the skipped list from the session (e.g., use session.skippedProviders ||
[]) instead of the hardcoded empty array so declined providers are included in
the authorization code metadata, and ensure any session creation/update code
(where access_denied is handled) pushes provider IDs into
session.skippedProviders so pendingAuthId/token migration and
localAuth.createAuthorizationCode receive accurate selectedProviderIds and
skippedProviderIds.
- Around line 89-113: Replace the thrown generic Error in
OauthProviderCallbackFlow::getLocalAuth with the project's MCP-specific error
class (e.g., MCPError or the project's canonical error type) and supply an
appropriate MCP error code/message; locate the getLocalAuth method in
OauthProviderCallbackFlow, change the throw new Error('OauthProviderCallbackFlow
requires LocalPrimaryAuth') to throw the MCP error class with a clear code
(e.g., AUTH_INVALID_TYPE or equivalent) and message so the flow uses MCP error
handling and codes instead of a plain Error.
- Around line 141-204: The loadFederatedSession flow logs the raw
federatedSessionId in the "Federated session not found or expired" warning;
change the logging in loadFederatedSession to avoid printing the full session ID
(federatedSessionId) by masking or truncating it (e.g., show only first/last few
chars or a short hash) before passing to this.logger.warn, and keep the full ID
stored in state as this.state.set('federatedSessionId', federatedSessionId) for
later use.
🧹 Nitpick comments (1)
libs/testing/src/auth/__tests__/mock-oauth-server.test.ts (1)
25-34: Remove the explicitstop()call;afterEachalready handles cleanup.The
stop()method is idempotent (lines 260–261 check if the server exists and return early if null), so calling it twice won't throw. However, the explicitawait server.stop()at line 33 is redundant since theafterEachhook already stops the server.🛠️ Suggested fix
- await server.stop();
…tication token management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/draft/docs/authentication/token.mdx`:
- Around line 138-178: The example imports InMemoryOrchestratedTokenStore from
the wrong entry point; update the example import to the true export location or
re-export the class: change the import for InMemoryOrchestratedTokenStore to
come from '@frontmcp/sdk/auth/session' in the code sample (or alternatively add
a re-export for InMemoryOrchestratedTokenStore in the SDK root export
(libs/sdk/src/index.ts) and add the ./auth/session entry in package.json
exports) so the token migration example using storeTokens, migrateTokens, and
getProviderIds works without import errors.
In `@libs/sdk/src/auth/session/federated-auth.session.ts`:
- Around line 395-402: The function completeCurrentProvider currently throws a
generic Error when session.currentProviderId is missing; replace that with the
project's MCP error type (e.g., throw new InternalMcpError(...) with an
appropriate error code such as 'INVALID_SESSION_STATE') and ensure the
InternalMcpError is imported where needed; update the error construction to
include a clear message ("No current provider to complete") and the MCP code so
MCP error handling and codes are preserved.
🧹 Nitpick comments (4)
libs/testing/src/auth/__tests__/mock-oauth-server.test.ts (1)
1-1: Remove unused import.
MockOAuthServerOptionsis imported but never used in this file.Suggested fix
-import { MockOAuthServer, MockOAuthServerOptions } from '../mock-oauth-server'; +import { MockOAuthServer } from '../mock-oauth-server';docs/draft/docs/testing/api-reference.mdx (1)
1220-1224: Add detailed documentation section for MockCimdServer.The CIMD mock server types (
MockCimdServer,MockCimdServerOptions,MockCimdServerInfo,MockCimdClientOptions) are exported here but lack a dedicated documentation section like the one provided forMockOAuthServer(lines 513-674). These testing components should be fully documented for consistency and clarity.Add a
## MockCimdServersection with:
- Constructor and options interface (port, debug)
- Methods:
start(),stop(),registerClient(options)- Property:
info- Usage example showing initialization and client registration
libs/sdk/src/auth/session/federated-auth.session.ts (2)
421-432: Avoid non‑null assertions and generic Errors in provider start.
shift()!is avoidable and genericErrorshould be replaced with an MCP error class. Please guard the shift result and throw a typed MCP error instead. As per coding guidelines, avoid non‑null assertions and use MCP error types.♻️ Suggested change
- if (session.currentProviderId) { - throw new Error('Cannot start next provider while current is in progress'); - } + if (session.currentProviderId) { + throw new InternalMcpError('Cannot start next provider while current is in progress', 'AUTH_FLOW_ERROR'); + } - if (session.providerQueue.length === 0) { - throw new Error('No more providers in queue'); - } + if (session.providerQueue.length === 0) { + throw new InternalMcpError('No more providers in queue', 'AUTH_FLOW_ERROR'); + } // Pop from queue and set as current - const providerId = session.providerQueue.shift()!; + const providerId = session.providerQueue.shift(); + if (!providerId) { + throw new InternalMcpError('No more providers in queue', 'AUTH_FLOW_ERROR'); + }
288-355: Consider a shared params interface for session creation.Both
createSessionandcreateFederatedAuthSessionduplicate the same inline object shape. Extracting aFederatedAuthSessionCreateParamsinterface improves reuse and aligns with the “prefer interface for object shapes” guideline. As per coding guidelines, prefer interfaces for object shapes.♻️ Possible refactor
+export interface FederatedAuthSessionCreateParams { + pendingAuthId: string; + clientId: string; + redirectUri: string; + scopes: string[]; + state?: string; + resource?: string; + userInfo: { email?: string; name?: string; sub?: string }; + frontmcpPkce: { challenge: string; method: 'S256' }; + providerIds: string[]; +} - createSession(params: { - pendingAuthId: string; - clientId: string; - redirectUri: string; - scopes: string[]; - state?: string; - resource?: string; - userInfo: { email?: string; name?: string; sub?: string }; - frontmcpPkce: { challenge: string; method: 'S256' }; - providerIds: string[]; - }): FederatedAuthSession { + createSession(params: FederatedAuthSessionCreateParams): FederatedAuthSession { -export function createFederatedAuthSession( - params: { - pendingAuthId: string; - clientId: string; - redirectUri: string; - scopes: string[]; - state?: string; - resource?: string; - userInfo: { email?: string; name?: string; sub?: string }; - frontmcpPkce: { challenge: string; method: 'S256' }; - providerIds: string[]; - }, +export function createFederatedAuthSession( + params: FederatedAuthSessionCreateParams,
…erated auth session parameters
# Conflicts: # libs/sdk/src/context/frontmcp-context.ts # libs/testing/package.json
…de-server to version 1.19.9
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/sdk/src/context/frontmcp-context.ts`:
- Around line 476-488: getContextTokens currently returns the internal Map
directly, leaking mutability at runtime; change getContextTokens to return a
snapshot (e.g., a new Map constructed from this._contextTokens) so callers
cannot mutate the backing _contextTokens, and keep setContextToken and
_contextTokens as-is; additionally ensure `@internal` members like
getContextTokens are excluded from published d.ts by enabling "stripInternal":
true in libs/sdk/tsconfig.lib.json or by moving these methods into a
non-exported/internal class so they are not part of the public API surface.
Summary by CodeRabbit
New Features
Performance / Ops
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.