From de6c0fb781a87f130cd91a9826f1ff2851b05c68 Mon Sep 17 00:00:00 2001 From: "Julien G." <66769052+jubnl@users.noreply.github.com> Date: Thu, 7 May 2026 13:49:39 +0200 Subject: [PATCH] fix: prevent Invalid URL crash when APP_URL lacks a protocol (#972) * fix: prevent Invalid URL crash when APP_URL lacks a protocol (issue #970) - Add getMcpSafeUrl() to notifications.ts: wraps getAppUrl() and guarantees a result that satisfies the MCP SDK's checkIssuerUrl requirement (https:// or http://localhost). Non-HTTPS, non-localhost URLs fall back to http://localhost:{PORT} instead of propagating an "Issuer URL must be HTTPS" error. - Switch app.ts, mcp/index.ts, mcp/oauthProvider.ts, and oauthService.ts to import getMcpSafeUrl instead of getAppUrl for all MCP resource URL construction, so a misconfigured APP_URL never crashes the metadata router initialisation. - Restrict the SDK metadata router middleware to /.well-known/* paths only. Previously it was invoked on every request; in production the lazy getMetaRouter() init ran on GET / and threw "Invalid URL" when APP_URL had no scheme, returning 500 for every page load. - Log a startup warning when APP_URL is set but not usable, and include the resolved App URL in the startup banner so operators can confirm the correct value at a glance. - Update oauth.test.ts mock to target notifications.getMcpSafeUrl. * fix: show getAppUrl in banner and add two separate APP_URL startup checks - Banner now displays getAppUrl() (the resolved app URL) rather than getMcpSafeUrl() so operators see the actual configured value - Two independent startup warnings after the banner when APP_URL is set: 1. whether APP_URL is a valid URL (parseable by new URL()) 2. whether APP_URL is MCP-safe (https:// or http://localhost) - Fix getMcpSafeUrl() fallback port to use Number(PORT) || 3001, consistent with how index.ts parses PORT * fix: update oidc.ts to import getAppUrl from notifications --- server/src/app.ts | 15 ++++------ server/src/index.ts | 41 +++++++++++++++++++------- server/src/mcp/index.ts | 6 ++-- server/src/mcp/oauthProvider.ts | 4 +-- server/src/routes/oidc.ts | 2 +- server/src/services/notifications.ts | 35 ++++++++++++++++++++-- server/src/services/oauthService.ts | 4 +-- server/src/services/oidcService.ts | 8 ----- server/tests/integration/oauth.test.ts | 5 +++- 9 files changed, 81 insertions(+), 39 deletions(-) diff --git a/server/src/app.ts b/server/src/app.ts index be4c9424..c03c7583 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -50,11 +50,11 @@ import { getCollabFeatures } from './services/adminService'; import { isAddonEnabled } from './services/adminService'; import { ADDON_IDS } from './addons'; import { ALL_SCOPES } from './mcp/scopes'; -import { getAppUrl } from './services/oidcService'; import { mcpAuthMetadataRouter } from '@modelcontextprotocol/sdk/server/auth/router'; import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize'; import { clientRegistrationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/register'; import type { OAuthMetadata } from '@modelcontextprotocol/sdk/shared/auth'; +import { getMcpSafeUrl } from './services/notifications'; export function createApp(): express.Application { const app = express(); @@ -388,7 +388,7 @@ export function createApp(): express.Application { function getOAuthMetadata(): OAuthMetadata { if (_oauthMetadata) return _oauthMetadata; - const base = (getAppUrl() || 'http://localhost:3001').replace(/\/+$/, ''); + const base = getMcpSafeUrl().replace(/\/+$/, ''); _oauthMetadata = { issuer: base, authorization_endpoint: `${base}/oauth/authorize`, @@ -416,14 +416,11 @@ export function createApp(): express.Application { return _sdkMetaRouter; } - // Path-aware gate: only /.well-known/* returns 404 when disabled; other paths pass through - // so static files and SPA routes are unaffected when MCP is off. + // Only invoke the SDK metadata router for /.well-known/* paths. + // Calling getMetaRouter() on every request triggers lazy init (new URL(...)) which + // throws "Invalid URL" when APP_URL lacks a protocol — breaking all page loads. app.use((req: Request, res: Response, next: NextFunction) => { - const isMetadataPath = - req.path === '/.well-known/oauth-authorization-server' || - req.path === '/.well-known/openid-configuration' || - req.path.startsWith('/.well-known/oauth-protected-resource'); - if (isMetadataPath && !isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); + if (req.path.startsWith('/.well-known/') && !isAddonEnabled(ADDON_IDS.MCP)) return res.status(404).end(); getMetaRouter()(req, res, next); }); diff --git a/server/src/index.ts b/server/src/index.ts index cc500433..92a280cb 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -19,6 +19,7 @@ const tmpDir = path.join(__dirname, '../data/tmp'); const app = createApp(); import * as scheduler from './scheduler'; +import { getAppUrl, getMcpSafeUrl } from './services/notifications'; const PORT = Number(process.env.PORT) || 3001; const HOST = process.env.HOST; @@ -29,22 +30,42 @@ const onListen = () => { const LOG_LVL = (process.env.LOG_LEVEL || 'info').toLowerCase(); const tz = process.env.TZ || Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC'; const origins = process.env.ALLOWED_ORIGINS || '(same-origin)'; + const appUrl = getAppUrl(); + const resolvedAppUrl = getMcpSafeUrl(); const banner = [ '──────────────────────────────────────', ' TREK API started', - ` Version ${APP_VERSION}`, - ...(HOST ? [` Host: ${HOST}`] : []), - ` Port: ${PORT}`, - ` Environment: ${process.env.NODE_ENV?.toLowerCase() || 'development'}`, - ` Timezone: ${tz}`, - ` Origins: ${origins}`, - ` Log level: ${LOG_LVL}`, - ` Log file: /app/data/logs/trek.log`, - ` PID: ${process.pid}`, - ` User: uid=${process.getuid?.()} gid=${process.getgid?.()}`, + ` Version ${APP_VERSION}`, + ...(HOST ? [` Host: ${HOST}`] : []), + ` Container Port: ${PORT}`, + ` App URL: ${appUrl}`, + ` Environment: ${process.env.NODE_ENV?.toLowerCase() || 'development'}`, + ` Timezone: ${tz}`, + ` Origins: ${origins}`, + ` Log level: ${LOG_LVL}`, + ` Log file: /app/data/logs/trek.log`, + ` PID: ${process.pid}`, + ` User: uid=${process.getuid?.()} gid=${process.getgid?.()}`, '──────────────────────────────────────', ]; banner.forEach(l => console.log(l)); + if (process.env.APP_URL) { + let parsedAppUrl: URL | null = null; + try { parsedAppUrl = new URL(process.env.APP_URL); } catch { /* invalid */ } + + if (!parsedAppUrl) { + sLogWarn(`APP_URL: "${process.env.APP_URL}" is not a valid URL — it will be ignored.`); + } + + const mcpSafe = parsedAppUrl !== null && ( + parsedAppUrl.protocol === 'https:' || + parsedAppUrl.hostname === 'localhost' || + parsedAppUrl.hostname === '127.0.0.1' + ); + if (!mcpSafe) { + sLogWarn(`APP_URL: not MCP-safe (requires https:// or http://localhost) — MCP will use ${resolvedAppUrl}.`); + } + } if (process.env.DEMO_MODE?.toLowerCase() === 'true') sLogInfo('Demo mode: ENABLED'); if (process.env.DEMO_MODE?.toLowerCase() === 'true' && process.env.NODE_ENV?.toLowerCase() === 'production') { sLogWarn('SECURITY WARNING: DEMO_MODE is enabled in production!'); diff --git a/server/src/mcp/index.ts b/server/src/mcp/index.ts index f5506026..8fa99f58 100644 --- a/server/src/mcp/index.ts +++ b/server/src/mcp/index.ts @@ -11,7 +11,7 @@ import { registerResources } from './resources'; import { registerTools } from './tools'; import { McpSession, sessions, revokeUserSessions, revokeUserSessionsForClient } from './sessionManager'; import { writeAudit, getClientIp } from '../services/auditLog'; -import { getAppUrl } from '../services/oidcService'; +import { getMcpSafeUrl } from '../services/notifications'; export { revokeUserSessions, revokeUserSessionsForClient }; @@ -153,7 +153,7 @@ const sessionSweepInterval = setInterval(() => { sessionSweepInterval.unref(); function setAuthChallenge(res: Response, error = 'invalid_token'): void { - const base = (getAppUrl() || '').replace(/\/+$/, ''); + const base = (getMcpSafeUrl() || '').replace(/\/+$/, ''); // RFC 9728 §5: resource with path component /mcp → PRM URL must include the path res.set('WWW-Authenticate', `Bearer realm="TREK MCP", resource_metadata="${base}/.well-known/oauth-protected-resource/mcp", error="${error}"`); @@ -183,7 +183,7 @@ function verifyToken(authHeader: string | undefined): VerifyTokenResult | null { if (!result) return null; // RFC 8707: audience must always match this resource endpoint. // Pre-audit tokens with audience=null are revoked by the SEC-H6 migration. - const expected = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + const expected = `${(getMcpSafeUrl() || '').replace(/\/+$/, '')}/mcp`; if (result.audience !== expected) return null; return { user: result.user, scopes: result.scopes, clientId: result.clientId, isStaticToken: false }; } diff --git a/server/src/mcp/oauthProvider.ts b/server/src/mcp/oauthProvider.ts index d04c3814..943a515f 100644 --- a/server/src/mcp/oauthProvider.ts +++ b/server/src/mcp/oauthProvider.ts @@ -16,7 +16,7 @@ import { getUserByAccessToken, } from '../services/oauthService'; import { ALL_SCOPES } from './scopes'; -import { getAppUrl } from '../services/oidcService'; +import { getMcpSafeUrl } from '../services/notifications'; import { writeAudit } from '../services/auditLog'; // --------------------------------------------------------------------------- @@ -125,7 +125,7 @@ export const trekOAuthProvider: OAuthServerProvider = { // Redirects browser to the SPA consent page with OAuth params forwarded. async authorize(client: OAuthClientInformationFull, params: AuthorizationParams, res: Response): Promise { - const mcpResource = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + const mcpResource = `${getMcpSafeUrl().replace(/\/+$/, '')}/mcp`; const resource = params.resource ? params.resource.href.replace(/\/+$/, '') : mcpResource; if (resource !== mcpResource) { diff --git a/server/src/routes/oidc.ts b/server/src/routes/oidc.ts index 7aefe593..beed1a41 100644 --- a/server/src/routes/oidc.ts +++ b/server/src/routes/oidc.ts @@ -14,8 +14,8 @@ import { touchLastLogin, generateToken, frontendUrl, - getAppUrl, } from '../services/oidcService'; +import { getAppUrl } from '../services/notifications'; import { resolveAuthToggles } from '../services/authService'; const router = express.Router(); diff --git a/server/src/services/notifications.ts b/server/src/services/notifications.ts index 5f0fa31e..ecc9a3bc 100644 --- a/server/src/services/notifications.ts +++ b/server/src/services/notifications.ts @@ -46,13 +46,42 @@ function getSmtpConfig(): SmtpConfig | null { // Exported for use by notificationService export function getAppUrl(): string { - if (process.env.APP_URL) return process.env.APP_URL; + if (process.env.APP_URL) { + try { + const _ = new URL(process.env.APP_URL); + return process.env.APP_URL.replace(/\/+$/, ''); + } catch (_ignored) { + } + } const origins = process.env.ALLOWED_ORIGINS; if (origins) { const first = origins.split(',')[0]?.trim(); - if (first) return first.replace(/\/+$/, ''); + if (first) { + try { + const _ = new URL(first); + return first.replace(/\/+$/, ''); + } catch (_ignored) { + } + } } - const port = process.env.PORT || '3000'; + const port = Number(process.env.PORT) || 3001; + return `http://localhost:${port}`; +} + +/** Returns a URL guaranteed to satisfy the MCP SDK's issuer requirements (HTTPS or localhost). + * Falls back to http://localhost:{PORT} when APP_URL/ALLOWED_ORIGINS use a non-HTTPS, non-localhost scheme + * that would cause checkIssuerUrl to throw "Issuer URL must be HTTPS". */ +export function getMcpSafeUrl(): string { + const candidate = getAppUrl(); + try { + const u = new URL(candidate); + if (u.protocol === 'https:' || u.hostname === 'localhost' || u.hostname === '127.0.0.1') { + return candidate; + } + } catch { + // candidate was somehow invalid — fall through to localhost + } + const port = Number(process.env.PORT) || 3001; return `http://localhost:${port}`; } diff --git a/server/src/services/oauthService.ts b/server/src/services/oauthService.ts index 67ff5e58..01864156 100644 --- a/server/src/services/oauthService.ts +++ b/server/src/services/oauthService.ts @@ -6,7 +6,7 @@ import { ADDON_IDS } from '../addons'; import { User } from '../types'; import { writeAudit, logWarn } from './auditLog'; import { revokeUserSessionsForClient } from '../mcp/sessionManager'; -import { getAppUrl } from './oidcService'; +import { getMcpSafeUrl } from './notifications'; // --------------------------------------------------------------------------- // Constants @@ -587,7 +587,7 @@ export function validateAuthorizeRequest( // bind the token to the MCP endpoint by default — previously this // left `audience = null`, and the audience-bind check on MCP requests // then treated a null audience as "valid for any resource". - const mcpResource = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`; + const mcpResource = `${getMcpSafeUrl().replace(/\/+$/, '')}/mcp`; const resource = params.resource ? params.resource.replace(/\/+$/, '') : mcpResource; diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index edce054f..245e576c 100644 --- a/server/src/services/oidcService.ts +++ b/server/src/services/oidcService.ts @@ -194,14 +194,6 @@ export function generateToken(user: { id: number }): string { return jwt.sign({ id: user.id }, JWT_SECRET, { expiresIn: '24h', algorithm: 'HS256' }); } -export function getAppUrl(): string | null { - return ( - process.env.APP_URL || - (db.prepare("SELECT value FROM app_settings WHERE key = 'app_url'").get() as { value: string } | undefined)?.value || - null - ); -} - // --------------------------------------------------------------------------- // Token exchange with OIDC provider // --------------------------------------------------------------------------- diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index a0487f5a..5afb6104 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -48,7 +48,10 @@ vi.mock('../../src/services/adminService', async (importOriginal) => { return { ...actual, isAddonEnabled: isAddonEnabledMock }; }); -vi.mock('../../src/services/oidcService', () => ({ getAppUrl: () => 'https://trek.example.com' })); +vi.mock('../../src/services/notifications', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getMcpSafeUrl: () => 'https://trek.example.com' }; +}); vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); vi.mock('../../src/mcp/sessionManager', () => ({ revokeUserSessions: vi.fn(), revokeUserSessionsForClient: vi.fn(), sessions: new Map() }));