feat: implement authorities module with RBAC/ABAC/ReBAC support and related tools#349
Open
frontegg-david wants to merge 3 commits intomainfrom
Open
feat: implement authorities module with RBAC/ABAC/ReBAC support and related tools#349frontegg-david wants to merge 3 commits intomainfrom
frontegg-david wants to merge 3 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a comprehensive authorization system enabling tools, resources, prompts, and agents to declare and enforce fine-grained access control policies via RBAC, ABAC, and ReBAC models. Includes evaluators, context builders, scope mapping, and integration across all execution flows with per-entry authority checks and discovery filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Flow as Tool Call Flow
participant Engine as Authorities Engine
participant Resolver as Relationship Resolver
participant Vault as Token Vault
Client->>Flow: callTool(tool)
activate Flow
Flow->>Flow: findTool(toolName)
Flow->>Flow: checkToolAuthorization()
alt Tool has metadata.authorities
Flow->>Engine: evaluate(authorities, ctx)
activate Engine
alt Policy type: roles/permissions
Engine->>Engine: evaluateRbacRoles/Permissions()
else Policy type: conditions
Engine->>Engine: evaluateAbac()
else Policy type: relationships
Engine->>Engine: evaluateRebac()
Engine->>Resolver: check(relationship)
activate Resolver
Resolver-->>Engine: relationship exists?
deactivate Resolver
end
alt Result.granted === true
Engine-->>Flow: AuthoritiesResult.granted
else
Engine-->>Flow: AuthoritiesResult.denied with deniedBy/denial
end
deactivate Engine
alt Granted
Flow->>Flow: createToolContext()
Flow->>Flow: executeTool()
else Denied
Flow-->>Client: AuthorityDeniedError(403)
end
else No authorities
Flow->>Flow: createToolContext()
Flow->>Flow: executeTool()
end
deactivate Flow
sequenceDiagram
participant Client as MCP Client
participant Flow as Tools List Flow
participant Builder as Authorities Context Builder
participant Engine as Authorities Engine
participant Registry as Profile Registry
Client->>Flow: listTools()
activate Flow
Flow->>Flow: findTools()
Note over Flow: filterByAuthorities stage
Loop for each tool
alt Tool has metadata.authorities
Flow->>Builder: build(authInfo, toolInput)
activate Builder
Builder-->>Flow: AuthoritiesEvaluationContext
deactivate Builder
Flow->>Engine: evaluate(tool.authorities, ctx)
activate Engine
alt authorities is string (profile name)
Engine->>Registry: resolve(profileName)
Registry-->>Engine: AuthoritiesPolicyMetadata
end
Engine->>Engine: evaluatePolicy()
alt Granted
Engine-->>Flow: result.granted = true
else
Engine-->>Flow: result.granted = false
end
deactivate Engine
alt Result NOT granted
Note over Flow: Remove from results
end
else No authorities
Note over Flow: Keep in results
end
end
Flow-->>Client: filteredTools[]
deactivate Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| function isFrontMcpCredentials( | ||
| creds: FrontMcpCredentials | RequestCredentials, | ||
| ): creds is FrontMcpCredentials { | ||
| return typeof creds === 'object' && creds !== null && 'provider' in creds; |
| }); | ||
|
|
||
| describe('AuthoritiesContextBuilder', () => { | ||
| const noopResolver: RelationshipResolver = { check: async () => false }; |
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 97 tests across 20 projects 📊 View full report in workflow run Generated at: 2026-04-08T02:37:35.560Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (24)
libs/auth/src/authorities/__tests__/authorities.registry.spec.ts (1)
1-110: Consider adding error path and edge case tests.The test suite covers happy-path scenarios well, but per coding guidelines requiring testing of all code paths including errors, consider adding:
- Tests for invalid inputs (e.g., empty string keys,
undefinedvalues).- Constructor validation tests if the registries perform any validation.
instanceofchecks if specific error classes are thrown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/__tests__/authorities.registry.spec.ts` around lines 1 - 110, Tests lack error-path and edge-case coverage; add unit tests that validate handling of invalid inputs and thrown errors. For AuthoritiesProfileRegistry add tests for register/resolve/remove with invalid keys (empty string, undefined) and invalid values (undefined), verify registerAll rejects or ignores malformed entries, and assert behavior of resolve/remove when given invalid types; if the constructor performs validation, add a test for constructing AuthoritiesProfileRegistry with bad initial state. For AuthoritiesEvaluatorRegistry add tests for register/get/remove with empty/undefined keys and undefined evaluators, ensure registerAll rejects malformed evaluator maps, and include instanceof assertions for any specific error classes thrown by register or constructor; reference the register, registerAll, resolve, remove, get, size and clear methods on AuthoritiesProfileRegistry and AuthoritiesEvaluatorRegistry when writing these tests.apps/e2e/demo-e2e-authorities/src/apps/authorities/resources/admin-config.resource.ts (1)
3-21: Consider extracting the URI constant to avoid duplication.The URI
config://admin-settingsappears in both the decorator (line 5) and the return value (line 15). Consider extracting to a constant for maintainability.♻️ Suggested refactor
import { Resource, ResourceContext } from '@frontmcp/sdk'; +const RESOURCE_URI = 'config://admin-settings'; + `@Resource`({ name: 'admin-config', - uri: 'config://admin-settings', + uri: RESOURCE_URI, description: 'Admin-only configuration resource', mimeType: 'application/json', authorities: 'admin', }) export default class AdminConfigResource extends ResourceContext { async execute(): Promise<{ contents: Array<{ uri: string; mimeType: string; text: string }> }> { return { contents: [ { - uri: 'config://admin-settings', + uri: RESOURCE_URI, mimeType: 'application/json', text: JSON.stringify({ secret: 'admin-only-value', level: 'restricted' }), }, ], }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/src/apps/authorities/resources/admin-config.resource.ts` around lines 3 - 21, The URI string "config://admin-settings" is duplicated in the `@Resource` decorator and the execute() return; extract it to a single constant (e.g., ADMIN_CONFIG_URI) near the top of the file and replace both occurrences with that constant to avoid drift. Update the `@Resource` decorator (name/uri/mimeType/authorities) and the object returned by AdminConfigResource.execute() to reference ADMIN_CONFIG_URI so both metadata and payload remain consistent.apps/e2e/demo-e2e-authorities/src/apps/authorities/resources/public-info.resource.ts (1)
10-16: Use the concrete MCP resource result type here.The inline return signature duplicates the protocol contract and will drift the next time the resource result shape changes. Please return the concrete resource result type instead of spelling the structure by hand.
As per coding guidelines, "For MCP response types (Tools, Prompts, Resources), use strictly typed MCP protocol types, never
unknownfor protocol types."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/src/apps/authorities/resources/public-info.resource.ts` around lines 10 - 16, The execute method currently returns an inline Promise signature duplicating the MCP resource result shape; change the return type of execute to the concrete MCP resource result type used in the project (replace Promise<{ contents: Array<...> }> with the protocol's Resource result type) and keep the returned value shape the same (the contents array with uri/mimeType/text). Update the signature on execute in public-info.resource.ts and import/use the canonical MCP resource result type from the protocol types so the method returns the typed MCP Resource result instead of an inline object type.libs/sdk/src/common/utils/path.utils.ts (1)
43-63: Consider scheme-specific default port handling.The current implementation excludes ports 80 and 443 regardless of scheme. Per RFC 3986, the default port is scheme-specific: 80 for
http://and 443 forhttps://. The current logic would incorrectly normalizehttp://example.com:443tohttp://example.com.♻️ Suggested fix for scheme-aware port handling
export function normalizeResourceUri(uri: string): string { try { const url = new URL(uri); // Lowercase scheme and host (URL constructor does this automatically) let normalized = `${url.protocol}//${url.hostname}`; // Include port only if non-standard for the scheme - if (url.port && url.port !== '443' && url.port !== '80') { + const isDefaultPort = + (url.protocol === 'https:' && url.port === '443') || + (url.protocol === 'http:' && url.port === '80'); + if (url.port && !isDefaultPort) { normalized += `:${url.port}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/utils/path.utils.ts` around lines 43 - 63, normalizeResourceUri incorrectly treats ports 80 and 443 as universally default; update the port inclusion logic to be scheme-aware: derive the default port from url.protocol (e.g., 'http:' -> '80', 'https:' -> '443', others none) and only append url.port when url.port exists and differs from that scheme's default. Modify the block in normalizeResourceUri that checks url.port to compare against the scheme-specific default (or implement a small helper like getDefaultPortForScheme used by normalizeResourceUri) so that, for example, http://example.com:443 is preserved while http://example.com:80 is normalized away.libs/observability/src/plugin/observability.hooks.ts (1)
280-285: Inconsistent event recording pattern inonToolDidCheckEntryAuthorities.The
onToolWillCheckEntryAuthoritiesusesrecordStageEvent(which tracks duration from previous stage), butonToolDidCheckEntryAuthoritiesusesspan.addEventdirectly. This breaks the duration tracking for this stage.♻️ Suggested fix for consistency
export function onToolDidCheckEntryAuthorities(flowCtx: any): void { const span: Span | undefined = flowCtx.state?.[SPAN_KEY]; if (!span) return; - span.addEvent('checkEntryAuthorities.done'); + recordStageEvent(span, 'checkEntryAuthorities.done', flowCtx.state); span.setAttribute('auth.authorities.result', 'granted'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/observability/src/plugin/observability.hooks.ts` around lines 280 - 285, The function onToolDidCheckEntryAuthorities currently calls span.addEvent directly which breaks the duration tracking used by the earlier stage; replace the direct addEvent call with the shared helper recordStageEvent(span, 'checkEntryAuthorities.done') so the stage duration is recorded, then keep the span.setAttribute('auth.authorities.result', 'granted') unchanged; ensure you reference the same SPAN_KEY extraction (flowCtx.state?.[SPAN_KEY]) and that recordStageEvent is imported/available in the module.libs/sdk/src/common/metadata/front-mcp.metadata.ts (1)
436-452: Schema usesz.any()for complex resolver/evaluator types.The use of
z.any()forrelationshipResolver,evaluators,claimsResolver, andpipesis pragmatic since these are typically functions or complex objects that can't be structurally validated by Zod. Consider adding a comment explaining this is intentional structural-only validation at the metadata level, with deeper validation deferred to@frontmcp/auth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/metadata/front-mcp.metadata.ts` around lines 436 - 452, The authorities schema currently uses z.any() for relationshipResolver, evaluators, claimsResolver, and pipes which is intentional because these are complex functions/objects validated elsewhere; update the authorities zod object (the authorities definition surrounding relationshipResolver, evaluators, claimsResolver, pipes) to include a concise inline comment explaining that z.any() is used deliberately to perform structural-only metadata validation here and that runtime or deeper validation is deferred to `@frontmcp/auth`; keep the comment short and colocated with those properties so future readers understand the design choice.libs/observability/src/plugin/observability.plugin.ts (1)
337-345: Hook callbacks reuse tool-named functions across entry types.The Resource, Prompt, and Agent
checkEntryAuthoritieshooks all callonToolWillCheckEntryAuthoritiesandonToolDidCheckEntryAuthorities. This is functional but the naming could be misleading in traces. Consider renaming toonEntryWillCheckAuthorities/onEntryDidCheckAuthoritiesfor clarity, or this is acceptable if the span names are generic.Also applies to: 376-384, 415-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/observability/src/plugin/observability.plugin.ts` around lines 337 - 345, The hook callbacks for Resource (methods _resourceWillCheckAuthorities and _resourceDidCheckAuthorities) reuse tool-specific functions onToolWillCheckEntryAuthorities/onToolDidCheckEntryAuthorities which makes trace/span names misleading; rename those helpers to more generic names (e.g., onEntryWillCheckAuthorities and onEntryDidCheckAuthorities) or add small wrapper functions that call the existing tool helpers but emit generic span names, then update all hook callers (_resourceWillCheckAuthorities, _resourceDidCheckAuthorities and the analogous Prompt/Agent hook methods that currently call onToolWillCheckEntryAuthorities/onToolDidCheckEntryAuthorities) to use the new generic helper/wrapper and keep the this.tracingEnabled checks intact.libs/sdk/src/resource/flows/resources-list.flow.ts (1)
193-194: Consider extracting authInfo access into a helper.The nested optional chaining with type casts (
(this.rawInput as Record<string, unknown>)['ctx'] as Record<string, unknown> | undefined) is repeated across multiple list flows. A shared helper method onFlowBaseor a utility function would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/resource/flows/resources-list.flow.ts` around lines 193 - 194, Extract the repeated extraction logic into a single helper (e.g., add a protected method getAuthInfo(): Record<string, unknown> on FlowBase or a small utility function) that safely reads this.rawInput as Record<string, unknown>, reads the 'ctx' key, and returns ctx?.['authInfo'] as Record<string, unknown> (or {}). Replace the inline expression in resources-list.flow.ts (the const ctx / const authInfo lines) and other list flows with a call to FlowBase.getAuthInfo() to remove duplicate optional chaining and type casts and centralize the null/typing handling.apps/e2e/demo-e2e-authorities/e2e/authorities-public.e2e.spec.ts (1)
158-168: Consider verifying the JSON-RPC error code for authority denials.Per the referenced documentation, authority denials should serialize as MCP JSON-RPC with error code
-32003. The current regex check validates the message content but doesn't verify the error code structure, which would strengthen the test's alignment with the documented contract.💡 Suggested enhancement
describe('Error content verification', () => { it('should return authority denial message (not generic error)', async () => { const client = await connectAnonymous(); const result = await client.tools.call('admin-only', { action: 'test' }); expect(result).toBeError(); const text = JSON.stringify(result); // Error should mention authority/access denial, not a generic server error expect(text.toLowerCase()).toMatch(/denied|authority|roles|forbidden/i); + // Verify JSON-RPC error code per MCP protocol + expect(result.error?.code).toBe(-32003); await client.disconnect(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/e2e/authorities-public.e2e.spec.ts` around lines 158 - 168, The test only asserts the error message text but must also verify the MCP JSON-RPC error code for authority denials; update the test in the 'Error content verification' suite (the it block that calls connectAnonymous() and client.tools.call('admin-only', { action: 'test' })) to parse the RPC error object from result and assert that the error.code equals -32003 in addition to the existing message regex check so the test enforces the documented authority-denied JSON-RPC contract.libs/sdk/src/common/utils/__tests__/path.utils.spec.ts (1)
53-93: Consider adding a test for scheme mismatch.The
resourceUriMatchestests thoroughly cover port, case, path, and host differences, but don't explicitly test that different schemes (http://vshttps://) are treated as non-matching. This would ensure the function doesn't normalize away security-significant differences.💡 Suggested test case
it('should not match different hosts', () => { expect(resourceUriMatches('https://evil.com/mcp', 'https://api.example.com/mcp')).toBe(false); }); + + it('should not match different schemes', () => { + expect(resourceUriMatches('http://api.example.com/mcp', 'https://api.example.com/mcp')).toBe(false); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/utils/__tests__/path.utils.spec.ts` around lines 53 - 93, Add a test to assert that resourceUriMatches treats different schemes as non-matching: call resourceUriMatches with an http:// URL and the equivalent https:// URL (e.g., 'http://api.example.com/mcp' vs 'https://api.example.com/mcp') and expect false; locate the new test alongside the existing describe('resourceUriMatches') cases referencing the resourceUriMatches function and follow the same style as the other assertions (expect(...).toBe(false)).libs/sdk/src/agent/flows/call-agent.flow.ts (2)
245-253: Consider hoisting dynamic imports to module level for performance.The dynamic
import('@frontmcp/auth')calls on Lines 245 and 253 execute on every authority check that results in a denial or needs scope resolution. Converting these to static imports at the module level would avoid the repeated resolution overhead.💡 Suggested refactor
Add at the top of the file with other imports:
import { AuthorityDeniedError, resolveRequiredScopes, type AuthoritiesMetadata } from '@frontmcp/auth';Then simplify the stage:
if (!result.granted) { let requiredScopes: string[] | undefined; const scopeMapping = this.scope.authoritiesScopeMapping; if (scopeMapping && result.denial) { - const { resolveRequiredScopes } = await import('@frontmcp/auth'); requiredScopes = resolveRequiredScopes( result.denial, scopeMapping, - authorities as import('@frontmcp/auth').AuthoritiesMetadata, + authorities as AuthoritiesMetadata, ); } - const { AuthorityDeniedError } = await import('@frontmcp/auth'); throw new AuthorityDeniedError({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/agent/flows/call-agent.flow.ts` around lines 245 - 253, Hoist the dynamic imports from inside the authority-check block to static module-level imports: add "import { AuthorityDeniedError, resolveRequiredScopes, type AuthoritiesMetadata } from '@frontmcp/auth';" at the top of the module and then replace the inline "await import('@frontmcp/auth')" usage in the functions by directly calling resolveRequiredScopes(...) and referencing AuthorityDeniedError and AuthoritiesMetadata; remove the two await import calls and ensure the existing calls that passed "authorities as import('@frontmcp/auth').AuthoritiesMetadata" now use the AuthoritiesMetadata type.
227-228: Type safety concern with metadata access.The double cast
metadata as unknown as Record<string, unknown>and subsequent dynamic property access bypasses TypeScript's type checking. IfExtendFrontMcpAgentMetadatais augmented with theauthoritiesfield (as suggested for the augment file), this could use proper typing.- const metadata = agent.metadata as unknown as Record<string, unknown>; - const authorities = metadata['authorities']; + const authorities = agent.metadata.authorities;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/agent/flows/call-agent.flow.ts` around lines 227 - 228, The code is unsafely casting agent.metadata via "as unknown as Record<string, unknown>" and then accessing metadata['authorities']; replace this with a proper typed metadata shape: import or reference the augmented interface ExtendFrontMcpAgentMetadata and type agent.metadata as ExtendFrontMcpAgentMetadata (or narrow it with a type guard) so you can access metadata.authorities with its declared type; update usages of metadata and authorities (the variables agent.metadata, metadata, authorities) to use that typed property and remove the double-cast.libs/auth/src/authorities/authorities.profiles.ts (1)
53-76: Orphaned JSDoc comment block at lines 53-70.The JSDoc block at lines 53-70 appears to be documentation for
AuthoritiesConfig, but it's immediately followed by another JSDoc block (lines 71-76) forAuthoritiesScopeMapping. The first block is orphaned and will not be associated with any declaration.Consider consolidating or repositioning:
♻️ Suggested fix
-/** - * Server/app-level authorities configuration. - * Registered in `@FrontMcp({ authorities: { ... } })` or `@App({ authorities: { ... } })`. - * - * `@example` - * ```typescript - * `@FrontMcp`({ - * authorities: { - * claimsMapping: { roles: 'realm_access.roles', permissions: 'scope' }, - * profiles: { - * admin: { roles: { any: ['admin', 'superadmin'] } }, - * authenticated: { attributes: { conditions: [{ path: 'user.sub', op: 'exists', value: true }] } }, - * matchTenant: { attributes: { conditions: [{ path: 'claims.org_id', op: 'eq', value: { fromInput: 'tenantId' } }] } }, - * }, - * }, - * }) - * ``` - */ /** * Maps authority denials to OAuth scope challenges. * When an authority check fails and the denial matches a scopeMapping key, * the required scopes are included in the 403 insufficient_scope response. * Explicit only — no automatic permission-to-scope inference. */ export interface AuthoritiesScopeMapping {Then move the server/app-level JSDoc to line 85 (above
AuthoritiesConfig):} +/** + * Server/app-level authorities configuration. + * Registered in `@FrontMcp({ authorities: { ... } })` or `@App({ authorities: { ... } })`. + * + * `@example` + * ```typescript + * `@FrontMcp`({ + * authorities: { + * claimsMapping: { roles: 'realm_access.roles', permissions: 'scope' }, + * profiles: { + * admin: { roles: { any: ['admin', 'superadmin'] } }, + * authenticated: { attributes: { conditions: [{ path: 'user.sub', op: 'exists', value: true }] } }, + * matchTenant: { attributes: { conditions: [{ path: 'claims.org_id', op: 'eq', value: { fromInput: 'tenantId' } }] } }, + * }, + * }, + * }) + * ``` + */ export interface AuthoritiesConfig {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.profiles.ts` around lines 53 - 76, The JSDoc example block is orphaned and not attached to AuthoritiesConfig; move or consolidate it so the example documents the correct interface. Specifically, relocate the server/app-level JSDoc (the `@FrontMcp` example) so it appears immediately above the AuthoritiesConfig declaration (reference: AuthoritiesConfig) and ensure the following JSDoc before AuthoritiesScopeMapping remains only for AuthoritiesScopeMapping (reference: AuthoritiesScopeMapping); remove the duplicate/orphaned block to avoid two adjacent doc comments.apps/e2e/demo-e2e-authorities/src/main.ts (1)
6-7: Consider using nullish coalescing (??) for environment variables.Lines 6-7 use
||which treats empty strings as falsy, defaulting to the fallback. If empty string should be a valid value (e.g., explicitly clearing the env var), use??instead:-const idpProviderUrl = process.env['IDP_PROVIDER_URL'] || 'https://sample-app.frontegg.com'; -const expectedAudience = process.env['IDP_EXPECTED_AUDIENCE'] || idpProviderUrl; +const idpProviderUrl = process.env['IDP_PROVIDER_URL'] ?? 'https://sample-app.frontegg.com'; +const expectedAudience = process.env['IDP_EXPECTED_AUDIENCE'] ?? idpProviderUrl;However, if treating empty strings as "not set" is intentional (common for env vars), the current behavior is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/src/main.ts` around lines 6 - 7, The code uses || to default env vars for idpProviderUrl and expectedAudience which treats empty strings as unset; change the fallback logic to use the nullish coalescing operator (??) for idpProviderUrl and expectedAudience so only null/undefined trigger the default, i.e., update the assignments for idpProviderUrl and expectedAudience to use ?? instead of ||.libs/sdk/src/scope/__tests__/validate-authorities-config.spec.ts (1)
151-160: Consider replacingfail()with Jest's standard assertion pattern.
fail()is not a standard Jest global and may cause issues depending on Jest configuration. Consider restructuring usingexpect().toThrow()or the inverse pattern:♻️ Suggested refactor
- it('should not truncate when exactly 5 entries have authorities', () => { + it('should not truncate when exactly 5 entries have authorities', () => { const tools = Array.from({ length: 5 }, (_, i) => ({ name: `tool-${i}`, metadata: { name: `tool-${i}`, authorities: 'admin' }, })); const scope = createMockScope({ scopeTools: { getTools: () => tools }, }); - try { - validateAuthoritiesConfig.call(scope); - fail('Expected an error to be thrown'); - } catch (err) { - const message = (err as Error).message; - expect(message).toContain('Tool "tool-0"'); - expect(message).toContain('Tool "tool-4"'); - expect(message).not.toContain('more'); - } + expect(() => validateAuthoritiesConfig.call(scope)).toThrow( + expect.objectContaining({ + message: expect.stringContaining('Tool "tool-0"'), + }), + ); + expect(() => validateAuthoritiesConfig.call(scope)).toThrow( + expect.objectContaining({ + message: expect.stringContaining('Tool "tool-4"'), + }), + ); + expect(() => validateAuthoritiesConfig.call(scope)).toThrow( + expect.objectContaining({ + message: expect.not.stringContaining('more'), + }), + ); });Or use a single call with multiple assertions:
it('should not truncate when exactly 5 entries have authorities', () => { const tools = Array.from({ length: 5 }, (_, i) => ({ name: `tool-${i}`, metadata: { name: `tool-${i}`, authorities: 'admin' }, })); const scope = createMockScope({ scopeTools: { getTools: () => tools }, }); let thrownError: Error | undefined; try { validateAuthoritiesConfig.call(scope); } catch (err) { thrownError = err as Error; } expect(thrownError).toBeDefined(); expect(thrownError?.message).toContain('Tool "tool-0"'); expect(thrownError?.message).toContain('Tool "tool-4"'); expect(thrownError?.message).not.toContain('more'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/scope/__tests__/validate-authorities-config.spec.ts` around lines 151 - 160, The test uses non-standard fail() which can break under some Jest configs; change the pattern in validate-authorities-config.spec.ts so you capture the thrown Error from calling validateAuthoritiesConfig (or use expect(() => validateAuthoritiesConfig.call(scope)).toThrow()) and then assert on thrownError.message contains 'Tool "tool-0"' and 'Tool "tool-4"' and does not contain 'more'; update the test around validateAuthoritiesConfig.call(scope) to either use expect().toThrow() or assign the caught error to a variable (e.g., thrownError) and run the three expect assertions against thrownError.message.libs/auth/src/authorities/authorities.errors.ts (1)
75-87: Consider omitting undefined fields from JSON-RPC error data.The
toJsonRpcError()method includesdenialandrequiredScopeseven when undefined. Consider omitting them to produce cleaner JSON-RPC responses.♻️ Conditional field inclusion
toJsonRpcError(): { code: number; message: string; data?: Record<string, unknown> } { + const data: Record<string, unknown> = { + entryType: this.entryType, + entryName: this.entryName, + deniedBy: this.deniedBy, + }; + if (this.denial !== undefined) data.denial = this.denial; + if (this.requiredScopes !== undefined) data.requiredScopes = this.requiredScopes; return { code: this.mcpErrorCode, message: this.message, - data: { - entryType: this.entryType, - entryName: this.entryName, - deniedBy: this.deniedBy, - denial: this.denial, - requiredScopes: this.requiredScopes, - }, + data, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.errors.ts` around lines 75 - 87, The toJsonRpcError() method currently includes denial and requiredScopes in the data object even when they are undefined; update toJsonRpcError (in the Authorities error class) to only add denial and requiredScopes to the returned data when they are not undefined (e.g., construct the data object and conditionally assign data.denial = this.denial and data.requiredScopes = this.requiredScopes if they are !== undefined) so the JSON-RPC payload omits those fields when absent.libs/sdk/src/scope/scope.instance.ts (1)
999-1038: Consider extracting authorities initialization to a helper file.Per project conventions,
scope.instance.tsshould stay lean by extracting feature-specific registration logic to separate helpers (similar toregisterSkillCapabilitiesandregisterJobCapabilities). TheinitAuthoritiesFromConfigmethod adds ~40 lines of authorities-specific setup that could be extracted to anauthorities-scope.helper.tsfile inlibs/sdk/src/auth/or similar.♻️ Suggested extraction pattern
// libs/sdk/src/auth/authorities-scope.helper.ts export function initAuthoritiesFromConfig( metadata: Record<string, unknown>, logger: FrontMcpLogger, ): { engine?: AuthoritiesEngine; contextBuilder?: AuthoritiesContextBuilder; scopeMapping?: AuthoritiesScopeMapping; } | undefined { const config = metadata['authorities'] as Record<string, unknown> | undefined; if (!config) return undefined; // ... existing logic }Then in
scope.instance.ts:const authResult = initAuthoritiesFromConfig(this.metadata, this.logger); if (authResult) { this._authoritiesEngine = authResult.engine; this._authoritiesContextBuilder = authResult.contextBuilder; this._authoritiesScopeMapping = authResult.scopeMapping; }Based on learnings: "Keep scope.instance.ts lean by using helper functions from skill module for feature-specific registration logic; extract complex logic to separate helper files"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/scope/scope.instance.ts` around lines 999 - 1038, The initAuthoritiesFromConfig method in scope.instance.ts contains authorities-specific setup and should be extracted to a helper to keep the file lean: create libs/sdk/src/auth/authorities-scope.helper.ts that exports an initAuthoritiesFromConfig(metadata: Record<string, unknown>, logger: FrontMcpLogger) function which performs the require('@frontmcp/auth') logic, builds AuthoritiesProfileRegistry, AuthoritiesEvaluatorRegistry, AuthoritiesEngine, AuthoritiesContextBuilder and returns an object with engine, contextBuilder, and scopeMapping (or undefined if no config); then replace the body of the existing initAuthoritiesFromConfig to call that helper and assign this._authoritiesEngine, this._authoritiesContextBuilder, and this._authoritiesScopeMapping from the helper result (mirroring the pattern used by registerSkillCapabilities and registerJobCapabilities).libs/auth/src/context/frontmcp-auth-context.factory.ts (1)
76-81: Consider accepting an optional logger instead of usingconsole.warn.Using
console.warndirectly in library code makes it harder for consumers to control log output or integrate with their logging infrastructure. Consider accepting an optional logger parameter or using a no-op by default.♻️ Optional logger injection
export function buildAuthContext( authInfo: AuthContextSourceInfo, claimsMapping?: AuthoritiesClaimsMapping, + options?: { logger?: { warn: (msg: string) => void } }, ): FrontMcpAuthContext; export function buildAuthContext( authInfo: AuthContextSourceInfo, claimsMapping: AuthoritiesClaimsMapping | undefined, pipes: AuthContextPipe[], + options?: { logger?: { warn: (msg: string) => void } }, ): Promise<FrontMcpAuthContext>;Then in
runPipes:- console.warn(`[FrontMcpAuth] pipe failed: ${message}`); + options?.logger?.warn(`[FrontMcpAuth] pipe failed: ${message}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/context/frontmcp-auth-context.factory.ts` around lines 76 - 81, Replace the direct console.warn in the runPipes catch block with an injectable logger: add an optional logger parameter (or options object) to the frontMcp auth context factory/runner (e.g., the createFrontMcpAuthContextFactory or runPipes function), defaulting to a no-op logger when none is provided, then call logger.warn(...) (or logger.error(...)) instead of console.warn and include the original error message; ensure all call sites pass the optional logger or rely on the default no-op.libs/auth/src/authorities/authorities.types.ts (1)
155-157: Consider supporting synchronous guards.
AuthorityGuardFnonly supports async returns (Promise<boolean | string>). For simple in-memory checks, supporting sync returns could improve performance and developer ergonomics.export type AuthorityGuardFn = ( ctx: AuthoritiesEvaluationContext, ) => boolean | string | Promise<boolean | string>;However, the engine's
evaluateGuardsawaits each guard, so this would work without changes there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.types.ts` around lines 155 - 157, Update the AuthorityGuardFn type to also accept synchronous returns by changing its return union to boolean | string | Promise<boolean | string>; modify the type definition in authorities.types.ts (AuthorityGuardFn) and ensure any references (e.g., evaluateGuards) continue to await the result so both sync and async guard implementations work transparently.libs/auth/src/authorities/authorities.context.ts (2)
163-169: Redundant type assertion onsub.The
subvariable is already typed asstringfrom the ternary expression. Theas stringassertion on line 169 is unnecessary.♻️ Remove redundant assertion
return { user: { - sub: sub as string, + sub, roles, permissions, claims: rawClaims, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.context.ts` around lines 163 - 169, The local variable sub (computed from this.claimsMapping?.userId ? String(resolveDotPath(rawClaims, this.claimsMapping.userId) ?? '') : (user.sub ?? '')) is already a string, so remove the redundant type assertion on return.user.sub; update the return to use sub directly (no "as string") and ensure no other code relies on that unnecessary cast in authorities.context.ts around the claimsMapping/resolveDotPath logic.
144-150: Fallback fromauthorization.scopesto roles may cause semantic confusion.Using
scopesas a fallback forrolesconflates two different authorization concepts. OAuth scopes typically represent delegated permissions, not user roles. This could lead to unexpected authorization behavior.Consider whether this fallback is intentional or if it should be removed to enforce explicit role configuration.
💡 Consider removing scopes-to-roles fallback
} else { roles = toStringArray( - (user as Record<string, unknown>)['roles'] ?? - (authorization as Record<string, unknown> | undefined)?.['scopes'] ?? - [], + (user as Record<string, unknown>)['roles'] ?? [], ); }If the fallback is intentional for specific IdP compatibility, document the reasoning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.context.ts` around lines 144 - 150, The current fallback that assigns roles from authorization.scopes conflates scopes with roles and should be removed or explicitly documented; update the roles assignment in authorities.context.ts (the block that sets the roles variable and calls toStringArray) to stop reading (authorization as Record<string, unknown>)?.['scopes'] as a fallback—use only (user as Record<string, unknown>)['roles'] (and default []), or if this fallback is required for specific IdP compatibility, keep the code but add a clear comment above the roles assignment explaining the intentional scopes-to-roles mapping and add a feature flag or config toggle to make the behavior explicit.libs/auth/src/authorities/authorities.evaluator.ts (1)
169-171: Silent failure on unknown operator could mask policy misconfigurations.When an unrecognized operator is used, the function silently returns
false. This makes debugging policy issues difficult since the denial reason won't indicate the operator was invalid.Consider logging a warning or including the unknown operator in the denial metadata.
♻️ Option: Track unknown operator for debugging
default: + // Unknown operators fail closed but could indicate misconfiguration return false;Alternatively, throw during policy validation at configuration time rather than at evaluation time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/auth/src/authorities/authorities.evaluator.ts` around lines 169 - 171, The switch in the evaluator that currently falls through to "default: return false;" silently hides unknown operators; update the evaluator to surface the operator name when it is unrecognized by either (a) emitting a warning via the existing logger and returning a denial object that includes unknownOperator/operatorName in its metadata, or (b) if there is a policy-validation phase, validate operators in configure/validate time and throw a clear error there. Locate the operator handling switch in authorities.evaluator.ts (the function that returns booleans/deny decisions) and replace the silent default with a logged warning and enriched denial metadata (or move to a validation throw) so unknown operators are visible during debugging.apps/e2e/demo-e2e-authorities/e2e/authorities.e2e.spec.ts (2)
22-52: OAuth server restart pattern works but is fragile.The pattern of starting, stopping, and recreating the OAuth server to obtain a stable port is a workaround. Consider using
port: 0to get a random available port directly, or passing the port through a dedicated method if MockOAuthServer supports it.That said, the current implementation does work and ensures consistent issuer URLs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/e2e/authorities.e2e.spec.ts` around lines 22 - 52, The current beforeAll uses a fragile start/stop/recreate pattern to capture a stable port from MockOAuthServer; instead modify the setup to let MockOAuthServer bind an ephemeral port (pass port: 0) or use a MockOAuthServer API that returns its bound port without restarting, then create the final TestTokenFactory with the returned oauthInfo.issuer and start TestServer with IDP_PROVIDER_URL set to oauthInfo.baseUrl and IDP_EXPECTED_AUDIENCE set to oauthInfo.issuer, removing the intermediate start/stop and the second instantiation of MockOAuthServer (referencing beforeAll, MockOAuthServer, tokenFactory, oauthInfo, finalOauthInfo, and TestServer.start to locate the code).
379-412: Error structure tests could verify MCP-specific error codes.Per the documentation in
authorities.mdx, denial errors should serialize as MCP JSON-RPC with error code-32003. The current tests only check for keyword presence in the stringified response.Consider adding explicit verification of the error code for compliance validation.
💡 Add explicit error code verification
it('should return MCP error code -32003 for authority denial', async () => { const client = await connectWithClaims({ sub: 'viewer', roles: ['viewer'], }); const result = await client.tools.call('admin-only', { action: 'test' }); expect(result).toBeError(); // Verify MCP JSON-RPC error code per authorities.mdx expect(result.error?.code).toBe(-32003); await client.disconnect(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-authorities/e2e/authorities.e2e.spec.ts` around lines 379 - 412, Add explicit assertions that denial responses include the MCP JSON-RPC error code -32003: after calling connectWithClaims and performing the denied action via client.tools.call('admin-only', ...), client.tools.call('permissions-required', ...), and client.resources.read('config://admin-settings'), keep the existing expect(result).toBeError() checks and then assert the structured error code (e.g., expect(result.error?.code).toBe(-32003)) on the returned result object; update the existing three tests (references: connectWithClaims, client.tools.call, client.resources.read, and result) to include this code assertion so the tests validate MCP-specific denial codes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c3f7075-86e8-465c-be43-27bd29185647
📒 Files selected for processing (83)
apps/e2e/demo-e2e-authorities/e2e/authorities-public.e2e.spec.tsapps/e2e/demo-e2e-authorities/e2e/authorities.e2e.spec.tsapps/e2e/demo-e2e-authorities/jest.e2e.config.tsapps/e2e/demo-e2e-authorities/project.jsonapps/e2e/demo-e2e-authorities/src/apps/authorities/index.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/resources/admin-config.resource.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/resources/public-info.resource.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/admin-only.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/combinator.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/editor-or-admin.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/permissions.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/profile-admin.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/profile-multi.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/public.tool.tsapps/e2e/demo-e2e-authorities/src/apps/authorities/tools/tenant-scoped.tool.tsapps/e2e/demo-e2e-authorities/src/main.tsapps/e2e/demo-e2e-authorities/tsconfig.app.jsonapps/e2e/demo-e2e-authorities/tsconfig.jsondocs/frontmcp/authentication/architecture.mdxdocs/frontmcp/authentication/authorities.mdxlibs/auth/src/authorities/__tests__/authorities.context.spec.tslibs/auth/src/authorities/__tests__/authorities.engine.spec.tslibs/auth/src/authorities/__tests__/authorities.errors.spec.tslibs/auth/src/authorities/__tests__/authorities.evaluator.spec.tslibs/auth/src/authorities/__tests__/authorities.guards.spec.tslibs/auth/src/authorities/__tests__/authorities.registry.spec.tslibs/auth/src/authorities/__tests__/authorities.schema.spec.tslibs/auth/src/authorities/__tests__/authorities.scope-mapping.spec.tslibs/auth/src/authorities/authorities.context.tslibs/auth/src/authorities/authorities.engine.tslibs/auth/src/authorities/authorities.errors.tslibs/auth/src/authorities/authorities.evaluator.tslibs/auth/src/authorities/authorities.metadata-augment.tslibs/auth/src/authorities/authorities.profiles.tslibs/auth/src/authorities/authorities.registry.tslibs/auth/src/authorities/authorities.schema.tslibs/auth/src/authorities/authorities.scope-mapping.tslibs/auth/src/authorities/authorities.types.tslibs/auth/src/authorities/index.tslibs/auth/src/context/__tests__/frontmcp-auth-context.spec.tslibs/auth/src/context/frontmcp-auth-context.factory.tslibs/auth/src/context/frontmcp-auth-context.impl.tslibs/auth/src/context/frontmcp-auth-context.tslibs/auth/src/context/index.tslibs/auth/src/fetch/__tests__/fetch-credential-middleware.spec.tslibs/auth/src/fetch/fetch-credential-middleware.tslibs/auth/src/fetch/index.tslibs/auth/src/index.tslibs/observability/src/plugin/observability.hooks.tslibs/observability/src/plugin/observability.plugin.tslibs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/auth/flows/auth.verify.flow.tslibs/sdk/src/auth/flows/oauth.authorize.flow.tslibs/sdk/src/auth/flows/oauth.token.flow.tslibs/sdk/src/auth/flows/session.verify.flow.tslibs/sdk/src/auth/flows/well-known.prm.flow.tslibs/sdk/src/builtin/authorities/authorities.plugin.options.tslibs/sdk/src/builtin/authorities/authorities.plugin.tslibs/sdk/src/builtin/authorities/index.tslibs/sdk/src/common/entries/scope.entry.tslibs/sdk/src/common/interfaces/execution-context.interface.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/schemas/http-output.schema.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/utils/__tests__/path.utils.spec.tslibs/sdk/src/common/utils/path.utils.tslibs/sdk/src/context/frontmcp-context.tslibs/sdk/src/prompt/flows/get-prompt.flow.tslibs/sdk/src/prompt/flows/prompts-list.flow.tslibs/sdk/src/resource/flows/read-resource.flow.tslibs/sdk/src/resource/flows/resource-templates-list.flow.tslibs/sdk/src/resource/flows/resources-list.flow.tslibs/sdk/src/scope/__tests__/validate-authorities-config.spec.tslibs/sdk/src/scope/flows/http.request.flow.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/skills/catalog/frontmcp-authorities/SKILL.mdlibs/skills/catalog/frontmcp-authorities/references/authority-profiles.mdlibs/skills/catalog/frontmcp-authorities/references/claims-mapping.mdlibs/skills/catalog/frontmcp-authorities/references/custom-evaluators.mdlibs/skills/catalog/frontmcp-authorities/references/rbac-abac-rebac.mdlibs/skills/catalog/skills-manifest.json
Comment on lines
+84
to
+97
| inputSchema: z.object({ org: z.string() }), | ||
| authProviders: [{ name: 'github', scopes: ['repo'] }], | ||
| authorities: 'authenticated', | ||
| }) | ||
| class ListReposTool extends ToolContext<typeof inputSchema> { | ||
| async execute(input: { org: string }) { | ||
| // Explicitly request credentials for the 'github' provider | ||
| // this.fetch() resolves the token from the vault and injects the Authorization header | ||
| const res = await this.fetch(`https://api.github.com/orgs/${input.org}/repos`, { | ||
| credentials: { provider: 'github' }, | ||
| }); | ||
| return { repos: await res.json() }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Undefined inputSchema reference in type parameter.
Line 88 uses typeof inputSchema in the generic parameter, but inputSchema is defined inline at line 84, not as a separate variable.
📝 Suggested fix
+const listReposSchema = z.object({ org: z.string() });
+
`@Tool`({
name: 'list_repos',
description: 'List GitHub repositories',
- inputSchema: z.object({ org: z.string() }),
+ inputSchema: listReposSchema,
authProviders: [{ name: 'github', scopes: ['repo'] }],
authorities: 'authenticated',
})
-class ListReposTool extends ToolContext<typeof inputSchema> {
+class ListReposTool extends ToolContext<typeof listReposSchema> {
async execute(input: { org: string }) {Or simplify by removing the generic and using the inferred type:
class ListReposTool extends ToolContext {
async execute(input: { org: string }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/frontmcp/authentication/architecture.mdx` around lines 84 - 97, The
generic type parameter uses typeof inputSchema which is not a named variable
(inputSchema is defined inline), so update the code to either extract the schema
into a separate constant (e.g., const inputSchema = z.object({ org: z.string()
}) and then keep class ListReposTool extends ToolContext<typeof inputSchema>) or
remove the generic and let TypeScript infer the input type (change class
ListReposTool extends ToolContext and keep async execute(input: { org: string
})). Ensure you modify the declaration around inputSchema and the class header
for ListReposTool and keep the execute method signature consistent with the
chosen approach.
Comment on lines
+534
to
+540
| @Tool({ name: 'calculator' }) | ||
| class CalculatorTool extends ToolContext { | ||
| async execute(input: { expression: string }) { | ||
| return { result: eval(input.expression) }; | ||
| } | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Avoid eval() in documentation examples — demonstrates security anti-pattern.
Using eval() in example code, even in documentation, normalizes a dangerous practice. Readers may copy-paste this pattern.
📝 Suggested safer alternative
`@Tool`({ name: 'calculator' })
class CalculatorTool extends ToolContext {
async execute(input: { expression: string }) {
- return { result: eval(input.expression) };
+ // Example: parse and evaluate safely (do not use eval in production)
+ return { result: parseFloat(input.expression) }; // Simplified for demo
}
}Or use a safe math expression parser library reference:
import { evaluate } from 'mathjs';
return { result: evaluate(input.expression) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/frontmcp/authentication/architecture.mdx` around lines 534 - 540, The
example uses eval() inside CalculatorTool.execute (extending ToolContext), which
is unsafe; replace it with a safe expression evaluator such as mathjs: import
and call evaluate(input.expression) (or another vetted math parser) and return
its result instead of using eval, ensuring any required error
handling/validation is added around the evaluate call.
Comment on lines
+82
to
+89
| authorities: { | ||
| claimsMapping: { | ||
| roles: 'https://myapp.com/roles', | ||
| permissions: 'permissions', | ||
| tenantId: 'org_id', | ||
| }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Syntax error in code example.
Extra closing parenthesis after the object literal on line 88 (}) should just be }).
authorities: {
claimsMapping: {
roles: 'https://myapp.com/roles',
permissions: 'permissions',
tenantId: 'org_id',
},
- })
+ },
+ })The same issue appears in the Keycloak (line 102), Okta (line 116), Cognito (line 130), and Frontegg (line 145) tabs.
📝 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.
Suggested change
| authorities: { | |
| claimsMapping: { | |
| roles: 'https://myapp.com/roles', | |
| permissions: 'permissions', | |
| tenantId: 'org_id', | |
| }, | |
| }) | |
| ``` | |
| authorities: { | |
| claimsMapping: { | |
| roles: 'https://myapp.com/roles', | |
| permissions: 'permissions', | |
| tenantId: 'org_id', | |
| }, | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/frontmcp/authentication/authorities.mdx` around lines 82 - 89, The code
examples for the authorities configuration contain an extra closing parenthesis
after the object literal (e.g., the authorities: { claimsMapping: { ... }, }) —
remove the stray ')' so the object literal ends with '}' only; update the same
pattern inside the Keycloak, Okta, Cognito, and Frontegg examples (look for the
authorities and claimsMapping examples) to remove the extra closing parenthesis
from each snippet.
Comment on lines
+162
to
+168
| case 'matches': | ||
| if (typeof actual !== 'string' || typeof expected !== 'string') return false; | ||
| try { | ||
| return new RegExp(expected).test(actual); | ||
| } catch { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are best practices for preventing ReDoS attacks in Node.js when using dynamic regex patterns?
💡 Result:
Best practices (Node.js + dynamic regex)
-
Avoid compiling regex from untrusted input (regex injection)
Don’t let users supply full patterns. Prefer allow-lists (e.g., pick from known-safe patterns) or build patterns from constrained tokens. ReDoS happens when a crafted input triggers catastrophic backtracking in common regex engines. (owasp.org) -
If you must interpolate user text, escape it (treat as a literal)
UseRegExp.escape()(where available) or a well-known escape helper so user input can’t introduce operators like.*, groups, or backreferences. (developer.mozilla.org) -
Design patterns to avoid backtracking traps
- Avoid nested quantifiers (e.g.,
(a+)+,(.*)*) and ambiguous alternations under repetition. - Prefer anchoring (
^...$) and tighter character classes over.*when possible.
(These reduce or eliminate super-linear backtracking cases exploited in ReDoS.) (owasp.org)
- Avoid nested quantifiers (e.g.,
-
Put hard bounds on input size before regexing
Even “okay” regexes can become expensive on huge inputs—cap lengths, reject unexpectedly large payloads, and validate early. (This is one of the most effective practical mitigations.) (owasp.org) -
Use a non-backtracking engine for high-risk cases (RE2)
If you must run potentially complex/dynamic patterns, consider switching evaluation to RE2 via there2package (linear-time matching; avoids catastrophic backtracking by design). (npmjs.com) -
Detect risky regexes in CI
- Lint for likely super-linear backtracking (e.g.,
eslint-plugin-regexp’sno-super-linear-backtracking). (ota-meshi.github.io) - Add automated checks/fuzzing for vulnerable patterns (e.g.,
redos-detector,vuln-regex-detector). (github.com)
- Lint for likely super-linear backtracking (e.g.,
-
Isolate/timeout worst-case work so it can’t block the event loop
JavaScript regex matching is synchronous; a bad match can stall a Node.js worker. For untrusted inputs/patterns, run regex work in Worker Threads / a worker pool and enforce timeouts/termination. (nodejs.org)
If you share a brief example of how you’re building the dynamic pattern (and what kind of inputs you expect), I can point out the specific ReDoS risks and safer rewrites.
Citations:
- 1: https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS?utm_source=openai
- 2: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/RegExp/escape?utm_source=openai
- 3: https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS?utm_source=openai
- 4: https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS?utm_source=openai
- 5: https://www.npmjs.com/package/re2?utm_source=openai
- 6: https://ota-meshi.github.io/eslint-plugin-regexp/rules/no-super-linear-backtracking.html?utm_source=openai
- 7: https://github.com/tjenkinson/redos-detector?utm_source=openai
- 8: https://nodejs.org/download/release/v11.10.1/docs/api/worker_threads.html?utm_source=openai
🏁 Script executed:
# First, examine the authorities evaluator file to understand context
fd -t f "authorities.evaluator.ts" --exec wc -l {} \;Repository: agentfront/frontmcp
Length of output: 119
🏁 Script executed:
# Read the evaluator file to see the full context and how `expected` is used
fd -t f "authorities.evaluator.ts" --exec head -200 {} \;Repository: agentfront/frontmcp
Length of output: 6185
🏁 Script executed:
# Search for where `applyOperator` and the `matches` operator are called
rg "applyOperator|'matches'|matches.*operator" libs/auth --type ts -B 2 -A 2Repository: agentfront/frontmcp
Length of output: 4275
🏁 Script executed:
# Check if there are any existing policy validation or input sanitization mechanisms
rg "policy.*validat|validat.*pattern|ReDoS|regex.*safe|dangerous.*pattern" libs/auth --type tsRepository: agentfront/frontmcp
Length of output: 594
🏁 Script executed:
# Look for where authority policies are parsed/loaded from config to understand the data source
rg "authorities.*config|policies.*load|expected.*pattern" libs/auth --type ts -B 3 -A 3Repository: agentfront/frontmcp
Length of output: 619
🏁 Script executed:
# Check the authorities schema to see if regex patterns are validated at the schema level
fd -t f "authorities.schema.ts" --exec head -250 {} \;Repository: agentfront/frontmcp
Length of output: 4725
🏁 Script executed:
# Look for where ABAC conditions are validated or loaded
rg "AbacCondition|AbacPolicy|conditions.*schema|value.*schema" libs/auth/src --type ts -B 2 -A 2Repository: agentfront/frontmcp
Length of output: 4289
🏁 Script executed:
# Check if there's any input validation/sanitization for the `value` field in conditions
rg "condition.*value|pattern.*validat|regex.*validat" libs/auth/src --type ts -B 3 -A 3Repository: agentfront/frontmcp
Length of output: 22319
🏁 Script executed:
# Look at how authorities policies are sourced/registered to understand if they're admin-controlled or user-supplied
rg "@FrontMcp|@App.*authorities|authorities.*config|policies.*register" libs --type ts -B 2 -A 5 | head -80Repository: agentfront/frontmcp
Length of output: 6048
🏁 Script executed:
# Check if there are any tests for ReDoS-like patterns or performance tests
rg "ReDoS|catastrophic|backtrack|performance.*regex|timeout.*regex" libs/auth --type tsRepository: agentfront/frontmcp
Length of output: 594
🏁 Script executed:
# Look at existing ReDoS safeguards in other parts of the codebase
fd -t f -name "*.ts" libs/auth/src/utils --exec grep -l "ReDoS\|character-by-character" {} \;Repository: agentfront/frontmcp
Length of output: 293
🏁 Script executed:
# Examine the full evaluateAbac function to see error handling and call context
sed -n '169,250p' libs/auth/src/authorities/authorities.evaluator.tsRepository: agentfront/frontmcp
Length of output: 2531
🏁 Script executed:
# Check if regex patterns can come from user input via fromInput or fromClaims
rg "fromInput.*matches|matches.*fromInput|op.*matches.*from" libs/auth --type ts -B 3 -A 3Repository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Search for examples of 'matches' operator usage in tests or documentation
rg -A 5 "op.*matches|'matches'" libs/auth/__tests__ --type ts | head -60Repository: agentfront/frontmcp
Length of output: 122
🏁 Script executed:
# Check the existing ReDoS mitigation pattern in www-authenticate.utils
cat libs/auth/src/utils/www-authenticate.utils.ts | head -100Repository: agentfront/frontmcp
Length of output: 2777
🏁 Script executed:
# Look at audience.validator to see how they prevent ReDoS
grep -A 10 "ReDoS protection" libs/auth/src/utils/audience.validator.tsRepository: agentfront/frontmcp
Length of output: 525
🏁 Script executed:
# Check how the regex pattern is typically used - from admin config or user input
rg "fromInput.*value|value.*fromInput" libs/auth/src --type ts -B 3 -A 3Repository: agentfront/frontmcp
Length of output: 3393
🏁 Script executed:
# Look at test cases to see if regex patterns ever come from input
fd authorities.evaluator.spec.ts --exec cat {} \; | grep -A 20 "matches"Repository: agentfront/frontmcp
Length of output: 1725
🏁 Script executed:
# Final verification: check if there are any tests using 'matches' with fromInput
rg "matches.*fromInput|fromInput.*matches" libs/auth --type tsRepository: agentfront/frontmcp
Length of output: 45
🏁 Script executed:
# Check if there's schema-level validation that could be enhanced
grep -A 10 "abacConditionSchema" libs/auth/src/authorities/authorities.schema.tsRepository: agentfront/frontmcp
Length of output: 752
ReDoS vulnerability: User-controlled regex patterns bypass catastrophic backtracking protection.
The matches operator at line 165 constructs regexes from the expected value without validation. Since value can resolve from fromInput via resolveValue(), untrusted request input can supply patterns like (a+)+$ that trigger catastrophic backtracking. The try-catch only guards against syntax errors, not ReDoS attacks.
Best practices recommend hard bounds on input size first, followed by pattern complexity checks (or preferring safe alternatives like escaping or RE2). The proposed mitigation—pattern length limit and dangerous pattern detection—provides practical defense-in-depth, though no single regex can catch all ReDoS variants. Alternatively, document that matches should not accept fromInput patterns, reserving regex for admin-configured policies only.
🛡️ Proposed layered mitigation
+// ReDoS mitigation: limit pattern size and detect dangerous constructs
+const MAX_REGEX_LENGTH = 256;
+const DANGEROUS_PATTERNS = /(\+\+|\*\*|\{\d+,\d*\}\{|\(\?[^)]*\)[\+\*])/;
+
+function isSafeRegexPattern(pattern: string): boolean {
+ if (pattern.length > MAX_REGEX_LENGTH) return false;
+ if (DANGEROUS_PATTERNS.test(pattern)) return false;
+ return true;
+}
+
case 'matches':
if (typeof actual !== 'string' || typeof expected !== 'string') return false;
+ if (!isSafeRegexPattern(expected)) return false;
try {
return new RegExp(expected).test(actual);
} catch {
return false;
}🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 164-164: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(expected)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/auth/src/authorities/authorities.evaluator.ts` around lines 162 - 168,
The 'matches' branch in the authorities evaluator currently builds a RegExp from
untrusted `expected` (resolved via resolveValue()), which risks ReDoS; to fix
it, validate and/or restrict patterns before constructing the RegExp in the
'matches' case: enforce a strict max pattern length (e.g., 100-500 chars),
reject patterns containing known problematic constructs (nested quantifiers like
(?:...)+, backreferences, catastrophic constructs such as (a+)+, excessive
repetition counts or lookbehinds), and/or require patterns only come from
trusted/admin sources (deny when `expected` originates from fromInput);
alternatively escape the pattern or switch to a safe engine (RE2) if available.
Update the 'matches' logic to return false for disallowed patterns and log/throw
a clear error for admin-reviewable patterns, referencing the 'matches' operator
and resolveValue() usage so reviewers can find and apply the validation.
Comment on lines
+10
to
+277
| describe('FetchCredentialMiddleware', () => { | ||
| const TEST_URL = 'https://api.github.com/user/repos'; | ||
| const TEST_TOKEN = 'gho_abc123_test_token'; | ||
|
|
||
| function createMockAccessor(tokens: Record<string, string | null> = {}): TokenAccessor { | ||
| return { | ||
| getToken: jest.fn(async (providerId: string) => tokens[providerId] ?? null), | ||
| }; | ||
| } | ||
|
|
||
| describe('provider specified with token available', () => { | ||
| it('should inject Authorization header with Bearer token', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| const headers = new Headers(result.headers); | ||
| expect(headers.get('Authorization')).toBe(`Bearer ${TEST_TOKEN}`); | ||
| expect(accessor.getToken).toHaveBeenCalledWith('github'); | ||
| }); | ||
|
|
||
| it('should remove the credentials field from the returned init', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'POST', | ||
| credentials: { provider: 'github' }, | ||
| body: JSON.stringify({ query: 'test' }), | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result.credentials).toBeUndefined(); | ||
| expect(result.method).toBe('POST'); | ||
| expect(result.body).toBe(JSON.stringify({ query: 'test' })); | ||
| }); | ||
|
|
||
| it('should resolve the correct provider token', async () => { | ||
| const accessor = createMockAccessor({ | ||
| github: 'github-token', | ||
| jira: 'jira-token', | ||
| slack: 'slack-token', | ||
| }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| credentials: { provider: 'jira' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials('https://jira.example.com/api', init); | ||
|
|
||
| const headers = new Headers(result.headers); | ||
| expect(headers.get('Authorization')).toBe('Bearer jira-token'); | ||
| expect(accessor.getToken).toHaveBeenCalledWith('jira'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('no credentials', () => { | ||
| it('should return init unchanged when credentials is undefined', async () => { | ||
| const accessor = createMockAccessor(); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result).toBe(init); | ||
| expect(accessor.getToken).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('standard string credentials', () => { | ||
| it('should pass through "include" unchanged', async () => { | ||
| const accessor = createMockAccessor(); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: 'include', | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result).toBe(init); | ||
| expect(result.credentials).toBe('include'); | ||
| expect(accessor.getToken).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should pass through "same-origin" unchanged', async () => { | ||
| const accessor = createMockAccessor(); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: 'same-origin', | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result).toBe(init); | ||
| expect(result.credentials).toBe('same-origin'); | ||
| expect(accessor.getToken).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should pass through "omit" unchanged', async () => { | ||
| const accessor = createMockAccessor(); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: 'omit', | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result).toBe(init); | ||
| expect(result.credentials).toBe('omit'); | ||
| expect(accessor.getToken).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('provider specified but no token available', () => { | ||
| it('should not inject Authorization header when token is null', async () => { | ||
| const accessor = createMockAccessor({ github: null }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result.headers).toBeUndefined(); | ||
| expect(result.credentials).toBeUndefined(); | ||
| expect(result.method).toBe('GET'); | ||
| expect(accessor.getToken).toHaveBeenCalledWith('github'); | ||
| }); | ||
|
|
||
| it('should not inject Authorization header when provider is unknown', async () => { | ||
| const accessor = createMockAccessor({}); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| credentials: { provider: 'unknown-provider' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result.headers).toBeUndefined(); | ||
| expect(result.credentials).toBeUndefined(); | ||
| expect(accessor.getToken).toHaveBeenCalledWith('unknown-provider'); | ||
| }); | ||
|
|
||
| it('should remove credentials field even when no token is available', async () => { | ||
| const accessor = createMockAccessor({}); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'POST', | ||
| credentials: { provider: 'github' }, | ||
| body: 'payload', | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result.credentials).toBeUndefined(); | ||
| expect(result.body).toBe('payload'); | ||
| expect(result.method).toBe('POST'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('existing headers preserved', () => { | ||
| it('should preserve existing headers when injecting token', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/vnd.github.v3+json', | ||
| 'X-Custom-Header': 'custom-value', | ||
| }, | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| const headers = new Headers(result.headers); | ||
| expect(headers.get('Authorization')).toBe(`Bearer ${TEST_TOKEN}`); | ||
| expect(headers.get('Content-Type')).toBe('application/json'); | ||
| expect(headers.get('Accept')).toBe('application/vnd.github.v3+json'); | ||
| expect(headers.get('X-Custom-Header')).toBe('custom-value'); | ||
| }); | ||
|
|
||
| it('should preserve Headers object when injecting token', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const existingHeaders = new Headers(); | ||
| existingHeaders.set('X-Request-Id', '12345'); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| headers: existingHeaders, | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| const headers = new Headers(result.headers); | ||
| expect(headers.get('Authorization')).toBe(`Bearer ${TEST_TOKEN}`); | ||
| expect(headers.get('X-Request-Id')).toBe('12345'); | ||
| }); | ||
|
|
||
| it('should override existing Authorization header with vault token', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const init: FrontMcpFetchInit = { | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: 'Bearer old-stale-token', | ||
| }, | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| const headers = new Headers(result.headers); | ||
| expect(headers.get('Authorization')).toBe(`Bearer ${TEST_TOKEN}`); | ||
| }); | ||
| }); | ||
|
|
||
| describe('other init properties', () => { | ||
| it('should preserve all non-credentials init properties', async () => { | ||
| const accessor = createMockAccessor({ github: TEST_TOKEN }); | ||
| const middleware = new FetchCredentialMiddleware(accessor); | ||
|
|
||
| const signal = new AbortController().signal; | ||
| const init: FrontMcpFetchInit = { | ||
| method: 'POST', | ||
| body: JSON.stringify({ data: 'test' }), | ||
| signal, | ||
| credentials: { provider: 'github' }, | ||
| }; | ||
|
|
||
| const result = await middleware.applyCredentials(TEST_URL, init); | ||
|
|
||
| expect(result.method).toBe('POST'); | ||
| expect(result.body).toBe(JSON.stringify({ data: 'test' })); | ||
| expect(result.signal).toBe(signal); | ||
| expect(result.credentials).toBeUndefined(); | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Coverage gap: built-in appliers are not directly tested.
Please add explicit tests for headerApplier, basicApplier, and especially queryApplier URL mutation behavior, so all new credential strategies are validated.
As per coding guidelines, "Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/auth/src/fetch/__tests__/fetch-credential-middleware.spec.ts` around
lines 10 - 277, Tests are missing for the built-in appliers: headerApplier,
basicApplier, and queryApplier; add unit tests that directly import these
appliers and validate their behavior (headerApplier injects the specified header
into init.headers preserving existing headers/Headers objects, basicApplier
encodes and sets the Authorization Basic header correctly and overrides any
existing Authorization, and queryApplier appends/mutates the URL query string as
expected without losing existing params and returns the new URL); use the same
test helpers (createMockAccessor or a stub token value) and call the applier
functions with representative FrontMcpFetchInit and URL inputs, asserting both
returned init.headers/Authorization and the returned URL string mutation
behavior for queryApplier so the credential strategies are fully covered.
Comment on lines
+48
to
+57
| ```typescript | ||
| // In @FrontMcp({ authorities: { ... } }) | ||
| authorities: { | ||
| claimsMapping: { | ||
| roles: 'https://myapp.com/roles', | ||
| permissions: 'permissions', | ||
| tenantId: 'org_id', | ||
| }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Syntax error in Auth0 code example: unbalanced braces.
The authorities config at line 50 opens with authorities: { but closes improperly at lines 56-57.
📝 Suggested fix
// In `@FrontMcp`({ authorities: { ... } })
authorities: {
claimsMapping: {
roles: 'https://myapp.com/roles',
permissions: 'permissions',
tenantId: 'org_id',
},
-})
+}Note: If this is meant to be inside @FrontMcp({...}), the full structure should be:
`@FrontMcp`({
authorities: {
claimsMapping: {
roles: 'https://myapp.com/roles',
permissions: 'permissions',
tenantId: 'org_id',
},
},
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/skills/catalog/frontmcp-authorities/references/claims-mapping.md` around
lines 48 - 57, The example has unbalanced braces in the `@FrontMcp` decorator: the
authorities object opens with authorities: { and the inner claimsMapping closes
but the outer authorities and the decorator object are not closed; update the
snippet so the authorities object and the decorator argument are properly closed
(ensure the authorities: { ... } block contains the claimsMapping object and
then add the missing closing brace and closing parentheses for `@FrontMcp`),
referencing the authorities and claimsMapping keys and the `@FrontMcp` decorator
to locate where to add the missing closing braces.
Comment on lines
+92
to
+100
| ```typescript | ||
| // In @FrontMcp({ authorities: { ... } }) | ||
| authorities: { | ||
| claimsMapping: { | ||
| roles: 'realm_access.roles', | ||
| permissions: 'resource_access.my-client.roles', | ||
| }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Same syntax issue in Keycloak code example.
The Keycloak example has the same unbalanced brace issue as the Auth0 example.
📝 Suggested fix
// In `@FrontMcp`({ authorities: { ... } })
authorities: {
claimsMapping: {
roles: 'realm_access.roles',
permissions: 'resource_access.my-client.roles',
},
-})
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/skills/catalog/frontmcp-authorities/references/claims-mapping.md` around
lines 92 - 100, The Keycloak example has an unbalanced brace/parenthesis in the
`@FrontMcp` decorator example; update the snippet so the authorities object and
the decorator call are properly closed. Specifically, fix the example that
references authorities, claimsMapping, roles and permissions so the structure is
`@FrontMcp`({ authorities: { claimsMapping: { roles: 'realm_access.roles',
permissions: 'resource_access.my-client.roles' } } }) — ensure claimsMapping,
authorities and the `@FrontMcp` call all have matching braces/parentheses.
Comment on lines
+302
to
+308
| const parts = formatter.formatToParts(now); | ||
| const hour = parseInt(parts.find((p) => p.type === 'hour')?.value ?? '0', 10); | ||
| const dayIndex = now.getDay(); // 0=Sun in the configured timezone context | ||
|
|
||
| const dayAllowed = days.includes(dayIndex); | ||
| const hourAllowed = hour >= startHour && hour < endHour; | ||
| const granted = dayAllowed && hourAllowed; |
Contributor
There was a problem hiding this comment.
Timezone handling bug in time window example.
Line 304 uses now.getDay() which returns the day of week in the local system timezone, not the configured timezone from the Intl.DateTimeFormat. This could cause incorrect day-of-week checks when the server timezone differs from the configured timezone.
💡 Suggested fix
const parts = formatter.formatToParts(now);
const hour = parseInt(parts.find((p) => p.type === 'hour')?.value ?? '0', 10);
- const dayIndex = now.getDay(); // 0=Sun in the configured timezone context
+ // Get day of week in the configured timezone
+ const dayFormatter = new Intl.DateTimeFormat('en-US', {
+ timeZone: timezone ?? 'UTC',
+ weekday: 'narrow',
+ });
+ const dayMap: Record<string, number> = { S: 0, M: 1, T: 2, W: 3, F: 5 };
+ // Note: 'T' appears for both Tuesday and Thursday - need 'long' format for accuracy
+ const dayParts = dayFormatter.formatToParts(now);
+ const dayName = dayParts.find((p) => p.type === 'weekday')?.value ?? '';
+ const dayIndex = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']
+ .indexOf(new Intl.DateTimeFormat('en-US', { timeZone: timezone ?? 'UTC', weekday: 'long' }).format(now));
const dayAllowed = days.includes(dayIndex);📝 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.
Suggested change
| const parts = formatter.formatToParts(now); | |
| const hour = parseInt(parts.find((p) => p.type === 'hour')?.value ?? '0', 10); | |
| const dayIndex = now.getDay(); // 0=Sun in the configured timezone context | |
| const dayAllowed = days.includes(dayIndex); | |
| const hourAllowed = hour >= startHour && hour < endHour; | |
| const granted = dayAllowed && hourAllowed; | |
| const parts = formatter.formatToParts(now); | |
| const hour = parseInt(parts.find((p) => p.type === 'hour')?.value ?? '0', 10); | |
| // Get day of week in the configured timezone | |
| const dayFormatter = new Intl.DateTimeFormat('en-US', { | |
| timeZone: timezone ?? 'UTC', | |
| weekday: 'narrow', | |
| }); | |
| const dayMap: Record<string, number> = { S: 0, M: 1, T: 2, W: 3, F: 5 }; | |
| // Note: 'T' appears for both Tuesday and Thursday - need 'long' format for accuracy | |
| const dayParts = dayFormatter.formatToParts(now); | |
| const dayName = dayParts.find((p) => p.type === 'weekday')?.value ?? ''; | |
| const dayIndex = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday'] | |
| .indexOf(new Intl.DateTimeFormat('en-US', { timeZone: timezone ?? 'UTC', weekday: 'long' }).format(now)); | |
| const dayAllowed = days.includes(dayIndex); | |
| const hourAllowed = hour >= startHour && hour < endHour; | |
| const granted = dayAllowed && hourAllowed; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/skills/catalog/frontmcp-authorities/references/custom-evaluators.md`
around lines 302 - 308, The bug is that dayIndex uses now.getDay() (system
timezone) instead of the configured Intl.DateTimeFormat timezone; replace the
getDay() call by extracting the weekday from the already-created parts (from
formatter.formatToParts(now)) — e.g. get parts.find(p => p.type ===
'weekday')?.value and map that localized weekday string to a numeric index (0-6)
that matches your days array, then use that mapped index for dayAllowed; update
any logic referencing dayIndex/dayAllowed to use the new mapped value.
Comment on lines
+266
to
+272
| // In @FrontMcp({ authorities: { ... } }) | ||
| authorities: { | ||
| claimsMapping: { roles: 'roles' }, | ||
| relationshipResolver: myRelationshipResolver, | ||
| profiles: { ... }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix syntax error in RelationshipResolver config example.
The code block has a closing }) that doesn't match the opening — it appears to mix @FrontMcp decorator syntax with a standalone object.
📝 Suggested fix
// In `@FrontMcp`({ authorities: { ... } })
-authorities: {
+@FrontMcp({
+ authorities: {
claimsMapping: { roles: 'roles' },
relationshipResolver: myRelationshipResolver,
profiles: { ... },
-})
+ },
+})Or if showing just the config object:
// In `@FrontMcp`({ authorities: { ... } })
authorities: {
claimsMapping: { roles: 'roles' },
relationshipResolver: myRelationshipResolver,
profiles: { ... },
-})
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/skills/catalog/frontmcp-authorities/references/rbac-abac-rebac.md`
around lines 266 - 272, The example shows a mismatched closing `})` mixing
decorator syntax and a standalone object; update the snippet so it is a valid
construct by either (A) presenting the full decorator form — start with
`@FrontMcp`({...}) and close with one `)` — ensuring the inner authorities object
contains claimsMapping, relationshipResolver: myRelationshipResolver, and
profiles, or (B) showing only the plain config object — start with `authorities:
{ ... }` and close with a single `}` — making sure `relationshipResolver:
myRelationshipResolver` is correctly placed; reference the `@FrontMcp` decorator
and the authorities.relationshipResolver/myRelationshipResolver symbols to
locate and fix the example.
Comment on lines
+115
to
+140
| ```typescript | ||
| // In @FrontMcp({ authorities: { ... } }) | ||
| authorities: { | ||
| claimsMapping: { roles: 'realm_access.roles', permissions: 'scope' }, | ||
| profiles: { | ||
| admin: { | ||
| roles: { any: ['admin', 'superadmin'] }, | ||
| }, | ||
| authenticated: { | ||
| attributes: { | ||
| conditions: [{ path: 'user.sub', op: 'exists', value: true }], | ||
| }, | ||
| }, | ||
| matchTenant: { | ||
| attributes: { | ||
| conditions: [ | ||
| { path: 'claims.org_id', op: 'eq', value: { fromInput: 'tenantId' } }, | ||
| ], | ||
| }, | ||
| }, | ||
| editor: { | ||
| permissions: { any: ['content:write', 'content:publish'] }, | ||
| }, | ||
| }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Syntax issue in code example.
The code block at lines 115-140 is missing the closing }) for the @FrontMcp decorator. The profiles object closes correctly, but the decorator call and class definition are incomplete.
📝 Proposed fix
editor: {
permissions: { any: ['content:write', 'content:publish'] },
},
},
- })
+ },
+})
+export class MyServer {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/skills/catalog/frontmcp-authorities/SKILL.md` around lines 115 - 140,
The example decorator usage for `@FrontMcp` with the authorities config is missing
the closing characters for the decorator call; update the snippet around the
`@FrontMcp`({ authorities: { ... } }) example (the authorities/profiles block
containing claimsMapping, profiles, admin/authenticated/matchTenant/editor) to
include the missing closing "})" for the decorator call and the following class
declaration closure so the decorator and class are syntactically complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation