diff --git a/package-lock.json b/package-lock.json index 5aba6445..bcdd6df7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -557,42 +557,40 @@ } }, "node_modules/@chevrotain/cst-dts-gen": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/@chevrotain/cst-dts-gen/-/cst-dts-gen-11.2.0.tgz", - "integrity": "sha512-ssJFvn/UXhQQeICw3SR/fZPmYVj+JM2mP+Lx7bZ51cOeHaMWOKp3AUMuyM3QR82aFFXTfcAp67P5GpPjGmbZWQ==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/@chevrotain/cst-dts-gen/-/cst-dts-gen-12.0.0.tgz", + "integrity": "sha512-fSL4KXjTl7cDgf0B5Rip9Q05BOrYvkJV/RrBTE/bKDN096E4hN/ySpcBK5B24T76dlQ2i32Zc3PAE27jFnFrKg==", "license": "Apache-2.0", "dependencies": { - "@chevrotain/gast": "11.2.0", - "@chevrotain/types": "11.2.0", - "lodash-es": "4.17.23" + "@chevrotain/gast": "12.0.0", + "@chevrotain/types": "12.0.0" } }, "node_modules/@chevrotain/gast": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/@chevrotain/gast/-/gast-11.2.0.tgz", - "integrity": "sha512-c+KoD6eSI1xjAZZoNUW+V0l13UEn+a4ShmUrjIKs1BeEWCji0Kwhmqn5FSx1K4BhWL7IQKlV7wLR4r8lLArORQ==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/@chevrotain/gast/-/gast-12.0.0.tgz", + "integrity": "sha512-1ne/m3XsIT8aEdrvT33so0GUC+wkctpUPK6zU9IlOyJLUbR0rg4G7ZiApiJbggpgPir9ERy3FRjT6T7lpgetnQ==", "license": "Apache-2.0", "dependencies": { - "@chevrotain/types": "11.2.0", - "lodash-es": "4.17.23" + "@chevrotain/types": "12.0.0" } }, "node_modules/@chevrotain/regexp-to-ast": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/@chevrotain/regexp-to-ast/-/regexp-to-ast-11.2.0.tgz", - "integrity": "sha512-lG73pBFqbXODTbXhdZwv0oyUaI+3Irm+uOv5/W79lI3g5hasYaJnVJOm3H2NkhA0Ef4XLBU4Scr7TJDJwgFkAw==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/@chevrotain/regexp-to-ast/-/regexp-to-ast-12.0.0.tgz", + "integrity": "sha512-p+EW9MaJwgaHguhoqwOtx/FwuGr+DnNn857sXWOi/mClXIkPGl3rn7hGNWvo31HA3vyeQxjqe+H36yZJwYU8cA==", "license": "Apache-2.0" }, "node_modules/@chevrotain/types": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/@chevrotain/types/-/types-11.2.0.tgz", - "integrity": "sha512-vBMSj/lz/LqolbGQEHB0tlpW5BnljHVtp+kzjQfQU+5BtGMTuZCPVgaAjtKvQYXnHb/8i/02Kii00y0tsuwfsw==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/@chevrotain/types/-/types-12.0.0.tgz", + "integrity": "sha512-S+04vjFQKeuYw0/eW3U52LkAHQsB1ASxsPGsLPUyQgrZ2iNNibQrsidruDzjEX2JYfespXMG0eZmXlhA6z7nWA==", "license": "Apache-2.0" }, "node_modules/@chevrotain/utils": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/@chevrotain/utils/-/utils-11.2.0.tgz", - "integrity": "sha512-+7whECg4yNWHottjvr2To2BRxL4XJVjIyyv5J4+bJ0iMOVU8j/8n1qPDLZS/90W/BObDR8VNL46lFbzY/Hosmw==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/@chevrotain/utils/-/utils-12.0.0.tgz", + "integrity": "sha512-lB59uJoaGIfOOL9knQqQRfhl9g7x8/wqFkp13zTdkRu1huG9kg6IJs1O8hqj9rs6h7orGxHJUKb+mX3rPbWGhA==", "license": "Apache-2.0" }, "node_modules/@colors/colors": { @@ -4288,17 +4286,19 @@ } }, "node_modules/chevrotain": { - "version": "11.2.0", - "resolved": "https://registry.npmjs.org/chevrotain/-/chevrotain-11.2.0.tgz", - "integrity": "sha512-mHCHTxM51nCklUw9RzRVc0DLjAh/SAUPM4k/zMInlTIo25ldWXOZoPt7XEIk/LwoT4lFVmJcu9g5MHtx371x3A==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/chevrotain/-/chevrotain-12.0.0.tgz", + "integrity": "sha512-csJvb+6kEiQaqo1woTdSAuOWdN0WTLIydkKrBnS+V5gZz0oqBrp4kQ35519QgK6TpBThiG3V1vNSHlIkv4AglQ==", "license": "Apache-2.0", "dependencies": { - "@chevrotain/cst-dts-gen": "11.2.0", - "@chevrotain/gast": "11.2.0", - "@chevrotain/regexp-to-ast": "11.2.0", - "@chevrotain/types": "11.2.0", - "@chevrotain/utils": "11.2.0", - "lodash-es": "4.17.23" + "@chevrotain/cst-dts-gen": "12.0.0", + "@chevrotain/gast": "12.0.0", + "@chevrotain/regexp-to-ast": "12.0.0", + "@chevrotain/types": "12.0.0", + "@chevrotain/utils": "12.0.0" + }, + "engines": { + "node": ">=22.0.0" } }, "node_modules/chownr": { @@ -6943,12 +6943,12 @@ "license": "MIT" }, "node_modules/js-toml": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/js-toml/-/js-toml-1.0.3.tgz", - "integrity": "sha512-sgyRKshBUSPIlUrbVXYQHReVZUXKHTldaW+Fj7KSan21vgnmMpuAAo00rBvm7W4HQrvZSvv186wNHlIjMPYC/A==", + "version": "1.1.3", + "resolved": "https://registry.npmjs.org/js-toml/-/js-toml-1.1.3.tgz", + "integrity": "sha512-AhsL7IPShAsXLIhe2/RwDoN6Jed4jsPsD8Bwkd6XZZFFWhIWmgTBXVx/RMEJ77KwXkRrsl7NbgS9nIJU7d4T1A==", "license": "MIT", "dependencies": { - "chevrotain": "^11.1.1", + "chevrotain": "^12.0.0", "xregexp": "^5.1.2" } }, @@ -7346,12 +7346,6 @@ "integrity": "sha512-dMInicTPVE8d1e5otfwmmjlxkZoUpiVLwyeTdUsi/Caj/gfzzblBcCE5sRHV/AsjuCmxWrte2TNGSYuCeCq+0Q==", "license": "MIT" }, - "node_modules/lodash-es": { - "version": "4.18.1", - "resolved": "https://registry.npmjs.org/lodash-es/-/lodash-es-4.18.1.tgz", - "integrity": "sha512-J8xewKD/Gk22OZbhpOVSwcs60zhd95ESDwezOFuA3/099925PdHJ7OFHNTGtajL3AlZkykD32HykiMo+BIBI8A==", - "license": "MIT" - }, "node_modules/lodash.camelcase": { "version": "4.3.0", "license": "MIT" diff --git a/src/router/worker-snapshots.ts b/src/router/worker-snapshots.ts index 0a316f9a..23cb6276 100644 --- a/src/router/worker-snapshots.ts +++ b/src/router/worker-snapshots.ts @@ -13,6 +13,156 @@ import { registerSnapshot } from './snapshot-manager.js'; const docker = new Docker(); +/** + * Env-var keys that must NEVER be baked into a committed snapshot image. + * + * `docker commit` preserves the container's `Config.Env` (every `-e` from the + * spawn) into the new image. Two problems if left unscrubbed: + * + * 1. **Correctness (ucho/MNG-1622 + MNG-1702).** A run that passes its payload + * INLINE bakes `JOB_DATA=` into the snapshot. A later run for the same + * work item whose payload is large is OFFLOADED (only `JOB_DATA_REDIS_KEY` + * is set, not `JOB_DATA`), and `docker run -e JOB_DATA_REDIS_KEY=...` does + * not clear the baked `JOB_DATA`. The worker then read the stale baked + * payload and ran the wrong (prior) agent. The primary fix is worker-side + * (`resolveRawJobData` prefers the Redis key); stripping job env here removes + * the stale artifact at the source too. + * 2. **Security.** The spawn env carries `DATABASE_URL`, `REDIS_URL`, the + * project's GitHub/Linear/OpenAI/etc. credentials, and the Claude OAuth + * token. Baking them means anyone with Docker/registry access to a + * `cascade-snapshot-*` image can read every project secret via + * `docker image inspect`. + * + * Static deny-set covers job + infra-secret keys; per-project credential names + * are dynamic and enumerated at runtime from `CASCADE_CREDENTIAL_KEYS`. + */ +const SNAPSHOT_ENV_DENYLIST: ReadonlySet = new Set([ + 'JOB_DATA', + 'JOB_DATA_REDIS_KEY', + 'JOB_ID', + 'JOB_TYPE', + 'DATABASE_URL', + 'DATABASE_SSL', + 'DATABASE_CA_CERT', + 'REDIS_URL', + 'CREDENTIAL_MASTER_KEY', + 'CASCADE_CREDENTIAL_KEYS', + 'CLAUDE_CODE_OAUTH_TOKEN', + 'CASCADE_POSTGRES_HOST', + 'CASCADE_POSTGRES_PORT', + 'CASCADE_SNAPSHOT_REUSE', + 'CASCADE_SNAPSHOT_ENABLED', +]); + +/** Parse the `KEY` out of a `KEY=VALUE` env line (split on the FIRST `=` only). */ +function envKey(line: string): string { + const eq = line.indexOf('='); + return eq === -1 ? line : line.slice(0, eq); +} + +/** + * Filter a container's `Config.Env` down to what is safe to bake into a snapshot + * image: drop the static deny-set plus every dynamic project-credential name + * listed in `CASCADE_CREDENTIAL_KEYS`. Everything else (PATH, NODE_*, LOG_LEVEL, + * SENTRY_*, PLAYWRIGHT_BROWSERS_PATH, CASCADE_DASHBOARD_URL, …) is PRESERVED so + * the snapshot still boots. Pure and total; splits on the first `=` so JSON / + * connection-string values containing `=` are handled. + */ +export function scrubSnapshotEnv(env: string[], extraCredentialKeys: string[] = []): string[] { + const deny = new Set(SNAPSHOT_ENV_DENYLIST); + for (const k of extraCredentialKeys) { + const trimmed = k.trim(); + if (trimmed) deny.add(trimmed); + } + return env.filter((line) => !deny.has(envKey(line))); +} + +/** Extract the dynamic project-credential key names from a container's env. */ +function extractCredentialKeys(env: string[]): string[] { + const line = env.find((e) => e.startsWith('CASCADE_CREDENTIAL_KEYS=')); + if (!line) return []; + return line + .slice('CASCADE_CREDENTIAL_KEYS='.length) + .split(',') + .map((s) => s.trim()) + .filter(Boolean); +} + +/** + * Compute the `changes` (Dockerfile `ENV KEY=` instructions) that BLANK the + * value of every deny-listed / credential env var actually present in `env`. + * + * WHY blank-via-changes and not a scrubbed `Env` body: `docker commit` cannot + * REMOVE an env var. The `POST /commit` body's `Env` list does not replace the + * container's env — moby re-appends every container env var whose key is absent + * from the body, so a "scrubbed Env body" is a **silent no-op** (verified + * against a live daemon: `JOB_DATA` / secrets survive unchanged, byte-identical + * to a bare commit). The supported mechanism is the `changes` param — Dockerfile + * instructions applied to the committed image — where `ENV KEY=` sets the value + * to empty. Empty `JOB_DATA` is falsy (the worker ignores it), and an emptied + * secret carries no value to leak. `Cmd` / `Entrypoint` / `WorkingDir` and every + * other env var are preserved by the daemon (a bare commit keeps the full + * container config; `changes` only overlays the named ENV keys). Only keys + * PRESENT in the env are blanked, so no spurious empty vars are introduced. + */ +export function buildSnapshotEnvScrubChanges(env: string[]): string[] { + const deny = new Set(SNAPSHOT_ENV_DENYLIST); + for (const k of extractCredentialKeys(env)) deny.add(k); + const present = new Set(); + for (const line of env) { + const key = envKey(line); + if (deny.has(key)) present.add(key); + } + return [...present].map((key) => `ENV ${key}=`); +} + +/** + * Commit `container` to `imageName` with its job + secret env vars blanked. + * + * Inspects the container's live `Config.Env`, then commits with `changes` that + * empty the value of every deny-listed / credential key present (see + * `buildSnapshotEnvScrubChanges` for why `changes` and not an `Env` body — the + * latter is a proven no-op). `Cmd`/`Entrypoint`/`WorkingDir`/all other env are + * preserved, so a reused snapshot still boots. + * + * If inspect fails or the env is empty, falls back to a bare commit (an + * unscrubbed but working snapshot) and captures Sentry under + * `snapshot_env_scrub_inspect_failed` so the regression to baking secrets is + * loud rather than silent. + */ +async function commitScrubbed( + container: Docker.Container, + repo: string, + imageName: string, +): Promise { + let env: string[] | undefined; + try { + const info = (await container.inspect()) as { Config?: { Env?: string[] } } | undefined; + env = info?.Config?.Env; + } catch (inspectErr) { + captureException(inspectErr, { + tags: { source: 'snapshot_env_scrub_inspect_failed' }, + extra: { imageName }, + level: 'warning', + }); + } + + if (Array.isArray(env) && env.length > 0) { + const changes = buildSnapshotEnvScrubChanges(env); + if (changes.length > 0) { + await container.commit({ repo, tag: 'latest', changes }); + logger.info('[WorkerManager] Snapshot committed with blanked job/secret env', { + imageName, + blankedKeys: changes.length, + }); + return; + } + } + + // inspect unavailable / empty env / nothing to blank → bare config-preserving commit. + await container.commit({ repo, tag: 'latest' }); +} + /** * Build a stable Docker image name for a snapshot. * Uses a sanitised project+workItem key so it's valid as a Docker image tag. @@ -56,7 +206,7 @@ export async function commitWorkerSnapshot( const imageName = buildWorkerSnapshotImageName(projectId, workItemId); try { const container = docker.getContainer(containerId); - await container.commit({ repo: imageName.split(':')[0], tag: 'latest' }); + await commitScrubbed(container, imageName.split(':')[0], imageName); const imageSize = await inspectImageSizeBestEffort(imageName); registerSnapshot(projectId, workItemId, imageName, imageSize); logger.info('[WorkerManager] Committed container to snapshot image:', { diff --git a/src/worker-entry.ts b/src/worker-entry.ts index 04973e57..830e6921 100644 --- a/src/worker-entry.ts +++ b/src/worker-entry.ts @@ -29,7 +29,7 @@ import { extractTrelloContext, generateAckMessage, } from './router/ackMessageGenerator.js'; -import { readOffloadedJobData } from './router/job-data-offload.js'; +import { buildJobDataRedisKey, readOffloadedJobData } from './router/job-data-offload.js'; import { dispatchPMAck } from './router/pm-ack-dispatch.js'; import { captureException, flush, setTag } from './sentry.js'; import { @@ -460,29 +460,56 @@ export async function dispatchJob( * with "argument list too long". Must run before scrubSensitiveEnv() strips * REDIS_URL. Exits the process with a clear, grep-able reason on any failure — * never the cryptic exec crash, never a payload-less worker. + * + * The Redis key is authoritative ONLY when it names THIS job's payload. The + * router sets EXACTLY ONE of JOB_DATA / JOB_DATA_REDIS_KEY per spawn + * (worker-env.ts is an if/else), and JOB_ID is set fresh on every spawn while the + * offload key embeds that jobId (`buildJobDataRedisKey`). But `docker commit` + * bakes the committed container's ENV into the snapshot image, and + * `docker run -e ...` does NOT clear a baked key that this run doesn't re-set. A + * reused snapshot can therefore carry a stale co-present channel from a PRIOR run + * of the same work item. Disambiguate both directions by matching the key to + * JOB_ID: + * + * - Forward case — this run is OFFLOADED (fresh JOB_DATA_REDIS_KEY) but the + * snapshot baked a stale `JOB_DATA=` from a prior INLINE run. The key + * matches JOB_ID → read Redis, ignoring the stale inline value. Reading inline + * first silently ran the wrong (prior) agent (prod incident ucho/MNG-1622 + + * MNG-1702: a 'splitting' run reused a 'planning' snapshot and re-ran planning, + * producing no story cards). + * - Reverse case — this run is INLINE (fresh JOB_DATA) but the snapshot baked a + * stale `JOB_DATA_REDIS_KEY=` from a prior OFFLOADED run. The key + * does NOT match JOB_ID → it is a stale baked artifact whose key the prior run + * already deleted from Redis; reading it would throw and crash the worker. + * Ignore it and fall through to this run's fresh inline JOB_DATA. */ async function resolveRawJobData(): Promise { - const inline = process.env.JOB_DATA; - if (inline) return inline; - const key = process.env.JOB_DATA_REDIS_KEY; - if (!key) { - // Defensive: main() validates that JOB_DATA or JOB_DATA_REDIS_KEY is present. - const err = new Error('JOB_DATA could not be resolved from env or Redis'); - console.error(`[Worker] ${err.message}`); - captureException(err, { tags: { source: 'worker_env' } }); - await flush(); - process.exit(1); + const jobId = process.env.JOB_ID; + // Trust the key only when it names THIS job's payload; a mismatched key is a + // stale artifact baked into a reused snapshot (see fn doc, reverse case). + if (key && jobId && key === buildJobDataRedisKey(jobId)) { + try { + return await readOffloadedJobData(key); + } catch (err) { + console.error('[Worker] Failed to read offloaded JOB_DATA from Redis:', err); + captureException(err, { tags: { source: 'worker_job_data_redis_read' } }); + await flush(); + process.exit(1); + } } - try { - return await readOffloadedJobData(key); - } catch (err) { - console.error('[Worker] Failed to read offloaded JOB_DATA from Redis:', err); - captureException(err, { tags: { source: 'worker_job_data_redis_read' } }); - await flush(); - process.exit(1); - } + const inline = process.env.JOB_DATA; + if (inline) return inline; + + // Defensive: main() validates that JOB_DATA or JOB_DATA_REDIS_KEY is present. + // Reaching here means the only channel set is a stale baked key (no matching + // JOB_ID) with no fresh inline fallback — the router set neither for this run. + const err = new Error('JOB_DATA could not be resolved from env or Redis'); + console.error(`[Worker] ${err.message}`); + captureException(err, { tags: { source: 'worker_env' } }); + await flush(); + process.exit(1); } export async function main(): Promise { diff --git a/tests/unit/router/worker-snapshots.test.ts b/tests/unit/router/worker-snapshots.test.ts index c6bc19aa..e8912ba5 100644 --- a/tests/unit/router/worker-snapshots.test.ts +++ b/tests/unit/router/worker-snapshots.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const { mockCaptureException, mockContainerCommit, + mockContainerInspect, mockContainerRemove, mockDockerGetContainer, mockDockerGetImage, @@ -14,6 +15,7 @@ const { } = vi.hoisted(() => ({ mockCaptureException: vi.fn(), mockContainerCommit: vi.fn(), + mockContainerInspect: vi.fn(), mockContainerRemove: vi.fn(), mockDockerGetContainer: vi.fn(), mockDockerGetImage: vi.fn(), @@ -49,11 +51,13 @@ vi.mock('../../../src/router/snapshot-manager.js', () => ({ })); import { + buildSnapshotEnvScrubChanges, buildWorkerSnapshotImageName, commitWorkerSnapshot, isImageNotFoundError, pullImageOnce, removeWorkerContainerBestEffort, + scrubSnapshotEnv, } from '../../../src/router/worker-snapshots.js'; describe('worker-snapshots', () => { @@ -62,8 +66,38 @@ describe('worker-snapshots', () => { mockContainerCommit.mockResolvedValue(undefined); mockContainerRemove.mockResolvedValue(undefined); mockImageInspect.mockResolvedValue({ Size: 1_234_567_890 }); + // Container inspect returns the live Config that docker commit would + // otherwise bake verbatim. Env is a mix of safe (PATH) + job + secret vars; + // the non-Env fields (Cmd/WorkingDir/User/Labels/ExposedPorts) are baked by + // Dockerfile.worker and MUST survive the scrubbed commit — a partial + // { Env } body would wipe them and break snapshot reuse. + mockContainerInspect.mockResolvedValue({ + Config: { + Cmd: ['node', '--import', './dist/instrument.js', 'dist/worker-entry.js'], + WorkingDir: '/app', + User: 'node', + Labels: { 'cascade.worker': 'true' }, + ExposedPorts: { '3000/tcp': {} }, + Env: [ + 'PATH=/usr/local/bin', + 'PLAYWRIGHT_BROWSERS_PATH=/ms-playwright', + 'JOB_DATA={"triggerResult":{"agentType":"planning"}}', + 'JOB_DATA_REDIS_KEY=cascade:jobdata:x', + 'JOB_ID=job-1', + 'JOB_TYPE=linear', + 'DATABASE_URL=postgres://secret', + 'REDIS_URL=redis://secret', + 'CLAUDE_CODE_OAUTH_TOKEN=sk-ant-oat01-x', + 'CASCADE_CREDENTIAL_KEYS=GITHUB_TOKEN_IMPLEMENTER,LINEAR_API_KEY', + 'GITHUB_TOKEN_IMPLEMENTER=ghp_x', + 'LINEAR_API_KEY=lin_y', + 'SENTRY_DSN=https://sentry', + ], + }, + }); mockDockerGetContainer.mockReturnValue({ commit: mockContainerCommit, + inspect: mockContainerInspect, remove: mockContainerRemove, }); mockDockerGetImage.mockReturnValue({ @@ -80,14 +114,48 @@ describe('worker-snapshots', () => { ); }); - it('commits the worker container, inspects image size, and registers metadata', async () => { + // Behavior (ucho/MNG-1622 + MNG-1702 + security): commit BLANKS job + secret env + // via `changes: ['ENV KEY=']`. A scrubbed `Env` body is a proven no-op — moby + // re-appends the container's env vars whose keys are absent from the body list, + // so `commit({_body:{Env:scrubbed}})` yields a byte-identical (unscrubbed) + // image (verified against a live daemon). `changes` is the supported mechanism; + // `ENV KEY=` empties the value while Cmd/Entrypoint/other env are preserved by + // the daemon. + it('commits with `changes` that blank job/secret env, inspects size, registers metadata', async () => { await commitWorkerSnapshot('container-snap-abc123', 'proj-snap', 'card-snap'); expect(mockDockerGetContainer).toHaveBeenCalledWith('container-snap-abc123'); - expect(mockContainerCommit).toHaveBeenCalledWith({ - repo: 'cascade-snapshot-proj-snap-card-snap', - tag: 'latest', - }); + expect(mockContainerInspect).toHaveBeenCalled(); + expect(mockContainerCommit).toHaveBeenCalledTimes(1); + const commitArg = mockContainerCommit.mock.calls[0][0] as { + repo: string; + tag: string; + changes: string[]; + }; + expect(commitArg.repo).toBe('cascade-snapshot-proj-snap-card-snap'); + expect(commitArg.tag).toBe('latest'); + // No _body / _query — a bare commit preserves the full container config + // (Cmd/WorkingDir/User/Labels), and `changes` overlays only the blanked keys. + expect(commitArg).not.toHaveProperty('_body'); + // Every job + secret key present in the container env is blanked… + for (const forbidden of [ + 'JOB_DATA', + 'JOB_DATA_REDIS_KEY', + 'JOB_ID', + 'JOB_TYPE', + 'DATABASE_URL', + 'REDIS_URL', + 'CLAUDE_CODE_OAUTH_TOKEN', + 'CASCADE_CREDENTIAL_KEYS', + 'GITHUB_TOKEN_IMPLEMENTER', + 'LINEAR_API_KEY', + ]) { + expect(commitArg.changes).toContain(`ENV ${forbidden}=`); + } + // …and safe vars are NEVER touched (no spurious empty PATH/SENTRY_DSN). + expect(commitArg.changes).not.toContain('ENV PATH='); + expect(commitArg.changes).not.toContain('ENV PLAYWRIGHT_BROWSERS_PATH='); + expect(commitArg.changes).not.toContain('ENV SENTRY_DSN='); expect(mockDockerGetImage).toHaveBeenCalledWith('cascade-snapshot-proj-snap-card-snap:latest'); expect(mockRegisterSnapshot).toHaveBeenCalledWith( 'proj-snap', @@ -97,6 +165,24 @@ describe('worker-snapshots', () => { ); }); + it('falls back to a bare commit and captures Sentry when container inspect fails', async () => { + mockContainerInspect.mockRejectedValueOnce(new Error('inspect boom')); + + await commitWorkerSnapshot('container-snap-abc123', 'proj-snap', 'card-snap'); + + // Bare commit preserves the prior behavior (working, if unscrubbed, snapshot)… + expect(mockContainerCommit).toHaveBeenCalledWith({ + repo: 'cascade-snapshot-proj-snap-card-snap', + tag: 'latest', + }); + // …but the scrub failure is loud, not silent. + expect(mockCaptureException).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ tags: { source: 'snapshot_env_scrub_inspect_failed' } }), + ); + expect(mockRegisterSnapshot).toHaveBeenCalled(); + }); + it('still registers snapshot metadata when image-size inspection fails', async () => { mockImageInspect.mockRejectedValueOnce(new Error('inspect failed')); @@ -166,6 +252,99 @@ describe('worker-snapshots', () => { }); }); +describe('scrubSnapshotEnv', () => { + it('strips per-job env (JOB_DATA and friends) but preserves safe vars', () => { + const out = scrubSnapshotEnv([ + 'PATH=/usr/bin', + 'JOB_DATA={"a":1}', + 'JOB_DATA_REDIS_KEY=cascade:jobdata:x', + 'JOB_ID=x', + 'JOB_TYPE=linear', + 'PLAYWRIGHT_BROWSERS_PATH=/ms-playwright', + ]); + expect(out).toEqual(['PATH=/usr/bin', 'PLAYWRIGHT_BROWSERS_PATH=/ms-playwright']); + }); + + it('strips infra secrets but keeps observability/config vars', () => { + const out = scrubSnapshotEnv([ + 'DATABASE_URL=postgres://s', + 'REDIS_URL=redis://s', + 'CREDENTIAL_MASTER_KEY=deadbeef', + 'CLAUDE_CODE_OAUTH_TOKEN=sk-ant-oat01-x', + 'SENTRY_DSN=https://sentry', + 'CASCADE_DASHBOARD_URL=https://dash', + 'LOG_LEVEL=info', + ]); + expect(out.map((e) => e.split('=')[0])).toEqual([ + 'SENTRY_DSN', + 'CASCADE_DASHBOARD_URL', + 'LOG_LEVEL', + ]); + }); + + it('strips dynamic project credentials named in extraCredentialKeys (and CASCADE_CREDENTIAL_KEYS itself)', () => { + const out = scrubSnapshotEnv( + [ + 'CASCADE_CREDENTIAL_KEYS=GITHUB_TOKEN_IMPLEMENTER,LINEAR_API_KEY', + 'GITHUB_TOKEN_IMPLEMENTER=ghp_x', + 'LINEAR_API_KEY=lin_y', + 'PATH=/bin', + ], + ['GITHUB_TOKEN_IMPLEMENTER', 'LINEAR_API_KEY'], + ); + expect(out).toEqual(['PATH=/bin']); + }); + + it('splits on the FIRST = so credential values containing = are stripped by key', () => { + const out = scrubSnapshotEnv( + ['CODEX_AUTH_JSON={"token":"a=b=c"}', 'PATH=/bin'], + ['CODEX_AUTH_JSON'], + ); + expect(out).toEqual(['PATH=/bin']); + }); + + it('handles env lines with no = (treats the whole string as the key)', () => { + expect(scrubSnapshotEnv(['JOB_DATA', 'BARE_FLAG', 'PATH=/bin'])).toEqual([ + 'BARE_FLAG', + 'PATH=/bin', + ]); + }); +}); + +describe('buildSnapshotEnvScrubChanges', () => { + it('emits `ENV KEY=` for each present deny/credential key, none for safe keys', () => { + const changes = buildSnapshotEnvScrubChanges([ + 'PATH=/bin', + 'JOB_DATA={"x":1}', + 'DATABASE_URL=postgres://s', + 'CASCADE_CREDENTIAL_KEYS=LINEAR_API_KEY', + 'LINEAR_API_KEY=lin_y', + 'SENTRY_DSN=https://sentry', + ]); + expect(changes.sort()).toEqual( + [ + 'ENV JOB_DATA=', + 'ENV DATABASE_URL=', + 'ENV CASCADE_CREDENTIAL_KEYS=', + 'ENV LINEAR_API_KEY=', + ].sort(), + ); + }); + + it('does NOT emit changes for deny keys that are absent (no spurious empty vars)', () => { + // Only PATH present → nothing to blank. + expect(buildSnapshotEnvScrubChanges(['PATH=/bin'])).toEqual([]); + }); + + it('blanks a credential value containing = (split on first =)', () => { + const changes = buildSnapshotEnvScrubChanges([ + 'CASCADE_CREDENTIAL_KEYS=CODEX_AUTH_JSON', + 'CODEX_AUTH_JSON={"t":"a=b=c"}', + ]); + expect(changes).toContain('ENV CODEX_AUTH_JSON='); + }); +}); + // Spec: pullImageOnce backs the spawn self-heal in container-manager.ts. // Single-flight + timeout are non-negotiable: without the in-flight cache, // every queued job under a missing-image outage races its own multi-GB pull. diff --git a/tests/unit/worker-entry.test.ts b/tests/unit/worker-entry.test.ts index 6ec73a4d..3232c24a 100644 --- a/tests/unit/worker-entry.test.ts +++ b/tests/unit/worker-entry.test.ts @@ -50,6 +50,9 @@ vi.mock('../../src/router/pm-ack-dispatch.js', () => ({ vi.mock('../../src/router/job-data-offload.js', () => ({ readOffloadedJobData: vi.fn(), + // Real key-building semantics so the JOB_ID↔key match check in + // resolveRawJobData behaves exactly as it does in production. + buildJobDataRedisKey: (jobId: string) => `cascade:jobdata:${jobId}`, })); vi.mock('../../src/router/ackMessageGenerator.js', () => ({ @@ -1072,9 +1075,11 @@ describe('main() - environment variable validation', () => { it('exits 1 with worker_job_data_redis_read tag when the offloaded key is missing/expired', async () => { process.env.JOB_ID = 'job-offload-missing'; process.env.JOB_TYPE = 'linear'; - process.env.JOB_DATA_REDIS_KEY = 'cascade:jobdata:gone'; + // A fresh offload key always matches JOB_ID (worker-env.ts builds it from + // job.id) — so it is trusted and read even when Redis has since dropped it. + process.env.JOB_DATA_REDIS_KEY = 'cascade:jobdata:job-offload-missing'; vi.mocked(readOffloadedJobData).mockRejectedValueOnce( - new Error('Offloaded JOB_DATA key cascade:jobdata:gone not found in Redis'), + new Error('Offloaded JOB_DATA key cascade:jobdata:job-offload-missing not found in Redis'), ); await expect(main()).rejects.toThrow('process.exit(1)'); @@ -1099,4 +1104,135 @@ describe('main() - environment variable validation', () => { expect.objectContaining({ tags: { source: 'worker_job_data_redis_read' } }), ); }); + + // ── stale-snapshot JOB_DATA regression (ucho/MNG-1622, MNG-1702) ──────────── + // + // The router sets EXACTLY ONE of JOB_DATA / JOB_DATA_REDIS_KEY per spawn + // (worker-env.ts is an if/else). But a snapshot IMAGE committed from a prior + // INLINE run bakes that prior run's `JOB_DATA=` into the image ENV + // (docker commit preserves container ENV). When a later run for the same work + // item is OFFLOADED (large payload → only JOB_DATA_REDIS_KEY is set) and spawns + // from that snapshot, `docker run -e JOB_DATA_REDIS_KEY=...` does NOT clear the + // baked JOB_DATA. So the worker sees BOTH. The offload key must win — it names + // THIS job's payload; the inline value is a stale artifact of a prior run. + // + // Live incident: a 'splitting' run reused a snapshot baked from an earlier + // 'planning' run of the same issue and silently re-ran planning (wrong agent, + // no story cards produced). + it('prefers the Redis-offloaded payload over a stale baked-in JOB_DATA when both are present', async () => { + process.env.JOB_ID = 'job-stale-snapshot'; + process.env.JOB_TYPE = 'linear'; + // Stale inline value baked into the snapshot image from a PRIOR inline run. + // This is the wrong (planning) agent's pre-resolved trigger result. + process.env.JOB_DATA = JSON.stringify({ + type: 'linear', + source: 'linear', + payload: { type: 'Issue', data: { id: 'lin-STALE' } }, + projectId: 'proj-1', + workItemId: 'lin-STALE', + eventType: 'update/Issue', + receivedAt: '2024-01-01T00:00:00Z', + triggerResult: { agentType: 'planning' }, + }); + // THIS job's real payload lives in Redis (it was offloaded because large). + // This is the correct (splitting) agent's pre-resolved trigger result. + process.env.JOB_DATA_REDIS_KEY = 'cascade:jobdata:job-stale-snapshot'; + vi.mocked(readOffloadedJobData).mockResolvedValueOnce( + JSON.stringify({ + type: 'linear', + source: 'linear', + payload: { type: 'Issue', data: { id: 'lin-FRESH' } }, + projectId: 'proj-1', + workItemId: 'lin-FRESH', + eventType: 'update/Issue', + receivedAt: '2024-06-25T00:00:00Z', + triggerResult: { agentType: 'splitting' }, + }), + ); + + await expect(main()).rejects.toThrow('process.exit('); + + // The offloaded (fresh) payload must be read and used… + expect(readOffloadedJobData).toHaveBeenCalledWith('cascade:jobdata:job-stale-snapshot'); + // …and the worker must dispatch with the FRESH payload, not the stale one. + expect(processLinearWebhook).toHaveBeenCalledWith( + { type: 'Issue', data: { id: 'lin-FRESH' } }, + expect.anything(), + undefined, + expect.objectContaining({ agentType: 'splitting' }), + 'proj-1', + ); + expect(flush).toHaveBeenCalled(); + }); + + it('still uses inline JOB_DATA when JOB_DATA_REDIS_KEY is absent (inline-only path unchanged)', async () => { + process.env.JOB_ID = 'job-inline-only'; + process.env.JOB_TYPE = 'linear'; + process.env.JOB_DATA = JSON.stringify({ + type: 'linear', + source: 'linear', + payload: { type: 'Issue', data: { id: 'lin-INLINE' } }, + projectId: 'proj-1', + workItemId: 'lin-INLINE', + eventType: 'update/Issue', + receivedAt: '2024-01-01T00:00:00Z', + triggerResult: { agentType: 'implementation' }, + }); + // No JOB_DATA_REDIS_KEY — pure inline path. + + await expect(main()).rejects.toThrow('process.exit('); + + expect(readOffloadedJobData).not.toHaveBeenCalled(); + expect(processLinearWebhook).toHaveBeenCalledWith( + { type: 'Issue', data: { id: 'lin-INLINE' } }, + expect.anything(), + undefined, + expect.objectContaining({ agentType: 'implementation' }), + 'proj-1', + ); + }); + + // ── reverse stale-snapshot case: baked JOB_DATA_REDIS_KEY + fresh inline ───── + // + // The symmetric hazard: a snapshot committed from a prior OFFLOADED run bakes a + // stale `JOB_DATA_REDIS_KEY=cascade:jobdata:` into the image ENV. + // When a later INLINE run for the same work item reuses that snapshot, + // `docker run -e JOB_DATA=...` does NOT clear the baked key, so the worker sees + // BOTH a fresh inline payload AND the stale key. The prior run already deleted + // that key from Redis, so trusting the key would throw → process.exit(1) on a + // run that used to succeed. The key must be ignored because it does not name + // THIS job (JOB_ID is fresh; the baked key embeds the prior jobId). + it('ignores a stale baked JOB_DATA_REDIS_KEY that does not match JOB_ID and uses fresh inline JOB_DATA', async () => { + process.env.JOB_ID = 'job-fresh-inline'; + process.env.JOB_TYPE = 'linear'; + // Fresh inline payload set by the router for THIS run. + process.env.JOB_DATA = JSON.stringify({ + type: 'linear', + source: 'linear', + payload: { type: 'Issue', data: { id: 'lin-FRESH-INLINE' } }, + projectId: 'proj-1', + workItemId: 'lin-FRESH-INLINE', + eventType: 'update/Issue', + receivedAt: '2024-06-25T00:00:00Z', + triggerResult: { agentType: 'implementation' }, + }); + // Stale key baked into the snapshot from a PRIOR offloaded run (different + // jobId). The prior run already del'd it from Redis, so reading it throws. + process.env.JOB_DATA_REDIS_KEY = 'cascade:jobdata:job-prior-offloaded'; + + await expect(main()).rejects.toThrow('process.exit('); + + // The stale key must NOT be read — it doesn't name THIS job's payload… + expect(readOffloadedJobData).not.toHaveBeenCalled(); + // …and the worker dispatches with the fresh inline payload instead of + // crashing on a key the prior run already deleted. + expect(processLinearWebhook).toHaveBeenCalledWith( + { type: 'Issue', data: { id: 'lin-FRESH-INLINE' } }, + expect.anything(), + undefined, + expect.objectContaining({ agentType: 'implementation' }), + 'proj-1', + ); + expect(flush).toHaveBeenCalled(); + }); });