refactor(server): move instance-URL resolution to app-config

This commit is contained in:
jubnl
2026-07-28 20:21:09 +02:00
parent 292f1b181c
commit cafff003a3
29 changed files with 202 additions and 73 deletions
+12
View File
@@ -9,6 +9,7 @@ the pre-init Express middleware.
env.schema.ts Zod catalog of the whole env surface; fail-fast at boot
derive.ts pure (raw env) → typed namespace functions, exact per-site coercions
env.ts readEnv() live accessor + validateEnvAtBoot()
app-url.ts getAppUrl()/getMcpSafeUrl() instance base-URL resolution
boot-validate.ts side-effect import used by index.ts (validates before other modules load)
parsers.ts shared coercion helpers (boolTrueLoose, numberOr, csvList, …)
```
@@ -43,6 +44,17 @@ tokens, `RuntimeEnvService`) and consumes the SAME derive functions.
rate limits, …) keep freeze-at-import timing, merely sourcing the value from
`readEnv()` at module top. Request-time reads stay request-time.
## app-url.ts — instance base-URL resolution (moved 2026-07-28)
`getAppUrl()` (APP_URL → first ALLOWED_ORIGINS entry → `http://localhost:PORT`,
each candidate URL-validated, ALL trailing slashes stripped) and
`getMcpSafeUrl()` (same, sanitized to HTTPS/localhost/127.0.0.1 for the MCP
SDK's issuer check) moved here **verbatim** from `services/notifications.ts`
(audit findings `auth-2`/`notifications-1`/`admin-3`/`mcp-1`). They follow
invariant 1: `readEnv()` per call, live, never cached. The invalid-URL silent
fallthrough and the strip-ALL-slashes quirk (see `parsers.ts`
`stripTrailingSlashes`) are parity-pinned — do not "fix" them here.
## Classification: boot-stable vs runtime-toggled
**Boot-stable** (frozen at app/module creation; snapshot `registerAs`/`ConfigType`
+47
View File
@@ -0,0 +1,47 @@
import { readEnv } from './env';
/**
* Instance base-URL resolution: APP_URL → first ALLOWED_ORIGINS entry →
* http://localhost:{PORT}. Moved verbatim from services/notifications.ts
* (2026-07-28, findings auth-2/notifications-1/admin-3/mcp-1) — the resolution
* chain, trailing-slash strip and invalid-URL fallthrough are parity-pinned.
* Live reads: readEnv() per call, never cached.
*/
export function getAppUrl(): string {
const appUrl = readEnv().app.appUrl;
if (appUrl) {
try {
const _ = new URL(appUrl);
return appUrl.replace(/\/+$/, '');
} catch (_ignored) {}
}
const origins = readEnv().http.allowedOriginsRaw;
if (origins) {
const first = origins.split(',')[0]?.trim();
if (first) {
try {
const _ = new URL(first);
return first.replace(/\/+$/, '');
} catch (_ignored) {}
}
}
const port = readEnv().app.port;
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 = readEnv().app.port;
return `http://localhost:${port}`;
}
+1
View File
@@ -1,4 +1,5 @@
export { readEnv, validateEnvAtBoot } from './env';
export { getAppUrl, getMcpSafeUrl } from './app-url';
export type { AppEnv, RawEnv } from './env';
export {
deriveAll,
+1 -1
View File
@@ -61,7 +61,7 @@ export function csvListFiltered(raw: string | undefined): string[] | null {
: null;
}
/** Strip ALL trailing slashes (`/\/+$/`) — notifications.getAppUrl / TRANSIT_API_URL variant. */
/** Strip ALL trailing slashes (`/\/+$/`) — app-url.getAppUrl / TRANSIT_API_URL variant. */
export function stripTrailingSlashes(value: string): string {
return value.replace(/\/+$/, '');
}
+1 -2
View File
@@ -24,8 +24,7 @@ const tmpDir = path.join(__dirname, '../data/tmp');
});
import * as scheduler from './scheduler';
import { readEnv } from './app-config';
import { getAppUrl, getMcpSafeUrl } from './services/notifications';
import { getAppUrl, getMcpSafeUrl, readEnv } from './app-config';
const PORT = readEnv().app.port;
const HOST = readEnv().app.host;
+1 -1
View File
@@ -12,7 +12,7 @@ import { registerTools } from './tools';
import { McpSession, sessions, revokeUserSessions, revokeUserSessionsForClient, evictOldestSessionForUser } from './sessionManager';
import { readEnv } from '../app-config';
import { writeAudit, getClientIp } from '../nest/audit/audit.bridge';
import { getMcpSafeUrl } from '../services/notifications';
import { getMcpSafeUrl } from '../app-config';
export { revokeUserSessions, revokeUserSessionsForClient };
+1 -1
View File
@@ -16,7 +16,7 @@ import {
getUserByAccessToken,
} from '../services/oauthService';
import { ALL_SCOPES } from './scopes';
import { getMcpSafeUrl } from '../services/notifications';
import { getMcpSafeUrl } from '../app-config';
import { writeAudit } from '../nest/audit/audit.bridge';
// ---------------------------------------------------------------------------
+2 -1
View File
@@ -2,7 +2,8 @@ import { Injectable } from '@nestjs/common';
import type { Request, Response } from 'express';
import * as auth from '../../services/authService';
import { setAuthCookie, clearAuthCookie } from '../../services/cookie';
import { sendPasswordResetEmail, getAppUrl } from '../../services/notifications';
import { sendPasswordResetEmail } from '../../services/notifications';
import { getAppUrl } from '../../app-config';
import type { User } from '../../types';
/**
+1 -1
View File
@@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common';
import * as oauth from '../../services/oauthService';
import { isAddonEnabled } from '../../services/adminService';
import { ADDON_IDS } from '../../addons';
import { getMcpSafeUrl } from '../../services/notifications';
import { getMcpSafeUrl } from '../../app-config';
/**
* Thin Nest wrapper around the existing OAuth 2.1 service. The grant handling,
+1 -1
View File
@@ -1,7 +1,7 @@
import { Injectable } from '@nestjs/common';
import type { Request, Response } from 'express';
import * as oidc from '../../services/oidcService';
import { getAppUrl } from '../../services/notifications';
import { getAppUrl } from '../../app-config';
import { resolveAuthToggles } from '../../services/authService';
import { setAuthCookie } from '../../services/cookie';
+3 -3
View File
@@ -14,7 +14,7 @@ import { mcpAuthMetadataRouter } from '@modelcontextprotocol/sdk/server/auth/rou
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';
import { getMcpSafeUrl } from '../../app-config';
// Platform / transport routes extracted verbatim from createApp() (app.ts) so they can be
// mounted on either the legacy Express app or the NestJS Express instance (strangler A6/A8).
@@ -126,8 +126,8 @@ export function applyPlatformTransport(app: express.Application): void {
next();
};
// SDK metadata router — built lazily on first request so getAppUrl() (which queries the DB)
// is not called at createApp() time, before test tables have been created.
// SDK metadata router — built lazily on first request so the issuer URL is
// resolved from the live env, not frozen at createApp() time.
// mcpAuthMetadataRouter serves:
// /.well-known/oauth-authorization-server — RFC 8414 AS metadata
// /.well-known/oauth-protected-resource/mcp — RFC 9728 path-based PRM (fixes issue #959 bug 1)
@@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common';
import crypto from 'node:crypto';
import { DatabaseService } from '../database/database.service';
import { encrypt_api_key, decrypt_api_key } from '../../services/apiKeyCrypto';
import { getAppUrl } from '../../services/notifications';
import { getAppUrl } from '../../app-config';
import { isPrivateIp } from './install/safe-fetch';
import { safeFetchLlm } from '../../utils/ssrfGuard';
+1 -1
View File
@@ -2,7 +2,7 @@ import { readEnv } from '../app-config';
import { db } from '../db/database';
import { safeFetchFollow, SsrfBlockedError } from '../utils/ssrfGuard';
import { decrypt_api_key } from './apiKeyCrypto';
import { getAppUrl } from './notifications';
import { getAppUrl } from '../app-config';
// ── Photo cache (disk-backed) ────────────────────────────────────────────────
import * as placePhotoCache from './placePhotoCache';
+2 -1
View File
@@ -9,7 +9,8 @@ import {
ADMIN_SCOPED_EVENTS,
type NotifEventType,
} from './notificationPreferencesService';
import { getEventText, getUserLanguage, getAppUrl } from './notifications';
import { getEventText, getUserLanguage } from './notifications';
import { getAppUrl } from '../app-config';
import { listChannels, type ChannelMessage, type ExternalChannel } from './notifications/channelRegistry';
import {
resolveRecipients,
+1 -41
View File
@@ -1,4 +1,4 @@
import { readEnv } from '../app-config';
import { getAppUrl, readEnv } from '../app-config';
import { db } from '../db/database';
import { checkSsrf, createPinnedDispatcher } from '../utils/ssrfGuard';
import { decrypt_api_key } from './apiKeyCrypto';
@@ -68,46 +68,6 @@ function getSmtpConfig(): SmtpConfig | null {
};
}
// Exported for use by notificationService
export function getAppUrl(): string {
const appUrl = readEnv().app.appUrl;
if (appUrl) {
try {
const _ = new URL(appUrl);
return appUrl.replace(/\/+$/, '');
} catch (_ignored) {}
}
const origins = readEnv().http.allowedOriginsRaw;
if (origins) {
const first = origins.split(',')[0]?.trim();
if (first) {
try {
const _ = new URL(first);
return first.replace(/\/+$/, '');
} catch (_ignored) {}
}
}
const port = readEnv().app.port;
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 = readEnv().app.port;
return `http://localhost:${port}`;
}
/** Is SMTP configured at the instance level? (Independent of any one user's address.) */
export function isSmtpConfigured(): boolean {
return !!(readEnv().smtp.host || getAppSetting('smtp_host'));
+1 -1
View File
@@ -6,7 +6,7 @@ import { ADDON_IDS } from '../addons';
import { User } from '../types';
import { writeAudit, logWarn } from '../nest/audit/audit.bridge';
import { revokeUserSessionsForClient } from '../mcp/sessionManager';
import { getMcpSafeUrl } from './notifications';
import { getMcpSafeUrl } from '../app-config';
// ---------------------------------------------------------------------------
// Constants
+1 -1
View File
@@ -1,6 +1,6 @@
import { readEnv } from '../app-config';
import { buildUserAgent } from './mapsService';
import { getAppUrl } from './notifications';
import { getAppUrl } from '../app-config';
/**
* Public transit routing (#1065) backed by Transitous (api.transitous.org), the
+1 -1
View File
@@ -1,6 +1,6 @@
import { readEnv } from '../app-config';
import { db } from '../db/database';
import { getAppUrl } from './notifications';
import { getAppUrl } from '../app-config';
/**
* Resolves the WebAuthn Relying Party ID + allowed origins for this deployment.
+5 -1
View File
@@ -32,7 +32,11 @@ vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: (
// The audit domain is DI-native now: writeAudit runs for real against the temp
// db's audit_log table; only the file logger is silenced.
vi.mock('../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logDebug: vi.fn(), logError: vi.fn(), logWarn: vi.fn() }));
vi.mock('../../src/services/notifications', () => ({ getAppUrl: () => 'https://x', sendPasswordResetEmail: vi.fn().mockResolvedValue({ delivered: true }) }));
vi.mock('../../src/services/notifications', () => ({ sendPasswordResetEmail: vi.fn().mockResolvedValue({ delivered: true }) }));
vi.mock('../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/app-config')>();
return { ...actual, getAppUrl: () => 'https://x' };
});
const { authSvc } = vi.hoisted(() => ({
authSvc: {
+4 -1
View File
@@ -31,7 +31,10 @@ vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: (
// The audit domain is DI-native now: writeAudit runs for real against the temp
// db's audit_log table; only the file logger is silenced.
vi.mock('../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logDebug: vi.fn(), logError: vi.fn(), logWarn: vi.fn() }));
vi.mock('../../src/services/notifications', () => ({ getMcpSafeUrl: () => 'https://app' }));
vi.mock('../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/app-config')>();
return { ...actual, getMcpSafeUrl: () => 'https://app' };
});
const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn(() => true) }));
vi.mock('../../src/services/adminService', () => ({ isAddonEnabled }));
+4 -1
View File
@@ -10,7 +10,10 @@ import cookieParser from 'cookie-parser';
import type { Server } from 'http';
import { Test } from '@nestjs/testing';
vi.mock('../../src/services/notifications', () => ({ getAppUrl: () => 'https://app' }));
vi.mock('../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/app-config')>();
return { ...actual, getAppUrl: () => 'https://app' };
});
const { toggles } = vi.hoisted(() => ({ toggles: { oidc_login: true } }));
vi.mock('../../src/services/authService', () => ({ resolveAuthToggles: () => toggles }));
+2 -2
View File
@@ -53,8 +53,8 @@ vi.mock('../../src/services/adminService', async (importOriginal) => {
return { ...actual, isAddonEnabled: isAddonEnabledMock };
});
vi.mock('../../src/services/notifications', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/services/notifications')>();
vi.mock('../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../src/app-config')>();
return { ...actual, getMcpSafeUrl: () => 'https://trek.example.com' };
});
@@ -0,0 +1,82 @@
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import { getAppUrl, getMcpSafeUrl } from '../../../src/app-config/app-url';
// These tests PIN the exact behavior moved verbatim from
// services/notifications.ts (2026-07-28): the APP_URL → ALLOWED_ORIGINS →
// localhost fallback chain, the strip-ALL-trailing-slashes quirk, the
// invalid-URL silent fallthrough, and getMcpSafeUrl's HTTPS/localhost issuer
// sanitization. If one fails after an edit, the edit changed runtime
// behavior — fix the edit, not the test (parity is law).
const KEYS = ['APP_URL', 'ALLOWED_ORIGINS', 'PORT'] as const;
const saved: Record<string, string | undefined> = {};
beforeEach(() => {
for (const k of KEYS) {
saved[k] = process.env[k];
delete process.env[k];
}
});
afterEach(() => {
for (const k of KEYS) {
if (saved[k] === undefined) delete process.env[k];
else process.env[k] = saved[k];
}
});
describe('getAppUrl', () => {
it('returns APP_URL with ALL trailing slashes stripped', () => {
process.env.APP_URL = 'https://trek.example.com///';
expect(getAppUrl()).toBe('https://trek.example.com');
});
it('falls through an invalid APP_URL to the first ALLOWED_ORIGINS entry', () => {
process.env.APP_URL = 'not a url';
process.env.ALLOWED_ORIGINS = 'https://a.example/, https://b.example';
expect(getAppUrl()).toBe('https://a.example');
});
it('uses the first ALLOWED_ORIGINS entry (trimmed, slashes stripped) when APP_URL is unset', () => {
process.env.ALLOWED_ORIGINS = ' https://origin.example// ,https://other.example';
expect(getAppUrl()).toBe('https://origin.example');
});
it('falls back to http://localhost:{PORT} when neither is usable', () => {
process.env.ALLOWED_ORIGINS = 'not-a-url';
process.env.PORT = '4123';
expect(getAppUrl()).toBe('http://localhost:4123');
});
it('defaults to port 3001 with no env at all', () => {
expect(getAppUrl()).toBe('http://localhost:3001');
});
it('reads the env live — no caching across calls', () => {
process.env.APP_URL = 'https://first.example';
expect(getAppUrl()).toBe('https://first.example');
process.env.APP_URL = 'https://second.example';
expect(getAppUrl()).toBe('https://second.example');
});
});
describe('getMcpSafeUrl', () => {
it('passes through HTTPS URLs', () => {
process.env.APP_URL = 'https://trek.example.com';
expect(getMcpSafeUrl()).toBe('https://trek.example.com');
});
it('passes through http://localhost and http://127.0.0.1', () => {
process.env.APP_URL = 'http://localhost:8080';
expect(getMcpSafeUrl()).toBe('http://localhost:8080');
process.env.APP_URL = 'http://127.0.0.1:8080';
expect(getMcpSafeUrl()).toBe('http://127.0.0.1:8080');
});
it('sanitizes non-HTTPS, non-localhost URLs to http://localhost:{PORT}', () => {
process.env.APP_URL = 'http://trek.internal.lan';
process.env.PORT = '3005';
expect(getMcpSafeUrl()).toBe('http://localhost:3005');
});
});
+5 -2
View File
@@ -29,7 +29,10 @@ const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn() }));
vi.mock('../../../src/services/adminService', () => ({ isAddonEnabled }));
const { getMcpSafeUrl } = vi.hoisted(() => ({ getMcpSafeUrl: vi.fn() }));
vi.mock('../../../src/services/notifications', () => ({ getMcpSafeUrl }));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getMcpSafeUrl };
});
import { OauthService } from '../../../src/nest/oauth/oauth.service';
import { ADDON_IDS } from '../../../src/addons';
@@ -47,7 +50,7 @@ describe('OauthService', () => {
expect(svc().mcpEnabled()).toBe(false);
});
it('mcpSafeUrl forwards to the notifications helper', () => {
it('mcpSafeUrl forwards to the app-config helper', () => {
getMcpSafeUrl.mockReturnValue('https://safe');
expect(svc().mcpSafeUrl()).toBe('https://safe');
expect(getMcpSafeUrl).toHaveBeenCalled();
+5 -2
View File
@@ -24,7 +24,10 @@ const { oidc } = vi.hoisted(() => ({
vi.mock('../../../src/services/oidcService', () => oidc);
const { getAppUrl } = vi.hoisted(() => ({ getAppUrl: vi.fn() }));
vi.mock('../../../src/services/notifications', () => ({ getAppUrl }));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getAppUrl };
});
const { resolveAuthToggles } = vi.hoisted(() => ({ resolveAuthToggles: vi.fn() }));
vi.mock('../../../src/services/authService', () => ({ resolveAuthToggles }));
@@ -54,7 +57,7 @@ describe('OidcService', () => {
expect(s.getOidcConfig()).toBe(cfg);
});
it('getAppUrl delegates to notifications.getAppUrl', () => {
it('getAppUrl delegates to app-config getAppUrl', () => {
getAppUrl.mockReturnValue('https://app');
expect(s.getAppUrl()).toBe('https://app');
});
@@ -21,7 +21,10 @@ vi.mock('../../../src/db/database', () => ({ db: { prepare: h.dbPrepare } }));
vi.mock('../../../src/mcp', () => ({ mcpHandler: h.mcpHandler }));
vi.mock('../../../src/mcp/oauthProvider', () => ({ trekOAuthProvider: {}, trekClientsStore: {} }));
vi.mock('../../../src/services/adminService', () => ({ isAddonEnabled: h.isAddonEnabled }));
vi.mock('../../../src/services/notifications', () => ({ getMcpSafeUrl: h.getMcpSafeUrl }));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getMcpSafeUrl: h.getMcpSafeUrl };
});
// SDK router/handler factories return distinct tagged middleware so we never hit
// real new URL(...) wiring during registration.
@@ -10,7 +10,10 @@ vi.mock('../../../src/services/apiKeyCrypto', () => ({
encrypt_api_key: (v: unknown) => (typeof v === 'string' ? `enc:${v}` : v),
decrypt_api_key: (v: unknown) => (typeof v === 'string' && v.startsWith('enc:') ? v.slice(4) : v),
}));
vi.mock('../../../src/services/notifications', () => ({ getAppUrl: () => 'https://trek.example' }));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getAppUrl: () => 'https://trek.example' };
});
const { getDb } = vi.hoisted(() => ({ getDb: { current: null as unknown } }));
vi.mock('../../../src/db/database', () => ({ get db() { return getDb.current; } }));
@@ -7,7 +7,10 @@ import { deriveTransitStats, geocode, plan, type TransitLeg } from '../../../src
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
vi.mock('../../../src/services/notifications', () => ({ getAppUrl: () => 'https://trek.example.com' }));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getAppUrl: () => 'https://trek.example.com' };
});
vi.mock('../../../src/services/mapsService', () => ({ buildUserAgent: () => 'TREK-Test-UA' }));
const fetchMock = vi.fn();
@@ -24,9 +24,10 @@ vi.mock('../../../src/db/database', () => ({
},
}));
vi.mock('../../../src/services/notifications', () => ({
getAppUrl: () => appUrlRef.value,
}));
vi.mock('../../../src/app-config', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../../src/app-config')>();
return { ...actual, getAppUrl: () => appUrlRef.value };
});
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { resolveWebauthnConfig, isPasskeyConfigured } from '../../../src/services/webauthnConfig';