fix(security): apply 2026-05 audit findings — MCP, records sort, trusted-proxy, Origin gate, sandbox

Two High + four Medium + one Low from a manual audit of the v0.11
surfaces. No critical bugs found; these are defense-in-depth /
scope-mismatch / footgun fixes. Test suite: 861 / 861 pass.

H-1 (MCP run_sql WITH-CTE write-bypass)
  src/mcp/admin-write-tools.ts — the allow_write:false gate previously
  only checked the prefix `^\s*(WITH|SELECT|EXPLAIN|PRAGMA)\b`. SQLite
  supports data-modifying CTEs (`WITH x AS (SELECT 1) UPDATE foo …`)
  which start with WITH and bypass the gate, executing on the writable
  raw connection. Mirror sql-runner.ts: strip comments + string
  literals, then scan for any of INSERT|UPDATE|DELETE|DROP|ALTER|
  CREATE|REPLACE|TRUNCATE|ATTACH|DETACH|REINDEX|VACUUM. Explicit error
  names the offending keyword.

H-2 (list_settings / get_setting scope leak)
  src/mcp/admin-write-tools.ts — both tools advertised mcp:read scope
  but returned decrypted secrets (smtp.password, oauth2.<provider>.
  client_secret, FCM service-account JSON, OneSignal app key, …). The
  mcp:read scope is the least-privileged read scope used for observer/
  auditor tokens — it must not see operator secrets. Raised both to
  mcp:admin. Test updated to match the new contract.

M-1 + L-1 (records sort SQL-identifier injection + naive quoteIdent)
  src/core/records.ts — user-supplied `?sort=…` flowed unvalidated
  through a local quoteIdent that didn't escape embedded `"`. Single-
  statement prepare blunted classic injection today, but error-based
  oracles + future API changes (multi-stmt exec) would turn it into
  RCE. Fix: whitelist sort columns against the actual collection's
  field set (id/created_at/updated_at + non-system fields). View
  collections fall back to a regex check since their schema is inferred
  from the SELECT. Local quoteIdent now matches collections.ts (doubles
  embedded `"` per spec).

M-2 (MCP SSE bypasses trusted-proxy)
  src/api/mcp.ts — /mcp/events read X-Forwarded-For inline without
  consulting VAULTBASE_TRUSTED_PROXIES. Replaced with
  trustedClientIp(request, peerIpOf(server, request)). Adapter strips
  the placeholder "unknown" trustedClientIp returns when no peer IP is
  resolvable (test transports / direct stdio).

M-3 (isOriginAllowed null Origin bypass)
  src/server.ts — isOriginAllowed returned true when Origin was null,
  on the premise that "same-origin requests omit Origin in some
  clients". True for some HTTP fetches, false for browser WebSocket /
  EventSource (which always send Origin). Non-browser clients without
  Origin were skipping the allowlist entirely. Added a requireOrigin
  parameter (default false to preserve existing behaviour); both
  /realtime WS and GET /api/v1/realtime SSE call sites now pass true.

M-4 (VAULTBASE_TRUSTED_PROXIES exact-match instead of CIDR)
  src/core/sec.ts — docs and SECURITY.md advertised CIDR-equivalent
  matching but the impl was Set.has(peerIp) exact-match. Operators
  setting VAULTBASE_TRUSTED_PROXIES=10.0.0.0/8 silently got zero
  trusted proxies. Re-exported parseCidr/ipInCidr/ParsedCidr from
  hook-egress.ts and reused them. List now accepts mixed bare IPs and
  CIDR ranges; per-process cache keyed on raw env string.

L-2 (sandbox table-name interpolation)
  src/core/sql-sandbox.ts — INSERT INTO main."${obj.name}" SELECT *
  FROM _vb_live."${obj.name}" used naive interpolation. Tables created
  outside Vaultbase could conceivably have names containing `"`
  (assertSqlIdent rejects, but the sandbox copies live state). Wrapped
  with a local quoteIdent that doubles embedded quotes.

Author: khalidsheet

src/__tests__/mcp.test.ts

@@ -376,12 +376,14 @@ describe("MCP — Phase 2 admin tools", () => {
     ) as JsonRpcSuccess;
     expect((set.result as CallToolResult).isError).toBeFalsy();
 
+    // get_setting requires mcp:admin — secrets are decrypted in the response,
+    // so a read-only token must not see them.
     const get = await dispatch(
       {
         jsonrpc: "2.0", id: 106, method: "tools/call",
         params: { name: "vaultbase.get_setting", arguments: { key: "test.foo" } },
       },
-      mkCtx(["mcp:read"]),
+      mkCtx(["mcp:admin"]),
     ) as JsonRpcSuccess;
     const text = ((get.result as CallToolResult).content[0] as { text: string }).text;
     const got = JSON.parse(text) as { value: string };

src/api/mcp.ts

@@ -17,7 +17,7 @@
  */
 
 import Elysia from "elysia";
-import { verifyAuthToken, extractBearer } from "../core/sec.ts";
+import { verifyAuthToken, extractBearer, trustedClientIp } from "../core/sec.ts";
 import { hasScope } from "../core/api-tokens.ts";
 import { buildRegistry, createDispatcher } from "../mcp/server.ts";
 import { RPC_ERR, type JsonRpcRequest, type JsonRpcResponse } from "../mcp/types.ts";
@@ -84,7 +84,8 @@ function isJsonRpcRequest(v: unknown): v is JsonRpcRequest {
 
 export function makeMcpPlugin(jwtSecret: string) {
   return new Elysia({ name: "mcp-http" })
-    .post("/mcp", async ({ request, set, body }) => {
+    .post("/mcp", async ({ request, set, body, server }) => {
+      void server; // peer-IP not needed for the POST path
       const auth = await authenticate(request, jwtSecret);
       if ("status" in auth) {
         set.status = auth.status;
@@ -113,7 +114,7 @@ export function makeMcpPlugin(jwtSecret: string) {
       }
       return res;
     })
-    .get("/mcp/events", ({ request, set }) => {
+    .get("/mcp/events", ({ request, set, server }) => {
       // Auth gate: SSE handlers can't easily return JSON, so we return a
       // small text body with the right status. EventSource clients surface
       // this as `onerror` with the status; that's the Right Thing™.
@@ -141,10 +142,15 @@ export function makeMcpPlugin(jwtSecret: string) {
           controller = null;
         };
 
-        const ip =
-          request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ??
-          request.headers.get("x-real-ip") ??
-          null;
+        // Honor VAULTBASE_TRUSTED_PROXIES: only consult X-Forwarded-For when
+        // the immediate peer is in the trust list. Otherwise the peer IP is
+        // authoritative — non-proxied clients can't spoof the origin field.
+        let peerIp: string | null = null;
+        try {
+          const s = server as { requestIP?: (r: Request) => { address: string } | null };
+          peerIp = s?.requestIP?.(request)?.address ?? null;
+        } catch { /* requestIP unsupported in tests */ }
+        const ip = trustedClientIp(request, peerIp);
         const ua = request.headers.get("user-agent");
 
         const stream = new ReadableStream<Uint8Array>({
@@ -156,7 +162,9 @@ export function makeMcpPlugin(jwtSecret: string) {
               scopes: auth.ctx.scopes,
               adminId: auth.ctx.adminId,
               adminEmail: auth.ctx.adminEmail,
-              ...(ip ? { ip } : {}),
+              // Skip the placeholder "unknown" trustedClientIp returns when
+              // no peer IP is resolvable (test transports / direct stdio).
+              ...(ip && ip !== "unknown" ? { ip } : {}),
               ...(ua ? { userAgent: ua } : {}),
               connectedAt: Math.floor(Date.now() / 1000),
               send(payload) {

src/core/hook-egress.ts

@@ -77,7 +77,7 @@ const DEFAULT_DENY: readonly string[] = [
 
 // ── CIDR parsing ────────────────────────────────────────────────────────────
 
-interface ParsedCidr {
+export interface ParsedCidr {
   /** "v4" or "v6" */
   family: "v4" | "v6";
   /** Network bytes — 4 for v4, 16 for v6. */

src/core/records.ts

@@ -183,8 +183,15 @@ function rawClient(): Database {
   return (getDb() as unknown as { $client: Database }).$client;
 }
 
+/**
+ * SQLite identifier quoting — doubles up embedded `"` per spec. Mirrors
+ * `core/collections.ts::quoteIdent`. All identifier inputs at this layer
+ * already pass `assertSqlIdent` upstream (no quote chars possible), but the
+ * defense-in-depth match prevents future bypasses if a path skips that
+ * upstream check.
+ */
 function quoteIdent(name: string): string {
-  return `"${name}"`;
+  return `"${name.replace(/"/g, '""')}"`;
 }
 
 function isJsonField(field: FieldDef): boolean {
@@ -299,13 +306,35 @@ export async function listRecords(
   // Build ORDER BY. View collections don't necessarily expose created_at /
   // updated_at, so skip the default sort there — caller can opt in by passing
   // a `sort` they know is valid for their query.
+  //
+  // Whitelist sort columns against the actual collection schema to refuse any
+  // attacker-supplied identifier from reaching the SQL string. Without this
+  // gate, a value like `sort=name";<sql>;--` would land inside the prepared
+  // statement string. SQLite's prepare_v2 only compiles the first statement
+  // (so multi-statement injection is blocked today), but error-based oracles
+  // and any future API change to multi-statement exec would turn this into
+  // RCE — fail fast instead of relying on the prepare contract.
+  const sortableCols = new Set<string>([
+    "id", "created_at", "updated_at",
+    ...fields.filter((f) => !f.system).map((f) => f.name),
+  ]);
   const sortSpec = opts.sort ?? (col.type === "view" ? "" : "-created_at");
-  const orderClauses = sortSpec.split(",").map((s) => s.trim()).filter(Boolean).map((s) => {
+  const orderClauses: string[] = [];
+  for (const s of sortSpec.split(",").map((x) => x.trim()).filter(Boolean)) {
     const desc = s.startsWith("-");
     const field = desc ? s.slice(1) : s;
-    const colName = field === "created" ? "created_at" : field === "updated" ? "updated_at" : field;
-    return `${tableRef}.${quoteIdent(colName)} ${desc ? "DESC" : "ASC"}`;
-  });
+    const colName = field === "created" ? "created_at"
+      : field === "updated" ? "updated_at" : field;
+    // View collections accept any column name (the schema is inferred from
+    // the SELECT and may not be fully reflected in `fields`); base + auth
+    // collections strictly whitelist.
+    if (col.type !== "view" && !sortableCols.has(colName)) continue;
+    // Defense-in-depth even after whitelist: reject any value that doesn't
+    // match a SQL identifier regex. Whitelist covers it for non-view, this
+    // is the view-collection-fallback safety net.
+    if (!/^[A-Za-z_][A-Za-z0-9_]{0,62}$/.test(colName)) continue;
+    orderClauses.push(`${tableRef}.${quoteIdent(colName)} ${desc ? "DESC" : "ASC"}`);
+  }
   const orderSql = orderClauses.length > 0 ? `ORDER BY ${orderClauses.join(", ")}` : "";
 
   // Execute SELECT — prepared statement cached by SQL string shape.

src/core/sec.ts

@@ -3,6 +3,7 @@ import { eq } from "drizzle-orm";
 import { getDb } from "../db/client.ts";
 import { admin, apiTokens, tokenRevocations } from "../db/schema.ts";
 import type { AuthContext } from "./rules.ts";
+import { parseCidr, ipInCidr, type ParsedCidr } from "./hook-egress.ts";
 
 /**
  * Centralized auth-token verification.
@@ -265,14 +266,54 @@ export function isAllowedUploadMime(mime: string): boolean {
 }
 
 /**
- * IP extraction that respects only the immediate proxy when the listening
- * peer is in `VAULTBASE_TRUSTED_PROXIES`. Returns the socket peer otherwise.
+ * IP extraction that respects `X-Forwarded-For` only when the immediate peer
+ * is in `VAULTBASE_TRUSTED_PROXIES`. Returns the socket peer otherwise.
+ *
+ * Trust list accepts both bare IPs (`10.0.0.5`) and CIDR ranges
+ * (`10.0.0.0/8`, `2001:db8::/32`) — the typical "everything from my private
+ * cluster" case shouldn't require enumerating every replica IP. Backwards-
+ * compatible with the v0.10 string-set form: bare IPs without `/` are still
+ * matched as exact peers.
+ *
+ * Parsed CIDR list is cached per-process; env reads are cheap but the regex
+ * parse isn't.
  */
+let _trustedProxiesCache: { raw: string; cidrs: ParsedCidr[]; bare: Set<string> } | null = null;
+
+function getTrustedProxies(): { cidrs: ParsedCidr[]; bare: Set<string> } | null {
+  const raw = process.env["VAULTBASE_TRUSTED_PROXIES"] ?? "";
+  if (!raw) return null;
+  if (_trustedProxiesCache && _trustedProxiesCache.raw === raw) {
+    return { cidrs: _trustedProxiesCache.cidrs, bare: _trustedProxiesCache.bare };
+  }
+  const cidrs: ParsedCidr[] = [];
+  const bare = new Set<string>();
+  for (const tok of raw.split(",").map((s) => s.trim()).filter(Boolean)) {
+    if (tok.includes("/")) {
+      const c = parseCidr(tok);
+      if (c) cidrs.push(c);
+      // Silently skip malformed entries; operator notices via logs/metrics.
+    } else {
+      bare.add(tok);
+    }
+  }
+  _trustedProxiesCache = { raw, cidrs, bare };
+  return { cidrs, bare };
+}
+
+function isTrustedProxy(ip: string): boolean {
+  const t = getTrustedProxies();
+  if (!t) return false;
+  if (t.bare.has(ip)) return true;
+  for (const c of t.cidrs) {
+    if (ipInCidr(ip, c)) return true;
+  }
+  return false;
+}
+
 export function trustedClientIp(request: Request, peerIp: string | null): string {
-  const trustedRaw = process.env["VAULTBASE_TRUSTED_PROXIES"] ?? "";
-  if (!trustedRaw || !peerIp) return peerIp ?? "unknown";
-  const trusted = new Set(trustedRaw.split(",").map((s) => s.trim()).filter(Boolean));
-  if (!trusted.has(peerIp)) return peerIp;
+  if (!peerIp) return "unknown";
+  if (!isTrustedProxy(peerIp)) return peerIp;
   const fwd = request.headers.get("x-forwarded-for");
   if (fwd) {
     const first = fwd.split(",")[0]?.trim();

src/core/sql-sandbox.ts

@@ -25,6 +25,11 @@ import { Database } from "bun:sqlite";
 
 export const SANDBOX_IDLE_TTL_SEC = 60 * 60; // 1h
 
+/** SQLite identifier quoting — doubles up embedded `"` per spec. */
+function quoteIdent(name: string): string {
+  return `"${name.replace(/"/g, '""')}"`;
+}
+
 interface SandboxSlot {
   db: Database;
   /** Unix-seconds the snapshot was created. */
@@ -103,7 +108,11 @@ export function resetSandbox(adminId: string, livePath: string): SandboxInfo {
       try {
         db.exec(obj.sql);
         if (obj.type === "table") {
-          db.exec(`INSERT INTO main."${obj.name}" SELECT * FROM _vb_live."${obj.name}"`);
+          // quoteIdent escapes embedded `"`. Tables created outside Vaultbase
+          // could conceivably have such names; the upstream `assertSqlIdent`
+          // path can't (rejects quote chars), so this is defense-in-depth.
+          const ident = quoteIdent(obj.name);
+          db.exec(`INSERT INTO main.${ident} SELECT * FROM _vb_live.${ident}`);
         }
       } catch { /* skip objects that fail to recreate (rare, e.g. virtual tables) */ }
     }

src/mcp/admin-write-tools.ts

@@ -529,21 +529,25 @@ export function registerAdminWriteTools(reg: ToolRegistry): void {
 
   // ── settings ───────────────────────────────────────────────────────────
 
+  // Settings tools require mcp:admin (not mcp:read) — the response decrypts
+  // smtp.password / oauth2.<provider>.client_secret / OneSignal app key /
+  // FCM service-account JSON / etc. An mcp:read token is the "least-privileged
+  // observer" scope and must never see operator secrets.
   reg.register({
-    requiredScope: "mcp:read",
+    requiredScope: "mcp:admin",
     definition: {
       name: "vaultbase.list_settings",
-      description: "List every setting key/value. Encrypted-at-rest keys are decrypted in this response (admin-equivalent visibility) — treat as sensitive.",
+      description: "List every setting key/value. Encrypted-at-rest keys are decrypted in this response (admin-equivalent visibility) — requires mcp:admin scope.",
       inputSchema: { type: "object", properties: {}, additionalProperties: false },
     },
     handler: async () => asJsonText(getAllSettings()),
   });
 
   reg.register({
-    requiredScope: "mcp:read",
+    requiredScope: "mcp:admin",
     definition: {
       name: "vaultbase.get_setting",
-      description: "Read a single setting by key. Returns the (decrypted, when applicable) value.",
+      description: "Read a single setting by key. Returns the (decrypted, when applicable) value — requires mcp:admin scope.",
       inputSchema: {
         type: "object",
         properties: { key: { type: "string" } },
@@ -599,13 +603,32 @@ export function registerAdminWriteTools(reg: ToolRegistry): void {
     handler: async (args) => {
       const q = String(args.query ?? "").trim();
       if (!q) throw new Error("query is required");
-      const isSelect = /^\s*(WITH|SELECT|EXPLAIN|PRAGMA)\b/i.test(q);
-      if (!isSelect && args.allow_write !== true) {
-        throw new Error("non-SELECT requires allow_write: true (write paths bypass RBAC + validation; consider the typed tool instead)");
+      const looksLikeRead = /^\s*(WITH|SELECT|EXPLAIN|PRAGMA)\b/i.test(q);
+      if (args.allow_write !== true) {
+        // SQLite supports data-modifying CTEs (`WITH x AS (...) UPDATE ...`),
+        // and PRAGMA can mutate session state — the prefix check above is
+        // necessary but not sufficient. Strip comments + string literals so
+        // the literal "DELETE" inside a WHERE doesn't false-positive, then
+        // scan for any mutating keyword. Mirrors core/sql-runner.ts.
+        if (!looksLikeRead) {
+          throw new Error("non-SELECT requires allow_write: true (write paths bypass RBAC + validation; consider the typed tool instead)");
+        }
+        const stripped = q
+          .replace(/--[^\n]*/g, " ")
+          .replace(/\/\*[\s\S]*?\*\//g, " ")
+          .replace(/'(?:[^']|'')*'/g, "''")
+          .replace(/"(?:[^"]|"")*"/g, '""');
+        const upper = stripped.toUpperCase();
+        const banned = ["INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE", "REPLACE", "TRUNCATE", "ATTACH", "DETACH", "REINDEX", "VACUUM"];
+        for (const kw of banned) {
+          if (new RegExp(`\\b${kw}\\b`).test(upper)) {
+            throw new Error(`'${kw}' is blocked without allow_write: true (data-modifying ${kw} can hide inside a WITH-CTE).`);
+          }
+        }
       }
       const params = (Array.isArray(args.params) ? args.params : []) as Array<string | number | bigint | boolean | null | Uint8Array>;
       const client = getRawClient();
-      if (isSelect) {
+      if (looksLikeRead) {
         const stmt = client.query(q);
         const rows = stmt.all(...params) as unknown[];
         const truncated = rows.slice(0, 100);

src/server.ts

@@ -79,9 +79,15 @@ async function verifyTokenForWS(token: string, jwtSecret: string): Promise<WSAut
  * True if `origin` is in the configured allowlist. Empty/missing settings →
  * deny cross-origin (WS upgrades from any non-same-origin caller fail).
  * Comma-separated list under `security.allowed_origins`.
+ *
+ * Null Origin: handled per call-site — pass `requireOrigin: true` for WS/SSE
+ * upgrade paths where browsers ALWAYS send Origin (so a missing header is
+ * either a non-browser client we can't authenticate by origin, or a
+ * deliberate spoof attempt). Pass false (the default) for cross-cutting
+ * checks where same-origin browser fetches may legitimately omit Origin.
  */
-function isOriginAllowed(origin: string | null): boolean {
-  if (!origin) return true; // same-origin requests omit Origin in some clients
+function isOriginAllowed(origin: string | null, requireOrigin = false): boolean {
+  if (!origin) return !requireOrigin;
   const settings = getAllSettings();
   const raw = settings["security.allowed_origins"] ?? "";
   if (!raw) return false;
@@ -243,7 +249,10 @@ export function createServer(config: Config) {
     // `POST /api/v1/realtime` for setting subscriptions.
     .get("/api/v1/realtime", ({ request, set }) => {
       const origin = request.headers.get("origin");
-      if (!isOriginAllowed(origin)) {
+      // requireOrigin: true — browsers always send Origin on EventSource;
+      // a missing header here is either a non-browser client we can't
+      // authenticate by origin, or a deliberate spoof attempt.
+      if (!isOriginAllowed(origin, true)) {
         set.status = 403;
         return { error: "Origin not allowed", code: 403 };
       }
@@ -284,7 +293,8 @@ export function createServer(config: Config) {
         const req = (ws.data as { request?: Request } | undefined)?.request;
         if (req) {
           const origin = req.headers.get("origin");
-          if (!isOriginAllowed(origin)) {
+          // requireOrigin: true — browsers always send Origin on WS upgrades.
+          if (!isOriginAllowed(origin, true)) {
             ws.send(JSON.stringify({ type: "error", reason: "origin_not_allowed" }));
             ws.close();
             return;