fix: close 5 findings from Oracle audit wave 4 (2 security P0 + 3 correctness)

Three parallel read-only Oracles (search/recall; tagger core + token caching;
CLI + config security). Cores verified safe (composite tag identity, FIFO owner
derivation, tagger cache invalidation, strip/replay purity; unified single-embedding,
git-commit execFile safety, no sqlite-vec; runtime config secret boundary + SSRF
guard + atomic raw-merge). 5 source-confirmed fixes; cache/embedding-identity items
banked (D2 reinforced, D10-D14).

SECURITY P0 (the malicious-repo boundary — CLI side bypassed the runtime's hardening):
- Pi doctor embedding probe expanded project-config {env:}/{file:} tokens
  (doctor-pi.ts). A repo's .pi/magic-context.jsonc could resolve
  {env:ANTHROPIC_API_KEY} and we'd send it to a repo-chosen endpoint. Now read
  project config with isProjectConfig:true (tokens stay literal), strip unsafe
  project fields, and drop the inherited user api_key on endpoint redirect —
  mirroring the runtime loader.
- Pi doctor/issue redaction was weaker than OpenCode (diagnostics-pi.ts): a log
  line with github_pat_/ghp_/AKIA/hf_/JWT/Slack tokens was NOT redacted and could
  land in a public `doctor --issue` report; the raw "Last plugin log line" was
  printed unsanitized. Now delegate to the shared comprehensive redactor (kept the
  original looser sk-/Bearer/key= patterns as a SUPERSET so short tokens still
  redact), and sanitizeDiagnosticText the last-log-line print.

CORRECTNESS:
- message-index single-message double-insert (message-index.ts). indexSingleMessage
  used a DEFERRED transaction; FTS5 has no UNIQUE, so two processes handling the
  same terminal message.updated could both pass the already-indexed check and
  double-insert. Switched to BEGIN IMMEDIATE so the in-lock re-check serializes —
  mirrors indexMessagesAfterOrdinal.
- search ordinal cutoff applied after LIMIT (search.ts). runMessageFtsQuery
  fetched LIMIT*3 rows then filtered messageOrdinal>cutoff in JS, so when the
  top-ranked rows are all live-tail, older eligible hits below the limit are never
  seen and explicit ctx_search could return nothing. Added a cutoff-aware SQL
  statement (predicate before LIMIT); hot-path auto-search (cutoff=null) unchanged.
- config array-element recovery collapsed to all-defaults (prune-config-leaf.ts).
  An invalid array leaf (e.g. system_prompt_injection.skip_signatures[0]) made
  pruneNestedConfigLeaf return null → recovery dropped the WHOLE config to defaults,
  losing valid siblings. Now prune the nearest owning field (→ its default) when
  the path descends into a non-object. +2 tests.

Gate: plugin 2176/0, Pi 471/0, CLI 179/0, tsc+biome clean all.

Author: ualtinok

packages/cli/src/commands/doctor-pi.ts

@@ -11,7 +11,10 @@ import {
 import { createRequire } from "node:module";
 import { homedir } from "node:os";
 import { dirname, isAbsolute, join } from "node:path";
-
+import {
+    dropInheritedEmbeddingKeyOnRedirect,
+    stripUnsafeProjectConfigFields,
+} from "@magic-context/core/config/project-security";
 import { MagicContextConfigSchema } from "@magic-context/core/config/schema/magic-context";
 import { substituteConfigVariables } from "@magic-context/core/config/variable";
 import {
@@ -48,6 +51,7 @@ import {
     isPiMagicContextPackageEntry,
 } from "../lib/pi-package-entry";
 import { type PromptIO, promptIO } from "../lib/prompts";
+import { sanitizeDiagnosticText } from "../lib/redaction";
 import { runV22BackfillCommands, type V22BackfillCommandArgs } from "../lib/v22-backfill-commands";
 import { writePiSettingsPackage } from "./setup-pi";
 
@@ -264,13 +268,21 @@ function projectConfigPath(cwd: string): string {
     return join(cwd, ".pi", "magic-context.jsonc");
 }
 
-function readConfigForEmbedding(path: string): Record<string, unknown> | null {
+function readConfigForEmbedding(
+    path: string,
+    isProjectConfig: boolean,
+): Record<string, unknown> | null {
     if (!existsSync(path)) return null;
     try {
         const rawText = readFileSync(path, "utf-8");
+        // SECURITY: project-level config must NOT expand {env:}/{file:} tokens —
+        // a malicious repo could otherwise resolve {env:ANTHROPIC_API_KEY} into a
+        // field we then send to a repo-chosen endpoint. Mirror the runtime loader
+        // (isProjectConfig leaves tokens literal for project config).
         const substituted = substituteConfigVariables({
             text: rawText,
             configPath: path,
+            isProjectConfig,
         });
         return parseJsonc(substituted.text) as Record<string, unknown>;
     } catch {
@@ -579,16 +591,33 @@ async function runHealthChecks(options: {
         options.deps.closeDatabase();
     }
 
-    const embeddingConfigs = [userConfigPath, projectPath]
-        .map(readConfigForEmbedding)
-        .filter((config): config is Record<string, unknown> => config !== null);
+    // Read user config (tokens expand) and project config (tokens stay literal —
+    // secret boundary) separately so we can apply the same redirect-drop the
+    // runtime loader does: if the PROJECT redirects embedding.endpoint without its
+    // own api_key, the inherited USER api_key must NOT be merged in (it would be
+    // sent to the repo-chosen endpoint — exfiltration).
+    const userRaw = readConfigForEmbedding(userConfigPath, false);
+    const projectRaw = readConfigForEmbedding(projectPath, true);
+    if (projectRaw) {
+        // Strip project-config fields that must never come from a repo (agent
+        // prompts, sqlite.*, etc.) before they can influence the probe.
+        stripUnsafeProjectConfigFields(projectRaw);
+    }
     const mergedEmbedding: Record<string, unknown> = {};
-    for (const config of embeddingConfigs) {
-        const embedding = config.embedding;
+    for (const config of [userRaw, projectRaw]) {
+        const embedding = config?.embedding;
         if (embedding && typeof embedding === "object" && !Array.isArray(embedding)) {
             Object.assign(mergedEmbedding, embedding);
         }
     }
+    // Drop the inherited user api_key if the project redirected the endpoint.
+    if (projectRaw) {
+        dropInheritedEmbeddingKeyOnRedirect(
+            projectRaw,
+            { embedding: mergedEmbedding },
+            userRaw ?? undefined,
+        );
+    }
     if (mergedEmbedding.provider === "openai-compatible") {
         const endpoint =
             typeof mergedEmbedding.endpoint === "string" ? mergedEmbedding.endpoint.trim() : "";
@@ -715,7 +744,14 @@ async function runHealthChecks(options: {
             .map((line) => line.trim())
             .filter(Boolean);
         add(results, "info", `Log file: ${logPath} (${sizeKb} KB)`);
-        add(results, "info", `Last plugin log line: ${lines.at(-1) ?? "<empty log>"}`);
+        // Sanitize before printing — a raw log line can carry a secret/path the
+        // user then pastes into a public issue.
+        const lastLine = lines.at(-1);
+        add(
+            results,
+            "info",
+            `Last plugin log line: ${lastLine ? sanitizeDiagnosticText(lastLine) : "<empty log>"}`,
+        );
     } else {
         add(results, "info", `No plugin log file yet at ${logPath}`);
     }

packages/cli/src/lib/diagnostics-pi.ts

@@ -24,6 +24,7 @@ import {
     hasPiMagicContextPackage,
     isPiMagicContextPackageEntry,
 } from "./pi-package-entry";
+import { redactSecretText } from "./redaction";
 
 export interface PiConfigDiagnostic {
     path: string;
@@ -162,7 +163,13 @@ function currentUserHash(): string {
 }
 
 function redactSecretString(value: string): string {
-    return value
+    // Apply the shared comprehensive redactor (OpenCode parity: adds
+    // github_pat_/ghp_/hf_/AKIA/Slack/Google/JWT and generic key=value forms that
+    // the bespoke version leaked) AND then the original looser patterns as a
+    // SUPERSET — the shared `sk-` pattern requires 32+ chars (real key length),
+    // so keep the looser `sk-{12,}` here too so short/synthetic sk- tokens are
+    // still caught. Redaction is safer over-broad than under.
+    return redactSecretText(value)
         .replace(/Bearer\s+[A-Za-z0-9._~+\-/=]+/g, "Bearer <REDACTED>")
         .replace(/sk-[A-Za-z0-9_-]{12,}/g, "sk-<REDACTED>")
         .replace(/api[_-]?key=([^\s&]+)/gi, "api_key=<REDACTED>")

packages/plugin/src/config/prune-config-leaf.test.ts

@@ -32,9 +32,32 @@ describe("pruneNestedConfigLeaf", () => {
         expect(block.git_commit_indexing).toEqual({ enabled: false, since_days: 99999 });
     });
 
-    it("returns null when an intermediate path segment is not an object", () => {
-        const block = { git_commit_indexing: true };
-        expect(pruneNestedConfigLeaf(block, ["git_commit_indexing", "since_days"])).toBeNull();
+    it("prunes the owning field when an intermediate segment is not an object", () => {
+        // The path descends into a non-object (here `git_commit_indexing` is a
+        // bare bool, not a block). We can't reach `since_days`, but we prune the
+        // owning field so it falls back to its default — NOT collapse to all
+        // defaults (which dropping to null upstream would cause).
+        const block = { git_commit_indexing: true, keep_me: { enabled: false } };
+        const result = pruneNestedConfigLeaf(block, ["git_commit_indexing", "since_days"]);
+        expect(result).not.toBeNull();
+        expect(result?.removed).toBe("git_commit_indexing");
+        expect(result?.block).toEqual({ keep_me: { enabled: false } });
+    });
+
+    it("prunes an invalid array-element leaf to its owning field", () => {
+        // system_prompt_injection.skip_signatures[0] invalid → Zod path descends
+        // into the array. Prune skip_signatures (→ default), keep siblings.
+        const block = {
+            system_prompt_injection: { enabled: true, skip_signatures: [123] },
+        };
+        const result = pruneNestedConfigLeaf(block, [
+            "system_prompt_injection",
+            "skip_signatures",
+            0,
+        ]);
+        expect(result).not.toBeNull();
+        expect(result?.removed).toBe("system_prompt_injection.skip_signatures");
+        expect(result?.block).toEqual({ system_prompt_injection: { enabled: true } });
     });
 
     it("returns null when the leaf is absent", () => {

packages/plugin/src/config/prune-config-leaf.ts

@@ -39,7 +39,23 @@ export function pruneNestedConfigLeaf(
     for (let i = 0; i < relativePath.length - 1; i++) {
         const seg = String(relativePath[i]);
         const child = cursor[seg];
-        if (!isPlainObject(child)) return null;
+        if (!isPlainObject(child)) {
+            // The path descends into a non-object (e.g. an ARRAY element, like an
+            // invalid `system_prompt_injection.skip_signatures[0]`). We can't reach
+            // the deeper leaf — but we CAN prune the nearest OWNING field (the
+            // array/primitive at `seg`) so it falls back to its schema default,
+            // preserving valid siblings. Without this we'd return null → the whole
+            // config collapses to all-defaults, dropping unrelated user settings.
+            if (!(seg in cursor)) return null;
+            delete cursor[seg];
+            return {
+                block: result,
+                removed: relativePath
+                    .slice(0, i + 1)
+                    .map(String)
+                    .join("."),
+            };
+        }
         const clonedChild: Record<string, unknown> = { ...child };
         cursor[seg] = clonedChild;
         cursor = clonedChild;

packages/plugin/src/features/magic-context/message-index.ts

@@ -188,9 +188,29 @@ function indexSingleMessageInTransaction(
 }
 
 export function indexSingleMessage(db: Database, sessionId: string, message: RawMessage): boolean {
-    return db.transaction(() =>
-        indexSingleMessageInTransaction(db, sessionId, message, Date.now()),
-    )();
+    // BEGIN IMMEDIATE (not a deferred db.transaction): message_history_fts is a
+    // plain FTS5 table with NO UNIQUE constraint, and the dedup is the
+    // isMessageAlreadyIndexed SELECT inside the body. Under a DEFERRED transaction
+    // two processes handling the same terminal message.updated can both pass that
+    // SELECT before either inserts → duplicate FTS rows. Taking the writer lock up
+    // front serializes them, so the second's in-lock re-check sees the first's
+    // insert and skips. Mirrors indexMessagesAfterOrdinal.
+    db.exec("BEGIN IMMEDIATE");
+    let committed = false;
+    try {
+        const result = indexSingleMessageInTransaction(db, sessionId, message, Date.now());
+        db.exec("COMMIT");
+        committed = true;
+        return result;
+    } finally {
+        if (!committed) {
+            try {
+                db.exec("ROLLBACK");
+            } catch {
+                // already closed by an earlier failure
+            }
+        }
+    }
 }
 
 export function indexMessagesAfterOrdinal(

packages/plugin/src/features/magic-context/search.ts

@@ -55,6 +55,7 @@ interface MessageSearchRow {
 }
 
 const messageSearchStatements = new WeakMap<Database, PreparedStatement>();
+const messageSearchStatementsWithCutoff = new WeakMap<Database, PreparedStatement>();
 
 export type SearchSource = "memory" | "message" | "git_commit";
 
@@ -251,6 +252,25 @@ function getMessageSearchStatement(db: Database): PreparedStatement {
     return stmt;
 }
 
+/**
+ * Cutoff-aware variant: filters `message_ordinal <= cutoff` IN SQL, BEFORE the
+ * LIMIT. The JS-side post-filter in runMessageFtsQuery applies the cutoff AFTER
+ * fetching `LIMIT` rows, so when the top-ranked rows are all live-tail (above the
+ * cutoff) they're fetched-then-discarded and older eligible hits below the limit
+ * are never seen — explicit ctx_search could then return nothing. Pushing the
+ * predicate into SQL makes LIMIT count only already-eligible rows.
+ */
+function getMessageSearchStatementWithCutoff(db: Database): PreparedStatement {
+    let stmt = messageSearchStatementsWithCutoff.get(db);
+    if (!stmt) {
+        stmt = db.prepare(
+            "SELECT message_ordinal AS messageOrdinal, message_id AS messageId, role, content FROM message_history_fts WHERE session_id = ? AND message_history_fts MATCH ? AND CAST(message_ordinal AS INTEGER) <= ? ORDER BY bm25(message_history_fts), CAST(message_ordinal AS INTEGER) ASC LIMIT ?",
+        );
+        messageSearchStatementsWithCutoff.set(db, stmt);
+    }
+    return stmt;
+}
+
 const ftsRowCountStatements = new WeakMap<Database, PreparedStatement>();
 const ftsMatchCountStatements = new WeakMap<Database, PreparedStatement>();
 
@@ -614,9 +634,13 @@ function runMessageFtsQuery(
     cutoff: number | null,
 ): NormalizedMessageRow[] {
     if (ftsQuery.length === 0) return [];
-    const rows = getMessageSearchStatement(db)
-        .all(sessionId, ftsQuery, fetchLimit)
-        .map((row) => row as MessageSearchRow);
+    // Apply the ordinal cutoff IN SQL (before LIMIT) so live-tail matches can't
+    // crowd out older eligible hits; null cutoff keeps the original statement.
+    const rows = (
+        cutoff !== null
+            ? getMessageSearchStatementWithCutoff(db).all(sessionId, ftsQuery, cutoff, fetchLimit)
+            : getMessageSearchStatement(db).all(sessionId, ftsQuery, fetchLimit)
+    ).map((row) => row as MessageSearchRow);
 
     const result: NormalizedMessageRow[] = [];
     for (const row of rows) {
@@ -629,7 +653,8 @@ function runMessageFtsQuery(
         ) {
             continue;
         }
-        // Skip messages still in the live context (not yet compartmentalized).
+        // Defense-in-depth: the SQL cutoff above already excluded these, but keep
+        // the guard so a future caller that skips the cutoff statement stays safe.
         if (cutoff !== null && messageOrdinal > cutoff) {
             continue;
         }