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

@@ -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);
   });
 

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,12 +30,15 @@ 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}`,
+    `  Container Port: ${PORT}`,
+    `  App URL:        ${appUrl}`,
     `  Environment:    ${process.env.NODE_ENV?.toLowerCase() || 'development'}`,
     `  Timezone:       ${tz}`,
     `  Origins:        ${origins}`,
@@ -45,6 +49,23 @@ const onListen = () => {
     '──────────────────────────────────────',
   ];
   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!');

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 };
   }

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<void> {
-        const mcpResource = `${(getAppUrl() || '').replace(/\/+$/, '')}/mcp`;
+        const mcpResource = `${getMcpSafeUrl().replace(/\/+$/, '')}/mcp`;
         const resource = params.resource ? params.resource.href.replace(/\/+$/, '') : mcpResource;
 
         if (resource !== mcpResource) {

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();

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}`;
 }
 

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;

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
 // ---------------------------------------------------------------------------

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<typeof import('../../src/services/notifications')>();
+    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() }));