From 8e5fdaa877b1554c720cbf777b93bcff9ff636e1 Mon Sep 17 00:00:00 2001
From: Maurice <mauriceboe@icloud.com>
Date: Sun, 23 Aug 2026 09:47:58 +0200
Subject: [PATCH] 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                       | 74 +++---------------
 server/src/middleware/globalMiddleware.ts     | 29 ++++++-
 server/src/nest-mcp/types.ts                  | 15 ++--
 server/src/nest/audit/audit.service.ts        | 13 +++-
 server/src/nest/audit/client-ip.ts            | 14 ++--
 server/src/nest/auth/auth.controller.ts       | 11 ++-
 .../nest/common/idempotency.interceptor.ts    | 78 ++++++++++++++++---
 server/src/nest/common/rate-limit.service.ts  | 25 ++++--
 .../mcp-transport/mcp-transport.service.ts    | 29 +++++++
 server/src/nest/oauth/oauth-api.controller.ts | 18 +++--
 server/src/nest/oauth/oauth-sdk.provider.ts   |  4 +-
 .../nest/permissions/permissions.service.ts   | 10 ++-
 .../nest/plugins/plugin-frame.controller.ts   | 17 ++++
 server/src/nest/plugins/plugins.service.ts    | 14 +++-
 server/tests/integration/mcp.test.ts          | 53 +++++++++++++
 server/tests/integration/oauth.test.ts        | 26 ++++++-
 .../middleware/globalMiddleware.csp.test.ts   | 49 +++++++++++-
 .../globalMiddleware.redaction.test.ts        | 19 +++++
 server/tests/unit/nest/audit.service.test.ts  | 40 +++++++---
 .../tests/unit/nest/auth.controller.test.ts   | 23 +++++-
 .../unit/nest/idempotency.interceptor.test.ts | 58 +++++++++++++-
 .../unit/nest/mcp-transport.service.test.ts   | 16 ++++
 .../unit/nest/oauth-sdk.provider.test.ts      | 11 ++-
 .../tests/unit/nest/oauth.controller.test.ts  | 15 +++-
 .../unit/nest/permissions.service.test.ts     | 21 ++++-
 .../unit/nest/rate-limit.service.test.ts      | 76 ++++++++++++++++++
 .../tests/unit/plugins/plugin-frame.test.ts   | 39 ++++++++++
 .../unit/plugins/plugins-service.test.ts      | 14 ++++
 28 files changed, 671 insertions(+), 140 deletions(-)
 create mode 100644 server/tests/unit/nest/rate-limit.service.test.ts

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<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);
     });
   }
 
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<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'
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<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}`);
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<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();
+      }),
+    );
   }
 }
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<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(); }
   }
 }
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<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 {
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<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);
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<string[]> {
+async function directiveSources(name: string): Promise<string[]> {
   const app = express();
   applyGlobalMiddleware(app);
   app.get('/probe', (_req, res) => res.json({ ok: true }));
@@ -13,11 +13,13 @@ async function connectSrcSources(): Promise<string[]> {
   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<string> {
+    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<string, unknown>;
+    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<string, { status_code: number; response_body: string }>();
+    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<string, Map<string, unknown>> }).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'), '<!doctype html><body>hi</body>');
+
+  // 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'), '<!doctype html><body>dev</body>');
+  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/);