diff --git a/server/src/mcp/tools.ts b/server/src/mcp/tools.ts index 62eb38673..0fc5ce0fd 100644 --- a/server/src/mcp/tools.ts +++ b/server/src/mcp/tools.ts @@ -1,69 +1,17 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; import type { McpAttachOptions, McpRegistry } from '../nest-mcp'; +/** + * Attaches the MCP surface to a session's server. + * + * Every domain registers through an @McpController class the container + * discovers, so there is nothing to wire here by hand any more. What is left + * is the seam that supplies the default arguments and the null-registry + * escape hatch. Production passes the container-discovered McpRegistryService + * (injected into the transport service); the no-Nest harness hands in + * createTestRegistry()'s build. A null registry (direct callers without either) + * skips the attach. + */ export function registerTools(registry: McpRegistry | null, server: McpServer, userId: number, scopes: string[] | null, isStaticToken = false, getDeprecationNotice: () => string | null = () => null, onInvoke?: McpAttachOptions['onInvoke']): void { - // The trip tools moved to the DI-discovered src/nest/trips/trips.mcp.ts and - // the share-link tools to src/nest/share/share.mcp.ts (@McpController, - // attached via the nest-mcp registry below — getDeprecationNotice rides the - // attach ctx). - - // The place tools moved to the DI-discovered src/nest/places/places.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The collection tools moved to the DI-discovered src/nest/collections/ - // collections.mcp.ts (@McpController, attached via the nest-mcp registry below). - - // The budget tools moved to the DI-discovered src/nest/budget/budget.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The packing tools moved to the DI-discovered src/nest/packing/packing.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The reservation tools moved to the DI-discovered src/nest/reservations/ - // reservations.mcp.ts (@McpController, attached via the nest-mcp registry below). - - // The day + accommodation tools moved to the DI-discovered src/nest/days/ - // days.mcp.ts (@McpController, attached via the nest-mcp registry below). - - // The assignment tools moved to the DI-discovered src/nest/assignments/ - // assignments.mcp.ts (@McpController, attached via the nest-mcp registry below). - - // The weather tools moved to the DI-discovered src/nest/weather/weather.mcp.ts - // and the airport tools to src/nest/airports/airports.mcp.ts (@McpController, - // attached via the nest-mcp registry below). - - // The notification tools moved to the DI-discovered src/nest/notifications/ - // notifications.mcp.ts (@McpController, attached via the nest-mcp registry below). - - // The atlas tools moved to the DI-discovered src/nest/atlas/atlas.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The collab tools moved to the DI-discovered src/nest/collab/collab.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The transport tools moved to the DI-discovered - // src/nest/reservations/reservations.mcp.ts — a transport is a reservation, - // same table and same service (@McpController, attached via the nest-mcp - // registry below). - - // The transit tools moved to the DI-discovered src/nest/transit/transit.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - - // The vacay tools moved to the DI-discovered src/nest/vacay/vacay.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The todo tools moved to the DI-discovered src/nest/todo/todo.mcp.ts - // (@McpController, attached via the nest-mcp registry below). - - // The prompts moved to the DI-discovered @McpController classes: - // packing-list to packing.mcp.ts, budget-overview to budget.mcp.ts and the - // static-token notice to auth.mcp.ts (its `if (isStaticToken)` became a - // `when` gate — the registry hands `when` the session context). - - // Decorator-registered domains (src/nest-mcp). Production passes the - // container-discovered McpRegistryService (injected into the transport - // service); the no-Nest harness hands in createTestRegistry()'s build. - // A null registry (direct callers without either) ⇒ skip. if (registry) registry.attach(server, { userId, scopes, isStaticToken, getDeprecationNotice }, { onInvoke }); } diff --git a/server/src/middleware/globalMiddleware.ts b/server/src/middleware/globalMiddleware.ts index cb7652005..aaca45557 100644 --- a/server/src/middleware/globalMiddleware.ts +++ b/server/src/middleware/globalMiddleware.ts @@ -30,13 +30,23 @@ export const SENSITIVE_KEYS = new Set([ 'secretaccesskey', ]); +/** + * Suffixes that make a field sensitive whatever its prefix. Matched on the end of + * the key, not anywhere in it: `accessKeyId` is an identifier and stays readable, + * while `refresh_token`, `code_verifier`, `mapbox_access_token` and the whole + * `*_api_key` family are secrets nobody needs in a debug log. + */ +const SENSITIVE_SUFFIXES = ['_token', '_key', '_secret', '_password', '_pass', '_verifier']; + /** Deep-redacts every key in `SENSITIVE_KEYS` (case-insensitive) from a request-log value. */ export function redact(value: unknown): unknown { if (!value || typeof value !== 'object') return value; if (Array.isArray(value)) return (value as unknown[]).map(redact); const out: Record = {}; for (const [k, v] of Object.entries(value as Record)) { - out[k] = SENSITIVE_KEYS.has(k.toLowerCase()) ? '[REDACTED]' : redact(v); + const lower = k.toLowerCase(); + const sensitive = SENSITIVE_KEYS.has(lower) || SENSITIVE_SUFFIXES.some((suffix) => lower.endsWith(suffix)); + out[k] = sensitive ? '[REDACTED]' : redact(v); } return out; } @@ -155,7 +165,7 @@ export function applyGlobalMiddleware( contentSecurityPolicy: { directives: { defaultSrc: ["'self'"], - scriptSrc: ["'self'", "'wasm-unsafe-eval'", "'unsafe-eval'"], + scriptSrc: ["'self'", "'wasm-unsafe-eval'"], styleSrc: ["'self'", "'unsafe-inline'", "https://fonts.googleapis.com", "https://unpkg.com"], imgSrc: ["'self'", "data:", "blob:", "https:"], connectSrc: [ @@ -197,11 +207,24 @@ export function applyGlobalMiddleware( referrerPolicy: { policy: 'strict-origin-when-cross-origin' }, })); + // The instance's own hostname, when the operator configured one. The redirect + // below is a 301, so echoing a client-supplied Host header would let a stranger + // park a foreign origin in a visitor's cache. Falls back to the request Host for + // instances that never set APP_URL, which is what it always did. + const configuredHost = (() => { + const url = readEnv().app.appUrl; + try { + return url ? new URL(url).host : null; + } catch { + return null; + } + })(); + if (shouldForceHttps) { app.use((req: Request, res: Response, next: NextFunction) => { if (req.path === '/api/health') return next(); if (req.secure || req.headers['x-forwarded-proto'] === 'https') return next(); - res.redirect(301, 'https://' + req.headers.host + req.url); + res.redirect(301, 'https://' + (configuredHost ?? req.headers.host) + req.url); }); } diff --git a/server/src/nest-mcp/types.ts b/server/src/nest-mcp/types.ts index 7125bf81c..31b6c0485 100644 --- a/server/src/nest-mcp/types.ts +++ b/server/src/nest-mcp/types.ts @@ -25,7 +25,8 @@ export interface McpContext { isStaticToken: boolean; /** * Fire-once static-token deprecation notice closure (built per session in - * src/mcp/index.ts and threaded through registerTools → registry.attach). + * src/nest/mcp-transport/mcp-transport.service.ts and threaded through + * registerTools → registry.attach). * Optional so direct createTestRegistry ctxs without it keep working — * consumers (list_trips / get_trip_summary) treat absence as "no notice". */ @@ -38,11 +39,10 @@ export interface McpContext { * a group come from the access policy in `src/mcp/nest-mcp-policy.ts`, not * from here. */ -// The registry pattern requires an empty single-extends interface — the -// keys ARE the contract, so the lint's "no members" finding is a false -// positive. -// eslint-disable-next-line @typescript-eslint/no-empty-object-type -export interface McpAccessGroupRegistry extends Record {} +// A plain alias, not an empty interface: the empty single-extends form only +// existed so an external host could augment the union, and since the fold into +// the server there is no such host. `keyof` resolves the same either way. +export type McpAccessGroupRegistry = Record; export type McpAccessGroup = keyof McpAccessGroupRegistry extends never ? string @@ -54,8 +54,7 @@ export type McpAccessGroup = keyof McpAccessGroupRegistry extends never * could only be expressed as opaque predicates, which the boot gate in * `src/mcp/nest-mcp-policy.ts` cannot check. */ -// eslint-disable-next-line @typescript-eslint/no-empty-object-type -export interface McpAccessModeRegistry extends Record {} +export type McpAccessModeRegistry = Record; export type McpAccessMode = keyof McpAccessModeRegistry extends never ? 'read' | 'write' diff --git a/server/src/nest/audit/audit.service.ts b/server/src/nest/audit/audit.service.ts index 58ff13546..b077ae8df 100644 --- a/server/src/nest/audit/audit.service.ts +++ b/server/src/nest/audit/audit.service.ts @@ -26,6 +26,13 @@ const ACTION_LABELS: Record = { 'immich.private_ip_configured': 'configured Immich with private IP', }; +/** + * Collapses line breaks so a trip title (free user text) cannot forge a second + * log line. Scoped to the audit lines rather than the shared logger, which + * still has to print multi-line stack traces as they are. + */ +const oneLine = (s: string): string => s.replace(/[\r\n\u2028\u2029]+/g, ' '); + function buildInfoSummary(action: string, details?: Record): string { if (!details || Object.keys(details).length === 0) return ''; if (action === 'trip.create') return ` "${details.title}"`; @@ -87,12 +94,12 @@ export class AuditService { const email = this.resolveUserEmail(entry.userId); const label = ACTION_LABELS[entry.action] || entry.action; const brief = buildInfoSummary(entry.action, entry.details); - logInfo(`${email} ${label}${brief} ip=${entry.ip || '-'}`); + logInfo(oneLine(`${email} ${label}${brief} ip=${entry.ip || '-'}`)); if (entry.debugDetails && Object.keys(entry.debugDetails).length > 0) { - logDebug(`AUDIT ${entry.action} userId=${entry.userId} ${JSON.stringify(entry.debugDetails)}`); + logDebug(oneLine(`AUDIT ${entry.action} userId=${entry.userId} ${JSON.stringify(entry.debugDetails)}`)); } else if (detailsJson) { - logDebug(`AUDIT ${entry.action} userId=${entry.userId} ${detailsJson}`); + logDebug(oneLine(`AUDIT ${entry.action} userId=${entry.userId} ${detailsJson}`)); } } catch (e) { logError(`Audit write failed: ${e instanceof Error ? e.message : e}`); diff --git a/server/src/nest/audit/client-ip.ts b/server/src/nest/audit/client-ip.ts index 8e44a77a5..7d01798b9 100644 --- a/server/src/nest/audit/client-ip.ts +++ b/server/src/nest/audit/client-ip.ts @@ -4,13 +4,13 @@ import { Request } from 'express'; * Pure request helper — no DB, no side effects — kept out of the injectable * AuditService so getClientIp-only consumers stay plain-function imports * (same carve-out precedent as files.constants.ts). + * + * It honours `trust proxy` (set in globalMiddleware from TRUST_PROXY, one hop by + * default) rather than reading X-Forwarded-For itself. The leftmost entry of + * that header is whatever the client typed, and this value ends up on the audit + * rows an operator reads after an incident — the one place a caller must not be + * able to dictate. With trust proxy off, req.ip is the socket address anyway. */ export function getClientIp(req: Request): string | null { - const xff = req.headers['x-forwarded-for']; - if (typeof xff === 'string') { - const first = xff.split(',')[0]?.trim(); - return first || null; - } - if (Array.isArray(xff) && xff[0]) return String(xff[0]).trim() || null; - return req.socket?.remoteAddress || null; + return req.ip?.trim() || req.socket?.remoteAddress || null; } diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index e63a732df..4b57cfa92 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -307,7 +307,13 @@ export class AuthController { @Post('ws-token') @HttpCode(200) - wsToken(@CurrentUser() user: User) { + wsToken(@CurrentUser() user: User, @Req() req: Request) { + // Own bucket, not 'login': a client that reconnects its socket in a loop + // must not be able to lock itself out of signing in. The ceiling is far + // above any real client, which mints one token per socket connect, but it + // stops a single account from filling the process-wide ephemeral store and + // 503-ing every other user's ws and download tokens. + this.limit('ws_token', req, 120); const result = this.tokens.createWsToken(user.id); if (result.error) { throw new HttpException({ error: result.error }, result.status!); @@ -317,7 +323,8 @@ export class AuthController { @Post('resource-token') @HttpCode(200) - resourceToken(@CurrentUser() user: User, @Body() body: ResourceTokenDto) { + resourceToken(@CurrentUser() user: User, @Body() body: ResourceTokenDto, @Req() req: Request) { + this.limit('resource_token', req, 120); const token = this.tokens.createResourceToken(user.id, body.purpose); if (!token) { throw new HttpException({ error: 'Service unavailable' }, 503); diff --git a/server/src/nest/common/idempotency.interceptor.ts b/server/src/nest/common/idempotency.interceptor.ts index 7908f65e3..77a9ec4ed 100644 --- a/server/src/nest/common/idempotency.interceptor.ts +++ b/server/src/nest/common/idempotency.interceptor.ts @@ -1,6 +1,7 @@ import { CallHandler, ExecutionContext, HttpException, Injectable, NestInterceptor } from '@nestjs/common'; import type { Request, Response } from 'express'; -import { Observable, of } from 'rxjs'; +import { Observable, from, of } from 'rxjs'; +import { finalize, switchMap } from 'rxjs/operators'; import { DatabaseService } from '../database/database.service'; /** @@ -16,8 +17,17 @@ import { DatabaseService } from '../database/database.service'; * - non-mutating method, or no key, or no authenticated user -> pass through * - key longer than the cap -> 400 with the exact legacy message * - (key, user, method, path) already stored -> replay the cached response + * - the same key still in flight -> wait for it, then replay its response * - otherwise -> capture a successful JSON response under the key * + * The in-flight step is the one thing the Express wrapper did not do. The row + * only exists once the first request answers, so two overlapping replays of one + * key (two tabs draining the same offline queue, or a client retrying after a + * timeout) both missed the SELECT and both ran the handler — the duplicate + * write the key exists to prevent. Waiting keeps the promise the client was + * given: the second caller gets the first one's response, not a new error to + * interpret. + * * Capturing wraps `res.json`, so 204 / `res.end()` responses are not cached — * matching the Express wrapper, which only fires on `res.json`. */ @@ -26,6 +36,14 @@ const MUTATING_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); const MAX_KEY_LENGTH = 128; const MAX_CACHED_BODY_BYTES = 256 * 1024; +/** + * (user, method, path, key) of every request currently running, resolved when it + * answers. In memory rather than a reservation row on purpose: better-sqlite3 is + * synchronous and the whole overlap lives inside one process, so a crash cannot + * leave a key wedged for the table's 30-day TTL. + */ +const inFlight = new Map>(); + interface IdempotencyRow { status_code: number; response_body: string; @@ -53,17 +71,51 @@ export class IdempotencyInterceptor implements NestInterceptor { throw new HttpException({ error: 'X-Idempotency-Key exceeds maximum length of 128 characters' }, 400); } - // Scope the lookup by method + path as well as user, so the same key replayed - // against a different endpoint can't return an unrelated cached body. - const existing = this.database.get( + const existing = this.lookup(key, userId, req); + if (existing) return this.replay(existing, res); + + const signature = `${userId}|${req.method}|${req.path}|${key}`; + const pending = inFlight.get(signature); + if (pending) { + return from(pending).pipe( + switchMap(() => { + const stored = this.lookup(key, userId, req); + if (stored) return this.replay(stored, res); + // The first request answered without caching anything (it failed, or + // it never went through res.json). Run this one normally rather than + // inventing a response for it. + return this.run(signature, key, userId, req, res, next); + }), + ); + } + + return this.run(signature, key, userId, req, res, next); + } + + /** + * Scope the lookup by method + path as well as user, so the same key replayed + * against a different endpoint can't return an unrelated cached body. + */ + private lookup(key: string, userId: number, req: Request): IdempotencyRow | undefined { + return this.database.get( 'SELECT status_code, response_body FROM idempotency_keys WHERE key = ? AND user_id = ? AND method = ? AND path = ?', key, userId, req.method, req.path, ); - if (existing) { - res.status(existing.status_code); - return of(JSON.parse(existing.response_body)); - } + } + private replay(row: IdempotencyRow, res: Response): Observable { + res.status(row.status_code); + return of(JSON.parse(row.response_body)); + } + + private run( + signature: string, + key: string, + userId: number, + req: Request, + res: Response, + next: CallHandler, + ): Observable { const originalJson = res.json.bind(res); const database = this.database; res.json = function (body: unknown): Response { @@ -84,6 +136,14 @@ export class IdempotencyInterceptor implements NestInterceptor { return originalJson(body); }; - return next.handle(); + let done!: () => void; + inFlight.set(signature, new Promise((resolve) => { done = resolve; })); + + return next.handle().pipe( + finalize(() => { + inFlight.delete(signature); + done(); + }), + ); } } diff --git a/server/src/nest/common/rate-limit.service.ts b/server/src/nest/common/rate-limit.service.ts index 04239a0c8..eae580588 100644 --- a/server/src/nest/common/rate-limit.service.ts +++ b/server/src/nest/common/rate-limit.service.ts @@ -7,14 +7,19 @@ interface Attempt { count: number; first: number } * `rateLimiter`. Each named bucket keeps its own attempt map; `check` returns * false once a key exceeds `max` within `windowMs` (the caller answers 429). * - * The legacy route also ran a setInterval to garbage-collect expired records; - * that was pure memory housekeeping (the window check below already treats an - * expired record as fresh), so it is intentionally omitted — the limit - * behaviour is identical and there's no dangling timer to leak in tests. + * The legacy route ran a setInterval to garbage-collect expired records. There + * is no timer here — a dangling one leaks in tests — but the housekeeping is + * back as a lazy sweep driven by the `now` the caller already passes: keys are + * per-IP and proxy-header derived, so without it a bucket grows for every + * address that ever knocked and never shrinks again. An expired record is + * treated as fresh by the window check below, so dropping it changes nothing + * about who gets a 429. */ @Injectable() export class RateLimitService { private readonly buckets = new Map>(); + /** Last sweep per bucket, so a busy bucket doesn't keep a quiet one from being cleaned. */ + private readonly lastSweep = new Map(); private store(bucket: string): Map { let s = this.buckets.get(bucket); @@ -25,6 +30,7 @@ export class RateLimitService { /** Returns true when the request is allowed, false when it should be rejected (429). */ check(bucket: string, key: string, max: number, windowMs: number, now: number): boolean { const store = this.store(bucket); + this.sweep(bucket, store, windowMs, now); const record = store.get(key); if (record && record.count >= max && now - record.first < windowMs) { return false; @@ -37,9 +43,18 @@ export class RateLimitService { return true; } + /** Drops records whose window has elapsed, at most once per window per bucket. */ + private sweep(bucket: string, store: Map, windowMs: number, now: number): void { + if (now - (this.lastSweep.get(bucket) ?? 0) < windowMs) return; + for (const [k, record] of store) { + if (now - record.first >= windowMs) store.delete(k); + } + this.lastSweep.set(bucket, now); + } + /** Test helper: clear a bucket (mirrors the legacy exported maps used for resets). */ reset(bucket?: string): void { if (bucket) this.buckets.get(bucket)?.clear(); - else this.buckets.clear(); + else { this.buckets.clear(); this.lastSweep.clear(); } } } diff --git a/server/src/nest/mcp-transport/mcp-transport.service.ts b/server/src/nest/mcp-transport/mcp-transport.service.ts index 18d3e56ff..8a71d062b 100644 --- a/server/src/nest/mcp-transport/mcp-transport.service.ts +++ b/server/src/nest/mcp-transport/mcp-transport.service.ts @@ -78,6 +78,15 @@ export function jsonRpcError(message: string, code = -32000): object { return { jsonrpc: '2.0', error: { code, message }, id: null }; } +/** Scope sets as the session froze them: null (full access) only matches null. */ +export function sameScopes(a: string[] | null, b: string[] | null): boolean { + if (a === null || b === null) return a === b; + if (a.length !== b.length) return false; + const left = [...a].sort(); + const right = [...b].sort(); + return left.every((scope, i) => scope === right[i]); +} + export function setAuthChallenge(res: Response, error = 'invalid_token'): void { const base = (getMcpSafeUrl() || '').replace(/\/+$/, ''); // RFC 9728 §5: resource with path component /mcp → PRM URL must include the path @@ -176,6 +185,15 @@ export class McpTransportService { res.status(403).json({ error: 'Session was created with a different OAuth client' }); return; } + // The tool surface is frozen at initialize (registerTools takes the scopes + // the session was created with), so a token that was later narrowed would + // otherwise ride the old session's full set of tools. Refusing here makes + // the client re-initialize, which is what re-authorizing should cost. + if (!sameScopes(session.scopes, scopes)) { + setAuthChallenge(res, 'insufficient_scope'); + res.status(403).json({ error: 'Session was created with different scopes' }); + return; + } session.lastActivity = Date.now(); session.lastClientIp = getClientIp(req); armSseKeepalive(res, () => { session.lastActivity = Date.now(); }); @@ -273,6 +291,17 @@ export class McpTransportService { sessionIdGenerator: () => randomUUID(), onsessioninitialized: (sid) => { sessions.set(sid, { server, transport, userId: user.id, scopes, clientId, isStaticToken, lastActivity: Date.now(), lastClientIp: createIp }); + // The cap was checked before the handler ran, and registration only + // happens here — so concurrent initializes all passed the same + // pre-registration count. Re-check now that the entry exists, or the + // overshoot stays in the map (each entry holding a full McpServer) + // until the TTL sweep. The new session has the freshest lastActivity, + // so it is never the one evicted; the sid guard covers a cap of 1. + while (countSessionsForUser(user.id) > MAX_SESSIONS_PER_USER) { + const dropped = evictOldestSessionForUser(user.id); + if (!dropped || dropped === sid) break; + console.log(`[MCP] Session limit (${MAX_SESSIONS_PER_USER}) exceeded for user ${user.id} — evicted idle session ${dropped}`); + } const authMethod = isStaticToken ? 'static-token' : scopes ? `oauth(${scopes.join(',')})` : 'jwt'; console.log(`[MCP] Session ${sid} created for user ${user.id} [${authMethod}]. Active sessions: ${sessions.size}`); }, diff --git a/server/src/nest/oauth/oauth-api.controller.ts b/server/src/nest/oauth/oauth-api.controller.ts index 2a406998a..bd3138fc7 100644 --- a/server/src/nest/oauth/oauth-api.controller.ts +++ b/server/src/nest/oauth/oauth-api.controller.ts @@ -73,13 +73,6 @@ export class OauthApiController { if (!this.oauth.mcpEnabled()) { throw new HttpException({ error: 'MCP is not enabled' }, 403); } - if (!body.approved) { - const url = new URL(body.redirect_uri); - url.searchParams.set('error', 'access_denied'); - url.searchParams.set('error_description', 'User denied the authorization request'); - if (body.state) url.searchParams.set('state', body.state); - return { redirect: url.toString() }; - } const params: AuthorizeParams = { response_type: 'code', client_id: body.client_id, @@ -90,10 +83,21 @@ export class OauthApiController { code_challenge_method: body.code_challenge_method, resource: body.resource, }; + // Validate before branching on approved: the deny path builds a redirect out + // of body.redirect_uri too, and only validateAuthorizeRequest checks it + // against the client's registered URIs. A string that is not a URL at all + // would additionally throw out of `new URL()` as a 500. const validation = this.oauth.validateAuthorizeRequest(params, user.id); if (!validation.valid) { throw new HttpException({ error: validation.error, error_description: validation.error_description }, 400); } + if (!body.approved) { + const url = new URL(body.redirect_uri); + url.searchParams.set('error', 'access_denied'); + url.searchParams.set('error_description', 'User denied the authorization request'); + if (body.state) url.searchParams.set('state', body.state); + return { redirect: url.toString() }; + } const scopes = validation.scopes!; this.oauth.saveConsent(body.client_id, user.id, scopes, ip); const code = this.oauth.createAuthCode({ diff --git a/server/src/nest/oauth/oauth-sdk.provider.ts b/server/src/nest/oauth/oauth-sdk.provider.ts index 1f44dcc67..961832f77 100644 --- a/server/src/nest/oauth/oauth-sdk.provider.ts +++ b/server/src/nest/oauth/oauth-sdk.provider.ts @@ -173,7 +173,9 @@ export class TrekOAuthProvider implements OAuthServerProvider { if (pending.resource && resourceStr && pending.resource !== resourceStr) throw new Error('Authorization grant is invalid.'); - if (codeVerifier && !this.oauth.verifyPKCE(codeVerifier, pending.codeChallenge)) + // A missing verifier is a failed exchange, not a skipped check — the live + // token endpoint refuses it the same way (oauth-public.controller.ts). + if (!codeVerifier || !this.oauth.verifyPKCE(codeVerifier, pending.codeChallenge)) throw new Error('Authorization grant is invalid.'); const tokens = this.oauth.issueTokens(client.client_id, pending.userId, pending.scopes, null, pending.resource ?? null); diff --git a/server/src/nest/permissions/permissions.service.ts b/server/src/nest/permissions/permissions.service.ts index e71b5ba96..cd964e921 100644 --- a/server/src/nest/permissions/permissions.service.ts +++ b/server/src/nest/permissions/permissions.service.ts @@ -78,7 +78,7 @@ export class PermissionsService { private loadPermissions(): Map { const cached = getPermissionsCache(); if (cached) return cached; - const cache = setPermissionsCache(new Map()); + const cache = new Map(); try { const rows = this.dbs.all<{ key: string; value: string }>( "SELECT key, value FROM app_settings WHERE key LIKE 'perm_%'" @@ -99,8 +99,14 @@ export class PermissionsService { // real DB failure that must not stay invisible (we still serve defaults). const msg = e instanceof Error ? e.message : String(e); if (!msg.includes('no such table')) logError(`Permissions load failed: ${msg}`); + // Serve defaults for THIS call, but do not install them: a half-built + // cache would freeze every later reader on the defaults until somebody + // invalidates by hand, which is how a stricter admin setting silently + // stops applying. The next call retries the read instead. + return cache; } - return cache; + // Only a completed read becomes the shared cache. + return setPermissionsCache(cache); } invalidatePermissionsCache(): void { diff --git a/server/src/nest/plugins/plugin-frame.controller.ts b/server/src/nest/plugins/plugin-frame.controller.ts index 24d7aa3c5..0545cd0bb 100644 --- a/server/src/nest/plugins/plugin-frame.controller.ts +++ b/server/src/nest/plugins/plugin-frame.controller.ts @@ -59,6 +59,23 @@ export class PluginFrameController { res.status(404).send('Not found'); return; } + // The check above is lexical, and statSync follows links, so a symlink inside + // client/ still reads whatever it points at. Re-check both sides resolved: + // a dev-linked plugin is a symlinked tree, so comparing against the raw root + // would reject it. + let realRoot: string; + let realFile: string; + try { + realRoot = fs.realpathSync(path.resolve(clientDir)); + realFile = fs.realpathSync(resolved); + } catch { + res.status(404).send('Not found'); + return; + } + if (realFile !== realRoot && !realFile.startsWith(realRoot + path.sep)) { + res.status(403).send('Forbidden'); + return; + } const ext = path.extname(resolved).toLowerCase(); res.setHeader('Content-Type', MIME[ext] ?? 'application/octet-stream'); diff --git a/server/src/nest/plugins/plugins.service.ts b/server/src/nest/plugins/plugins.service.ts index d939988f8..4522b6ba6 100644 --- a/server/src/nest/plugins/plugins.service.ts +++ b/server/src/nest/plugins/plugins.service.ts @@ -166,8 +166,9 @@ export class PluginsService { /** * Merge instance-scope settings into the plugin's config, encrypting fields - * declared secret (unless the value is the unchanged mask sentinel). Returns - * the config with secrets masked for the client. + * declared secret (unless the value is the unchanged mask sentinel). Only keys + * declared as `scope:'instance'` fields are accepted, same as updateUserConfig. + * Returns the config with secrets masked for the client. */ updateInstanceConfig(id: string, patch: Record): Record { const row = this.db.prepare('SELECT config FROM plugins WHERE id = ?').get(id) as { config: string } | undefined; @@ -181,8 +182,17 @@ export class PluginsService { ).map((r) => r.field_key), ); + const allowed = new Set( + ( + this.db + .prepare("SELECT field_key FROM plugin_settings_fields WHERE plugin_id = ? AND scope = 'instance'") + .all(id) as Array<{ field_key: string }> + ).map((r) => r.field_key), + ); + const config = safeParse(row.config); for (const [k, v] of Object.entries(patch)) { + if (!allowed.has(k)) continue; // never store an undeclared key if (secretKeys.has(k)) { if (v === SECRET_MASK) continue; // unchanged secret — keep stored ciphertext config[k] = maybe_encrypt_api_key(v); diff --git a/server/tests/integration/mcp.test.ts b/server/tests/integration/mcp.test.ts index 906086795..50e5e943e 100644 --- a/server/tests/integration/mcp.test.ts +++ b/server/tests/integration/mcp.test.ts @@ -250,6 +250,22 @@ describe('MCP session management', () => { expect(sessions.has(firstSessionId)).toBe(false); // the least-recently-active one made room }); + it('MCP-006 — initializes that race past the cap check are trimmed when they register', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'mcp'").run(); + + const sessionsForUser = () => [...sessions.values()].filter((s) => s.userId === user.id).length; + + for (let i = 0; i < 20; i++) await createSession(user.id); + expect(sessionsForUser()).toBe(20); + + // All five read the same pre-registration count, so all five pass the cap + // check. Registration is where the overshoot has to be given back — nothing + // else drops these entries before the TTL sweep, and each holds a server. + await Promise.all([1, 2, 3, 4, 5].map(() => createSession(user.id))); + expect(sessionsForUser()).toBe(20); + }); + it('MCP — session resumption with valid mcp-session-id', async () => { const { user } = createUser(testDb); testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'mcp'").run(); @@ -433,6 +449,43 @@ describe('MCP transport parity pins (Nest-hosted /mcp)', () => { expect(res.headers['www-authenticate']).toBe(EXPECTED_CHALLENGE); }); + it('MCP-P08b — a narrower token cannot resume a session that was created with wider scopes', async () => { + const { user } = createUser(testDb); + const created = oauthSvc.createOAuthClient(user.id, 'Scope Test Client', ['https://client.example.com/cb'], ['trips:read', 'trips:write']); + const clientId = (created.client as { client_id: string }).client_id; + const wide = oauthSvc.issueTokens(clientId, user.id, ['trips:read', 'trips:write'], null, MCP_AUDIENCE); + const narrow = oauthSvc.issueTokens(clientId, user.id, ['trips:read'], null, MCP_AUDIENCE); + const sessionId = await createSession(wide.access_token); + + // The tool surface was registered from the wide set; resuming with the + // narrow one would ride it. + const res = await request(app) + .post('/mcp') + .set('Authorization', `Bearer ${narrow.access_token}`) + .set('mcp-session-id', sessionId) + .send({ jsonrpc: '2.0', method: 'tools/list', id: 2 }); + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: 'Session was created with different scopes' }); + expect(res.headers['www-authenticate']).toContain('error="insufficient_scope"'); + }); + + it('MCP-P08c — the same scopes in a different order still resume', async () => { + const { user } = createUser(testDb); + const created = oauthSvc.createOAuthClient(user.id, 'Scope Order Client', ['https://client.example.com/cb'], ['trips:read', 'trips:write']); + const clientId = (created.client as { client_id: string }).client_id; + const first = oauthSvc.issueTokens(clientId, user.id, ['trips:read', 'trips:write'], null, MCP_AUDIENCE); + const reordered = oauthSvc.issueTokens(clientId, user.id, ['trips:write', 'trips:read'], null, MCP_AUDIENCE); + const sessionId = await createSession(first.access_token); + + const res = await request(app) + .post('/mcp') + .set('Authorization', `Bearer ${reordered.access_token}`) + .set('mcp-session-id', sessionId) + .set('Accept', 'application/json, text/event-stream') + .send({ jsonrpc: '2.0', method: 'tools/list', id: 2 }); + expect(res.status).toBe(200); + }); + it('MCP-P09 — a static-token session carries the deprecation notice on the first tool call only', async () => { const { user } = createUser(testDb); const { rawToken } = createMcpToken(testDb, user.id); diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index 4c06f37be..30f8725a1 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -820,22 +820,44 @@ describe('POST /api/oauth/authorize', () => { it('OAUTH-030 — user denied returns redirect with error=access_denied', async () => { const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); const res = await request(app) .post('/api/oauth/authorize') .set('Cookie', authCookie(user.id)) .send({ approved: false, - client_id: 'any', + client_id: r.client!.client_id, redirect_uri: 'https://app.example.com/cb', scope: 'trips:read', - code_challenge: 'c', + code_challenge: challenge, code_challenge_method: 'S256', }); expect(res.status).toBe(200); expect(res.body.redirect).toContain('error=access_denied'); }); + it('OAUTH-030b — a denial for an unregistered redirect_uri is refused, not redirected', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + const { challenge } = makePkce(); + + const res = await request(app) + .post('/api/oauth/authorize') + .set('Cookie', authCookie(user.id)) + .send({ + approved: false, + client_id: r.client!.client_id, + redirect_uri: 'https://attacker.example.com/cb', + scope: 'trips:read', + code_challenge: challenge, + code_challenge_method: 'S256', + }); + expect(res.status).toBe(400); + expect(res.body.error).toBe('invalid_redirect_uri'); + }); + it('OAUTH-031 — invalid params returns 400', async () => { const { user } = createUser(testDb); const { challenge } = makePkce(); diff --git a/server/tests/unit/middleware/globalMiddleware.csp.test.ts b/server/tests/unit/middleware/globalMiddleware.csp.test.ts index 07b179b59..0baaead11 100644 --- a/server/tests/unit/middleware/globalMiddleware.csp.test.ts +++ b/server/tests/unit/middleware/globalMiddleware.csp.test.ts @@ -1,9 +1,9 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterEach } from 'vitest'; import express from 'express'; import request from 'supertest'; import { applyGlobalMiddleware } from '../../../src/middleware/globalMiddleware'; -async function connectSrcSources(): Promise { +async function directiveSources(name: string): Promise { const app = express(); applyGlobalMiddleware(app); app.get('/probe', (_req, res) => res.json({ ok: true })); @@ -13,11 +13,13 @@ async function connectSrcSources(): Promise { const directive = csp .split(';') .map(d => d.trim()) - .find(d => d.startsWith('connect-src')); + .find(d => d.startsWith(name)); return directive ? directive.split(/\s+/).slice(1) : []; } +const connectSrcSources = () => directiveSources('connect-src'); + describe('global CSP: OpenStreetMap tile hosts (#1733)', () => { it('allows the bare tile.openstreetmap.org host', async () => { // A CSP wildcard host never matches the apex, so `*.tile.openstreetmap.org` @@ -30,3 +32,44 @@ describe('global CSP: OpenStreetMap tile hosts (#1733)', () => { expect(await connectSrcSources()).toContain('https://*.tile.openstreetmap.org'); }); }); + +describe('global CSP: script-src', () => { + it("allows 'wasm-unsafe-eval' so the WASM decoders keep running", async () => { + expect(await directiveSources('script-src')).toContain("'wasm-unsafe-eval'"); + }); + + it("does not allow 'unsafe-eval' — the policy the wiki documents, and nothing first-party evals", async () => { + expect(await directiveSources('script-src')).not.toContain("'unsafe-eval'"); + }); +}); + +describe('forced-HTTPS redirect', () => { + const saved = { FORCE_HTTPS: process.env.FORCE_HTTPS, APP_URL: process.env.APP_URL }; + + afterEach(() => { + process.env.FORCE_HTTPS = saved.FORCE_HTTPS; + process.env.APP_URL = saved.APP_URL; + if (saved.FORCE_HTTPS === undefined) delete process.env.FORCE_HTTPS; + if (saved.APP_URL === undefined) delete process.env.APP_URL; + }); + + async function redirectLocation(): Promise { + const app = express(); + applyGlobalMiddleware(app); + app.get('/trips', (_req, res) => res.json({ ok: true })); + const res = await request(app).get('/trips').set('Host', 'evil.example.com'); + return String(res.headers.location || ''); + } + + it('redirects to the configured APP_URL host, not the Host header the caller sent', async () => { + process.env.FORCE_HTTPS = 'true'; + process.env.APP_URL = 'https://trip.pakulat.org'; + expect(await redirectLocation()).toBe('https://trip.pakulat.org/trips'); + }); + + it('falls back to the request host when APP_URL is unset', async () => { + process.env.FORCE_HTTPS = 'true'; + delete process.env.APP_URL; + expect(await redirectLocation()).toBe('https://evil.example.com/trips'); + }); +}); diff --git a/server/tests/unit/middleware/globalMiddleware.redaction.test.ts b/server/tests/unit/middleware/globalMiddleware.redaction.test.ts index 63f116b1a..ed4116b43 100644 --- a/server/tests/unit/middleware/globalMiddleware.redaction.test.ts +++ b/server/tests/unit/middleware/globalMiddleware.redaction.test.ts @@ -45,6 +45,25 @@ describe('globalMiddleware request-log redaction', () => { }); }); + it('redacts secret-shaped keys by suffix — the OAuth token body and the settings API keys', () => { + const out = redact({ + refresh_token: 'rt-1', + code_verifier: 'cv-1', + llm_api_key: 'sk-1', + mapbox_access_token: 'pk-1', + grant_type: 'refresh_token', + }) as Record; + expect(out.refresh_token).toBe('[REDACTED]'); + expect(out.code_verifier).toBe('[REDACTED]'); + expect(out.llm_api_key).toBe('[REDACTED]'); + expect(out.mapbox_access_token).toBe('[REDACTED]'); + expect(out.grant_type).toBe('refresh_token'); + }); + + it('matches the suffix on the end of the key only, so identifiers stay readable', () => { + expect(redact({ accessKeyId: 'ak-1', tokenCount: 5 })).toEqual({ accessKeyId: 'ak-1', tokenCount: 5 }); + }); + it('passes non-object values through untouched', () => { expect(redact('plain string')).toBe('plain string'); expect(redact(42)).toBe(42); diff --git a/server/tests/unit/nest/audit.service.test.ts b/server/tests/unit/nest/audit.service.test.ts index 3c9499218..e37c5ec15 100644 --- a/server/tests/unit/nest/audit.service.test.ts +++ b/server/tests/unit/nest/audit.service.test.ts @@ -61,10 +61,12 @@ afterAll(() => { }); function makeReq(options: { + ip?: string; xff?: string | string[]; remoteAddress?: string; } = {}): Request { return { + ip: options.ip, headers: { ...(options.xff !== undefined ? { 'x-forwarded-for': options.xff } : {}), }, @@ -75,33 +77,34 @@ function makeReq(options: { // ── getClientIp (pure, moved 1:1) ───────────────────────────────────────────── describe('getClientIp', () => { - it('AUDIT-SVC-001: returns first IP from comma-separated X-Forwarded-For string', () => { - expect(getClientIp(makeReq({ xff: '1.2.3.4, 5.6.7.8, 9.10.11.12' }))).toBe('1.2.3.4'); + it('AUDIT-SVC-001: returns req.ip, which Express resolved through the trust-proxy hop count', () => { + expect(getClientIp(makeReq({ ip: '1.2.3.4', xff: '9.9.9.9, 1.2.3.4' }))).toBe('1.2.3.4'); }); - it('AUDIT-SVC-002: returns single IP when X-Forwarded-For has no comma', () => { - expect(getClientIp(makeReq({ xff: '10.0.0.1' }))).toBe('10.0.0.1'); + it('AUDIT-SVC-002: ignores a hand-written X-Forwarded-For — the leftmost entry is whatever the caller typed', () => { + expect(getClientIp(makeReq({ ip: '203.0.113.7', xff: '10.0.0.1' }))).toBe('203.0.113.7'); }); - it('AUDIT-SVC-003: returns first element when X-Forwarded-For is an array', () => { - expect(getClientIp(makeReq({ xff: ['203.0.113.1', '10.0.0.1'] }))).toBe('203.0.113.1'); + it('AUDIT-SVC-003: ignores an array-valued X-Forwarded-For too', () => { + expect(getClientIp(makeReq({ ip: '203.0.113.7', xff: ['203.0.113.1', '10.0.0.1'] }))).toBe('203.0.113.7'); }); - it('AUDIT-SVC-004: trims whitespace from extracted IP', () => { - expect(getClientIp(makeReq({ xff: ' 192.168.1.1 , 10.0.0.1' }))).toBe('192.168.1.1'); + it('AUDIT-SVC-004: trims whitespace from the resolved IP', () => { + expect(getClientIp(makeReq({ ip: ' 192.168.1.1 ' }))).toBe('192.168.1.1'); }); - it('AUDIT-SVC-005: falls back to req.socket.remoteAddress when no X-Forwarded-For', () => { + it('AUDIT-SVC-005: falls back to req.socket.remoteAddress when req.ip is unset', () => { expect(getClientIp(makeReq({ remoteAddress: '172.16.0.1' }))).toBe('172.16.0.1'); }); - it('AUDIT-SVC-006: returns null when no forwarded header and no socket address', () => { + it('AUDIT-SVC-006: returns null when there is neither a resolved IP nor a socket address', () => { expect(getClientIp(makeReq({}))).toBeNull(); }); - it('AUDIT-SVC-007: returns null for empty string X-Forwarded-For', () => { + it('AUDIT-SVC-007: returns null for an empty req.ip with no socket address', () => { const req = { - headers: { 'x-forwarded-for': '' }, + ip: '', + headers: { 'x-forwarded-for': '203.0.113.9' }, socket: { remoteAddress: undefined }, } as unknown as Request; expect(getClientIp(req)).toBeNull(); @@ -147,6 +150,19 @@ describe('writeAudit', () => { expect(logInfo).toHaveBeenLastCalledWith('anonymous logged in ip=9.9.9.9'); }); + it('AUDIT-SVC-019: a trip title with a line break cannot forge a second log line', () => { + seedUser(1, 'a@b.c'); + svc.writeAudit({ + userId: 1, + action: 'trip.create', + details: { title: 'Rome\nadmin@b.c changed user role' }, + ip: '1.2.3.4', + }); + const logged = String((logInfo as unknown as { mock: { calls: string[][] } }).mock.calls.at(-1)?.[0]); + expect(logged).not.toContain('\n'); + expect(logged).toBe('a@b.c created trip "Rome admin@b.c changed user role" ip=1.2.3.4'); + }); + it('AUDIT-SVC-011: omitted resource/ip store NULL and the log line ends ip=-', () => { seedUser(1, 'a@b.c'); svc.writeAudit({ userId: 1, action: 'user.login' }); diff --git a/server/tests/unit/nest/auth.controller.test.ts b/server/tests/unit/nest/auth.controller.test.ts index b3f2afea2..0078e4196 100644 --- a/server/tests/unit/nest/auth.controller.test.ts +++ b/server/tests/unit/nest/auth.controller.test.ts @@ -249,8 +249,23 @@ describe('AuthController (authenticated)', () => { }); it('resource-token 503 when unavailable, else returns the token payload', () => { - expect(thrown(() => ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue(null) }).resourceToken(user, {}))).toEqual({ status: 503, body: { error: 'Service unavailable' } }); - expect(ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, { purpose: 'download' })).toEqual({ token: 'rt' }); + expect(thrown(() => ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue(null) }).resourceToken(user, {}, req))).toEqual({ status: 503, body: { error: 'Service unavailable' } }); + expect(ac(asvc({}), rl(), { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, { purpose: 'download' }, req)).toEqual({ token: 'rt' }); + }); + + it('ws/resource tokens throttle on their own buckets, so a mint loop cannot drain the store or block login', () => { + const s = rl(); + const now = Date.now(); + for (let i = 0; i < 120; i++) s.check('ws_token', '9.9.9.9', 120, 15 * 60 * 1000, now); + expect(thrown(() => ac(asvc({}), s, { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user, req))) + .toEqual({ status: 429, body: { error: 'Too many attempts. Please try again later.' } }); + // The login bucket is untouched: an exhausted socket loop must not lock the account out. + expect(s.check('login', '9.9.9.9', 5, 15 * 60 * 1000, now)).toBe(true); + + const s2 = rl(); + for (let i = 0; i < 120; i++) s2.check('resource_token', '9.9.9.9', 120, 15 * 60 * 1000, now); + expect(thrown(() => ac(asvc({}), s2, { createResourceToken: vi.fn().mockReturnValue({ token: 'rt' }) }).resourceToken(user, {}, req))) + .toEqual({ status: 429, body: { error: 'Too many attempts. Please try again later.' } }); }); it('rate-limited account ops throw 429 once the bucket is exhausted', () => { @@ -385,8 +400,8 @@ describe('AuthController (authenticated)', () => { }); it('ws-token maps error, else returns the token', () => { - expect(thrown(() => ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ error: 'down', status: 503 }) }).wsToken(user))).toEqual({ status: 503, body: { error: 'down' } }); - expect(ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user)).toEqual({ token: 'ws' }); + expect(thrown(() => ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ error: 'down', status: 503 }) }).wsToken(user, req))).toEqual({ status: 503, body: { error: 'down' } }); + expect(ac(asvc({}), rl(), { createWsToken: vi.fn().mockReturnValue({ token: 'ws' }) }).wsToken(user, req)).toEqual({ token: 'ws' }); }); it('avatar saves when not in demo mode (env present but email is not a demo email)', async () => { diff --git a/server/tests/unit/nest/idempotency.interceptor.test.ts b/server/tests/unit/nest/idempotency.interceptor.test.ts index 31d4e4915..646e601dd 100644 --- a/server/tests/unit/nest/idempotency.interceptor.test.ts +++ b/server/tests/unit/nest/idempotency.interceptor.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { HttpException } from '@nestjs/common'; import type { CallHandler, ExecutionContext } from '@nestjs/common'; -import { of, lastValueFrom } from 'rxjs'; +import { from, of, lastValueFrom } from 'rxjs'; import { IdempotencyInterceptor } from '../../../src/nest/common/idempotency.interceptor'; import type { DatabaseService } from '../../../src/nest/database/database.service'; @@ -181,6 +181,62 @@ describe('IdempotencyInterceptor (parity with the legacy applyIdempotency middle expect(returned).toEqual({ ok: true }); }); + it('makes an overlapping replay of the same key wait, then answers it from the first response', async () => { + // The row only appears once the first request answers, so without the + // in-flight map both of these would miss the SELECT and both would run. + const rows = new Map(); + const db = makeDb({ + get: vi.fn((_sql: string, ...params: unknown[]) => rows.get(String(params[0]))) as unknown as DatabaseService['get'], + run: vi.fn((_sql: string, ...params: unknown[]) => { + rows.set(String(params[0]), { status_code: Number(params[4]), response_body: String(params[5]) }); + return {} as never; + }) as unknown as DatabaseService['run'], + }); + const interceptor = new IdempotencyInterceptor(db); + + // The first handler is still running when the second request arrives. + let finish!: (value: unknown) => void; + const slow = { handle: vi.fn(() => from(new Promise((resolve) => { finish = resolve; }))) }; + const firstRes = makeRes(); + const first = lastValueFrom(interceptor.intercept(ctx({ method: 'POST', headers: { 'x-idempotency-key': 'k' }, path: '/api/places', user: { id: 1 } }, firstRes), slow)); + + const secondHandler = handler({ id: 'second' }); + const secondRes = makeRes(); + const second = lastValueFrom(interceptor.intercept(ctx({ method: 'POST', headers: { 'x-idempotency-key': 'k' }, path: '/api/places', user: { id: 1 } }, secondRes), secondHandler)); + + firstRes.statusCode = 201; + firstRes.json({ id: 'first' }); + finish({ id: 'first' }); + + expect(await first).toEqual({ id: 'first' }); + expect(await second).toEqual({ id: 'first' }); + expect(secondHandler.handle).not.toHaveBeenCalled(); + expect(secondRes.status).toHaveBeenCalledWith(201); + }); + + it('runs the waiting request itself when the first one cached nothing', async () => { + const db = makeDb({ get: vi.fn().mockReturnValue(undefined), run: vi.fn() }); + const interceptor = new IdempotencyInterceptor(db); + + let finish!: (value: unknown) => void; + const slow = { handle: vi.fn(() => from(new Promise((resolve) => { finish = resolve; }))) }; + const first = lastValueFrom(interceptor.intercept( + ctx({ method: 'POST', headers: { 'x-idempotency-key': 'k' }, path: '/api/places', user: { id: 1 } }, makeRes()), + slow, + )); + + const secondHandler = handler({ id: 'second' }); + const second = lastValueFrom(interceptor.intercept( + ctx({ method: 'POST', headers: { 'x-idempotency-key': 'k' }, path: '/api/places', user: { id: 1 } }, makeRes()), + secondHandler, + )); + + finish({ id: 'first' }); + await first; + expect(await second).toEqual({ id: 'second' }); + expect(secondHandler.handle).toHaveBeenCalled(); + }); + it('treats a PATCH as a mutating method', async () => { const db = makeDb({ get: vi.fn().mockReturnValue(undefined), run: vi.fn() }); const res = makeRes(); diff --git a/server/tests/unit/nest/mcp-transport.service.test.ts b/server/tests/unit/nest/mcp-transport.service.test.ts index f5c1de0c1..4755c8e6c 100644 --- a/server/tests/unit/nest/mcp-transport.service.test.ts +++ b/server/tests/unit/nest/mcp-transport.service.test.ts @@ -15,6 +15,7 @@ import { armSseKeepalive, countSessionsForUser, jsonRpcError, + sameScopes, setAuthChallenge, } from '../../../src/nest/mcp-transport/mcp-transport.service'; import { McpTransportController } from '../../../src/nest/mcp-transport/mcp-transport.controller'; @@ -70,6 +71,21 @@ describe('setAuthChallenge', () => { }); }); +describe('sameScopes', () => { + it('MCPT-008: full access only matches full access', () => { + expect(sameScopes(null, null)).toBe(true); + expect(sameScopes(null, ['trips:read'])).toBe(false); + expect(sameScopes(['trips:read'], null)).toBe(false); + }); + + it('MCPT-009: order does not matter, membership does', () => { + expect(sameScopes(['a', 'b'], ['b', 'a'])).toBe(true); + expect(sameScopes(['a', 'b'], ['a'])).toBe(false); + expect(sameScopes(['a'], ['b'])).toBe(false); + expect(sameScopes([], [])).toBe(true); + }); +}); + describe('countSessionsForUser', () => { afterEach(() => sessions.clear()); diff --git a/server/tests/unit/nest/oauth-sdk.provider.test.ts b/server/tests/unit/nest/oauth-sdk.provider.test.ts index c113e8340..7cd6a8033 100644 --- a/server/tests/unit/nest/oauth-sdk.provider.test.ts +++ b/server/tests/unit/nest/oauth-sdk.provider.test.ts @@ -374,10 +374,15 @@ describe('TrekOAuthProvider.exchangeAuthorizationCode', () => { it('SDKP-046: a pending code without a resource issues a null-audience token', async () => { const oauth = makeOauth({ consumeAuthCode: vi.fn(() => pending({ resource: null })) }); const { provider } = makeProvider(oauth); - await provider.exchangeAuthorizationCode(clientInfo(), 'code'); + await provider.exchangeAuthorizationCode(clientInfo(), 'code', 'verifier'); expect(oauth.issueTokens).toHaveBeenCalledWith('cid-1', 7, ['trips:read'], null, null); - // No codeVerifier was supplied, so PKCE verification is skipped entirely. - expect(oauth.verifyPKCE).not.toHaveBeenCalled(); + }); + + it('SDKP-047: a missing codeVerifier is refused, not waved through', async () => { + const oauth = makeOauth({ consumeAuthCode: vi.fn(() => pending()) }); + const { provider } = makeProvider(oauth); + await expect(provider.exchangeAuthorizationCode(clientInfo(), 'code')).rejects.toThrow(invalid); + expect(oauth.issueTokens).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/nest/oauth.controller.test.ts b/server/tests/unit/nest/oauth.controller.test.ts index 2d0c407de..f111229f6 100644 --- a/server/tests/unit/nest/oauth.controller.test.ts +++ b/server/tests/unit/nest/oauth.controller.test.ts @@ -339,7 +339,8 @@ describe('OauthApiController', () => { }); it('authorize: denied returns a redirect with access_denied, approved issues a code', () => { - const denied = new OauthApiController(osvc(), rl()).authorize(user, { client_id: 'c', redirect_uri: 'https://cb', scope: 's', code_challenge: 'cc', code_challenge_method: 'S256', approved: false }, req); + const deniedSvc = osvc({ validateAuthorizeRequest: vi.fn().mockReturnValue({ valid: true, scopes: ['s'] }) }); + const denied = new OauthApiController(deniedSvc, rl()).authorize(user, { client_id: 'c', redirect_uri: 'https://cb', scope: 's', code_challenge: 'cc', code_challenge_method: 'S256', approved: false }, req); expect((denied as { redirect: string }).redirect).toContain('error=access_denied'); const svc = osvc({ validateAuthorizeRequest: vi.fn().mockReturnValue({ valid: true, scopes: ['s'], resource: null }), saveConsent: vi.fn(), createAuthCode: vi.fn().mockReturnValue('the_code') }); const ok = new OauthApiController(svc, rl()).authorize(user, { client_id: 'c', redirect_uri: 'https://cb', scope: 's', code_challenge: 'cc', code_challenge_method: 'S256', approved: true }, req); @@ -405,7 +406,8 @@ describe('OauthApiController', () => { }); it('authorize: carries the state through both the denied and approved redirects', () => { - const denied = new OauthApiController(osvc(), rl()).authorize(user, { client_id: 'c', redirect_uri: 'https://cb', scope: 's', state: 'xyz', code_challenge: 'cc', code_challenge_method: 'S256', approved: false }, req); + const deniedSvc = osvc({ validateAuthorizeRequest: vi.fn().mockReturnValue({ valid: true, scopes: ['s'] }) }); + const denied = new OauthApiController(deniedSvc, rl()).authorize(user, { client_id: 'c', redirect_uri: 'https://cb', scope: 's', state: 'xyz', code_challenge: 'cc', code_challenge_method: 'S256', approved: false }, req); expect((denied as { redirect: string }).redirect).toContain('state=xyz'); const svc = osvc({ validateAuthorizeRequest: vi.fn().mockReturnValue({ valid: true, scopes: ['s'], resource: 'https://aud' }), saveConsent: vi.fn(), createAuthCode: vi.fn().mockReturnValue('the_code') }); @@ -414,6 +416,15 @@ describe('OauthApiController', () => { expect((ok as { redirect: string }).redirect).toContain('state=xyz'); }); + it('authorize: a denial is validated too, so the redirect can only be a registered URI', () => { + const validateAuthorizeRequest = vi.fn().mockReturnValue({ valid: false, error: 'invalid_redirect_uri', error_description: 'Invalid redirect URI' }); + const svc = osvc({ validateAuthorizeRequest, saveConsent: vi.fn(), createAuthCode: vi.fn() }); + expect(thrown(() => new OauthApiController(svc, rl()) + .authorize(user, { client_id: 'c', redirect_uri: 'not a url', scope: 's', code_challenge: 'cc', code_challenge_method: 'S256', approved: false }, req))) + .toEqual({ status: 400, body: { error: 'invalid_redirect_uri', error_description: 'Invalid redirect URI' } }); + expect(validateAuthorizeRequest).toHaveBeenCalled(); + }); + it('client/session errors default the status to 400 when the service omits it', () => { expect(thrown(() => new OauthApiController(osvc({ createOAuthClient: vi.fn().mockReturnValue({ error: 'bad' }) }), rl()).createClient(user, { name: 'X', allowed_scopes: ['a'] }, req))) .toEqual({ status: 400, body: { error: 'bad' } }); diff --git a/server/tests/unit/nest/permissions.service.test.ts b/server/tests/unit/nest/permissions.service.test.ts index 97342743b..e38e200ab 100644 --- a/server/tests/unit/nest/permissions.service.test.ts +++ b/server/tests/unit/nest/permissions.service.test.ts @@ -43,7 +43,10 @@ import { runMigrations } from '../../../src/db/migrations'; import { logError } from '../../../src/nest/audit/audit-log.logger'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService, PERMISSION_ACTIONS } from '../../../src/nest/permissions/permissions.service'; -import { invalidatePermissionsCache as invalidateSharedCache } from '../../../src/nest/permissions/permissions-cache'; +import { + getPermissionsCache, + invalidatePermissionsCache as invalidateSharedCache, +} from '../../../src/nest/permissions/permissions-cache'; const svc = new PermissionsService(new DatabaseService(testDb)); @@ -192,6 +195,22 @@ describe('load failures', () => { svc.invalidatePermissionsCache(); // don't leak the bare-DB cache to later tests }); + it('PERM-SVC-024: a failed read serves defaults without installing them, and a later read populates the cache', () => { + const failing = { all: vi.fn((): { key: string; value: string }[] => { throw new Error('database connection is closed'); }) }; + const flakySvc = new PermissionsService(failing as unknown as DatabaseService); + flakySvc.invalidatePermissionsCache(); + + expect(flakySvc.getPermissionLevel('trip_edit')).toBe('trip_owner'); + // Nothing installed: an admin's stricter stored level would otherwise stay + // invisible until somebody invalidated by hand. + expect(getPermissionsCache()).toBe(null); + + failing.all.mockReturnValue([{ key: 'perm_trip_edit', value: 'trip_member' }]); + expect(flakySvc.getPermissionLevel('trip_edit')).toBe('trip_member'); + expect(getPermissionsCache()).not.toBe(null); + svc.invalidatePermissionsCache(); // don't leak the stub cache to later tests + }); + it('PERM-SVC-023: an all-skipped save writes nothing and leaves the cache untouched', () => { expect(svc.getPermissionLevel('trip_edit')).toBe('trip_owner'); // prime the cache testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run('perm_trip_edit', 'trip_member'); diff --git a/server/tests/unit/nest/rate-limit.service.test.ts b/server/tests/unit/nest/rate-limit.service.test.ts new file mode 100644 index 000000000..b0c9d7e73 --- /dev/null +++ b/server/tests/unit/nest/rate-limit.service.test.ts @@ -0,0 +1,76 @@ +/** + * RateLimitService — the per-IP buckets every auth-adjacent route throttles on. + * The limit behaviour is the legacy one line for line; what is worth pinning + * here is that the maps do not grow forever, because the keys come from + * proxy headers and are therefore attacker-influenced. + */ +import { describe, it, expect } from 'vitest'; +import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; + +const WINDOW = 15 * 60 * 1000; + +/** The private bucket map, which is the only place the housekeeping is visible. */ +function bucketSize(service: RateLimitService, bucket: string): number { + const buckets = (service as unknown as { buckets: Map> }).buckets; + return buckets.get(bucket)?.size ?? 0; +} + +describe('RateLimitService', () => { + it('allows up to max attempts in a window, then refuses', () => { + const s = new RateLimitService(); + const now = Date.now(); + for (let i = 0; i < 5; i++) expect(s.check('login', '1.2.3.4', 5, WINDOW, now)).toBe(true); + expect(s.check('login', '1.2.3.4', 5, WINDOW, now)).toBe(false); + }); + + it('starts a fresh window once the old one elapsed', () => { + const s = new RateLimitService(); + const now = Date.now(); + for (let i = 0; i < 5; i++) s.check('login', '1.2.3.4', 5, WINDOW, now); + expect(s.check('login', '1.2.3.4', 5, WINDOW, now + WINDOW)).toBe(true); + }); + + it('drops expired records instead of keeping one per address that ever knocked', () => { + const s = new RateLimitService(); + const now = Date.now(); + for (let i = 0; i < 100; i++) s.check('login', `10.0.0.${i}`, 5, WINDOW, now); + expect(bucketSize(s, 'login')).toBe(100); + + // One request a window later sweeps the hundred stale keys and leaves its own. + s.check('login', '10.0.0.0', 5, WINDOW, now + WINDOW); + expect(bucketSize(s, 'login')).toBe(1); + }); + + it('sweeps each bucket on its own clock, so a busy bucket cannot starve a quiet one', () => { + const s = new RateLimitService(); + const now = Date.now(); + s.check('login', '1.2.3.4', 5, WINDOW, now); + for (let i = 0; i < 3; i++) s.check('mfa', `10.0.0.${i}`, 5, WINDOW, now + WINDOW * (i + 1)); + expect(bucketSize(s, 'login')).toBe(1); + + s.check('login', '5.6.7.8', 5, WINDOW, now + WINDOW); + expect(bucketSize(s, 'login')).toBe(1); + }); + + it('keeps a record that is still inside its window when the sweep runs', () => { + const s = new RateLimitService(); + const now = Date.now(); + s.check('login', '1.2.3.4', 5, WINDOW, now); + s.check('login', '5.6.7.8', 5, WINDOW, now + WINDOW); + // The second key was written after the sweep, the first one was expired. + expect(bucketSize(s, 'login')).toBe(1); + expect(s.check('login', '1.2.3.4', 5, WINDOW, now + WINDOW)).toBe(true); + }); + + it('reset clears a single bucket, or all of them', () => { + const s = new RateLimitService(); + const now = Date.now(); + s.check('login', '1.2.3.4', 5, WINDOW, now); + s.check('mfa', '1.2.3.4', 5, WINDOW, now); + s.reset('login'); + expect(bucketSize(s, 'login')).toBe(0); + expect(bucketSize(s, 'mfa')).toBe(1); + s.reset(); + expect(bucketSize(s, 'mfa')).toBe(0); + }); +}); diff --git a/server/tests/unit/plugins/plugin-frame.test.ts b/server/tests/unit/plugins/plugin-frame.test.ts index 0d59379e0..7c1428b7d 100644 --- a/server/tests/unit/plugins/plugin-frame.test.ts +++ b/server/tests/unit/plugins/plugin-frame.test.ts @@ -15,16 +15,36 @@ import { PluginFrameController } from '../../../src/nest/plugins/plugin-frame.co import type { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; let codeRoot: string; +let outsideRoot: string; +// Windows only lets an unprivileged process create a directory junction. +const dirLink = process.platform === 'win32' ? 'junction' : 'dir'; +let symlinksAvailable = true; + beforeAll(() => { codeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-frame-')); + outsideRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-outside-')); process.env.TREK_PLUGINS_DIR = codeRoot; const dir = path.join(codeRoot, 'widget', 'client'); fs.mkdirSync(dir, { recursive: true }); fs.writeFileSync(path.join(dir, 'index.html'), 'hi'); + + // Something worth stealing, plus a link out of client/ that points at it. + fs.mkdirSync(path.join(outsideRoot, 'secrets'), { recursive: true }); + fs.writeFileSync(path.join(outsideRoot, 'secrets', 'leak.js'), 'TOP SECRET'); + // A dev-linked plugin: the whole tree is a link, which must still be served. + fs.mkdirSync(path.join(outsideRoot, 'linked', 'client'), { recursive: true }); + fs.writeFileSync(path.join(outsideRoot, 'linked', 'client', 'index.html'), 'dev'); + try { + fs.symlinkSync(path.join(outsideRoot, 'secrets'), path.join(dir, 'esc'), dirLink); + fs.symlinkSync(path.join(outsideRoot, 'linked'), path.join(codeRoot, 'linked'), dirLink); + } catch { + symlinksAvailable = false; + } }); afterAll(() => { delete process.env.TREK_PLUGINS_DIR; fs.rmSync(codeRoot, { recursive: true, force: true }); + fs.rmSync(outsideRoot, { recursive: true, force: true }); }); function fakeRes() { @@ -157,4 +177,23 @@ describe('PluginFrameController', () => { new PluginFrameController(runtime(true)).serve('widget', req('missing.js'), res as never); expect(res.statusCode).toBe(404); }); + + // The lexical guard above cannot see through a link, and statSync follows one. + it('403 on a symlink inside client/ that points out of the plugin', () => { + if (!symlinksAvailable) return; + const res = fakeRes(); + new PluginFrameController(runtime(true)).serve('widget', req('esc/leak.js'), res as never); + expect(res.statusCode).toBe(403); + expect(res.filePath).toBeUndefined(); + }); + + // Dev-link mode: the plugin directory itself is a link, so the containment + // check has to resolve BOTH sides or local development stops working. + it('still serves a dev-linked plugin whose whole tree is a symlink', () => { + if (!symlinksAvailable) return; + const res = fakeRes(); + new PluginFrameController(runtime(true)).serve('linked', req(''), res as never); + expect(res.statusCode).toBe(200); + expect(res.filePath).toContain('index.html'); + }); }); diff --git a/server/tests/unit/plugins/plugins-service.test.ts b/server/tests/unit/plugins/plugins-service.test.ts index 45f10baa5..a3d931427 100644 --- a/server/tests/unit/plugins/plugins-service.test.ts +++ b/server/tests/unit/plugins/plugins-service.test.ts @@ -246,6 +246,7 @@ describe('PluginsService instance config', () => { it('encrypts secret fields on write and masks them on read; keeps plaintext for non-secrets', () => { testDb.prepare("INSERT INTO plugins (id, name, status, config) VALUES ('x','X','inactive','{}')").run(); testDb.prepare("INSERT INTO plugin_settings_fields (plugin_id, field_key, scope, secret) VALUES ('x','api_key','instance',1)").run(); + testDb.prepare("INSERT INTO plugin_settings_fields (plugin_id, field_key, scope, secret) VALUES ('x','server','instance',0)").run(); const svc = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))); const masked = svc.updateInstanceConfig('x', { api_key: 'super-secret', server: 'https://h' }); @@ -267,6 +268,19 @@ describe('PluginsService instance config', () => { expect(svc.getInstanceConfig('x').api_key).toBe('••••••••'); }); + it('drops a key the plugin never declared, like the user-scope sibling does', () => { + testDb.prepare("INSERT INTO plugins (id, name, status, config) VALUES ('y','Y','inactive','{}')").run(); + testDb.prepare("INSERT INTO plugin_settings_fields (plugin_id, field_key, scope, secret) VALUES ('y','server','instance',0)").run(); + + const svc = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))); + const masked = svc.updateInstanceConfig('y', { server: 'https://h', smuggled: 'nope' }); + + expect(masked.server).toBe('https://h'); + expect(masked.smuggled).toBeUndefined(); + const stored = JSON.parse((testDb.prepare("SELECT config FROM plugins WHERE id='y'").get() as { config: string }).config); + expect(stored).toEqual({ server: 'https://h' }); + }); + it('throws for an unknown plugin', () => { expect(() => new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).updateInstanceConfig('nope', {})).toThrow(/not found/); expect(() => new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).getInstanceConfig('nope')).toThrow(/not found/);