From d872308426eab7b5d7826e23dec99815a5613941 Mon Sep 17 00:00:00 2001 From: David Antoon Date: Tue, 3 Feb 2026 20:10:51 +0200 Subject: [PATCH 1/8] feat: Enhance security by implementing least-privilege permissions and improving HTML sanitization --- .github/workflows/push.yml | 4 + .../providers/request-logger.provider.ts | 16 +- .../src/apps/sessions/data/session.store.ts | 9 +- libs/auth/src/jwks/jwks.utils.ts | 8 +- libs/auth/src/utils/www-authenticate.utils.ts | 89 ++++- .../3rd-party-integration/src/http-client.ts | 9 +- .../__tests__/oauth.authorize.flow.test.ts | 68 +++- .../server/adapters/express.host.adapter.ts | 57 +++- libs/ui/src/bridge/adapters/claude.adapter.ts | 20 +- libs/ui/src/bridge/adapters/gemini.adapter.ts | 20 +- libs/ui/src/components/alert.ts | 45 ++- libs/ui/src/components/badge.ts | 26 +- libs/ui/src/components/card.ts | 53 ++- libs/ui/src/components/form.ts | 45 ++- libs/ui/src/renderers/react.adapter.ts | 22 +- .../src/universal/renderers/html.renderer.ts | 301 ++++++++++++++++- .../src/runtime/adapters/html.adapter.ts | 4 +- libs/uipack/src/runtime/index.ts | 2 + libs/uipack/src/runtime/mcp-bridge.ts | 15 +- libs/uipack/src/runtime/sanitizer.ts | 313 ++++++++++++++++++ libs/utils/src/crypto/index.ts | 8 +- 21 files changed, 1060 insertions(+), 74 deletions(-) diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 1af853e7..4a0104aa 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -16,6 +16,10 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +# Minimal permissions for security (least-privilege principle) +permissions: + contents: read + jobs: # Shared setup job that other jobs can reference setup: diff --git a/apps/e2e/demo-e2e-providers/src/apps/config/providers/request-logger.provider.ts b/apps/e2e/demo-e2e-providers/src/apps/config/providers/request-logger.provider.ts index 12c8109c..fcf0fe96 100644 --- a/apps/e2e/demo-e2e-providers/src/apps/config/providers/request-logger.provider.ts +++ b/apps/e2e/demo-e2e-providers/src/apps/config/providers/request-logger.provider.ts @@ -1,4 +1,5 @@ import { AsyncProvider, ProviderScope } from '@frontmcp/sdk'; +import { randomBytes } from '@frontmcp/utils'; /** * Token for the RequestLoggerProvider @@ -28,9 +29,16 @@ export interface RequestLoggerInfo { /** * Implementation class for RequestLogger */ +/** + * Generate a cryptographically secure random ID. + */ +function generateSecureId(prefix: string, bytes = 6): string { + return `${prefix}-${Buffer.from(randomBytes(bytes)).toString('hex')}`; +} + class RequestLoggerImpl implements RequestLogger { readonly createdAt = new Date(); - readonly instanceId = `req-${Math.random().toString(36).substring(2, 10)}`; + readonly instanceId = generateSecureId('req'); private logs: string[] = []; constructor(readonly requestId: string, readonly sessionId: string) {} @@ -64,10 +72,10 @@ export const RequestLoggerProvider = AsyncProvider({ scope: ProviderScope.CONTEXT, inject: () => [] as const, useFactory: async (): Promise => { - // Generate unique IDs for this request instance + // Generate unique IDs for this request instance using cryptographically secure randomness // In a real implementation, this would receive context from the tool - const requestId = `req-${Date.now()}-${Math.random().toString(36).substring(2, 8)}`; - const sessionId = `sess-${Math.random().toString(36).substring(2, 10)}`; + const requestId = `req-${Date.now()}-${Buffer.from(randomBytes(4)).toString('hex')}`; + const sessionId = generateSecureId('sess'); return new RequestLoggerImpl(requestId, sessionId); }, }); diff --git a/apps/e2e/demo-e2e-redis/src/apps/sessions/data/session.store.ts b/apps/e2e/demo-e2e-redis/src/apps/sessions/data/session.store.ts index c28f86dd..92c74390 100644 --- a/apps/e2e/demo-e2e-redis/src/apps/sessions/data/session.store.ts +++ b/apps/e2e/demo-e2e-redis/src/apps/sessions/data/session.store.ts @@ -51,7 +51,14 @@ class SessionStore { const allKeys = Array.from(this.data.keys()); if (!pattern) return allKeys; - const regex = new RegExp(pattern.replace('*', '.*')); + // Escape all regex special characters except *, then convert * to .* + const regex = new RegExp( + '^' + + pattern + .replace(/[.+?^${}()|[\]\\]/g, '\\$&') // Escape special chars + .replace(/\*/g, '.*') + // Convert glob * to regex .* + '$' + ); return allKeys.filter((k) => regex.test(k)); } diff --git a/libs/auth/src/jwks/jwks.utils.ts b/libs/auth/src/jwks/jwks.utils.ts index f2af5ba1..d52264db 100644 --- a/libs/auth/src/jwks/jwks.utils.ts +++ b/libs/auth/src/jwks/jwks.utils.ts @@ -1,5 +1,11 @@ export function trimSlash(s: string) { - return (s ?? '').replace(/\/+$/, ''); + // Safe: Use simple character-by-character approach to avoid ReDoS + const str = s ?? ''; + let end = str.length; + while (end > 0 && str[end - 1] === '/') { + end--; + } + return str.slice(0, end); } export function normalizeIssuer(u?: string) { return trimSlash(String(u ?? '')); diff --git a/libs/auth/src/utils/www-authenticate.utils.ts b/libs/auth/src/utils/www-authenticate.utils.ts index bba23b8b..acd742e4 100644 --- a/libs/auth/src/utils/www-authenticate.utils.ts +++ b/libs/auth/src/utils/www-authenticate.utils.ts @@ -172,6 +172,8 @@ export function buildInvalidRequestHeader(prmUrl: string, description?: string): /** * Parse a WWW-Authenticate header value * + * Uses character-by-character parsing to avoid ReDoS vulnerabilities. + * * @param header - The WWW-Authenticate header value * @returns Parsed header options */ @@ -183,13 +185,11 @@ export function parseWwwAuthenticate(header: string): WwwAuthenticateOptions { return result; } - // Extract parameters + // Extract parameters using safe character-by-character parsing const paramString = header.substring(6).trim(); - const paramRegex = /(\w+)="([^"\\]*(?:\\.[^"\\]*)*)"/g; - let match: RegExpExecArray | null; + const params = parseQuotedParams(paramString); - while ((match = paramRegex.exec(paramString)) !== null) { - const [, key, value] = match; + for (const [key, value] of params) { const unescapedValue = unescapeQuotedString(value); switch (key.toLowerCase()) { @@ -217,6 +217,78 @@ export function parseWwwAuthenticate(header: string): WwwAuthenticateOptions { return result; } +/** + * Parse key="value" pairs from a string using character-by-character parsing. + * This avoids ReDoS vulnerabilities from complex regex patterns. + */ +function parseQuotedParams(input: string): Array<[string, string]> { + const result: Array<[string, string]> = []; + let i = 0; + const len = input.length; + + while (i < len) { + // Skip whitespace and commas + while (i < len && (input[i] === ' ' || input[i] === ',' || input[i] === '\t')) { + i++; + } + if (i >= len) break; + + // Parse key (word characters) + const keyStart = i; + while (i < len && /\w/.test(input[i])) { + i++; + } + const key = input.slice(keyStart, i); + if (!key) break; + + // Skip whitespace + while (i < len && (input[i] === ' ' || input[i] === '\t')) { + i++; + } + + // Expect '=' + if (i >= len || input[i] !== '=') { + // Skip to next comma or end + while (i < len && input[i] !== ',') i++; + continue; + } + i++; // skip '=' + + // Skip whitespace + while (i < len && (input[i] === ' ' || input[i] === '\t')) { + i++; + } + + // Expect '"' + if (i >= len || input[i] !== '"') { + // Skip to next comma or end + while (i < len && input[i] !== ',') i++; + continue; + } + i++; // skip opening quote + + // Parse value (handle escaped characters) + let value = ''; + while (i < len && input[i] !== '"') { + if (input[i] === '\\' && i + 1 < len) { + // Escaped character + value += input[i + 1]; + i += 2; + } else { + value += input[i]; + i++; + } + } + if (i < len && input[i] === '"') { + i++; // skip closing quote + } + + result.push([key, value]); + } + + return result; +} + /** * Escape special characters for quoted-string per RFC 7230 */ @@ -237,5 +309,10 @@ function unescapeQuotedString(value: string): string { function normalizePathSegment(path: string): string { if (!path || path === '/') return ''; const normalized = path.startsWith('/') ? path : `/${path}`; - return normalized.replace(/\/+$/, ''); + // Safe: Use character-by-character approach to trim trailing slashes + let end = normalized.length; + while (end > 0 && normalized[end - 1] === '/') { + end--; + } + return normalized.slice(0, end); } diff --git a/libs/cli/src/templates/3rd-party-integration/src/http-client.ts b/libs/cli/src/templates/3rd-party-integration/src/http-client.ts index a3a6e048..d2dc9467 100644 --- a/libs/cli/src/templates/3rd-party-integration/src/http-client.ts +++ b/libs/cli/src/templates/3rd-party-integration/src/http-client.ts @@ -67,10 +67,17 @@ export class HttpClient { } if (this.ctx.logDebug) { + // Redact sensitive headers to prevent credential leakage in logs + const SENSITIVE_HEADERS = ['authorization', 'x-api-key', 'cookie', 'x-auth-token']; + const sanitizedHeaders = Object.fromEntries( + Object.entries(baseHeaders).map(([k, v]) => + [k, SENSITIVE_HEADERS.includes(k.toLowerCase()) ? '[REDACTED]' : v] + ) + ); console.debug("[HttpClient] Request", { url: url.toString(), method: options.method, - headers: baseHeaders, + headers: sanitizedHeaders, body: options.bodyJson }); } diff --git a/libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts b/libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts index 883f5d4e..5bfe018e 100644 --- a/libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts +++ b/libs/sdk/src/auth/flows/__tests__/oauth.authorize.flow.test.ts @@ -963,13 +963,75 @@ describe('OAuth Authorize Flow', () => { // ============================================ describe('Invalid redirect_uri', () => { + /** + * Helper to validate redirect URIs safely. + * Only allows http: and https: schemes. + */ + function isValidRedirectUri(uri: string): boolean { + try { + const url = new URL(uri); + // Only allow http and https schemes (case-insensitive via URL parsing) + return url.protocol === 'http:' || url.protocol === 'https:'; + } catch { + return false; + } + } + it('should reject non-URL redirect_uri', () => { expect(() => new URL('not-a-url')).toThrow(); + expect(isValidRedirectUri('not-a-url')).toBe(false); + }); + + it('should identify javascript: URI as malicious (all case variations)', () => { + const maliciousUris = [ + 'javascript:alert(1)', + 'JAVASCRIPT:alert(1)', + 'JaVaScRiPt:alert(1)', + 'javascript:void(0)', + ]; + + for (const uri of maliciousUris) { + // The URL class normalizes protocol to lowercase + const url = new URL(uri); + expect(url.protocol).toBe('javascript:'); + expect(isValidRedirectUri(uri)).toBe(false); + } + }); + + it('should identify data: URI as malicious', () => { + const dataUris = [ + 'data:text/html,', + 'DATA:text/html,', + 'data:application/javascript,alert(1)', + ]; + + for (const uri of dataUris) { + expect(isValidRedirectUri(uri)).toBe(false); + } + }); + + it('should identify vbscript: URI as malicious', () => { + const vbUris = [ + 'vbscript:msgbox(1)', + 'VBSCRIPT:msgbox(1)', + ]; + + for (const uri of vbUris) { + expect(isValidRedirectUri(uri)).toBe(false); + } }); - it('should identify javascript: URI as malicious', () => { - const maliciousUri = 'javascript:alert(1)'; - expect(maliciousUri.startsWith('javascript:')).toBe(true); + it('should accept valid http/https URIs', () => { + const validUris = [ + 'http://localhost:3000/callback', + 'https://example.com/oauth/callback', + 'HTTP://EXAMPLE.COM/CALLBACK', + 'HTTPS://EXAMPLE.COM/CALLBACK', + ]; + + for (const uri of validUris) { + expect(isValidRedirectUri(uri)).toBe(true); + } }); }); }); diff --git a/libs/sdk/src/server/adapters/express.host.adapter.ts b/libs/sdk/src/server/adapters/express.host.adapter.ts index 9151204e..cf4e1af0 100644 --- a/libs/sdk/src/server/adapters/express.host.adapter.ts +++ b/libs/sdk/src/server/adapters/express.host.adapter.ts @@ -5,16 +5,69 @@ import cors from 'cors'; import { HostServerAdapter } from './base.host.adapter'; import { HttpMethod, ServerRequest, ServerRequestHandler, ServerResponse } from '../../common'; +/** + * CORS configuration options for ExpressHostAdapter. + */ +export interface ExpressCorsOptions { + /** + * Allowed origins. Can be: + * - `true` to reflect the request origin (allows all origins with credentials) + * - `false` to disable CORS + * - `'*'` to allow all origins (no credentials) + * - A string for a single origin + * - An array of strings for multiple origins + * - A function that dynamically determines if an origin is allowed + * @default false (CORS disabled by default for security) + */ + origin?: boolean | string | string[] | ((origin: string | undefined, callback: (err: Error | null, allow?: boolean) => void) => void); + + /** + * Whether to allow credentials (cookies, authorization headers). + * Cannot be used with `origin: '*'`. + * @default false + */ + credentials?: boolean; + + /** + * How long preflight results can be cached (in seconds). + * @default 300 + */ + maxAge?: number; +} + +/** + * Options for ExpressHostAdapter. + */ +export interface ExpressHostAdapterOptions { + /** + * CORS configuration. + * For security, CORS is disabled by default. + * Enable it explicitly with appropriate origins. + */ + cors?: ExpressCorsOptions; +} + export class ExpressHostAdapter extends HostServerAdapter { private app = express(); private router = express.Router(); private prepared = false; - constructor() { + constructor(options?: ExpressHostAdapterOptions) { super(); this.app.use(express.json()); this.app.use(express.urlencoded({ extended: true })); - this.app.use(cors({ origin: '*', maxAge: 300 })); + + // Configure CORS with secure defaults + // By default, CORS is disabled (origin: false) + const corsOptions = options?.cors; + if (corsOptions !== undefined && corsOptions.origin !== false) { + this.app.use(cors({ + origin: corsOptions.origin ?? false, + credentials: corsOptions.credentials ?? false, + maxAge: corsOptions.maxAge ?? 300, + })); + } + // When creating the HTTP(S) server that hosts /mcp: this.app.use((req, res, next) => { res.setHeader('Access-Control-Expose-Headers', 'WWW-Authenticate'); diff --git a/libs/ui/src/bridge/adapters/claude.adapter.ts b/libs/ui/src/bridge/adapters/claude.adapter.ts index 8c5c04d5..9a26a3b3 100644 --- a/libs/ui/src/bridge/adapters/claude.adapter.ts +++ b/libs/ui/src/bridge/adapters/claude.adapter.ts @@ -11,6 +11,22 @@ import type { DisplayMode } from '../types'; import { BaseAdapter, DEFAULT_CAPABILITIES } from './base-adapter'; +/** + * Allowed Claude domains for security validation. + */ +const CLAUDE_DOMAINS = ['claude.ai', 'anthropic.com']; + +/** + * Check if a hostname is a valid Claude domain. + * Validates exact match or proper subdomain (prevents attacker.com/claude.ai attacks). + */ +function isValidClaudeDomain(hostname: string): boolean { + const lowerHost = hostname.toLowerCase(); + return CLAUDE_DOMAINS.some(domain => + lowerHost === domain || lowerHost.endsWith('.' + domain) + ); +} + /** * Claude adapter for Anthropic's Claude AI. * @@ -78,9 +94,9 @@ export class ClaudeAdapter extends BaseAdapter { if (win.__claudeArtifact) return true; // Check URL patterns for Claude (legacy only) + // Use proper domain validation to prevent subdomain attacks if (typeof location !== 'undefined') { - const href = location.href; - if (href.includes('claude.ai') || href.includes('anthropic.com')) { + if (isValidClaudeDomain(location.hostname)) { return true; } } diff --git a/libs/ui/src/bridge/adapters/gemini.adapter.ts b/libs/ui/src/bridge/adapters/gemini.adapter.ts index 5b032670..fe6b9f38 100644 --- a/libs/ui/src/bridge/adapters/gemini.adapter.ts +++ b/libs/ui/src/bridge/adapters/gemini.adapter.ts @@ -9,6 +9,22 @@ import { BaseAdapter, DEFAULT_CAPABILITIES } from './base-adapter'; +/** + * Allowed Gemini domains for security validation. + */ +const GEMINI_DOMAINS = ['gemini.google.com', 'bard.google.com']; + +/** + * Check if a hostname is a valid Gemini domain. + * Validates exact match or proper subdomain (prevents attacker.com/gemini.google.com attacks). + */ +function isValidGeminiDomain(hostname: string): boolean { + const lowerHost = hostname.toLowerCase(); + return GEMINI_DOMAINS.some(domain => + lowerHost === domain || lowerHost.endsWith('.' + domain) + ); +} + /** * Gemini SDK global interface (simplified type). */ @@ -76,9 +92,9 @@ export class GeminiAdapter extends BaseAdapter { if (win.gemini) return true; // Check URL patterns for Gemini + // Use proper domain validation to prevent subdomain attacks if (typeof location !== 'undefined') { - const href = location.href; - if (href.includes('gemini.google.com') || href.includes('bard.google.com')) { + if (isValidGeminiDomain(location.hostname)) { return true; } } diff --git a/libs/ui/src/components/alert.ts b/libs/ui/src/components/alert.ts index 82b038b3..71e553b2 100644 --- a/libs/ui/src/components/alert.ts +++ b/libs/ui/src/components/alert.ts @@ -5,6 +5,7 @@ */ import { escapeHtml } from '../layouts/base'; +import { sanitizeHtmlContent } from '@frontmcp/uipack/runtime'; // ============================================ // Alert Types @@ -16,7 +17,12 @@ import { escapeHtml } from '../layouts/base'; export type AlertVariant = 'info' | 'success' | 'warning' | 'danger' | 'neutral'; /** - * Alert component options + * Alert component options. + * + * **Security Note**: The `icon` and `actions` parameters accept raw HTML. + * Do NOT pass untrusted user input to these parameters without sanitization. + * Use `escapeHtml()` from `@frontmcp/ui/layouts` for text content, or use the + * `sanitize` option to automatically sanitize HTML content. */ export interface AlertOptions { /** Alert variant */ @@ -25,7 +31,10 @@ export interface AlertOptions { title?: string; /** Show icon */ showIcon?: boolean; - /** Custom icon (overrides default) */ + /** + * Custom icon (overrides default, raw HTML). + * **Warning**: Do not pass untrusted user input without sanitization. + */ icon?: string; /** Dismissible alert */ dismissible?: boolean; @@ -33,8 +42,17 @@ export interface AlertOptions { className?: string; /** Alert ID */ id?: string; - /** Actions (buttons) */ + /** + * Actions (buttons, raw HTML). + * **Warning**: Do not pass untrusted user input without sanitization. + */ actions?: string; + /** + * If true, sanitizes HTML content to prevent XSS. + * Removes script tags, event handlers, and dangerous attributes. + * @default false + */ + sanitize?: boolean; } // ============================================ @@ -96,7 +114,22 @@ function getVariantClasses(variant: AlertVariant): { container: string; icon: st * Build an alert component */ export function alert(message: string, options: AlertOptions = {}): string { - const { variant = 'info', title, showIcon = true, icon, dismissible = false, className = '', id, actions } = options; + const { + variant = 'info', + title, + showIcon = true, + icon, + dismissible = false, + className = '', + id, + actions, + sanitize = false, + } = options; + + // Sanitize raw HTML content if requested + // codeql[js/html-constructed-from-input]: icon and actions are intentionally raw HTML for composability; sanitize option available + const safeIcon = sanitize && icon ? sanitizeHtmlContent(icon) : icon; + const safeActions = sanitize && actions ? sanitizeHtmlContent(actions) : actions; const variantClasses = getVariantClasses(variant); @@ -104,7 +137,7 @@ export function alert(message: string, options: AlertOptions = {}): string { const iconHtml = showIcon ? `
- ${icon || alertIcons[variant]} + ${safeIcon || alertIcons[variant]}
` : ''; @@ -123,7 +156,7 @@ export function alert(message: string, options: AlertOptions = {}): string { ` : ''; - const actionsHtml = actions ? `
${actions}
` : ''; + const actionsHtml = safeActions ? `
${safeActions}
` : ''; const idAttr = id ? `id="${escapeHtml(id)}"` : ''; diff --git a/libs/ui/src/components/badge.ts b/libs/ui/src/components/badge.ts index 88d29b8e..9801e658 100644 --- a/libs/ui/src/components/badge.ts +++ b/libs/ui/src/components/badge.ts @@ -5,6 +5,7 @@ */ import { escapeHtml } from '../layouts/base'; +import { sanitizeHtmlContent } from '@frontmcp/uipack/runtime'; // ============================================ // Badge Types @@ -21,7 +22,12 @@ export type BadgeVariant = 'default' | 'primary' | 'secondary' | 'success' | 'wa export type BadgeSize = 'sm' | 'md' | 'lg'; /** - * Badge component options + * Badge component options. + * + * **Security Note**: The `icon` parameter accepts raw HTML. + * Do NOT pass untrusted user input to this parameter without sanitization. + * Use `escapeHtml()` from `@frontmcp/ui/layouts` for text content, or use the + * `sanitize` option to automatically sanitize HTML content. */ export interface BadgeOptions { /** Badge variant */ @@ -30,7 +36,10 @@ export interface BadgeOptions { size?: BadgeSize; /** Rounded pill style */ pill?: boolean; - /** Icon before text */ + /** + * Icon before text (raw HTML). + * **Warning**: Do not pass untrusted user input without sanitization. + */ icon?: string; /** Dot indicator (no text) */ dot?: boolean; @@ -38,6 +47,12 @@ export interface BadgeOptions { className?: string; /** Removable badge */ removable?: boolean; + /** + * If true, sanitizes HTML content to prevent XSS. + * Removes script tags, event handlers, and dangerous attributes. + * @default false + */ + sanitize?: boolean; } // ============================================ @@ -94,8 +109,13 @@ export function badge(text: string, options: BadgeOptions = {}): string { dot = false, className = '', removable = false, + sanitize = false, } = options; + // Sanitize raw HTML content if requested + // codeql[js/html-constructed-from-input]: icon is intentionally raw HTML for composability; sanitize option available + const safeIcon = sanitize && icon ? sanitizeHtmlContent(icon) : icon; + // Dot badge (status indicator) if (dot) { const dotVariants: Record = { @@ -129,7 +149,7 @@ export function badge(text: string, options: BadgeOptions = {}): string { .filter(Boolean) .join(' '); - const iconHtml = icon ? `${icon}` : ''; + const iconHtml = safeIcon ? `${safeIcon}` : ''; const removeHtml = removable ? `