feat(auth): pluggable authentication providers system - #892
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a pluggable multi-provider authentication subsystem: provider factory & registry, 11 provider implementations, base provider/session primitives, middleware (auth, RBAC, rate-limiting), session manager (memory/Redis), CLI commands, SDK/server integration (per-call token validation and context merging), types, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant NeuroLink as NeuroLink
participant Factory as AuthProviderFactory
participant Provider as AuthProvider
participant Session as SessionStorage
Client->>NeuroLink: generate/stream(auth: { token })
NeuroLink->>Factory: ensure/create provider (lazy)
Factory->>Provider: instantiate(provider-config)
NeuroLink->>Provider: authenticateToken(token) (with timeout)
Provider->>Provider: verify JWT/JWKS or call provider API
alt valid & user returned
Provider->>Session: get/create session for user
Session-->>Provider: AuthSession
Provider-->>NeuroLink: TokenValidationResult (user, expiresAt)
NeuroLink-->>Client: proceed with merged context (userId, userEmail, userRoles)
else invalid or error
Provider-->>NeuroLink: invalid / error
NeuroLink-->>Client: throw AuthenticationFailedError / InvalidTokenError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
This PR introduces a pluggable, multi-provider authentication system and wires it into the NeuroLink SDK and CLI, enabling per-call token validation or pre-validated request context to flow into generate/stream and downstream tool execution.
Changes:
- Adds auth provider factory/registry, provider implementations, session manager, auth context utilities, and error types.
- Extends
generate()/stream()option types and NeuroLink runtime to acceptrequestContextandauth: { token }. - Adds CLI subcommands for provider listing, token validation, and health checks plus supporting fixtures/spec docs.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/auth/test-credentials.json | Adds placeholder test users/tokens/JWKS fixture data |
| test/fixtures/auth/session-data.json | Adds session and rate-limit fixture data |
| test/fixtures/auth/provider-config.json | Adds provider config fixtures and RBAC/middleware/session configs |
| src/lib/types/streamTypes.ts | Adds requestContext and auth.token to StreamOptions |
| src/lib/types/generateTypes.ts | Adds requestContext and auth.token to GenerateOptions |
| src/lib/types/index.ts | Re-exports new auth-related types from types/authTypes |
| src/lib/types/configTypes.ts | Adds NeurolinkConstructorConfig.auth / NeuroLinkAuthConfig |
| src/lib/types/authTypes.ts | Introduces a new auth type system (provider/user/session/context contracts) |
| src/lib/server/routes/agentRoutes.ts | Propagates server auth context fields into generate/stream context |
| src/lib/neurolink.ts | Lazy auth provider init; per-call token validation; requestContext merging; auth context getters/setters |
| src/lib/mcp/toolRegistry.ts | Pulls userId from async-local auth context when absent from exec context |
| src/lib/index.ts | Exports auth factory/registry/providers/middleware/session/context/errors/types at top-level |
| src/lib/auth/sessionManager.ts | Adds memory + Redis session storage and SessionManager |
| src/lib/auth/serverBridge.ts | Adds adapter to use provider authenticateToken() with existing server middleware validate callback shape |
| src/lib/auth/providers/auth0.ts | Auth0 provider implementation |
| src/lib/auth/providers/clerk.ts | Clerk provider implementation |
| src/lib/auth/providers/firebase.ts | Firebase provider implementation |
| src/lib/auth/providers/supabase.ts | Supabase provider implementation |
| src/lib/auth/providers/workos.ts | WorkOS provider implementation |
| src/lib/auth/providers/betterAuth.ts | Better Auth provider implementation |
| src/lib/auth/providers/oauth2.ts | Generic OAuth2/OIDC provider implementation |
| src/lib/auth/providers/jwt.ts | Generic JWT provider implementation |
| src/lib/auth/providers/custom.ts | Custom provider implementation |
| src/lib/auth/providers/CognitoProvider.ts | Cognito provider implementation (provider-specific logic) |
| src/lib/auth/index.ts | Aggregates/auth-exports multi-provider auth system from src/lib/auth/* |
| src/lib/auth/authProvider.ts | Adds new BaseAuthProvider (token extraction, authz helpers, request auth flow) |
| src/lib/auth/authErrors.ts | Adds auth error classes + type guards |
| src/lib/auth/authContext.ts | Adds AsyncLocalStorage-based auth context utilities + fallback holder |
| src/lib/auth/RequestContext.ts | Adds request-scoped context map with reserved-key protection |
| src/lib/auth/AuthProviderRegistry.ts | Adds provider registry + discovery + (currently shallow) health checks |
| src/lib/auth/AuthProviderFactory.ts | Adds singleton factory w/ lazy dynamic imports for providers |
| src/cli/parser.ts | Registers auth command group in CLI |
| src/cli/factories/authCommandFactory.ts | Adds auth providers/validate/health subcommands |
| src/cli/commands/authProviders.ts | Implements provider listing/token validation/health check handlers |
| package.json | Adds jose dependency for JWT/JWKS validation |
| docs/superpowers/specs/2026-03-16-auth-providers-redesign.md | Adds design spec documenting intended auth integration contract |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AuthProviderConfig, | ||
| AuthProviderType, | ||
| MastraAuthProvider, | ||
| } from "./types/authTypes.js"; |
There was a problem hiding this comment.
AuthProviderRegistry imports AuthProviderConfig/AuthProviderType/MastraAuthProvider from ./types/authTypes.js, but AuthProviderFactory (used by the registry) is typed against ../types/authTypes.js. This mixes two different auth type definitions and forces as any casts (see createProvider). Recommend standardizing both registry and factory on the same authTypes module to avoid subtle incompatibilities and TS export conflicts.
| } from "./types/authTypes.js"; | |
| } from "../types/authTypes.js"; |
| /** | ||
| * Auth provider choices for multi-provider commands | ||
| */ | ||
| private static readonly AUTH_PROVIDER_CHOICES = [ | ||
| "auth0", | ||
| "clerk", | ||
| "supabase", | ||
| "firebase", | ||
| "workos", | ||
| "better-auth", | ||
| ] as const; |
There was a problem hiding this comment.
AUTH_PROVIDER_CHOICES limits auth validate/auth health to a subset of providers, but auth providers lists additional ones (e.g., jwt, oauth2, cognito, keycloak). This makes the CLI inconsistent and prevents validating/health-checking providers that the system claims to support. Consider either expanding the choices to all supported providers or filtering the list output to only those supported by validate/health.
| // Handle per-call auth token validation | ||
| if (options.auth?.token) { | ||
| await this.ensureAuthProvider(); | ||
| if (!this.authProvider) { | ||
| throw new Error( | ||
| "No auth provider configured. Set auth in constructor or via setAuthProvider() before using auth: { token }.", | ||
| ); | ||
| } | ||
| const authResult = await this.authProvider.authenticateToken( | ||
| options.auth.token, | ||
| ); | ||
| if (!authResult.valid) { | ||
| const { InvalidTokenError } = await import( | ||
| "./auth/authErrors.js" | ||
| ); | ||
| throw new InvalidTokenError( | ||
| authResult.error || "Token validation failed", | ||
| this.authProvider.type, | ||
| ); | ||
| } | ||
| // Merge validated user into context | ||
| if (authResult.user) { | ||
| options.context = { | ||
| ...((options.context as Record<string, unknown>) || {}), | ||
| userId: authResult.user.id, | ||
| userEmail: authResult.user.email, | ||
| userRoles: authResult.user.roles, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Handle pre-validated requestContext | ||
| if (options.requestContext) { | ||
| options.context = { | ||
| ...((options.context as Record<string, unknown>) || {}), | ||
| ...options.requestContext, | ||
| }; |
There was a problem hiding this comment.
generate() merges options.requestContext into options.context after token validation, so a caller can provide both auth: { token } and a conflicting requestContext to override userId/userRoles derived from the validated token. This undermines the point of token validation and can enable privilege escalation. Consider either (a) rejecting calls that provide both auth.token and requestContext, or (b) giving token-derived fields precedence (merge requestContext first, then overlay validated user fields, or explicitly omit/lock identity keys when merging).
| async setAuthProvider(config: NeuroLinkAuthConfig): Promise<void> { | ||
| if ("provider" in config) { | ||
| this.authProvider = config.provider; | ||
| logger.info(`Auth provider set: ${config.provider.type}`); | ||
| } else { | ||
| const { AuthProviderFactory } = await import( |
There was a problem hiding this comment.
This setAuthProvider implementation only supports the wrapper shape ({ provider } or { type, config }). If the intended public API includes passing a provider instance directly (as described in the spec), calls like setAuthProvider(new Auth0Provider(...)) will not work. Either widen the accepted parameter type / runtime branching to handle MastraAuthProvider directly, or adjust the documented contract to require the wrapper.
| if (strategy?.fromHeader) { | ||
| const headerName = strategy.fromHeader.name.toLowerCase(); | ||
| const headerValue = context.headers[headerName]; | ||
|
|
||
| if (typeof headerValue === "string") { | ||
| if (strategy.fromHeader.scheme) { | ||
| const prefix = `${strategy.fromHeader.scheme} `; | ||
| if (headerValue.startsWith(prefix)) { | ||
| return headerValue.slice(prefix.length); | ||
| } | ||
| } else { | ||
| return headerValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
BaseAuthProvider.extractToken() lowercases the configured header name and then does a direct lookup (context.headers[headerName]). If callers populate headers with canonical casing (e.g., Authorization) this will fail to find the token. Consider doing a case-insensitive header lookup (iterate entries like AuthMiddleware.extractToken does) or normalizing headers when building AuthRequestContext.
| function buildProviderConfig( | ||
| argv: ArgumentsCamelCase<AuthValidateArgs | AuthHealthArgs>, | ||
| ): Record<string, unknown> | null { | ||
| switch (argv.provider) { | ||
| case "auth0": { | ||
| const domain = argv.domain || process.env.AUTH0_DOMAIN; | ||
| const clientId = argv.clientId || process.env.AUTH0_CLIENT_ID; | ||
| if (!domain || !clientId) { | ||
| return null; | ||
| } | ||
| return { | ||
| domain, | ||
| clientId, | ||
| audience: process.env.AUTH0_AUDIENCE, | ||
| }; | ||
| } | ||
|
|
||
| case "clerk": { | ||
| const secretKey = argv.secretKey || process.env.CLERK_SECRET_KEY; | ||
| const publishableKey = process.env.CLERK_PUBLISHABLE_KEY || ""; | ||
| if (!secretKey) { | ||
| return null; | ||
| } | ||
| return { | ||
| publishableKey, | ||
| secretKey, | ||
| }; | ||
| } | ||
|
|
||
| case "supabase": { | ||
| const url = argv.url || process.env.SUPABASE_URL; | ||
| const anonKey = argv.anonKey || process.env.SUPABASE_ANON_KEY; | ||
| if (!url || !anonKey) { | ||
| return null; | ||
| } | ||
| return { | ||
| url, | ||
| anonKey, | ||
| jwtSecret: process.env.SUPABASE_JWT_SECRET, | ||
| }; | ||
| } | ||
|
|
||
| case "firebase": { | ||
| const projectId = process.env.FIREBASE_PROJECT_ID; | ||
| if (!projectId) { | ||
| return null; | ||
| } | ||
| return { | ||
| projectId, | ||
| apiKey: process.env.FIREBASE_API_KEY, | ||
| }; | ||
| } | ||
|
|
||
| case "workos": { | ||
| const apiKey = argv.apiKey || process.env.WORKOS_API_KEY; | ||
| const clientId = argv.clientId || process.env.WORKOS_CLIENT_ID; | ||
| if (!apiKey || !clientId) { | ||
| return null; | ||
| } | ||
| return { | ||
| apiKey, | ||
| clientId, | ||
| }; | ||
| } | ||
|
|
||
| case "better-auth": { | ||
| const secret = process.env.BETTER_AUTH_SECRET; | ||
| const baseUrl = argv.url || process.env.BETTER_AUTH_BASE_URL; | ||
| if (!secret || !baseUrl) { | ||
| return null; | ||
| } | ||
| return { | ||
| secret, | ||
| baseUrl, | ||
| }; |
There was a problem hiding this comment.
buildProviderConfig() returns provider config objects without a type field, even though AuthProviderConfig (and most provider constructors) are modeled as { type, ... }. Right now this is masked by as AuthProviderConfig, but it weakens type safety and can break providers that rely on config.type. Consider adding type: argv.provider to the returned config for each case (and validating required fields consistently with PROVIDER_INFO).
| // Fall back to in-memory if Redis is not available | ||
| logger.warn( | ||
| "Redis not available, falling back to in-memory session storage", |
There was a problem hiding this comment.
RedisSessionStorage.getClient() logs that it is "falling back to in-memory session storage" when ioredis import fails, but it then throws an error and does not actually fall back. This is misleading and can cause unexpected runtime failures in environments without ioredis. Either implement a real fallback to MemorySessionStorage at the SessionManager level, or update the log/message to reflect that Redis is required for storage: "redis".
| // Fall back to in-memory if Redis is not available | |
| logger.warn( | |
| "Redis not available, falling back to in-memory session storage", | |
| logger.error( | |
| "Redis client (ioredis) not available. When using storage: \"redis\", the ioredis package must be installed and Redis must be reachable.", |
| // Handle per-call auth token validation | ||
| if (options.auth?.token) { | ||
| await this.ensureAuthProvider(); | ||
| if (!this.authProvider) { | ||
| throw new Error( | ||
| "No auth provider configured. Set auth in constructor or via setAuthProvider() before using auth: { token }.", | ||
| ); | ||
| } | ||
| const authResult = await this.authProvider.authenticateToken( | ||
| options.auth.token, | ||
| ); | ||
| if (!authResult.valid) { | ||
| const { InvalidTokenError } = await import( | ||
| "./auth/authErrors.js" | ||
| ); | ||
| throw new InvalidTokenError( | ||
| authResult.error || "Token validation failed", | ||
| this.authProvider.type, | ||
| ); | ||
| } | ||
| // Merge validated user into context | ||
| if (authResult.user) { | ||
| options.context = { | ||
| ...((options.context as Record<string, unknown>) || {}), | ||
| userId: authResult.user.id, | ||
| userEmail: authResult.user.email, | ||
| userRoles: authResult.user.roles, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Handle pre-validated requestContext | ||
| if (options.requestContext) { | ||
| options.context = { | ||
| ...((options.context as Record<string, unknown>) || {}), | ||
| ...options.requestContext, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same issue in stream(): options.requestContext is merged after token validation, allowing a caller to override userId/userRoles derived from auth.token. For security, reject using both, or ensure token-derived identity fields take precedence / cannot be overridden by requestContext.
| /** | ||
| * Authentication configuration for NeuroLink SDK | ||
| */ | ||
| export type NeuroLinkAuthConfig = | ||
| | { provider: MastraAuthProvider } | ||
| | { type: "auth0"; config: Auth0Config } | ||
| | { type: "clerk"; config: ClerkConfig } | ||
| | { type: "firebase"; config: FirebaseConfig } | ||
| | { type: "supabase"; config: SupabaseConfig } | ||
| | { type: "workos"; config: WorkOSConfig } | ||
| | { type: "better-auth"; config: BetterAuthConfig } | ||
| | { type: AuthProviderType; config: AuthProviderConfig }; |
There was a problem hiding this comment.
NeuroLinkAuthConfig only allows passing a pre-built provider via { provider: MastraAuthProvider }, but the PR description/spec show auth: new Auth0Provider(...) (direct instance) as supported. If a user follows the documented API, it won't type-check, and NeuroLink's runtime logic also expects the wrapper shape. Consider allowing auth?: MastraAuthProvider | { provider: MastraAuthProvider } | { type, config } (or updating docs/constructor handling to match the intended contract).
| // Auth Types (re-exported for convenience) | ||
| export type { | ||
| AuthProviderType, | ||
| AuthProviderConfig, | ||
| AuthUser, | ||
| AuthSession, | ||
| AuthenticatedContext, | ||
| AuthMiddlewareConfig, | ||
| TokenClaims, | ||
| SessionConfig, | ||
| RBACConfig, | ||
| MastraAuthProvider, | ||
| } from "./auth/types/authTypes.js"; |
There was a problem hiding this comment.
src/lib/index.ts already does export * from "./types/index.js";, and types/index.ts now re-exports AuthProviderType, AuthProviderConfig, etc. Re-exporting the same names again here from ./auth/types/authTypes.js will cause duplicate/conflicting exports (and also mixes two different auth type systems). Consider removing this block, or re-exporting the auth types from a single canonical module to avoid name collisions and inconsistent public types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/factories/authCommandFactory.ts (1)
15-22:⚠️ Potential issue | 🟠 MajorRemove
as anycasts and use properly typed args fromauthProviders.ts.The "providers" command accepts
format: "table"(line 93) butAuthCommandArgs.formatonly allows"text" | "json". Theas anycasts witheslint-disable-next-line@typescript-eslint/no-explicit-any`` (lines 103, 115, 127) hide this type mismatch and violate the enforced no-explicit-any rule for src/.Import
AuthProvidersArgs,AuthValidateArgs, andAuthHealthArgsfromauthProviders.tsand use them directly instead of casting toany. The types already exist and properly account for all command options—AuthProvidersArgsincludes"table"in the format union while validate/health use the narrower"text" | "json"set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/authCommandFactory.ts` around lines 15 - 22, The AuthCommandArgs type is too broad and the file uses multiple "as any" casts to hide mismatched formats; import the specific types AuthProvidersArgs, AuthValidateArgs, and AuthHealthArgs from authProviders.ts and replace uses of AuthCommandArgs (and remove the eslint-disable-next-line casts) with the correct provider-specific types for the corresponding commands (e.g., use AuthProvidersArgs for the "providers" command so "table" is allowed, and AuthValidateArgs/AuthHealthArgs for validate/health commands to keep "text"|"json"); update the function/method signatures and argument typings (e.g., where AuthCommandArgs is referenced and where the as any casts occur) to use these imported types and remove all explicit any casts.src/lib/auth/index.ts (1)
54-58:⚠️ Potential issue | 🟡 MinorPotential
TokenValidationResulttype collision.Line 54 exports
TokenValidationResultfrom"./anthropicOAuth.js", and line 265 exports the same name (aliased asAuthTokenValidationResult) from"./types/authTypes.js". While the alias avoids direct collision, consumers importingTokenValidationResultwill get the OAuth version, which may cause confusion.💡 Consider consistent aliasing
// OAuth types (canonical definitions in types/subscriptionTypes.ts) export type { OAuthTokenResponse, OAuthFlowTokens, OAuthFlowTokens as OAuthTokens, - TokenValidationResult, + TokenValidationResult as OAuthTokenValidationResult, AnthropicOAuthConfig, PKCEParams, CallbackResult, } from "./anthropicOAuth.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/index.ts` around lines 54 - 58, The export TokenValidationResult from "./anthropicOAuth.js" conflicts by name with the other exported type aliased as AuthTokenValidationResult from "./types/authTypes.js", so update the re-exports to use distinct, explicit aliases: change the export from "./anthropicOAuth.js" (TokenValidationResult) to a clear alias like OAuthTokenValidationResult (or AnthropicTokenValidationResult) and ensure the other type remains AuthTokenValidationResult; then update any internal imports that rely on the unaliased name to use the new alias (look for references to TokenValidationResult, AuthTokenValidationResult, and the file "./anthropicOAuth.js"/"./types/authTypes.js" to apply the rename).
🟠 Major comments (23)
src/lib/auth/middleware/rateLimitByUser.ts-487-500 (1)
487-500:⚠️ Potential issue | 🟠 MajorType mismatch:
authMiddlewareparameter expectsRequestbutcreateAuthMiddlewarereturns a handler forAuthRequestContext.Per the context snippet from
AuthMiddleware.ts(lines 192-215),createAuthMiddlewarereturnsMiddlewareHandler<AuthRequestContext>, which is a function acceptingAuthRequestContext, not a rawRequestobject. The signature here expects(request: Request) => Promise<...>, which won't match.Either:
- Update this signature to accept
(context: AuthRequestContext) => Promise<...>, or- Provide an adapter that converts
RequesttoAuthRequestContextbefore calling the auth middleware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 487 - 500, The auth middleware parameter type is incorrect: createAuthMiddleware returns a MiddlewareHandler<AuthRequestContext> (a function accepting AuthRequestContext) but createAuthenticatedRateLimitMiddleware currently types authMiddleware as (request: Request) => Promise<...>. Fix by changing the authMiddleware parameter type to MiddlewareHandler<AuthRequestContext> (or the equivalent type alias used in your codebase) so signatures match, and update internal calls to pass an AuthRequestContext; alternatively implement a small adapter inside createAuthenticatedRateLimitMiddleware that constructs an AuthRequestContext from the incoming Request and then calls the provided auth middleware (reference symbols: createAuthenticatedRateLimitMiddleware, createAuthMiddleware, MiddlewareHandler<AuthRequestContext>, AuthRequestContext, authMiddleware).src/lib/types/index.ts-255-296 (1)
255-296:⚠️ Potential issue | 🟠 MajorAdd
MastraAuthProviderto the selective auth type exports.The core auth provider interface is missing from the re-exports. While
MastraAuthProvideris available via the mainsrc/lib/index.tsentry point, it's excluded from thesrc/lib/types/index.tsauth exports, creating an inconsistency with how the provider domain handles its core interface (AIProvider). All implementations of the auth system extendMastraAuthProvider, and external consumers implementing custom auth providers will need this type exported here for consistency.Add to lines 256-296:
MastraAuthProvider,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/index.ts` around lines 255 - 296, The export block in src/lib/types/index.ts omits the core provider interface MastraAuthProvider, causing consumers implementing custom auth providers to lack the type; add MastraAuthProvider to the selective re-exports in the auth types export list (the same block that exports AuthProviderType, AuthProviderConfig, AuthUser, AuthSession, etc.) so MastraAuthProvider is re-exported alongside those other auth types.test/fixtures/auth/test-credentials.json-46-107 (1)
46-107:⚠️ Potential issue | 🟠 MajorThese "valid" token fixtures are already expired and still trip secret scanning.
validToken,noRolesToken, andadminTokenall expire on February 1, 2024, so any test that checksexpagainst the wall clock will now treat them as invalid. The committed JWT-shaped strings also trigger Betterleaks on Lines 63, 75, 91, and 106. Generate these tokens relative toDate.now()in the test harness, or keep only the payload metadata here and use obvious non-token placeholders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/auth/test-credentials.json` around lines 46 - 107, The fixtures validToken, noRolesToken, and adminToken contain hard-coded JWT strings that are expired and trigger secret scanning; remove the baked token strings for those entries (token fields on validToken, noRolesToken, adminToken) or replace them with obvious placeholders like "GENERATE_AT_RUNTIME", keep the payload metadata as-is, and update tests to generate real JWTs at runtime (use Date.now()/new Date() to set iat/exp relative to current time) in the test harness function that consumes these fixtures (refer to the fixture keys validToken, noRolesToken, adminToken and the test helper that builds tokens) so tests validate expiration correctly and avoid committing secrets.test/fixtures/auth/provider-config.json-6-125 (1)
6-125:⚠️ Potential issue | 🟠 MajorUse non-secret-shaped placeholders in this fixture.
Values like
AIza...,sk_test_..., and the JWT-looking Supabase keys are fake, but secret scanners still treat them as credentials—Betterleaks already flags Line 54. Keeping credential-shaped strings in git creates noisy CI and desensitizes the repo to real leaks. Prefer obvious sentinels such as<SUPABASE_ANON_KEY_PLACEHOLDER>or load realistic samples from ignored env files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/auth/provider-config.json` around lines 6 - 125, Replace any credential-shaped literal values in the provider fixtures (e.g., the firebase apiKey, supabase anonKey/serviceRoleKey/jwtSecret, workos apiKey, clerk secretKey, auth0 clientSecret, better-auth secret, jwt secret, etc.) with clearly non-secret placeholders like <FIREBASE_API_KEY_PLACEHOLDER>, <SUPABASE_ANON_KEY_PLACEHOLDER>, <WORKOS_API_KEY_PLACEHOLDER> or load them from a test-only env loader; update entries under "firebase", "supabase", "workos", "clerk", "auth0", "better-auth", and "jwt" to use those sentinel strings so secret scanners no longer flag them while preserving the shape of the fixture for tests.src/lib/auth/serverBridge.ts-18-27 (1)
18-27:⚠️ Potential issue | 🟠 MajorHandle valid tokens that only return claims.
TokenValidationResult.useris optional, and the server middleware treatsnullas an invalid token. Returningnullwheneverresult.useris missing will reject providers that validate successfully but only surface claims. Fall back toresult.claimsbefore failing this bridge.Suggested fallback
return async (token: string) => { const result = await provider.authenticateToken(token); - if (!result.valid || !result.user) { + if (!result.valid) { return null; } + const id = + result.user?.id ?? + (typeof result.claims?.sub === "string" ? result.claims.sub : undefined) ?? + (typeof result.claims?.user_id === "string" + ? result.claims.user_id + : undefined); + if (!id) { + return null; + } return { - id: result.user.id, - email: result.user.email, - roles: result.user.roles, + id, + email: + result.user?.email ?? + (typeof result.claims?.email === "string" + ? result.claims.email + : undefined), + roles: + result.user?.roles ?? + (Array.isArray(result.claims?.roles) + ? result.claims.roles.filter( + (role): role is string => typeof role === "string", + ) + : undefined), }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/serverBridge.ts` around lines 18 - 27, The bridge currently returns null when provider.authenticateToken yields a valid result with no result.user; instead, update the returned async function (the token bridge that calls provider.authenticateToken) to fall back to result.claims before returning null: if result.valid but result.user is missing, build the user object from result.claims (e.g. use claims.sub or claims.id for id, claims.email for email, and claims.roles for roles) and return that constructed user shape; only return null when result.valid is false and there are no usable claims.test/fixtures/auth/session-data.json-6-128 (1)
6-128:⚠️ Potential issue | 🟠 MajorAll sessions marked
isValid: trueare already expired.
validSession,adminSession,managerSession,aboutToExpireSession, and bothmultiDeviceentries expire between January 31, 2026 and February 1, 2026. Any TTL-aware test will now reject them despiteisValid: true. Make these timestamps relative in the test setup, or move them far enough into the future to avoid time-bombed fixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/auth/session-data.json` around lines 6 - 128, The fixture marks several sessions (validSession, adminSession, managerSession, aboutToExpireSession, multiDeviceSession1, multiDeviceSession2) as isValid: true but uses hardcoded expiresAt timestamps that are already expired; update these session objects to use future or relative timestamps (e.g., compute expiresAt/createdAt/lastActivityAt relative to now in the test setup or set them far enough in the future) and keep aboutToExpireSession as a relative short-lived expiry if the test needs it; ensure refreshToken/accessToken fields remain unchanged and only timestamp fields are adjusted so TTL-aware tests accept the sessions.src/cli/factories/authCommandFactory.ts-263-318 (1)
263-318:⚠️ Potential issue | 🟠 MajorThe new CLI only exposes a subset of the auth providers added in this PR.
validateandhealthcan select onlyauth0,clerk,supabase,firebase,workos, andbetter-auth, and the shared option builder only wires flags for that same subset. The PR adds Cognito, Keycloak, OAuth2, JWT, and Custom too, so the CLI cannot validate or health-check a large chunk of the provider matrix. Derive the choices/config from the auth registry, or add the missing providers and their required options here.Also applies to: 324-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/authCommandFactory.ts` around lines 263 - 318, The AUTH_PROVIDER_CHOICES constant and buildProviderOptions method only include a subset of providers; update AUTH_PROVIDER_CHOICES to include the new providers (e.g., "cognito", "keycloak", "oauth2", "jwt", "custom") and extend buildProviderOptions to add the flags needed by those providers (e.g., pool/client ids and secrets for Cognito, realm/client secret/issuer for Keycloak, token/issuer/alg for JWT, and generic client_id/client_secret/authorize/token endpoints for OAuth2/custom), or replace the hardcoded AUTH_PROVIDER_CHOICES and the options wiring inside buildProviderOptions with a dynamic derivation from the auth registry so choices and required flags come from the central provider registry used elsewhere.test/continuous-test-suite-auth.ts-225-305 (1)
225-305:⚠️ Potential issue | 🟠 MajorThese helper fixtures already blow the test warning budget.
These two harness sections alone introduce well over 10 explicit
anys. Sincetest/only gets 10 ESLint warnings total, this single file can makepnpm run check:allfail before the auth assertions even run.As per coding guidelines,
CI pipeline enforces max 300 warnings for src/, 10 warnings for test/ via ESLint. Validate before PR submission with pnpm run check:all.Also applies to: 508-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-auth.ts` around lines 225 - 305, Tests exceed the allowed ESLint warnings because this fixture uses many explicit any types; replace them with concrete types and narrow casts. Define an AuthUser interface and a RequestContext type and use those for ctx and ctx.user instead of any; update the bearer middleware call and the handler invocation to use the RequestContext type (referencing createBearerAuthMiddleware and bearerMiddleware.handler), replace c.set("user" as any, ...) and c.get("user" as any) with typed c.set<User> and c.get<User> (or the framework's generic accessors), type the headers map as Record<string,string> without any casts by using proper iteration types, and give `body` a precise type (e.g., unknown or a specific interface) instead of any; these changes will remove the excessive explicit any usages throughout the auth helper block.src/lib/auth/providers/CognitoProvider.ts-269-290 (1)
269-290:⚠️ Potential issue | 🟠 MajorAdd a timeout around JWKS fetches.
fetch(this.jwksUri)sits on the auth path with no timeout or cancellation. A slow or blackholed JWKS endpoint can stall protected requests until the platform default network timeout, so this should fail fast and ideally keep serving the last good cached JWKS on transient failures.As per coding guidelines,
Use ErrorFactory for typed errors, withTimeout for async operations, and graceful degradation with provider fallback for error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 269 - 290, Wrap the fetch(this.jwksUri) call in withTimeout using a configurable timeout (e.g., this.jwksFetchTimeout) inside CognitoProvider so the JWKS request fails fast; on timeout or transient network error, return the last cached JWKS from jwksCache (keyed by this.jwksUri) if still valid instead of throwing, and only throw a typed error via ErrorFactory/AuthProviderError.create when no cached JWKS exists. Ensure the thrown error includes the original cause ({ cause: error }) and use a clear code like "JWKS_FETCH_FAILED_TIMEOUT" or reuse "JWKS_FETCH_FAILED" via ErrorFactory to meet the typed error guideline.test/continuous-test-suite-auth.ts-95-158 (1)
95-158:⚠️ Potential issue | 🟠 MajorThe generate/stream sections can skip on the bug they’re meant to catch.
These cases default to
vertexand then convert very broad substrings like"failed to","not found","network", and"no providers"intoSKIP. That means brokenrequestContextorauthhandling can still produce a green run whenever the model path fails first; use a deterministic mocked provider so the assertions actually exercise auth propagation.Based on learnings,
Setup file is test/setup.ts with global AI provider mocks.Also applies to: 756-940
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-auth.ts` around lines 95 - 158, The tests defaulting to "vertex" allow broad provider errors (checked by isExpectedProviderError) to SKIP auth tests; switch the suite to use the deterministic mock provider from the setup file so auth/generate/stream assertions run. Concretely, change the TEST_PROVIDER default in this file (symbol TEST_PROVIDER) to the global mock identifier used in test/setup.ts (or require process.env.TEST_PROVIDER to be set and fail fast), and ensure skipIfProviderError and isExpectedProviderError are not treating generic phrases (e.g., "failed to", "not found", "network", "no providers") as SKIP for the mocked provider paths so auth propagation code (createBearerAuthMiddleware, createRoleAuthMiddleware, NeuroLink) is exercised deterministically.src/lib/neurolink.ts-2995-3005 (1)
2995-3005:⚠️ Potential issue | 🟠 MajorPut a timeout around
authenticateToken().For Auth0/OIDC/Cognito/JWKS-backed providers this call can block on network I/O. Right now a stalled auth backend can hang
generate()/stream()indefinitely before any model work starts. Please wrap the validation call inwithTimeout()and translate timeout failures into your auth error taxonomy.⏱️ Proposed fix
- const authResult = await this.authProvider.authenticateToken( - options.auth.token, - ); + const authResult = await withTimeout( + this.authProvider.authenticateToken(options.auth.token), + PROVIDER_TIMEOUTS.AUTH_MS, + );As per coding guidelines, "Use ErrorFactory for typed errors, withTimeout for async operations, and graceful degradation with provider fallback for error handling."
Also applies to: 5558-5568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 2995 - 3005, Wrap the call to authProvider.authenticateToken (used after ensureAuthProvider in methods like generate and stream) with withTimeout and map timeout rejections to a typed auth error via ErrorFactory (e.g., create an AuthTimeoutError or use the existing auth error type) so a stalled JWKS/OIDC/Auth0 call won't hang model work; specifically, in the block after ensureAuthProvider() where authenticateToken(options.auth.token) is awaited, replace the direct await with withTimeout(authProvider.authenticateToken(...), <appropriate ms>) and catch a timeout to throw the appropriate ErrorFactory-produced auth error, preserving other auth failures as-is; apply the same pattern to the other authenticateToken usage locations (the other authenticateToken call referenced in the diff).src/lib/neurolink.ts-10910-10914 (1)
10910-10914:⚠️ Potential issue | 🟠 MajorRemove the
anycast and use proper type discrimination to ensure provider/config alignment.When
configis narrowed in the else branch (afterif ("provider" in config)), TypeScript should enforce that thetypeandconfigfields match one of the discriminated union members. Theanycast bypasses this validation, allowing mismatched provider/config pairs like{ type: "auth0", config: ClerkConfig }to compile but fail only at runtime. This also violates the repo'sno-explicit-anyrule insrc/.The type mismatch exists because
AuthProviderFactory.create()acceptsAuthProviderConfig(generic config withtype,required,tokenExtraction, etc.), butNeuroLinkAuthConfigspecific arms use provider-specific configs likeAuth0Config(withdomain,clientId, etc.) that don't extendAuthProviderConfig.Consider refactoring
AuthProviderFactory.create()to accept the full union of possible configs (using conditional or overloaded signatures) instead of the genericAuthProviderConfig, so the type system can verify alignment at the call site:// Instead of: factory.create(config.type as AuthProviderType, config.config as any) // Use proper type discrimination that validates provider/config pairs factory.create(config.type, config.config)This approach is consistent with the discriminated union pattern already used in
NeuroLinkAuthConfigand would catch misconfigurations at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 10910 - 10914, Remove the `as any` cast and make the types line up so TypeScript enforces the provider/config pair: update AuthProviderFactory.create's signature to accept the full discriminated union (or overloads) of provider-specific config types instead of the generic AuthProviderConfig, then call factory.create(config.type, config.config) directly from the NeuroLinkAuthConfig branch (the code setting this.authProvider) so the compiler validates that the config matches the AuthProviderType; refer to symbols AuthProviderFactory.create, AuthProviderConfig, AuthProviderType, NeuroLinkAuthConfig and the this.authProvider assignment when making the change.src/lib/auth/providers/clerk.ts-68-79 (1)
68-79:⚠️ Potential issue | 🟠 MajorThe JWT branch isn't bound to the configured Clerk trust material.
Lines 68-69 accept
jwtKey, but Lines 77-78 always build a verifier from a hard-coded JWKS endpoint andvalidateJWT()only uses that verifier. The configured key never participates, so JWT validation is not actually pinned to the app-specific config.Also applies to: 101-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/clerk.ts` around lines 68 - 79, The Clerk provider currently always uses the hard-coded remote JWKS (this.jwks = jose.createRemoteJWKSet(...)) and never binds the configured jwtKey; update initialize() to construct the JWT verifier from the configured jwtKey when present (falling back to the remote JWKS only if jwtKey is absent) and ensure validateJWT() uses that constructed verifier; specifically, reference this.jwtKey, this.jwks, initialize() and validateJWT() and implement a conditional path that imports or builds a local JWK/JWKSet from this.jwtKey (or uses jose.importJWK / createLocalJWKSet equivalent) so the app-specific trust material is actually used for verification (also apply same change to the logic referenced around lines 101-107).src/lib/auth/providers/auth0.ts-89-99 (1)
89-99:⚠️ Potential issue | 🟠 Major
clientIdis required but never enforced whenaudienceis absent.Lines 89-94 make
clientIdmandatory, but Lines 139-142 only constrain byaudience. If callers omitaudience, this provider never scopes tokens to the configured app, so same-tenant tokens for other clients/APIs can pass validation. Either requireaudiencehere or add an explicitaud/azpcheck againstclientId.Also applies to: 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/auth0.ts` around lines 89 - 99, The code makes clientId required but doesn't ensure tokens are scoped to that client when audience is absent; update the Auth0 provider so either (A) require config.audience alongside config.clientId in the constructor (throw InvalidConfigurationError if audience is missing), or (B) keep audience optional but add an explicit token-claim check in the token validation routine (the block that currently inspects audience around the lines referencing this.audience) to verify that either the token's "aud" includes this.audience or that the token's "azp"/"aud" matches this.clientId; implement one approach consistently and use this.clientId/this.audience and the token validation function name (e.g., validateToken/verifyJwt) to locate where to add the enforcement.src/lib/auth/providers/supabase.ts-169-176 (1)
169-176:⚠️ Potential issue | 🟠 Major
emailVerifiedflips totruewhen the field is missing.Line 175 checks
!== null, soundefinedis treated as verified. Any user record withoutemail_confirmed_atwill come back withemailVerified: true.✅ Minimal fix
- emailVerified: userData.email_confirmed_at !== null, + emailVerified: Boolean(userData.email_confirmed_at),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/supabase.ts` around lines 169 - 176, The emailVerified computation in the return object incorrectly treats undefined as verified because it uses strict !== null; update the check for email confirmation in the function that returns the user object (referencing userData and the email_confirmed_at field) to a nullish check that rejects both null and undefined (e.g., use a != null or explicit !== undefined && !== null check) so emailVerified is true only when email_confirmed_at has a real value.src/lib/auth/providers/custom.ts-113-127 (1)
113-127:⚠️ Potential issue | 🟠 MajorDon't create a local session after the custom hook fails.
If
createSessionFnrejects because the external session store is down or intentionally denies login, Lines 125-127 silently fall back to a new in-memory session. That turns a failed custom session creation into a successful login.🛑 Safer behavior
} catch (error) { - logger.warn("Custom createSession failed, using default:", error); + logger.error("Custom createSession failed:", error); + throw error instanceof Error ? error : new Error(String(error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/custom.ts` around lines 113 - 127, The catch block around this.createSessionFn should NOT silently fall through to the default in-memory session; update the error handling in the method that calls createSessionFn so that on error it either rethrows the error or returns a failed result (e.g., null/undefined) instead of allowing the code that follows to create and register a local session; specifically modify the catch in the create-session flow that references this.createSessionFn, remove the fallback that calls this.sessions.set(...), this.userSessions.get(...).add(...), and this.emit("auth:login", user) when the custom hook fails, and ensure the function returns/throws immediately so failed external session creation does not become a successful in-memory login.src/lib/auth/providers/jwt.ts-84-85 (1)
84-85:⚠️ Potential issue | 🟠 MajorPass
config.algorithmstojwtVerify()to enforce consistent algorithm policy.Lines 84–85 configure an allowed algorithm allow-list, but lines 133–146 never pass it to
jwtVerify(). This allows the verification to accept any algorithm compatible with the key, enabling algorithm confusion attacks. AddverifyOptions.algorithms = this.algorithms;before callingjwtVerify().Also applies to: 133–146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/jwt.ts` around lines 84 - 85, The allowed-algorithms list initialized on this.algorithms is never passed into jwtVerify, allowing algorithm confusion; before calling jwtVerify(...) in the verification routine (the block that constructs verifyOptions and then calls jwtVerify), set verifyOptions.algorithms = this.algorithms so jwtVerify enforces the configured allow-list; update the code path that builds verifyOptions and calls jwtVerify to include this assignment (refer to this.algorithms, verifyOptions, and jwtVerify).src/lib/auth/sessionManager.ts-277-288 (1)
277-288:⚠️ Potential issue | 🟠 MajorAvoid using Redis
KEYScommand in production.The
KEYScommand with a pattern (${this.prefix}*) scans all keys in Redis and can block the server for seconds or more with large datasets. This is a well-documented anti-pattern for production Redis usage.🔧 Recommended fix using SCAN
async clear(): Promise<void> { try { const client = await this.getClient(); - const keys = await client.keys(`${this.prefix}*`); - - if (keys.length > 0) { - await client.del(...keys); - } + // Use SCAN with cursor-based iteration for production safety + let cursor = "0"; + do { + const [nextCursor, keys] = await client.scan(cursor, "MATCH", `${this.prefix}*`, "COUNT", 100); + cursor = nextCursor; + if (keys.length > 0) { + await client.del(...keys); + } + } while (cursor !== "0"); } catch (error) { logger.error("Redis clear error:", error); } }Note: This requires adding
scanto theRedisClientinterface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/sessionManager.ts` around lines 277 - 288, The clear() method uses Redis KEYS which is unsafe for production; replace it with an incremental SCAN loop: update the RedisClient interface to include scan(cursor: string, options?: { MATCH?: string, COUNT?: number }) (or the client-specific scan signature), then in clear() call getClient() and repeatedly call scan starting with cursor "0", using MATCH `${this.prefix}*` and a reasonable COUNT, collect matched keys into a batch and call client.del(...) for each batch (or pipeline) until the scan cursor returns "0"; keep existing try/catch and use logger.error on failure. Ensure you reference the clear method, getClient(), this.prefix and the RedisClient.scan addition when making the changes.src/lib/auth/providers/betterAuth.ts-137-143 (1)
137-143:⚠️ Potential issue | 🟠 MajorAdd timeout to external HTTP call to prevent hanging requests.
The
validateSessionmethod makes an external HTTP request to the Better Auth API without a timeout. If the Better Auth server is slow or unresponsive, this could cause requests to hang indefinitely, degrading user experience and potentially exhausting resources.🛡️ Proposed fix to add timeout
private async validateSession( sessionToken: string, ): Promise<TokenValidationResult> { try { const proxyFetch = createProxyFetch(); - const response = await proxyFetch(`${this.baseUrl}/api/auth/session`, { - headers: { - Cookie: `better-auth.session_token=${sessionToken}`, - }, - }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout + + try { + const response = await proxyFetch(`${this.baseUrl}/api/auth/session`, { + headers: { + Cookie: `better-auth.session_token=${sessionToken}`, + }, + signal: controller.signal, + }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/betterAuth.ts` around lines 137 - 143, The validateSession method calls createProxyFetch to GET `${this.baseUrl}/api/auth/session` with a Cookie but has no timeout; wrap the fetch call in an AbortController and set a short timeout (e.g., 5s) that calls controller.abort(), pass controller.signal to proxyFetch, clear the timeout after response, and handle AbortError by treating the session as invalid or returning an explicit timeout error; update validateSession (and any related error handling around response) to ensure the timeout is applied and resources are cleaned up.src/cli/commands/authProviders.ts-480-482 (1)
480-482:⚠️ Potential issue | 🟠 MajorMissing provider implementations in
buildProviderConfig.The
PROVIDER_INFOcatalog includesoauth2,cognito,keycloak, andjwtproviders, butbuildProviderConfighas no cases for them. Users attempting to validate tokens or check health for these providers will always get "Missing required configuration" errors.Do you want me to generate the missing cases for these providers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/authProviders.ts` around lines 480 - 482, buildProviderConfig currently returns null for unhandled providers, causing "Missing required configuration" for oauth2, cognito, keycloak, and jwt from PROVIDER_INFO; add explicit cases in the switch inside buildProviderConfig for 'oauth2', 'cognito', 'keycloak', and 'jwt' that validate and return the provider-specific config object (e.g., expected fields like issuer, clientId, clientSecret, jwksUri, region, userPoolId, realm, tokenEndpoint, publicKey, algorithms) and throw or return clear errors when required fields are missing; reference the PROVIDER_INFO entries to mirror required keys and reuse any existing helpers (e.g., parseUrl/validateString) and ensure the returned config shape matches what validateToken and health-check functions expect.src/lib/auth/providers/workos.ts-145-158 (1)
145-158:⚠️ Potential issue | 🟠 MajorAdd timeout to session validation API call.
The
validateSessionmethod makes an external HTTP POST to WorkOS without a timeout, risking hung requests if the API is unresponsive.🛡️ Proposed fix
try { const proxyFetch = createProxyFetch(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + const response = await proxyFetch( "https://api.workos.com/user_management/authenticate", { method: "POST", headers: { Authorization: `Bearer ${this.apiKey}`, "Content-Type": "application/json", }, body: JSON.stringify({ session_token: token, client_id: this.clientId, }), + signal: controller.signal, }, ); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/workos.ts` around lines 145 - 158, validateSession currently calls proxyFetch for WorkOS without a timeout, risking hung requests; wrap the HTTP POST to "https://api.workos.com/user_management/authenticate" in a cancellable timeout using an AbortController (or the existing proxyFetch timeout option if supported) so the request is aborted after a sensible duration (e.g., 5s). Update the proxyFetch invocation inside validateSession to create an AbortController, pass signal to proxyFetch (or pass timeout param), start a timer that calls controller.abort() on expiry, and clear the timer on completion; ensure the thrown abort error is handled/translated to a timeout error in validateSession.src/lib/auth/providers/oauth2.ts-397-411 (1)
397-411:⚠️ Potential issue | 🟠 MajorAdd timeout to token exchange request.
The
exchangeCodemethod makes an external HTTP POST without a timeout. This is particularly important for OAuth flows where users are waiting for completion.🛡️ Proposed fix
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 15000); + const response = await proxyFetch(this.tokenUrl, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded", }, body: body.toString(), + signal: controller.signal, }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 397 - 411, exchangeCode currently calls proxyFetch(this.tokenUrl, ...) without a timeout; wrap the request with an AbortController and set a timeout (e.g., via setTimeout) so the POST to this.tokenUrl is aborted after a reasonable period, clear the timeout on success, and catch an AbortError to throw a ProviderAPIError with a descriptive message (use "oauth2" and an appropriate status/code). Update the proxyFetch invocation inside exchangeCode to pass signal from the AbortController and ensure any cleanup (clearTimeout) happens before returning or rethrowing.src/lib/auth/providers/oauth2.ts-175-191 (1)
175-191:⚠️ Potential issue | 🟠 MajorAdd timeout to userInfo endpoint call.
The
validateViaUserInfomethod makes an external HTTP request without a timeout, which could cause requests to hang if the identity provider is unresponsive.🛡️ Proposed fix
private async validateViaUserInfo( token: string, ): Promise<TokenValidationResult> { try { const proxyFetch = createProxyFetch(); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + const response = await proxyFetch(this.userInfoUrl!, { headers: { Authorization: `Bearer ${token}`, }, + signal: controller.signal, }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 175 - 191, The call to the userInfo endpoint in validateViaUserInfo lacks a timeout and can hang; wrap the proxyFetch(this.userInfoUrl!, ...) call with an AbortController: create an AbortController, pass signal to proxyFetch, start a setTimeout that calls controller.abort() after a reasonable default (e.g. 5s or configurable via provider options), clear the timeout after the request finishes, and catch the abort error to return { valid: false, error: 'UserInfo request timed out' } (or include the abort/error message); update validateViaUserInfo to use the controller/signal and handle cleanup so hung requests are prevented.
🟡 Minor comments (10)
src/lib/auth/middleware/rateLimitByUser.ts-75-80 (1)
75-80:⚠️ Potential issue | 🟡 Minor
setIntervalwithoutunref()prevents graceful Node.js shutdown.The cleanup interval will keep the Node.js event loop alive even when all other work is done, preventing clean process exit. Call
unref()on the interval handle:constructor(cleanupIntervalMs: number = 60000) { // Periodically cleanup expired buckets this.cleanupInterval = setInterval(() => { this.cleanupExpiredBuckets(); }, cleanupIntervalMs); + // Don't block process exit + this.cleanupInterval.unref?.(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 75 - 80, The interval created in the constructor using setInterval keeps the Node.js event loop alive; update the constructor to call unref() on the interval handle (this.cleanupInterval) after creation so it doesn't block process exit — locate the constructor in rateLimitByUser.ts where setInterval(...) invokes this.cleanupExpiredBuckets() and call this.cleanupInterval.unref() (or otherwise unref the returned timer) immediately after assigning it.test/continuous-test-suite-auth.ts-11-15 (1)
11-15:⚠️ Potential issue | 🟡 MinorFix linting violations for explicit
anytype annotations.This file contains 25 explicit
anyannotations spread across helper functions (lines 226–235, 264, 279, 292, 304–305, 368, 384, 415, 431, 509–520, 539, 552, 565, 575, 621, 739). The CI pipeline enforces a maximum of 10 warnings for test files. Refactor to use proper types or suppress specific uses with//@ts-expect-error`` comments where type safety cannot be achieved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-auth.ts` around lines 11 - 15, Multiple helper variables and parameters in test/continuous-test-suite-auth.ts are declared with explicit any (25 occurrences); replace these with precise types or safer alternatives and add targeted ts-expect-error only where typing is impossible: locate helper functions that construct or inspect requests/responses (e.g., any parameters/vars in the auth helper functions and validators), change parameter/variable types from any to concrete types such as RequestInit, Response, HeadersInit, string, number, boolean, or a small AuthConfig/TestContext interface, prefer unknown + runtime narrowing when dynamic, and for the few cases that cannot be typed, add a single-line // `@ts-expect-error` with a comment explaining why; ensure no more than 10 explicit any usages remain by refactoring functions that create requests/responses and the validation helpers to use proper types instead of any.src/cli/commands/authProviders.ts-468-478 (1)
468-478:⚠️ Potential issue | 🟡 MinorBetter Auth
secretcannot be provided via CLI option.The
better-authprovider requiressecret(perPROVIDER_INFO), but it can only be read fromBETTER_AUTH_SECRETenvironment variable. Unlike other providers (e.g.,secretKeyfor Clerk), there's no CLI option for the secret. This inconsistency may confuse users.💡 Suggested improvement
Either add a
--secretCLI option inauthCommandFactory.ts, or document in the error message that the secret must be set via environment variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/authProviders.ts` around lines 468 - 478, The better-auth branch currently only reads process.env.BETTER_AUTH_SECRET and never accepts a CLI flag; update the CLI to accept a --secret option and make the provider prefer argv.secret over the env var. Specifically, add a secret/--secret flag in authCommandFactory.ts (wired into argv for auth commands) and then change the "better-auth" case in authProviders.ts to use argv.secret || process.env.BETTER_AUTH_SECRET for the secret check (keeping the baseUrl logic as-is and still returning null if neither is present). Ensure the flag name matches any PROVIDER_INFO metadata for better-auth if present.src/lib/auth/sessionManager.ts-148-163 (1)
148-163:⚠️ Potential issue | 🟡 MinorInconsistent error handling in Redis client initialization.
The code logs a warning that it's "falling back to in-memory" but then throws an error instead of actually returning a fallback storage. This creates a confusing user experience and the warning message is misleading.
🔧 Proposed fix - either throw without misleading message or implement fallback
Option 1 - Throw with accurate message:
} catch { - // Fall back to in-memory if Redis is not available - logger.warn( - "Redis not available, falling back to in-memory session storage", - ); - throw new Error("Redis client not available"); + throw new Error("Redis client not available. Install ioredis package or use memory storage."); }Option 2 - Actually implement fallback (requires refactoring class structure).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/sessionManager.ts` around lines 148 - 163, The getClient method logs "falling back to in-memory session storage" but then throws instead of returning a fallback client; fix by either (A) making the behavior honest: remove the misleading logger.warn and change the catch to throw a clear error (e.g., log an error and throw new Error("Redis client not available")), or (B) implement a real in-memory fallback: instantiate and return an in-memory RedisClient-compatible implementation inside the catch (use this.client = <in-memory impl> and return this.client), ensuring references to this.redisUrl, this.client, getClient and the RedisClient type remain consistent; pick one option and update the catch block accordingly.src/lib/auth/providers/oauth2.ts-206-211 (1)
206-211:⚠️ Potential issue | 🟡 MinorMisleading
tokenType: "jwt"for userInfo-based validation.When validation is performed via the userInfo endpoint (not JWT verification), the response incorrectly reports
tokenType: "jwt". This should be"access_token"or"opaque"to accurately reflect that no JWT was validated.🔧 Proposed fix
return { valid: true, payload: data, user, - tokenType: "jwt", + tokenType: "access_token", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 206 - 211, The returned auth result in src/lib/auth/providers/oauth2.ts currently sets tokenType: "jwt" even in the userInfo-based validation path; update the userInfo validation code path (the function that returns the object with properties valid, payload, user, tokenType) to set tokenType to "access_token" (or "opaque" if you prefer that terminology) when the token was validated via the userInfo endpoint rather than JWT verification, and keep tokenType "jwt" only for the JWT verification branch so callers can distinguish validation types.src/lib/auth/providers/workos.ts-374-380 (1)
374-380:⚠️ Potential issue | 🟡 MinorAvoid logging full error objects that may contain sensitive data.
Logging the complete error object could expose sensitive information like API keys or user data in stack traces.
🛡️ Proposed fix
} catch (error) { - logger.error("Failed to fetch WorkOS user:", error); + logger.error("Failed to fetch WorkOS user:", { + userId, + error: error instanceof Error ? error.message : String(error), + }); if (error instanceof ProviderAPIError) { throw error; } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/workos.ts` around lines 374 - 380, Replace the current logger.error call that logs the full error object in the catch block of the WorkOS user fetch (the block handling errors around ProviderAPIError) with a sanitized log that only records non-sensitive information (e.g., a descriptive message plus error.message and any safe, non-secret fields like error.code or status if present); keep the existing instanceof ProviderAPIError rethrow behavior unchanged and do not include stack traces or raw error objects in logs.src/lib/auth/providers/betterAuth.ts-317-339 (1)
317-339:⚠️ Potential issue | 🟡 MinorAdd timeout to health check HTTP call.
Similar to
validateSession, the health check makes an external call without a timeout. Health checks should be bounded to prevent cascading timeouts.🛡️ Proposed fix
async healthCheck(): Promise<AuthHealthCheck> { try { const proxyFetch = createProxyFetch(); - // Better Auth typically exposes session endpoint that we can check - const response = await proxyFetch(`${this.baseUrl}/api/auth/session`); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + + const response = await proxyFetch(`${this.baseUrl}/api/auth/session`, { + signal: controller.signal, + }); + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/betterAuth.ts` around lines 317 - 339, The healthCheck method in betterAuth.ts calls createProxyFetch(`${this.baseUrl}/api/auth/session`) without a timeout; modify healthCheck to use the same timeout pattern as validateSession by creating an AbortController, set a short timeout (e.g., 2–5s) to call controller.abort(), pass the controller.signal into the proxyFetch call, and ensure the abort timer is cleared on success; update error handling to treat an AbortError as an unhealthy/service-timeout case while preserving the existing response status logic in healthCheck.src/lib/auth/providers/oauth2.ts-139-147 (1)
139-147:⚠️ Potential issue | 🟡 MinorEmpty string fallback for user ID could cause downstream issues.
If
payload.subis undefined, the user ID becomes an empty string (""), which could cause problems with session tracking, database lookups, or security checks that expect valid identifiers.🛡️ Proposed fix
Consider returning an invalid result if
subis missing:+ if (!payload.sub) { + return { + valid: false, + error: "JWT missing required 'sub' claim", + }; + } + const user: AuthUser = { - id: payload.sub ?? "", + id: payload.sub, email: payload.email as string | undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 139 - 147, The code currently constructs an AuthUser with id: payload.sub ?? "" which allows an empty-string ID; change the logic in the oauth2 provider where the AuthUser is created (the block that builds AuthUser) to validate payload.sub first and return/raise an invalid authentication result instead of creating a user with an empty id. Specifically, check payload.sub (and optionally payload.sub.trim()) before building AuthUser, and if it's missing/empty, return a failure (or throw a clear error) from the function that processes the token so callers do not receive an AuthUser with an empty id.src/lib/auth/AuthProviderRegistry.ts-403-422 (1)
403-422:⚠️ Potential issue | 🟡 MinorWeak health check implementation.
The health check only verifies that
provider.type === type, which is essentially checking if the provider was created with the correct type. This doesn't actually test connectivity or provider health. Most providers implement ahealthCheck()method that should be called instead.🔧 Proposed fix to use provider's healthCheck method
try { const provider = await this.createProvider(type, config); - // Simple health check - try to access config - const healthy = provider.type === type; + // Use provider's health check if available + let healthy = true; + if (typeof provider.healthCheck === "function") { + const healthResult = await provider.healthCheck(); + healthy = healthResult.healthy; + } // Clean up if provider has dispose method - if (provider.dispose) { + if (typeof provider.dispose === "function") { await provider.dispose(); + } else if (typeof provider.cleanup === "function") { + await provider.cleanup(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderRegistry.ts` around lines 403 - 422, The current health check only compares provider.type and must instead call the provider's healthCheck method to verify real connectivity; in the health check flow where you call this.createProvider(type, config) (and later provider.dispose), invoke await provider.healthCheck() if present and use its boolean/result to set the ProviderHealthStatus. Maintain cleanup by still calling provider.dispose() when defined, compute latency from startTime, populate ProviderHealthStatus (type, healthy from healthCheck, lastCheck, latency) and update this.healthCache.set(type, status) before returning.src/lib/auth/types/authTypes.ts-719-724 (1)
719-724:⚠️ Potential issue | 🟡 MinorInterface signature mismatch for
authenticateToken.The
MastraAuthProviderinterface definesauthenticateToken(token: string)without a context parameter, butBaseAuthProviderdeclares it asauthenticateToken(token: string, context?: AuthRequestContext). This inconsistency could cause issues for providers that need request context.🔧 Proposed fix in authTypes.ts
/** * Validate and authenticate a token * `@param` token - JWT or access token to validate + * `@param` context - Optional request context for additional validation * `@returns` Validation result with user data if valid */ - authenticateToken(token: string): Promise<TokenValidationResult>; + authenticateToken( + token: string, + context?: AuthRequestContext, + ): Promise<TokenValidationResult>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/types/authTypes.ts` around lines 719 - 724, The interface signature for authenticateToken in MastraAuthProvider doesn't include the optional context parameter and must match BaseAuthProvider; update MastraAuthProvider.authenticateToken to accept (token: string, context?: AuthRequestContext): Promise<TokenValidationResult> so implementations can receive the optional AuthRequestContext; ensure you import or reference AuthRequestContext and TokenValidationResult types used by BaseAuthProvider and keep the return type as Promise<TokenValidationResult>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ecf3bf4-16d1-4794-99b5-e935fdab35c4
📒 Files selected for processing (43)
docs/superpowers/plans/2026-03-16-auth-providers-implementation.mddocs/superpowers/specs/2026-03-16-auth-providers-redesign.mdpackage.jsonsrc/cli/commands/authProviders.tssrc/cli/factories/authCommandFactory.tssrc/cli/parser.tssrc/lib/auth/AuthProviderFactory.tssrc/lib/auth/AuthProviderRegistry.tssrc/lib/auth/RequestContext.tssrc/lib/auth/authContext.tssrc/lib/auth/authErrors.tssrc/lib/auth/authProvider.tssrc/lib/auth/index.tssrc/lib/auth/middleware/AuthMiddleware.tssrc/lib/auth/middleware/rateLimitByUser.tssrc/lib/auth/providers/BaseAuthProvider.tssrc/lib/auth/providers/CognitoProvider.tssrc/lib/auth/providers/KeycloakProvider.tssrc/lib/auth/providers/auth0.tssrc/lib/auth/providers/betterAuth.tssrc/lib/auth/providers/clerk.tssrc/lib/auth/providers/custom.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/oauth2.tssrc/lib/auth/providers/supabase.tssrc/lib/auth/providers/workos.tssrc/lib/auth/serverBridge.tssrc/lib/auth/sessionManager.tssrc/lib/auth/types/authTypes.tssrc/lib/index.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/server/routes/agentRoutes.tssrc/lib/types/authTypes.tssrc/lib/types/configTypes.tssrc/lib/types/generateTypes.tssrc/lib/types/index.tssrc/lib/types/streamTypes.tstest/continuous-test-suite-auth.tstest/fixtures/auth/provider-config.jsontest/fixtures/auth/session-data.jsontest/fixtures/auth/test-credentials.json
| // Handle per-call auth token validation | ||
| if (enhancedOptions.auth?.token) { | ||
| await this.ensureAuthProvider(); | ||
| if (!this.authProvider) { | ||
| throw new Error( | ||
| "No auth provider configured. Set auth in constructor or via setAuthProvider().", | ||
| ); | ||
| } | ||
| const result = await this.authProvider.authenticateToken( | ||
| enhancedOptions.auth.token, | ||
| ); | ||
| if (!result.valid) { | ||
| const { InvalidTokenError } = await import("./auth/authErrors.js"); | ||
| throw new InvalidTokenError( | ||
| result.error || "Token validation failed", | ||
| this.authProvider.type, | ||
| ); | ||
| } | ||
| // Merge validated user into context | ||
| if (result.user) { | ||
| enhancedOptions.context = { | ||
| ...enhancedOptions.context, | ||
| userId: result.user.id, | ||
| userEmail: result.user.email, | ||
| userRoles: result.user.roles, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Handle pre-validated requestContext | ||
| if (enhancedOptions.requestContext) { | ||
| enhancedOptions.context = { | ||
| ...enhancedOptions.context, | ||
| ...enhancedOptions.requestContext, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Merge requestContext before token-derived identity.
If callers provide both auth.token and requestContext, the second spread here can overwrite userId, userEmail, and userRoles that were just derived from a validated token. That collapses the trust boundary the reserved-key design is trying to create unless these auth-owned keys are merged last or explicitly filtered out.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-16-auth-providers-implementation.md` around
lines 310 - 345, Currently requestContext is merged after token-derived
identity, allowing it to overwrite userId/userEmail/userRoles; change the merge
order or filter reserved keys so token-derived identity wins: when handling
enhancedOptions.requestContext, either spread requestContext into
enhancedOptions.context before the token-derived merge (move the requestContext
block above the token validation) or remove reserved keys (userId, userEmail,
userRoles) from enhancedOptions.requestContext before spreading; reference
ensureAuthProvider, authProvider.authenticateToken, and the
enhancedOptions.context merges to locate where to reorder or filter.
| context: { | ||
| sessionId: request.sessionId, | ||
| userId: request.userId, | ||
| sessionId: ctx.session?.id ?? request.sessionId, | ||
| userId: ctx.user?.id ?? request.userId, | ||
| userEmail: ctx.user?.email, | ||
| userRoles: ctx.user?.roles, | ||
| requestId: ctx.requestId, | ||
| }, |
There was a problem hiding this comment.
Don't promote caller-supplied IDs into trusted auth context.
request.userId and request.sessionId come from the body, but these blocks forward them as context.userId / context.sessionId whenever ctx.user or ctx.session is missing. That gives unauthenticated callers a direct impersonation path for any downstream logic that keys memory, tools, rate limiting, or auditing off the execution context. Only populate identity from ctx.user / ctx.session; if you still need the raw request values, keep them under separate untrusted fields.
Suggested hardening
context: {
- sessionId: ctx.session?.id ?? request.sessionId,
- userId: ctx.user?.id ?? request.userId,
+ sessionId: ctx.session?.id,
+ userId: ctx.user?.id,
userEmail: ctx.user?.email,
userRoles: ctx.user?.roles,
requestId: ctx.requestId,
+ requestedSessionId: request.sessionId,
+ requestedUserId: request.userId,
},Also applies to: 133-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/routes/agentRoutes.ts` around lines 70 - 76, The context
object in agentRoutes.ts is promoting caller-supplied IDs into the trusted
execution context by using request.sessionId and request.userId when ctx.session
or ctx.user are absent; update the logic in the context assignment (the block
that sets context.sessionId, context.userId, context.userEmail,
context.userRoles, context.requestId) to only populate sessionId and userId from
ctx.session and ctx.user respectively and never fallback to request.* for those
trusted fields, and if you still need the raw body values keep them under
clearly untrusted names (e.g., rawRequestSessionId, rawRequestUserId) so
downstream code uses only ctx.user/ctx.session for authentication/authorization;
apply the same change in the analogous block around the 133-139 region.
0c52a72 to
21834e2
Compare
Review Feedback Addressed (Cycle 1)Changes MadeAll 11 Copilot review comments + CI build failure resolved: CI Fix
Security Fixes (2)
Architecture/Consistency Fixes (9)
Files Modified
Validation
Requesting Re-review@copilot-pull-request-reviewer — All 11 inline comments addressed. Please re-review. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
21834e2 to
c492b6e
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
c492b6e to
4931b64
Compare
Review Feedback Addressed (Cycle 2 — CodeRabbit)Critical Security Fixes (3)
High Priority Fixes (6)
Minor Fixes (1)
Also Fixed
Validation
@coderabbitai review |
|
Triggering an incremental review now to inspect the Cycle 2 changes. 🧠 Learnings used✅ Actions performedReview triggered.
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
4931b64 to
c326783
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/factories/authCommandFactory.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting issues flagged by CI.
The pipeline reports formatting issues in this file. Run
pnpm formator your formatter to resolve before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/authCommandFactory.ts` at line 1, Prettier formatting errors are present in src/cli/factories/authCommandFactory.ts; run the project formatter (pnpm format) or your configured Prettier to reformat this file, then re-stage the changes. Locate the file and ensure exported symbols such as authCommandFactory or createAuthCommand (and any surrounding JSDoc/header comments) are properly formatted according to the repo Prettier settings, fix any trailing whitespace or missing semicolons reported by CI, and commit the formatted file so the pipeline passes.
♻️ Duplicate comments (4)
src/lib/server/routes/agentRoutes.ts (1)
70-76:⚠️ Potential issue | 🔴 CriticalSecurity: Caller-supplied IDs still fall back to request body values.
The previous review correctly identified this as a privilege escalation vector. While the PR summary mentions fixing token precedence in
neurolink.ts, this route still falls back torequest.sessionId/request.userIdwhenctx.session/ctx.userare missing, allowing unauthenticated callers to impersonate users in downstream logic (memory, tools, rate limiting, auditing).🔒 Recommended fix
context: { - sessionId: ctx.session?.id ?? request.sessionId, - userId: ctx.user?.id ?? request.userId, + sessionId: ctx.session?.id, + userId: ctx.user?.id, userEmail: ctx.user?.email, userRoles: ctx.user?.roles, requestId: ctx.requestId, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/agentRoutes.ts` around lines 70 - 76, The context construction in agentRoutes.ts currently falls back to caller-supplied values (request.sessionId/request.userId), enabling impersonation; change the context to use only authenticated values (ctx.session?.id and ctx.user?.id) without falling back to request.sessionId or request.userId, i.e., remove the "?? request.sessionId" and "?? request.userId" fallbacks for sessionId and userId (leave them undefined when ctx lacks them), and ensure authentication/authorization is enforced earlier (e.g., return 401 or validate in the calling middleware) rather than relying on request body IDs.src/lib/auth/providers/CognitoProvider.ts (1)
241-257:⚠️ Potential issue | 🔴 Critical
verifySignature()does not perform cryptographic verification - security bypass.This was flagged in a previous review and remains unfixed. After finding a matching
kid, the method returnstruewithout verifying the signature. Any token with valid structure and a copiedkidwill pass authentication.The
joselibrary is already a project dependency. Use it to properly verify the token.🔒 Proposed fix using jose library
+import * as jose from "jose"; + // In verifySignature method: // Get JWKS const jwks = await this.getJWKS(); const key = jwks.keys.find((k) => k.kid === kid); if (!key) { logger.warn(`[CognitoProvider] Key not found for kid: ${kid}`); return false; } - // For full verification, use jose library - // For now, we trust the token structure - return true; + // Import the JWK and verify the signature + const publicKey = await jose.importJWK(key, header.alg); + await jose.jwtVerify(token, publicKey, { + issuer: this.expectedIssuer, + audience: this.cognitoConfig.clientId, + }); + return true; } catch (error) { logger.error(`[CognitoProvider] Signature verification error:`, error); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 241 - 257, The verifySignature method currently returns true after finding a matching kid without cryptographic verification; update verifySignature to use the jose library to actually verify the token signature: use getJWKS() to find the matching JWK for the token's kid, convert/import that JWK (via jose importJWK or createRemoteJWKSet) and call jose.jwtVerify (or appropriate verifier) against the raw token and the imported key, ensure you validate alg/exp/iss/aud as needed, return true only on successful verification and false on any verification error, and keep the existing logger.error path for exceptions; refer to verifySignature, getJWKS, and the kid/token variables to locate where to insert the jose-based verification.src/lib/neurolink.ts (2)
3015-3048:⚠️ Potential issue | 🟠 MajorDo not overwrite
requestContextidentity withundefinedtoken fields.Line 3020/3021 and Line 5600/5601 can carry
undefined, and Line 3047 / Line 5627 spreads those values last. That can erase validrequestContext.userEmail/userRoles.🛠️ Suggested fix (copy only defined token-derived fields)
-const tokenDerivedFields = - options.auth?.token && this.authProvider - ? { - userId: (options.context as Record<string, unknown> | undefined)?.userId, - userEmail: (options.context as Record<string, unknown> | undefined)?.userEmail, - userRoles: (options.context as Record<string, unknown> | undefined)?.userRoles, - } - : {}; +const tokenDerivedFields: Record<string, unknown> = {}; +if (options.auth?.token && this.authProvider) { + const ctx = (options.context as Record<string, unknown> | undefined) ?? {}; + if (ctx.userId !== undefined) tokenDerivedFields.userId = ctx.userId; + if (ctx.userEmail !== undefined) tokenDerivedFields.userEmail = ctx.userEmail; + if (ctx.userRoles !== undefined) tokenDerivedFields.userRoles = ctx.userRoles; +}Apply the same change to both
generate()andstream().Also applies to: 5595-5628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3015 - 3048, The code is overwriting requestContext identity fields with undefined token-derived values; update the logic that builds tokenDerivedFields so it only includes properties that are defined (e.g., check auth?.token && this.authProvider then conditionally add userId, userEmail, userRoles only if they are not undefined) before spreading into options.context; apply the same change in both generate() and stream() so options.context = { ...(options.context || {}), ...options.requestContext, ...tokenDerivedFields } never erases valid requestContext values with undefined token fields.
519-523:⚠️ Potential issue | 🔴 CriticalAuth context is instance-scoped and can leak across concurrent requests.
Line 521 / Line 10993 store auth context on the
NeuroLinkinstance, while Line 11030 exports a shared singleton. Concurrent requests can overwrite each other’s auth context.🔒 Suggested fix (request-scoped auth context via AsyncLocalStorage)
+const authContextStorage = + new AsyncLocalStorage<AuthenticatedContext | undefined>(); export class NeuroLink { private authProvider?: MastraAuthProvider; - private authContext?: AuthenticatedContext; private pendingAuthConfig?: NeuroLinkAuthConfig; @@ setAuthContext(context: AuthenticatedContext): void { - this.authContext = context; + authContextStorage.enterWith(context); logger.debug("Auth context set", { userId: context.user.id, provider: context.provider, sessionId: context.session.id, }); } getAuthContext(): AuthenticatedContext | undefined { - return this.authContext; + return authContextStorage.getStore(); } clearAuthContext(): void { - const userId = this.authContext?.user.id; - this.authContext = undefined; + const userId = authContextStorage.getStore()?.user.id; + authContextStorage.enterWith(undefined); if (userId) { logger.debug(`Auth context cleared for user: ${userId}`); } }Also applies to: 10992-11017, 11030-11030
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 519 - 523, NeuroLink currently stores request-specific state (authContext and pendingAuthConfig) as instance fields on the NeuroLink class (authContext, pendingAuthConfig, and authProvider of type MastraAuthProvider), which will leak/overwrite when the NeuroLink instance is exported as a shared singleton; refactor to make auth state request-scoped: remove instance fields for authContext/pendingAuthConfig (and avoid mutating authProvider per-request) and instead use an AsyncLocalStorage store (or require an explicit AuthenticatedContext param) to set/get authContext and pendingAuthConfig per request; update all methods that read/write authContext/pendingAuthConfig to pull from the AsyncLocalStorage store (or accept the context param) so the exported shared NeuroLink singleton no longer holds mutable per-request state.
🟠 Major comments (20)
src/lib/auth/providers/CognitoProvider.ts-269-274 (1)
269-274:⚠️ Potential issue | 🟠 MajorAdd timeout to JWKS fetch to prevent indefinite hanging.
The
fetch()call has no timeout, which could cause the authentication flow to hang indefinitely if the Cognito endpoint is unresponsive.🛡️ Proposed fix using AbortController
try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10s timeout + - const response = await fetch(this.jwksUri); + const response = await fetch(this.jwksUri, { + signal: controller.signal, + }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error(`JWKS fetch failed: ${response.status}`); }As per coding guidelines: "Use withTimeout for async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 269 - 274, The JWKS fetch in CognitoProvider (the fetch(this.jwksUri) call) lacks a timeout; wrap the fetch in a cancellable timeout using the project's withTimeout utility or an AbortController so the request cannot hang indefinitely. Modify the code in the method that uses this.jwksUri to create an AbortController, pass its signal to fetch, and race/withTimeout the fetch so that on timeout you abort the controller and throw a clear error (e.g., "JWKS fetch timed out") before the existing response.ok check; ensure existing error handling/logging remains and include the original error/timeouts in the thrown/logged message.src/lib/neurolink.ts-10979-10985 (1)
10979-10985:⚠️ Potential issue | 🟠 Major
ensureAuthProvider()needs in-flight deduplication.Parallel first authenticated calls can race through Line 10980 and initialize multiple providers, because factory creation is per-call and not internally deduplicated.
♻️ Suggested fix (coalesce concurrent auth init)
export class NeuroLink { private authProvider?: MastraAuthProvider; private pendingAuthConfig?: NeuroLinkAuthConfig; + private authInitPromise: Promise<void> | null = null; @@ private async ensureAuthProvider(): Promise<void> { if (this.authProvider || !this.pendingAuthConfig) { return; } - await this.setAuthProvider(this.pendingAuthConfig); - this.pendingAuthConfig = undefined; + if (this.authInitPromise) { + return this.authInitPromise; + } + this.authInitPromise = (async () => { + await this.setAuthProvider(this.pendingAuthConfig!); + this.pendingAuthConfig = undefined; + })(); + try { + await this.authInitPromise; + } finally { + this.authInitPromise = null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 10979 - 10985, ensureAuthProvider can race when multiple callers enter before authProvider is set; add an in-flight deduplication promise (e.g., a private field like authInitPromise: Promise<void> | undefined) so only the first caller triggers setAuthProvider(pendingAuthConfig) and others await the same promise; implement logic in ensureAuthProvider to return immediately if authProvider exists, if authInitPromise exists await it, otherwise create and assign authInitPromise = (async () => { await this.setAuthProvider(this.pendingAuthConfig); this.pendingAuthConfig = undefined; })(), await it, then finally clear authInitPromise (and ensure it clears on rejection) so subsequent attempts can retry.src/lib/neurolink.ts-10947-10955 (1)
10947-10955:⚠️ Potential issue | 🟠 MajorRemove the
anycast by properly typing the auth config parameter.The
anycast at line 10951 violates the strict TypeScript configuration requirement enforced in src/. However, this is a systemic issue—the factory'screate()method signature declares it acceptsAuthProviderConfig(a generic interface), butNeuroLinkAuthConfigafter type narrowing is a union of specific provider configs (Auth0Config, ClerkConfig, FirebaseConfig, etc.) that don't align with the generic interface.This same pattern repeats throughout AuthProviderFactory's internal provider registrations (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, BetterAuth, WorkOS, Custom, OAuth2, JWT—all using
as anycasts).To fix this properly:
- Update the factory's
create()method signature to accept the union of specific provider configs instead of the generic interface, or- Ensure the specific provider config types (Auth0Config, ClerkConfig, etc.) align with the generic AuthProviderConfig interface, or
- Add explicit type narrowing/guards to eliminate the need for casting.
This requires refactoring the auth provider type architecture to maintain strict TypeScript compliance across the system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 10947 - 10955, The auth config is being passed to AuthProviderFactory.create with an unsafe any cast; remove the cast by making the factory accept the concrete union or by narrowing the config before the call. Update AuthProviderFactory.create's signature to accept NeuroLinkAuthConfig (the union of Auth0Config | ClerkConfig | FirebaseConfig | SupabaseConfig | CognitoConfig | KeycloakConfig | BetterAuthConfig | WorkOSConfig | CustomConfig | OAuth2Config | JWTConfig), or add overloads/discriminated-union typing so calls from NeuroLinkAuthConfig (in neurolink.ts) are type-safe, and then remove all other `as any` casts used inside AuthProviderFactory when registering providers (e.g., the Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, BetterAuth, WorkOS, Custom, OAuth2, JWT registrations) by adjusting provider registration types or adding explicit type guards to narrow to the correct provider config before instantiation.src/lib/auth/providers/oauth2.ts-109-123 (1)
109-123:⚠️ Potential issue | 🟠 MajorJWKS validation never initializes on first use.
authenticateToken()only enters the JWT path whenthis.jwksis already populated, but nothing callsinitialize()before that. WithjwksUrlconfigured and no manual pre-initialization, the first auth attempt falls straight through to"No validation method available".🐛 Minimal fix
async authenticateToken( token: string, _context?: AuthRequestContext, ): Promise<TokenValidationResult> { - // Try JWKS validation first if available - if (this.jwksUrl && this.jwks) { + if (this.jwksUrl && !this.jwks) { + await this.initialize(); + } + + if (this.jwks) { try { const { payload } = await jose.jwtVerify(token, this.jwks);Also applies to: 130-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 109 - 123, The JWKS setup in initialize() is never invoked before authenticateToken() runs, so authentication skips JWT validation when this.jwks is unset; modify authenticateToken() (and any other token/validation entrypoints around the jwt-handling block) to lazily initialize JWKS by awaiting this.initialize() when this.jwks is undefined and this.jwksUrl is present, ensuring initialize() is idempotent and returns immediately if jwks is already set; reference initialize(), authenticateToken(), this.jwks and jwksUrl to locate and update the logic.src/lib/auth/providers/BaseAuthProvider.ts-425-460 (1)
425-460:⚠️ Potential issue | 🟠 MajorRBAC wildcards and transitive inheritance are missing here.
authorize()only does exact permission matches, andgetEffectiveRoles()expands the hierarchy one hop. Configs likerolePermissions: { admin: ["*"] }, namespace wildcards such asagents:*, or chains likeadmin -> editor -> viewerwill be denied unless every effective permission/role is duplicated manually.Also applies to: 477-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 425 - 460, The authorize() logic must be extended to support wildcard permission matching and full transitive role inheritance: update getEffectiveRoles() to compute the transitive closure of the role hierarchy (follow parent links repeatedly so chains like admin->editor->viewer are included), update getEffectivePermissions() to expand rolePermissions entries (treat a permission of "*" as matching everything and preserve namespace patterns like "agents:*"), and change the authorize() checks to consider wildcard/prefix matches (e.g., treat "agents:*" or "*" as satisfying "agents:read") instead of only exact set membership; ensure missingRoles/missingPermissions are computed against these expanded/evaluated sets so rolePermissions, namespace wildcards, and multi-hop inheritance are honored.test/continuous-test-suite-auth.ts-109-112 (1)
109-112:⚠️ Potential issue | 🟠 MajorUse
createRoleAuthMiddlewarein the RBAC section.Section 2 mounts
createBearerAuthMiddleware()and then hand-rolls its own permission check inbuildRBACApp()instead of exercising the exported RBAC middleware. This suite can pass whilecreateRoleAuthMiddleware()regresses, because the helper imported on Line 111 is never actually used.Also applies to: 501-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-auth.ts` around lines 109 - 112, The RBAC test suite currently mounts createBearerAuthMiddleware() but uses a hand-rolled permission check inside buildRBACApp() instead of exercising the exported RBAC middleware; replace the custom permission-checking logic in buildRBACApp() with the createRoleAuthMiddleware(...) middleware (pass the same role/permission config used by the custom check) so the tests actually invoke createRoleAuthMiddleware and will catch regressions to that exported helper rather than silently passing via the ad-hoc check.src/lib/auth/providers/custom.ts-113-127 (1)
113-127:⚠️ Potential issue | 🟠 MajorDon't hide custom session backend failures behind an in-memory fallback.
If
createSessionFnthrows, this code logs a warning and returns a local session anyway. That makes upstream storage/revocation failures look like success and can desynchronize session state from the caller's auth system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/custom.ts` around lines 113 - 127, The custom session creation block around createSessionFn should not swallow errors and proceed with an in-memory fallback; change the behavior so that if this.createSessionFn throws you log the error and rethrow it (or reject the caller) instead of creating/returning a local session via this.sessions/this.userSessions and emitting "auth:login". In practice, update the try/catch around this.createSessionFn(user, context) to remove the fallback path and propagate the original error (after logging) so upstream storage/revocation failures are visible to callers.src/lib/auth/providers/BaseAuthProvider.ts-234-388 (1)
234-388:⚠️ Potential issue | 🟠 MajorThe shared session policy is not actually applied to the new providers.
This base class centralizes
customStorage,allowMultipleSessions,maxSessionsPerUser,touch(), and revocation behavior, but the new providers in this PR keep their ownMap-backed create/get/refresh/destroy flows instead of delegating here. Session config will behave differently per provider until those lifecycles are routed through the base implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 234 - 388, New providers bypass the BaseAuthProvider session lifecycle by using their own Map-backed create/get/refresh/destroy flows; update each provider to delegate session operations to the base implementation (use BaseAuthProvider.createSession, validateSession, refreshSession, revokeSession, revokeAllSessions and this.sessionStorage) instead of maintaining internal Maps so the centralized sessionConfig fields (allowMultipleSessions, maxSessionsPerUser, autoRefresh/refreshThreshold, touch()) and revocation behavior are consistently applied.src/lib/auth/types/authTypes.ts-1-7 (1)
1-7:⚠️ Potential issue | 🟠 MajorMake these auth contracts a single source of truth.
This new shared surface sits outside
src/lib/types, and it's already drifting internally:CustomAuthConfig.validateToken/createSessionare context-aware, whileMastraAuthProvider.authenticateToken/createSessiondrop that context. Consumers typed against the interface cannot forward request metadata consistently, and sibling providers in this PR are already importing different auth type modules.Based on learnings: Project standard: place reusable/shared types under
src/lib/types/*.ts; all new type definitions outside this directory should be flagged and blocked.Also applies to: 420-451, 712-738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/types/authTypes.ts` around lines 1 - 7, The auth type definitions in authTypes.ts must be moved into the project-wide types area and unified so providers share one contract: relocate these types into src/lib/types (e.g., types/auth.ts) and export them as the single source of truth; update the signatures for CustomAuthConfig (validateToken, createSession) and MastraAuthProvider.authenticateToken/createSession to accept the same request/context parameter (the request metadata/context object used across providers) so consumers can forward metadata consistently, then adjust all imports to reference the new unified types file and remove the duplicate authTypes.ts definitions.test/continuous-test-suite-auth.ts-118-158 (1)
118-158:⚠️ Potential issue | 🟠 MajorTighten the
SKIP:classifier.Substrings like
"not found"and"failed to"are broad enough to catch real auth regressions and rewrite them into skips. This should only skip on explicit missing-credential/connectivity signals or provider-specific error codes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-auth.ts` around lines 118 - 158, The classifier is too broad and converts real auth regressions into skips; update isExpectedProviderError to only match explicit credential/connectivity signals and provider-specific error codes (e.g., exact tokens like "api key", "api_key", "authentication", "credentials", "cannot connect", "ECONNREFUSED"/"econnrefused", "ENOTFOUND"/"enotfound", "403", "429", "quota", "permission denied", "billing", "google_application_credentials", "application default credentials", "service account", "project_id", "default credentials", "no providers", "invalid api", "missing api", "could not resolve", "network", "timeout") and remove overly broad substrings such as "not found" and "failed to" (and any other generic phrases that could match unrelated errors); update isExpectedProviderError (and consequently skipIfProviderError which uses it) to use these tightened, explicit patterns and consider matching whole words or uppercase variants for provider codes to avoid accidental matches.src/lib/auth/providers/auth0.ts-89-99 (1)
89-99:⚠️ Potential issue | 🟠 MajorUse
clientIdin verification or stop requiring it.The constructor rejects configs without
clientId, but token verification only usesthis.audience. If callers provide the requiredclientIdand omitaudience, the value they were forced to configure never constrains verification.🔧 Common fix
- this.audience = config.audience; + this.audience = config.audience ?? config.clientId;Also applies to: 139-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/auth0.ts` around lines 89 - 99, The constructor currently enforces a required clientId but the token verification logic only uses this.audience, so either stop requiring clientId or incorporate it into verification; update the Auth0 provider constructor/validation and the verifyToken (or verify) routine so that if clientId is provided/required (this.clientId) it is passed into the Auth0/JWT verification options (as an allowed audience or client check) or, alternatively, remove clientId from the InvalidConfigurationError check and only require audience; make the change consistently for both the constructor checks and the other validation block referenced around the other occurrence (the code near the 139-142 check) so the required fields align with actual verification behavior.src/lib/auth/authProvider.ts-263-277 (1)
263-277:⚠️ Potential issue | 🟠 MajorDon't persist a brand-new session on every authenticated request.
All current providers back
createSession()with a stored UUID session and emitauth:login. Calling it unconditionally insideauthenticateRequest()turns normal bearer-token validation into unbounded session creation, duplicate login events, and needless Redis/Map growth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authProvider.ts` around lines 263 - 277, authenticateRequest currently calls createSession() for every validated token which causes unbounded session creation and duplicate auth:login events; change authenticateRequest to only call createSession(validation.user, context) when there is no existing session tied to the token or request (e.g., check for a session id or flag returned by authenticateToken like validation.sessionId or validation.existingSession) and otherwise skip session creation and do not emit auth:login; update authenticateToken/validation usage to surface existing session identity if available and ensure createSession is only invoked for fresh logins.src/lib/auth/authContext.ts-62-124 (1)
62-124:⚠️ Potential issue | 🟠 MajorWire the documented fallback into the exported helpers.
globalAuthContextis presented as the non-ALS path, but every public helper here still reads onlyauthContextStorage. In a runtime that callsglobalAuthContext.set(...),getAuthContext(),getCurrentUser(),getCurrentSession(),isAuthenticated(), andrequireAuth()will all still behave as if no user is present.Also applies to: 359-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authContext.ts` around lines 62 - 124, The exported helpers (getAuthContext, getCurrentUser, getCurrentSession, isAuthenticated, requireAuth) only read from authContextStorage and ignore the documented global fallback; update each to first try authContextStorage.getStore() and if that is undefined, fall back to globalAuthContext.get() (or the appropriate accessor on globalAuthContext) so the non-ALS path is honored; ensure requireAuth uses the fallback value before throwing and that isAuthenticated returns true when either store or globalAuthContext has a context. Reference symbols: authContextStorage, globalAuthContext, getAuthContext, getCurrentUser, getCurrentSession, isAuthenticated, requireAuth.src/lib/auth/authProvider.ts-52-59 (1)
52-59:⚠️ Potential issue | 🟠 MajorPartial
tokenExtractionconfigs currently erase the default header extractor.The constructor does a shallow spread. A config like
{ tokenExtraction: { fromCookie: ... } }replaces the whole default object, so bearer-header auth stops working as soon as a caller adds another extraction source.🔧 Preserve nested defaults
constructor(config: AuthProviderConfig) { + const defaultTokenExtraction = { + fromHeader: { name: "Authorization", scheme: "Bearer" }, + }; + this.config = { - required: true, - tokenExtraction: { - fromHeader: { name: "Authorization", scheme: "Bearer" }, - }, - ...config, + ...config, + required: config.required ?? true, + tokenExtraction: { + ...defaultTokenExtraction, + ...config.tokenExtraction, + fromHeader: { + ...defaultTokenExtraction.fromHeader, + ...config.tokenExtraction?.fromHeader, + }, + }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authProvider.ts` around lines 52 - 59, The constructor currently does a shallow merge so a caller-supplied tokenExtraction object replaces the default fromHeader extractor; update the constructor (in AuthProviderConfig handling inside the class constructor) to deep-merge tokenExtraction instead of overwriting it — keep the default tokenExtraction.fromHeader and merge any provided fields like fromCookie into it (e.g., compute final tokenExtraction by merging defaultTokenExtraction and config.tokenExtraction, then assign this.config with other top-level spreads such as required), so adding `{ tokenExtraction: { fromCookie: ... } }` augments rather than erases the default fromHeader.src/lib/auth/AuthProviderFactory.ts-136-139 (1)
136-139:⚠️ Potential issue | 🟠 MajorInitialize the factory before serving discovery data.
These discovery helpers read
registrationssynchronously. On a cold singleton that map is still empty until some other call runsensureInitialized(), so provider listing andhasProvider()become order-dependent. Either make the discovery APIs async and await initialization, or register the static metadata eagerly.Also applies to: 439-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderFactory.ts` around lines 136 - 139, The discovery APIs (e.g., AuthProviderFactory.getAvailableProviders()) access the registrations map synchronously on a cold singleton so listing and hasProvider() are order-dependent; update these static discovery helpers to ensure the factory is initialized before reading registrations: either make them async and await factory.ensureInitialized() (call AuthProviderFactory.getInstance().ensureInitialized()) or call a one-time eager initializer from getInstance() so registrations are populated before returning; apply the same change to the other static discovery methods (including hasProvider and the helpers around lines 439-482) so all discovery paths await or trigger ensureInitialized() before accessing registrations.src/lib/auth/providers/jwt.ts-133-147 (1)
133-147:⚠️ Potential issue | 🟠 MajorAdd the configured algorithm allow-list to JWT verification.
this.algorithmsis set during initialization but never passed tojwtVerify()inauthenticateToken(). This allows any algorithm compatible with the supplied key to be accepted, bypassing the intended restriction. A provider configured as["HS256"]can still validate tokens using other HMAC variants.Fix
const verifyOptions: jose.JWTVerifyOptions = {}; + verifyOptions.algorithms = this.algorithms; if (this.issuer) { verifyOptions.issuer = this.issuer; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/jwt.ts` around lines 133 - 147, The JWT verification is missing the algorithm allow-list: update authenticateToken to include this.algorithms in the options passed to jose.jwtVerify by setting verifyOptions.algorithms = this.algorithms (or merging into verifyOptions before calling jwtVerify), so jwtVerify(token, this.keyObject!, verifyOptions) enforces the configured algorithm list; reference the authenticateToken method, the verifyOptions object, this.algorithms and the jwtVerify call.src/lib/auth/middleware/rateLimitByUser.ts-472-475 (1)
472-475:⚠️ Potential issue | 🟠 MajorAlign this helper with
createAuthMiddleware()'s actual contract.This helper expects an auth function over
Requestreturning{ proceed, context, response? }, butcreateAuthMiddleware()from the sibling file resolves to a handler overAuthRequestContextwith{ proceed, context, error? }. The advertised composition here will not type-check or forward auth failures losslessly.Either accept the same middleware shape as
createAuthMiddleware(), or adaptRequesttoAuthRequestContextinside this helper before invoking auth.Also applies to: 487-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 472 - 475, The helper createAuthenticatedRateLimitMiddleware currently expects an auth function taking a Request and returning { proceed, context, response? } but createAuthMiddleware() actually returns a handler that accepts an AuthRequestContext and returns { proceed, context, error? }; update createAuthenticatedRateLimitMiddleware to either accept the same middleware shape as createAuthMiddleware (AuthRequestContext -> { proceed, context, error? }) or, if you want to keep a Request-based signature, adapt the Request into an AuthRequestContext before invoking the auth handler by constructing an AuthRequestContext object (including the original request and any required metadata) and translate the returned { error } into the helper’s expected response shape so auth failures are forwarded losslessly; ensure you update the call sites and the return handling around proceed/context/error to match the createAuthMiddleware contract (also apply the same change to the other instance noted).src/lib/auth/middleware/rateLimitByUser.ts-105-113 (1)
105-113:⚠️ Potential issue | 🟠 MajorDrive bucket expiry from
windowMs, not a fixed hour.Both backends drop state after one hour regardless of the configured refill window. Any rate limit with
windowMs > 3600000will reset early after one hour of inactivity, which grants more burst capacity than configured.Use the bucket's time-to-full / configured window when deciding the in-memory cleanup threshold and Redis TTL.
Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 105 - 113, The cleanup logic currently uses a fixed one-hour cutoff; change it to compute the expiry from the rate limit window (windowMs) and each bucket's time-to-full so buckets are only removed after they could have fully refilled. In cleanupExpiredBuckets(), replace the hardcoded oneHourAgo with a threshold derived from bucket.windowMs (or the middleware-configured windowMs) and bucket.timeToFull (or compute timeToFull = windowMs * (capacity - tokens) / capacity) and delete entries only when lastRefill < now - max(windowMs, timeToFull). Do the same for Redis key TTL setup (the code around the Redis set/expire logic referenced at lines ~126-130): set Redis TTL based on windowMs or computed time-to-full so remote state expires consistently with in-memory buckets. Ensure you reference and use the bucket properties (lastRefill, tokens, capacity) and the middleware's configured windowMs when computing these thresholds.src/lib/auth/middleware/AuthMiddleware.ts-577-585 (1)
577-585:⚠️ Potential issue | 🟠 MajorNormalize
req.urlbefore public-route matching.
req.urloften includes the query string or even an absolute URL. Using it verbatim ascontext.pathmeans values like/health?ping=1orhttps://host/healthwill not match/healthinisPublicRoute(), so public endpoints can unexpectedly require auth.Patch sketch
export function createRequestContext(req: { @@ }): AuthRequestContext { + const rawPath = req.path ?? req.url ?? "/"; + const path = + rawPath.startsWith("http://") || rawPath.startsWith("https://") + ? new URL(rawPath).pathname + : rawPath.split("?")[0].split("#")[0] || "/"; + return { method: req.method ?? "GET", - path: req.path ?? req.url ?? "/", + path, headers: req.headers ?? {}, cookies: req.cookies, query: req.query,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 577 - 585, The context.path assigned in AuthMiddleware (returned object where method/path/headers are set) uses req.url verbatim which may include query strings or absolute URLs causing isPublicRoute() mismatches; normalize req.url by parsing it and extracting only the pathname (strip query and origin) and prefer req.path if present, ensuring the resulting path starts with a "/" before assigning to context.path so public-route matching works reliably.src/lib/auth/middleware/AuthMiddleware.ts-245-280 (1)
245-280:⚠️ Potential issue | 🟠 MajorReject
validtoken results that do not include a user.The
user!/anypath here turns a malformed provider response into{ proceed: true }withcontext.user === undefined. Downstream code that only checksproceedwill treat that request as authenticated, so this branch needs to fail closed whenvalidationResult.useris missing.Patch sketch
- if (!validationResult.valid) { + if (!validationResult.valid || !validationResult.user) { // If auth is optional, proceed without user if (config.optional) { return { proceed: true }; } const error = { statusCode: 401, message: validationResult.error ?? "Invalid token", // eslint-disable-next-line `@typescript-eslint/no-explicit-any` code: (validationResult as any).errorCode ?? "AUTH-001", }; @@ const authenticatedContext: AuthenticatedContext = { ...context, - user: validationResult.user! as AuthUser, + user: validationResult.user, token, // eslint-disable-next-line `@typescript-eslint/no-explicit-any` claims: (validationResult as any).claims, };As per coding guidelines, "Use strict TypeScript configuration (--strict) everywhere. Enforce no-explicit-any error in src/ (warn in test/)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 245 - 280, The code currently trusts validationResult.valid even when validationResult.user is missing, which can mark requests as authenticated with user undefined; change the authenticate handling so that you require both validationResult.valid === true and validationResult.user to be present before creating AuthenticatedContext: treat missing user the same as a failed validation (honor config.optional and call config.onError if provided, and return { proceed: false, error } with the same error shape used for other failures). Update the branch that builds AuthenticatedContext (the spread into AuthenticatedContext, token and claims) to only run when validationResult.user exists and remove the unsafe any/non-null assertions (use proper types from provider.authenticateToken result) so malformed provider responses are rejected.
🟡 Minor comments (2)
src/lib/auth/providers/BaseAuthProvider.ts-370-376 (1)
370-376:⚠️ Potential issue | 🟡 MinorRevocation currently collapses into “not found”.
revokeSession()marks the session invalid and deletes it immediately, so the nextvalidateSession()returnsAUTH-008instead ofAUTH-010. Keep a revoked tombstone until expiry, or remove the revoked branch from validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 370 - 376, revokeSession currently sets session.isValid = false then immediately calls this.sessionStorage.delete(sessionId), causing validateSession to treat it as missing (AUTH-008) instead of revoked (AUTH-010); change revokeSession (in BaseAuthProvider) to set session.isValid = false and persist it via this.sessionStorage.save(session) but do NOT call this.sessionStorage.delete(sessionId) so the revoked tombstone remains until natural expiry (or alternatively update validateSession to return AUTH-010 when session.isValid === false), ensuring validateSession observes the revoked state rather than “not found”.src/lib/auth/providers/supabase.ts-169-176 (1)
169-176:⚠️ Potential issue | 🟡 MinorDon't treat a missing
email_confirmed_atfield as verified.
undefined !== nullistrue, so payloads that omitemail_confirmed_atget promoted toemailVerified: true. That can incorrectly unlock flows gated on verified email state.🔧 Narrow fix
- emailVerified: userData.email_confirmed_at !== null, + emailVerified: + userData.email_confirmed_at !== null && + userData.email_confirmed_at !== undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/supabase.ts` around lines 169 - 176, The returned user object currently sets emailVerified using "userData.email_confirmed_at !== null", which treats undefined as verified; change this to explicitly check for both null and undefined (e.g., use "userData.email_confirmed_at != null" or a boolean cast like "Boolean(userData.email_confirmed_at)") in the return block where id/email/name/picture/roles are assembled so emailVerified is only true when email_confirmed_at is actually present.
🧹 Nitpick comments (7)
src/lib/types/configTypes.ts (1)
107-119: Consider adding explicit union variants for all supported providers.The union type includes
auth0,clerk,firebase,supabase,workos, andbetter-auth, but the PR supports additional providers (jwt,oauth2,cognito,keycloak) that rely on the generic fallback{ type: AuthProviderType; config: AuthProviderConfig }. While functional, adding explicit variants would improve TypeScript IntelliSense and documentation.♻️ Suggested enhancement
export type NeuroLinkAuthConfig = | MastraAuthProvider | { provider: MastraAuthProvider } | { type: "auth0"; config: Auth0Config } | { type: "clerk"; config: ClerkConfig } | { type: "firebase"; config: FirebaseConfig } | { type: "supabase"; config: SupabaseConfig } | { type: "workos"; config: WorkOSConfig } | { type: "better-auth"; config: BetterAuthConfig } + | { type: "jwt"; config: import("./authTypes.js").JWTConfig } + | { type: "oauth2"; config: import("./authTypes.js").OAuth2Config } + | { type: "cognito"; config: import("./authTypes.js").CognitoConfig } + | { type: "keycloak"; config: import("./authTypes.js").KeycloakConfig } | { type: AuthProviderType; config: AuthProviderConfig };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/configTypes.ts` around lines 107 - 119, NeuroLinkAuthConfig currently lists several explicit providers but leaves jwt, oauth2, cognito, and keycloak handled only by the generic fallback; add explicit union variants for these providers (e.g., { type: "jwt"; config: AuthProviderConfig }, { type: "oauth2"; config: AuthProviderConfig }, { type: "cognito"; config: AuthProviderConfig }, { type: "keycloak"; config: AuthProviderConfig }) so TypeScript IntelliSense and docs surface them; update the NeuroLinkAuthConfig union to include these variants while keeping the existing fallback { type: AuthProviderType; config: AuthProviderConfig } for any other providers.src/cli/factories/authCommandFactory.ts (1)
102-103: Consider defining proper argument types for new command handlers.The
as anycasts work but lose type safety. SinceAuthCommandArgsinterface exists, consider extending it or creating specific interfaces for each new command's arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/authCommandFactory.ts` around lines 102 - 103, Replace the unsafe cast to any when calling handleProvidersCommand by defining and using proper argument types: create a specific interface (e.g., ProvidersCommandArgs) that extends the existing AuthCommandArgs (or extend AuthCommandArgs with the provider-specific fields) and update the handleProvidersCommand signature to accept that interface instead of any; then change the call from await handleProvidersCommand(argv as any) to await handleProvidersCommand(argv) so TypeScript enforces the correct shape and you may adjust the CLI parsing/command builder to produce the new typed argv where necessary.src/lib/auth/providers/CognitoProvider.ts (4)
191-199: Consider importingJsonValueat the top of the file.The inline
import("../../types/common.js").JsonValueworks but is inconsistent with the rest of the codebase. Moving it to the top-level imports improves readability.♻️ Suggested refactor
Add to imports at top:
import type { JsonValue } from "../../types/common.js";Then simplify:
- const validClaims: Record< - string, - import("../../types/common.js").JsonValue - > = {}; + const validClaims: Record<string, JsonValue> = {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 191 - 199, The inline type reference import("../../types/common.js").JsonValue in CognitoProvider.ts is inconsistent; add a top-level type import "import type { JsonValue } from '../../types/common.js';" and then replace the inline type on validClaims with Record<string, JsonValue>. Update occurrences in the file (e.g., the validClaims declaration in the CognitoProvider code) to use the imported JsonValue to keep imports consistent and improve readability.
22-27: Module-level JWKS cache limits testability.The singleton
jwksCachemakes it harder to isolate tests or reset state between provider instances. Consider injecting the cache or exposing aclearCache()method for testing scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 22 - 27, The module-level singleton jwksCache (type JWKSCacheEntry) reduces testability; modify CognitoProvider to accept an optional JWKS cache via constructor parameter (e.g., allow passing Map<string, JWKSCacheEntry>) and fall back to the existing jwksCache if none provided, or alternatively add and export a clearCache() function that clears the module jwksCache; update constructor/usage in functions that reference jwksCache to use the injected instance (or callers to call clearCache() in tests) so tests can provide a fresh cache or reset state between runs.
353-355: Redundant undefined check fortokenUse.The
tokenUseparameter is always a string ("id"or"access") when this method is called fromauthenticateToken. The undefined check is unnecessary.- if (tokenUse !== undefined) { - providerData.token_use = tokenUse; - } + providerData.token_use = tokenUse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 353 - 355, The conditional guarding assignment to providerData.token_use is redundant because tokenUse is always a string when called from authenticateToken; replace the if (tokenUse !== undefined) { providerData.token_use = tokenUse; } with a direct assignment providerData.token_use = tokenUse and, if applicable, tighten the method signature/type for the parameter in CognitoProvider (and any callers) to reflect a non-optional string to avoid future confusion.
379-387: Stub implementation returnsnullsilently.The method logs at debug level and returns
null, which callers may not distinguish from "user not found." Consider documenting this limitation in the class docstring or throwingAuthProviderError.create("NOT_IMPLEMENTED", ...)if this feature is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 379 - 387, The getUser method in CognitoProvider currently logs a debug message and returns null, which is ambiguous; replace the silent null return with a clear not-implemented error by throwing AuthProviderError.create("NOT_IMPLEMENTED", "<descriptive message>") from the getUser method (and optionally call logger.error or logger.warn before throwing). Update the CognitoProvider class docstring to note that getUser requires the AWS SDK if you prefer a documented stub instead of throwing, but prefer throwing AuthProviderError.create("NOT_IMPLEMENTED", ...) inside getUser so callers can distinguish “not implemented” from “user not found.”src/lib/auth/middleware/AuthMiddleware.ts (1)
11-23: Prefer$lib/@aliases for new imports.These deep relative imports will get brittle quickly as the auth package moves. Please use the repo TypeScript aliases here instead of introducing another relative-import island.
As per coding guidelines, "Use path aliases in TypeScript: $lib, $lib/* for SDK and @ → ./src for imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 11 - 23, Replace the deep relative imports in AuthMiddleware.ts with the project's TypeScript path aliases (e.g., $lib or @) so imports like createErrorFactory, logger, AuthProviderFactory and the auth types (AuthErrorCode, AuthenticatedContext, AuthMiddlewareConfig, AuthorizationResult, AuthRequestContext, AuthUser, RBACMiddlewareConfig, TokenExtractionConfig) use the configured aliases instead of long relative paths; update each import statement to the corresponding alias-based path while preserving the same named imports and exported symbols so AuthMiddleware continues to reference the same functions, classes and types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d16b47c-6def-4e6d-912f-890c6dd94a11
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
docs/superpowers/plans/2026-03-16-auth-providers-implementation.mddocs/superpowers/specs/2026-03-16-auth-providers-redesign.mdpackage.jsonsrc/cli/commands/authProviders.tssrc/cli/factories/authCommandFactory.tssrc/cli/parser.tssrc/lib/auth/AuthProviderFactory.tssrc/lib/auth/AuthProviderRegistry.tssrc/lib/auth/RequestContext.tssrc/lib/auth/authContext.tssrc/lib/auth/authErrors.tssrc/lib/auth/authProvider.tssrc/lib/auth/index.tssrc/lib/auth/middleware/AuthMiddleware.tssrc/lib/auth/middleware/rateLimitByUser.tssrc/lib/auth/providers/BaseAuthProvider.tssrc/lib/auth/providers/CognitoProvider.tssrc/lib/auth/providers/KeycloakProvider.tssrc/lib/auth/providers/auth0.tssrc/lib/auth/providers/betterAuth.tssrc/lib/auth/providers/clerk.tssrc/lib/auth/providers/custom.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/oauth2.tssrc/lib/auth/providers/supabase.tssrc/lib/auth/providers/workos.tssrc/lib/auth/serverBridge.tssrc/lib/auth/sessionManager.tssrc/lib/auth/types/authTypes.tssrc/lib/index.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/server/routes/agentRoutes.tssrc/lib/types/authTypes.tssrc/lib/types/configTypes.tssrc/lib/types/generateTypes.tssrc/lib/types/index.tssrc/lib/types/streamTypes.tstest/continuous-test-suite-auth.tstest/fixtures/auth/provider-config.jsontest/fixtures/auth/session-data.jsontest/fixtures/auth/test-credentials.json
✅ Files skipped from review due to trivial changes (6)
- package.json
- src/cli/parser.ts
- test/fixtures/auth/session-data.json
- docs/superpowers/plans/2026-03-16-auth-providers-implementation.md
- docs/superpowers/specs/2026-03-16-auth-providers-redesign.md
- src/lib/types/authTypes.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/types/streamTypes.ts
- src/lib/auth/serverBridge.ts
- src/lib/auth/RequestContext.ts
- src/lib/auth/providers/firebase.ts
- src/lib/types/index.ts
- src/lib/auth/authErrors.ts
- src/lib/auth/index.ts
- src/cli/commands/authProviders.ts
- src/lib/auth/providers/KeycloakProvider.ts
c326783 to
ae982c9
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Review Feedback Addressed (Cycle 6 — Type Consolidation + Documentation)Type System Cleanup (BREAKING FIX)
Documentation Created
Validation
@coderabbitai review |
|
Triggering an incremental review of the Cycle 6 changes now. ✏️ Learnings added
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/auth/index.ts (1)
1-11:⚠️ Potential issue | 🟡 MinorUpdate the module header comment; it is now stale.
The header still says this module is only Anthropic OAuth + token storage, but this file now exports a full multi-provider auth system (factory/registry, middleware, session, RBAC, errors, context).
Suggested doc update
- * Provides OAuth 2.0 authentication support for Claude Pro/Max subscriptions - * and secure token storage. + * Provides unified authentication support including Anthropic OAuth, + * pluggable auth providers, middleware (RBAC/token extraction), + * session management, request context, and auth utilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/index.ts` around lines 1 - 11, Update the stale top-of-file header in src/lib/auth/index.ts to accurately describe the expanded multi-provider authentication system: mention the factory/registry (authFactory, authRegistry), middleware (authMiddleware), session management (sessionManager), RBAC utilities (rbac), error types (AuthError), and context helpers (AuthContext) in addition to the existing AnthropicOAuth and TokenStore; keep the comment concise, list key components and purpose, and ensure names match the exported symbols in this file.
♻️ Duplicate comments (10)
src/lib/neurolink.ts (1)
520-524:⚠️ Potential issue | 🔴 CriticalKeep auth context request-scoped.
authContextstill lives on theNeuroLinkinstance. Because this file also exports a default singleton, overlapping requests can racesetAuthContext()/clearAuthContext()and leak one caller’s user/session into another. Back this withAsyncLocalStorage, likemetricsTraceContextStorage, instead of an instance field.Suggested direction
+const authContextStorage = + new AsyncLocalStorage<AuthenticatedContext | undefined>(); + export class NeuroLink { private authProvider?: MastraAuthProvider; - private authContext?: AuthenticatedContext; private pendingAuthConfig?: NeuroLinkAuthConfig; private authInitPromise?: Promise<void>; @@ setAuthContext(context: AuthenticatedContext): void { - this.authContext = context; + authContextStorage.enterWith(context); logger.debug("Auth context set", { userId: context.user.id, provider: context.provider, sessionId: context.session?.id, }); } @@ getAuthContext(): AuthenticatedContext | undefined { - return this.authContext; + return authContextStorage.getStore(); } @@ clearAuthContext(): void { - const userId = this.authContext?.user.id; - this.authContext = undefined; + const userId = authContextStorage.getStore()?.user.id; + authContextStorage.enterWith(undefined); if (userId) { logger.debug(`Auth context cleared for user: ${userId}`); } }Also applies to: 11001-11025
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 520 - 524, Replace the instance-scoped authContext field with an AsyncLocalStorage-backed store so auth is request-scoped: introduce an AsyncLocalStorage<AuthenticatedContext | undefined> (e.g., authContextStorage) and remove the private authContext property; update setAuthContext(session) to run a callback via authContextStorage.run(session, ...) or authContextStorage.enterWith(session) as appropriate, update clearAuthContext() to clear via authContextStorage.enterWith(undefined) or by ending the run, and change any getter/uses of authContext (including in setAuthContext, clearAuthContext, and any code referenced around setAuthContext/clearAuthContext and auth access at symbols in this file) to read from authContextStorage.getStore(); apply the same replacement for all occurrences mentioned (including the range around 11001-11025) so the default exported singleton no longer shares authContext between overlapping requests.src/lib/auth/providers/KeycloakProvider.ts (2)
271-276:⚠️ Potential issue | 🟠 MajorAdd timeout to JWKS fetch to prevent hung requests.
The
fetch()call has no timeout, so an unreachable Keycloak host will blockauthenticateToken()indefinitely. TheCognitoProvidercorrectly usesAbortSignal.timeout(5000)at line 271—apply the same pattern here.Proposed fix
try { - const response = await fetch(this.jwksUri); + const response = await fetch(this.jwksUri, { + signal: AbortSignal.timeout(5000), + }); if (!response.ok) {As per coding guidelines: "Use withTimeout for async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/KeycloakProvider.ts` around lines 271 - 276, The JWKS fetch in KeycloakProvider.authenticateToken currently uses fetch(this.jwksUri) with no timeout; update the call to include a timeout by passing a signal (e.g. fetch(this.jwksUri, { signal: AbortSignal.timeout(5000) })) or use the project’s withTimeout helper for async ops, and ensure the try/catch around the fetch handles the abort error consistently with CognitoProvider; reference the fetch call, this.jwksUri, and authenticateToken when making the change.
138-150:⚠️ Potential issue | 🟠 MajorAlways validate
audregardless ofazppresence.The current logic only checks
audwhenazpexists and differs fromclientId. Ifazpis absent—or equalsclientIdwhileauddoes not contain it—the token passes without any audience verification, allowing tokens minted for another client in the same realm.Proposed fix
// Validate audience (azp for access tokens) const azp = claims.azp as string | undefined; - if (azp && azp !== this.keycloakConfig.clientId) { - // Check if clientId is in aud array - const audiences = Array.isArray(claims.aud) ? claims.aud : [claims.aud]; - if (!audiences.includes(this.keycloakConfig.clientId)) { - return { - valid: false, - error: `Invalid authorized party: ${azp}. Expected: ${this.keycloakConfig.clientId}`, - errorCode: "AUTH-001", - }; - } + const audiences = Array.isArray(claims.aud) ? claims.aud : [claims.aud]; + + // Always verify clientId is in audiences + if (!audiences.includes(this.keycloakConfig.clientId)) { + return { + valid: false, + error: `Token audience does not include clientId: ${this.keycloakConfig.clientId}`, + errorCode: "AUTH-001", + }; + } + + // Additionally check azp if present + if (azp && azp !== this.keycloakConfig.clientId) { + return { + valid: false, + error: `Invalid authorized party: ${azp}. Expected: ${this.keycloakConfig.clientId}`, + errorCode: "AUTH-001", + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/KeycloakProvider.ts` around lines 138 - 150, Always validate the token audience regardless of azp: in KeycloakProvider (the block referencing azp, claims.aud and this.keycloakConfig.clientId) extract audiences = Array.isArray(claims.aud) ? claims.aud : [claims.aud] and check that this.keycloakConfig.clientId is included; if not, return the same invalid response (error + errorCode). Preserve the existing azp check (if azp exists and azp !== clientId) as an additional check, but do not gate the aud validation behind the azp conditional so tokens without azp or with azp equal to clientId still require aud to contain the clientId.src/lib/auth/providers/firebase.ts (2)
133-145:⚠️ Potential issue | 🟠 MajorAdd timeout to Firebase API validation call.
The
fetch()call can hang indefinitely if the Google endpoint is slow or unreachable. This is on the token validation hot path.Proposed fix
const proxyFetch = createProxyFetch(); const response = await proxyFetch( `https://identitytoolkit.googleapis.com/v1/accounts:lookup?key=${this.apiKey}`, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ idToken: token }), + signal: AbortSignal.timeout(5000), }, );As per coding guidelines: "Use withTimeout for async operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/firebase.ts` around lines 133 - 145, The validateViaApi method can hang because the proxyFetch call has no timeout; wrap the async fetch call created by createProxyFetch() in the project's withTimeout helper so the POST to https://identitytoolkit.googleapis.com/v1/accounts:lookup?key=${this.apiKey} fails fast on slowness. Update the logic inside validateViaApi to call withTimeout(proxyFetch(...), <appropriateTimeoutMs>) (and handle the timeout error path consistently with TokenValidationResult) so token validation cannot block indefinitely.
381-402:⚠️ Potential issue | 🟡 MinorAdd timeout to health check fetch.
The health check is exposed via CLI and could hang indefinitely without a timeout.
Proposed fix
const proxyFetch = createProxyFetch(); const response = await proxyFetch( "https://www.googleapis.com/service_accounts/v1/jwk/securetoken@system.gserviceaccount.com", + { signal: AbortSignal.timeout(5000) }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/firebase.ts` around lines 381 - 402, In the healthCheck method, add a request timeout when calling createProxyFetch so the health check cannot hang indefinitely: create an AbortController, set a timeout (e.g., 5000 ms) that calls controller.abort(), pass controller.signal into the proxyFetch call (the call site is in async healthCheck and uses createProxyFetch()), and clear the timer after the fetch completes; ensure the catch branch treats an AbortError like a failed fetch and returns the existing error string logic.src/lib/auth/providers/CognitoProvider.ts (1)
249-253:⚠️ Potential issue | 🟠 MajorPass
clockTolerancetojwtVerify()for consistent token validation.The configured
clockToleranceis applied to the manual expiration check at line 167, butjwtVerify()is called without forwarding this tolerance. SincejwtVerify()re-validatesexp/nbf/iatclaims with a default tolerance of 0, a token can pass the configured tolerance window but fail signature verification with zero skew.Proposed fix
// Verify the JWT signature against the public key const publicKey = await importJWK(key, header.alg); - await jwtVerify(token, publicKey); + const clockTolerance = this.config.tokenValidation?.clockTolerance ?? 0; + await jwtVerify(token, publicKey, { clockTolerance }); return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/CognitoProvider.ts` around lines 249 - 253, jwtVerify is called without the configured clock tolerance, causing exp/nbf/iat to be rechecked with zero skew; update the call site where jwtVerify(token, publicKey) is invoked (in CognitoProvider.ts near importJWK and jwtVerify) to pass the configured clockTolerance option (the same value used in the manual expiration check) so jwtVerify(token, publicKey, { clockTolerance }) uses a consistent tolerance for time-based claim validation.src/lib/auth/providers/BaseAuthProvider.ts (1)
122-136:⚠️ Potential issue | 🟠 MajorPrune expired sessions in
getForUser()to enforcemaxSessionsPerUsercorrectly.
getForUser()returns all stored sessions including expired ones. WhencreateSession()checksmaxSessionsPerUser, it counts stale entries and may evict only one, allowing the live session count to exceed the limit.Proposed fix
async getForUser(userId: string): Promise<AuthSession[]> { const userSessionSet = this.userSessions.get(userId); if (!userSessionSet) { return []; } const sessions: AuthSession[] = []; + const expiredIds: string[] = []; + const now = Date.now(); + for (const sessionId of userSessionSet) { const session = this.sessions.get(sessionId); - if (session) { + if (session && session.isValid && (!session.expiresAt || session.expiresAt.getTime() > now)) { sessions.push(session); + } else if (session) { + expiredIds.push(sessionId); } } + + // Prune expired sessions + for (const id of expiredIds) { + this.sessions.delete(id); + userSessionSet.delete(id); + } + if (userSessionSet.size === 0) { + this.userSessions.delete(userId); + } + return sessions; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 122 - 136, getForUser() currently returns stored sessions including expired ones, causing createSession() to overcount stale sessions against maxSessionsPerUser; update getForUser() to filter out expired sessions before returning and to prune them from the internal maps. Specifically, iterate userSessions for the given userId, for each sessionId retrieve the AuthSession from this.sessions, check expiration (e.g., session.expiresAt against Date.now() or session.isExpired()), and if expired remove that sessionId from the userSessions set and delete the entry from this.sessions; only push non-expired sessions into the returned array so createSession() sees the correct live count.src/lib/auth/AuthProviderFactory.ts (2)
472-482:⚠️ Potential issue | 🟡 Minor
getAllProviderInfo()also needs initialization guard.This synchronous method reads from
registrationswithout ensuring the factory is initialized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderFactory.ts` around lines 472 - 482, getAllProviderInfo reads this.registrations synchronously without ensuring the factory is initialized; add an initialization guard at the start of getAllProviderInfo to avoid reading uninitialized state. Either throw a clear error when !this.initialized (e.g. "AuthProviderFactory not initialized") or convert getAllProviderInfo to async and await the existing initialize/ensureInitialized method before accessing this.registrations; reference getAllProviderInfo, this.registrations, and the class's initialize/ensureInitialized/initialized members when making the change.
434-454:⚠️ Potential issue | 🟡 MinorSync discovery helpers may return empty results before initialization.
getAvailableProviders(),getProviderAliases(), andgetProviderMetadata()are synchronous and read fromregistrationsdirectly. If called beforecreate()triggers initialization, they return empty/undefined results. Either convert them to async withensureInitialized()or document this limitation clearly.Option 1: Convert to async (recommended)
- getAvailableProviders(): string[] { + async getAvailableProviders(): Promise<string[]> { + await this.ensureInitialized(); return Array.from(this.registrations.keys()); } - getProviderAliases(type: string): string[] { + async getProviderAliases(type: string): Promise<string[]> { + await this.ensureInitialized(); const registration = this.registrations.get(type.toLowerCase()); return registration?.aliases ?? []; } - getProviderMetadata( + async getProviderMetadata( type: string, - ): AuthFactoryRegistration["metadata"] | undefined { + ): Promise<AuthFactoryRegistration["metadata"] | undefined> { + await this.ensureInitialized(); const registration = this.registrations.get(type.toLowerCase()); return registration?.metadata; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderFactory.ts` around lines 434 - 454, These synchronous discovery helpers (getAvailableProviders, getProviderAliases, getProviderMetadata) read this.registrations and can return empty/undefined if called before initialization; make them call await this.ensureInitialized() and become async (returning Promise<string[]|metadata>) so they always wait for create() initialization (or alternatively document the limitation). Update the method signatures to async, call await this.ensureInitialized() at the start of each method, and keep the same return semantics (resolve to the existing Array.from/aliases/metadata results) so callers get correct values after initialization.src/lib/auth/middleware/AuthMiddleware.ts (1)
250-251:⚠️ Potential issue | 🟠 MajorWrap
authenticateToken()with a timeout to prevent request hangs.The
provider.authenticateToken(token)call can reach external JWKS/userinfo endpoints. A slow or unresponsive IdP will stall the middleware indefinitely. Per past review feedback and coding guidelines, wrap this inwithTimeoutto ensure deterministic failure.Proposed fix
+import { withTimeout } from "../../utils/withTimeout.js"; + // ... in createAuthMiddleware handler ... // Validate token - const validationResult = await provider.authenticateToken(token); + const validationResult = await withTimeout( + provider.authenticateToken(token), + 5000, + "Token validation timed out", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 250 - 251, Wrap the call to provider.authenticateToken(token) in the existing withTimeout helper so an unresponsive IdP cannot hang the middleware: replace the direct await provider.authenticateToken(token) in AuthMiddleware with await withTimeout(provider.authenticateToken(token), <timeoutMs>) (choose the project-standard timeout constant or e.g. 5000), and ensure you propagate/handle the timeout rejection the same way other authentication failures are handled so validationResult remains falsy/error-handled on timeout.
🧹 Nitpick comments (11)
src/lib/auth/providers/auth0.ts (2)
139-142: Avoid non-null assertion onthis.jwks.Static analysis flags the
!assertion at line 139. Althoughinitialize()is called above, a type guard would be clearer.Proposed fix
+ if (!this.jwks) { + throw new ProviderInitializationError( + "JWKS not initialized", + "auth0", + ); + } - const { payload } = await jose.jwtVerify(token, this.jwks!, { + const { payload } = await jose.jwtVerify(token, this.jwks, { issuer: `https://${this.domain}/`, audience: this.audience, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/auth0.ts` around lines 139 - 142, Replace the non-null assertion on this.jwks when calling jose.jwtVerify with an explicit type guard: ensure this.jwks is present (e.g. call or await initialize() / check this.jwks) and if missing throw a clear error before invoking jose.jwtVerify; reference the jwtVerify call and the class' initialize() / this.jwks property so the guard is added immediately prior to the jose.jwtVerify invocation.
186-193: Replace non-null assertions with fallback values.Static analysis flags
this.rolesNamespace!andthis.permissionsNamespace!. These have defaults assigned in the constructor, but the!assertion is unnecessary since you can use optional chaining with fallback.Proposed fix
roles: - (payload[this.rolesNamespace!] as string[]) || + (payload[this.rolesNamespace ?? ""] as string[]) || auth0Payload["https://your-namespace/roles"] || [], permissions: - (payload[this.permissionsNamespace!] as string[]) || + (payload[this.permissionsNamespace ?? ""] as string[]) || auth0Payload["https://your-namespace/permissions"] || [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/auth0.ts` around lines 186 - 193, The code uses non-null assertions this.rolesNamespace! and this.permissionsNamespace! when reading payload keys; replace them with optional chaining and a fallback so static analysis is satisfied — e.g., read payload[this.rolesNamespace ?? default] or use payload?.[this.rolesNamespace] with a fallback to the auth0Payload keys or [] when building the roles and permissions properties in the Auth0 provider (look for the roles/permissions object construction in auth0.ts, inside the method that maps payload to user claims). Ensure you remove the "!" operators and rely on optional chaining/nullish coalescing so roles and permissions default to the provided auth0Payload keys or an empty array.src/lib/auth/providers/clerk.ts (2)
119-119: Avoid non-null assertion onthis.jwks.Static analysis flags the
!assertion. Theinitialize()call above should guarantee it's set, but a guard would be cleaner.Proposed fix
if (!this.jwks) { await this.initialize(); } - ({ payload } = await jose.jwtVerify(token, this.jwks!)); + if (!this.jwks) { + throw new Error("Failed to initialize JWKS"); + } + ({ payload } = await jose.jwtVerify(token, this.jwks));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/clerk.ts` at line 119, The code uses a non-null assertion on this.jwks in the jose.jwtVerify call; replace the assertion with a runtime guard: ensure initialize() has set this.jwks or throw/return a clear error before calling jose.jwtVerify, e.g., check if this.jwks is undefined and handle it, then call ({ payload } = await jose.jwtVerify(token, this.jwks));; reference the initialize() method and the this.jwks property and the jose.jwtVerify call when making the change.
78-83: Use Clerk's backend JWKS endpoint/v1/jwksinstead of the public well-known endpoint.This provider has authentication credentials (secretKey) and makes authenticated calls to Clerk's backend API. Using the backend JWKS endpoint
https://api.clerk.com/v1/jwksis more consistent with the provider's design and purpose, rather than the public OpenID Connect endpoint.Proposed fix
- const jwksUrl = new URL("https://api.clerk.com/.well-known/jwks.json"); + const jwksUrl = new URL("https://api.clerk.com/v1/jwks");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/clerk.ts` around lines 78 - 83, The JWKS URL used in Clerk provider initialization is the public OIDC endpoint; update the initialize() method to use Clerk's backend JWKS endpoint by replacing the URL string passed to new URL(...) from "https://api.clerk.com/.well-known/jwks.json" to "https://api.clerk.com/v1/jwks" so this.jwks = jose.createRemoteJWKSet(jwksUrl) uses the backend key set aligned with the provider's authenticated backend calls.src/lib/auth/providers/jwt.ts (1)
68-69: Consider using inherited session storage instead of custom Maps.
JWTProvidermaintains its ownsessionsanduserSessionsMaps, butBaseAuthProvideralready providesInMemorySessionStorage(see context snippet fromBaseAuthProvider.ts:79-161) with the same functionality. The custom implementation duplicates code and diverges in behavior (e.g.,refreshSessionreturnsnullvs throwingSESSION_NOT_FOUND).Consider removing the custom session Maps and relying on the inherited
sessionStoragefromBaseAuthProvider, or explicitly document why custom behavior is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/jwt.ts` around lines 68 - 69, The JWTProvider currently duplicates session management via the private fields sessions and userSessions; instead, remove these Maps and delegate to the inherited sessionStorage provided by BaseAuthProvider (which includes InMemorySessionStorage) to avoid divergence. Update JWTProvider methods that currently reference sessions/userSessions (e.g., createSession, getSession, refreshSession, revokeSession, getUserSessions) to call the corresponding sessionStorage methods (or adapt to its API), and ensure refreshSession preserves the BaseAuthProvider behavior (throw SESSION_NOT_FOUND if appropriate) or explicitly document the differing behavior if you must keep a custom implementation. Locate usages by the class name JWTProvider and the private fields sessions/userSessions and replace them with sessionStorage operations to consolidate logic and remove duplication.src/lib/auth/providers/oauth2.ts (2)
53-68: Consider using inheritedsessionStorageinstead of duplicating session management.
OAuth2ProviderextendsBaseAuthProviderbut maintains separatesessionsanduserSessionsMaps. This bypassessessionConfigenforcement (e.g.,maxSessionsPerUser,allowMultipleSessions) and creates inconsistency. Delegate tothis.sessionStorageor callsuper.createSession()to inherit the base behavior.Also applies to: 273-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 53 - 68, The OAuth2Provider duplicates session state with private maps sessions and userSessions instead of using the inherited session storage and lifecycle in BaseAuthProvider; remove or stop using those maps and delegate session operations to this.sessionStorage or call the base implementations (e.g., use super.createSession(), super.deleteSession(), super.getSession()/super.listSessions() or this.sessionStorage APIs) in all places that reference sessions or userSessions (including the methods around the 273-392 region), ensuring sessionConfig rules like maxSessionsPerUser and allowMultipleSessions are honored via the base/sessionStorage logic.
141-173: Consider usingjose.jwtVerify()options for issuer/audience validation.The manual post-verification checks work, but
jose.jwtVerify()acceptsissuerandaudienceoptions that enforce these claims atomically during verification, reducing the window for logic errors.Proposed enhancement
try { - const { payload } = await jose.jwtVerify(token, this.jwks!); - - // Validate issuer against the authorization server origin - if (payload.iss) { - const expectedIssuerOrigin = new URL(this.authorizationUrl).origin; - if (!payload.iss.startsWith(expectedIssuerOrigin)) { - return { - valid: false, - error: `Invalid issuer: ${payload.iss}. Expected origin: ${expectedIssuerOrigin}`, - }; - } - } - - // Validate audience against the configured clientId - if (payload.aud) { - const audiences = Array.isArray(payload.aud) - ? payload.aud - : [payload.aud]; - if (!audiences.includes(this.clientId)) { - return { - valid: false, - error: `Invalid audience: ${audiences.join(", ")}. Expected: ${this.clientId}`, - }; - } - } + const expectedIssuerOrigin = new URL(this.authorizationUrl).origin; + const { payload } = await jose.jwtVerify(token, this.jwks!, { + audience: this.clientId, + // Note: jose doesn't support prefix matching for issuer, so keep manual check if needed + }); + + // Validate issuer prefix (jose doesn't support prefix matching) + if (payload.iss && !payload.iss.startsWith(expectedIssuerOrigin)) { + return { + valid: false, + error: `Invalid issuer: ${payload.iss}. Expected origin: ${expectedIssuerOrigin}`, + }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 141 - 173, Replace the manual issuer/audience checks after calling jose.jwtVerify in the token verification logic by passing issuer and audience options into jose.jwtVerify so verification is atomic: compute expectedIssuerOrigin from this.authorizationUrl, call jose.jwtVerify(token, this.jwks!, { issuer: expectedIssuerOrigin, audience: this.clientId }), and then remove the manual payload.iss/payload.aud checks in the try block while keeping the required payload.sub check (the 'sub' existence check in the same function) and returning the same error if missing; ensure errors from jwtVerify are propagated/handled the same way.src/lib/auth/providers/custom.ts (1)
69-70: Session management duplicatesBaseAuthProviderfunctionality.This provider also maintains separate
sessionsanduserSessionsMaps. Consider consolidating with the base class'ssessionStorageto inherit configuration enforcement and reduce duplication across providers.Also applies to: 109-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/custom.ts` around lines 69 - 70, The custom provider duplicates session storage with private maps sessions and userSessions; remove these maps and switch all session-handling logic (methods referencing sessions/userSessions in the Custom* provider around the blocks you saw and lines ~109-152) to use the base class's sessionStorage instead (e.g., call this.sessionStorage.get/set/delete or the base-provided helpers to store, retrieve and enumerate sessions by user). Ensure you replace direct Map operations with sessionStorage API calls, preserve existing behavior (create, lookup by session ID, list/delete sessions for a user), and remove any remaining references to the sessions and userSessions fields so configuration and enforcement come from BaseAuthProvider.src/lib/auth/providers/betterAuth.ts (1)
53-54: Session management duplicatesBaseAuthProviderfunctionality.Like
OAuth2Provider, this class maintains its ownsessionsanduserSessionsMaps instead of using the inheritedsessionStorage. This bypassessessionConfigenforcement and creates maintenance burden. Consider delegating to the base class methods.Also applies to: 199-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/betterAuth.ts` around lines 53 - 54, Remove the duplicate in-class session storage (the private fields "sessions" and "userSessions") and change all code that reads/writes those Maps (including the methods between the 199-277 block) to use the inherited session storage and helper methods on the base class instead (i.e., delegate to this.sessionStorage and the base class session management APIs such as the base get/save/delete session methods so sessionConfig validation/enforcement is honored). Ensure all places that previously mutated this.sessions or this.userSessions now call the base provider's session create/update/remove and any lookup utilities rather than maintaining local Maps.src/lib/auth/AuthProviderRegistry.ts (1)
344-346: Discovery methods calllist()without ensuring initialization.
getAvailableTypes(),getProvidersByFeature(), andgetBuiltInProviders()callthis.list()which reads fromitems. If called before initialization, they return empty results. Consider making them async or documenting the requirement to callensureInitialized()first.Proposed fix
- getAvailableTypes(): AuthProviderType[] { + async getAvailableTypes(): Promise<AuthProviderType[]> { + await this.ensureInitialized(); return Array.from(new Set(this.list().map((p) => p.metadata.type))); } // ... - getProvidersByFeature(feature: string): AuthProviderMetadata[] { + async getProvidersByFeature(feature: string): Promise<AuthProviderMetadata[]> { + await this.ensureInitialized(); return this.list() .filter((p) => p.metadata.features?.includes(feature)) .map((p) => p.metadata); } - getBuiltInProviders(): AuthProviderMetadata[] { + async getBuiltInProviders(): Promise<AuthProviderMetadata[]> { + await this.ensureInitialized(); return this.list() .filter((p) => !p.metadata.requiresExternalDependencies) .map((p) => p.metadata); }Also applies to: 361-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderRegistry.ts` around lines 344 - 346, The discovery methods getAvailableTypes, getProvidersByFeature, and getBuiltInProviders currently call this.list() which reads items before the registry is initialized; change these methods to ensure initialization first by awaiting ensureInitialized() (or make them explicitly async and call await this.ensureInitialized()) before calling this.list(), or add a clear doc comment requiring callers to call ensureInitialized() first—update the signatures of getAvailableTypes/getProvidersByFeature/getBuiltInProviders and any callers accordingly so list() is never invoked on an uninitialized registry.src/lib/auth/providers/BaseAuthProvider.ts (1)
765-786: Consider reusing existing sessions instead of creating new ones on every request.
authenticateRequest()callscreateSession()for every valid token, which can rapidly accumulate sessions and trigger unnecessary evictions. Consider looking up an existing session by token or user first.Sketch
async authenticateRequest( context: AuthRequestContext, ): Promise<AuthenticatedContext | null> { const token = this.extractToken(context); if (!token) { return null; } const result = await this.authenticateToken(token); if (!result.valid || !result.user) { return null; } - const session = await this.createSession(result.user); + // Reuse existing valid session for this user if available + const existingSessions = await this.sessionStorage.getForUser(result.user.id); + let session = existingSessions.find( + (s) => s.isValid && s.expiresAt && s.expiresAt.getTime() > Date.now(), + ); + if (!session) { + session = await this.createSession(result.user); + } else { + await this.sessionStorage.touch(session.id); + } + return { ...context, user: result.user, session,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 765 - 786, authenticateRequest currently calls createSession for every validated token which creates new sessions each request; change authenticateRequest to first look up an existing session (by token or by user id) using the same session store used by createSession (e.g., query by token or user in your session repository), and if found reuse that session (optionally refresh lastActive/authenticatedAt) instead of always calling createSession; only call createSession when no active session exists. Use the helper methods extractToken and authenticateToken to get token and user, then check the session store for an existing session for that token or result.user before creating a new one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/authentication-providers.md`:
- Around line 538-540: The example uses fromHeader with key "prefix" which
doesn't match the documented config shape; update the sample to use
fromHeader.scheme instead of prefix so the shared tokenExtraction config aligns
with the documented API (i.e., change tokenExtraction.fromHeader.prefix ->
tokenExtraction.fromHeader.scheme in the example). Ensure the object key is
"scheme" and the value remains "Bearer" so consumers can copy the snippet
correctly.
In `@src/lib/auth/authContext.ts`:
- Around line 143-147: The requireUser function currently throws a generic Error
on mismatch; replace that with a typed auth error so callers can special-case
auth failures. Modify requireUser (which calls requireAuth and returns
AuthenticatedContext) to throw the project’s authentication error type (e.g.,
AuthError or AuthenticationError) instead of new Error("User mismatch"), and
import that error class from the auth/errors module (or wherever auth errors
live); ensure the thrown error includes a clear message like "User mismatch" so
callers can detect and handle auth failures properly.
In `@src/lib/auth/authProvider.ts`:
- Around line 146-175: authorizeUser currently only checks user.permissions and
wildcard strings and ignores rolePermissions and superAdminRoles, while
authorizeRoles only walks one level of roleHierarchy; update both to mirror
middleware behavior by (1) in authorizeUser, expand the user's effective roles
and permissions: resolve transitive role inheritance using roleHierarchy to get
all inherited roles, include permissions from rolePermissions for every resolved
role, and treat any role in superAdminRoles as an immediate allow; (2) update
authorizeRoles to compute the full transitive closure of roleHierarchy (not just
one hop) when checking membership; ensure you reference and use the existing
symbols authorizeUser, authorizeRoles, rolePermissions, superAdminRoles,
roleHierarchy and AuthUser when implementing these changes so provider-side RBAC
matches middleware RBAC.
In `@src/lib/auth/index.ts`:
- Around line 117-135: Remove the static re-exports of concrete provider classes
(Auth0Provider, BetterAuthProvider, CognitoProvider, ClerkProvider,
CustomAuthProvider, FirebaseAuthProvider/FirebaseProvider, JWTProvider,
KeycloakProvider, OAuth2Provider, SupabaseAuthProvider/SupabaseProvider,
WorkOSProvider) from the top-level barrel and instead surface only the provider
API surface (types/interfaces) plus accessors that use a
ProviderRegistry/factory; update the module to export functions like
registerProvider, getProvider (or a ProviderRegistry instance) and ensure each
concrete provider is loaded via dynamic import inside the registry (use import()
inside ProviderRegistry’s register/get logic) so callers obtain providers
through the registry/factory rather than via direct value re-exports,
eliminating the static module edges and enabling lazy loading.
In `@src/lib/auth/middleware/AuthMiddleware.ts`:
- Around line 406-421: The non-null assertion on queue.pop() in expandRoles
should be removed and replaced with a safe guard: retrieve the popped value into
a variable (e.g., const role = queue.pop()) and check for undefined (if (!role)
continue or break) before using it; keep the rest of the logic (expanded Set,
roleHierarchy lookup, pushing children) unchanged so expandRoles and its use of
roleHierarchy remain intact.
In `@src/lib/auth/providers/betterAuth.ts`:
- Around line 219-226: The non-null assertion this.userSessions.get(user.id)!
should be replaced with a safe guard: retrieve the set into a local variable
(e.g., const sessions = this.userSessions.get(user.id)), if it's undefined
create a new Set and store it via this.userSessions.set(user.id, sessions), then
call sessions.add(sessionId); update the code around the login flow (the block
that currently creates/gets the set and calls add) so no ! is used and the map
is always populated before adding.
- Around line 101-129: In validateJWT, ensure payload.sub exists and is a string
before using it as the AuthUser.id: if payload.sub is missing or not a string
return a TokenValidationResult with valid: false and an appropriate error
message instead of constructing a user with an undefined id; update the
validateJWT logic (function validateJWT, TokenValidationResult creation, and the
constructed user object) to check typeof payload.sub === "string" and handle the
invalid case early so downstream code never receives a user with an undefined
id.
In `@src/lib/auth/providers/custom.ts`:
- Around line 119-126: The code uses a non-null assertion
this.userSessions.get(sessionUser.id)! after checking
this.userSessions.has(sessionUser.id); instead, retrieve the set into a local
variable (e.g., const sessionsForUser = this.userSessions.get(sessionUser.id))
immediately after the has() check, create and set a new Set when absent, then
call sessionsForUser.add(session.id); update the block around this.sessions.set,
this.userSessions.has, this.userSessions.get, and this.emit("auth:login",
sessionUser) to use that local reference instead of the ! assertion to make the
code null-safe.
In `@src/lib/auth/providers/firebase.ts`:
- Around line 216-239: In firebaseUserToAuthUser, guard the JSON.parse of
userData.customAttributes by wrapping it in a try-catch to avoid throwing on
malformed JSON; if parsing fails, fall back to an empty object (or a safe
default) and optionally record the parsing failure (e.g., via logger) so the
rest of the conversion (roles, permissions, metadata) can proceed safely; update
references to customAttributes.roles and customAttributes.permissions to use the
safe fallback.
In `@src/lib/auth/providers/KeycloakProvider.ts`:
- Around line 406-457: The getUser method currently awaits three fetches
(tokenResponse, userResponse, rolesResponse) with no timeouts; wrap each fetch
in an AbortController with a per-request timeout (e.g., configurable constant)
so slow Keycloak responses can't hang the call, pass controller.signal into each
fetch for tokenResponse, userResponse and rolesResponse, start a setTimeout to
call controller.abort() after the timeout, and clear the timer after the fetch
completes (or on catch) to avoid leaks; ensure aborted fetches are handled
(treat as error/timeout) and surface a clear timeout error from getUser.
In `@src/lib/auth/serverBridge.ts`:
- Around line 12-19: The validator returned by createAuthValidatorFromProvider
currently ignores the ctx param; update the returned async function to accept
(token, ctx) and pass ctx through to provider.authenticateToken so any provider
logic using request metadata (headers, IP, org hints) receives the context;
locate createAuthValidatorFromProvider and the provider.authenticateToken call
and ensure the ctx argument is forwarded.
- Around line 23-38: Remove the fabricated-user fallback: when result.user is
absent, do not construct a minimal user from result.payload; instead return null
to preserve the middleware's fail-closed behavior. Concretely, delete the branch
that checks result.payload and creates { id: (result.payload.sub as string) ??
"unknown", ... } and replace it with return null; also don't try to read
result.payload or result.claims here (KeycloakProvider/CognitoProvider use
claims), keeping the bridge consistent with the middleware's "Token valid but no
user identity resolved" rejection.
In `@src/lib/auth/sessionManager.ts`:
- Around line 432-433: The debug logs in sessionManager.ts currently emit raw
sensitive identifiers (sessionId and user.id) via logger.debug calls; update
each logger.debug that references sessionId or user.id (including the instances
around the shown line and the other occurrences flagged) to avoid printing raw
values — either remove those identifiers from the message or replace them with
non-sensitive alternatives such as a masked/truncated session token or a salted
hash/consistent pseudonymized user identifier, or log only safe metadata (e.g.,
"session_created" with userExists=true); ensure the changes are applied to all
logger.debug invocations that reference sessionId or user.id so no raw
identifiers are written to logs.
- Around line 214-228: The set(session: AuthSession) and shouldRefresh() logic
currently dereference session.expiresAt unconditionally; update both so they
first check for the presence of expiresAt and handle non-expiring/tombstoned
sessions safely: in set(), if session.expiresAt is missing, store the session
without a TTL (use client.set(...) or set with no EXPIRE) or use a safe fallback
TTL instead of calling getTime(); in shouldRefresh(), return false when
session.expiresAt is undefined (only compute time-deltas when expiresAt exists).
Apply the same guarded logic to the other occurrence referenced (lines ~463-466)
so no code calls getTime() on an undefined expiresAt.
- Around line 15-35: Remove the locally redefined SessionStorage interface and
instead consume the canonical type from the auth types module: replace the
export/interface block in sessionManager.ts with an import of SessionStorage
from src/lib/types/authTypes.ts (and re-export if this file needs to expose it),
then update any local references (e.g. SessionConfig.customStorage and
BaseAuthProvider usages) to use the imported type so there is a single shared
SessionStorage contract across the auth layer. Ensure the file no longer
declares its own SessionStorage and that the module's exports remain consistent
with the canonical type.
In `@src/lib/core/infrastructure/baseRegistry.ts`:
- Around line 28-35: The register() method currently only reads metadata from
options?.metadata, breaking callers that pass raw metadata as the fourth
positional argument; update register(id, factory, aliases = [], options?) to
accept either the new options object or legacy raw metadata: detect whether the
fourth argument is an object containing a metadata key and use options.metadata,
otherwise treat the fourth argument itself as the metadata object; then set
metadata = detectedMetadata ?? {} before calling this.items.set(id, { factory,
metadata }) so existing callers (e.g., AuthProviderFactory) keep their metadata.
In `@src/lib/neurolink.ts`:
- Around line 2997-3051: The code currently accepts authResult.valid === true
even when authResult.user is missing, allowing caller-controlled options.context
to persist; modify the auth path so that after calling
this.authProvider.authenticateToken (via withTimeout) you require
authResult.user when authResult.valid is true and throw an InvalidTokenError (or
other rejection) if user is absent, and when merging token-derived identity
fields (tokenDerivedFields) build them directly from authResult.user (not from
options.context) so token values always override requestContext; apply the same
change in the other occurrence referenced (the block around
authenticateToken/withTimeout and tokenDerivedFields at the later range).
- Around line 2999-3009: Replace raw Error throws in the auth flow with the
project's typed auth error so callers preserve auth vs non-auth semantics: when
the no-auth-provider branch in ensureAuthProvider detects a missing provider,
throw the typed AuthenticationError (or AuthError) instead of new Error(...);
likewise when calling withTimeout around authProvider.authenticateToken use an
Auth/AuthenticationError instance as the timeout error argument (or catch and
rethrow as that typed error) so the timeout path surfaces the correct auth error
type; apply the same replacement at the other occurrence around authProvider
usage (the block referenced at lines ~5579-5590) and ensure tests/type checks
still accept the typed error class.
---
Outside diff comments:
In `@src/lib/auth/index.ts`:
- Around line 1-11: Update the stale top-of-file header in src/lib/auth/index.ts
to accurately describe the expanded multi-provider authentication system:
mention the factory/registry (authFactory, authRegistry), middleware
(authMiddleware), session management (sessionManager), RBAC utilities (rbac),
error types (AuthError), and context helpers (AuthContext) in addition to the
existing AnthropicOAuth and TokenStore; keep the comment concise, list key
components and purpose, and ensure names match the exported symbols in this
file.
---
Duplicate comments:
In `@src/lib/auth/AuthProviderFactory.ts`:
- Around line 472-482: getAllProviderInfo reads this.registrations synchronously
without ensuring the factory is initialized; add an initialization guard at the
start of getAllProviderInfo to avoid reading uninitialized state. Either throw a
clear error when !this.initialized (e.g. "AuthProviderFactory not initialized")
or convert getAllProviderInfo to async and await the existing
initialize/ensureInitialized method before accessing this.registrations;
reference getAllProviderInfo, this.registrations, and the class's
initialize/ensureInitialized/initialized members when making the change.
- Around line 434-454: These synchronous discovery helpers
(getAvailableProviders, getProviderAliases, getProviderMetadata) read
this.registrations and can return empty/undefined if called before
initialization; make them call await this.ensureInitialized() and become async
(returning Promise<string[]|metadata>) so they always wait for create()
initialization (or alternatively document the limitation). Update the method
signatures to async, call await this.ensureInitialized() at the start of each
method, and keep the same return semantics (resolve to the existing
Array.from/aliases/metadata results) so callers get correct values after
initialization.
In `@src/lib/auth/middleware/AuthMiddleware.ts`:
- Around line 250-251: Wrap the call to provider.authenticateToken(token) in the
existing withTimeout helper so an unresponsive IdP cannot hang the middleware:
replace the direct await provider.authenticateToken(token) in AuthMiddleware
with await withTimeout(provider.authenticateToken(token), <timeoutMs>) (choose
the project-standard timeout constant or e.g. 5000), and ensure you
propagate/handle the timeout rejection the same way other authentication
failures are handled so validationResult remains falsy/error-handled on timeout.
In `@src/lib/auth/providers/BaseAuthProvider.ts`:
- Around line 122-136: getForUser() currently returns stored sessions including
expired ones, causing createSession() to overcount stale sessions against
maxSessionsPerUser; update getForUser() to filter out expired sessions before
returning and to prune them from the internal maps. Specifically, iterate
userSessions for the given userId, for each sessionId retrieve the AuthSession
from this.sessions, check expiration (e.g., session.expiresAt against Date.now()
or session.isExpired()), and if expired remove that sessionId from the
userSessions set and delete the entry from this.sessions; only push non-expired
sessions into the returned array so createSession() sees the correct live count.
In `@src/lib/auth/providers/CognitoProvider.ts`:
- Around line 249-253: jwtVerify is called without the configured clock
tolerance, causing exp/nbf/iat to be rechecked with zero skew; update the call
site where jwtVerify(token, publicKey) is invoked (in CognitoProvider.ts near
importJWK and jwtVerify) to pass the configured clockTolerance option (the same
value used in the manual expiration check) so jwtVerify(token, publicKey, {
clockTolerance }) uses a consistent tolerance for time-based claim validation.
In `@src/lib/auth/providers/firebase.ts`:
- Around line 133-145: The validateViaApi method can hang because the proxyFetch
call has no timeout; wrap the async fetch call created by createProxyFetch() in
the project's withTimeout helper so the POST to
https://identitytoolkit.googleapis.com/v1/accounts:lookup?key=${this.apiKey}
fails fast on slowness. Update the logic inside validateViaApi to call
withTimeout(proxyFetch(...), <appropriateTimeoutMs>) (and handle the timeout
error path consistently with TokenValidationResult) so token validation cannot
block indefinitely.
- Around line 381-402: In the healthCheck method, add a request timeout when
calling createProxyFetch so the health check cannot hang indefinitely: create an
AbortController, set a timeout (e.g., 5000 ms) that calls controller.abort(),
pass controller.signal into the proxyFetch call (the call site is in async
healthCheck and uses createProxyFetch()), and clear the timer after the fetch
completes; ensure the catch branch treats an AbortError like a failed fetch and
returns the existing error string logic.
In `@src/lib/auth/providers/KeycloakProvider.ts`:
- Around line 271-276: The JWKS fetch in KeycloakProvider.authenticateToken
currently uses fetch(this.jwksUri) with no timeout; update the call to include a
timeout by passing a signal (e.g. fetch(this.jwksUri, { signal:
AbortSignal.timeout(5000) })) or use the project’s withTimeout helper for async
ops, and ensure the try/catch around the fetch handles the abort error
consistently with CognitoProvider; reference the fetch call, this.jwksUri, and
authenticateToken when making the change.
- Around line 138-150: Always validate the token audience regardless of azp: in
KeycloakProvider (the block referencing azp, claims.aud and
this.keycloakConfig.clientId) extract audiences = Array.isArray(claims.aud) ?
claims.aud : [claims.aud] and check that this.keycloakConfig.clientId is
included; if not, return the same invalid response (error + errorCode). Preserve
the existing azp check (if azp exists and azp !== clientId) as an additional
check, but do not gate the aud validation behind the azp conditional so tokens
without azp or with azp equal to clientId still require aud to contain the
clientId.
In `@src/lib/neurolink.ts`:
- Around line 520-524: Replace the instance-scoped authContext field with an
AsyncLocalStorage-backed store so auth is request-scoped: introduce an
AsyncLocalStorage<AuthenticatedContext | undefined> (e.g., authContextStorage)
and remove the private authContext property; update setAuthContext(session) to
run a callback via authContextStorage.run(session, ...) or
authContextStorage.enterWith(session) as appropriate, update clearAuthContext()
to clear via authContextStorage.enterWith(undefined) or by ending the run, and
change any getter/uses of authContext (including in setAuthContext,
clearAuthContext, and any code referenced around setAuthContext/clearAuthContext
and auth access at symbols in this file) to read from
authContextStorage.getStore(); apply the same replacement for all occurrences
mentioned (including the range around 11001-11025) so the default exported
singleton no longer shares authContext between overlapping requests.
---
Nitpick comments:
In `@src/lib/auth/AuthProviderRegistry.ts`:
- Around line 344-346: The discovery methods getAvailableTypes,
getProvidersByFeature, and getBuiltInProviders currently call this.list() which
reads items before the registry is initialized; change these methods to ensure
initialization first by awaiting ensureInitialized() (or make them explicitly
async and call await this.ensureInitialized()) before calling this.list(), or
add a clear doc comment requiring callers to call ensureInitialized()
first—update the signatures of
getAvailableTypes/getProvidersByFeature/getBuiltInProviders and any callers
accordingly so list() is never invoked on an uninitialized registry.
In `@src/lib/auth/providers/auth0.ts`:
- Around line 139-142: Replace the non-null assertion on this.jwks when calling
jose.jwtVerify with an explicit type guard: ensure this.jwks is present (e.g.
call or await initialize() / check this.jwks) and if missing throw a clear error
before invoking jose.jwtVerify; reference the jwtVerify call and the class'
initialize() / this.jwks property so the guard is added immediately prior to the
jose.jwtVerify invocation.
- Around line 186-193: The code uses non-null assertions this.rolesNamespace!
and this.permissionsNamespace! when reading payload keys; replace them with
optional chaining and a fallback so static analysis is satisfied — e.g., read
payload[this.rolesNamespace ?? default] or use payload?.[this.rolesNamespace]
with a fallback to the auth0Payload keys or [] when building the roles and
permissions properties in the Auth0 provider (look for the roles/permissions
object construction in auth0.ts, inside the method that maps payload to user
claims). Ensure you remove the "!" operators and rely on optional
chaining/nullish coalescing so roles and permissions default to the provided
auth0Payload keys or an empty array.
In `@src/lib/auth/providers/BaseAuthProvider.ts`:
- Around line 765-786: authenticateRequest currently calls createSession for
every validated token which creates new sessions each request; change
authenticateRequest to first look up an existing session (by token or by user
id) using the same session store used by createSession (e.g., query by token or
user in your session repository), and if found reuse that session (optionally
refresh lastActive/authenticatedAt) instead of always calling createSession;
only call createSession when no active session exists. Use the helper methods
extractToken and authenticateToken to get token and user, then check the session
store for an existing session for that token or result.user before creating a
new one.
In `@src/lib/auth/providers/betterAuth.ts`:
- Around line 53-54: Remove the duplicate in-class session storage (the private
fields "sessions" and "userSessions") and change all code that reads/writes
those Maps (including the methods between the 199-277 block) to use the
inherited session storage and helper methods on the base class instead (i.e.,
delegate to this.sessionStorage and the base class session management APIs such
as the base get/save/delete session methods so sessionConfig
validation/enforcement is honored). Ensure all places that previously mutated
this.sessions or this.userSessions now call the base provider's session
create/update/remove and any lookup utilities rather than maintaining local
Maps.
In `@src/lib/auth/providers/clerk.ts`:
- Line 119: The code uses a non-null assertion on this.jwks in the
jose.jwtVerify call; replace the assertion with a runtime guard: ensure
initialize() has set this.jwks or throw/return a clear error before calling
jose.jwtVerify, e.g., check if this.jwks is undefined and handle it, then call
({ payload } = await jose.jwtVerify(token, this.jwks));; reference the
initialize() method and the this.jwks property and the jose.jwtVerify call when
making the change.
- Around line 78-83: The JWKS URL used in Clerk provider initialization is the
public OIDC endpoint; update the initialize() method to use Clerk's backend JWKS
endpoint by replacing the URL string passed to new URL(...) from
"https://api.clerk.com/.well-known/jwks.json" to "https://api.clerk.com/v1/jwks"
so this.jwks = jose.createRemoteJWKSet(jwksUrl) uses the backend key set aligned
with the provider's authenticated backend calls.
In `@src/lib/auth/providers/custom.ts`:
- Around line 69-70: The custom provider duplicates session storage with private
maps sessions and userSessions; remove these maps and switch all
session-handling logic (methods referencing sessions/userSessions in the Custom*
provider around the blocks you saw and lines ~109-152) to use the base class's
sessionStorage instead (e.g., call this.sessionStorage.get/set/delete or the
base-provided helpers to store, retrieve and enumerate sessions by user). Ensure
you replace direct Map operations with sessionStorage API calls, preserve
existing behavior (create, lookup by session ID, list/delete sessions for a
user), and remove any remaining references to the sessions and userSessions
fields so configuration and enforcement come from BaseAuthProvider.
In `@src/lib/auth/providers/jwt.ts`:
- Around line 68-69: The JWTProvider currently duplicates session management via
the private fields sessions and userSessions; instead, remove these Maps and
delegate to the inherited sessionStorage provided by BaseAuthProvider (which
includes InMemorySessionStorage) to avoid divergence. Update JWTProvider methods
that currently reference sessions/userSessions (e.g., createSession, getSession,
refreshSession, revokeSession, getUserSessions) to call the corresponding
sessionStorage methods (or adapt to its API), and ensure refreshSession
preserves the BaseAuthProvider behavior (throw SESSION_NOT_FOUND if appropriate)
or explicitly document the differing behavior if you must keep a custom
implementation. Locate usages by the class name JWTProvider and the private
fields sessions/userSessions and replace them with sessionStorage operations to
consolidate logic and remove duplication.
In `@src/lib/auth/providers/oauth2.ts`:
- Around line 53-68: The OAuth2Provider duplicates session state with private
maps sessions and userSessions instead of using the inherited session storage
and lifecycle in BaseAuthProvider; remove or stop using those maps and delegate
session operations to this.sessionStorage or call the base implementations
(e.g., use super.createSession(), super.deleteSession(),
super.getSession()/super.listSessions() or this.sessionStorage APIs) in all
places that reference sessions or userSessions (including the methods around the
273-392 region), ensuring sessionConfig rules like maxSessionsPerUser and
allowMultipleSessions are honored via the base/sessionStorage logic.
- Around line 141-173: Replace the manual issuer/audience checks after calling
jose.jwtVerify in the token verification logic by passing issuer and audience
options into jose.jwtVerify so verification is atomic: compute
expectedIssuerOrigin from this.authorizationUrl, call jose.jwtVerify(token,
this.jwks!, { issuer: expectedIssuerOrigin, audience: this.clientId }), and then
remove the manual payload.iss/payload.aud checks in the try block while keeping
the required payload.sub check (the 'sub' existence check in the same function)
and returning the same error if missing; ensure errors from jwtVerify are
propagated/handled the same way.
| async authorizeUser( | ||
| user: AuthUser, | ||
| permission: string, | ||
| ): Promise<AuthorizationResult> { | ||
| // Check if user has the permission directly | ||
| if (user.permissions.includes(permission)) { | ||
| return { authorized: true }; | ||
| } | ||
|
|
||
| // Check if user has wildcard permission | ||
| if (user.permissions.includes("*")) { | ||
| return { authorized: true }; | ||
| } | ||
|
|
||
| // Check permission hierarchy (e.g., "tools:*" includes "tools:execute") | ||
| const permissionParts = permission.split(":"); | ||
| for (let i = permissionParts.length - 1; i > 0; i--) { | ||
| const wildcardPermission = [...permissionParts.slice(0, i), "*"].join( | ||
| ":", | ||
| ); | ||
| if (user.permissions.includes(wildcardPermission)) { | ||
| return { authorized: true }; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| authorized: false, | ||
| reason: `User lacks permission: ${permission}`, | ||
| missingPermissions: [permission], | ||
| }; |
There was a problem hiding this comment.
Keep provider-side RBAC in sync with middleware RBAC.
authorizeUser() still ignores rolePermissions and superAdminRoles, and authorizeRoles() only walks one hop of roleHierarchy. With transitive inheritance and role-to-permission expansion now part of the auth config, these helpers can deny a user that the middleware would authorize.
Also applies to: 188-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/authProvider.ts` around lines 146 - 175, authorizeUser currently
only checks user.permissions and wildcard strings and ignores rolePermissions
and superAdminRoles, while authorizeRoles only walks one level of roleHierarchy;
update both to mirror middleware behavior by (1) in authorizeUser, expand the
user's effective roles and permissions: resolve transitive role inheritance
using roleHierarchy to get all inherited roles, include permissions from
rolePermissions for every resolved role, and treat any role in superAdminRoles
as an immediate allow; (2) update authorizeRoles to compute the full transitive
closure of roleHierarchy (not just one hop) when checking membership; ensure you
reference and use the existing symbols authorizeUser, authorizeRoles,
rolePermissions, superAdminRoles, roleHierarchy and AuthUser when implementing
these changes so provider-side RBAC matches middleware RBAC.
| register( | ||
| id: string, | ||
| factory: () => Promise<TItem>, | ||
| metadata: TMetadata, | ||
| aliases: string[] = [], | ||
| options?: { metadata: TMetadata }, | ||
| ): void { | ||
| const metadata = options?.metadata ?? ({} as TMetadata); | ||
| this.items.set(id, { factory, metadata }); |
There was a problem hiding this comment.
Keep register() compatible with existing metadata call sites.
register() now only reads metadata from options?.metadata, but src/lib/auth/AuthProviderFactory.ts still passes raw metadata as the fourth positional argument. That path now registers auth providers with {} metadata, so provider discovery/health listings lose their metadata until every caller is migrated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/core/infrastructure/baseRegistry.ts` around lines 28 - 35, The
register() method currently only reads metadata from options?.metadata, breaking
callers that pass raw metadata as the fourth positional argument; update
register(id, factory, aliases = [], options?) to accept either the new options
object or legacy raw metadata: detect whether the fourth argument is an object
containing a metadata key and use options.metadata, otherwise treat the fourth
argument itself as the metadata object; then set metadata = detectedMetadata ??
{} before calling this.items.set(id, { factory, metadata }) so existing callers
(e.g., AuthProviderFactory) keep their metadata.
12f863a to
0bfef96
Compare
Review Feedback Addressed (Cycle 7 — CodeRabbit Round 6)CRITICAL Fixes (3)
MAJOR Fixes (15)
MINOR/NITPICK Fixes (14)
Validation
@coderabbitai review |
|
Triggering an incremental review of the Cycle 7 changes now. 🧠 Learnings used✅ Actions performedReview triggered.
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (5)
src/lib/auth/middleware/rateLimitByUser.ts (1)
162-177:⚠️ Potential issue | 🔴 CriticalRedis client initialization has a race condition.
Two concurrent requests calling
getClient()whenthis.clientisnullwill both see!this.clientas true and both proceed to create separate Redis clients. This leaks connections and causes inconsistent state.Additionally, Node Redis v5 requires registering an error listener before
connect()to prevent uncaught exceptions from crashing the process.🔧 Suggested fix using init promise
export class RedisRateLimitStorage implements RateLimitStorage { private redisUrl: string; private prefix: string; private ttlSeconds: number; private client: RedisClient | null = null; + private initPromise: Promise<RedisClient> | null = null; // ... private async getClient(): Promise<RedisClient> { - if (!this.client) { - // Dynamic import to avoid loading Redis unless needed - const { createClient } = await import("redis"); - const client = createClient({ url: this.redisUrl }); - await client.connect(); - this.client = client as unknown as RedisClient; + if (this.client) { + return this.client; } - return this.client; + if (!this.initPromise) { + this.initPromise = (async () => { + const { createClient } = await import("redis"); + const client = createClient({ url: this.redisUrl }); + client.on("error", (err) => logger.warn("Redis rate-limit client error:", err)); + await client.connect(); + this.client = client as unknown as RedisClient; + return this.client; + })(); + } + return this.initPromise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 162 - 177, getClient() has a race where concurrent calls create multiple Redis clients and it connects after creation without an error listener; make initialization atomic by adding an init promise (e.g., this.initPromise) that is set when first entering getClient(), have other callers await it, and only create a single client inside that promise; also register an 'error' event handler on the client before calling client.connect() so Node Redis v5 errors are caught; update getClient() and constructor to use the init promise and ensure this.client is assigned once the promise resolves.src/lib/auth/authContext.ts (1)
374-375:⚠️ Potential issue | 🟠 MajorThe process-wide
globalAuthContextsingleton still poses cross-request leakage risk.The fallback to
globalAuthContextingetAuthContext(),getCurrentUser(), etc. (lines 68, 87, 98, 108-110) means that ifrunWithAuthContext()is never called beforegenerate()/stream()executes, any code reading auth context will receive the last authenticated user's identity. This enables cross-request state leakage in concurrent server scenarios.Consider either:
- Removing the
globalAuthContextexport and fallback entirely- Restricting
AuthContextHolderto test-only usage with clear documentation- Adding runtime warnings when fallback is used in non-test environments
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authContext.ts` around lines 374 - 375, The exported process-wide singleton globalAuthContext in AuthContextHolder is unsafe because getAuthContext(), getCurrentUser(), and other readers (used by generate() / stream()) fall back to it and can leak the previous request's identity; remove the public fallback by either deleting the globalAuthContext export and ensuring getAuthContext() throws or returns undefined unless runWithAuthContext() explicitly set a context, or make globalAuthContext test-only (unexported) and gate its use behind an environment check that emits a runtime warning if used outside tests; update callers to handle the absence of a global fallback (raise explicit errors or require runWithAuthContext()) and adjust AuthContextHolder, getAuthContext(), getCurrentUser(), runWithAuthContext(), generate(), and stream() to reflect the chosen approach.src/lib/neurolink.ts (1)
3027-3046:⚠️ Potential issue | 🔴 CriticalRequire
authResult.user.idbefore treating the token as authenticated.These guards only reject
valid: truewhenuseris missing entirely. A provider can still return a partialuserobject with noid, and both paths then mergeuserId: undefinedintooptions.contextwhile the request continues as token-authenticated. Fail closed on!authResult.user?.idbefore any context merge.Also applies to: 5629-5648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3027 - 3046, The current auth flow treats a token as authenticated if authResult.valid is true even when authResult.user exists but lacks an id, allowing userId: undefined to be merged into options.context; update the guard to fail closed by checking authResult.user?.id (i.e., throw InvalidTokenError with the same provider type when authResult.user is falsy or authResult.user.id is missing) before the context merge in the block that builds options.context (refer to authResult, InvalidTokenError, and the options.context merge), and apply the same fix at the other analogous location handling authResult to ensure no partial user (missing id) is accepted.src/lib/auth/authProvider.ts (2)
130-133:⚠️ Potential issue | 🟠 MajorAwait custom token extractors before handing the value to
authenticateToken().A Promise returned from
strategy.custom()is truthy, so this flow skips the null check and passes the Promise object intoauthenticateToken()instead of a token string.Patch sketch
- extractToken(context: AuthRequestContext): string | null { + async extractToken(context: AuthRequestContext): Promise<string | null> { @@ - return strategy.custom(context); + return await strategy.custom(context); @@ - const token = this.extractToken(context); + const token = await this.extractToken(context);Also applies to: 302-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authProvider.ts` around lines 130 - 133, The custom extractor flow is returning a Promise (from strategy.custom) directly instead of awaiting it, causing authenticateToken to receive a Promise rather than a token string; update the code in authProvider.ts to await the result of strategy.custom(context) (e.g., const token = await strategy.custom(context)), check token for null/undefined, and then return or pass that token into authenticateToken(); apply the same change to the other custom-extractor block referenced around the authenticateToken usage so both places await strategy.custom before null-checking and forwarding the token.
328-334:⚠️ Potential issue | 🟠 MajorReuse a session only when it matches the current token/session.
Picking the first live session for
validation.user.idcan bind this request to another device's session id. That breaks logout, refresh, and audit flows as soon as one user holds multiple active tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authProvider.ts` around lines 328 - 334, The code currently picks the first live session for validation.user.id which can incorrectly bind this request to another device's session; modify the selection so validSession is only reused when it corresponds to the current incoming session/token (e.g., compare session.id or session.token to the session id/token sent with the request in context or validation), otherwise call createSession(validation.user, context). Change the find predicate in the block that calls getUserSessions to require both isValid/expiry and equality with the currentSessionId/currentToken from context/validation, referencing getUserSessions, validSession, createSession, and validation.user.id to locate and update the logic. Ensure behavior falls back to createSession when no matching session for this device/token exists.
🧹 Nitpick comments (6)
src/lib/auth/middleware/rateLimitByUser.ts (2)
162-166: Redis TTL should default to at leastwindowMsto prevent early bucket expiry.
RedisRateLimitStoragedefaultsttlSecondsto 3600 (1 hour), butRateLimitConfig.windowMscould be longer. If a user configures a 24-hour rate limit window without explicitly settingredis.ttlSeconds, buckets expire after 1 hour, allowing users to bypass limits by waiting for storage expiry.🔧 Suggested fix
- constructor(config: { url: string; prefix?: string; ttlSeconds?: number }) { + constructor(config: { url: string; prefix?: string; ttlSeconds?: number; windowMs?: number }) { this.redisUrl = config.url; this.prefix = config.prefix || "neurolink:ratelimit:"; - this.ttlSeconds = config.ttlSeconds || 3600; // 1 hour default TTL + // TTL must be at least as long as the rate limit window + const windowSeconds = config.windowMs ? Math.ceil(config.windowMs / 1000) : 3600; + this.ttlSeconds = config.ttlSeconds || Math.max(windowSeconds, 3600); }And update
createRateLimitStorageto passwindowMsthrough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 162 - 166, The Redis TTL default is too short: update RedisRateLimitStorage's constructor to default ttlSeconds to the RateLimitConfig.windowMs (in seconds) when not provided so buckets don't expire before the rate limit window; ensure the constructor (symbols: RedisRateLimitStorage, ttlSeconds, redisUrl, prefix) uses the provided ttlSeconds or falls back to Math.ceil(windowMs/1000). Also update createRateLimitStorage to accept and pass the RateLimitConfig.windowMs (symbol: createRateLimitStorage, windowMs) into the storage config so the storage layer can compute the correct default TTL.
566-569: Consider maskinguserIdin log output.For consistency with the session manager's approach to avoiding PII in logs, consider masking the user ID here.
async resetUser(userId: string): Promise<void> { await this.storage.deleteBucket(userId); - logger.debug(`Rate limit reset for user: ${userId}`); + logger.debug(`Rate limit reset for user: ${userId.slice(0, 4)}***`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 566 - 569, The debug log in resetUser currently prints the raw userId; change it to log a masked identifier instead to avoid PII. Update the resetUser method (which calls this.storage.deleteBucket and logger.debug) to compute a masked version of userId (e.g., keep first 4 and last 2 chars and replace the middle with asterisks, or call the existing session-manager mask helper if available) and log that masked value instead of the plain userId while leaving the deleteBucket call unchanged.src/lib/auth/authContext.ts (1)
144-150: Consider usingInsufficientPermissionsErrorfor user mismatch.Semantically, a user mismatch (authenticated as user A, requesting resource for user B) is an authorization failure rather than authentication failure. Using
InsufficientPermissionsErrorwould:
- Enable callers to distinguish "invalid credentials" (401) from "wrong user" (403)
- Align with REST conventions where 403 indicates "authenticated but not authorized"
However, if the intent is to avoid leaking information about which user IDs exist,
AuthenticationFailedErrorreturning 401 is also reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authContext.ts` around lines 144 - 150, The requireUser function currently throws AuthenticationFailedError on a user-id mismatch; change it to throw InsufficientPermissionsError to reflect an authorization (403) failure instead of an authentication (401) failure. Update the throw in requireUser (and add the necessary import for InsufficientPermissionsError) so when context.user.id !== userId the code throws InsufficientPermissionsError("User mismatch") while leaving requireAuth and the rest of requireUser behavior unchanged.src/lib/auth/providers/oauth2.ts (2)
139-141: Refactor to avoid non-null assertion onthis.jwks.Static analysis flags the non-null assertion. Since
initialize()is called just above,this.jwkswill be set, but the assertion can be avoided by checking after init.Proposed fix
if (!this.jwks) { await this.initialize(); } + if (!this.jwks) { + return { + valid: false, + error: "Failed to initialize JWKS", + }; + } try { - const { payload } = await jose.jwtVerify(token, this.jwks!); + const { payload } = await jose.jwtVerify(token, this.jwks);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 139 - 141, The code uses a non-null assertion on this.jwks when calling jose.jwtVerify in the method that follows initialize(); remove the assertion by ensuring this.jwks is present (or initialize() succeeded) before calling jwtVerify: call await this.initialize() if needed, check that this.jwks is truthy and throw or return a clear error if not, then pass this.jwks to jose.jwtVerify without using the ! operator (reference the initialize() method and the jwtVerify call that currently uses this.jwks! to locate the change).
213-215: Refactor to avoid non-null assertion onthis.userInfoUrl.The assertion is safe here since the method is only called when
this.userInfoUrlis truthy (line 196), but for clarity you can either pass the URL as a parameter or use a local check.Proposed fix
- private async validateViaUserInfo( - token: string, - ): Promise<TokenValidationResult> { + private async validateViaUserInfo( + token: string, + userInfoUrl: string, + ): Promise<TokenValidationResult> { try { const proxyFetch = createProxyFetch(); - const response = await proxyFetch(this.userInfoUrl!, { + const response = await proxyFetch(userInfoUrl, {And update the call site:
- return this.validateViaUserInfo(token); + return this.validateViaUserInfo(token, this.userInfoUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 213 - 215, The code currently uses a non-null assertion on this.userInfoUrl when calling createProxyFetch()/proxyFetch; change the method that calls proxyFetch (the OAuth2 user info fetch function) to either accept a userInfoUrl: string parameter and use that instead of this.userInfoUrl, or capture it into a local const (e.g., const url = this.userInfoUrl) and guard with an explicit check that throws or returns early if undefined, then pass that checked url to proxyFetch; update all call sites to supply the URL when choosing the parameter approach and remove the "!" non-null assertion from the proxyFetch call.src/lib/index.ts (1)
1183-1299: Single-source the auth barrel from./auth/index.js.This block re-lists the auth public surface that
src/lib/auth/index.tsalready curates. Keeping both barrels in sync by hand is brittle; please re-export from./auth/index.jshere and only alias the small set of top-level name collisions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/index.ts` around lines 1183 - 1299, Replace the long manual re-exports for auth with a single canonical re-export from ./auth/index.js and only keep explicit aliases for the handful of top-level name collisions; remove the duplicated export list (the block exporting AuthProviderFactory, AuthProviderRegistry, BaseAuthProvider, createAuthMiddleware/createRBACMiddleware/createProtectedMiddleware/createExpressAuthMiddleware/createRequestContext/extractToken, UserRateLimiter/MemoryRateLimitStorage/RedisRateLimitStorage, SessionManager/MemorySessionStorage/RedisSessionStorage, AuthContextHolder/getCurrentUser..., AuthError/InvalidTokenError..., and the various type exports) and instead add a single export-from for the auth barrel and explicit alias exports for the conflicting names you rely on (examples from the diff to preserve: export createAuthMiddleware as createAuthProviderMiddleware, export type MiddlewareHandler as AuthMiddlewareHandler, export type RateLimitConfig as AuthRateLimitConfig, export type SessionManagerStorage as SessionStorageInterface), ensuring the public surface remains identical while delegating maintenance to ./auth/index.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/auth/authProvider.ts`:
- Around line 16-17: There are two divergent BaseAuthProvider implementations
causing inconsistent behavior; consolidate them into the single canonical
BaseAuthProvider (the one under src/lib/auth/providers/BaseAuthProvider.ts) by
removing the duplicate file and updating all providers (auth0.ts, betterAuth.ts,
firebase.ts, jwt.ts, workos.ts, etc.) to import that canonical class; modify
BaseAuthProvider to stop hardcoding InMemorySessionStorage (add a
constructor/sessionStorage injection or factory parameter), unify the
token-extraction configuration surface and RBAC hooks, and ensure a single,
well-defined authenticateRequest() implementation (or abstract it consistently)
so all providers rely on the same core interface. Ensure exports/public path
reference the canonical BaseAuthProvider and migrate any provider-specific
overrides to extend the unified class.
In `@src/lib/auth/AuthProviderRegistry.ts`:
- Around line 363-379: getProvidersByFeature and getBuiltInProviders can return
duplicate provider metadata because BaseRegistry.register stores aliases as
separate entries and BaseRegistry.list returns them all; update each method to
deduplicate results by provider identity (e.g., provider.id or another unique
key) before mapping to metadata. Concretely: after calling this.list() and
applying the filter, reduce or convert to a Map/Set keyed by the provider's
unique identifier to remove alias duplicates, then return Array.from(...) of the
unique entries' metadata; reference getProvidersByFeature, getBuiltInProviders,
BaseRegistry.register, and BaseRegistry.list when locating the logic to change.
- Around line 90-100: The public AuthProviderRegistry.get() is a foot-gun
because the registered factory always throws a raw Error; replace this unsafe
behavior by making get() surface a typed AuthRegistryError (via your
ErrorFactory) that clearly directs callers to use createProvider() or
getMetadata(), or alternatively remove/expose get() from the public API; locate
the get method in AuthProviderRegistry and either (A) wrap its thrown error in a
new AuthRegistryError (use ErrorFactory to construct it and include actionable
text pointing to createProvider()/getMetadata()), or (B) change its visibility
to private/internal and update callers to use createProvider() and
getMetadata(); ensure any async paths use withTimeout if applicable and maintain
existing usage sites.
In `@src/lib/auth/middleware/AuthMiddleware.ts`:
- Around line 305-312: The middleware is populating AuthenticatedContext.claims
from (validationResult as any).claims which is empty because new providers put
decoded token data under payload; update the construction of
AuthenticatedContext in AuthMiddleware to use the canonical field by reading
payload (e.g. (validationResult as any).payload) or fall back to claims if
present so decoded token data is preserved; specifically change the claims
assignment in the AuthenticatedContext creation (where validationResult, user,
token are set) to pull from payload first, ensuring downstream hooks see the raw
decoded claims.
In `@src/lib/auth/providers/BaseAuthProvider.ts`:
- Around line 783-804: The authenticateRequest implementation currently mints a
new session unconditionally by calling createSession() on every authenticated
request; change it so createSession is only called when there is no existing
session associated with the validated token/user. Specifically, in
authenticateRequest (and the related authenticateToken/extractToken flow) check
for an existing session id/session object returned by authenticateToken or
present in the token payload (e.g., result.session or result.sessionId or
context.session) and only call createSession(result.user) if no existing session
is found; update authenticateToken implementations to return session/sessionId
when available so authenticateRequest can reuse that instead of always creating
a fresh session.
- Around line 122-154: The getForUser method currently only filters expired
sessions and therefore returns and counts revoked sessions; update getForUser to
also treat revoked sessions (session.isValid === false) as expired: during the
loop over userSessionSet, if session.isValid is false push its id onto
expiredIds (or a revokedIds list), skip adding it to sessions, and then in the
cleanup loop remove those ids from this.sessions and userSessionSet (same as
expired cleanup) so revoked tombstones no longer appear in getForUser() or count
toward maxSessionsPerUser; reference getForUser, revokeSession, isValid,
userSessions, and sessions when making the change.
In `@src/lib/auth/providers/clerk.ts`:
- Around line 262-266: The non-null assertion on this.userSessions.get(user.id)!
is unsafe; replace it by storing the retrieved Set in a local variable after
ensuring it exists (e.g., let sessions = this.userSessions.get(user.id); if
(!sessions) { sessions = new Set(); this.userSessions.set(user.id, sessions); }
) and then call sessions.add(sessionId); reference this.userSessions, user.id
and sessionId when making the change to remove the "!" operator and use the
local sessions variable.
In `@src/lib/auth/providers/CognitoProvider.ts`:
- Around line 166-168: The manual expiration check uses clockTolerance default 0
while the jwtVerify call defaults to 30, causing inconsistent validation; fix
this by computing a single clockTolerance variable once from
this.config.tokenValidation?.clockTolerance (use 30 as the default to match
jwtVerify) and use that same variable in the isTokenExpired call and in the
jwtVerify invocation (references: isTokenExpired, jwtVerify,
this.config.tokenValidation?.clockTolerance inside the CognitoProvider class).
- Around line 381-386: The CognitoProvider.getUser implementation currently
throws an AuthProviderError which violates the MastraAuthProvider optional
contract and BaseAuthProvider behavior; replace the throw in async
getUser(_userId: string): Promise<AuthUser | null> with a simple return null so
callers receive null when the method is not implemented (preserve the same
signature and no additional side effects).
In `@src/lib/auth/providers/firebase.ts`:
- Around line 56-57: The Firebase provider currently keeps provider-local Maps
(sessions and userSessions) and custom session lifecycle logic which bypasses
the shared session backend; remove these in-memory Maps and update all
session-related methods in firebase.ts (e.g., createSession, getSession,
updateSession, deleteSession and any session cleanup/prune logic found in the
provider between the previous Map declarations and the later lifecycle code) to
delegate to the common session storage API (use config.session.customStorage or
the shared session service used by other providers) for get/set/delete/iterate
operations, ensure you enforce shared session limits and use the common
revoke/tombstone semantics when revoking or expiring sessions, and preserve
AuthSession shape when reading/writing so behavior is identical to the other
providers.
In `@src/lib/auth/providers/jwt.ts`:
- Around line 153-170: The code currently maps missing JWT subject to an empty
string (id: payload.sub ?? ""), which allows tokens without a subject to be
treated as a valid user; update the JWT handling to reject tokens that lack a
non-empty payload.sub by validating payload.sub before constructing the AuthUser
(e.g., in the function that verifies/parses the token or immediately before
creating the user object), and if payload.sub is missing or empty return an
error/null or throw an authentication error instead of proceeding; replace the
permissive fallback in the user mapping (remove the ?? "" behavior) and ensure
any callers of the function handle the authentication failure.
In `@src/lib/auth/providers/supabase.ts`:
- Around line 229-233: The non-null assertion on this.userSessions.get(user.id)!
should be removed; instead retrieve the set into a local variable after ensuring
it exists (e.g., let sessions = this.userSessions.get(user.id); if (!sessions) {
sessions = new Set(); this.userSessions.set(user.id, sessions); } ) and then
call sessions.add(sessionId). Update the logic around this.userSessions,
user.id, and sessionId to use the local reference so no "!" is needed.
- Around line 171-175: The roles extraction is using a nonstandard claim
`payload.user_role`; update the assignment that sets `roles` to use the standard
Supabase JWT `payload.role` (e.g., `payload.role as string`) for token-type
roles and fall back to application roles from `appMetadata?.roles` for
app-specific roles, so change the ternary in the `roles:` field to check
`payload.role` not `payload.user_role` while keeping the existing fallback to
`(appMetadata?.roles as string[]) || []`; ensure `emailVerified` and
`permissions` logic remain unchanged.
In `@src/lib/auth/providers/workos.ts`:
- Around line 394-402: The catch blocks around the WorkOS user fetch currently
swallow all non-ProviderAPIError exceptions by returning null; change them so
only an explicit "not found" response maps to null: if the caught error is a
ProviderAPIError (or equivalent WorkOS API error) and indicates a 404 /
user-not-found status or the API returned an empty result, return null,
otherwise rethrow the error after logging so transport/retry/outage failures
surface to callers; update both catch sites used when fetching WorkOS users (the
catch shown and the similar one later) to implement this conditional rethrow
logic while keeping the existing error log.
In `@src/lib/auth/sessionManager.ts`:
- Around line 210-218: The code calls new Date(session.expiresAt)
unconditionally which yields an Invalid Date when expiresAt is missing; update
the session parsing logic (where session.createdAt and session.expiresAt are
handled in SessionManager) to only parse expiresAt if session.expiresAt !=
null/undefined and results in a valid date (check !isNaN(parsed.getTime())); if
expiresAt is absent or invalid leave session.expiresAt undefined/null (or skip
the expiration check) so the if (new Date() > session.expiresAt) branch (which
may call this.delete(sessionId)) only runs when expiresAt is a valid Date.
In `@src/lib/neurolink.ts`:
- Around line 10980-11003: The lazy auth init currently leaves authInitPromise
set and can race with manual overrides in setAuthProvider; change
ensureAuthProvider and setAuthProvider to track and invalidate the init promise
on settle and ignore stale completions: when starting a lazy init create a local
initId/marker and assign authInitPromise, then in the async completion check
whether the current authInitPromise or a stored marker still matches (or whether
this.authProvider was set meanwhile) before assigning this.authProvider; also
add a finally block to always clear/authInitPromise = undefined on settle so
failed inits don't poison future calls; apply the same pattern to any other
lazy-init paths referenced around ensureAuthProvider/authInitPromise to make
late resolutions no-ops.
- Around line 11047-11075: The methods setAuthContext, getAuthContext, and
clearAuthContext are using require("./auth/authContext.js") which fails under
ESM; replace each require(...) call with a dynamic import(...) and destructure
the exported members from the resolved module (e.g., const { globalAuthContext }
= await import("./auth/authContext.js") in setAuthContext/clearAuthContext, and
const { getAuthContext: getCtx } = await import("./auth/authContext.js") in
getAuthContext), ensuring the functions remain synchronous in signature but
await the import inside the method or make them async if needed to await the
dynamic import; update usages of globalAuthContext and getCtx accordingly.
---
Duplicate comments:
In `@src/lib/auth/authContext.ts`:
- Around line 374-375: The exported process-wide singleton globalAuthContext in
AuthContextHolder is unsafe because getAuthContext(), getCurrentUser(), and
other readers (used by generate() / stream()) fall back to it and can leak the
previous request's identity; remove the public fallback by either deleting the
globalAuthContext export and ensuring getAuthContext() throws or returns
undefined unless runWithAuthContext() explicitly set a context, or make
globalAuthContext test-only (unexported) and gate its use behind an environment
check that emits a runtime warning if used outside tests; update callers to
handle the absence of a global fallback (raise explicit errors or require
runWithAuthContext()) and adjust AuthContextHolder, getAuthContext(),
getCurrentUser(), runWithAuthContext(), generate(), and stream() to reflect the
chosen approach.
In `@src/lib/auth/authProvider.ts`:
- Around line 130-133: The custom extractor flow is returning a Promise (from
strategy.custom) directly instead of awaiting it, causing authenticateToken to
receive a Promise rather than a token string; update the code in authProvider.ts
to await the result of strategy.custom(context) (e.g., const token = await
strategy.custom(context)), check token for null/undefined, and then return or
pass that token into authenticateToken(); apply the same change to the other
custom-extractor block referenced around the authenticateToken usage so both
places await strategy.custom before null-checking and forwarding the token.
- Around line 328-334: The code currently picks the first live session for
validation.user.id which can incorrectly bind this request to another device's
session; modify the selection so validSession is only reused when it corresponds
to the current incoming session/token (e.g., compare session.id or session.token
to the session id/token sent with the request in context or validation),
otherwise call createSession(validation.user, context). Change the find
predicate in the block that calls getUserSessions to require both isValid/expiry
and equality with the currentSessionId/currentToken from context/validation,
referencing getUserSessions, validSession, createSession, and validation.user.id
to locate and update the logic. Ensure behavior falls back to createSession when
no matching session for this device/token exists.
In `@src/lib/auth/middleware/rateLimitByUser.ts`:
- Around line 162-177: getClient() has a race where concurrent calls create
multiple Redis clients and it connects after creation without an error listener;
make initialization atomic by adding an init promise (e.g., this.initPromise)
that is set when first entering getClient(), have other callers await it, and
only create a single client inside that promise; also register an 'error' event
handler on the client before calling client.connect() so Node Redis v5 errors
are caught; update getClient() and constructor to use the init promise and
ensure this.client is assigned once the promise resolves.
In `@src/lib/neurolink.ts`:
- Around line 3027-3046: The current auth flow treats a token as authenticated
if authResult.valid is true even when authResult.user exists but lacks an id,
allowing userId: undefined to be merged into options.context; update the guard
to fail closed by checking authResult.user?.id (i.e., throw InvalidTokenError
with the same provider type when authResult.user is falsy or authResult.user.id
is missing) before the context merge in the block that builds options.context
(refer to authResult, InvalidTokenError, and the options.context merge), and
apply the same fix at the other analogous location handling authResult to ensure
no partial user (missing id) is accepted.
---
Nitpick comments:
In `@src/lib/auth/authContext.ts`:
- Around line 144-150: The requireUser function currently throws
AuthenticationFailedError on a user-id mismatch; change it to throw
InsufficientPermissionsError to reflect an authorization (403) failure instead
of an authentication (401) failure. Update the throw in requireUser (and add the
necessary import for InsufficientPermissionsError) so when context.user.id !==
userId the code throws InsufficientPermissionsError("User mismatch") while
leaving requireAuth and the rest of requireUser behavior unchanged.
In `@src/lib/auth/middleware/rateLimitByUser.ts`:
- Around line 162-166: The Redis TTL default is too short: update
RedisRateLimitStorage's constructor to default ttlSeconds to the
RateLimitConfig.windowMs (in seconds) when not provided so buckets don't expire
before the rate limit window; ensure the constructor (symbols:
RedisRateLimitStorage, ttlSeconds, redisUrl, prefix) uses the provided
ttlSeconds or falls back to Math.ceil(windowMs/1000). Also update
createRateLimitStorage to accept and pass the RateLimitConfig.windowMs (symbol:
createRateLimitStorage, windowMs) into the storage config so the storage layer
can compute the correct default TTL.
- Around line 566-569: The debug log in resetUser currently prints the raw
userId; change it to log a masked identifier instead to avoid PII. Update the
resetUser method (which calls this.storage.deleteBucket and logger.debug) to
compute a masked version of userId (e.g., keep first 4 and last 2 chars and
replace the middle with asterisks, or call the existing session-manager mask
helper if available) and log that masked value instead of the plain userId while
leaving the deleteBucket call unchanged.
In `@src/lib/auth/providers/oauth2.ts`:
- Around line 139-141: The code uses a non-null assertion on this.jwks when
calling jose.jwtVerify in the method that follows initialize(); remove the
assertion by ensuring this.jwks is present (or initialize() succeeded) before
calling jwtVerify: call await this.initialize() if needed, check that this.jwks
is truthy and throw or return a clear error if not, then pass this.jwks to
jose.jwtVerify without using the ! operator (reference the initialize() method
and the jwtVerify call that currently uses this.jwks! to locate the change).
- Around line 213-215: The code currently uses a non-null assertion on
this.userInfoUrl when calling createProxyFetch()/proxyFetch; change the method
that calls proxyFetch (the OAuth2 user info fetch function) to either accept a
userInfoUrl: string parameter and use that instead of this.userInfoUrl, or
capture it into a local const (e.g., const url = this.userInfoUrl) and guard
with an explicit check that throws or returns early if undefined, then pass that
checked url to proxyFetch; update all call sites to supply the URL when choosing
the parameter approach and remove the "!" non-null assertion from the proxyFetch
call.
In `@src/lib/index.ts`:
- Around line 1183-1299: Replace the long manual re-exports for auth with a
single canonical re-export from ./auth/index.js and only keep explicit aliases
for the handful of top-level name collisions; remove the duplicated export list
(the block exporting AuthProviderFactory, AuthProviderRegistry,
BaseAuthProvider,
createAuthMiddleware/createRBACMiddleware/createProtectedMiddleware/createExpressAuthMiddleware/createRequestContext/extractToken,
UserRateLimiter/MemoryRateLimitStorage/RedisRateLimitStorage,
SessionManager/MemorySessionStorage/RedisSessionStorage,
AuthContextHolder/getCurrentUser..., AuthError/InvalidTokenError..., and the
various type exports) and instead add a single export-from for the auth barrel
and explicit alias exports for the conflicting names you rely on (examples from
the diff to preserve: export createAuthMiddleware as
createAuthProviderMiddleware, export type MiddlewareHandler as
AuthMiddlewareHandler, export type RateLimitConfig as AuthRateLimitConfig,
export type SessionManagerStorage as SessionStorageInterface), ensuring the
public surface remains identical while delegating maintenance to
./auth/index.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b47a217-c653-4690-b66a-95dba3ad3fa0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
docs-site/sidebars.tsdocs/advanced/auth-architecture.mddocs/features/authentication-providers.mddocs/features/index.mddocs/superpowers/plans/2026-03-16-remove-redundant-tests.mdpackage.jsonsrc/cli/commands/authProviders.tssrc/cli/factories/authCommandFactory.tssrc/cli/parser.tssrc/lib/auth/AuthProviderFactory.tssrc/lib/auth/AuthProviderRegistry.tssrc/lib/auth/RequestContext.tssrc/lib/auth/authContext.tssrc/lib/auth/authErrors.tssrc/lib/auth/authProvider.tssrc/lib/auth/index.tssrc/lib/auth/middleware/AuthMiddleware.tssrc/lib/auth/middleware/rateLimitByUser.tssrc/lib/auth/providers/BaseAuthProvider.tssrc/lib/auth/providers/CognitoProvider.tssrc/lib/auth/providers/KeycloakProvider.tssrc/lib/auth/providers/auth0.tssrc/lib/auth/providers/betterAuth.tssrc/lib/auth/providers/clerk.tssrc/lib/auth/providers/custom.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/oauth2.tssrc/lib/auth/providers/supabase.tssrc/lib/auth/providers/workos.tssrc/lib/auth/serverBridge.tssrc/lib/auth/sessionManager.tssrc/lib/core/infrastructure/baseRegistry.tssrc/lib/index.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/rag/ChunkerRegistry.tssrc/lib/rag/metadata/MetadataExtractorRegistry.tssrc/lib/rag/reranker/RerankerRegistry.tssrc/lib/server/routes/agentRoutes.tssrc/lib/types/authTypes.tssrc/lib/types/configTypes.tssrc/lib/types/generateTypes.tssrc/lib/types/index.tssrc/lib/types/streamTypes.tstest/continuous-test-suite-auth.tstest/fixtures/auth/provider-config.jsontest/fixtures/auth/session-data.jsontest/fixtures/auth/test-credentials.json
💤 Files with no reviewable changes (1)
- docs/superpowers/plans/2026-03-16-remove-redundant-tests.md
✅ Files skipped from review due to trivial changes (11)
- docs-site/sidebars.ts
- src/cli/parser.ts
- src/lib/rag/metadata/MetadataExtractorRegistry.ts
- test/fixtures/auth/session-data.json
- test/fixtures/auth/provider-config.json
- src/lib/auth/RequestContext.ts
- test/fixtures/auth/test-credentials.json
- src/cli/factories/authCommandFactory.ts
- src/lib/auth/AuthProviderFactory.ts
- src/lib/types/authTypes.ts
- src/lib/auth/authErrors.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- package.json
- src/lib/rag/reranker/RerankerRegistry.ts
- src/lib/rag/ChunkerRegistry.ts
- src/lib/mcp/toolRegistry.ts
- src/lib/core/infrastructure/baseRegistry.ts
- src/lib/types/streamTypes.ts
- docs/features/index.md
- test/continuous-test-suite-auth.ts
- src/lib/types/configTypes.ts
- src/cli/commands/authProviders.ts
| * @example | ||
| * ```typescript | ||
| * // Get registry instance | ||
| * const registry = AuthProviderRegistry.getInstance(); | ||
| * | ||
| * // List available providers | ||
| * const providers = registry.list(); | ||
| * | ||
| * // Get provider info | ||
| * const auth0Info = await registry.get('auth0'); | ||
| * |
There was a problem hiding this comment.
Don't leave get() as a public foot-gun.
The example on Line 99 advertises registry.get("auth0"), but the factory registered here always throws, and it throws a raw Error. So AuthProviderRegistry exposes a public method that fails by design and bypasses the typed auth error surface. Please either override get() with an AuthRegistryError that points callers to createProvider() / getMetadata(), or stop exposing the value-registry API here.
Suggested fix
+ override async get(_id: string): Promise<MastraAuthProvider | undefined> {
+ throw AuthRegistryError.create(
+ "REGISTRATION_FAILED",
+ "AuthProviderRegistry.get() is unsupported. Use createProvider(type, config) or getMetadata(type).",
+ );
+ }
+
private registerProviderMetadata(
type: string,
metadata: AuthProviderMetadata,
): void {
// Register with base registry — aliases as 3rd arg, metadata in options object as 4th arg
this.register(
type,
async () => {
- throw new Error(
- `Direct provider creation not supported. Use AuthProviderFactory.create() with config.`,
- );
+ throw AuthRegistryError.create(
+ "REGISTRATION_FAILED",
+ "Direct provider creation is unsupported. Use createProvider(type, config).",
+ );
},
metadata.aliases,
{ metadata },
);
}As per coding guidelines, "Use ErrorFactory for typed errors, withTimeout for async operations, and graceful degradation with provider fallback for error handling."
Also applies to: 295-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/AuthProviderRegistry.ts` around lines 90 - 100, The public
AuthProviderRegistry.get() is a foot-gun because the registered factory always
throws a raw Error; replace this unsafe behavior by making get() surface a typed
AuthRegistryError (via your ErrorFactory) that clearly directs callers to use
createProvider() or getMetadata(), or alternatively remove/expose get() from the
public API; locate the get method in AuthProviderRegistry and either (A) wrap
its thrown error in a new AuthRegistryError (use ErrorFactory to construct it
and include actionable text pointing to createProvider()/getMetadata()), or (B)
change its visibility to private/internal and update callers to use
createProvider() and getMetadata(); ensure any async paths use withTimeout if
applicable and maintain existing usage sites.
| // Parse dates | ||
| session.createdAt = new Date(session.createdAt); | ||
| session.expiresAt = new Date(session.expiresAt); | ||
|
|
||
| // Check expiration | ||
| if (new Date() > session.expiresAt) { | ||
| await this.delete(sessionId); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Guard expiresAt parsing for sessions that may lack expiration.
If session.expiresAt is missing from the stored JSON (e.g., tombstoned or externally created sessions), new Date(undefined) produces an Invalid Date, which may cause unexpected comparison behavior on line 215.
🛡️ Suggested defensive guard
// Parse dates
session.createdAt = new Date(session.createdAt);
- session.expiresAt = new Date(session.expiresAt);
+ if (session.expiresAt) {
+ session.expiresAt = new Date(session.expiresAt);
+ }
// Check expiration
- if (new Date() > session.expiresAt) {
+ if (session.expiresAt && new Date() > session.expiresAt) {
await this.delete(sessionId);
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/sessionManager.ts` around lines 210 - 218, The code calls new
Date(session.expiresAt) unconditionally which yields an Invalid Date when
expiresAt is missing; update the session parsing logic (where session.createdAt
and session.expiresAt are handled in SessionManager) to only parse expiresAt if
session.expiresAt != null/undefined and results in a valid date (check
!isNaN(parsed.getTime())); if expiresAt is absent or invalid leave
session.expiresAt undefined/null (or skip the expiration check) so the if (new
Date() > session.expiresAt) branch (which may call this.delete(sessionId)) only
runs when expiresAt is a valid Date.
0bfef96 to
9757568
Compare
Review Feedback Addressed (Cycle 8 — CodeRabbit Round 7 + Vercel Fix)Vercel Fix
CRITICAL Fixes (2)
MAJOR Fixes (13)
MINOR/NITPICK Fixes (15)
Validation
@coderabbitai review |
|
Triggering an incremental review of the Cycle 8 changes now. 🧠 Learnings used✅ Actions performedReview triggered.
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (13)
src/lib/auth/authProvider.ts (1)
48-67:⚠️ Potential issue | 🟠 MajorConsolidate this into the canonical
BaseAuthProvider.This second base class is still live:
src/lib/auth/providers/supabase.ts,src/lib/auth/providers/custom.ts, andsrc/lib/auth/providers/jwt.tsextend it, whilesrc/lib/auth/providers/CognitoProvider.tsextendssrc/lib/auth/providers/BaseAuthProvider.ts. The two bases already diverge on extraction, RBAC, session binding, and storage behavior, so provider behavior now depends on which import path a file chose. Based on learnings: All 12+ providers must support consistent core interface across all providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authProvider.ts` around lines 48 - 67, There are two divergent base classes causing inconsistent provider behavior; consolidate into the canonical BaseAuthProvider in BaseAuthProvider (class BaseAuthProvider in src/lib/auth/authProvider.ts) by merging any logic from the alternate base (src/lib/auth/providers/BaseAuthProvider.ts) into this class: ensure tokenExtraction defaults, RBAC hooks, session binding, and sessionStorage (InMemorySessionStorage) behavior are unified under the single BaseAuthProvider, update MastraAuthProvider/AuthProviderConfig types as needed, and change all providers (supabase.ts, custom.ts, jwt.ts, CognitoProvider.ts) to extend this consolidated BaseAuthProvider and remove or redirect the duplicate base to prevent split implementations.src/lib/auth/authContext.ts (1)
66-68:⚠️ Potential issue | 🟠 MajorKeep the process-wide fallback out of request handling.
These helpers fall back to
globalAuthContext, but the providedsrc/lib/auth/middleware/AuthMiddleware.tssnippets only return or attach the authenticated context and never callrunWithAuthContext(). Any server path that uses this singleton as the escape hatch therefore exposes one mutable auth slot to all overlapping requests.Also applies to: 85-109, 373-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/authContext.ts` around lines 66 - 68, The code currently falls back to a process-wide globalAuthContext in getAuthContext (and the related helpers around requireAuthContext and any getters at the other noted spots), which exposes a shared mutable auth slot; remove the fallback so getAuthContext returns only authContextStorage.getStore() and update requireAuthContext and any other accessor functions to rely exclusively on the request-scoped authContextStorage (or throw/return undefined) and stop reading from globalAuthContext.get(); keep globalAuthContext only as an explicit, opt-in API (e.g., via a runWithAuthContext helper) and add/update callers to explicitly use runWithAuthContext when process-wide behavior is actually intended.src/lib/auth/providers/BaseAuthProvider.ts (1)
800-807:⚠️ Potential issue | 🟠 MajorMatch session reuse to the current token/session, not just
user.id.Reusing the first active session for the user can attach another device or token's
session.idto this request. That breaks refresh, logout, and audit flows as soon as a user has more than one valid session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/BaseAuthProvider.ts` around lines 800 - 807, The code currently picks any valid session for the user (existingSessions.find(...)) which can incorrectly reuse another device/token's session; change the selection to prefer an existing session that matches the incoming authentication result's session identifier (e.g., result.sessionId or result.session?.id) and only fall back to creating a new session if no matching valid session exists; update the logic in BaseAuthProvider where existingSessions, validSession, sessionStorage.getForUser, and createSession are used so you check session.id === result.sessionId (or the equivalent field on result) in addition to s.isValid and expiry before reusing it.src/lib/auth/providers/supabase.ts (1)
172-175:⚠️ Potential issue | 🟠 MajorMerge the standard Supabase
roleclaim withapp_metadata.roles.Supabase tokens commonly include
role: "authenticated", so this ternary drops every application role fromapp_metadata.roleson the normal path. RBAC then sees onlyauthenticated.♻️ Suggested fix
- roles: payload.role - ? [payload.role as string] - : (appMetadata?.roles as string[]) || [], + roles: Array.from( + new Set([ + ...(typeof payload.role === "string" ? [payload.role] : []), + ...((appMetadata?.roles as string[] | undefined) ?? []), + ]), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/supabase.ts` around lines 172 - 175, The current logic for building the roles array uses payload.role alone and discards appMetadata.roles when payload.role exists; update the construction in the roles field so it merges payload.role (if present) with any appMetadata.roles instead of choosing one or the other: collect payload.role into an array (e.g., [payload.role]) and concat or merge it with (appMetadata?.roles as string[]) || [], avoiding duplicates so RBAC receives both the standard Supabase role and any application roles defined in appMetadata.src/lib/server/routes/agentRoutes.ts (2)
70-80:⚠️ Potential issue | 🟠 MajorCaller-supplied IDs still flow into trusted context when unauthenticated.
The conditional logic improves the authenticated path, but when
ctx.useris absent (unauthenticated request),request.userIdandrequest.sessionIdstill populatecontext.userIdandcontext.sessionId. If downstream code (memory, rate limiting, auditing) treats these fields as trusted identity, unauthenticated callers can impersonate arbitrary users.Consider either:
- Only populating
userId/sessionIdfrom authenticated sources, leaving themundefinedfor anonymous requests- Separating untrusted caller-provided IDs into distinct fields (e.g.,
rawRequestUserId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/agentRoutes.ts` around lines 70 - 80, The context construction currently allows unauthenticated callers to inject request.userId and request.sessionId into trusted context fields; update the object built in agentRoutes.ts so that context.sessionId and context.userId are only set when ctx.user is present (e.g., sessionId: ctx.user ? ctx.session?.id : undefined, userId: ctx.user ? ctx.user.id : undefined) or, if you want to preserve caller values, move them into separate untrusted fields like rawRequestUserId and rawRequestSessionId instead of populating the trusted context properties; ensure references to ctx.user, request.userId, and request.sessionId are adjusted accordingly.
137-147:⚠️ Potential issue | 🟠 MajorSame impersonation concern applies to the stream endpoint.
This block mirrors the execute handler's context construction. The same risk of caller-supplied IDs being used as trusted identity applies here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/agentRoutes.ts` around lines 137 - 147, The stream endpoint's context construction currently risks using caller-supplied IDs (request.sessionId/request.userId) even when an authenticated ctx.user exists; update the context building in the stream handler so that if ctx.user is present you always use ctx.session?.id and ctx.user.id (and ctx.user.email / ctx.user.roles) and never fall back to request.* values, otherwise fall back to request.sessionId/request.userId when no ctx.user exists; adjust the assignments for sessionId, userId, userEmail, and userRoles accordingly in the context object (look for context, sessionId, userId, userEmail, userRoles, ctx.user, ctx.session, request.sessionId, request.userId).src/lib/neurolink.ts (1)
10992-11018:⚠️ Potential issue | 🟠 MajorPrevent stale lazy auth init from overwriting a newer provider.
Clearing
authInitPromiseinsetAuthProvider()does not cancel an olderensureAuthProvider()already in flight. If that older init resolves later, it still writesthis.authProviderand can clobber a manual override. Gate the final assignment behind an init generation/token and ignore stale completions.Also applies to: 11041-11055
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 10992 - 11018, setAuthProvider currently clears authInitPromise but doesn't prevent an in-flight ensureAuthProvider completion from overwriting a newer provider; introduce a generation/token (e.g., this.authInitGeneration: number) incremented whenever setAuthProvider is called or a new init starts, capture the current generation in the local scope inside ensureAuthProvider and in any async init paths, and before assigning this.authProvider (and before clearing this.authInitPromise) compare the captured generation to this.authInitGeneration and bail out if they differ to ignore stale completions; apply the same generation-check pattern to the other init path referenced around lines 11041-11055 so any late-resolving async create() or factory results do not clobber a later manual override.src/lib/auth/middleware/AuthMiddleware.ts (2)
103-133:⚠️ Potential issue | 🟠 MajorHonor
fromHeader.schemeinstead of always falling back to Bearer.
extractToken()only readsprefix, sofromHeader: { scheme: "Token" }still behaves like Bearer auth. Custom header schemes are effectively broken unless callers keep using the old field name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 103 - 133, The header extraction is only using headerConfig.prefix and ignores a provided fromHeader.scheme, breaking custom schemes; update the logic in the extractToken (AuthMiddleware) flow to prefer headerConfig.scheme (if defined) and fall back to headerConfig.prefix and then "Bearer" only if neither is set. Concretely, when computing the scheme used to validate the header, replace the current const prefix = headerConfig.prefix ?? "Bearer" with something like const scheme = headerConfig.scheme ?? headerConfig.prefix ?? "Bearer", use scheme for the empty-scheme check (return raw value when scheme is falsy) and for building schemeWithSpace (scheme + " "), and perform the case-insensitive comparison against schemeWithSpace instead of prefixWithSpace so custom fromHeader.scheme values are honored.
409-440:⚠️ Potential issue | 🟠 MajorExact permission checks still ignore wildcard grants.
Set.has()here will not match*,tools:*, or other hierarchical permissions, socreateProtectedMiddleware()can deny users that the shared auth authorization logic would allow. Reuse the common permission matcher here instead of exact string comparison.Also applies to: 499-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 409 - 440, The effectivePermissions set is built from user.permissions and rolePermissions but later permission checks (e.g., in createProtectedMiddleware) use exact Set.has() so wildcard grants like '*' or 'tools:*' are ignored; update the authorization checks to reuse the shared permission matcher (the project's common permission matching function) instead of Set.has(), and where expandRoles/rolePermissions/effectivePermissions are used, ensure you compare requested permissions by iterating effectivePermissions and calling the common matcher (e.g., permissionMatches(requested, granted)) to allow wildcard/hierarchical matches; apply the same replacement to the other occurrence referenced near createProtectedMiddleware so all checks use the common matcher.src/cli/commands/authProviders.ts (1)
128-133:⚠️ Potential issue | 🟡 MinorJWT config guidance is now inaccurate.
The CLI metadata says JWT requires
secret, butbuildProviderConfig()acceptspublicKeyalone.auth providersand the missing-config message will mislead validJWT_PUBLIC_KEY-only setups.Suggested fix
jwt: { name: "JWT", description: "Generic JWT token validation with configurable secret/keys", - requiredConfig: ["secret"], + requiredConfig: ["secret or publicKey"], website: "https://jwt.io/", },Also applies to: 229-243, 537-549
src/lib/auth/providers/workos.ts (1)
453-462:⚠️ Potential issue | 🟠 MajorRe-throw lookup failures from
getUserByEmail().This catch still turns transport/retry errors into
null, so a WorkOS outage looks the same as “user not found”. Only an empty result should returnnull; everything else should propagate.Suggested fix
} catch (error) { logger.error( "Failed to fetch WorkOS user by email:", error instanceof Error ? error.message : String(error), ); - if (error instanceof ProviderAPIError) { - throw error; - } - return null; + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/workos.ts` around lines 453 - 462, The catch in getUserByEmail currently logs errors but swallows non-ProviderAPIError exceptions by returning null, which hides transport/retry failures; update the catch in getUserByEmail to log and then re-throw the caught error (preserving ProviderAPIError behavior) instead of returning null — only allow an explicit empty API result path to return null, but do not convert exceptions into a null "not found" result.src/lib/auth/providers/oauth2.ts (1)
146-171:⚠️ Potential issue | 🔴 CriticalRequire exact
issandaudclaims on the JWKS path.This branch only checks
isswhen it exists and only compares its origin. A signature-valid token from another tenant/path on the same host—or one with no audience claim at all—can still pass here. This likely needs an explicit expected issuer inOAuth2Configinstead of deriving only fromauthorizationUrl.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/oauth2.ts` around lines 146 - 171, The token validation currently only optionally checks payload.iss and compares origins and treats payload.aud as optional; update the logic in the try block that handles jose.jwtVerify to require exact issuer and audience matches: add or use an explicit expected issuer configuration on the OAuth2Config (e.g., expectedIssuer) and fail validation if payload.iss is missing or not exactly equal to that expectedIssuer (fall back to a strict match against new URL(this.authorizationUrl).origin only if expectedIssuer is not set), and fail if payload.aud is missing or does not include this.clientId; update the error messages to reflect missing vs. mismatched claims and reference the symbols jwks, authorizationUrl, clientId, and the OAuth2Config expectedIssuer when locating code to change.src/lib/auth/AuthProviderRegistry.ts (1)
300-309:⚠️ Potential issue | 🟡 MinorUse typed error in the placeholder factory.
The factory function registered here throws a raw
Error, bypassing the typed error surface. Per coding guidelines, useAuthRegistryErrorfor consistency.Proposed fix
this.register( type, async () => { - throw new Error( - `Direct provider creation not supported. Use AuthProviderFactory.create() with config.`, + throw AuthRegistryError.create( + "REGISTRATION_FAILED", + "Direct provider creation is unsupported. Use createProvider(type, config).", ); }, metadata.aliases, { metadata }, );As per coding guidelines: "Use ErrorFactory for typed errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/AuthProviderRegistry.ts` around lines 300 - 309, Replace the raw Error thrown in the placeholder factory registered via AuthProviderRegistry.register with a typed AuthRegistryError created through the ErrorFactory; locate the async factory passed to register (the one that currently does throw new Error(...)) and change it to throw ErrorFactory.create(AuthRegistryError, "Direct provider creation not supported. Use AuthProviderFactory.create() with config.") or the project's equivalent ErrorFactory usage so the registered factory throws the typed AuthRegistryError instead of a plain Error.
🧹 Nitpick comments (1)
src/lib/auth/providers/clerk.ts (1)
56-57: Clerk uses provider-local Maps instead of shared session storage.Unlike
FirebaseAuthProviderwhich delegates tothis.sessionStorage,ClerkProvidermaintains its own in-memorysessionsanduserSessionsMaps. This bypasses any shared session backend configured viaconfig.session.customStorage, breaking consistency across providers.Consider delegating session operations to
this.sessionStorage(inherited fromBaseAuthProvider) as Firebase does, or document this as an intentional provider-specific behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/clerk.ts` around lines 56 - 57, ClerkProvider currently uses its own private maps (sessions and userSessions) instead of the shared sessionStorage on BaseAuthProvider; replace usages of the private maps with calls to the inherited this.sessionStorage API (the same pattern used by FirebaseAuthProvider) so session create/read/update/delete and user->session mappings use the configured customStorage backend, or if provider-specific in-memory behavior is intentional, add a clear comment in the ClerkProvider class documenting that sessions and userSessions are intentionally local and will not use this.sessionStorage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/index.md`:
- Line 28: The table entry for "Authentication Providers" uses an absolute path;
update the link target for the "Authentication Providers" table cell to use a
relative path consistent with the other entries (e.g., change
/docs/features/authentication-providers to a relative link like
./authentication-providers or authentication-providers.md) so it points to the
existing docs/features/authentication-providers.md file and matches the
surrounding link format.
In `@src/lib/auth/middleware/AuthMiddleware.ts`:
- Around line 284-303: AuthMiddleware currently treats a token as authenticated
if validationResult.user exists even when user.id is empty; update the guard in
the validation flow (the block checking validationResult.user) to also require a
non-empty user.id (or equivalent unique identifier) and treat missing/empty id
as authentication failure: construct the same AuthError (statusCode 401, message
like "Token valid but no user identity resolved", code "AUTH-001"), call
config.onError with an Error augmented with code when present (as currently
done), and return { proceed: false, error } so requests with user objects
lacking an id are rejected; locate the check around validationResult.user in
AuthMiddleware to make this change.
In `@src/lib/auth/middleware/rateLimitByUser.ts`:
- Around line 194-206: In createClient(), register an 'error' event listener on
the newly created Redis client before calling client.connect() so Node Redis v5
won't emit unhandled errors; specifically, in the createClient() method add
client.on('error', (err) => { /* log via your logger or reject/initPromise
cleanup */ }) immediately after const client = createClient({ url: this.redisUrl
}) and before await client.connect(), and ensure you still clear initPromise and
throw the same error if connection fails (preserve existing initPromise cleanup
behavior).
In `@src/lib/auth/providers/clerk.ts`:
- Around line 176-187: The Clerk provider calls to the external API must be
bounded by a 5s timeout: update validateSessionToken(), getUser(),
getUserByEmail(), and healthCheck() to wrap their async fetch calls (the ones
using createProxyFetch()) with the shared withTimeout helper set to 5000 ms (or
5_000) so the promise rejects after 5s; locate the proxyFetch invocation in each
method (e.g., the POST to "https://api.clerk.com/v1/sessions/verify" in
validateSessionToken) and replace the direct await call with await
withTimeout(<existing proxyFetch call>, 5000) so all external Clerk requests
obey the timeout.
In `@src/lib/auth/providers/firebase.ts`:
- Around line 94-103: The non-null assertion this.jwks! used when calling
jose.jwtVerify(token, this.jwks!, ...) should be removed; instead ensure a
definite JWKS reference by capturing the result of initialization into a local
variable (e.g., call await this.initialize() when this.jwks is falsy, then set
const jwks = this.jwks) and pass that local jwks to jose.jwtVerify along with
token and the issuer/audience (use projectId as before) so static analysis no
longer needs the unsafe assertion and the code still guarantees a valid key set
for jwtVerify.
In `@src/lib/auth/providers/KeycloakProvider.ts`:
- Around line 193-214: The parser currently accepts tokens with no subject
because extractKeycloakUser() falls back to an empty id; update the
KeycloakProvider token validation to reject JWTs that lack a non-empty
claims.sub before returning valid: true (i.e., in the block that builds the
result object using extractKeycloakUser and validClaims), and return valid:
false (with no user) when claims.sub is missing or an empty string; apply the
same check to the other similar token-return path that also uses
extractKeycloakUser so tokens without a stable subject are fail-closed.
In `@src/lib/auth/providers/supabase.ts`:
- Around line 56-57: The Supabase provider is forking session state into private
maps (sessions and userSessions) which bypasses the shared sessionStorage and
breaks allowMultipleSessions, maxSessionsPerUser, revocation and any
custom/Redis-backed storage; remove these private maps and stop reading/writing
to sessions and userSessions, and instead use the shared sessionStorage API used
by the rest of the auth layer (the same session create/get/delete/update calls)
so all session lifecycle logic (allowMultipleSessions, maxSessionsPerUser,
revoke semantics) is preserved; update all methods that reference sessions or
userSessions (including the blocks you noted around 209–239 and 244–326) to
delegate to the central sessionStorage implementation and ensure revocation and
session limits are enforced via that shared interface.
In `@src/lib/auth/sessionManager.ts`:
- Around line 427-455: createSession currently always inserts a new session and
ignores SessionConfig limits; before creating the session, retrieve the user's
current sessions (e.g., via this.storage.getSessionsForUser or
this.storage.findByUser) and enforce this.config.allowMultipleSessions and
this.config.maxSessionsPerUser: if allowMultipleSessions is false, remove or
mark all existing sessions for the user invalid via the storage API before
calling this.storage.set; if maxSessionsPerUser is set and the new session would
exceed it, evict oldest sessions (remove/invalidate via storage) until the count
is within limit, then persist the new session and update logs accordingly (use
maskId(sessionId) and maskId(user.id) as currently done).
In `@src/lib/neurolink.ts`:
- Around line 3010-3017: withTimeout is being called with an Error instance
which violates its signature; change the third arg to a plain string (e.g.,
"Auth token validation timed out") and replace the hardcoded 5000 with
PROVIDER_TIMEOUTS.AUTH_MS; then move mapping to AuthenticationFailedError into
the surrounding catch that handles errors from withTimeout (wrap/convert the
caught TimeoutError or other errors into new AuthenticationFailedError using
this.authProvider.type) so the call becomes
withTimeout(this.authProvider.authenticateToken(options.auth.token),
PROVIDER_TIMEOUTS.AUTH_MS, "Auth token validation timed out") and authResult
handling/conversion happens in the catch.
- Around line 11067-11097: The public auth-context methods setAuthContext,
getAuthContext, and clearAuthContext should be synchronous: import the
auth/authContext.js module once at module scope (so you can reference
globalAuthContext and getAuthContext directly) and change these methods to
return void / AuthenticatedContext | undefined synchronously (remove async/await
and Promise return types), call globalAuthContext.set(context),
getAuthContext(), and globalAuthContext.clear() directly, and keep the same
logging behavior (logger.debug) so callers need not await these methods and you
avoid the race described.
---
Duplicate comments:
In `@src/lib/auth/authContext.ts`:
- Around line 66-68: The code currently falls back to a process-wide
globalAuthContext in getAuthContext (and the related helpers around
requireAuthContext and any getters at the other noted spots), which exposes a
shared mutable auth slot; remove the fallback so getAuthContext returns only
authContextStorage.getStore() and update requireAuthContext and any other
accessor functions to rely exclusively on the request-scoped authContextStorage
(or throw/return undefined) and stop reading from globalAuthContext.get(); keep
globalAuthContext only as an explicit, opt-in API (e.g., via a
runWithAuthContext helper) and add/update callers to explicitly use
runWithAuthContext when process-wide behavior is actually intended.
In `@src/lib/auth/authProvider.ts`:
- Around line 48-67: There are two divergent base classes causing inconsistent
provider behavior; consolidate into the canonical BaseAuthProvider in
BaseAuthProvider (class BaseAuthProvider in src/lib/auth/authProvider.ts) by
merging any logic from the alternate base
(src/lib/auth/providers/BaseAuthProvider.ts) into this class: ensure
tokenExtraction defaults, RBAC hooks, session binding, and sessionStorage
(InMemorySessionStorage) behavior are unified under the single BaseAuthProvider,
update MastraAuthProvider/AuthProviderConfig types as needed, and change all
providers (supabase.ts, custom.ts, jwt.ts, CognitoProvider.ts) to extend this
consolidated BaseAuthProvider and remove or redirect the duplicate base to
prevent split implementations.
In `@src/lib/auth/AuthProviderRegistry.ts`:
- Around line 300-309: Replace the raw Error thrown in the placeholder factory
registered via AuthProviderRegistry.register with a typed AuthRegistryError
created through the ErrorFactory; locate the async factory passed to register
(the one that currently does throw new Error(...)) and change it to throw
ErrorFactory.create(AuthRegistryError, "Direct provider creation not supported.
Use AuthProviderFactory.create() with config.") or the project's equivalent
ErrorFactory usage so the registered factory throws the typed AuthRegistryError
instead of a plain Error.
In `@src/lib/auth/middleware/AuthMiddleware.ts`:
- Around line 103-133: The header extraction is only using headerConfig.prefix
and ignores a provided fromHeader.scheme, breaking custom schemes; update the
logic in the extractToken (AuthMiddleware) flow to prefer headerConfig.scheme
(if defined) and fall back to headerConfig.prefix and then "Bearer" only if
neither is set. Concretely, when computing the scheme used to validate the
header, replace the current const prefix = headerConfig.prefix ?? "Bearer" with
something like const scheme = headerConfig.scheme ?? headerConfig.prefix ??
"Bearer", use scheme for the empty-scheme check (return raw value when scheme is
falsy) and for building schemeWithSpace (scheme + " "), and perform the
case-insensitive comparison against schemeWithSpace instead of prefixWithSpace
so custom fromHeader.scheme values are honored.
- Around line 409-440: The effectivePermissions set is built from
user.permissions and rolePermissions but later permission checks (e.g., in
createProtectedMiddleware) use exact Set.has() so wildcard grants like '*' or
'tools:*' are ignored; update the authorization checks to reuse the shared
permission matcher (the project's common permission matching function) instead
of Set.has(), and where expandRoles/rolePermissions/effectivePermissions are
used, ensure you compare requested permissions by iterating effectivePermissions
and calling the common matcher (e.g., permissionMatches(requested, granted)) to
allow wildcard/hierarchical matches; apply the same replacement to the other
occurrence referenced near createProtectedMiddleware so all checks use the
common matcher.
In `@src/lib/auth/providers/BaseAuthProvider.ts`:
- Around line 800-807: The code currently picks any valid session for the user
(existingSessions.find(...)) which can incorrectly reuse another device/token's
session; change the selection to prefer an existing session that matches the
incoming authentication result's session identifier (e.g., result.sessionId or
result.session?.id) and only fall back to creating a new session if no matching
valid session exists; update the logic in BaseAuthProvider where
existingSessions, validSession, sessionStorage.getForUser, and createSession are
used so you check session.id === result.sessionId (or the equivalent field on
result) in addition to s.isValid and expiry before reusing it.
In `@src/lib/auth/providers/oauth2.ts`:
- Around line 146-171: The token validation currently only optionally checks
payload.iss and compares origins and treats payload.aud as optional; update the
logic in the try block that handles jose.jwtVerify to require exact issuer and
audience matches: add or use an explicit expected issuer configuration on the
OAuth2Config (e.g., expectedIssuer) and fail validation if payload.iss is
missing or not exactly equal to that expectedIssuer (fall back to a strict match
against new URL(this.authorizationUrl).origin only if expectedIssuer is not
set), and fail if payload.aud is missing or does not include this.clientId;
update the error messages to reflect missing vs. mismatched claims and reference
the symbols jwks, authorizationUrl, clientId, and the OAuth2Config
expectedIssuer when locating code to change.
In `@src/lib/auth/providers/supabase.ts`:
- Around line 172-175: The current logic for building the roles array uses
payload.role alone and discards appMetadata.roles when payload.role exists;
update the construction in the roles field so it merges payload.role (if
present) with any appMetadata.roles instead of choosing one or the other:
collect payload.role into an array (e.g., [payload.role]) and concat or merge it
with (appMetadata?.roles as string[]) || [], avoiding duplicates so RBAC
receives both the standard Supabase role and any application roles defined in
appMetadata.
In `@src/lib/auth/providers/workos.ts`:
- Around line 453-462: The catch in getUserByEmail currently logs errors but
swallows non-ProviderAPIError exceptions by returning null, which hides
transport/retry failures; update the catch in getUserByEmail to log and then
re-throw the caught error (preserving ProviderAPIError behavior) instead of
returning null — only allow an explicit empty API result path to return null,
but do not convert exceptions into a null "not found" result.
In `@src/lib/neurolink.ts`:
- Around line 10992-11018: setAuthProvider currently clears authInitPromise but
doesn't prevent an in-flight ensureAuthProvider completion from overwriting a
newer provider; introduce a generation/token (e.g., this.authInitGeneration:
number) incremented whenever setAuthProvider is called or a new init starts,
capture the current generation in the local scope inside ensureAuthProvider and
in any async init paths, and before assigning this.authProvider (and before
clearing this.authInitPromise) compare the captured generation to
this.authInitGeneration and bail out if they differ to ignore stale completions;
apply the same generation-check pattern to the other init path referenced around
lines 11041-11055 so any late-resolving async create() or factory results do not
clobber a later manual override.
In `@src/lib/server/routes/agentRoutes.ts`:
- Around line 70-80: The context construction currently allows unauthenticated
callers to inject request.userId and request.sessionId into trusted context
fields; update the object built in agentRoutes.ts so that context.sessionId and
context.userId are only set when ctx.user is present (e.g., sessionId: ctx.user
? ctx.session?.id : undefined, userId: ctx.user ? ctx.user.id : undefined) or,
if you want to preserve caller values, move them into separate untrusted fields
like rawRequestUserId and rawRequestSessionId instead of populating the trusted
context properties; ensure references to ctx.user, request.userId, and
request.sessionId are adjusted accordingly.
- Around line 137-147: The stream endpoint's context construction currently
risks using caller-supplied IDs (request.sessionId/request.userId) even when an
authenticated ctx.user exists; update the context building in the stream handler
so that if ctx.user is present you always use ctx.session?.id and ctx.user.id
(and ctx.user.email / ctx.user.roles) and never fall back to request.* values,
otherwise fall back to request.sessionId/request.userId when no ctx.user exists;
adjust the assignments for sessionId, userId, userEmail, and userRoles
accordingly in the context object (look for context, sessionId, userId,
userEmail, userRoles, ctx.user, ctx.session, request.sessionId, request.userId).
---
Nitpick comments:
In `@src/lib/auth/providers/clerk.ts`:
- Around line 56-57: ClerkProvider currently uses its own private maps (sessions
and userSessions) instead of the shared sessionStorage on BaseAuthProvider;
replace usages of the private maps with calls to the inherited
this.sessionStorage API (the same pattern used by FirebaseAuthProvider) so
session create/read/update/delete and user->session mappings use the configured
customStorage backend, or if provider-specific in-memory behavior is
intentional, add a clear comment in the ClerkProvider class documenting that
sessions and userSessions are intentionally local and will not use
this.sessionStorage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f46ba752-cddc-4303-9edc-e33c93f4c7d4
⛔ Files ignored due to path filters (2)
landing/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
docs-site/sidebars.tsdocs/advanced/auth-architecture.mddocs/features/authentication-providers.mddocs/features/index.mddocs/superpowers/plans/2026-03-16-remove-redundant-tests.mdpackage.jsonsrc/cli/commands/authProviders.tssrc/cli/factories/authCommandFactory.tssrc/cli/parser.tssrc/lib/auth/AuthProviderFactory.tssrc/lib/auth/AuthProviderRegistry.tssrc/lib/auth/RequestContext.tssrc/lib/auth/authContext.tssrc/lib/auth/authErrors.tssrc/lib/auth/authProvider.tssrc/lib/auth/index.tssrc/lib/auth/middleware/AuthMiddleware.tssrc/lib/auth/middleware/rateLimitByUser.tssrc/lib/auth/providers/BaseAuthProvider.tssrc/lib/auth/providers/CognitoProvider.tssrc/lib/auth/providers/KeycloakProvider.tssrc/lib/auth/providers/auth0.tssrc/lib/auth/providers/betterAuth.tssrc/lib/auth/providers/clerk.tssrc/lib/auth/providers/custom.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/oauth2.tssrc/lib/auth/providers/supabase.tssrc/lib/auth/providers/workos.tssrc/lib/auth/serverBridge.tssrc/lib/auth/sessionManager.tssrc/lib/core/infrastructure/baseRegistry.tssrc/lib/index.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/rag/ChunkerRegistry.tssrc/lib/rag/metadata/MetadataExtractorRegistry.tssrc/lib/rag/reranker/RerankerRegistry.tssrc/lib/server/routes/agentRoutes.tssrc/lib/types/authTypes.tssrc/lib/types/configTypes.tssrc/lib/types/generateTypes.tssrc/lib/types/index.tssrc/lib/types/streamTypes.tstest/continuous-test-suite-auth.tstest/fixtures/auth/provider-config.jsontest/fixtures/auth/session-data.jsontest/fixtures/auth/test-credentials.json
💤 Files with no reviewable changes (1)
- docs/superpowers/plans/2026-03-16-remove-redundant-tests.md
✅ Files skipped from review due to trivial changes (9)
- package.json
- docs-site/sidebars.ts
- src/lib/types/streamTypes.ts
- test/fixtures/auth/test-credentials.json
- test/fixtures/auth/provider-config.json
- test/fixtures/auth/session-data.json
- src/lib/auth/RequestContext.ts
- src/lib/auth/providers/betterAuth.ts
- src/lib/auth/authErrors.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/lib/rag/metadata/MetadataExtractorRegistry.ts
- src/cli/parser.ts
- src/lib/rag/ChunkerRegistry.ts
- src/lib/types/configTypes.ts
- src/lib/core/infrastructure/baseRegistry.ts
- src/lib/auth/serverBridge.ts
- src/lib/rag/reranker/RerankerRegistry.ts
- src/cli/factories/authCommandFactory.ts
- src/lib/types/index.ts
- src/lib/mcp/toolRegistry.ts
- src/lib/types/authTypes.ts
- test/continuous-test-suite-auth.ts
| | **[Context Compaction](context-compaction.md)** | 4-stage context compaction pipeline with automatic budget management, per-provider token estimation, and non-destructive message tagging. | | ||
| | **[Memory](memory.md)** | Per-user condensed memory that persists across conversations. LLM-powered condensation with S3, Redis, or SQLite storage backends. | | ||
| | **[Claude Subscription Support](claude-subscription.md)** | Multiple authentication methods for Claude (API key, OAuth) with support for Free, Pro, Max, and API tiers. | | ||
| | **[Authentication Providers](/docs/features/authentication-providers)** | Secure AI endpoints with 11 auth providers (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, WorkOS, Better Auth, OAuth2, JWT, Custom) with RBAC, session management, and rate limiting. | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the authentication-providers doc exists and what format other links use
fd -e md authentication-providers
# Check existing link formats in the features index
rg -n '\[.*\]\(' docs/features/index.md | head -20Repository: juspay/neurolink
Length of output: 4661
Fix inconsistent link format for consistency with other entries in the table.
The Authentication Providers link uses an absolute-style path /docs/features/authentication-providers while all other table entries use relative paths. Since the file exists at docs/features/authentication-providers.md, use the relative path format for consistency:
Suggested change
-| **[Authentication Providers](/docs/features/authentication-providers)** | Secure AI endpoints with 11 auth providers (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, WorkOS, Better Auth, OAuth2, JWT, Custom) with RBAC, session management, and rate limiting. |
+| **[Authentication Providers](authentication-providers.md)** | Secure AI endpoints with 11 auth providers (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, WorkOS, Better Auth, OAuth2, JWT, Custom) with RBAC, session management, and rate limiting. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | **[Authentication Providers](/docs/features/authentication-providers)** | Secure AI endpoints with 11 auth providers (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, WorkOS, Better Auth, OAuth2, JWT, Custom) with RBAC, session management, and rate limiting. | | |
| | **[Authentication Providers](authentication-providers.md)** | Secure AI endpoints with 11 auth providers (Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, WorkOS, Better Auth, OAuth2, JWT, Custom) with RBAC, session management, and rate limiting. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features/index.md` at line 28, The table entry for "Authentication
Providers" uses an absolute path; update the link target for the "Authentication
Providers" table cell to use a relative path consistent with the other entries
(e.g., change /docs/features/authentication-providers to a relative link like
./authentication-providers or authentication-providers.md) so it points to the
existing docs/features/authentication-providers.md file and matches the
surrounding link format.
| // Fail closed: valid token without a user object is treated as failure | ||
| if (!validationResult.user) { | ||
| const error = { | ||
| statusCode: 401, | ||
| message: "Token valid but no user identity resolved", | ||
| code: "AUTH-001" as AuthErrorCode, | ||
| }; | ||
|
|
||
| if (config.onError) { | ||
| await config.onError( | ||
| Object.assign(new Error(error.message), { | ||
| code: error.code, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| }) as any, | ||
| context, | ||
| ); | ||
| } | ||
|
|
||
| return { proceed: false, error }; | ||
| } |
There was a problem hiding this comment.
Reject “authenticated” users with no id.
This guard only checks that user exists. A provider can still return valid: true with user.id empty/undefined, and the middleware will treat that request as authenticated.
Suggested fix
- if (!validationResult.user) {
+ if (!validationResult.user?.id) {
const error = {
statusCode: 401,
- message: "Token valid but no user identity resolved",
+ message: "Token valid but no stable user identity resolved",
code: "AUTH-001" as AuthErrorCode,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/middleware/AuthMiddleware.ts` around lines 284 - 303,
AuthMiddleware currently treats a token as authenticated if
validationResult.user exists even when user.id is empty; update the guard in the
validation flow (the block checking validationResult.user) to also require a
non-empty user.id (or equivalent unique identifier) and treat missing/empty id
as authentication failure: construct the same AuthError (statusCode 401, message
like "Token valid but no user identity resolved", code "AUTH-001"), call
config.onError with an Error augmented with code when present (as currently
done), and return { proceed: false, error } so requests with user objects
lacking an id are rejected; locate the check around validationResult.user in
AuthMiddleware to make this change.
| private async createClient(): Promise<RedisClient> { | ||
| try { | ||
| // Dynamic import to avoid loading Redis unless needed | ||
| const { createClient } = await import("redis"); | ||
| const client = createClient({ url: this.redisUrl }); | ||
| await client.connect(); | ||
| this.client = client as unknown as RedisClient; | ||
| return this.client; | ||
| } catch { | ||
| this.initPromise = null; | ||
| throw new Error("Redis client not available for rate limiting"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
node-redis v5 error listener requirement before connect
💡 Result:
Yes, in node-redis v5, you must attach an 'error' event listener to the client before calling connect. This prevents unhandled 'error' events from crashing the Node.js process, as per Node.js EventEmitter behavior. Official examples and docs show: import { createClient } from 'redis'; const client = createClient .on('error', err => console.error('Redis Client Error', err)) .connect; Without the listener, errors (e.g., connection failures, socket closures) emit as unhandled, terminating the process. This applies during and after connect, including reconnections. Docs emphasize: "You MUST listen to error events. If a client doesn't have at least one error listener registered and an error occurs, that error will be thrown and the Node.js process will exit." This requirement exists in v4 and continues unchanged in v5.
Citations:
- 1: https://redis.io/docs/latest/develop/clients/nodejs/error-handling/
- 2: https://www.npmjs.com/package/redis/v/5.0.0-next.5
- 3: https://www.npmjs.com/package/redis/v/5.0.0?activeTab=versions
- 4: https://redis.io/docs/latest/develop/clients/nodejs/
- 5: https://redis.io/docs/latest/develop/clients/nodejs/produsage
- 6: https://github.com/redis/node-redis/releases
- 7: https://redis.io/docs/latest/develop/clients/nodejs/connect
🏁 Script executed:
# Check the actual code at lines 194-206 in rateLimitByUser.ts
fd "rateLimitByUser.ts" --type f -x cat -n {} | sed -n '190,210p'Repository: juspay/neurolink
Length of output: 837
🏁 Script executed:
# Also check the imports at the top of the file to see if logger is available
fd "rateLimitByUser.ts" --type f -x head -50 {}Repository: juspay/neurolink
Length of output: 1351
🏁 Script executed:
# Check if there are any error listeners registered elsewhere in the file
fd "rateLimitByUser.ts" --type f -x grep -n "\.on(" {}Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check getClient() and other methods to see if there's higher-level error handling
fd "rateLimitByUser.ts" --type f -x sed -n '170,195p' {}Repository: juspay/neurolink
Length of output: 821
🏁 Script executed:
# Check if this client is used elsewhere and how errors might be handled
fd "rateLimitByUser.ts" --type f -x grep -n "this.client" {}Repository: juspay/neurolink
Length of output: 297
Register Redis error listener before connect().
Node Redis v5 requires an error event listener before calling connect() to prevent unhandled errors from crashing the process. The initialization promise pattern is correct, but the error listener is missing.
Proposed fix
private async createClient(): Promise<RedisClient> {
try {
// Dynamic import to avoid loading Redis unless needed
const { createClient } = await import("redis");
const client = createClient({ url: this.redisUrl });
+
+ // Register error listener before connect (required by node-redis v5)
+ client.on("error", (err) => {
+ logger.warn("Redis rate limit client error:", err);
+ });
+
await client.connect();
this.client = client as unknown as RedisClient;
return this.client;
} catch {
this.initPromise = null;
throw new Error("Redis client not available for rate limiting");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/middleware/rateLimitByUser.ts` around lines 194 - 206, In
createClient(), register an 'error' event listener on the newly created Redis
client before calling client.connect() so Node Redis v5 won't emit unhandled
errors; specifically, in the createClient() method add client.on('error', (err)
=> { /* log via your logger or reject/initPromise cleanup */ }) immediately
after const client = createClient({ url: this.redisUrl }) and before await
client.connect(), and ensure you still clear initPromise and throw the same
error if connection fails (preserve existing initPromise cleanup behavior).
| // Check permissions (all required) | ||
| if (options.permissions && options.permissions.length > 0) { | ||
| const userPermissions = this.getEffectivePermissions(user); | ||
| const missingPermissions = options.permissions.filter( | ||
| (p) => !userPermissions.has(p), | ||
| ); | ||
|
|
||
| if (missingPermissions.length > 0) { | ||
| result.authorized = false; | ||
| result.missingPermissions = missingPermissions; | ||
| result.reason = result.reason | ||
| ? `${result.reason}; Missing permissions: ${missingPermissions.join(", ")}` | ||
| : `Missing required permissions: ${missingPermissions.join(", ")}`; | ||
| } |
There was a problem hiding this comment.
Honor wildcard permissions in authorize().
This exact Set.has check rejects users that only have "*" or "tools:*". authorizeUser() and authorizePermissions() delegate here, so providers using this base can deny requests that src/lib/auth/authContext.ts and the other auth base would authorize.
| const proxyFetch = createProxyFetch(); | ||
| const response = await proxyFetch( | ||
| "https://api.clerk.com/v1/sessions/verify", | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| Authorization: `Bearer ${this.secretKey}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ token }), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Add timeout to external Clerk API calls.
The validateSessionToken() method calls the Clerk API without a timeout. Per the PR objectives, external provider calls should be bounded by a 5s timeout to prevent hung requests. The same applies to getUser(), getUserByEmail(), and healthCheck().
Proposed fix for validateSessionToken
const proxyFetch = createProxyFetch();
const response = await proxyFetch(
"https://api.clerk.com/v1/sessions/verify",
{
method: "POST",
headers: {
Authorization: `Bearer ${this.secretKey}`,
"Content-Type": "application/json",
},
body: JSON.stringify({ token }),
+ signal: AbortSignal.timeout(5000),
},
);As per coding guidelines: "Use withTimeout for async operations."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const proxyFetch = createProxyFetch(); | |
| const response = await proxyFetch( | |
| "https://api.clerk.com/v1/sessions/verify", | |
| { | |
| method: "POST", | |
| headers: { | |
| Authorization: `Bearer ${this.secretKey}`, | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ token }), | |
| }, | |
| ); | |
| const proxyFetch = createProxyFetch(); | |
| const response = await proxyFetch( | |
| "https://api.clerk.com/v1/sessions/verify", | |
| { | |
| method: "POST", | |
| headers: { | |
| Authorization: `Bearer ${this.secretKey}`, | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ token }), | |
| signal: AbortSignal.timeout(5000), | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/providers/clerk.ts` around lines 176 - 187, The Clerk provider
calls to the external API must be bounded by a 5s timeout: update
validateSessionToken(), getUser(), getUserByEmail(), and healthCheck() to wrap
their async fetch calls (the ones using createProxyFetch()) with the shared
withTimeout helper set to 5000 ms (or 5_000) so the promise rejects after 5s;
locate the proxyFetch invocation in each method (e.g., the POST to
"https://api.clerk.com/v1/sessions/verify" in validateSessionToken) and replace
the direct await call with await withTimeout(<existing proxyFetch call>, 5000)
so all external Clerk requests obey the timeout.
| // Extract user from claims | ||
| const user = this.extractKeycloakUser(claims); | ||
|
|
||
| // Convert claims to Record<string, JsonValue> by filtering out undefined | ||
| const validClaims: Record< | ||
| string, | ||
| import("../../types/common.js").JsonValue | ||
| > = {}; | ||
| for (const [key, value] of Object.entries(claims)) { | ||
| if (value !== undefined) { | ||
| validClaims[key] = value; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| valid: true, | ||
| user, | ||
| claims: validClaims, | ||
| expiresAt: claims.exp ? new Date(claims.exp * 1000) : undefined, | ||
| issuer: claims.iss, | ||
| audience: claims.aud, | ||
| }; |
There was a problem hiding this comment.
Reject JWTs that don't carry a subject.
extractKeycloakUser() falls back to id = "", so this path can return valid: true for a token with no stable user identity. That breaks fail-closed auth and can collapse sessions or per-user controls onto the empty id.
Suggested fix
- // Extract user from claims
- const user = this.extractKeycloakUser(claims);
+ if (!claims.sub) {
+ return {
+ valid: false,
+ error: "Token is missing required 'sub' claim",
+ errorCode: "AUTH-001",
+ };
+ }
+
+ // Extract user from claims
+ const user = this.extractKeycloakUser(claims);Also applies to: 307-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/providers/KeycloakProvider.ts` around lines 193 - 214, The
parser currently accepts tokens with no subject because extractKeycloakUser()
falls back to an empty id; update the KeycloakProvider token validation to
reject JWTs that lack a non-empty claims.sub before returning valid: true (i.e.,
in the block that builds the result object using extractKeycloakUser and
validClaims), and return valid: false (with no user) when claims.sub is missing
or an empty string; apply the same check to the other similar token-return path
that also uses extractKeycloakUser so tokens without a stable subject are
fail-closed.
| private sessions: Map<string, AuthSession> = new Map(); | ||
| private userSessions: Map<string, Set<string>> = new Map(); |
There was a problem hiding this comment.
Don't fork session state into private maps here.
These maps sidestep the shared sessionStorage path, so allowMultipleSessions, maxSessionsPerUser, revocation behavior, and any custom or Redis-backed storage configured on the auth layer never apply to Supabase sessions. This provider will behave differently from the rest of the auth stack for the same session config.
Also applies to: 209-239, 244-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/providers/supabase.ts` around lines 56 - 57, The Supabase
provider is forking session state into private maps (sessions and userSessions)
which bypasses the shared sessionStorage and breaks allowMultipleSessions,
maxSessionsPerUser, revocation and any custom/Redis-backed storage; remove these
private maps and stop reading/writing to sessions and userSessions, and instead
use the shared sessionStorage API used by the rest of the auth layer (the same
session create/get/delete/update calls) so all session lifecycle logic
(allowMultipleSessions, maxSessionsPerUser, revoke semantics) is preserved;
update all methods that reference sessions or userSessions (including the blocks
you noted around 209–239 and 244–326) to delegate to the central sessionStorage
implementation and ensure revocation and session limits are enforced via that
shared interface.
| async createSession( | ||
| user: AuthUser, | ||
| metadata?: { | ||
| ipAddress?: string; | ||
| userAgent?: string; | ||
| deviceId?: string; | ||
| }, | ||
| ): Promise<AuthSession> { | ||
| const sessionId = crypto.randomUUID(); | ||
| const now = new Date(); | ||
| const duration = this.config.duration || 3600; | ||
|
|
||
| const session: AuthSession = { | ||
| id: sessionId, | ||
| user, | ||
| createdAt: now, | ||
| expiresAt: new Date(now.getTime() + duration * 1000), | ||
| isValid: true, | ||
| ipAddress: metadata?.ipAddress, | ||
| userAgent: metadata?.userAgent, | ||
| deviceId: metadata?.deviceId, | ||
| }; | ||
|
|
||
| await this.storage.set(session); | ||
| logger.debug( | ||
| `Session created: ${maskId(sessionId)} for user: ${maskId(user.id)}`, | ||
| ); | ||
|
|
||
| return session; |
There was a problem hiding this comment.
Enforce session limits in SessionManager.createSession().
This path ignores allowMultipleSessions and maxSessionsPerUser from SessionConfig and always inserts a new session. Consumers using SessionManager directly will therefore get unlimited concurrent sessions even when the same config would be enforced by provider-backed session creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/auth/sessionManager.ts` around lines 427 - 455, createSession
currently always inserts a new session and ignores SessionConfig limits; before
creating the session, retrieve the user's current sessions (e.g., via
this.storage.getSessionsForUser or this.storage.findByUser) and enforce
this.config.allowMultipleSessions and this.config.maxSessionsPerUser: if
allowMultipleSessions is false, remove or mark all existing sessions for the
user invalid via the storage API before calling this.storage.set; if
maxSessionsPerUser is set and the new session would exceed it, evict oldest
sessions (remove/invalidate via storage) until the count is within limit, then
persist the new session and update logs accordingly (use maskId(sessionId) and
maskId(user.id) as currently done).
| authResult = await withTimeout( | ||
| this.authProvider.authenticateToken(options.auth.token), | ||
| 5000, | ||
| new AuthenticationFailedError( | ||
| "Auth token validation timed out after 5000ms", | ||
| this.authProvider.type, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== withTimeout signature =="
sed -n '63,85p' src/lib/utils/async/withTimeout.ts
echo
echo "== generate()/stream() auth timeout call sites =="
sed -n '3010,3017p;5618,5625p' src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 1365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PROVIDER_TIMEOUTS constant =="
rg "PROVIDER_TIMEOUTS|AUTH_MS" src/lib/neurolink.ts | head -20
echo
echo "== Surrounding code at line 3010 (context: 2995-3025) =="
sed -n '2995,3025p' src/lib/neurolink.ts
echo
echo "== Surrounding code at line 5618 (context: 5603-5633) =="
sed -n '5603,5633p' src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 3045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PROVIDER_TIMEOUTS definition =="
rg "PROVIDER_TIMEOUTS\s*=" src/lib/ -A 5 --type ts
echo
echo "== AUTH_MS value =="
rg "AUTH_MS\s*:" src/lib/ --type tsRepository: juspay/neurolink
Length of output: 704
Pass a string message to withTimeout(), not an error instance.
withTimeout() expects message?: string as the third argument. Passing new AuthenticationFailedError(...) violates the type contract and breaks strict TypeScript. The helper throws TimeoutError internally regardless—the passed error instance is ignored. Move the auth-error mapping to the surrounding catch block and pass a plain string message to withTimeout().
Additionally, replace the hardcoded 5000 timeout with PROVIDER_TIMEOUTS.AUTH_MS for consistency with other timeout values.
Suggested fix
- authResult = await withTimeout(
- this.authProvider.authenticateToken(options.auth.token),
- 5000,
- new AuthenticationFailedError(
- "Auth token validation timed out after 5000ms",
- this.authProvider.type,
- ),
- );
+ authResult = await withTimeout(
+ this.authProvider.authenticateToken(options.auth.token),
+ PROVIDER_TIMEOUTS.AUTH_MS,
+ `Auth token validation timed out after ${PROVIDER_TIMEOUTS.AUTH_MS}ms`,
+ );Also applies to: 5618-5625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 3010 - 3017, withTimeout is being called
with an Error instance which violates its signature; change the third arg to a
plain string (e.g., "Auth token validation timed out") and replace the hardcoded
5000 with PROVIDER_TIMEOUTS.AUTH_MS; then move mapping to
AuthenticationFailedError into the surrounding catch that handles errors from
withTimeout (wrap/convert the caught TimeoutError or other errors into new
AuthenticationFailedError using this.authProvider.type) so the call becomes
withTimeout(this.authProvider.authenticateToken(options.auth.token),
PROVIDER_TIMEOUTS.AUTH_MS, "Auth token validation timed out") and authResult
handling/conversion happens in the catch.
| async setAuthContext(context: AuthenticatedContext): Promise<void> { | ||
| const { globalAuthContext } = await import("./auth/authContext.js"); | ||
| globalAuthContext.set(context); | ||
| logger.debug("Auth context set", { | ||
| userId: context.user.id, | ||
| provider: context.provider, | ||
| sessionId: context.session?.id, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get the current authentication context. | ||
| * | ||
| * Checks AsyncLocalStorage first, then falls back to the global holder. | ||
| */ | ||
| async getAuthContext(): Promise<AuthenticatedContext | undefined> { | ||
| const { getAuthContext: getCtx } = await import("./auth/authContext.js"); | ||
| return getCtx(); | ||
| } | ||
|
|
||
| /** | ||
| * Clear the current authentication context | ||
| */ | ||
| async clearAuthContext(): Promise<void> { | ||
| const { globalAuthContext } = await import("./auth/authContext.js"); | ||
| const userId = globalAuthContext.get()?.user.id; | ||
| globalAuthContext.clear(); | ||
| if (userId) { | ||
| logger.debug(`Auth context cleared for user: ${userId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if these methods existed before and their previous signatures
cd /tmp/repo && git log --oneline -n 20 -- src/lib/neurolink.ts | head -10Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Check the current state of authContext.js to understand dependencies
cd /tmp/repo && cat -n src/lib/auth/authContext.js | head -50Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Search for usage of these auth context methods in the codebase
cd /tmp/repo && rg "setAuthContext|getAuthContext|clearAuthContext" --type ts --type js -A 2 -B 2 | head -100Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Check if there's top-level await in neurolink.ts and how imports are currently done
cd /tmp/repo && head -100 src/lib/neurolink.ts | grep -E "(^import|^export|await|top-level)" -A 1Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Check if authContext is already imported at module scope anywhere
cd /tmp/repo && rg "authContext" src/lib/neurolink.ts | head -20Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Check the current state of authContext.js to understand dependencies
cat -n src/lib/auth/authContext.js | head -60Repository: juspay/neurolink
Length of output: 119
🏁 Script executed:
# Search for usage of these auth context methods in the codebase
rg "setAuthContext|getAuthContext|clearAuthContext" -A 2 -B 2 | head -150Repository: juspay/neurolink
Length of output: 5935
🏁 Script executed:
# Check if there's top-level await in neurolink.ts and how imports are currently done
head -150 src/lib/neurolink.ts | grep -E "(import|export|await)" -A 1Repository: juspay/neurolink
Length of output: 3349
🏁 Script executed:
# Check how authContext is currently referenced in neurolink.ts
rg "authContext" src/lib/neurolink.ts -B 2 -A 2 | head -100Repository: juspay/neurolink
Length of output: 1853
🏁 Script executed:
# Get file structure to understand where auth context is located
git ls-files | grep -i auth | head -20Repository: juspay/neurolink
Length of output: 786
🏁 Script executed:
# Check the authContext.ts file structure
cat -n src/lib/auth/authContext.ts | head -100Repository: juspay/neurolink
Length of output: 3259
🏁 Script executed:
# Check if authContext.ts has any circular dependencies or async initialization
rg "import|export|async" src/lib/auth/authContext.ts | head -50Repository: juspay/neurolink
Length of output: 1303
🏁 Script executed:
# Look for how getAuthContext is currently imported/used in other parts of the SDK
rg "from.*authContext" src/lib --type ts | head -20Repository: juspay/neurolink
Length of output: 193
🏁 Script executed:
# Check existing tests to see how setAuthContext/getAuthContext are used
rg "setAuthContext\|getAuthContext\|clearAuthContext" test --type ts -B 2 -A 2 | head -100Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if neurolink.ts already imports anything from auth/authContext.ts at module scope
grep -n "auth/authContext" src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 301
🏁 Script executed:
# Check documentation to see if setAuthContext/getAuthContext are documented as sync
grep -A 5 "setAuthContext\|getAuthContext" docs/features/authentication-providers.md | head -30Repository: juspay/neurolink
Length of output: 536
🏁 Script executed:
# Look for any migration guide or changelog that might indicate API change
fd -e md . docs | xargs grep -l "breaking\|migration\|changelog" 2>/dev/null | head -5Repository: juspay/neurolink
Length of output: 228
🏁 Script executed:
# Check if there's a CHANGELOG or migration guide
fd -e md . . | xargs grep -l "setAuthContext" 2>/dev/nullRepository: juspay/neurolink
Length of output: 140
🏁 Script executed:
# Check the auth-architecture docs to understand intended design
cat -n docs/advanced/auth-architecture.md | head -100Repository: juspay/neurolink
Length of output: 5125
🏁 Script executed:
# Look at the actual method signatures and comments in neurolink.ts for these methods
sed -n '11060,11100p' src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 1483
🏁 Script executed:
# Check if there are any exports or API declarations that show the intended signature
rg "setAuthContext|getAuthContext|clearAuthContext" src/lib/index.ts -B 2 -A 2Repository: juspay/neurolink
Length of output: 158
🏁 Script executed:
# Check if there's a git history showing these methods were previously sync
git log -p --follow -S "setAuthContext" -- src/lib/neurolink.ts | head -80Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Verify globalAuthContext and related exports are available at module level
sed -n '100,150p' src/lib/auth/authContext.tsRepository: juspay/neurolink
Length of output: 1230
🏁 Script executed:
# Check what globalAuthContext class looks like and verify it's synchronous
sed -n '150,200p' src/lib/auth/authContext.tsRepository: juspay/neurolink
Length of output: 1175
🏁 Script executed:
# Double-check if there are any async operations in authContext initialization
tail -100 src/lib/auth/authContext.tsRepository: juspay/neurolink
Length of output: 2013
🏁 Script executed:
# Verify the documentation shows sync behavior expected
cat docs/features/authentication-providers.md | grep -A 20 "setAuthContext"Repository: juspay/neurolink
Length of output: 1128
Keep the auth-context API synchronous.
Making these public methods return Promises is a breaking SDK change, and setAuthContext() can now race with an immediate generate()/stream() call if callers do not await it. The underlying authContext module is fully synchronous (containing only sync functions like globalAuthContext.set(), getAuthContext()) and is already imported at module scope elsewhere (e.g., toolRegistry.ts). Load authContext.js once at module scope and keep the public methods sync to maintain backward compatibility and avoid race conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 11067 - 11097, The public auth-context
methods setAuthContext, getAuthContext, and clearAuthContext should be
synchronous: import the auth/authContext.js module once at module scope (so you
can reference globalAuthContext and getAuthContext directly) and change these
methods to return void / AuthenticatedContext | undefined synchronously (remove
async/await and Promise return types), call globalAuthContext.set(context),
getAuthContext(), and globalAuthContext.clear() directly, and keep the same
logging behavior (logger.debug) so callers need not await these methods and you
avoid the race described.
9757568 to
f5099a8
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
f5099a8 to
af3cfeb
Compare
…ddleware, RBAC, and session management Implements a pluggable authentication layer following the Factory + Registry pattern: - 12 auth providers: Auth0, Clerk, Firebase, Supabase, Cognito, Keycloak, BetterAuth, WorkOS, JWT, OAuth2/OIDC, Custom - AuthMiddleware with token extraction (header/cookie/query), RBAC enforcement, public route bypass - Rate limiting middleware (token bucket, per-user/per-role limits, memory + Redis backends) - SessionManager with memory and Redis storage, auto-refresh - AsyncLocalStorage-based auth context propagation - SDK integration: NeuroLink constructor auth config, generate/stream requestContext and auth.token support - CLI integration: auth providers/validate/health/login/logout/status/refresh commands - 22 test cases across 4 sections (server auth, RBAC, per-call pre-validated, per-call token validation)
af3cfeb to
0013d91
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.32.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
NeuroLinkconstructorauthconfig,generate()/stream()requestContextandauth.tokensupport) and CLI (auth providers/validate/health/login/logout/status/refreshcommands)What's included
Core System (~3,200 lines)
AuthProviderFactory— Singleton factory with lazy dynamic imports for all 12 providersAuthProviderRegistry— Metadata tracking, feature discovery, health checks with latency metricsBaseAuthProvider— Abstract base class with token extraction, permission hierarchy (tools:*includestools:execute), role hierarchy, event emitterauthContext—AsyncLocalStorageimplicit context propagation +AuthContextHolderfallbackauthErrors— 13 error classes, 20 error codes, type guards (isAuthError,isTokenError, etc.)sessionManager—MemorySessionStorage,RedisSessionStorage,SessionManagerwith auto-refreshRequestContext— Type-safe request-scoped Map with reserved keysAuth Providers (~5,300 lines)
joselibraryvalidateTokenfunctionMiddleware (~900 lines)
AuthMiddleware— Token extraction (header/cookie/query/custom), RBAC enforcement, public route bypass, optional auth moderateLimitByUser— Token bucket algorithm, per-role/per-user limits, memory + Redis storageSDK Integration
NeurolinkConstructorConfig.authaccepts pre-built provider or{ type, config }for factory creationgenerate()/stream()acceptrequestContext(pre-validated user context) andauth.token(runtime token validation)setAuthProvider()/getAuthProvider()methods on NeuroLink instanceCLI Integration
auth providers— List available providers with metadataauth validate <token>— Validate token against configured providerauth health— Check provider health statusauth login/logout/status/refresh— Auth lifecycle commandsType System (878 lines)
AuthProviderType,AuthUser,AuthSession,TokenValidationResult,AuthorizationResultAuthEventType/AuthEventDatafor lifecycle eventsAuthErrorCodevaluesTest plan
generate()/stream()acceptrequestContextwith userId, roles, empty contexttsc --noEmit --strictSummary by CodeRabbit
New Features
Documentation