diff --git a/.changeset/harness-credential-fetch.md b/.changeset/harness-credential-fetch.md new file mode 100644 index 000000000..aeb93e87f --- /dev/null +++ b/.changeset/harness-credential-fetch.md @@ -0,0 +1,8 @@ +--- +"@sapiom/harness": minor +"@sapiom/analytics-core": minor +--- + +Agent Studio now sends the tenant API key through the shared credential policy: plain http to a non-loopback Sapiom host is refused (Agents and core panels, the Assistant bridge, and the MCP config written for Claude Code and Codex), and the key is dropped on a redirect to another origin. `https://` and `localhost` are unchanged. For a trusted private network, opt in with `SAPIOM_ALLOW_INSECURE_HTTP=1`. + +`@sapiom/analytics-core/credential` adds an `"authorization"` credential header (for Bearer APIs) and `credentialHeaders()`, which checks a URL before a credential is handed to another process. diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 64bb0cd18..3c5c4488e 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -5,6 +5,7 @@ import { CredentialChannelError, assertCredentialMayTravel, credentialFetch, + credentialHeaders, isLoopbackHostname, matchesIntegrity, mayCarryCredential, @@ -380,4 +381,87 @@ describe("credentialFetch", () => { undefined, ]); }); + + describe("with an authorization credential", () => { + const BEARER = { header: "authorization", value: "Bearer k" } as const; + const redirectTo = (location: string) => + new Response(null, { status: 307, headers: { location } }); + + it("replaces the caller's Authorization header", async () => { + const { calls, fetchImpl } = recorder(new Response("ok")); + await credentialFetch( + fetchImpl, + "https://api.example/x", + { headers: { Authorization: "Bearer forged" } }, + BEARER, + STRICT, + ); + expect(calls[0].headers).toEqual({ authorization: "Bearer k" }); + }); + + it("drops it on a redirect to another origin", async () => { + const { calls, fetchImpl } = recorder( + redirectTo("https://other.example/y"), + new Response("ok"), + ); + await credentialFetch( + fetchImpl, + "https://api.example/x", + {}, + BEARER, + STRICT, + ); + expect(calls.map((c) => c.headers.authorization)).toEqual([ + "Bearer k", + undefined, + ]); + }); + + it("keeps it on a same-origin redirect", async () => { + const { calls, fetchImpl } = recorder( + redirectTo("/y"), + new Response("ok"), + ); + await credentialFetch( + fetchImpl, + "https://api.example/x", + {}, + BEARER, + STRICT, + ); + expect(calls.map((c) => c.headers.authorization)).toEqual([ + "Bearer k", + "Bearer k", + ]); + }); + }); +}); + +describe("credentialHeaders", () => { + const KEY = { header: "x-api-key", value: "sk_real" } as const; + + it.each([ + "https://api.example/mcp", + "http://localhost:3000/mcp", + "http://127.0.0.1/mcp", + ])("returns the credential header for %s", (url) => { + expect(credentialHeaders(url, KEY, STRICT)).toEqual({ + "x-api-key": "sk_real", + }); + }); + + it.each(["http://10.0.0.5:3000/mcp", "ftp://host/mcp"])( + "refuses %s", + (url) => { + expect(() => credentialHeaders(url, KEY, STRICT)).toThrow( + CredentialChannelError, + ); + }, + ); + + it("allows plain http to a private host when opted in", () => { + expect( + credentialHeaders("http://10.0.0.5:3000/mcp", KEY, OPTED_IN), + ).toEqual({ "x-api-key": "sk_real" }); + }); }); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 07943e3e4..fe4696ff8 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -1,5 +1,4 @@ -/** Centralize channel and redirect checks so clients share one tenant-credential policy. - * Harness exceptions to the literal-header guard are tracked in SAP-4112. */ +/** Centralize channel and redirect checks so clients share one tenant-credential policy. */ import { createHash } from "node:crypto"; export const ALLOW_INSECURE_HTTP_ENV = "SAPIOM_ALLOW_INSECURE_HTTP"; @@ -293,10 +292,21 @@ export async function fetchKeepingCredential( } export interface Credential { - readonly header: "x-api-key" | "x-sapiom-api-key"; + readonly header: "x-api-key" | "x-sapiom-api-key" | "authorization"; readonly value: string; } +/** For a credential handed to another process (a config file): check the URL where it is chosen, + * since the request itself is made outside this policy. */ +export function credentialHeaders( + url: string, + credential: Credential, + policy: CredentialPolicy, +): Record { + assertCredentialMayTravel(new URL(url), policy); + return { [credential.header]: credential.value }; +} + /** Set the credential after caller headers so they cannot replace it. * Apply shared channel checks and remove credentials when following origin changes. */ export async function credentialFetch( diff --git a/packages/harness/README.md b/packages/harness/README.md index 8eb0960ac..25ad282a3 100644 --- a/packages/harness/README.md +++ b/packages/harness/README.md @@ -397,6 +397,20 @@ from what its other components do on their own (the app's product analytics, and (`source: served` or `source: bundled`). `SAPIOM_AUTHORING_RULES_FETCH_DISABLED=1` (or `true`) skips the request. +Requests that carry your Sapiom API key (the Agents and account panels, the +Assistant, and the MCP config written for Claude Code and Codex) only travel over +`https://` or to a loopback host (`localhost`, `127.0.0.1`), and the key is +dropped on a redirect to another origin. A custom environment on plain `http://` +to any other host (for example `http://10.0.0.5:3000`) is refused, and starting a +signed-in session against it fails (a session without an API key has no key to +protect and starts as before). On a trusted private network, opt in with +`SAPIOM_ALLOW_INSECURE_HTTP=1`: + +- **CLI**: set it in the shell that runs `npx @sapiom/harness`. +- **Desktop app**: a launched app does not read your shell profile, so set it in + the session environment and reopen the app (on macOS, + `launchctl setenv SAPIOM_ALLOW_INSECURE_HTTP 1`). + ## Development ```bash diff --git a/packages/harness/src/core/account-plan.test.ts b/packages/harness/src/core/account-plan.test.ts index 879b98ccd..3709f6f44 100644 --- a/packages/harness/src/core/account-plan.test.ts +++ b/packages/harness/src/core/account-plan.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createAccountPlanReader, extractBalance, extractLimit, extractPlan } from "./account-plan.js"; import type { ApiKeyProvider } from "./api-key-provider.js"; @@ -142,7 +142,7 @@ describe("createAccountPlanReader", () => { ]), ); for (const call of fetchImpl.mock.calls) { - expect((call[1] as RequestInit).headers).toEqual({ Authorization: "Bearer sk_live" }); + expect((call[1] as RequestInit).headers).toEqual({ authorization: "Bearer sk_live" }); } }); @@ -236,7 +236,7 @@ describe("createAccountPlanReader", () => { it("refreshes the key once on a 401 and retries", async () => { const keys = provider(["sk_stale", "sk_fresh"]); const fetchImpl = vi.fn(async (input: string | URL | Request, init?: RequestInit) => { - const auth = (init?.headers as Record).Authorization; + const auth = (init?.headers as Record).authorization; if (auth === "Bearer sk_stale") return jsonResponse({ error: "unauthorized" }, 401); const url = String(input); if (url.endsWith("/v1/dashboard/plan")) return jsonResponse(planBody()); @@ -253,3 +253,30 @@ describe("createAccountPlanReader", () => { expect(keys.refreshCalls).toBeGreaterThanOrEqual(1); }); }); + +describe("createAccountPlanReader — credential channel", () => { + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + }); + + it("refuses plaintext http to a non-loopback host and logs why", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + const fetchImpl = vi.fn() as unknown as typeof fetch; + const reader = createAccountPlanReader({ + apiKey: "sk_live", + baseUrl: "http://api.internal", + fetchImpl, + }); + + expect(await reader.view()).toEqual({ + plan: null, + readout: { kind: "none" }, + source: "fallback", + reason: "unreachable", + }); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(String(errors.mock.calls[0]?.[0])).toContain("refusing plaintext HTTP"); + }); +}); diff --git a/packages/harness/src/core/account-plan.ts b/packages/harness/src/core/account-plan.ts index ce23fd628..db68738a4 100644 --- a/packages/harness/src/core/account-plan.ts +++ b/packages/harness/src/core/account-plan.ts @@ -23,6 +23,8 @@ * never take the rail down. */ +import { credentialFetch } from "@sapiom/analytics-core/credential"; +import { harnessCredentialPolicy } from "./credential-policy.js"; import type { AccountPlanView } from "../shared/types.js"; import { type ApiKeyProvider, @@ -131,10 +133,14 @@ export function createAccountPlanReader(opts: { const attempt = async (key: string): Promise => { try { - return await fetchImpl(`${baseUrl}${path}`, { - // Core (`api.*`) takes a Bearer token — see template-catalog.ts. - headers: { Authorization: `Bearer ${key}` }, - }); + // Core (`api.*`) takes a Bearer token: see template-catalog.ts. + return await credentialFetch( + fetchImpl, + `${baseUrl}${path}`, + {}, + { header: "authorization", value: `Bearer ${key}` }, + harnessCredentialPolicy(), + ); } catch (err) { warnOnce(err instanceof Error ? err.message : String(err)); return null; diff --git a/packages/harness/src/core/credential-policy.ts b/packages/harness/src/core/credential-policy.ts new file mode 100644 index 000000000..caecb7f62 --- /dev/null +++ b/packages/harness/src/core/credential-policy.ts @@ -0,0 +1,9 @@ +import { + resolveCredentialPolicy, + type CredentialPolicy, +} from "@sapiom/analytics-core/credential"; + +/** Per call, so the `SAPIOM_ALLOW_INSECURE_HTTP` opt-in is read when the request is made. */ +export function harnessCredentialPolicy(): CredentialPolicy { + return resolveCredentialPolicy(undefined, { label: "@sapiom/harness" }); +} diff --git a/packages/harness/src/core/definition-app-link.test.ts b/packages/harness/src/core/definition-app-link.test.ts index 45cce2c41..dbd28bfc2 100644 --- a/packages/harness/src/core/definition-app-link.test.ts +++ b/packages/harness/src/core/definition-app-link.test.ts @@ -6,7 +6,7 @@ * Link", and the key contract (Bearer, `/v1`, one refresh on 401/403). */ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import type { ApiKeyProvider } from "./api-key-provider.js"; import { @@ -102,7 +102,7 @@ describe("createDefinitionAppLinkReader", () => { expect(fetchImpl).toHaveBeenCalledWith( `${BASE}/v1/workflows/definitions/886/app-link`, expect.objectContaining({ - headers: { Authorization: "Bearer sk_a" }, + headers: { authorization: "Bearer sk_a" }, signal: expect.any(AbortSignal), }), ); @@ -143,7 +143,7 @@ describe("createDefinitionAppLinkReader", () => { expect(fetchImpl).toHaveBeenLastCalledWith( `${BASE}/v1/workflows/definitions/886/app-link`, expect.objectContaining({ - headers: { Authorization: "Bearer sk_new" }, + headers: { authorization: "Bearer sk_new" }, signal: expect.any(AbortSignal), }), ); @@ -221,3 +221,20 @@ describe("createDefinitionAppLinkReader", () => { expect(signals[0]).toBe(signals[1]); }); }); + +describe("createDefinitionAppLinkReader — credential channel", () => { + afterEach(() => vi.unstubAllEnvs()); + + it("refuses plaintext http to a non-loopback host without calling fetch", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const fetchImpl = vi.fn() as unknown as typeof fetch; + const reader = createDefinitionAppLinkReader({ + apiKey: "sk_a", + baseUrl: "http://api.internal", + fetchImpl, + }); + + await expect(reader.read("886")).resolves.toEqual(NO_APP_LINK); + expect(fetchImpl).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/harness/src/core/definition-app-link.ts b/packages/harness/src/core/definition-app-link.ts index f056d1ece..8f249983f 100644 --- a/packages/harness/src/core/definition-app-link.ts +++ b/packages/harness/src/core/definition-app-link.ts @@ -18,6 +18,8 @@ * A broken dashboard read must never take the session bar down. */ +import { credentialFetch } from "@sapiom/analytics-core/credential"; +import { harnessCredentialPolicy } from "./credential-policy.js"; import type { DefinitionAppLinkView } from "../shared/types.js"; import { type ApiKeyProvider, @@ -86,11 +88,14 @@ export function createDefinitionAppLinkReader(opts: { signal: AbortSignal, ): Promise => { try { - return await fetchImpl(`${baseUrl}${path}`, { - // Core (`api.*`) takes a Bearer token — see template-catalog.ts. - headers: { Authorization: `Bearer ${key}` }, - signal, - }); + // Core (`api.*`) takes a Bearer token: see template-catalog.ts. + return await credentialFetch( + fetchImpl, + `${baseUrl}${path}`, + { signal }, + { header: "authorization", value: `Bearer ${key}` }, + harnessCredentialPolicy(), + ); } catch { return null; } diff --git a/packages/harness/src/core/definition-slug-resolver.test.ts b/packages/harness/src/core/definition-slug-resolver.test.ts index 4d97f91ef..7bf360c9b 100644 --- a/packages/harness/src/core/definition-slug-resolver.test.ts +++ b/packages/harness/src/core/definition-slug-resolver.test.ts @@ -80,10 +80,10 @@ describe("createDefinitionSlugResolver", () => { expect(fetchImpl).toHaveBeenCalledWith( "https://tools.sapiom.ai/agents/v1/definitions/42", - { + expect.objectContaining({ headers: { "x-sapiom-api-key": "my-api-key" }, signal: expect.any(AbortSignal), - }, + }), ); }); @@ -147,10 +147,13 @@ describe("createDefinitionSlugResolver", () => { status: "available", metadata: { slug: "signed-in-agent", activeBuildRunStatus: "ready" }, }); - expect(fetchImpl).toHaveBeenCalledWith(expect.any(String), { - headers: { "x-sapiom-api-key": "sk-after-login" }, - signal: expect.any(AbortSignal), - }); + expect(fetchImpl).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + headers: { "x-sapiom-api-key": "sk-after-login" }, + signal: expect.any(AbortSignal), + }), + ); }); it("does not cache mutable build status", async () => { @@ -696,3 +699,53 @@ describe("authenticated deployment evidence", () => { }); }); }); + +describe("createDefinitionSlugResolver — credential channel", () => { + beforeEach(() => { + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + }); + + const resolverAt = (fetchImpl: typeof fetch) => + createDefinitionSlugResolver({ + apiKey: "sk-test", + baseUrl: "http://agents.internal", + fetchImpl, + }); + + it("refuses plaintext http to a non-loopback host without calling fetch", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const fetchImpl = makeFetch(200, []); + const resolver = resolverAt(fetchImpl); + + await expect(resolver.resolveMetadata("188")).resolves.toEqual({ + status: "unavailable", + lastConfirmedDeployed: null, + }); + await expect(resolver.listVisible()).resolves.toMatchObject({ + status: "unavailable", + }); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + it("sends the key over plaintext http when SAPIOM_ALLOW_INSECURE_HTTP=1", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", "1"); + const fetchImpl = makeFetch(200, { + slug: "lease-abstractor", + activeBuildRunId: null, + activeBuildRunStatus: null, + }); + + await resolverAt(fetchImpl).resolveMetadata("188"); + expect(fetchImpl).toHaveBeenCalledWith( + "http://agents.internal/agents/v1/definitions/188", + expect.objectContaining({ + headers: { "x-sapiom-api-key": "sk-test" }, + signal: expect.any(AbortSignal), + }), + ); + }); +}); diff --git a/packages/harness/src/core/definition-slug-resolver.ts b/packages/harness/src/core/definition-slug-resolver.ts index c1c99bd56..c02e76bb3 100644 --- a/packages/harness/src/core/definition-slug-resolver.ts +++ b/packages/harness/src/core/definition-slug-resolver.ts @@ -8,6 +8,9 @@ * Mirrors `@sapiom/tools`'s DEFAULT_BASE_URL for the base URL resolution. */ +import { credentialFetch } from "@sapiom/analytics-core/credential"; +import { harnessCredentialPolicy } from "./credential-policy.js"; + /** Returns the agents API base URL, honouring the same env-var precedence as * @sapiom/tools's DEFAULT_BASE_URL. */ export function resolveAgentsBaseUrl(): string { @@ -183,12 +186,12 @@ export function createDefinitionSlugResolver(opts: { }; let failure = "network error or timeout"; try { - const response = await fetchImpl( + const response = await credentialFetch( + fetchImpl, `${baseUrl}/agents/v1/definitions/${encodeURIComponent(definitionId)}`, - { - headers: { "x-sapiom-api-key": key }, - signal: AbortSignal.timeout(5_000), - }, + { signal: AbortSignal.timeout(5_000) }, + { header: "x-sapiom-api-key", value: key }, + harnessCredentialPolicy(), ); failure = response.ok ? "invalid response" : `HTTP ${response.status}`; if (response.status === 401 || response.status === 403) @@ -251,10 +254,13 @@ export function createDefinitionSlugResolver(opts: { }; let failure = "network error or timeout"; try { - const response = await fetchImpl(`${baseUrl}/agents/v1/definitions`, { - headers: { "x-sapiom-api-key": currentKey }, - signal: AbortSignal.timeout(5_000), - }); + const response = await credentialFetch( + fetchImpl, + `${baseUrl}/agents/v1/definitions`, + { signal: AbortSignal.timeout(5_000) }, + { header: "x-sapiom-api-key", value: currentKey }, + harnessCredentialPolicy(), + ); failure = response.ok ? "invalid response" : `HTTP ${response.status}`; if (response.status === 401 || response.status === 403) failure += diff --git a/packages/harness/src/core/inject/mcp-config.test.ts b/packages/harness/src/core/inject/mcp-config.test.ts index d287d62cb..1e0cba4d5 100644 --- a/packages/harness/src/core/inject/mcp-config.test.ts +++ b/packages/harness/src/core/inject/mcp-config.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { CredentialChannelError } from "@sapiom/analytics-core/credential"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -220,6 +221,67 @@ describe("generateMcpConfig", () => { expect(withNull.mcpServers.sapiom.headers).toBeUndefined(); }); + describe("credential channel for the remote MCP key", () => { + afterEach(() => vi.unstubAllEnvs()); + + const useEnvironment = (apiURL: string) => + writeCredentialsFile({ + currentEnvironment: "custom", + environments: { custom: { appURL: apiURL, apiURL } }, + }); + const configPath = (sessionId: string) => + path.join(tmpDir, ".sapiom", "harness", "generated", sessionId, "mcp-config.json"); + + it("refuses to write the key for plaintext http to a non-loopback host", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + await useEnvironment("http://10.0.0.5:3000"); + + await expect( + generateMcpConfig("session-lan", { apiKey: "sk_live_test123" }), + ).rejects.toBeInstanceOf(CredentialChannelError); + await expect(fs.access(configPath("session-lan"))).rejects.toThrow(); + }); + + it("still writes the keyless config for that host", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + await useEnvironment("http://10.0.0.5:3000"); + + const config = JSON.parse( + await fs.readFile(await generateMcpConfig("session-lan-anon"), "utf-8"), + ); + expect(config.mcpServers.sapiom).toEqual({ + type: "http", + url: "http://10.0.0.5:3000/v1/mcp", + }); + }); + + it("writes the key for a loopback host", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + await useEnvironment("http://localhost:3000"); + + const config = JSON.parse( + await fs.readFile( + await generateMcpConfig("session-local", { apiKey: "sk_live_test123" }), + "utf-8", + ), + ); + expect(config.mcpServers.sapiom.headers).toEqual({ "x-api-key": "sk_live_test123" }); + }); + + it("writes the key for a private host when SAPIOM_ALLOW_INSECURE_HTTP=1", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", "1"); + await useEnvironment("http://10.0.0.5:3000"); + + const config = JSON.parse( + await fs.readFile( + await generateMcpConfig("session-lan-opt-in", { apiKey: "sk_live_test123" }), + "utf-8", + ), + ); + expect(config.mcpServers.sapiom.headers).toEqual({ "x-api-key": "sk_live_test123" }); + }); + }); + it("writes the config file with owner-only permissions (it can carry a live API key)", async () => { const filePath = await generateMcpConfig("session-perm", { apiKey: "sk_live_test123", diff --git a/packages/harness/src/core/inject/mcp-config.ts b/packages/harness/src/core/inject/mcp-config.ts index e75c13bbe..e25c35e46 100644 --- a/packages/harness/src/core/inject/mcp-config.ts +++ b/packages/harness/src/core/inject/mcp-config.ts @@ -1,9 +1,11 @@ import { STUDIO_HOST_CONTEXT_ENV } from "@sapiom/agent-map/host-protocol"; import * as fs from "node:fs/promises"; import * as path from "node:path"; +import { credentialHeaders } from "@sapiom/analytics-core/credential"; import { resolveEnvironment } from "@sapiom/mcp/auth"; import { HARNESS_PATHS } from "../../shared/types.js"; import { expandHome } from "../../cli/paths.js"; +import { harnessCredentialPolicy } from "../credential-policy.js"; /** * A host-supplied way to launch the local `sapiom-dev` stdio server instead of @@ -93,7 +95,16 @@ export async function generateMcpConfig( sapiom: { type: "http", url: remoteMcpUrl, - ...(options.apiKey ? { headers: { "x-api-key": options.apiKey } } : {}), + ...(options.apiKey + ? { + // Refuse to hand the key to Claude Code / Codex for a channel we would refuse ourselves. + headers: credentialHeaders( + remoteMcpUrl, + { header: "x-api-key", value: options.apiKey }, + harnessCredentialPolicy(), + ), + } + : {}), }, "sapiom-dev": options.devServer ? { diff --git a/packages/harness/src/core/run-state.test.ts b/packages/harness/src/core/run-state.test.ts index 5b22a748e..47a6dc658 100644 --- a/packages/harness/src/core/run-state.test.ts +++ b/packages/harness/src/core/run-state.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from "vitest"; +import { afterEach, describe, it, expect, vi } from "vitest"; import { createRunStateFetcher, @@ -216,7 +216,7 @@ describe("createRunStateFetcher — success path (proves succeeded→passed fix expect(mockFetch).toHaveBeenCalledWith( "https://agents.test/agents/v1/executions/exec_invoice_001", - { headers: { "x-sapiom-api-key": "sk-my-key" } }, + expect.objectContaining({ headers: { "x-sapiom-api-key": "sk-my-key" } }), ); }); }); @@ -338,13 +338,13 @@ describe("createRunStateFetcher — refresh-on-401", () => { expect(mockFetch).toHaveBeenNthCalledWith( 1, "https://agents.test/agents/v1/executions/exec_invoice_001", - { headers: { "x-sapiom-api-key": "sk-stale" } }, + expect.objectContaining({ headers: { "x-sapiom-api-key": "sk-stale" } }), ); // …the retry used the refreshed key. expect(mockFetch).toHaveBeenNthCalledWith( 2, "https://agents.test/agents/v1/executions/exec_invoice_001", - { headers: { "x-sapiom-api-key": "sk-fresh" } }, + expect.objectContaining({ headers: { "x-sapiom-api-key": "sk-fresh" } }), ); }); @@ -467,3 +467,40 @@ describe("createRunStateFetcher — refresh-on-401", () => { expect(mockFetch).not.toHaveBeenCalled(); }); }); + +describe("createRunStateFetcher — credential channel", () => { + afterEach(() => vi.unstubAllEnvs()); + + it("refuses plaintext http to a non-loopback host without calling fetch", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const mockFetch = makeFetch(200, RAW_SUCCESS_DOC); + const fetcher = createRunStateFetcher({ + apiKey: "sk-test", + baseUrl: "http://agents.internal", + fetchImpl: mockFetch, + }); + + expect(await fetcher.fetch("exec_invoice_001")).toEqual({ + ok: false, + status: 502, + error: "gateway unreachable", + }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("sends the key over plaintext http when SAPIOM_ALLOW_INSECURE_HTTP=1", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", "1"); + const mockFetch = makeFetch(200, RAW_SUCCESS_DOC); + const fetcher = createRunStateFetcher({ + apiKey: "sk-test", + baseUrl: "http://agents.internal", + fetchImpl: mockFetch, + }); + + await fetcher.fetch("exec_invoice_001"); + expect(mockFetch).toHaveBeenCalledWith( + "http://agents.internal/agents/v1/executions/exec_invoice_001", + expect.objectContaining({ headers: { "x-sapiom-api-key": "sk-test" } }), + ); + }); +}); diff --git a/packages/harness/src/core/run-state.ts b/packages/harness/src/core/run-state.ts index f5ad5bd30..5c96bc26e 100644 --- a/packages/harness/src/core/run-state.ts +++ b/packages/harness/src/core/run-state.ts @@ -17,6 +17,7 @@ */ import { decodeExecutionProjection } from "@sapiom/agent-core"; +import { credentialFetch } from "@sapiom/analytics-core/credential"; import type { RunView } from "../shared/types.js"; import { renderRunState } from "./render-run-state.js"; @@ -24,6 +25,7 @@ import { type ApiKeyProvider, staticApiKeyProvider, } from "./api-key-provider.js"; +import { harnessCredentialPolicy } from "./credential-policy.js"; /** * Normalize the polymorphic `apiKey` option to an {@link ApiKeyProvider}. A @@ -99,9 +101,12 @@ export function createRunStateFetcher( { kind: "ok"; res: Response } | { kind: "err"; result: RunStateResult } > => { try { - const res = await fetchImpl( + const res = await credentialFetch( + fetchImpl, `${baseUrl}/agents/v1/executions/${encodeURIComponent(executionId)}`, - { headers: { "x-sapiom-api-key": apiKey } }, + {}, + { header: "x-sapiom-api-key", value: apiKey }, + harnessCredentialPolicy(), ); return { kind: "ok", res }; } catch { diff --git a/packages/harness/src/core/template-catalog.test.ts b/packages/harness/src/core/template-catalog.test.ts index 5fbe39466..d8f6dcc9d 100644 --- a/packages/harness/src/core/template-catalog.test.ts +++ b/packages/harness/src/core/template-catalog.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createTemplateCatalog } from "./template-catalog.js"; import type { ApiKeyProvider } from "./api-key-provider.js"; @@ -68,7 +68,7 @@ describe("createTemplateCatalog", () => { expect(fetchImpl).toHaveBeenCalledTimes(1); const [url, init] = fetchImpl.mock.calls[0] as [string, RequestInit]; expect(url).toBe(`${BASE}/v1/workflows/templates`); - expect(init.headers).toEqual({ Authorization: "Bearer sk_test" }); + expect(init.headers).toEqual({ authorization: "Bearer sk_test" }); }); it("returns the live catalog, preserving every template", async () => { @@ -119,7 +119,7 @@ describe("createTemplateCatalog", () => { expect(result.source).toBe("live"); expect(keys.refreshCalls).toBe(1); const [, secondInit] = fetchImpl.mock.calls[1] as [string, RequestInit]; - expect(secondInit.headers).toEqual({ Authorization: "Bearer sk_fresh" }); + expect(secondInit.headers).toEqual({ authorization: "Bearer sk_fresh" }); }); it("does not retry when refresh yields the same key (no infinite recovery loop)", async () => { @@ -494,3 +494,29 @@ describe("createTemplateCatalog", () => { }); }); }); + +describe("createTemplateCatalog — credential channel", () => { + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + }); + + it("refuses plaintext http to a non-loopback host and logs why", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + const fetchImpl = vi.fn() as unknown as typeof fetch; + const catalog = createTemplateCatalog({ + apiKey: "sk_test", + baseUrl: "http://api.internal", + fetchImpl, + }); + + await expect(catalog.list()).resolves.toEqual({ + templates: [], + source: "fallback", + reason: "unreachable", + }); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(String(errors.mock.calls[0]?.[0])).toContain("refusing plaintext HTTP"); + }); +}); diff --git a/packages/harness/src/core/template-catalog.ts b/packages/harness/src/core/template-catalog.ts index a9ee70c95..7342caf3f 100644 --- a/packages/harness/src/core/template-catalog.ts +++ b/packages/harness/src/core/template-catalog.ts @@ -28,6 +28,8 @@ * mirroring `run-state.ts`, so a rotated key recovers in place. */ +import { credentialFetch } from "@sapiom/analytics-core/credential"; +import { harnessCredentialPolicy } from "./credential-policy.js"; import type { TemplateComplexity, TemplateDetailView, @@ -345,11 +347,15 @@ export function createTemplateCatalog(opts: { const attempt = async (key: string): Promise => { try { - return await fetchImpl(`${baseUrl}${path}`, { - // Core (`api.*`) takes a Bearer token; the agents surface (`tools.*`) - // takes `x-sapiom-api-key`. Sending the latter here 401s — verified. - headers: { Authorization: `Bearer ${key}` }, - }); + // Core (`api.*`) takes a Bearer token; the agents surface (`tools.*`) + // takes `x-sapiom-api-key`. Sending the latter here 401s. + return await credentialFetch( + fetchImpl, + `${baseUrl}${path}`, + {}, + { header: "authorization", value: `Bearer ${key}` }, + harnessCredentialPolicy(), + ); } catch (err) { warnOnce(err instanceof Error ? err.message : String(err)); return null; diff --git a/packages/harness/src/core/vault-secrets.test.ts b/packages/harness/src/core/vault-secrets.test.ts index e5c9ddbd2..4e17f4224 100644 --- a/packages/harness/src/core/vault-secrets.test.ts +++ b/packages/harness/src/core/vault-secrets.test.ts @@ -7,7 +7,7 @@ * rather than a dead end. */ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { VaultSecretError, createVaultSecretsClient } from "./vault-secrets.js"; @@ -147,3 +147,23 @@ describe("remove", () => { ); }); }); + +describe("credential channel", () => { + afterEach(() => vi.unstubAllEnvs()); + + it("refuses plaintext http to a non-loopback host without calling fetch", async () => { + vi.stubEnv("SAPIOM_ALLOW_INSECURE_HTTP", ""); + const { fetchImpl, calls } = stubFetch(json({ keys: [] })); + const client = createVaultSecretsClient({ + apiKey: "sk_test", + baseUrl: "http://api.internal", + fetchImpl, + }); + + await expect(client.list("188")).resolves.toBeNull(); + await expect(client.set("188", "API_TOKEN", "v")).rejects.toThrow( + "could not be sent", + ); + expect(calls).toHaveLength(0); + }); +}); diff --git a/packages/harness/src/core/vault-secrets.ts b/packages/harness/src/core/vault-secrets.ts index cf1dd6a4c..fcb5e7e54 100644 --- a/packages/harness/src/core/vault-secrets.ts +++ b/packages/harness/src/core/vault-secrets.ts @@ -20,6 +20,8 @@ * credential is in place and find out during a production run. */ +import { credentialFetch } from "@sapiom/analytics-core/credential"; +import { harnessCredentialPolicy } from "./credential-policy.js"; import { type ApiKeyProvider, staticApiKeyProvider, @@ -89,14 +91,14 @@ export function createVaultSecretsClient(opts: { const attempt = async (key: string): Promise => { try { - return await fetchImpl(`${baseUrl}${path}`, { - ...init, - headers: { - ...init.headers, - // Core (`api.*`) takes a Bearer token — see template-catalog.ts. - Authorization: `Bearer ${key}`, - }, - }); + // Core (`api.*`) takes a Bearer token: see template-catalog.ts. + return await credentialFetch( + fetchImpl, + `${baseUrl}${path}`, + init, + { header: "authorization", value: `Bearer ${key}` }, + harnessCredentialPolicy(), + ); } catch { return null; } diff --git a/packages/harness/src/server/opencode-bridge.ts b/packages/harness/src/server/opencode-bridge.ts index 1403121b3..2dfef1e44 100644 --- a/packages/harness/src/server/opencode-bridge.ts +++ b/packages/harness/src/server/opencode-bridge.ts @@ -8,12 +8,14 @@ import { Readable } from "node:stream"; import { pipeline } from "node:stream/promises"; import express, { type Request, type Response, type Router } from "express"; import rateLimit, { MemoryStore } from "express-rate-limit"; +import { credentialFetch } from "@sapiom/analytics-core/credential"; import type { ResolvedEnvironment } from "@sapiom/mcp/auth"; import { samePrincipal, type AssistantAccess, type AssistantGrant, } from "../core/assistant-access.js"; +import { harnessCredentialPolicy } from "../core/credential-policy.js"; import { fetchOpenCodeModelResponse, openCodeModelCompletionToken, @@ -299,7 +301,6 @@ export class OpenCodeBridge { "Accept-Encoding": "identity", Accept: req.header("Accept") ?? "application/json", "Content-Type": "application/json", - "x-api-key": key, }); // Queued MCP tools close the POST stream and deliver results through // GET replay. Its cursor must survive the credential bridge. @@ -341,13 +342,19 @@ export class OpenCodeBridge { completionToken = openCodeModelCompletionToken(request); } const request = () => - fetch(upstream, { - method: req.method, - headers, - body: body as Uint8Array | undefined, - redirect: "error", - signal: abort.signal, - }); + credentialFetch( + fetch, + upstream.href, + { + method: req.method, + headers, + body: body as Uint8Array | undefined, + redirect: "error", + signal: abort.signal, + }, + { header: "x-api-key", value: key }, + harnessCredentialPolicy(), + ); const response = streamingModel ? await fetchOpenCodeModelResponse( request, diff --git a/packages/harness/src/server/runs.test.ts b/packages/harness/src/server/runs.test.ts index d54f5bf01..5ef44eb84 100644 --- a/packages/harness/src/server/runs.test.ts +++ b/packages/harness/src/server/runs.test.ts @@ -206,7 +206,9 @@ describe("createRunsRouter", () => { // the local boot token (which only gates the harness's own /api surface). expect(fetchImpl).toHaveBeenCalledWith( "https://agents.test/agents/v1/executions/exec_invoice_002", - { headers: { "x-sapiom-api-key": "sk-held-key" } }, + expect.objectContaining({ + headers: { "x-sapiom-api-key": "sk-held-key" }, + }), ); expect(provider.refreshCalls).toBe(0); }); diff --git a/plans/credential-transport/harness-tenant-key-credential-fetch.md b/plans/credential-transport/harness-tenant-key-credential-fetch.md new file mode 100644 index 000000000..692244441 --- /dev/null +++ b/plans/credential-transport/harness-tenant-key-credential-fetch.md @@ -0,0 +1,148 @@ +# SAP-4112: route the harness tenant key through credentialFetch + +## Goal + +Every place `packages/harness` puts the Sapiom tenant key on a request (or into a config another process sends) goes +through the shared channel policy of `@sapiom/analytics-core/credential`, and `scripts/credential-header-allowlist.json` +becomes `[]`. After this PR, the only code in `packages/*/src` that writes a tenant-key header is +`packages/analytics-core/src/credential.ts`. + +## Approach + +Reuse `credentialFetch` as is for the eight network sites; extend it by the smallest amount for the two things it +cannot do today: + +1. **Bearer sites.** `Credential.header` only accepts `"x-api-key" | "x-sapiom-api-key"` + (`packages/analytics-core/src/credential.ts:295-298`). Add `"authorization"` to the union. The rest already works for + it: the caller's own `Authorization` is replaced case-insensitively (`credential.ts:314`), and a cross-origin hop + drops `authorization` (`credential.ts:80-86`, `287-289`). The four core-surface sites then pass + `{ header: "authorization", value: \`Bearer ${key}\` }`. One route for all ten sites, no second "equivalent check". +2. **MCP config file.** The key is not sent by harness but written into `mcp-config.json` for Claude Code / Codex. + Add one helper next to `credentialFetch`: `credentialHeaders(url, credential, policy)` runs + `assertCredentialMayTravel(new URL(url), policy)` and returns `{ [credential.header]: credential.value }`. + `generateMcpConfig` calls it at the line where it chooses `remoteMcpUrl` (`mcp-config.ts:88-96`). The Codex adapter + reads URL and headers from that same file (`core/adapters/codex-mcp.ts:88-120`), so one check covers both harnesses. + +Harness policy: one function `harnessCredentialPolicy()` = +`resolveCredentialPolicy(undefined, { label: "@sapiom/harness" })`, called per request (same as `@sapiom/mcp`, +`app-links-api.ts:103`) so `SAPIOM_ALLOW_INSECURE_HTTP` is read when the request is made. The opt-in is that env var, +which the refusal message already names (`credential.ts:71-76`). No harness CLI flag or desktop option. + +Refusal behaviour: `credentialFetch` throws `CredentialChannelError` (a `TypeError`) before any byte is sent. Every +network site already wraps its fetch in a `try/catch` that maps a throw to its existing failure result (502 +"gateway unreachable", "unavailable", `null`, Studio "Assistant connection failed"), so no new error plumbing. +`generateMcpConfig` lets the error propagate: session start fails with the refusal message, which names the opt-in. + +## Tasks + +### T1. analytics-core: Bearer credential and config-file headers + +Files: `packages/analytics-core/src/credential.ts`, `packages/analytics-core/src/credential.spec.ts` + +Steps: +- `Credential.header`: add `"authorization"`. +- Add `export function credentialHeaders(url: string, credential: Credential, policy: CredentialPolicy): Record` + (assert, then return the one header). Doc comment: for a key handed to another process, checked where the URL is + chosen. +- Module comment line 2: drop the "Harness exceptions ... SAP-4112" sentence. +- Specs: an `authorization` credential replaces a caller `Authorization` header and is dropped on a cross-origin + redirect, kept on a same-origin one; `credentialHeaders` returns the header for https and loopback http, throws + `CredentialChannelError` for non-loopback http and for a non-http scheme, and honours `allowInsecureHttp`. + +### T2. harness: shared policy + +Files: `packages/harness/src/core/credential-policy.ts` (new, ~5 lines) + +Steps: +- `export function harnessCredentialPolicy(): CredentialPolicy` as above. + +### T3. harness: agents-surface sites (`x-sapiom-api-key`) + +Files: `packages/harness/src/core/run-state.ts`, `packages/harness/src/core/definition-slug-resolver.ts`, +their `.test.ts` + +Steps: +- `run-state.ts:102-105`: `credentialFetch(fetchImpl, url, {}, { header: "x-sapiom-api-key", value: apiKey }, harnessCredentialPolicy())`. +- `definition-slug-resolver.ts:186-192` and `254-257`: same, keeping `signal: AbortSignal.timeout(5_000)` in `init`. +- Tests: exact-argument assertions become `expect.objectContaining({ headers: { "x-sapiom-api-key": ... } })` + (credentialFetch adds `redirect: "manual"`, `method`, `body`). Add one test per file: base URL + `http://agents.internal` makes no fetch call and returns the existing failure result; with + `SAPIOM_ALLOW_INSECURE_HTTP=1` the call is made. + +### T4. harness: core-surface Bearer sites + +Files: `packages/harness/src/core/vault-secrets.ts`, `account-plan.ts`, `template-catalog.ts`, +`definition-app-link.ts`, their `.test.ts` + +Steps: +- Each `fetchImpl(url, { ...init, headers: { ..., Authorization: \`Bearer ${key}\` } })` becomes + `credentialFetch(fetchImpl, url, init, { header: "authorization", value: \`Bearer ${key}\` }, harnessCredentialPolicy())` + (`vault-secrets.ts:92-99`, `account-plan.ts:134-137`, `template-catalog.ts:348-352`, `definition-app-link.ts:89-93`). + Keep `signal` and the other init fields. +- Tests: header assertions read `authorization` (lowercase: credentialFetch writes the credential name as given), or + `new Headers(init.headers).get("Authorization")` where the test already uses `Headers`. Add one refusal test per + file (non-loopback http base URL: no fetch call, existing failure result). + +### T5. harness: OpenCode bridge + +Files: `packages/harness/src/server/opencode-bridge.ts`, `opencode-bridge.test.ts` + +Steps: +- Drop `"x-api-key": key` from the `Headers` built at `opencode-bridge.ts:298-303`; the `request` thunk + (`343-350`) calls `credentialFetch(fetch, upstream.href, { method, headers, body, redirect: "error", signal }, { header: "x-api-key", value: key }, harnessCredentialPolicy())`. + `redirect: "error"` keeps plain-fetch redirect semantics (`credential.ts:230-232`); `assistantUpstreams` keeps its + stricter https-or-loopback check (`129-162`). +- Existing tests already prove the upstream receives `x-api-key: sk_private_studio` and a caller `x-api-key` is + ignored (`opencode-bridge.test.ts:288-306`, `364`, `401`); they must stay green unchanged. + +### T6. harness: MCP config + +Files: `packages/harness/src/core/inject/mcp-config.ts`, `mcp-config.test.ts` + +Steps: +- `headers: credentialHeaders(remoteMcpUrl, { header: "x-api-key", value: options.apiKey }, harnessCredentialPolicy())` + when `options.apiKey` is set; unchanged when absent. +- Tests: existing https case unchanged; a credentials file env with `apiURL: "http://10.0.0.5:3000"` and an apiKey + rejects with `CredentialChannelError` and writes no file; same env without apiKey still writes the config; + `apiURL: "http://localhost:3000"` writes the header; `SAPIOM_ALLOW_INSECURE_HTTP=1` lets the LAN URL through. + +### T7. Allowlist, comments, changeset + +Files: `scripts/credential-header-allowlist.json`, `packages/analytics-core/src/credential.ts` (comment, T1), +`.changeset/harness-credential-fetch.md` (new) + +Steps: +- Allowlist file becomes `[]` (keep the file and the mechanism: the check reads it). +- Changeset: `@sapiom/harness: minor` (plain http to a non-loopback Sapiom host is now refused, opt in with + `SAPIOM_ALLOW_INSECURE_HTTP=1`), `@sapiom/analytics-core: minor` (`"authorization"` credential, + `credentialHeaders`). `harness-desktop` follows through `updateInternalDependencies: "patch"`. + +## Testing Strategy + +Order matters: harness resolves `@sapiom/analytics-core/credential` through its `dist` exports. + +1. `pnpm --filter @sapiom/analytics-core exec jest --maxWorkers=2 src/credential.spec.ts` +2. `pnpm --filter @sapiom/analytics-core build` +3. `pnpm --filter @sapiom/harness exec vitest run --maxWorkers=2 src/core/run-state.test.ts src/core/definition-slug-resolver.test.ts src/core/vault-secrets.test.ts src/core/account-plan.test.ts src/core/template-catalog.test.ts src/core/definition-app-link.test.ts src/server/opencode-bridge.test.ts src/core/inject/mcp-config.test.ts` +4. `pnpm credential-header:check` and `node --test scripts/credential-header-check.test.mjs` (its "passes on the + repository" case now runs with an empty allowlist). +5. `pnpm --filter @sapiom/harness typecheck` and `pnpm --filter @sapiom/analytics-core typecheck` if present, else `build`. + +## Risks + +- A user with a custom `~/.sapiom/credentials.json` environment on plain http to a non-loopback host (Docker service + name, LAN IP) loses Studio sessions at start (MCP config refusal) and sees the core/agents panels as unavailable, + until they set `SAPIOM_ALLOW_INSECURE_HTTP=1`. Built-in presets are https (`packages/mcp/src/credentials.ts:22-27`) + and `localhost` keeps working, so default users see no change. Called out in the changeset. +- The agents/core panels report a refusal through their existing generic failure text ("gateway unreachable", + "network error or timeout"); only account-plan and template-catalog log the refusal message itself. Acceptable + because session start (T6) surfaces the actionable message first. +- Test churn: exact-argument fetch assertions in the eight harness test files need `objectContaining` or lowercase + `authorization`. + +## Open Questions + +- Opt-in: is the env var enough, or does Studio need its own switch (CLI flag plus desktop option, both hosts)? + This plan ships the env var only. +- MCP config refusal: fail session start (this plan) or write the config without the header (remote MCP tools then + 401)? Failing loudly names the fix; omitting keeps local tools usable. diff --git a/scripts/credential-header-allowlist.json b/scripts/credential-header-allowlist.json index 9b4c3034e..fe51488c7 100644 --- a/scripts/credential-header-allowlist.json +++ b/scripts/credential-header-allowlist.json @@ -1,22 +1 @@ -[ - { - "id": "harness-run-state", - "path": "packages/harness/src/core/run-state.ts", - "ticket": "SAP-4112" - }, - { - "id": "harness-mcp-config", - "path": "packages/harness/src/core/inject/mcp-config.ts", - "ticket": "SAP-4112" - }, - { - "id": "harness-definition-slug-resolver", - "path": "packages/harness/src/core/definition-slug-resolver.ts", - "ticket": "SAP-4112" - }, - { - "id": "harness-opencode-bridge", - "path": "packages/harness/src/server/opencode-bridge.ts", - "ticket": "SAP-4112" - } -] +[]