From f4c116ac0b14d8da81b4d474688e619be72d1bc8 Mon Sep 17 00:00:00 2001 From: joshavant <830519+joshavant@users.noreply.github.com> Date: Fri, 6 Mar 2026 17:15:29 -0600 Subject: [PATCH] Secrets: avoid header marker collisions and audit false positives --- src/agents/pi-embedded-runner/model.test.ts | 35 ++++++++++- src/agents/pi-embedded-runner/model.ts | 17 ++++-- .../runner.deepgram.test.ts | 6 +- src/media-understanding/runner.entries.ts | 3 +- src/secrets/audit.test.ts | 61 +++++++++++++++++++ src/secrets/audit.ts | 43 +++++++++++++ 6 files changed, 153 insertions(+), 12 deletions(-) diff --git a/src/agents/pi-embedded-runner/model.test.ts b/src/agents/pi-embedded-runner/model.test.ts index 52c943f7c54..ca12a76cb36 100644 --- a/src/agents/pi-embedded-runner/model.test.ts +++ b/src/agents/pi-embedded-runner/model.test.ts @@ -180,7 +180,7 @@ describe("buildInlineProviderModels", () => { expect(result[0].headers).toBeUndefined(); }); - it("drops SecretRef marker headers when building inline provider models", () => { + it("preserves literal marker-shaped headers in inline provider models", () => { const providers: Parameters[0] = { custom: { headers: { @@ -195,7 +195,11 @@ describe("buildInlineProviderModels", () => { const result = buildInlineProviderModels(providers); expect(result).toHaveLength(1); - expect(result[0].headers).toEqual({ "X-Static": "tenant-a" }); + expect(result[0].headers).toEqual({ + Authorization: "secretref-env:OPENAI_HEADER_TOKEN", + "X-Managed": "secretref-managed", + "X-Static": "tenant-a", + }); }); }); @@ -241,7 +245,7 @@ describe("resolveModel", () => { }); }); - it("drops marker-backed provider headers in fallback models", () => { + it("preserves literal marker-shaped provider headers in fallback models", () => { const cfg = { models: { providers: { @@ -262,10 +266,35 @@ describe("resolveModel", () => { expect(result.error).toBeUndefined(); expect((result.model as unknown as { headers?: Record }).headers).toEqual({ + Authorization: "secretref-env:OPENAI_HEADER_TOKEN", + "X-Managed": "secretref-managed", "X-Custom-Auth": "token-123", }); }); + it("drops marker headers from discovered models.json entries", () => { + mockDiscoveredModel({ + provider: "custom", + modelId: "listed-model", + templateModel: { + ...makeModel("listed-model"), + provider: "custom", + headers: { + Authorization: "secretref-env:OPENAI_HEADER_TOKEN", + "X-Managed": "secretref-managed", + "X-Static": "tenant-a", + }, + }, + }); + + const result = resolveModel("custom", "listed-model", "/tmp/agent"); + + expect(result.error).toBeUndefined(); + expect((result.model as unknown as { headers?: Record }).headers).toEqual({ + "X-Static": "tenant-a", + }); + }); + it("prefers matching configured model metadata for fallback token limits", () => { const cfg = { models: { diff --git a/src/agents/pi-embedded-runner/model.ts b/src/agents/pi-embedded-runner/model.ts index 082ff8a0c70..f1b31a5e49a 100644 --- a/src/agents/pi-embedded-runner/model.ts +++ b/src/agents/pi-embedded-runner/model.ts @@ -23,13 +23,19 @@ type InlineProviderConfig = { headers?: unknown; }; -function sanitizeModelHeaders(headers: unknown): Record | undefined { +function sanitizeModelHeaders( + headers: unknown, + opts?: { stripSecretRefMarkers?: boolean }, +): Record | undefined { if (!headers || typeof headers !== "object" || Array.isArray(headers)) { return undefined; } const next: Record = {}; for (const [headerName, headerValue] of Object.entries(headers)) { - if (typeof headerValue !== "string" || isSecretRefHeaderValueMarker(headerValue)) { + if (typeof headerValue !== "string") { + continue; + } + if (opts?.stripSecretRefMarkers && isSecretRefHeaderValueMarker(headerValue)) { continue; } next[headerName] = headerValue; @@ -63,11 +69,14 @@ function applyConfiguredProviderOverrides(params: { if (!providerConfig) { return { ...discoveredModel, - headers: sanitizeModelHeaders(discoveredModel.headers), + // Discovered models originate from models.json and may contain persistence markers. + headers: sanitizeModelHeaders(discoveredModel.headers, { stripSecretRefMarkers: true }), }; } const configuredModel = providerConfig.models?.find((candidate) => candidate.id === modelId); - const discoveredHeaders = sanitizeModelHeaders(discoveredModel.headers); + const discoveredHeaders = sanitizeModelHeaders(discoveredModel.headers, { + stripSecretRefMarkers: true, + }); const providerHeaders = sanitizeModelHeaders(providerConfig.headers); const configuredHeaders = sanitizeModelHeaders(configuredModel?.headers); if (!configuredModel && !providerConfig.baseUrl && !providerConfig.api && !providerHeaders) { diff --git a/src/media-understanding/runner.deepgram.test.ts b/src/media-understanding/runner.deepgram.test.ts index a2e52aaec3e..253c8d6eefa 100644 --- a/src/media-understanding/runner.deepgram.test.ts +++ b/src/media-understanding/runner.deepgram.test.ts @@ -88,12 +88,12 @@ describe("runCapability deepgram provider options", () => { expect(seenBaseUrl).toBe("https://entry.example"); expect(seenHeaders).toMatchObject({ "X-Provider": "1", + "X-Provider-Managed": "secretref-managed", "X-Config": "2", + "X-Config-Managed": "secretref-env:DEEPGRAM_HEADER_TOKEN", "X-Entry": "3", + "X-Entry-Managed": "secretref-managed", }); - expect((seenHeaders as Record)["X-Provider-Managed"]).toBeUndefined(); - expect((seenHeaders as Record)["X-Config-Managed"]).toBeUndefined(); - expect((seenHeaders as Record)["X-Entry-Managed"]).toBeUndefined(); expect(seenQuery).toMatchObject({ detect_language: false, punctuate: false, diff --git a/src/media-understanding/runner.entries.ts b/src/media-understanding/runner.entries.ts index a07d2a9b2b6..5175d764526 100644 --- a/src/media-understanding/runner.entries.ts +++ b/src/media-understanding/runner.entries.ts @@ -4,7 +4,6 @@ import { collectProviderApiKeysForExecution, executeWithApiKeyRotation, } from "../agents/api-key-rotation.js"; -import { isSecretRefHeaderValueMarker } from "../agents/model-auth-markers.js"; import { requireApiKey, resolveApiKeyForProvider } from "../agents/model-auth.js"; import type { MsgContext } from "../auto-reply/templating.js"; import { applyTemplate } from "../auto-reply/templating.js"; @@ -49,7 +48,7 @@ function sanitizeProviderHeaders( } const next: Record = {}; for (const [key, value] of Object.entries(headers)) { - if (typeof value !== "string" || isSecretRefHeaderValueMarker(value)) { + if (typeof value !== "string") { continue; } next[key] = value; diff --git a/src/secrets/audit.test.ts b/src/secrets/audit.test.ts index 28172d7c770..8a175199bcd 100644 --- a/src/secrets/audit.test.ts +++ b/src/secrets/audit.test.ts @@ -322,6 +322,33 @@ describe("secrets audit", () => { ).toBe(true); }); + it("does not flag non-sensitive routing headers in models.json", async () => { + await writeJsonFile(fixture.modelsPath, { + providers: { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: "OPENAI_API_KEY", + headers: { + "X-Proxy-Region": "us-west", + }, + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + }, + }); + + const report = await runSecretsAudit({ env: fixture.env }); + expect( + hasFinding( + report, + (entry) => + entry.code === "PLAINTEXT_FOUND" && + entry.file === fixture.modelsPath && + entry.jsonPath === "providers.openai.headers.X-Proxy-Region", + ), + ).toBe(false); + }); + it("does not flag models.json marker values as plaintext", async () => { await writeJsonFile(fixture.modelsPath, { providers: { @@ -420,4 +447,38 @@ describe("secrets audit", () => { ), ).toBe(true); }); + + it("does not flag non-sensitive routing headers in openclaw config", async () => { + await writeJsonFile(fixture.configPath, { + models: { + providers: { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, + headers: { + "X-Proxy-Region": "us-west", + }, + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + }, + }, + }); + await writeJsonFile(fixture.authStorePath, { + version: 1, + profiles: {}, + }); + await fs.writeFile(fixture.envPath, "", "utf8"); + + const report = await runSecretsAudit({ env: fixture.env }); + expect( + hasFinding( + report, + (entry) => + entry.code === "PLAINTEXT_FOUND" && + entry.file === fixture.configPath && + entry.jsonPath === "models.providers.openai.headers.X-Proxy-Region", + ), + ).toBe(false); + }); }); diff --git a/src/secrets/audit.ts b/src/secrets/audit.ts index 41509a61fb9..3215b3ce855 100644 --- a/src/secrets/audit.ts +++ b/src/secrets/audit.ts @@ -97,6 +97,40 @@ type AuditCollector = { }; const REF_RESOLVE_FALLBACK_CONCURRENCY = 8; +const ALWAYS_SENSITIVE_MODEL_PROVIDER_HEADER_NAMES = new Set([ + "authorization", + "proxy-authorization", + "x-api-key", + "api-key", + "apikey", + "x-auth-token", + "auth-token", + "x-access-token", + "access-token", + "x-secret-key", + "secret-key", +]); +const SENSITIVE_MODEL_PROVIDER_HEADER_NAME_FRAGMENTS = [ + "api-key", + "apikey", + "token", + "secret", + "password", + "credential", +]; + +function isLikelySensitiveModelProviderHeaderName(value: string): boolean { + const normalized = value.trim().toLowerCase(); + if (!normalized) { + return false; + } + if (ALWAYS_SENSITIVE_MODEL_PROVIDER_HEADER_NAMES.has(normalized)) { + return true; + } + return SENSITIVE_MODEL_PROVIDER_HEADER_NAME_FRAGMENTS.some((fragment) => + normalized.includes(fragment), + ); +} function addFinding(collector: AuditCollector, finding: SecretsAuditFinding): void { collector.findings.push(finding); @@ -198,6 +232,12 @@ function collectConfigSecrets(params: { target.value, target.entry.expectedResolvedValue, ); + if ( + target.entry.id === "models.providers.*.headers.*" && + !isLikelySensitiveModelProviderHeaderName(target.pathSegments.at(-1) ?? "") + ) { + continue; + } if (!hasPlaintext) { continue; } @@ -393,6 +433,9 @@ function collectModelsJsonSecrets(params: { if (isSecretRefHeaderValueMarker(headerValue)) { continue; } + if (!isLikelySensitiveModelProviderHeaderName(headerKey)) { + continue; + } addFinding(params.collector, { code: "PLAINTEXT_FOUND", severity: "warn",