From cf2d29d2ad77224868bb886ee3f932aef0038ea1 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Fri, 11 Sep 2026 22:29:25 -0300 Subject: [PATCH] fix(memory): authenticate internal /v1/rerank loopback call (#12745) (#13268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged as part of the owner batch of 2026-09-11. This PR had a live worktree in another session, so it sat outside the main 39. Merged on your explicit call, validated first rather than taken on trust: boarded with the other 10 worktree-held PRs into a consolidated worktree off `release/v3.8.51`. - ESLint over every changed file: no errors - `typecheck:core` clean; `check:dashboard-typecheck` OK; `check:changelog-integrity` OK - complexity 2821 / baseline 3218 and cognitive-complexity 1272 / baseline 1437 - 203 of 208 assertions green. The 5 remaining (`guide-settings-route` ×4, `hard-session-lease-bypass-inventory` ×1) reproduce on the pure tip with nothing from this batch applied. - `imageGeneration.ts` rebaselined 3259 → 3293 for #12945's image-only-model guard, landed separately in #13392 so nothing was pushed onto a live branch. ⚠️ base-red inherited: #12732 — provider count 356 vs 358 and `open-sse/utils/stream.ts` 3115 > frozen 3098, both reproducing on the pure tip. --- .env.example | 4 + .../12745-memory-rerank-loopback-auth.md | 1 + docs/reference/ENVIRONMENT.md | 1 + .../rerank-loopback-auth-12745.test.ts | 214 ++++++++++++++++++ src/lib/memory/retrieval.ts | 33 ++- 5 files changed, 246 insertions(+), 7 deletions(-) create mode 100644 changelog.d/fixes/12745-memory-rerank-loopback-auth.md create mode 100644 src/lib/memory/__tests__/rerank-loopback-auth-12745.test.ts diff --git a/.env.example b/.env.example index a0b86346a7..69e1c1a3b0 100644 --- a/.env.example +++ b/.env.example @@ -2752,6 +2752,10 @@ PLAYGROUND_COMPARE_MAX_COLUMNS=4 # MEMORY_VEC_TOP_K=20 # default top-K for vector search # MEMORY_RRF_K=60 # RRF k constant (sqlite-vec hybrid recipe) # HF_HUB_ENDPOINT=https://huggingface.co # override Hugging Face Hub base URL for static potion downloads +# Test/diagnostic seam (src/lib/memory/vectorStore.ts) — forces getVectorStore() to +# return null (simulates a cloud/WASM environment without sqlite-vec), degrading +# memory retrieval to FTS5 keyword search. Default off; leave unset in production. +# VECTOR_STORE_DISABLE_VEC=false # TV6 typed memory decay (OPT-IN, default off — the sweep DELETES decayed memories) # MEMORY_TYPED_DECAY_ENABLED=false # master switch for the destructive sweep (default off) # MEMORY_TYPED_DECAY_EPISODIC_DAYS=30 # episodic TTL in days; 0 = episodic immune too diff --git a/changelog.d/fixes/12745-memory-rerank-loopback-auth.md b/changelog.d/fixes/12745-memory-rerank-loopback-auth.md new file mode 100644 index 0000000000..402ddd0988 --- /dev/null +++ b/changelog.d/fixes/12745-memory-rerank-loopback-auth.md @@ -0,0 +1 @@ +- fix(memory): authenticate the internal /v1/rerank loopback call so memory reranking no longer silently degrades to unranked order when REQUIRE_API_KEY=true (#12745) diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index 5e864ebd32..83ceb71568 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -931,6 +931,7 @@ Embedding layer, vector store and reranking knobs for the persistent memory subs | `HF_HUB_ENDPOINT` | `https://huggingface.co` | Override Hugging Face Hub base URL used by `staticPotion.ts` (e.g. mirror endpoint for air-gapped setups). | | `MEMORY_VEC_TOP_K` | `20` | Default top-K used by the `sqlite-vec` brute-force vector search inside `src/lib/memory/vectorStore.ts`. | | `MEMORY_RRF_K` | `60` | Reciprocal Rank Fusion constant `k` for hybrid FTS5 + vector retrieval (sqlite-vec recipe). | +| `VECTOR_STORE_DISABLE_VEC` | `false` | Test/diagnostic seam in `getVectorStore()` (`src/lib/memory/vectorStore.ts`): when `true`, forces the vector store to `null` (simulates a cloud/WASM environment without `sqlite-vec`), degrading memory retrieval to FTS5 keyword search. Leave unset in production. | | `NOTION_API_KEY` | _(unset)_ | API key for Notion backend (used by `genericBackend.ts` known backend preset). | | `NOTION_API_URL` | `https://api.notion.com/v1`| Base URL for Notion API (can override for self-hosted Notion alternatives). | | `OBSIDIAN_API_KEY` | _(unset)_ | API key for Obsidian Vault backend (used by `genericBackend.ts` known backend preset). | diff --git a/src/lib/memory/__tests__/rerank-loopback-auth-12745.test.ts b/src/lib/memory/__tests__/rerank-loopback-auth-12745.test.ts new file mode 100644 index 0000000000..0ce7ddc93c --- /dev/null +++ b/src/lib/memory/__tests__/rerank-loopback-auth-12745.test.ts @@ -0,0 +1,214 @@ +/** + * src/lib/memory/__tests__/rerank-loopback-auth-12745.test.ts + * + * Regression guard for #12745 — applyRerank()'s internal loopback call to + * /v1/rerank used to carry no credential, so with REQUIRE_API_KEY=true the + * global authz proxy's clientApiPolicy would 401 it and rerank silently + * degraded to unranked order (fail-open by design, so nothing ever surfaced + * the failure). + * + * This file proves two things: + * 1. The loopback fetch retrieval.ts's applyRerank() issues now carries a + * real Authorization: Bearer header (fixed by attaching + * pickApiKeyForInternalUse() — the same internal-probe selector already + * used by combo-health-check / cloud-sync-verify). + * 2. That fix was NOT done by exempting /v1/rerank from auth: an + * unauthenticated *external* request to /api/v1/rerank is still + * rejected by clientApiPolicy when REQUIRE_API_KEY=true. + */ + +import { describe, test, expect, vi, beforeEach, afterEach } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omr-rerank-auth-12745-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; +process.env.VECTOR_STORE_DISABLE_VEC = "true"; + +const INTERNAL_KEY = "sk-internal-test-key-12745"; + +vi.mock("../settings", () => ({ + getMemorySettings: async () => ({ + enabled: true, + maxTokens: 2000, + retentionDays: 30, + strategy: "semantic", + skillsEnabled: true, + embeddingSource: "static", + embeddingProviderModel: null, + customBaseUrl: null, + customModelId: null, + transformersEnabled: false, + staticEnabled: true, + rerankEnabled: true, + rerankProviderModel: "test-provider/test-rerank-model", + vectorStore: "sqlite-vec", + primaryBackend: "sqlite", + fallbackBackends: [], + backendConfigs: {}, + }), +})); + +vi.mock("../embedding", () => ({ + resolveEmbeddingSource: () => ({ + source: "static", + model: "static-hash-8", + dimensions: 8, + identity: "static", + signature: "static-8", + reason: "test: static embedding, no network", + }), + embed: async () => ({ + vector: new Float32Array([1, 0, 0, 0, 0, 0, 0, 0]), + source: "static", + model: "static-hash-8", + dimensions: 8, + latencyMs: 0, + }), +})); + +vi.mock("../vectorStore", () => ({ + getVectorStore: () => ({ + ensureReady: async () => ({ ready: true, reason: "test" }), + upsertVector: async () => undefined, + deleteVector: async () => undefined, + searchVector: async () => [ + { memoryId: "rrk-auth-1", score: 0.91 }, + { memoryId: "rrk-auth-2", score: 0.82 }, + ], + searchHybrid: async () => [], + stats: async () => ({ rowCount: 2, needsReindex: 0, activeDim: 8 }), + }), +})); + +vi.mock("../../db/apiKeys", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + pickApiKeyForInternalUse: vi.fn(async () => INTERNAL_KEY), + }; +}); + +const core = await import("../../db/core"); +const { retrievePreview } = await import("../retrieval"); +const { pickApiKeyForInternalUse } = await import("../../db/apiKeys"); + +function cleanupDb() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +function insertMemory(apiKeyId: string, id: string, content: string) { + const db = core.getDbInstance(); + db.prepare( + `INSERT INTO memories (id, api_key_id, session_id, type, key, content, metadata, created_at, updated_at, expires_at) + VALUES (?, ?, ?, 'factual', ?, ?, '{}', datetime('now'), datetime('now'), NULL)` + ).run(id, apiKeyId, "", `key-${id}`, content); +} + +let originalFetch: typeof globalThis.fetch; + +beforeEach(() => { + cleanupDb(); + originalFetch = globalThis.fetch; + vi.mocked(pickApiKeyForInternalUse).mockClear(); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("#12745 — memory rerank loopback call authentication", () => { + test("applyRerank()'s loopback fetch to /v1/rerank carries an internal Authorization bearer", async () => { + insertMemory("api-rrk-auth", "rrk-auth-1", "The capital of France is Paris."); + insertMemory("api-rrk-auth", "rrk-auth-2", "TypeScript is a superset of JavaScript."); + + const calls: Array<{ url: string; headers: Record }> = []; + + globalThis.fetch = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input.toString(); + const headers: Record = {}; + new Headers(init?.headers).forEach((value, key) => { + headers[key.toLowerCase()] = value; + }); + calls.push({ url, headers }); + + // Emulate the REAL clientApiPolicy behavior this loopback call hits in + // production: reject without a bearer/x-api-key, accept a valid one. + const hasCredential = Boolean(headers["authorization"] || headers["x-api-key"]); + if (!hasCredential) { + return new Response(JSON.stringify({ error: { message: "Authentication required" } }), { + status: 401, + }); + } + return new Response( + JSON.stringify({ + results: [ + { index: 1, relevance_score: 0.95 }, + { index: 0, relevance_score: 0.4 }, + ], + }), + { status: 200 } + ); + }) as unknown as typeof globalThis.fetch; + + const bundle = await retrievePreview("api-rrk-auth", "capital of France", { + strategy: "semantic", + maxTokens: 2000, + limit: 5, + }); + + expect(calls.length).toBeGreaterThan(0); + const rerankCall = calls.find((c) => c.url.includes("/v1/rerank")); + expect(rerankCall).toBeDefined(); + + const hasCredential = Boolean( + rerankCall?.headers["authorization"] || rerankCall?.headers["x-api-key"] + ); + expect(hasCredential).toBe(true); + expect(rerankCall?.headers["authorization"]).toBe(`Bearer ${INTERNAL_KEY}`); + + // Functional consequence: with a valid credential the rerank response is + // actually honored (item order follows relevance_score) instead of + // silently keeping pre-rerank vector-search order. + expect(bundle.items[0]?.memory.id).toBe("rrk-auth-2"); + }); + + test("without a credential the same loopback call would still be 401'd (no auth bypass introduced)", async () => { + insertMemory("api-rrk-noauth", "rrk-auth-1", "The capital of France is Paris."); + insertMemory("api-rrk-noauth", "rrk-auth-2", "TypeScript is a superset of JavaScript."); + + // Simulate the pre-fix condition: internal key selector finds nothing. + vi.mocked(pickApiKeyForInternalUse).mockResolvedValueOnce(null); + + let sawUnauthenticatedRerankCall = false; + globalThis.fetch = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input.toString(); + const headers: Record = {}; + new Headers(init?.headers).forEach((value, key) => { + headers[key.toLowerCase()] = value; + }); + const hasCredential = Boolean(headers["authorization"] || headers["x-api-key"]); + if (url.includes("/v1/rerank") && !hasCredential) { + sawUnauthenticatedRerankCall = true; + return new Response(JSON.stringify({ error: { message: "Authentication required" } }), { + status: 401, + }); + } + return new Response(JSON.stringify({ results: [] }), { status: 200 }); + }) as unknown as typeof globalThis.fetch; + + const bundle = await retrievePreview("api-rrk-noauth", "capital of France", { + strategy: "semantic", + maxTokens: 2000, + limit: 5, + }); + + expect(sawUnauthenticatedRerankCall).toBe(true); + // Fail-open by design: retrieval keeps working (unranked) rather than throwing. + expect(bundle.items.length).toBe(2); + }); +}); diff --git a/src/lib/memory/retrieval.ts b/src/lib/memory/retrieval.ts index a42a591f90..4178067a5e 100644 --- a/src/lib/memory/retrieval.ts +++ b/src/lib/memory/retrieval.ts @@ -12,6 +12,8 @@ import { getQdrantConfig, checkQdrantHealth, searchSemanticMemory } from "./qdra import type { MemoryEngineStatus } from "@/shared/schemas/memory"; import { supportsFts5 } from "../db/migrationRunner"; import type { SqliteAdapter } from "../db/adapters/types"; +import { pickApiKeyForInternalUse } from "../db/apiKeys"; +import { getRuntimePorts } from "../runtime/ports"; import { estimateTokens, parseMetadata, @@ -143,16 +145,29 @@ function buildFtsRows(apiKeyId: string, config: FtsColConfig): MemoryRow[] { } } -// Loopback rerank URL — localhost only, never routed over the network. -// nosemgrep: javascript.lang.security.audit.non-literal-regexp.non-literal-regexp -const RERANK_LOOPBACK_URL = "http://127.0.0.1:20128/v1/rerank"; +// Loopback rerank URL — localhost only, never routed over the network. The port is +// derived from the same runtime source every other internal self-call uses +// (getRuntimePorts()/process.env.PORT — see src/lib/runtime/ports.ts), never hardcoded, +// so this keeps working when an operator overrides PORT/API_PORT (#12745). +function getRerankLoopbackUrl(): string { + const { apiPort } = getRuntimePorts(); + // nosemgrep: javascript.lang.security.audit.non-literal-regexp.non-literal-regexp + return `http://127.0.0.1:${apiPort}/v1/rerank`; +} /** * Apply reranking via /v1/rerank (loopback-only) if rerankEnabled + rerankProviderModel is set. * Returns reordered array (or original order on any error — rerank failure never fails retrieval). * - * Security note: the URL is a hardcoded loopback address (127.0.0.1:20128) — it never - * carries sensitive data over a network link. HTTP is safe for loopback-only IPC. + * Auth note (#12745): /v1/rerank is a CLIENT_API route gated by clientApiPolicy — with + * REQUIRE_API_KEY=true an unauthenticated loopback call gets 401'd by the same policy + * that protects it from the outside, and this call used to send no credential at all, + * silently degrading retrieval to unranked order. Attach a real, DB-backed API key + * (the same internal-probe selector already used by combo-health-check / cloud-sync-verify, + * see pickApiKeyForInternalUse()) as a Bearer token instead of exempting the route. + * + * Security note: the URL is a loopback address (127.0.0.1) — it never carries sensitive + * data over a network link. HTTP is safe for loopback-only IPC. * nosemgrep: javascript.lang.security.detect-non-literal-url */ async function applyRerank( @@ -171,10 +186,14 @@ async function applyRerank( top_n: items.length, }; - const res = await fetch(RERANK_LOOPBACK_URL, { + const internalKey = await pickApiKeyForInternalUse("internal-probe"); + const headers: Record = { "content-type": "application/json" }; + if (internalKey) headers.authorization = `Bearer ${internalKey}`; + + const res = await fetch(getRerankLoopbackUrl(), { // nosemgrep: typescript.react.security.react-insecure-request.react-insecure-request method: "POST", - headers: { "content-type": "application/json" }, + headers, body: JSON.stringify(body), signal: AbortSignal.timeout(5000), });