fix(server): tighten the guard, audit and token paths

Permissions cached an empty map before it had loaded, so a DB error during
the load cached permissive defaults and every later check answered from that
cache - fail-open, and it never expired. The rate limiter grew its per-IP map
forever; the GC the legacy version had was dropped in the fold. Idempotency
did a check-then-insert, so two concurrent duplicates both ran the handler.

Audit rows recorded whatever X-Forwarded-For said, which the client writes,
and the request-body debug logger redacted by exact key match - it missed
refresh tokens, api keys and PKCE verifiers. FORCE_HTTPS built its redirect
from the client's Host header. CSP asked for unsafe-eval on top of the
wasm-unsafe-eval it actually needs.

The OAuth deny path redirected to an unvalidated URI, ws-token minting had
no rate limit at all, and PKCE was skipped when code_verifier was absent
rather than refused. An MCP session resumed with a token whose scopes had
since been reduced kept the capability set it was created with, and the
session cap was checked before the async registration that increments it.

Plugin frame containment was lexical, so a symlink out of the directory was
never noticed, and instance config accepted keys the manifest never
declared.

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

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<string, unknown> = {};
   for (const [k, v] of Object.entries(value as Record<string, unknown>)) {
-    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);
     });
   }
 

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<ScopeGroup, true> {}
+// 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<ScopeGroup, true>;
 
 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<ScopeMode, true> {}
+export type McpAccessModeRegistry = Record<ScopeMode, true>;
 
 export type McpAccessMode = keyof McpAccessModeRegistry extends never
   ? 'read' | 'write'

server/src/nest/audit/audit.service.ts

@@ -26,6 +26,13 @@ const ACTION_LABELS: Record<string, string> = {
   '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, unknown>): 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}`);

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

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

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<string, Promise<void>>();
+
 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<IdempotencyRow>(
+    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<IdempotencyRow>(
       '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<unknown> {
+    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<unknown> {
     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<void>((resolve) => { done = resolve; }));
+
+    return next.handle().pipe(
+      finalize(() => {
+        inFlight.delete(signature);
+        done();
+      }),
+    );
   }
 }

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<string, Map<string, Attempt>>();
+  /** Last sweep per bucket, so a busy bucket doesn't keep a quiet one from being cleaned. */
+  private readonly lastSweep = new Map<string, number>();
 
   private store(bucket: string): Map<string, Attempt> {
     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<string, Attempt>, 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(); }
   }
 }

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

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({

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

server/src/nest/permissions/permissions.service.ts

@@ -78,7 +78,7 @@ export class PermissionsService {
   private loadPermissions(): Map<string, PermissionLevel> {
     const cached = getPermissionsCache();
     if (cached) return cached;
-    const cache = setPermissionsCache(new Map<string, PermissionLevel>());
+    const cache = new Map<string, PermissionLevel>();
     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 {

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

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<string, unknown>): Record<string, unknown> {
     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);