From 26813d83a48a9ac76e1aa62aa2e50e24281f65f4 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:10:22 -0500 Subject: [PATCH 01/11] fix(sdk): route the tenant key through one shared credentialFetch, enforced in CI [SAP-4106] The channel rule from #1110 lived in @sapiom/tools only, so analytics-core, core, agent-core and mcp kept sending the key over plain http and across cross-origin redirects. The policy module moves to the new @sapiom/analytics-core/credential subpath, and credentialFetch becomes the one function that sets the key header for every client. A refused channel is a CredentialChannelError (a TypeError) that core does not retry. scripts/credential-header-check.mjs fails CI when packages/*/src sets the key header by hand; the four harness files are allowlisted until SAP-4112. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/sdk-shared-credential-transport.md | 28 +++ .github/workflows/test.yml | 5 + package.json | 1 + .../agent-core/src/__tests__/client.test.ts | 86 ++++++++++ packages/agent-core/src/client.ts | 58 +++++-- packages/analytics-core/CONTRACT.md | 4 + packages/analytics-core/package.json | 8 + .../src/__tests__/http-sender-channel.test.ts | 144 ++++++++++++++++ .../src/credential.spec.ts} | 160 +++++++++++++++++- .../src/credential.ts} | 111 +++++++++--- packages/analytics-core/src/http-sender.ts | 70 ++++++-- packages/analytics-core/src/types.ts | 4 + packages/core/package.json | 4 +- .../src/client/HttpClient.channel.test.ts | 153 +++++++++++++++++ packages/core/src/client/HttpClient.ts | 49 +++++- packages/core/src/client/SapiomClient.test.ts | 5 +- packages/core/src/client/SapiomClient.ts | 23 ++- packages/mcp/src/tools/app-links-api.test.ts | 106 ++++++++++++ packages/mcp/src/tools/app-links-api.ts | 25 ++- .../tools/src/_client/analytics.e2e.spec.ts | 2 +- packages/tools/src/_client/index.ts | 29 ++-- .../src/_client/transport-security.spec.ts | 5 +- pnpm-lock.yaml | 6 +- scripts/credential-header-allowlist.json | 22 +++ scripts/credential-header-check.mjs | 126 ++++++++++++++ scripts/credential-header-check.test.mjs | 57 +++++++ 26 files changed, 1202 insertions(+), 89 deletions(-) create mode 100644 .changeset/sdk-shared-credential-transport.md create mode 100644 packages/analytics-core/src/__tests__/http-sender-channel.test.ts rename packages/{tools/src/_client/credential-policy.spec.ts => analytics-core/src/credential.spec.ts} (54%) rename packages/{tools/src/_client/credential-policy.ts => analytics-core/src/credential.ts} (64%) create mode 100644 packages/core/src/client/HttpClient.channel.test.ts create mode 100644 packages/mcp/src/tools/app-links-api.test.ts create mode 100644 scripts/credential-header-allowlist.json create mode 100644 scripts/credential-header-check.mjs create mode 100644 scripts/credential-header-check.test.mjs diff --git a/.changeset/sdk-shared-credential-transport.md b/.changeset/sdk-shared-credential-transport.md new file mode 100644 index 000000000..67f177d40 --- /dev/null +++ b/.changeset/sdk-shared-credential-transport.md @@ -0,0 +1,28 @@ +--- +"@sapiom/analytics-core": minor +"@sapiom/core": minor +"@sapiom/agent-core": minor +"@sapiom/mcp": patch +"@sapiom/tools": patch +--- + +The tenant API key now only travels over HTTPS, or plain HTTP to a loopback +host, from every SDK client, and never follows a redirect to another origin. +One function, `credentialFetch` from the new `@sapiom/analytics-core/credential` +subpath, is the only code that sets the key header. + +- `@sapiom/core`, `@sapiom/agent-core` and `@sapiom/mcp`: a plain `http://` + URL to a non-loopback host (for example a Docker service name or a LAN IP) + now fails before the request is sent. On a trusted private network, opt in + with `new SapiomClient({ allowInsecureHttp: true })` (core), + `createClient({ allowInsecureHttp: true })` (agent-core) or + `SAPIOM_ALLOW_INSECURE_HTTP=1` (all clients). `localhost`, `*.localhost`, + `127.0.0.1` and `[::1]` work as before. Core does not retry a refused channel. +- `@sapiom/analytics-core`: on such an endpoint the batch is still sent, without + the key. A redirect to a refused channel drops the batch. +- Every client follows redirects itself: the first hop to another origin drops + `x-api-key`, `x-sapiom-api-key` and every `x-sapiom-*` header. +- `@sapiom/core`: `getHttpClient().defaults.headers` no longer carries the key; + read `defaults.credential` instead. +- `@sapiom/tools`: no behavior change; a refused call is now reported as a + `CredentialChannelError` (still a `TypeError`). diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9f637a212..e4b41699d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -109,6 +109,11 @@ jobs: - name: Check Agent Studio visible terminology run: pnpm terminology:check + # Only credentialFetch (@sapiom/analytics-core/credential) may set the + # tenant key header; see scripts/credential-header-check.mjs. + - name: Check tenant credential headers + run: pnpm credential-header:check + # Fixture tests for the gate itself: a typo'd declaration field must fail # rather than be silently stripped downstream. This glob also runs the # terminology guard's forbidden-copy, compatibility, and stale-allowlist diff --git a/package.json b/package.json index e35c285d2..fed7abb07 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ "examples:check:test": "node --test scripts/*.test.mjs", "examples:test": "node scripts/examples-test.mjs", "terminology:check": "node scripts/agent-studio-terminology-check.mjs", + "credential-header:check": "node scripts/credential-header-check.mjs", "terminology:check:test": "node --test scripts/agent-studio-terminology-check.test.mjs", "provider-copy:check": "node scripts/provider-neutral-copy-check.mjs", "provider-copy:check:test": "node --test scripts/provider-neutral-copy-check.test.mjs", diff --git a/packages/agent-core/src/__tests__/client.test.ts b/packages/agent-core/src/__tests__/client.test.ts index 7576b62ec..c0f03f625 100644 --- a/packages/agent-core/src/__tests__/client.test.ts +++ b/packages/agent-core/src/__tests__/client.test.ts @@ -238,6 +238,92 @@ describe('signal', () => { }); }); +// ── Credential channel ──────────────────────────────────────────────────────── + +describe('GatewayClient credential channel', () => { + const ENV = 'SAPIOM_ALLOW_INSECURE_HTTP'; + const ORIGINAL = process.env[ENV]; + beforeEach(() => { + delete process.env[ENV]; + }); + afterAll(() => { + if (ORIGINAL === undefined) delete process.env[ENV]; + else process.env[ENV] = ORIGINAL; + }); + + const sentKey = (spy: jest.SpyInstance, call: number) => + (spy.mock.calls[call][1] as RequestInit & { headers: Record }).headers[ + 'x-api-key' + ]; + + it.each([ + ['get', (c: GatewayClient) => c.get('/foo')], + ['post', (c: GatewayClient) => c.post('/foo', { a: 1 })], + ['postAtHostRoot', (c: GatewayClient) => c.postAtHostRoot('/v1/studio-feedback', {})], + ['openStream', (c: GatewayClient) => c.openStream('/executions/e1/stream')], + ])('%s refuses plaintext to a non-loopback host as NETWORK, before sending', async (_m, call) => { + const spy = mockFetch([{ status: 200, body: {} }]); + const client = createClient({ host: 'http://gw.internal', apiKey: 'sk_live' }); + await expect(call(client)).rejects.toMatchObject({ + code: 'NETWORK', + hint: expect.stringContaining('@sapiom/agent-core: refusing plaintext HTTP to http://gw.internal'), + }); + expect(spy).not.toHaveBeenCalled(); + }); + + it('names its own opt-in in the refusal', async () => { + mockFetch([{ status: 200, body: {} }]); + const client = createClient({ host: 'http://gw.internal', apiKey: 'sk_live' }); + await expect(client.get('/foo')).rejects.toMatchObject({ + hint: expect.stringContaining( + 'createClient({ allowInsecureHttp: true }) or SAPIOM_ALLOW_INSECURE_HTTP=1', + ), + }); + }); + + it.each([ + ['a loopback host', { host: 'http://localhost:3000' }, undefined], + ['the client option', { host: 'http://gw.internal', allowInsecureHttp: true }, undefined], + ['the env var', { host: 'http://gw.internal' }, '1'], + ])('sends over plain http to %s', async (_label, options, env) => { + if (env) process.env[ENV] = env; + const spy = mockFetch([{ status: 200, body: {} }]); + await createClient({ apiKey: 'sk_live', ...options }).get('/foo'); + expect(sentKey(spy, 0)).toBe('sk_live'); + }); + + it('drops x-api-key on a redirect to another origin (get)', async () => { + const spy = jest + .spyOn(global, 'fetch' as any) + .mockResolvedValueOnce( + new Response(null, { status: 307, headers: { location: 'https://other.example/foo' } }), + ) + .mockResolvedValueOnce(new Response('{"ok":true}', { status: 200 })); + const client = createClient({ host: 'https://example.com', apiKey: 'sk_live' }); + await expect(client.get('/foo')).resolves.toEqual({ ok: true }); + expect(spy.mock.calls.map((c) => c[0])).toEqual([ + 'https://example.com/v1/workflows/foo', + 'https://other.example/foo', + ]); + expect([sentKey(spy, 0), sentKey(spy, 1)]).toEqual(['sk_live', undefined]); + }); + + it('drops x-api-key on a redirect to another origin (openStream), keeping the body open', async () => { + const spy = jest + .spyOn(global, 'fetch' as any) + .mockResolvedValueOnce( + new Response(null, { status: 307, headers: { location: 'https://other.example/s' } }), + ) + .mockResolvedValueOnce(new Response('data: 1\n\n', { status: 200 })); + const client = createClient({ host: 'https://example.com', apiKey: 'sk_live' }); + const res = await client.openStream('/executions/e1/stream'); + expect(spy.mock.calls[1][0]).toBe('https://other.example/s'); + expect([sentKey(spy, 0), sentKey(spy, 1)]).toEqual(['sk_live', undefined]); + expect(res.bodyUsed).toBe(false); + expect(await res.text()).toBe('data: 1\n\n'); + }); +}); + // ── emitEvent ───────────────────────────────────────────────────────────────── describe('emitEvent', () => { diff --git a/packages/agent-core/src/client.ts b/packages/agent-core/src/client.ts index f68cc2ffa..8173e7101 100644 --- a/packages/agent-core/src/client.ts +++ b/packages/agent-core/src/client.ts @@ -1,8 +1,15 @@ /** * Configurable HTTP client for the Sapiom workflows backend API. All inputs are - * passed explicitly (base URL + API key) — no process.env reads, no global - * state — so the client is usable from a CLI, an MCP tool, or a test harness. + * passed explicitly (base URL + API key), with no global state and no + * process.env read but the shared `SAPIOM_ALLOW_INSECURE_HTTP` opt-in, so the + * client is usable from a CLI, an MCP tool, or a test harness. */ +import { + credentialFetch, + resolveCredentialPolicy, + type Credential, + type CredentialPolicy, +} from '@sapiom/analytics-core/credential'; import { AgentOperationError } from './errors.js'; /** @@ -17,6 +24,12 @@ export interface ClientOptions { host?: string; /** API key sent as `x-api-key`. Must start with `sk_`. */ apiKey: string; + /** + * Let the API key travel over plain http to a non-loopback host (a trusted + * private network). By default it only goes over https or to localhost, and + * `SAPIOM_ALLOW_INSECURE_HTTP=1` opts in too. + */ + allowInsecureHttp?: boolean; } /** Result shape from a failed gateway request. */ @@ -42,12 +55,17 @@ export class GatewayClient { private readonly host: string; /** `${host}/v1/workflows` — what get/post/request/openStream paths are relative to. */ private readonly base: string; - private readonly apiKey: string; + private readonly credential: Credential; + private readonly policy: CredentialPolicy; constructor(opts: ClientOptions) { this.host = stripTrailingSlashes(opts.host ?? DEFAULT_WORKFLOWS_HOST); this.base = `${this.host}/v1/workflows`; - this.apiKey = opts.apiKey; + this.credential = { header: 'x-api-key', value: opts.apiKey }; + this.policy = resolveCredentialPolicy(opts.allowInsecureHttp, { + label: '@sapiom/agent-core', + optIn: 'createClient({ allowInsecureHttp: true })', + }); } /** `path` is relative to `/v1/workflows`. */ @@ -110,12 +128,19 @@ export class GatewayClient { let res: Response; try { - res = await fetch(url, { - method, - headers: { 'x-api-key': this.apiKey, 'content-type': 'application/json' }, - body: payload, - signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), - }); + // A refused channel lands in the same NETWORK error, its reason as the hint. + res = await credentialFetch( + fetch, + url, + { + method, + headers: { 'content-type': 'application/json' }, + body: payload, + signal: AbortSignal.timeout(REQUEST_TIMEOUT_MS), + }, + this.credential, + this.policy, + ); } catch (err) { throw networkError(url, err); } @@ -160,10 +185,7 @@ export class GatewayClient { path: string, opts: { signal?: AbortSignal; lastEventId?: string } = {}, ): Promise { - const headers: Record = { - 'x-api-key': this.apiKey, - accept: 'text/event-stream', - }; + const headers: Record = { accept: 'text/event-stream' }; if (opts.lastEventId) { headers['last-event-id'] = opts.lastEventId; } @@ -171,7 +193,13 @@ export class GatewayClient { const url = `${this.base}${path}`; let res: Response; try { - res = await fetch(url, { method: 'GET', headers, signal: opts.signal }); + res = await credentialFetch( + fetch, + url, + { method: 'GET', headers, signal: opts.signal }, + this.credential, + this.policy, + ); } catch (err) { throw networkError(url, err); } diff --git a/packages/analytics-core/CONTRACT.md b/packages/analytics-core/CONTRACT.md index b4acfb15a..4e8991890 100644 --- a/packages/analytics-core/CONTRACT.md +++ b/packages/analytics-core/CONTRACT.md @@ -21,6 +21,10 @@ Authentication is optional and does exactly one thing: a **valid** to the events (see [Server-stamped fields](#server-stamped-fields-never-client-trusted)). An invalid or missing key never causes a rejection. +The key is only sent over https or to a loopback host (or with +`SAPIOM_ALLOW_INSECURE_HTTP=1`); on any other endpoint the batch goes out +without it. A redirect to another origin drops the key. + This URL is exported by `@sapiom/analytics-core` as the constant `SAPIOM_COLLECTOR_ENDPOINT` and is the emitter's **default endpoint**: an emitter with no explicit `endpoint` configured delivers here, unless the diff --git a/packages/analytics-core/package.json b/packages/analytics-core/package.json index 3f5e843b4..fa29ff412 100644 --- a/packages/analytics-core/package.json +++ b/packages/analytics-core/package.json @@ -22,12 +22,20 @@ "types": "./dist/cjs/testing/index.d.ts", "import": "./dist/esm/testing/index.js", "require": "./dist/cjs/testing/index.js" + }, + "./credential": { + "types": "./dist/cjs/credential.d.ts", + "import": "./dist/esm/credential.js", + "require": "./dist/cjs/credential.js" } }, "typesVersions": { "*": { "testing": [ "./dist/cjs/testing/index.d.ts" + ], + "credential": [ + "./dist/cjs/credential.d.ts" ] } }, diff --git a/packages/analytics-core/src/__tests__/http-sender-channel.test.ts b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts new file mode 100644 index 000000000..1ae2f13ce --- /dev/null +++ b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts @@ -0,0 +1,144 @@ +import { ALLOW_INSECURE_HTTP_ENV } from "../credential.js"; +import { HttpSender } from "../http-sender.js"; +import type { FetchRequestInit, FetchResponseLike } from "../types.js"; + +type Call = { url: string; init: FetchRequestInit }; + +function senderWith( + endpoint: string, + responses: FetchResponseLike[], + apiKey: string | null = "sk_live", +) { + const calls: Call[] = []; + const debug: Array<[string, unknown]> = []; + const sender = new HttpSender({ + endpoint, + apiKey: apiKey ?? undefined, + debug: (message, detail) => debug.push([message, detail]), + fetchImpl: async (url, init) => { + calls.push({ url, init }); + return responses.shift() ?? { ok: true, status: 202 }; + }, + }); + return { sender, calls, debug }; +} + +const redirectTo = (location: string, status = 307): FetchResponseLike => ({ + ok: false, + status, + headers: new Headers({ location }), +}); + +const keyOf = (call: Call) => call.init.headers["x-sapiom-api-key"]; + +describe("HttpSender credential channel", () => { + const ORIGINAL = process.env[ALLOW_INSECURE_HTTP_ENV]; + beforeEach(() => { + delete process.env[ALLOW_INSECURE_HTTP_ENV]; + }); + afterAll(() => { + if (ORIGINAL === undefined) delete process.env[ALLOW_INSECURE_HTTP_ENV]; + else process.env[ALLOW_INSECURE_HTTP_ENV] = ORIGINAL; + }); + + it("sends the batch without the key to an endpoint that is not https or loopback", async () => { + const { sender, calls, debug } = senderWith( + "http://collector.internal/v1/analytics/collector?t=secret", + [{ ok: true, status: 202 }], + ); + await expect(sender.send([])).resolves.toBe("ok"); + expect(calls).toHaveLength(1); + expect(keyOf(calls[0])).toBeUndefined(); + expect(calls[0].init.body).toBe('{"events":[]}'); + expect(debug).toEqual([ + [ + "endpoint is not https or loopback; batch sent without the API key", + "http://collector.internal", + ], + ]); + }); + + it.each([ + ["https", "https://collector.example/c"], + ["loopback", "http://127.0.0.1:4318/c"], + ])("carries the key over %s", async (_label, endpoint) => { + const { sender, calls, debug } = senderWith(endpoint, []); + await expect(sender.send([])).resolves.toBe("ok"); + expect(keyOf(calls[0])).toBe("sk_live"); + expect(calls[0].init.redirect).toBe("manual"); + expect(debug).toEqual([]); + }); + + it("carries the key over plain http once opted in through the env", async () => { + process.env[ALLOW_INSECURE_HTTP_ENV] = "1"; + const { sender, calls } = senderWith("http://collector.internal/c", []); + await sender.send([]); + expect(keyOf(calls[0])).toBe("sk_live"); + }); + + it("without a key, sends no header and says nothing", async () => { + const { sender, calls, debug } = senderWith( + "http://collector.internal/c", + [], + null, + ); + await expect(sender.send([])).resolves.toBe("ok"); + expect("x-sapiom-api-key" in calls[0].init.headers).toBe(false); + expect(debug).toEqual([]); + }); + + it("drops the key on a redirect to another origin, resending the body", async () => { + const { sender, calls } = senderWith("https://collector.example/c", [ + redirectTo("https://other.example/c"), + { ok: false, status: 429 }, + ]); + await expect(sender.send([])).resolves.toBe("retry"); + expect(calls.map((c) => c.url)).toEqual([ + "https://collector.example/c", + "https://other.example/c", + ]); + expect(calls.map(keyOf)).toEqual(["sk_live", undefined]); + expect(calls[1].init.body).toBe('{"events":[]}'); + expect(calls.every((c) => c.init.redirect === "manual")).toBe(true); + }); + + it("keeps the key on a same-origin redirect", async () => { + const { sender, calls } = senderWith("https://collector.example/c", [ + redirectTo("/v2/c"), + { ok: true, status: 202 }, + ]); + await expect(sender.send([])).resolves.toBe("ok"); + expect(calls.map(keyOf)).toEqual(["sk_live", "sk_live"]); + }); + + it("drops the batch, without a second call, when a redirect leaves https", async () => { + const { sender, calls, debug } = senderWith("https://collector.example/c", [ + redirectTo("http://evil.internal/c"), + ]); + await expect(sender.send([])).resolves.toBe("drop"); + expect(calls).toHaveLength(1); + expect(debug).toHaveLength(1); + expect(debug[0][0]).toBe("collector redirect refused; batch dropped"); + expect(String(debug[0][1])).toContain("http://evil.internal"); + }); + + it("treats a redirect status with no headers as the final response", async () => { + const { sender, calls } = senderWith("https://collector.example/c", [ + { ok: false, status: 307 }, + ]); + await expect(sender.send([])).resolves.toBe("drop"); + expect(calls).toHaveLength(1); + }); + + it("still retries a failed connection", async () => { + const sender = new HttpSender({ + endpoint: "https://collector.example/c", + apiKey: "sk_live", + debug: () => undefined, + fetchImpl: async () => { + throw new TypeError("fetch failed"); + }, + }); + await expect(sender.send([])).resolves.toBe("retry"); + }); +}); diff --git a/packages/tools/src/_client/credential-policy.spec.ts b/packages/analytics-core/src/credential.spec.ts similarity index 54% rename from packages/tools/src/_client/credential-policy.spec.ts rename to packages/analytics-core/src/credential.spec.ts index 7657c5cdd..0025b40d2 100644 --- a/packages/tools/src/_client/credential-policy.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -2,16 +2,23 @@ import { createHash, type BinaryToTextEncoding } from "node:crypto"; import { ALLOW_INSECURE_HTTP_ENV, + CredentialChannelError, assertCredentialMayTravel, + credentialFetch, isLoopbackHostname, matchesIntegrity, + mayCarryCredential, resolveCredentialPolicy, withoutBodyHeaders, withoutCrossOriginHeaders, -} from "./credential-policy.js"; +} from "./credential.js"; -const STRICT = { allowInsecureHttp: false }; -const OPTED_IN = { allowInsecureHttp: true }; +const WORDING = { + label: "@sapiom/test", + optIn: "createClient({ allowInsecureHttp: true })", +}; +const STRICT = { allowInsecureHttp: false, ...WORDING }; +const OPTED_IN = { allowInsecureHttp: true, ...WORDING }; describe("isLoopbackHostname", () => { it.each([ @@ -67,6 +74,24 @@ describe("assertCredentialMayTravel", () => { ); }); + it("names the label, the client's opt-in and the env var", () => { + expect(check("http://api.internal/v1")).toThrow( + "@sapiom/test: refusing plaintext HTTP to http://api.internal. " + + "The tenant API key only travels over https or to a loopback host " + + "(localhost, *.localhost, 127.0.0.1, [::1]). For a trusted private network, " + + "opt in with createClient({ allowInsecureHttp: true }) or SAPIOM_ALLOW_INSECURE_HTTP=1.", + ); + }); + + it("names only the env var when the client has no opt-in of its own", () => { + expect(() => + assertCredentialMayTravel(new URL("http://api.internal/v1"), { + allowInsecureHttp: false, + label: "@sapiom/mcp", + }), + ).toThrow(/^@sapiom\/mcp: .*opt in with SAPIOM_ALLOW_INSECURE_HTTP=1\.$/); + }); + it("allows plaintext to a non-loopback host once opted in", () => { expect(check("http://api:3000/v1", OPTED_IN)).not.toThrow(); }); @@ -85,7 +110,9 @@ describe("assertCredentialMayTravel", () => { } catch (e) { thrown = e; } + expect(thrown).toBeInstanceOf(CredentialChannelError); expect(thrown).toBeInstanceOf(TypeError); + expect((thrown as Error).name).toBe("CredentialChannelError"); expect((thrown as { cause?: unknown }).cause).toBeUndefined(); expect((thrown as Error).message).not.toMatch(/secret|token=abc/); }); @@ -101,6 +128,33 @@ describe("assertCredentialMayTravel", () => { }); }); +describe("mayCarryCredential", () => { + it.each([ + "https://api.internal/", + "http://localhost:3000/", + "http://vault.localhost:3999/", + "http://127.0.0.5/", + "http://[::1]:3000/", + ])("%s may carry the key", (url) => { + expect(mayCarryCredential(new URL(url), STRICT)).toBe(true); + }); + + it("plaintext to a non-loopback host may not, unless opted in", () => { + expect(mayCarryCredential(new URL("http://api.internal/"), STRICT)).toBe( + false, + ); + expect(mayCarryCredential(new URL("http://api.internal/"), OPTED_IN)).toBe( + true, + ); + }); + + it("another scheme may not, even opted in", () => { + expect(mayCarryCredential(new URL("ws://localhost/"), OPTED_IN)).toBe( + false, + ); + }); +}); + describe("resolveCredentialPolicy", () => { const ORIGINAL = process.env[ALLOW_INSECURE_HTTP_ENV]; afterEach(() => { @@ -110,7 +164,9 @@ describe("resolveCredentialPolicy", () => { it("is strict when nothing is set", () => { delete process.env[ALLOW_INSECURE_HTTP_ENV]; - expect(resolveCredentialPolicy(undefined).allowInsecureHttp).toBe(false); + expect(resolveCredentialPolicy(undefined, WORDING).allowInsecureHttp).toBe( + false, + ); }); it.each([ @@ -122,14 +178,22 @@ describe("resolveCredentialPolicy", () => { ["", false], ])("env %j opts in: %s", (value, expected) => { process.env[ALLOW_INSECURE_HTTP_ENV] = value; - expect(resolveCredentialPolicy(undefined).allowInsecureHttp).toBe(expected); + expect(resolveCredentialPolicy(undefined, WORDING).allowInsecureHttp).toBe( + expected, + ); + }); + + it("carries the client's wording", () => { + expect(resolveCredentialPolicy(undefined, WORDING)).toMatchObject(WORDING); }); it("an explicit value beats the env", () => { process.env[ALLOW_INSECURE_HTTP_ENV] = "1"; - expect(resolveCredentialPolicy(false).allowInsecureHttp).toBe(false); + expect(resolveCredentialPolicy(false, WORDING).allowInsecureHttp).toBe( + false, + ); delete process.env[ALLOW_INSECURE_HTTP_ENV]; - expect(resolveCredentialPolicy(true).allowInsecureHttp).toBe(true); + expect(resolveCredentialPolicy(true, WORDING).allowInsecureHttp).toBe(true); }); }); @@ -213,3 +277,85 @@ describe("matchesIntegrity", () => { expect(matchesIntegrity(BODY, metadata)).toBe(false); }); }); + +describe("credentialFetch", () => { + type Call = { url: string; headers: Record }; + const recorder = (...responses: unknown[]) => { + const calls: Call[] = []; + const fetchImpl = (async (url: string, init: RequestInit) => { + calls.push({ url, headers: init.headers as Record }); + return responses.shift(); + }) as unknown as typeof fetch; + return { calls, fetchImpl }; + }; + const KEY = { header: "x-api-key", value: "sk_real" } as const; + + it("sets the key after the caller's headers, which cannot forge it", async () => { + const { calls, fetchImpl } = recorder(new Response("ok")); + await credentialFetch( + fetchImpl, + "https://api.example/x", + { headers: { "X-Api-Key": "forged", accept: "application/json" } }, + KEY, + STRICT, + ); + expect(calls[0].headers).toEqual({ + accept: "application/json", + "x-api-key": "sk_real", + }); + }); + + it("refuses an untrusted channel before calling fetch", async () => { + const { calls, fetchImpl } = recorder(); + await expect( + credentialFetch(fetchImpl, "http://api.internal/x", {}, KEY, STRICT), + ).rejects.toBeInstanceOf(CredentialChannelError); + expect(calls).toHaveLength(0); + }); + + it("without a credential, sends no key and does not check the first URL", async () => { + const { calls, fetchImpl } = recorder(new Response("ok")); + await credentialFetch( + fetchImpl, + "http://api.internal/x", + { headers: { accept: "*/*" } }, + undefined, + STRICT, + ); + expect(calls).toEqual([ + { url: "http://api.internal/x", headers: { accept: "*/*" } }, + ]); + }); + + it("returns a minimal redirect with no headers as is, after one call", async () => { + const minimal = { ok: false, status: 307 }; + const { calls, fetchImpl } = recorder(minimal); + await expect( + credentialFetch(fetchImpl, "https://api.example/x", {}, KEY, STRICT), + ).resolves.toBe(minimal); + expect(calls).toHaveLength(1); + }); + + it("follows a minimal redirect carrying headers to a response with no clone", async () => { + const { calls, fetchImpl } = recorder( + { + ok: false, + status: 307, + headers: new Headers({ location: "https://other.example/y" }), + }, + { ok: true, status: 202 }, + ); + const response = await credentialFetch( + fetchImpl, + "https://api.example/x", + {}, + KEY, + STRICT, + ); + expect(response).toMatchObject({ ok: true, status: 202, redirected: true }); + expect(calls.map((c) => c.headers["x-api-key"])).toEqual([ + "sk_real", + undefined, + ]); + }); +}); diff --git a/packages/tools/src/_client/credential-policy.ts b/packages/analytics-core/src/credential.ts similarity index 64% rename from packages/tools/src/_client/credential-policy.ts rename to packages/analytics-core/src/credential.ts index 0a641c8aa..2503bc557 100644 --- a/packages/tools/src/_client/credential-policy.ts +++ b/packages/analytics-core/src/credential.ts @@ -3,6 +3,11 @@ * loopback host. Redirects are followed here rather than by `fetch`, because * `fetch` drops `authorization` on an origin change but keeps custom headers * such as `x-sapiom-api-key`. + * + * Every SDK client puts the key on a request through {@link credentialFetch}, + * never by setting the header itself; `pnpm credential-header:check` enforces + * it in CI. The check reads literals only: a header name held in a variable, + * or the key sent as `Authorization: Bearer`, is for review to catch. */ import { createHash } from "node:crypto"; @@ -11,14 +16,22 @@ export const ALLOW_INSECURE_HTTP_ENV = "SAPIOM_ALLOW_INSECURE_HTTP"; export interface CredentialPolicy { /** Plain http to a non-loopback host is allowed (trusted private network). */ readonly allowInsecureHttp: boolean; + /** Prefix of every error message: the package the caller uses. */ + readonly label: string; + /** The client's own opt-in, named in a refusal next to the env var. */ + readonly optIn?: string; } /** Explicit config wins, then `SAPIOM_ALLOW_INSECURE_HTTP=1` (or `true`). */ export function resolveCredentialPolicy( explicit: boolean | undefined, + wording: { label: string; optIn?: string }, ): CredentialPolicy { const env = process.env[ALLOW_INSECURE_HTTP_ENV]?.trim().toLowerCase(); - return { allowInsecureHttp: explicit ?? (env === "1" || env === "true") }; + return { + allowInsecureHttp: explicit ?? (env === "1" || env === "true"), + ...wording, + }; } /** `localhost`, `*.localhost` (RFC 6761, used by the local services stack), 127/8, `[::1]`. */ @@ -33,30 +46,53 @@ export function isLoopbackHostname(hostname: string): boolean { } /** - * Throws, before anything is sent, when `url` is not a channel the credential - * may use. A TypeError with no `cause`, unlike a failed connection. Only the - * origin is echoed: a path or query can carry a secret. + * A refused channel. A TypeError like the ones `fetch` throws, so existing + * catches still apply, but one a retry loop can tell apart from a failed + * connection: retrying it cannot succeed. + */ +export class CredentialChannelError extends TypeError { + constructor(message: string) { + super(message); + this.name = "CredentialChannelError"; + } +} + +/** The non-throwing form of {@link assertCredentialMayTravel}. */ +export function mayCarryCredential( + url: URL, + policy: CredentialPolicy, +): boolean { + if (url.protocol === "https:") return true; + return ( + url.protocol === "http:" && + (policy.allowInsecureHttp || isLoopbackHostname(url.hostname)) + ); +} + +/** + * Throws a {@link CredentialChannelError}, before anything is sent, when `url` + * is not a channel the credential may use. It has no `cause`, unlike a failed + * connection. Only the origin is echoed: a path or query can carry a secret. */ export function assertCredentialMayTravel( url: URL, policy: CredentialPolicy, redirectedFrom?: URL, ): void { - if (url.protocol === "https:") return; + if (mayCarryCredential(url, policy)) return; const from = redirectedFrom ? ` (redirected from ${redirectedFrom.origin})` : ""; if (url.protocol !== "http:") { - throw new TypeError( - `@sapiom/tools: cannot request '${url.protocol}' URLs, expected http or https${from}`, + throw new CredentialChannelError( + `${policy.label}: cannot request '${url.protocol}' URLs, expected http or https${from}`, ); } - if (policy.allowInsecureHttp || isLoopbackHostname(url.hostname)) return; - throw new TypeError( - `@sapiom/tools: refusing plaintext HTTP to ${url.origin}${from}. The tenant API key only ` + + const optIn = policy.optIn ? `${policy.optIn} or ` : ""; + throw new CredentialChannelError( + `${policy.label}: refusing plaintext HTTP to ${url.origin}${from}. The tenant API key only ` + `travels over https or to a loopback host (localhost, *.localhost, 127.0.0.1, [::1]). ` + - `For a trusted private network, opt in with createClient({ allowInsecureHttp: true }) ` + - `or ${ALLOW_INSECURE_HTTP_ENV}=1.`, + `For a trusted private network, opt in with ${optIn}${ALLOW_INSECURE_HTTP_ENV}=1.`, ); } @@ -162,6 +198,7 @@ async function assertIntegrity( response: Response, metadata: string, url: URL, + policy: CredentialPolicy, ): Promise { if ( response.body !== null && @@ -174,15 +211,19 @@ async function assertIntegrity( } await response.body?.cancel().catch(() => undefined); throw new TypeError( - `@sapiom/tools: the response from ${url.origin} does not match the request's integrity metadata`, + `${policy.label}: the response from ${url.origin} does not match the request's integrity metadata`, ); } /** * Each hop is its own fetch, so none can report what `follow` would: set it on - * the final response and on every clone of it. + * the final response and on every clone of it. An injected fetch may answer a + * bare `{ ok, status }` with no `clone`. */ function markRedirected(response: Response): Response { + if (typeof response.clone !== "function") { + return Object.defineProperty(response, "redirected", { value: true }); + } const clone = response.clone.bind(response); return Object.defineProperties(response, { redirected: { value: true }, @@ -223,11 +264,13 @@ export async function fetchKeepingCredential( headers: hopHeaders, redirect: "manual", }); + // A response without `headers` (a minimal injected fetch) cannot be followed. const location = REDIRECT_STATUSES.has(response.status) - ? response.headers.get("location") + ? ((response.headers as Headers | undefined)?.get?.("location") ?? null) : null; if (location === null) { - if (integrity) await assertIntegrity(response, integrity, current); + if (integrity) + await assertIntegrity(response, integrity, current, policy); return redirects > 0 ? markRedirected(response) : response; } // Never handed to the caller: free the connection, even if we refuse the hop. @@ -237,17 +280,17 @@ export async function fetchKeepingCredential( assertCredentialMayTravel(next, policy, current); if (redirects === MAX_REDIRECTS) { throw new TypeError( - `@sapiom/tools: more than ${MAX_REDIRECTS} redirects from ${start.origin}`, + `${policy.label}: more than ${MAX_REDIRECTS} redirects from ${start.origin}`, ); } if (init.mode === "same-origin" && next.origin !== start.origin) { throw new TypeError( - `@sapiom/tools: a "same-origin" request to ${start.origin} was redirected to ${next.origin}`, + `${policy.label}: a "same-origin" request to ${start.origin} was redirected to ${next.origin}`, ); } if (response.status !== 303 && isStreamBody(body)) { throw new TypeError( - `@sapiom/tools: cannot resend a streamed request body after a ${response.status} from ${current.origin}`, + `${policy.label}: cannot resend a streamed request body after a ${response.status} from ${current.origin}`, ); } const verb = (method ?? "GET").toUpperCase(); @@ -267,3 +310,33 @@ export async function fetchKeepingCredential( href = next.href; } } + +/** The tenant key and the header it rides on. */ +export interface Credential { + readonly header: "x-api-key" | "x-sapiom-api-key"; + readonly value: string; +} + +/** + * The one way an SDK client sends the tenant key: checks the channel, sets the + * header after `init.headers` (which can neither override nor forge it), and + * follows redirects through {@link fetchKeepingCredential}. Without a + * credential, nothing is checked on the first URL; redirect hops still are. + */ +export async function credentialFetch( + fetchImpl: typeof globalThis.fetch, + url: string, + init: RequestInit, + credential: Credential | undefined, + policy: CredentialPolicy, +): Promise { + // Merged as a plain object: callers pass records. + const callerHeaders = (init.headers ?? {}) as Record; + if (!credential) { + return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, policy); + } + assertCredentialMayTravel(new URL(url), policy); + const headers = pick(callerHeaders, (name) => name !== credential.header); + headers[credential.header] = credential.value; + return fetchKeepingCredential(fetchImpl, url, init, headers, policy); +} diff --git a/packages/analytics-core/src/http-sender.ts b/packages/analytics-core/src/http-sender.ts index 22deac8ef..ced42ec4c 100644 --- a/packages/analytics-core/src/http-sender.ts +++ b/packages/analytics-core/src/http-sender.ts @@ -1,3 +1,11 @@ +import { + CredentialChannelError, + credentialFetch, + mayCarryCredential, + resolveCredentialPolicy, + type Credential, + type CredentialPolicy, +} from "./credential.js"; import type { DebugHook, Envelope, FetchLike } from "./types.js"; /** @@ -59,7 +67,13 @@ export interface HttpSenderOptions { * Never rejects — every failure maps to a {@link SendOutcome}. */ export class HttpSender { - constructor(private readonly options: HttpSenderOptions) {} + private readonly policy: CredentialPolicy; + + constructor(private readonly options: HttpSenderOptions) { + this.policy = resolveCredentialPolicy(undefined, { + label: "@sapiom/analytics-core", + }); + } async send(events: Envelope[]): Promise { try { @@ -74,31 +88,55 @@ export class HttpSender { return "drop"; } - const headers: Record = { - "content-type": "application/json", - }; - if (this.options.apiKey) { - headers["x-sapiom-api-key"] = this.options.apiKey; - } - - const response = await fetchImpl(this.options.endpoint, { - method: "POST", - headers, - body, - signal: createTimeoutSignal( - this.options.timeoutMs ?? REQUEST_TIMEOUT_MS, - ), - }); + const response = await credentialFetch( + // Only `{ ok, status }` is read, and `credentialFetch` tolerates a + // response that has nothing more. + fetchImpl as unknown as typeof fetch, + this.options.endpoint, + { + method: "POST", + headers: { "content-type": "application/json" }, + body, + signal: createTimeoutSignal( + this.options.timeoutMs ?? REQUEST_TIMEOUT_MS, + ), + }, + this.credentialFor(this.options.endpoint), + this.policy, + ); if (response.ok) return "ok"; if (response.status === 429 || response.status >= 500) return "retry"; // Anything else (400 not-JSON, 413 too large, …): retrying cannot help. return "drop"; } catch (error) { + if (error instanceof CredentialChannelError) { + // A redirect to a channel the key may not use: resending lands there again. + this.options.debug("collector redirect refused; batch dropped", error); + return "drop"; + } this.options.debug("collector request failed", error); return "retry"; } } + /** + * Analytics never fails the host over its channel: on an endpoint the key + * may not use, the batch still goes out, anonymous. + */ + private credentialFor(endpoint: string): Credential | undefined { + const { apiKey } = this.options; + if (!apiKey) return undefined; + const url = new URL(endpoint); + if (mayCarryCredential(url, this.policy)) { + return { header: "x-sapiom-api-key", value: apiKey }; + } + this.options.debug( + "endpoint is not https or loopback; batch sent without the API key", + url.origin, + ); + return undefined; + } + private resolveFetch(): FetchLike | null { if (this.options.fetchImpl) return this.options.fetchImpl; const globalFetch = (globalThis as { fetch?: FetchLike }).fetch; diff --git a/packages/analytics-core/src/types.ts b/packages/analytics-core/src/types.ts index 0ca654e88..df5019362 100644 --- a/packages/analytics-core/src/types.ts +++ b/packages/analytics-core/src/types.ts @@ -17,6 +17,8 @@ export type EventSource = export interface FetchResponseLike { ok: boolean; status: number; + /** Read only on a redirect, to follow its `location`. */ + headers?: { get(name: string): string | null }; } /** Request options the sender passes to `fetch` (or an injected replacement). */ @@ -25,6 +27,8 @@ export interface FetchRequestInit { headers: Record; body: string; signal?: AbortSignal; + /** Redirects are followed by the sender, which checks each hop. */ + redirect?: "manual"; } /** diff --git a/packages/core/package.json b/packages/core/package.json index b4c1ffa69..0f049fcef 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -52,7 +52,9 @@ "format": "prettier --write \"src/**/*.ts\"", "prepublishOnly": "pnpm build" }, - "dependencies": {}, + "dependencies": { + "@sapiom/analytics-core": "workspace:^" + }, "devDependencies": { "@types/jest": "^29.5.14", "@types/node": "^20.11.30", diff --git a/packages/core/src/client/HttpClient.channel.test.ts b/packages/core/src/client/HttpClient.channel.test.ts new file mode 100644 index 000000000..5099f9ebd --- /dev/null +++ b/packages/core/src/client/HttpClient.channel.test.ts @@ -0,0 +1,153 @@ +import { + ALLOW_INSECURE_HTTP_ENV, + CredentialChannelError, +} from "@sapiom/analytics-core/credential"; + +import { SapiomClient } from "./SapiomClient"; + +const mockFetch = jest.fn(); +global.fetch = mockFetch as any; + +const json = (status = 200, headers: Record = {}) => + new Response(status === 200 ? '{"ok":true}' : null, { + status, + headers: { "content-type": "application/json", ...headers }, + }); + +const sentHeaders = (call: number): Record => + mockFetch.mock.calls[call][1].headers; + +// A retry delay would show as a slow test; zero it so only the call count matters. +const RETRY = { maxAttempts: 3, baseDelayMs: 0 }; + +describe("HttpClient credential channel", () => { + const ORIGINAL = process.env[ALLOW_INSECURE_HTTP_ENV]; + beforeEach(() => { + mockFetch.mockReset(); + delete process.env[ALLOW_INSECURE_HTTP_ENV]; + }); + afterAll(() => { + if (ORIGINAL === undefined) delete process.env[ALLOW_INSECURE_HTTP_ENV]; + else process.env[ALLOW_INSECURE_HTTP_ENV] = ORIGINAL; + }); + + it("refuses plaintext to a non-loopback API host before sending, without retrying", async () => { + const client = new SapiomClient({ + apiKey: "sk_live", + baseURL: "http://api.internal", + retry: RETRY, + }); + const refusal = client.request({ url: "/x" }); + await expect(refusal).rejects.toBeInstanceOf(CredentialChannelError); + await expect(refusal).rejects.toThrow( + /^@sapiom\/core: refusing plaintext HTTP to http:\/\/api\.internal\..*new SapiomClient\(\{ allowInsecureHttp: true \}\) or SAPIOM_ALLOW_INSECURE_HTTP=1\.$/, + ); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("refuses plaintext to a non-loopback vault host", async () => { + const client = new SapiomClient({ + apiKey: "sk_live", + servicesDomain: "http://svc.internal", + retry: RETRY, + }); + await expect(client.vault.getAll("demo")).rejects.toBeInstanceOf( + CredentialChannelError, + ); + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it("sends to loopback API and vault hosts", async () => { + mockFetch.mockImplementation(async () => json()); + const client = new SapiomClient({ + apiKey: "sk_live", + baseURL: "http://localhost:3000", + servicesDomain: "http://localhost:3999", + }); + await client.request({ url: "/x" }); + await client.vault.getAll("demo"); + expect(mockFetch.mock.calls.map((c) => c[0])).toEqual([ + "http://localhost:3000/v1/x", + "http://vault.localhost:3999/v2/secrets/demo", + ]); + expect(sentHeaders(0)["x-api-key"]).toBe("sk_live"); + expect(sentHeaders(1)["x-sapiom-api-key"]).toBe("sk_live"); + }); + + it.each([ + ["the client option", { allowInsecureHttp: true }, undefined], + ["the env var", {}, "1"], + ])( + "sends plaintext to a non-loopback host when opted in by %s", + async (_label, options, env) => { + if (env) process.env[ALLOW_INSECURE_HTTP_ENV] = env; + mockFetch.mockImplementation(async () => json()); + const client = new SapiomClient({ + apiKey: "sk_live", + baseURL: "http://api.internal", + ...options, + }); + await client.request({ url: "/x" }); + expect(sentHeaders(0)["x-api-key"]).toBe("sk_live"); + }, + ); + + it("setApiKey changes the key both clients send", async () => { + mockFetch.mockImplementation(async () => json()); + const client = new SapiomClient({ apiKey: "sk_old" }); + client.setApiKey("sk_new"); + await client.request({ url: "/x" }); + await client.vault.getAll("demo"); + expect(sentHeaders(0)["x-api-key"]).toBe("sk_new"); + expect(sentHeaders(1)["x-sapiom-api-key"]).toBe("sk_new"); + }); + + it("drops the key and x-sapiom-* headers on a redirect to another origin", async () => { + mockFetch + .mockResolvedValueOnce( + json(307, { location: "https://other.example/landed" }), + ) + .mockResolvedValueOnce(json()); + const client = new SapiomClient({ + apiKey: "sk_live", + headers: { "x-sapiom-trace": "t1" }, + }); + await client.request({ url: "/x" }); + expect(mockFetch.mock.calls.map((c) => c[0])).toEqual([ + "https://api.sapiom.ai/v1/x", + "https://other.example/landed", + ]); + expect(sentHeaders(0)["x-api-key"]).toBe("sk_live"); + expect(sentHeaders(1)).toEqual({ "Content-Type": "application/json" }); + }); + + it("keeps the key on a same-origin redirect", async () => { + mockFetch + .mockResolvedValueOnce(json(307, { location: "/v2/x" })) + .mockResolvedValueOnce(json()); + const client = new SapiomClient({ apiKey: "sk_live" }); + await client.request({ url: "/x" }); + expect(mockFetch.mock.calls[1][0]).toBe("https://api.sapiom.ai/v2/x"); + expect(sentHeaders(1)["x-api-key"]).toBe("sk_live"); + }); + + it("rejects a redirect to plaintext after one call, without retrying", async () => { + mockFetch.mockResolvedValue( + json(302, { location: "http://evil.internal/x" }), + ); + const client = new SapiomClient({ apiKey: "sk_live", retry: RETRY }); + await expect(client.request({ url: "/x" })).rejects.toBeInstanceOf( + CredentialChannelError, + ); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it("still retries a failed connection", async () => { + mockFetch + .mockRejectedValueOnce(new TypeError("fetch failed")) + .mockResolvedValueOnce(json()); + const client = new SapiomClient({ apiKey: "sk_live", retry: RETRY }); + await expect(client.request({ url: "/x" })).resolves.toEqual({ ok: true }); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/core/src/client/HttpClient.ts b/packages/core/src/client/HttpClient.ts index c1176a621..fd969f5cd 100644 --- a/packages/core/src/client/HttpClient.ts +++ b/packages/core/src/client/HttpClient.ts @@ -1,5 +1,13 @@ import { randomUUID } from "node:crypto"; +import { + CredentialChannelError, + credentialFetch, + resolveCredentialPolicy, + type Credential, + type CredentialPolicy, +} from "@sapiom/analytics-core/credential"; + /** * Request configuration for the HTTP client */ @@ -33,6 +41,10 @@ export interface HttpClientConfig { headers: Record; retry?: RetryConfig; versionPrefix?: string | null; + /** The tenant key. Sent by `credentialFetch`, never through `headers`. */ + credential?: Credential; + /** Plain http to a non-loopback host may carry the credential. */ + allowInsecureHttp?: boolean; } /** @@ -45,9 +57,16 @@ export class HttpClient { private headers: Record; private readonly retry: RetryConfig; private readonly versionPrefix: string | null; + private credential: Credential | undefined; + private readonly policy: CredentialPolicy; constructor(config: HttpClientConfig) { this.baseURL = config.baseURL; + this.credential = config.credential; + this.policy = resolveCredentialPolicy(config.allowInsecureHttp, { + label: "@sapiom/core", + optIn: "new SapiomClient({ allowInsecureHttp: true })", + }); this.timeout = config.timeout; this.headers = { ...config.headers }; const retry = config.retry ?? DEFAULT_RETRY; @@ -68,6 +87,13 @@ export class HttpClient { this.headers[key] = value; } + /** + * Replace the tenant key + */ + setCredential(credential: Credential): void { + this.credential = credential; + } + /** * Get client defaults (for testing and compatibility) */ @@ -75,6 +101,7 @@ export class HttpClient { baseURL: string; timeout: number; headers: Record; + credential: Credential | undefined; retry: RetryConfig; versionPrefix: string | null; } { @@ -82,6 +109,7 @@ export class HttpClient { baseURL: this.baseURL, timeout: this.timeout, headers: this.headers, + credential: this.credential, retry: this.retry, versionPrefix: this.versionPrefix, }; @@ -134,6 +162,9 @@ export class HttpClient { private isRetryable(error: unknown): boolean { if (!(error instanceof Error)) return false; + // A refused channel, first URL or redirect hop: it would be refused again + if (error instanceof CredentialChannelError) return false; + // Timeouts are NOT safe to retry (server may have processed) if (error.message.includes("Request timeout")) return false; @@ -161,12 +192,18 @@ export class HttpClient { const timeoutId = setTimeout(() => controller.abort(), this.timeout); try { - const response = await fetch(url, { - method: config.method || "GET", - headers, - body: this.prepareRequestBody(config.body), - signal: controller.signal, - }); + const response = await credentialFetch( + fetch, + url, + { + method: config.method || "GET", + headers, + body: this.prepareRequestBody(config.body), + signal: controller.signal, + }, + this.credential, + this.policy, + ); clearTimeout(timeoutId); diff --git a/packages/core/src/client/SapiomClient.test.ts b/packages/core/src/client/SapiomClient.test.ts index e44cd34cf..45e223cd4 100644 --- a/packages/core/src/client/SapiomClient.test.ts +++ b/packages/core/src/client/SapiomClient.test.ts @@ -52,7 +52,10 @@ describe("SapiomClient", () => { client.setApiKey("new-api-key"); const httpClient = client.getHttpClient(); - expect(httpClient.defaults.headers["x-api-key"]).toBe("new-api-key"); + expect(httpClient.defaults.credential).toEqual({ + header: "x-api-key", + value: "new-api-key", + }); }); }); diff --git a/packages/core/src/client/SapiomClient.ts b/packages/core/src/client/SapiomClient.ts index c4e2850fe..a5881c3a4 100644 --- a/packages/core/src/client/SapiomClient.ts +++ b/packages/core/src/client/SapiomClient.ts @@ -1,6 +1,7 @@ import { ApiKeyAPI } from "./ApiKeyAPI.js"; import { HttpClient, HttpRequestConfig } from "./HttpClient.js"; import type { RetryConfig } from "./HttpClient.js"; +import type { Credential } from "@sapiom/analytics-core/credential"; import { IdentityManager } from "./IdentityManager.js"; import { TransactionAPI } from "./TransactionAPI.js"; import { VaultAPI } from "./VaultAPI.js"; @@ -51,6 +52,14 @@ export interface SapiomClientConfig { * @see IdentityManagerOptions.backgroundRefresh */ backgroundRefresh?: boolean; + + /** + * Let the API key travel over plain http to a non-loopback host (a trusted + * private network). By default it only goes over https or to localhost, and + * `SAPIOM_ALLOW_INSECURE_HTTP=1` opts in too. + * @default false + */ + allowInsecureHttp?: boolean; } /** @@ -104,9 +113,10 @@ export class SapiomClient { timeout: config.timeout || 30000, headers: { "Content-Type": "application/json", - "x-api-key": config.apiKey, ...config.headers, }, + credential: { header: "x-api-key", value: config.apiKey }, + allowInsecureHttp: config.allowInsecureHttp, retry: config.retry, }); @@ -115,9 +125,10 @@ export class SapiomClient { timeout: config.timeout || 30000, headers: { "Content-Type": "application/json", - "x-sapiom-api-key": config.apiKey, ...config.headers, }, + credential: { header: "x-sapiom-api-key", value: config.apiKey }, + allowInsecureHttp: config.allowInsecureHttp, retry: config.retry, versionPrefix: null, }); @@ -135,8 +146,11 @@ export class SapiomClient { * Update the API key */ setApiKey(apiKey: string): void { - this.httpClient.setHeader("x-api-key", apiKey); - this.vaultHttpClient.setHeader("x-sapiom-api-key", apiKey); + this.httpClient.setCredential({ header: "x-api-key", value: apiKey }); + this.vaultHttpClient.setCredential({ + header: "x-sapiom-api-key", + value: apiKey, + }); } /** @@ -148,6 +162,7 @@ export class SapiomClient { baseURL: string; timeout: number; headers: Record; + credential: Credential | undefined; retry: RetryConfig; versionPrefix: string | null; }; diff --git a/packages/mcp/src/tools/app-links-api.test.ts b/packages/mcp/src/tools/app-links-api.test.ts new file mode 100644 index 000000000..f1a98b4d4 --- /dev/null +++ b/packages/mcp/src/tools/app-links-api.test.ts @@ -0,0 +1,106 @@ +import { CredentialChannelError } from "@sapiom/analytics-core/credential"; +import { + afterAll, + afterEach, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { appLinksFetch } from "./app-links-api.js"; + +const ENV = "SAPIOM_ALLOW_INSECURE_HTTP"; +const ORIGINAL_ENV = process.env[ENV]; +const ORIGINAL_FETCH = globalThis.fetch; + +function mockFetch(...responses: Response[]): ReturnType { + const fetchMock = vi.fn(); + for (const r of responses) fetchMock.mockResolvedValueOnce(r); + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; + return fetchMock; +} + +const sentKey = (fetchMock: ReturnType, call: number) => + (fetchMock.mock.calls[call][1] as { headers: Record }) + .headers["x-api-key"]; + +describe("appLinksFetch credential channel", () => { + beforeEach(() => { + delete process.env[ENV]; + }); + afterEach(() => { + globalThis.fetch = ORIGINAL_FETCH; + }); + afterAll(() => { + if (ORIGINAL_ENV === undefined) delete process.env[ENV]; + else process.env[ENV] = ORIGINAL_ENV; + }); + + it("refuses plaintext to a non-loopback host as a network failure, nothing sent", async () => { + const fetchMock = mockFetch(); + const result = await appLinksFetch( + "http://api.internal", + "sk_live", + "GET", + "/v1/app-links", + ); + expect(result).toMatchObject({ + kind: "network", + url: "http://api.internal/v1/app-links", + }); + const cause = (result as { cause: unknown }).cause; + expect(cause).toBeInstanceOf(CredentialChannelError); + expect(String(cause)).toMatch( + /@sapiom\/mcp: refusing plaintext HTTP.*opt in with SAPIOM_ALLOW_INSECURE_HTTP=1\.$/, + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it.each([ + ["the env opt-in", "http://api.internal", "1"], + ["a loopback host", "http://localhost:3000", undefined], + ])("sends over plain http with %s", async (_label, apiURL, env) => { + if (env) process.env[ENV] = env; + const fetchMock = mockFetch(new Response('{"items":[]}', { status: 200 })); + const result = await appLinksFetch( + apiURL, + "sk_live", + "GET", + "/v1/app-links", + ); + expect(result).toEqual({ + kind: "response", + status: 200, + ok: true, + data: { items: [] }, + }); + expect(sentKey(fetchMock, 0)).toBe("sk_live"); + }); + + it("drops x-api-key on a redirect to another origin", async () => { + const fetchMock = mockFetch( + new Response(null, { + status: 307, + headers: { location: "https://other.example/v1/app-links" }, + }), + new Response('{"items":[]}', { status: 200 }), + ); + await appLinksFetch( + "https://api.sapiom.ai", + "sk_live", + "POST", + "/v1/app-links", + { name: "x" }, + ); + expect(fetchMock.mock.calls.map((c) => c[0])).toEqual([ + "https://api.sapiom.ai/v1/app-links", + "https://other.example/v1/app-links", + ]); + expect([sentKey(fetchMock, 0), sentKey(fetchMock, 1)]).toEqual([ + "sk_live", + undefined, + ]); + }); +}); diff --git a/packages/mcp/src/tools/app-links-api.ts b/packages/mcp/src/tools/app-links-api.ts index 65b0a16e0..7bca7cde5 100644 --- a/packages/mcp/src/tools/app-links-api.ts +++ b/packages/mcp/src/tools/app-links-api.ts @@ -17,6 +17,10 @@ * `AgentOperationError`, while every App Links tool throws `PreviewOperationError` * (the sandbox-preview family these tools belong to). */ +import { + credentialFetch, + resolveCredentialPolicy, +} from "@sapiom/analytics-core/credential"; import { PreviewOperationError } from "@sapiom/sandbox-preview"; export type ToolResult = { @@ -80,6 +84,8 @@ export type AppLinksResponse = * The URL is concatenated, not `new URL(route, base)`: a custom `apiURL` carrying * a base path (a local proxy, a tunnel) would have it silently dropped by the * latter. A body-less method (`GET`, `DELETE`) sends no body and no content-type. + * A channel the key may not use (plain http to a non-loopback host, unless + * `SAPIOM_ALLOW_INSECURE_HTTP=1`) comes back as `network`, nothing sent. */ export async function appLinksFetch( apiURL: string, @@ -91,14 +97,19 @@ export async function appLinksFetch( const url = `${apiURL.replace(/\/+$/, "")}${route}`; let res: Response; try { - res = await fetch(url, { - method, - headers: { - "x-api-key": apiKey, - ...(body === undefined ? {} : { "content-type": "application/json" }), + res = await credentialFetch( + fetch, + url, + { + method, + headers: + body === undefined ? {} : { "content-type": "application/json" }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), }, - ...(body === undefined ? {} : { body: JSON.stringify(body) }), - }); + { header: "x-api-key", value: apiKey }, + // Per call: the env opt-in is read when the request is made. + resolveCredentialPolicy(undefined, { label: "@sapiom/mcp" }), + ); } catch (cause) { return { kind: "network", url, cause }; } diff --git a/packages/tools/src/_client/analytics.e2e.spec.ts b/packages/tools/src/_client/analytics.e2e.spec.ts index c3d7a136b..63498f097 100644 --- a/packages/tools/src/_client/analytics.e2e.spec.ts +++ b/packages/tools/src/_client/analytics.e2e.spec.ts @@ -485,7 +485,7 @@ describe("capability.call analytics (e2e, mock collector)", () => { const events = await flushedEvents(transport); expect(events).toHaveLength(1); expect(events[0]!.data.ok).toBe(false); - expect(events[0]!.data.error).toBe("TypeError"); + expect(events[0]!.data.error).toBe("CredentialChannelError"); }); it("never records query strings — a direct transport.fetch with a secret query stores a query-free url", async () => { diff --git a/packages/tools/src/_client/index.ts b/packages/tools/src/_client/index.ts index cfa7bb972..a48f5e24f 100644 --- a/packages/tools/src/_client/index.ts +++ b/packages/tools/src/_client/index.ts @@ -24,7 +24,7 @@ * * It is also where the credential's channel is enforced: https, or plain http * to loopback only, and never across a redirect to another origin. See - * `./credential-policy.ts`. + * `@sapiom/analytics-core/credential`. */ import { CAPABILITY_CALL_EVENT, @@ -33,11 +33,10 @@ import { type AnalyticsHolder, } from "./analytics.js"; import { - assertCredentialMayTravel, - fetchKeepingCredential, + credentialFetch, resolveCredentialPolicy, type CredentialPolicy, -} from "./credential-policy.js"; +} from "@sapiom/analytics-core/credential"; import { VERSION } from "../_generated/version.js"; import { TransportHttpError, @@ -53,6 +52,12 @@ import { */ const CLIENT_MARKER = `sapiom-tools/${VERSION}`; +/** How a refused channel names this package and its opt-in. */ +const POLICY_WORDING = { + label: "@sapiom/tools", + optIn: "createClient({ allowInsecureHttp: true })", +}; + /** * Per-request attribution recorded with the gateway transaction. Every field is * optional and maps 1:1 to an `x-sapiom-*` header the collapsed flow understands. @@ -236,7 +241,10 @@ export class Transport { config.resumeToken ?? process.env.SAPIOM_CAPABILITY_RESUME_TOKEN ?? undefined; - this.policy = resolveCredentialPolicy(config.allowInsecureHttp); + this.policy = resolveCredentialPolicy( + config.allowInsecureHttp, + POLICY_WORDING, + ); } /** @@ -298,7 +306,6 @@ export class Transport { ); } const headers: Record = { - [options.authHeader ?? DEFAULT_AUTH_HEADER]: this.apiKey, "x-sapiom-client": CLIENT_MARKER, ...attributionToHeaders(this.attribution), // Merged as a plain object, as before: callers pass records. @@ -309,12 +316,14 @@ export class Transport { try { // A malformed URL or a refused channel throws before anything is sent, and // is still counted as a failed call. - assertCredentialMayTravel(new URL(url), this.policy); - response = await fetchKeepingCredential( + response = await credentialFetch( this.fetchImpl, url, - init, - headers, + { ...init, headers }, + { + header: options.authHeader ?? DEFAULT_AUTH_HEADER, + value: this.apiKey, + }, this.policy, ); } catch (error) { diff --git a/packages/tools/src/_client/transport-security.spec.ts b/packages/tools/src/_client/transport-security.spec.ts index 902b5d3ae..672203738 100644 --- a/packages/tools/src/_client/transport-security.spec.ts +++ b/packages/tools/src/_client/transport-security.spec.ts @@ -2,7 +2,10 @@ import { createHash } from "node:crypto"; import { Transport, type TransportConfig } from "./index.js"; import { capabilityCall } from "./capability-call.js"; -import { ALLOW_INSECURE_HTTP_ENV, MAX_REDIRECTS } from "./credential-policy.js"; +import { + ALLOW_INSECURE_HTTP_ENV, + MAX_REDIRECTS, +} from "@sapiom/analytics-core/credential"; interface Hop { url: string; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index be82293df..f88624c61 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -333,6 +333,10 @@ importers: version: 5.9.3 packages/core: + dependencies: + '@sapiom/analytics-core': + specifier: workspace:^ + version: link:../analytics-core devDependencies: '@types/jest': specifier: ^29.5.14 @@ -1286,7 +1290,7 @@ packages: engines: {node: '>=12'} '@electron/node-gyp@https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2': - resolution: {gitHosted: true, integrity: sha512-MXgzlTDEEndJB3TBbvd5uFQO/8gaINo1Hfen8vef5rq/VHVPeB63uuv/uO5+8GFsAJ/rauu6XB79S6K4+aXc+w==, tarball: https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2} + resolution: {gitHosted: true, tarball: https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2} version: 10.2.0-electron.1 engines: {node: '>=12.13.0'} hasBin: true diff --git a/scripts/credential-header-allowlist.json b/scripts/credential-header-allowlist.json new file mode 100644 index 000000000..9b4c3034e --- /dev/null +++ b/scripts/credential-header-allowlist.json @@ -0,0 +1,22 @@ +[ + { + "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" + } +] diff --git a/scripts/credential-header-check.mjs b/scripts/credential-header-check.mjs new file mode 100644 index 000000000..c84e797cb --- /dev/null +++ b/scripts/credential-header-check.mjs @@ -0,0 +1,126 @@ +#!/usr/bin/env node +/** + * Fails when SDK source sets the tenant key header (`x-api-key` / + * `x-sapiom-api-key`) by hand instead of passing the key to `credentialFetch` + * from `@sapiom/analytics-core/credential`, the one place that checks the + * channel and drops the key on a cross-origin redirect (SAP-4106). + * + * It reads literals only: a header name held in a variable, or the key sent as + * `Authorization: Bearer`, passes. An allowlist entry exempts one file by exact + * path; an entry whose file no longer matches fails too, so the list only shrinks. + */ +import { readFile, readdir } from "node:fs/promises"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); +export const REPOSITORY_ROOT = path.resolve(SCRIPT_DIR, ".."); +const ALLOWLIST_PATH = path.join( + SCRIPT_DIR, + "credential-header-allowlist.json", +); + +/** The shared module itself: it is where the header is set. */ +const OWNER = "packages/analytics-core/src/credential.ts"; + +const HEADER = String.raw`["'\x60]x-(?:sapiom-)?api-key["'\x60]`; +const FORBIDDEN = [ + // { "x-api-key": key } + new RegExp(String.raw`${HEADER}\s*:`, "i"), + // headers["x-api-key"] = key + new RegExp(String.raw`\[\s*${HEADER}\s*\]\s*=(?!=)`, "i"), + // headers.set("x-api-key", key), req.setHeader(...), headers.append(...) + new RegExp(String.raw`\.(?:setHeader|set|append)\(\s*${HEADER}`, "i"), +]; + +const SCANNED = /\.(?:ts|tsx|mts|js|mjs)$/; +const TEST_FILE = /\.(?:test|spec)\.[cm]?[jt]sx?$/; +const SKIPPED_DIRS = new Set(["node_modules", "dist", "__tests__"]); + +/** 1-based line numbers where `text` sets a credential header by hand. */ +export function findCredentialHeaderSites(text) { + const lines = []; + text.split("\n").forEach((line, i) => { + if (FORBIDDEN.some((re) => re.test(line))) lines.push(i + 1); + }); + return lines; +} + +async function collect(root, dir, out) { + for (const entry of await readdir(path.join(root, dir), { + withFileTypes: true, + })) { + const child = path.posix.join(dir, entry.name); + if (entry.isDirectory()) { + if (!SKIPPED_DIRS.has(entry.name)) await collect(root, child, out); + } else if (SCANNED.test(entry.name) && !TEST_FILE.test(entry.name)) { + out.push(child); + } + } +} + +/** Every non-test source file under `packages/*\/src`. */ +export async function sourceFiles(root = REPOSITORY_ROOT) { + const files = []; + for (const pkg of await readdir(path.join(root, "packages"), { + withFileTypes: true, + })) { + if (!pkg.isDirectory()) continue; + const src = path.posix.join("packages", pkg.name, "src"); + try { + await collect(root, src, files); + } catch (error) { + if (error?.code !== "ENOENT") throw error; + } + } + return files.filter((file) => file !== OWNER).sort(); +} + +/** + * `sources`: `{ path, content }[]`. `allowlist`: `{ id, path, ticket }[]`. + * Returns the violations (`path:line`) and the stale allowlist entries. + */ +export function audit(sources, allowlist) { + const allowed = new Set(allowlist.map((entry) => entry.path)); + const matched = new Set(); + const violations = []; + for (const { path: file, content } of sources) { + const lines = findCredentialHeaderSites(content); + if (lines.length === 0) continue; + matched.add(file); + if (!allowed.has(file)) { + for (const line of lines) violations.push(`${file}:${line}`); + } + } + const stale = allowlist.filter((entry) => !matched.has(entry.path)); + return { violations, stale }; +} + +export async function auditRepository(root = REPOSITORY_ROOT) { + const allowlist = JSON.parse(await readFile(ALLOWLIST_PATH, "utf8")); + const sources = await Promise.all( + (await sourceFiles(root)).map(async (file) => ({ + path: file, + content: await readFile(path.join(root, file), "utf8"), + })), + ); + return audit(sources, allowlist); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const { violations, stale } = await auditRepository(); + for (const site of violations) { + console.error( + `${site}: sets the tenant key header by hand. Pass the key to ` + + `credentialFetch from @sapiom/analytics-core/credential instead.`, + ); + } + for (const entry of stale) { + console.error( + `scripts/credential-header-allowlist.json: "${entry.id}" (${entry.path}) ` + + `no longer sets the header; remove the entry.`, + ); + } + if (violations.length > 0 || stale.length > 0) process.exit(1); + console.log("credential-header:check passed"); +} diff --git a/scripts/credential-header-check.test.mjs b/scripts/credential-header-check.test.mjs new file mode 100644 index 000000000..c04f67beb --- /dev/null +++ b/scripts/credential-header-check.test.mjs @@ -0,0 +1,57 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; + +import { + audit, + auditRepository, + findCredentialHeaderSites, +} from "./credential-header-check.mjs"; + +describe("credential header guard", () => { + for (const [label, line] of [ + ["an object key", `fetch(url, { headers: { "x-api-key": key } });`], + ["a single-quoted object key", `const h = { 'X-Sapiom-Api-Key': k };`], + ["a bracket assignment", `h["x-sapiom-api-key"] = k;`], + ["setHeader", `client.setHeader("x-api-key", k);`], + ["Headers.set", `headers.set('x-api-key', k);`], + ["Headers.append", "headers.append(`x-sapiom-api-key`, k);"], + ]) { + it(`flags ${label}`, () => { + assert.deepEqual(findCredentialHeaderSites(`ok\n${line}`), [2]); + }); + } + + for (const [label, line] of [ + ["a header name as a value", `const opts = { authHeader: "x-api-key" };`], + ["a type union", `type H = "x-api-key" | "x-sapiom-api-key";`], + ["a Set entry", `const s = new Set(["x-api-key", "cookie"]);`], + ["a comparison", `if (h["x-api-key"] === k) return;`], + ["a credential passed by value", `{ header: "x-api-key", value: key }`], + ]) { + it(`does not flag ${label}`, () => { + assert.deepEqual(findCredentialHeaderSites(line), []); + }); + } + + it("exempts an allowlisted file and fails a stale entry", () => { + const sources = [ + { path: "packages/a/src/x.ts", content: `({ "x-api-key": k })` }, + { path: "packages/b/src/y.ts", content: `({ "x-api-key": k })` }, + { path: "packages/c/src/z.ts", content: `credentialFetch()` }, + ]; + const allowlist = [ + { id: "a", path: "packages/a/src/x.ts", ticket: "SAP-1" }, + { id: "c", path: "packages/c/src/z.ts", ticket: "SAP-1" }, + ]; + const { violations, stale } = audit(sources, allowlist); + assert.deepEqual(violations, ["packages/b/src/y.ts:1"]); + assert.deepEqual( + stale.map((entry) => entry.id), + ["c"], + ); + }); + + it("passes on the repository", async () => { + assert.deepEqual(await auditRepository(), { violations: [], stale: [] }); + }); +}); From e528cc8c3eed8f1d3220c356f8908ff607789d34 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:18:46 -0500 Subject: [PATCH 02/11] fix(sdk): normalize Headers in credentialFetch, match split header writes, fix comments [SAP-4106] credentialFetch read a Headers instance as empty and a tuple list by index. The credential-header check now matches across lines, so a setter whose arguments are wrapped no longer passes. Comments now name the harness allowlist exception instead of claiming every client is covered. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/test.yml | 4 +- .../agent-core/src/__tests__/client.test.ts | 2 - packages/agent-core/src/client.ts | 17 ++----- .../analytics-core/src/credential.spec.ts | 21 ++++++++ packages/analytics-core/src/credential.ts | 49 ++++++++----------- packages/core/src/client/HttpClient.ts | 8 +-- packages/core/src/client/SapiomClient.ts | 8 +-- packages/mcp/src/tools/app-links-api.ts | 3 +- packages/tools/src/_client/index.ts | 2 +- scripts/credential-header-check.mjs | 35 ++++++------- scripts/credential-header-check.test.mjs | 3 ++ 11 files changed, 72 insertions(+), 80 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e4b41699d..0be454b92 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -109,8 +109,8 @@ jobs: - name: Check Agent Studio visible terminology run: pnpm terminology:check - # Only credentialFetch (@sapiom/analytics-core/credential) may set the - # tenant key header; see scripts/credential-header-check.mjs. + # Keep literal tenant-key header writes in the shared owner, except for + # the exact-path exceptions in scripts/credential-header-allowlist.json. - name: Check tenant credential headers run: pnpm credential-header:check diff --git a/packages/agent-core/src/__tests__/client.test.ts b/packages/agent-core/src/__tests__/client.test.ts index c0f03f625..feadc28cc 100644 --- a/packages/agent-core/src/__tests__/client.test.ts +++ b/packages/agent-core/src/__tests__/client.test.ts @@ -238,8 +238,6 @@ describe('signal', () => { }); }); -// ── Credential channel ──────────────────────────────────────────────────────── - describe('GatewayClient credential channel', () => { const ENV = 'SAPIOM_ALLOW_INSECURE_HTTP'; const ORIGINAL = process.env[ENV]; diff --git a/packages/agent-core/src/client.ts b/packages/agent-core/src/client.ts index 8173e7101..f6d8eb581 100644 --- a/packages/agent-core/src/client.ts +++ b/packages/agent-core/src/client.ts @@ -1,9 +1,5 @@ -/** - * Configurable HTTP client for the Sapiom workflows backend API. All inputs are - * passed explicitly (base URL + API key), with no global state and no - * process.env read but the shared `SAPIOM_ALLOW_INSECURE_HTTP` opt-in, so the - * client is usable from a CLI, an MCP tool, or a test harness. - */ +/** Explicit credentials keep this client reusable by CLI, MCP, and test callers. + * The shared channel policy also reads SAPIOM_ALLOW_INSECURE_HTTP. */ import { credentialFetch, resolveCredentialPolicy, @@ -24,11 +20,8 @@ export interface ClientOptions { host?: string; /** API key sent as `x-api-key`. Must start with `sk_`. */ apiKey: string; - /** - * Let the API key travel over plain http to a non-loopback host (a trusted - * private network). By default it only goes over https or to localhost, and - * `SAPIOM_ALLOW_INSECURE_HTTP=1` opts in too. - */ + /** Permit plaintext credentials only for a trusted private network. + * Explicit configuration overrides SAPIOM_ALLOW_INSECURE_HTTP. */ allowInsecureHttp?: boolean; } @@ -128,7 +121,7 @@ export class GatewayClient { let res: Response; try { - // A refused channel lands in the same NETWORK error, its reason as the hint. + // Preserve the NETWORK boundary so callers can surface channel refusals through hint. res = await credentialFetch( fetch, url, diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 0025b40d2..36e4ab63b 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -305,6 +305,27 @@ describe("credentialFetch", () => { }); }); + it.each([ + ["a Headers instance", new Headers({ "content-type": "application/json" })], + [ + "a tuple list", + [["content-type", "application/json"]] as [string, string][], + ], + ])("reads caller headers given as %s", async (_label, headers) => { + const { calls, fetchImpl } = recorder(new Response("ok")); + await credentialFetch( + fetchImpl, + "https://api.example/x", + { headers }, + KEY, + STRICT, + ); + expect(calls[0].headers).toEqual({ + "content-type": "application/json", + "x-api-key": "sk_real", + }); + }); + it("refuses an untrusted channel before calling fetch", async () => { const { calls, fetchImpl } = recorder(); await expect( diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 2503bc557..3a58de74a 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -4,10 +4,8 @@ * `fetch` drops `authorization` on an origin change but keeps custom headers * such as `x-sapiom-api-key`. * - * Every SDK client puts the key on a request through {@link credentialFetch}, - * never by setting the header itself; `pnpm credential-header:check` enforces - * it in CI. The check reads literals only: a header name held in a variable, - * or the key sent as `Authorization: Bearer`, is for review to catch. + * Clients send the key through {@link credentialFetch} so they share one policy; + * `pnpm credential-header:check` guards literal header writes (harness: SAP-4112). */ import { createHash } from "node:crypto"; @@ -45,11 +43,8 @@ export function isLoopbackHostname(hostname: string): boolean { ); } -/** - * A refused channel. A TypeError like the ones `fetch` throws, so existing - * catches still apply, but one a retry loop can tell apart from a failed - * connection: retrying it cannot succeed. - */ +/** Extend TypeError for existing catches while distinguishing permanent channel refusals. + * Retrying with the same URL and policy cannot resolve the refusal. */ export class CredentialChannelError extends TypeError { constructor(message: string) { super(message); @@ -57,7 +52,7 @@ export class CredentialChannelError extends TypeError { } } -/** The non-throwing form of {@link assertCredentialMayTravel}. */ +/** Let analytics omit an unsafe credential without rejecting the initial batch request. */ export function mayCarryCredential( url: URL, policy: CredentialPolicy, @@ -69,11 +64,8 @@ export function mayCarryCredential( ); } -/** - * Throws a {@link CredentialChannelError}, before anything is sent, when `url` - * is not a channel the credential may use. It has no `cause`, unlike a failed - * connection. Only the origin is echoed: a path or query can carry a secret. - */ +/** Refuse unsafe credential channels before sending to them. + * Echo only origins because paths and queries may contain secrets. */ export function assertCredentialMayTravel( url: URL, policy: CredentialPolicy, @@ -215,11 +207,8 @@ async function assertIntegrity( ); } -/** - * Each hop is its own fetch, so none can report what `follow` would: set it on - * the final response and on every clone of it. An injected fetch may answer a - * bare `{ ok, status }` with no `clone`. - */ +/** Manual hops cannot preserve fetch's redirected flag, so mark final responses and clones. + * Injected minimal responses need not implement clone. */ function markRedirected(response: Response): Response { if (typeof response.clone !== "function") { return Object.defineProperty(response, "redirected", { value: true }); @@ -311,18 +300,13 @@ export async function fetchKeepingCredential( } } -/** The tenant key and the header it rides on. */ export interface Credential { readonly header: "x-api-key" | "x-sapiom-api-key"; readonly value: string; } -/** - * The one way an SDK client sends the tenant key: checks the channel, sets the - * header after `init.headers` (which can neither override nor forge it), and - * follows redirects through {@link fetchKeepingCredential}. Without a - * credential, nothing is checked on the first URL; redirect hops still are. - */ +/** 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( fetchImpl: typeof globalThis.fetch, url: string, @@ -330,8 +314,7 @@ export async function credentialFetch( credential: Credential | undefined, policy: CredentialPolicy, ): Promise { - // Merged as a plain object: callers pass records. - const callerHeaders = (init.headers ?? {}) as Record; + const callerHeaders = headerRecord(init.headers); if (!credential) { return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, policy); } @@ -340,3 +323,11 @@ export async function credentialFetch( headers[credential.header] = credential.value; return fetchKeepingCredential(fetchImpl, url, init, headers, policy); } + +// Object.entries reads a Headers instance as empty and a tuple list by index. +function headerRecord(headers: RequestInit["headers"]): Record { + if (headers instanceof Headers || Array.isArray(headers)) { + return Object.fromEntries(new Headers(headers)); + } + return { ...(headers as Record | undefined) }; +} diff --git a/packages/core/src/client/HttpClient.ts b/packages/core/src/client/HttpClient.ts index fd969f5cd..9a9e75e05 100644 --- a/packages/core/src/client/HttpClient.ts +++ b/packages/core/src/client/HttpClient.ts @@ -47,10 +47,7 @@ export interface HttpClientConfig { allowInsecureHttp?: boolean; } -/** - * Core HTTP client for making API requests. - * Uses native fetch for zero external dependencies. - */ +/** Share credential transport policy so API and vault requests enforce the same channel constraints. */ export class HttpClient { private readonly baseURL: string; private readonly timeout: number; @@ -87,9 +84,6 @@ export class HttpClient { this.headers[key] = value; } - /** - * Replace the tenant key - */ setCredential(credential: Credential): void { this.credential = credential; } diff --git a/packages/core/src/client/SapiomClient.ts b/packages/core/src/client/SapiomClient.ts index a5881c3a4..2eee0504e 100644 --- a/packages/core/src/client/SapiomClient.ts +++ b/packages/core/src/client/SapiomClient.ts @@ -53,12 +53,8 @@ export interface SapiomClientConfig { */ backgroundRefresh?: boolean; - /** - * Let the API key travel over plain http to a non-loopback host (a trusted - * private network). By default it only goes over https or to localhost, and - * `SAPIOM_ALLOW_INSECURE_HTTP=1` opts in too. - * @default false - */ + /** Permit plaintext credentials only for a trusted private network. + * Explicit configuration overrides SAPIOM_ALLOW_INSECURE_HTTP. */ allowInsecureHttp?: boolean; } diff --git a/packages/mcp/src/tools/app-links-api.ts b/packages/mcp/src/tools/app-links-api.ts index 7bca7cde5..3d3ff4be7 100644 --- a/packages/mcp/src/tools/app-links-api.ts +++ b/packages/mcp/src/tools/app-links-api.ts @@ -84,8 +84,7 @@ export type AppLinksResponse = * The URL is concatenated, not `new URL(route, base)`: a custom `apiURL` carrying * a base path (a local proxy, a tunnel) would have it silently dropped by the * latter. A body-less method (`GET`, `DELETE`) sends no body and no content-type. - * A channel the key may not use (plain http to a non-loopback host, unless - * `SAPIOM_ALLOW_INSECURE_HTTP=1`) comes back as `network`, nothing sent. + * Refusals map to network; a refused redirect can follow an already-sent request. */ export async function appLinksFetch( apiURL: string, diff --git a/packages/tools/src/_client/index.ts b/packages/tools/src/_client/index.ts index a48f5e24f..0e2767587 100644 --- a/packages/tools/src/_client/index.ts +++ b/packages/tools/src/_client/index.ts @@ -52,7 +52,7 @@ import { */ const CLIENT_MARKER = `sapiom-tools/${VERSION}`; -/** How a refused channel names this package and its opt-in. */ +/** Shared-policy refusals must identify this package's public opt-in. */ const POLICY_WORDING = { label: "@sapiom/tools", optIn: "createClient({ allowInsecureHttp: true })", diff --git a/scripts/credential-header-check.mjs b/scripts/credential-header-check.mjs index c84e797cb..2e3194acb 100644 --- a/scripts/credential-header-check.mjs +++ b/scripts/credential-header-check.mjs @@ -7,7 +7,7 @@ * * It reads literals only: a header name held in a variable, or the key sent as * `Authorization: Bearer`, passes. An allowlist entry exempts one file by exact - * path; an entry whose file no longer matches fails too, so the list only shrinks. + * path, and an entry whose file no longer matches fails, so a fixed file leaves it. */ import { readFile, readdir } from "node:fs/promises"; import path from "node:path"; @@ -20,30 +20,30 @@ const ALLOWLIST_PATH = path.join( "credential-header-allowlist.json", ); -/** The shared module itself: it is where the header is set. */ +/** Exempt the shared owner because it must attach credentials after checking the channel. */ const OWNER = "packages/analytics-core/src/credential.ts"; const HEADER = String.raw`["'\x60]x-(?:sapiom-)?api-key["'\x60]`; const FORBIDDEN = [ - // { "x-api-key": key } - new RegExp(String.raw`${HEADER}\s*:`, "i"), - // headers["x-api-key"] = key - new RegExp(String.raw`\[\s*${HEADER}\s*\]\s*=(?!=)`, "i"), - // headers.set("x-api-key", key), req.setHeader(...), headers.append(...) - new RegExp(String.raw`\.(?:setHeader|set|append)\(\s*${HEADER}`, "i"), + new RegExp(String.raw`${HEADER}\s*:`, "gi"), + new RegExp(String.raw`\[\s*${HEADER}\s*\]\s*=(?!=)`, "gi"), + new RegExp(String.raw`\.(?:setHeader|set|append)\(\s*${HEADER}`, "gi"), ]; const SCANNED = /\.(?:ts|tsx|mts|js|mjs)$/; const TEST_FILE = /\.(?:test|spec)\.[cm]?[jt]sx?$/; const SKIPPED_DIRS = new Set(["node_modules", "dist", "__tests__"]); -/** 1-based line numbers where `text` sets a credential header by hand. */ +/** Preserve source line numbers so CI diagnostics point directly to matched header sites. */ export function findCredentialHeaderSites(text) { - const lines = []; - text.split("\n").forEach((line, i) => { - if (FORBIDDEN.some((re) => re.test(line))) lines.push(i + 1); - }); - return lines; + const lines = new Set(); + // Whole text, not line by line: a call's arguments may span lines. + for (const re of FORBIDDEN) { + for (const match of text.matchAll(re)) { + lines.add(text.slice(0, match.index).split("\n").length); + } + } + return [...lines].sort((a, b) => a - b); } async function collect(root, dir, out) { @@ -59,7 +59,7 @@ async function collect(root, dir, out) { } } -/** Every non-test source file under `packages/*\/src`. */ +/** Exclude the shared owner so the guard audits consumers of the credential policy. */ export async function sourceFiles(root = REPOSITORY_ROOT) { const files = []; for (const pkg of await readdir(path.join(root, "packages"), { @@ -76,10 +76,7 @@ export async function sourceFiles(root = REPOSITORY_ROOT) { return files.filter((file) => file !== OWNER).sort(); } -/** - * `sources`: `{ path, content }[]`. `allowlist`: `{ id, path, ticket }[]`. - * Returns the violations (`path:line`) and the stale allowlist entries. - */ +/** Reject unapproved header sites and obsolete exceptions so exemptions remain tied to matching files. */ export function audit(sources, allowlist) { const allowed = new Set(allowlist.map((entry) => entry.path)); const matched = new Set(); diff --git a/scripts/credential-header-check.test.mjs b/scripts/credential-header-check.test.mjs index c04f67beb..7b1aa635b 100644 --- a/scripts/credential-header-check.test.mjs +++ b/scripts/credential-header-check.test.mjs @@ -15,6 +15,9 @@ describe("credential header guard", () => { ["setHeader", `client.setHeader("x-api-key", k);`], ["Headers.set", `headers.set('x-api-key', k);`], ["Headers.append", "headers.append(`x-sapiom-api-key`, k);"], + ["a setter split over lines", `headers.set(\n "x-api-key",\n k,\n);`], + ["an assignment split over lines", `h[\n "x-api-key"\n] = k;`], + ["an object key before a wrapped value", `({ "x-api-key":\n key })`], ]) { it(`flags ${label}`, () => { assert.deepEqual(findCredentialHeaderSites(`ok\n${line}`), [2]); From 2f45c399c9d573de086d77d49850e43bf43eceb6 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:21:04 -0500 Subject: [PATCH 03/11] docs(sdk): shorten module comments on the credential path [SAP-4106] Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/analytics-core/src/credential.ts | 11 ++------- packages/mcp/src/tools/app-links-api.ts | 11 ++------- packages/tools/src/_client/index.ts | 30 ++--------------------- scripts/credential-header-check.mjs | 12 ++------- 4 files changed, 8 insertions(+), 56 deletions(-) diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 3a58de74a..b46e21867 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -1,12 +1,5 @@ -/** - * Where the tenant credential may travel: over https, or plain http to a - * loopback host. Redirects are followed here rather than by `fetch`, because - * `fetch` drops `authorization` on an origin change but keeps custom headers - * such as `x-sapiom-api-key`. - * - * Clients send the key through {@link credentialFetch} so they share one policy; - * `pnpm credential-header:check` guards literal header writes (harness: SAP-4112). - */ +/** Centralize channel and redirect checks so clients share one tenant-credential policy. + * Harness exceptions to the literal-header guard are tracked in SAP-4112. */ import { createHash } from "node:crypto"; export const ALLOW_INSECURE_HTTP_ENV = "SAPIOM_ALLOW_INSECURE_HTTP"; diff --git a/packages/mcp/src/tools/app-links-api.ts b/packages/mcp/src/tools/app-links-api.ts index 3d3ff4be7..92ba1c31a 100644 --- a/packages/mcp/src/tools/app-links-api.ts +++ b/packages/mcp/src/tools/app-links-api.ts @@ -77,15 +77,8 @@ export type AppLinksResponse = | { kind: "response"; status: number; ok: boolean; data: unknown } | { kind: "network"; url: string; cause: unknown }; -/** - * One JSON call against the backend's App Links REST API, authenticated with the - * cached `sapiom_authenticate` credential as `x-api-key`. - * - * The URL is concatenated, not `new URL(route, base)`: a custom `apiURL` carrying - * a base path (a local proxy, a tunnel) would have it silently dropped by the - * latter. A body-less method (`GET`, `DELETE`) sends no body and no content-type. - * Refusals map to network; a refused redirect can follow an already-sent request. - */ +/** Concatenate routes so a configured proxy base path is preserved. + * Refusals map to network; a refused redirect can follow an already-sent request. */ export async function appLinksFetch( apiURL: string, apiKey: string, diff --git a/packages/tools/src/_client/index.ts b/packages/tools/src/_client/index.ts index 0e2767587..7aee6fe5e 100644 --- a/packages/tools/src/_client/index.ts +++ b/packages/tools/src/_client/index.ts @@ -1,31 +1,5 @@ -/** - * Auth + transport core for `@sapiom/tools` — the private "runtime half" every - * capability namespace wraps. Capability modules call THIS; they never import each - * other's runtime, so the cross-capability mesh (`repo.pushFromSandbox(sandbox)`) - * has no package/module cycles. See docs/plans/capability-authoring-sdk.md. - * - * The credential resolves from one of two sources, same surface either way: - * - explicit: createClient({ apiKey }) — standalone / open-source use - * - ambient: SAPIOM_API_KEY (env), which the workflow engine injects per - * execution so step authors write zero auth plumbing. - * - * Attribution (which agent / trace a call belongs to) is execution context the - * runtime owns, NOT a parameter of any operation — so it lives here on the - * transport, set once and injected on every request, never threaded through - * capability methods. The engine sets it per execution by constructing the - * per-execution client; `withAttribution(...)` is a quiet escape hatch for the - * router / standalone cases. Capability method signatures deliberately have no - * attribution argument: that absence keeps LLM-authored step code from setting - * (and getting wrong) context it doesn't own. - * - * Being the single HTTP choke point also makes this the (one) instrumentation - * seam: every call enqueues a `capability.call` usage event — synchronously, - * never awaited, live by default. See `./analytics.ts`. - * - * It is also where the credential's channel is enforced: https, or plain http - * to loopback only, and never across a redirect to another origin. See - * `@sapiom/analytics-core/credential`. - */ +/** Shared transport keeps credential handling consistent across capability modules. + * See @sapiom/analytics-core/credential for channel and redirect constraints. */ import { CAPABILITY_CALL_EVENT, analyticsFor, diff --git a/scripts/credential-header-check.mjs b/scripts/credential-header-check.mjs index 2e3194acb..0576b7710 100644 --- a/scripts/credential-header-check.mjs +++ b/scripts/credential-header-check.mjs @@ -1,14 +1,6 @@ #!/usr/bin/env node -/** - * Fails when SDK source sets the tenant key header (`x-api-key` / - * `x-sapiom-api-key`) by hand instead of passing the key to `credentialFetch` - * from `@sapiom/analytics-core/credential`, the one place that checks the - * channel and drops the key on a cross-origin redirect (SAP-4106). - * - * It reads literals only: a header name held in a variable, or the key sent as - * `Authorization: Bearer`, passes. An allowlist entry exempts one file by exact - * path, and an entry whose file no longer matches fails, so a fixed file leaves it. - */ +/** Enforce shared credential transport for literal header writes (SAP-4106). + * Exact-path exceptions must still match; see credential-header-allowlist.json. */ import { readFile, readdir } from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; From 8f394f6d20defc34e19879f0086f958c6f2d8c5e Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:25:43 -0500 Subject: [PATCH 04/11] fix(sdk): keep the TypeError name on a refused credential channel [SAP-4106] Recorded errors and logs keep naming a refused call TypeError, as tools reported before; callers tell it apart with instanceof. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/sdk-shared-credential-transport.md | 4 ++-- packages/analytics-core/src/credential.spec.ts | 2 +- packages/analytics-core/src/credential.ts | 9 ++------- packages/tools/src/_client/analytics.e2e.spec.ts | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/.changeset/sdk-shared-credential-transport.md b/.changeset/sdk-shared-credential-transport.md index 67f177d40..90bac7bc4 100644 --- a/.changeset/sdk-shared-credential-transport.md +++ b/.changeset/sdk-shared-credential-transport.md @@ -24,5 +24,5 @@ subpath, is the only code that sets the key header. `x-api-key`, `x-sapiom-api-key` and every `x-sapiom-*` header. - `@sapiom/core`: `getHttpClient().defaults.headers` no longer carries the key; read `defaults.credential` instead. -- `@sapiom/tools`: no behavior change; a refused call is now reported as a - `CredentialChannelError` (still a `TypeError`). +- `@sapiom/tools`: no behavior change. A refused call now throws a + `CredentialChannelError`, a `TypeError` subclass that keeps `name: "TypeError"`. diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 36e4ab63b..b8c6654d5 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -112,7 +112,7 @@ describe("assertCredentialMayTravel", () => { } expect(thrown).toBeInstanceOf(CredentialChannelError); expect(thrown).toBeInstanceOf(TypeError); - expect((thrown as Error).name).toBe("CredentialChannelError"); + expect((thrown as Error).name).toBe("TypeError"); expect((thrown as { cause?: unknown }).cause).toBeUndefined(); expect((thrown as Error).message).not.toMatch(/secret|token=abc/); }); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index b46e21867..28d774272 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -36,14 +36,9 @@ export function isLoopbackHostname(hostname: string): boolean { ); } -/** Extend TypeError for existing catches while distinguishing permanent channel refusals. +/** Extend TypeError, name included, so existing catches and recorded errors are unchanged. * Retrying with the same URL and policy cannot resolve the refusal. */ -export class CredentialChannelError extends TypeError { - constructor(message: string) { - super(message); - this.name = "CredentialChannelError"; - } -} +export class CredentialChannelError extends TypeError {} /** Let analytics omit an unsafe credential without rejecting the initial batch request. */ export function mayCarryCredential( diff --git a/packages/tools/src/_client/analytics.e2e.spec.ts b/packages/tools/src/_client/analytics.e2e.spec.ts index 63498f097..c3d7a136b 100644 --- a/packages/tools/src/_client/analytics.e2e.spec.ts +++ b/packages/tools/src/_client/analytics.e2e.spec.ts @@ -485,7 +485,7 @@ describe("capability.call analytics (e2e, mock collector)", () => { const events = await flushedEvents(transport); expect(events).toHaveLength(1); expect(events[0]!.data.ok).toBe(false); - expect(events[0]!.data.error).toBe("CredentialChannelError"); + expect(events[0]!.data.error).toBe("TypeError"); }); it("never records query strings — a direct transport.fetch with a secret query stores a query-free url", async () => { From 2ddfaec5e6295eff96ec20e1f5dfb800fd469422 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:29:25 -0500 Subject: [PATCH 05/11] chore: restore the electron node-gyp integrity hash in the lockfile [SAP-4106] Co-Authored-By: Claude Opus 5.5 (1M context) --- pnpm-lock.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f88624c61..4c240252b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1290,7 +1290,7 @@ packages: engines: {node: '>=12'} '@electron/node-gyp@https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2': - resolution: {gitHosted: true, tarball: https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2} + resolution: {gitHosted: true, integrity: sha512-MXgzlTDEEndJB3TBbvd5uFQO/8gaINo1Hfen8vef5rq/VHVPeB63uuv/uO5+8GFsAJ/rauu6XB79S6K4+aXc+w==, tarball: https://codeload.github.com/electron/node-gyp/tar.gz/06b29aafb7708acef8b3669835c8a7857ebc92d2} version: 10.2.0-electron.1 engines: {node: '>=12.13.0'} hasBin: true From 5ae9446ecbaa452b3cc7c146baf9509c6479afcb Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Fri, 16 Oct 2026 18:37:49 -0500 Subject: [PATCH 06/11] fix(analytics-core): strip SRI digest padding without a quadratic regex [SAP-4106] CodeQL flagged /=+$/ on integrity metadata: a long run of '=' made it quadratic. A linear scan trims the padding instead. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/analytics-core/src/credential.spec.ts | 1 + packages/analytics-core/src/credential.ts | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index b8c6654d5..64bb0cd18 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -273,6 +273,7 @@ describe("matchesIntegrity", () => { ], ["an empty digest for a known algorithm", "sha256-"], ["a malformed digest for a known algorithm", "sha256-!!!"], + ["a digest of only padding", `sha256-${"=".repeat(100_000)}x`], ])("rejects %s", (_label, metadata) => { expect(matchesIntegrity(BODY, metadata)).toBe(false); }); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 28d774272..07943e3e4 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -145,8 +145,12 @@ function isStreamBody(body: RequestInit["body"]): boolean { const SRI_ALGORITHMS = ["sha256", "sha384", "sha512"]; /** base64url and a missing `=` pad compare equal, as in Node's `fetch`. */ -const normalizeDigest = (digest: string): string => - digest.replace(/-/g, "+").replace(/_/g, "/").replace(/=+$/, ""); +function normalizeDigest(digest: string): string { + // A scan, not `/=+$/`: that regex is quadratic on a long run of `=` in the metadata. + let end = digest.length; + while (end > 0 && digest.charCodeAt(end - 1) === 61 /* '=' */) end -= 1; + return digest.slice(0, end).replace(/-/g, "+").replace(/_/g, "/"); +} /** * Subresource Integrity, as `fetch` applies `integrity`: unknown algorithms and From d8ac7d97243c7749497521ac1434f2c7a99240bc Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:07:18 -0500 Subject: [PATCH 07/11] fix(analytics-core): follow plain http redirects when no key is sent [SAP-4106] Without a credential there is nothing to protect, so a keyless request (an anonymous analytics batch) no longer has a same-origin http redirect refused and dropped; a non-http(s) hop is still refused. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/__tests__/http-sender-channel.test.ts | 13 +++++++ .../analytics-core/src/credential.spec.ts | 34 +++++++++++++++++++ packages/analytics-core/src/credential.ts | 4 ++- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/analytics-core/src/__tests__/http-sender-channel.test.ts b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts index 1ae2f13ce..d505061f9 100644 --- a/packages/analytics-core/src/__tests__/http-sender-channel.test.ts +++ b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts @@ -122,6 +122,19 @@ describe("HttpSender credential channel", () => { expect(String(debug[0][1])).toContain("http://evil.internal"); }); + it("delivers an anonymous batch through a same-origin redirect on plain http", async () => { + const { sender, calls } = senderWith("http://collector.internal/c", [ + redirectTo("/c/", 308), + { ok: true, status: 202 }, + ]); + await expect(sender.send([])).resolves.toBe("ok"); + expect(calls.map((c) => c.url)).toEqual([ + "http://collector.internal/c", + "http://collector.internal/c/", + ]); + expect(calls.map(keyOf)).toEqual([undefined, undefined]); + }); + it("treats a redirect status with no headers as the final response", async () => { const { sender, calls } = senderWith("https://collector.example/c", [ { ok: false, status: 307 }, diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 64bb0cd18..48156a37d 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -349,6 +349,40 @@ describe("credentialFetch", () => { ]); }); + it("without a credential, follows a plain http redirect but still refuses another scheme", async () => { + const { calls, fetchImpl } = recorder( + new Response(null, { status: 308, headers: { location: "/c/" } }), + new Response("ok"), + ); + await credentialFetch( + fetchImpl, + "http://collector.internal/c", + {}, + undefined, + STRICT, + ); + expect(calls.map((c) => c.url)).toEqual([ + "http://collector.internal/c", + "http://collector.internal/c/", + ]); + + const refused = recorder( + new Response(null, { + status: 307, + headers: { location: "file:///etc/passwd" }, + }), + ); + await expect( + credentialFetch( + refused.fetchImpl, + "http://collector.internal/c", + {}, + undefined, + STRICT, + ), + ).rejects.toBeInstanceOf(CredentialChannelError); + }); + it("returns a minimal redirect with no headers as is, after one call", async () => { const minimal = { ok: false, status: 307 }; const { calls, fetchImpl } = recorder(minimal); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 07943e3e4..f6a634914 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -308,7 +308,9 @@ export async function credentialFetch( ): Promise { const callerHeaders = headerRecord(init.headers); if (!credential) { - return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, policy); + // No key to protect: plain http hops are fine, only the http/https scheme check remains. + const keyless = { ...policy, allowInsecureHttp: true }; + return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, keyless); } assertCredentialMayTravel(new URL(url), policy); const headers = pick(callerHeaders, (name) => name !== credential.header); From cbe7b79c66abe7d367ee833e6e4d3151e4ccb697 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Fri, 16 Oct 2026 20:07:44 -0500 Subject: [PATCH 08/11] docs(core, mcp): document the plain-http opt-in for the API key [SAP-4106] Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/core/README.md | 4 ++++ packages/mcp/README.md | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/packages/core/README.md b/packages/core/README.md index dfcd70a34..65855269d 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -138,6 +138,9 @@ new SapiomClient(config: SapiomClientConfig) - `servicesDomain?: string` - Optional: Services domain for gateway clients (default: services.sapiom.ai) - `timeout?: number` - Optional: Request timeout in ms (default: 30000) - `headers?: Record` - Optional: Additional headers +- `allowInsecureHttp?: boolean` - Optional: Let the API key travel over plain `http://` to a non-loopback host, for a trusted private network (default: false) + +The API key only travels over `https://`, or plain `http://` to a loopback host (`localhost`, `*.localhost`, `127.0.0.1`, `[::1]`). Any other `http://` `baseURL` or `servicesDomain` (a Docker service name, a LAN IP) makes every request fail with a `CredentialChannelError` before anything is sent. On a trusted private network, opt in with `allowInsecureHttp: true` or `SAPIOM_ALLOW_INSECURE_HTTP=1`. Redirects to another origin never carry the key. #### Methods @@ -225,6 +228,7 @@ All integrations automatically read from environment: - `SAPIOM_API_KEY` (required) - `SAPIOM_BASE_URL` or `SAPIOM_API_URL` (optional) - `SAPIOM_TIMEOUT` (optional, in milliseconds) +- `SAPIOM_ALLOW_INSECURE_HTTP=1` (optional) - let the API key travel over plain `http://` to a non-loopback host ## Error Handling diff --git a/packages/mcp/README.md b/packages/mcp/README.md index ba227f666..3cbf415d7 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -59,6 +59,11 @@ Both resolve from built-in presets, so no config file is required. A custom target can be defined in `~/.sapiom/credentials.json` (the server prints the expected shape if it encounters an unknown environment name). +The API key only travels over `https://`, or plain `http://` to a loopback host +(`localhost`, `*.localhost`, `127.0.0.1`, `[::1]`). A custom target on any other +`http://` host is refused before the request is sent. On a trusted private +network, set `SAPIOM_ALLOW_INSECURE_HTTP=1` in the server's `env`. + ## Authentication The first networked call (`link`, `deploy`, `run`, `inspect`, `signal`) needs a From 46996a5550f98c0fb04625ecad7e421272567d6e Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Fri, 16 Oct 2026 21:07:25 -0500 Subject: [PATCH 09/11] fix(analytics-core): never downgrade a keyless request from https to plain http [SAP-4106] Plain http hops are allowed only when the request already started on http; a request that started on https keeps the policy, so caller headers such as Authorization cannot follow a redirect onto plaintext. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../analytics-core/src/credential.spec.ts | 19 +++++++++++++++++++ packages/analytics-core/src/credential.ts | 16 +++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 48156a37d..b9cd2c651 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -383,6 +383,25 @@ describe("credentialFetch", () => { ).rejects.toBeInstanceOf(CredentialChannelError); }); + it("without a credential, refuses a redirect from https down to plain http", async () => { + const { calls, fetchImpl } = recorder( + new Response(null, { + status: 307, + headers: { location: "http://other.internal/x" }, + }), + ); + await expect( + credentialFetch( + fetchImpl, + "https://api.example/x", + { headers: { authorization: "Bearer caller" } }, + undefined, + STRICT, + ), + ).rejects.toBeInstanceOf(CredentialChannelError); + expect(calls).toHaveLength(1); + }); + it("returns a minimal redirect with no headers as is, after one call", async () => { const minimal = { ok: false, status: 307 }; const { calls, fetchImpl } = recorder(minimal); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index f6a634914..408c2d0c8 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -308,9 +308,19 @@ export async function credentialFetch( ): Promise { const callerHeaders = headerRecord(init.headers); if (!credential) { - // No key to protect: plain http hops are fine, only the http/https scheme check remains. - const keyless = { ...policy, allowInsecureHttp: true }; - return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, keyless); + // A request that started on plain http has no TLS to lose, so its http hops are allowed; + // one that started on https keeps the policy, so caller headers are never downgraded. + const hopPolicy = + new URL(url).protocol === "http:" + ? { ...policy, allowInsecureHttp: true } + : policy; + return fetchKeepingCredential( + fetchImpl, + url, + init, + callerHeaders, + hopPolicy, + ); } assertCredentialMayTravel(new URL(url), policy); const headers = pick(callerHeaders, (name) => name !== credential.header); From 035841b7cdba72bb5ec21211ca2f34bf7d753b10 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Sat, 17 Oct 2026 09:15:12 -0500 Subject: [PATCH 10/11] fix(analytics-core): refuse any TLS downgrade on a keyless redirect chain [SAP-4106] One rule replaces the start-scheme special case: without a credential, plain http hops are allowed, but a hop from https to http is refused wherever it happens in the chain (an https detour back to http, or an https start redirected to a loopback or opted-in http host). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../analytics-core/src/credential.spec.ts | 47 +++++++++++++++++++ packages/analytics-core/src/credential.ts | 20 +++++--- 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index b9cd2c651..7701614a9 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -402,6 +402,53 @@ describe("credentialFetch", () => { expect(calls).toHaveLength(1); }); + it("without a credential, refuses an https detour back to plain http", async () => { + const { calls, fetchImpl } = recorder( + new Response(null, { + status: 307, + headers: { location: "https://collector.internal/c" }, + }), + new Response(null, { + status: 307, + headers: { location: "http://collector.internal/c" }, + }), + ); + await expect( + credentialFetch( + fetchImpl, + "http://collector.internal/c", + { headers: { authorization: "Bearer caller" } }, + undefined, + STRICT, + ), + ).rejects.toThrow( + /refusing a redirect from https:\/\/collector\.internal down to plaintext/, + ); + expect(calls).toHaveLength(2); + }); + + it.each([ + ["a loopback host", "http://127.0.0.1:4318/c", STRICT], + ["an opted-in host", "http://collector.internal/c", OPTED_IN], + ])( + "without a credential, refuses a redirect from https down to %s", + async (_label, location, policy) => { + const { calls, fetchImpl } = recorder( + new Response(null, { status: 308, headers: { location } }), + ); + await expect( + credentialFetch( + fetchImpl, + "https://api.example/c", + {}, + undefined, + policy, + ), + ).rejects.toBeInstanceOf(CredentialChannelError); + expect(calls).toHaveLength(1); + }, + ); + it("returns a minimal redirect with no headers as is, after one call", async () => { const minimal = { ok: false, status: 307 }; const { calls, fetchImpl } = recorder(minimal); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 408c2d0c8..244ff2c6c 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -226,6 +226,7 @@ export async function fetchKeepingCredential( init: RequestInit, headers: Record, policy: CredentialPolicy, + { refuseDowngrade = false }: { refuseDowngrade?: boolean } = {}, ): Promise { if (init.redirect === "manual" || init.redirect === "error") { return fetchImpl(url, { ...init, headers }); @@ -259,6 +260,15 @@ export async function fetchKeepingCredential( const next = new URL(location, current); assertCredentialMayTravel(next, policy, current); + if ( + refuseDowngrade && + current.protocol === "https:" && + next.protocol === "http:" + ) { + throw new CredentialChannelError( + `${policy.label}: refusing a redirect from ${current.origin} down to plaintext ${next.origin}`, + ); + } if (redirects === MAX_REDIRECTS) { throw new TypeError( `${policy.label}: more than ${MAX_REDIRECTS} redirects from ${start.origin}`, @@ -308,18 +318,14 @@ export async function credentialFetch( ): Promise { const callerHeaders = headerRecord(init.headers); if (!credential) { - // A request that started on plain http has no TLS to lose, so its http hops are allowed; - // one that started on https keeps the policy, so caller headers are never downgraded. - const hopPolicy = - new URL(url).protocol === "http:" - ? { ...policy, allowInsecureHttp: true } - : policy; + // No key: plain http hops are fine, but a hop never drops TLS once a request reached https. return fetchKeepingCredential( fetchImpl, url, init, callerHeaders, - hopPolicy, + { ...policy, allowInsecureHttp: true }, + { refuseDowngrade: true }, ); } assertCredentialMayTravel(new URL(url), policy); From 7b8eaebbb6d28cf88b119d0ae51b482028b86067 Mon Sep 17 00:00:00 2001 From: antoine-berger <64917674+antoine-berger@users.noreply.github.com> Date: Sat, 17 Oct 2026 10:36:47 -0500 Subject: [PATCH 11/11] revert(analytics-core): one channel rule for every redirect hop, keyed or not [SAP-4106] Reverts 035841b7, 46996a55 and d8ac7d97. Exempting keyless requests from the hop check took three patches to make safe; every hop now uses the same rule as a keyed request. The cost: an anonymous analytics batch to a plain-http collector that redirects is dropped, which analytics, being best-effort, accepts. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/__tests__/http-sender-channel.test.ts | 13 --- .../analytics-core/src/credential.spec.ts | 100 ------------------ packages/analytics-core/src/credential.ts | 20 +--- 3 files changed, 1 insertion(+), 132 deletions(-) diff --git a/packages/analytics-core/src/__tests__/http-sender-channel.test.ts b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts index d505061f9..1ae2f13ce 100644 --- a/packages/analytics-core/src/__tests__/http-sender-channel.test.ts +++ b/packages/analytics-core/src/__tests__/http-sender-channel.test.ts @@ -122,19 +122,6 @@ describe("HttpSender credential channel", () => { expect(String(debug[0][1])).toContain("http://evil.internal"); }); - it("delivers an anonymous batch through a same-origin redirect on plain http", async () => { - const { sender, calls } = senderWith("http://collector.internal/c", [ - redirectTo("/c/", 308), - { ok: true, status: 202 }, - ]); - await expect(sender.send([])).resolves.toBe("ok"); - expect(calls.map((c) => c.url)).toEqual([ - "http://collector.internal/c", - "http://collector.internal/c/", - ]); - expect(calls.map(keyOf)).toEqual([undefined, undefined]); - }); - it("treats a redirect status with no headers as the final response", async () => { const { sender, calls } = senderWith("https://collector.example/c", [ { ok: false, status: 307 }, diff --git a/packages/analytics-core/src/credential.spec.ts b/packages/analytics-core/src/credential.spec.ts index 7701614a9..64bb0cd18 100644 --- a/packages/analytics-core/src/credential.spec.ts +++ b/packages/analytics-core/src/credential.spec.ts @@ -349,106 +349,6 @@ describe("credentialFetch", () => { ]); }); - it("without a credential, follows a plain http redirect but still refuses another scheme", async () => { - const { calls, fetchImpl } = recorder( - new Response(null, { status: 308, headers: { location: "/c/" } }), - new Response("ok"), - ); - await credentialFetch( - fetchImpl, - "http://collector.internal/c", - {}, - undefined, - STRICT, - ); - expect(calls.map((c) => c.url)).toEqual([ - "http://collector.internal/c", - "http://collector.internal/c/", - ]); - - const refused = recorder( - new Response(null, { - status: 307, - headers: { location: "file:///etc/passwd" }, - }), - ); - await expect( - credentialFetch( - refused.fetchImpl, - "http://collector.internal/c", - {}, - undefined, - STRICT, - ), - ).rejects.toBeInstanceOf(CredentialChannelError); - }); - - it("without a credential, refuses a redirect from https down to plain http", async () => { - const { calls, fetchImpl } = recorder( - new Response(null, { - status: 307, - headers: { location: "http://other.internal/x" }, - }), - ); - await expect( - credentialFetch( - fetchImpl, - "https://api.example/x", - { headers: { authorization: "Bearer caller" } }, - undefined, - STRICT, - ), - ).rejects.toBeInstanceOf(CredentialChannelError); - expect(calls).toHaveLength(1); - }); - - it("without a credential, refuses an https detour back to plain http", async () => { - const { calls, fetchImpl } = recorder( - new Response(null, { - status: 307, - headers: { location: "https://collector.internal/c" }, - }), - new Response(null, { - status: 307, - headers: { location: "http://collector.internal/c" }, - }), - ); - await expect( - credentialFetch( - fetchImpl, - "http://collector.internal/c", - { headers: { authorization: "Bearer caller" } }, - undefined, - STRICT, - ), - ).rejects.toThrow( - /refusing a redirect from https:\/\/collector\.internal down to plaintext/, - ); - expect(calls).toHaveLength(2); - }); - - it.each([ - ["a loopback host", "http://127.0.0.1:4318/c", STRICT], - ["an opted-in host", "http://collector.internal/c", OPTED_IN], - ])( - "without a credential, refuses a redirect from https down to %s", - async (_label, location, policy) => { - const { calls, fetchImpl } = recorder( - new Response(null, { status: 308, headers: { location } }), - ); - await expect( - credentialFetch( - fetchImpl, - "https://api.example/c", - {}, - undefined, - policy, - ), - ).rejects.toBeInstanceOf(CredentialChannelError); - expect(calls).toHaveLength(1); - }, - ); - it("returns a minimal redirect with no headers as is, after one call", async () => { const minimal = { ok: false, status: 307 }; const { calls, fetchImpl } = recorder(minimal); diff --git a/packages/analytics-core/src/credential.ts b/packages/analytics-core/src/credential.ts index 244ff2c6c..07943e3e4 100644 --- a/packages/analytics-core/src/credential.ts +++ b/packages/analytics-core/src/credential.ts @@ -226,7 +226,6 @@ export async function fetchKeepingCredential( init: RequestInit, headers: Record, policy: CredentialPolicy, - { refuseDowngrade = false }: { refuseDowngrade?: boolean } = {}, ): Promise { if (init.redirect === "manual" || init.redirect === "error") { return fetchImpl(url, { ...init, headers }); @@ -260,15 +259,6 @@ export async function fetchKeepingCredential( const next = new URL(location, current); assertCredentialMayTravel(next, policy, current); - if ( - refuseDowngrade && - current.protocol === "https:" && - next.protocol === "http:" - ) { - throw new CredentialChannelError( - `${policy.label}: refusing a redirect from ${current.origin} down to plaintext ${next.origin}`, - ); - } if (redirects === MAX_REDIRECTS) { throw new TypeError( `${policy.label}: more than ${MAX_REDIRECTS} redirects from ${start.origin}`, @@ -318,15 +308,7 @@ export async function credentialFetch( ): Promise { const callerHeaders = headerRecord(init.headers); if (!credential) { - // No key: plain http hops are fine, but a hop never drops TLS once a request reached https. - return fetchKeepingCredential( - fetchImpl, - url, - init, - callerHeaders, - { ...policy, allowInsecureHttp: true }, - { refuseDowngrade: true }, - ); + return fetchKeepingCredential(fetchImpl, url, init, callerHeaders, policy); } assertCredentialMayTravel(new URL(url), policy); const headers = pick(callerHeaders, (name) => name !== credential.header);