From d8ddcc5af01866d2c631e27258f11cda05ecf985 Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Tue, 15 Sep 2026 03:54:28 +0000 Subject: [PATCH 1/5] security(pipelines): withhold operator-authored commands on the pipeline stage config carrier (PEN-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pipeline_stages.config` is the THIRD carrier of `issueExecutionWorkspaceSettingsSchema`, and the first that is not a column. `validators/pipeline.ts` declares `executionWorkspaceSettings` on both `pipelineStageOnEnterSchema` and `pipelineStageAutomationSchema`, so the same `workspaceStrategy` command strings and the same open `workspaceRuntime` record that PEN-3073 masks and PEN-3252 withholds cross under a parent key here. The PEN-3252 sweep could not see it: that sweep was scoped to the `execution_workspace_settings` COLUMN, which exists on exactly one table. Read paths are gated by `assertPipelineAccess`, which resolves the company and stops — company scope only, no `workspace_runtime:read` anywhere in the pipeline route or service. So an ordinary same-company agent received operator-authored shell commands. The exit set is wider than the originating ticket recorded. It enumerated the two `withDerivedStageAutomation` sites; auditing every response exit in the route found three more that answer with the `db.select()`ed row, config and all: - `GET /companies/:companyId/pipelines` — every stage of every pipeline in the company, and the widest of the five. - `POST /pipelines/:pipelineId/stages` - `PATCH /pipelines/:pipelineId/stages/:stageId` — which returns the STORED config untouched whenever the patch omits `config`. `onEnter` is also the primary carrier rather than the derived `automation` copy: `withDerivedStageAutomation` returns the raw config verbatim when a stage has no backing routine, so the derived block is not reliably present while `onEnter` always is. Masking only the derived copy would have masked the one that is sometimes absent and left the one that is always there. `publicPipelineStageConfig` reuses `publicIssueExecutionWorkspaceSettings` rather than re-deriving a second walk; a second implementation is how one exit ends up masked and another not. `onEnter`/`automation` are enumerated rather than the blob walked whole, because the rest of `config` is unrelated operator prose that has to survive intact. Shipping the read projection alone would have been a regression, not a partial fix. The pipeline editor round-trips this field — `PipelineSettings.tsx` seeds form state from the GET response and writes the whole automation block back on save — so an editor holding `pipelines:write` but not `workspace_runtime:read` would have saved the masked sentinel over the real command while editing something unrelated, like the stage name. That shape has reached production here before; `restoreRedactedAdapterValue` (`routes/agents.ts`) exists because "a sentinel written back into live config killed every run". So `restoreWithheldPipelineStageConfig` restores from the stored row on write. It is viewer-independent by design: it asks what the incoming bytes say, not who sent them, which keeps it correct even if the read projection and the write path ever disagree about entitlement. The write-path INTEGRITY question the ticket raises — an unentitled author planting a command another principal executes — is a different question and is deliberately not answered here. Refs: PEN-3266, PEN-3073, PEN-3252, PEN-2852, PEN-2370 Signed-off-by: Security Engineer --- server/src/redaction.ts | 68 +++++++++++++++++++++++++ server/src/routes/pipelines.ts | 49 +++++++++++++++--- server/src/routes/workspace-response.ts | 48 +++++++++++++++++ server/src/services/pipelines.ts | 14 +++-- 4 files changed, 168 insertions(+), 11 deletions(-) diff --git a/server/src/redaction.ts b/server/src/redaction.ts index a533fd8572c3..2727c8ecfb1d 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -930,6 +930,74 @@ export function maskWorkspaceRuntimeTextForRead(value: string | null): string | return value === null ? null : REDACTED_EVENT_VALUE; } +/** + * PEN-3266. The write-side counterpart to `publicPipelineStageConfig` + * (`routes/workspace-response.ts`), without which that read projection would be a + * regression rather than a partial fix. + * + * The pipeline editor round-trips this field: `ui/src/pages/PipelineSettings.tsx` + * seeds its form state from the GET response and writes the whole automation + * block back on save. So an editor holding `pipelines:write` but NOT + * `workspace_runtime:read` holds the masked sentinel where the operator-authored + * command is, and saving any unrelated field — the stage name — would persist + * that sentinel over the real command and destroy it. + * + * That shape has reached production here before: the `restoreRedactedAdapterValue` + * guard in `routes/agents.ts` exists because "a sentinel written back into live + * config killed every run". This is the same guard for the stage-config carrier. + * + * The check is viewer-INDEPENDENT by design — it asks what the incoming bytes + * say, not who sent them — because an entitled caller has no cause to send the + * sentinel and no legitimate configured value contains it. That keeps it correct + * even if the read projection and the write path ever disagree about entitlement. + * + * Substring rather than equality, matching the precedent: a masked value can sit + * inside a longer string, and an equality test would miss it and persist a broken + * value. `onEnter` and `automation` are enumerated rather than walked whole + * because the rest of `config` is unrelated operator prose that must round-trip + * byte-for-byte. + */ +export function restoreWithheldPipelineStageConfig(incoming: unknown, existing: unknown): unknown { + if (!isPlainObject(incoming)) return incoming; + const existingRecord = isPlainObject(existing) ? existing : {}; + + const restored: Record = { ...incoming }; + for (const key of ["onEnter", "automation"]) { + const block = restored[key]; + if (!isPlainObject(block)) continue; + if (!("executionWorkspaceSettings" in block)) continue; + const existingBlock = isPlainObject(existingRecord[key]) + ? (existingRecord[key] as Record) + : {}; + restored[key] = { + ...block, + executionWorkspaceSettings: restoreWithheldValue( + block.executionWorkspaceSettings, + existingBlock.executionWorkspaceSettings, + ), + }; + } + return restored; +} + +function restoreWithheldValue(incoming: unknown, existing: unknown): unknown { + if (typeof incoming === "string") { + return incoming.includes(REDACTED_EVENT_VALUE) ? existing : incoming; + } + if (Array.isArray(incoming)) { + const existingArray = Array.isArray(existing) ? existing : []; + return incoming.map((value, index) => restoreWithheldValue(value, existingArray[index])); + } + if (!isPlainObject(incoming)) return incoming; + + const existingRecord = isPlainObject(existing) ? existing : {}; + const restored: Record = {}; + for (const [key, value] of Object.entries(incoming)) { + restored[key] = restoreWithheldValue(value, existingRecord[key]); + } + return restored; +} + /** * Approval payloads are a human-facing escalation channel (BLO-20810), so a * field the scanner actually blanked must read differently from one the diff --git a/server/src/routes/pipelines.ts b/server/src/routes/pipelines.ts index 68d50b8aaf69..50ed10ca8fb4 100644 --- a/server/src/routes/pipelines.ts +++ b/server/src/routes/pipelines.ts @@ -69,6 +69,11 @@ import { type PipelineHealthFailedAutomationInput, type PipelineHealthStageInput, } from "@paperclipai/shared"; +import { + publicPipelineStageConfig, + resolveWorkspaceRuntimeViewer, + type WorkspaceRuntimeViewer, +} from "./workspace-response.js"; import { documentAnnotationService } from "../services/document-annotations.js"; import { logActivity } from "../services/activity-log.js"; import { @@ -272,16 +277,21 @@ function withDerivedStageAutomation( latestRevisionId: string | null; latestRevisionNumber: number; }>, + viewer: WorkspaceRuntimeViewer, ) { const config = stage.config && typeof stage.config === "object" && !Array.isArray(stage.config) ? { ...(stage.config as Record) } : {}; const routineId = stageAutomationRoutineId(config); const routine = routineId ? routineById.get(routineId) : null; - if (!routine) return { ...stage, config }; + // PEN-3266: the no-routine branch returns the STORED config, so it carries + // `onEnter.executionWorkspaceSettings` even though no derived `automation` block exists. It needs the + // projection just as much as the branch below — masking only the derived copy would leave the + // always-present carrier in the clear. + if (!routine) return { ...stage, config: publicPipelineStageConfig(config, viewer) }; return { ...stage, - config: { + config: publicPipelineStageConfig({ ...config, automation: { routineId, @@ -293,10 +303,19 @@ function withDerivedStageAutomation( latestRoutineRevisionId: routine.latestRevisionId, latestRoutineRevisionNumber: routine.latestRevisionNumber, }, - }, + }, viewer), }; } +/** + * PEN-3266. The stage row as it leaves any route that answers with the row itself rather than through + * `withDerivedStageAutomation` — the company pipeline LIST, and the create/update stage responses. + * Those return `db.select()`ed rows, so `config` reaches the caller with every key it was stored with. + */ +function publicPipelineStage(stage: T, viewer: WorkspaceRuntimeViewer): T { + return { ...stage, config: publicPipelineStageConfig(stage.config, viewer) }; +} + function extractIntakeFormFields(stage: typeof pipelineStages.$inferSelect | null) { const baseFields = [{ key: "title", label: "Name", type: "text", required: true, options: [] as string[] }]; const variables = stage?.config && typeof stage.config === "object" && !Array.isArray(stage.config) @@ -821,6 +840,7 @@ export function pipelineRoutes(db: Db, options: Parameters { const companyId = req.params.companyId as string; assertPipelineCompanyAccess(req, companyId); + const stageViewer = await resolveWorkspaceRuntimeViewer(access, req, companyId); const rows = await db .select({ pipeline: pipelines, @@ -857,7 +877,7 @@ export function pipelineRoutes(db: Db, options: Parameters ({ ...row.pipeline, stageCount: row.stageCount, - stages: stagesByPipelineId.get(row.pipeline.id) ?? [], + stages: (stagesByPipelineId.get(row.pipeline.id) ?? []).map((stage) => publicPipelineStage(stage, stageViewer)), openCaseCount: row.openCaseCount, attentionCount: row.attentionCount, inMotionCount: row.inMotionCount, @@ -970,6 +990,7 @@ export function pipelineRoutes(db: Db, options: Parameters { const pipelineId = req.params.pipelineId as string; const companyId = await assertPipelineAccess(db, req, pipelineId); + const stageViewer = await resolveWorkspaceRuntimeViewer(access, req, companyId); const [pipeline, stages, transitions, documentKeys] = await Promise.all([ db.select().from(pipelines).where(and(eq(pipelines.id, pipelineId), eq(pipelines.companyId, companyId))).then((rows) => rows[0] ?? null), db.select().from(pipelineStages).where(eq(pipelineStages.pipelineId, pipelineId)).orderBy(asc(pipelineStages.position)), @@ -1008,7 +1029,14 @@ export function pipelineRoutes(db: Db, options: Parameters withDerivedStageAutomation(stage, routineById)), transitions, documentKeys }); + res.json({ + ...pipeline, + stages: stages.map((stage) => + withDerivedStageAutomation(stage, routineById, stageViewer), + ), + transitions, + documentKeys, + }); }); // Setup-health warnings: surface any configuration that won't actually run @@ -1018,6 +1046,10 @@ export function pipelineRoutes(db: Db, options: Parameters { const pipelineId = req.params.pipelineId as string; const companyId = await assertPipelineAccess(db, req, pipelineId); + // PEN-3266: `computePipelineHealth` derives diagnostics and does not echo `config` back, so this + // projection is inert for the response today. It is threaded anyway so the health route cannot + // become the exit that was missed if that function ever starts quoting the config it is handed. + const stageViewer = await resolveWorkspaceRuntimeViewer(access, req, companyId); const [pipeline, stages, instructionDocs, companyAgents, companyPipelines, companyStages, failedAutomationRows] = await Promise.all([ db.select().from(pipelines) .where(and(eq(pipelines.id, pipelineId), eq(pipelines.companyId, companyId))) @@ -1127,7 +1159,7 @@ export function pipelineRoutes(db: Db, options: Parameters { - const stageWithAutomation = withDerivedStageAutomation(stage, routineById); + const stageWithAutomation = withDerivedStageAutomation(stage, routineById, stageViewer); const automation = (stageWithAutomation.config as { automation?: { instructionsBody?: string | null } }).automation; return { id: stage.id, @@ -1202,7 +1234,7 @@ export function pipelineRoutes(db: Db, options: Parameters = { ...config }; + for (const key of ["onEnter", "automation"]) { + const block = projected[key]; + if (!isPlainObject(block)) continue; + if (!("executionWorkspaceSettings" in block)) continue; + projected[key] = { + ...block, + executionWorkspaceSettings: publicIssueExecutionWorkspaceSettings(block.executionWorkspaceSettings, viewer), + }; + } + return projected; +} + +/** + * PEN-3266. The read projection above is only half of the fix; the write-side counterpart that keeps + * it from destroying the values it masks is `restoreWithheldPipelineStageConfig` in `../redaction.ts`. + * It lives there rather than here because `services/pipelines.ts` is what has to call it, and services + * do not import from the route layer. + */ export function publicExecutionWorkspace( workspace: ExecutionWorkspace, viewer: WorkspaceRuntimeViewer, diff --git a/server/src/services/pipelines.ts b/server/src/services/pipelines.ts index 892218ae1880..41e4f09724e5 100644 --- a/server/src/services/pipelines.ts +++ b/server/src/services/pipelines.ts @@ -2,6 +2,7 @@ import { randomUUID } from "node:crypto"; import { isDeepStrictEqual } from "node:util"; import { and, asc, desc, eq, inArray, isNotNull, isNull, ne, or, sql } from "drizzle-orm"; import { alias } from "drizzle-orm/pg-core"; +import { restoreWithheldPipelineStageConfig } from "../redaction.js"; import type { Db, DbTransaction } from "@paperclipai/db"; import { agentWakeupRequests, @@ -4355,13 +4356,20 @@ export function pipelineService( }) { await getPipelineOrThrow(db, input.companyId, input.pipelineId); const existing = await getStageOrThrow(db, input.pipelineId, input.stageId); + // PEN-3266: the pipeline editor round-trips the stage config, so a caller that read this stage + // WITHOUT `workspace_runtime:read` holds the masked sentinel where the operator-authored command + // is, and saving any unrelated field would otherwise persist that sentinel over the real command. + // Restore from the stored row before anything downstream reads the patch. + const patchConfig = input.patch.config === undefined + ? undefined + : restoreWithheldPipelineStageConfig(input.patch.config, stageConfig(existing)) as PipelineStageConfig; const kind = normalizeStageKind(input.patch.kind ?? existing.kind); const previousRoutineId = stageAutomationRoutineIdFromConfig(stageConfig(existing)); - const automationRequest = input.patch.config !== undefined - ? readStageAutomationRequest(input.patch.config) + const automationRequest = patchConfig !== undefined + ? readStageAutomationRequest(patchConfig) : null; const stageName = input.patch.name ?? existing.name; - let config = normalizeStageConfig(kind, input.patch.config !== undefined ? input.patch.config : stageConfig(existing)); + let config = normalizeStageConfig(kind, patchConfig !== undefined ? patchConfig : stageConfig(existing)); if (automationRequest) { config = reconcilePipelineStageConfigVariables(config, [ automationRequest.titleTemplate ?? PIPELINE_AUTOMATION_DEFAULT_TITLE_TEMPLATE, From 7ca388a8ad0d64436c1c8492f24cef184897d367 Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Tue, 15 Sep 2026 04:08:07 +0000 Subject: [PATCH 2/5] test(security): pin both halves of the pipeline stage config withholding (PEN-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit coverage for `publicPipelineStageConfig` and for `restoreWithheldPipelineStageConfig`, asserted separately because they fail for different reasons and a single fixture would let one carry the other. Mutation-checked in both directions, and the isolation matters: neutering the read projection alone leaves the write-guard cases receiving real commands, so they pass trivially and the run still looks meaningful. Verified independently — read projection neutered => the three masking cases fail; write guard neutered (mask intact) => the three restore cases fail. Covered: - Both `onEnter` and the derived `automation` copy are masked, with `onEnter` called out as the primary carrier since it is the one that is always present. - Neither sentinel appears anywhere in the serialized projection. - The unrelated parts of stage config — `variables`, `disabledReason`, and the `type`/`routineId`/`mode`/`environmentId` routing keys the editor needs — survive byte-for-byte, so the mask cannot be the blob-wide one. - An entitled viewer gets the config unchanged. - The write guard restores a masked command rather than persisting the sentinel, restores the nested `workspaceRuntime` record, does NOT resurrect a value the caller genuinely changed, and catches a sentinel embedded in a longer string rather than only an exact match. Fixture names avoid the key/token/secret/password/credential stems that `check-pr-security.mjs` flags on long literals: a security change whose own fixtures report as secrets costs a reviewer a triage every round. Refs: PEN-3266 Signed-off-by: Security Engineer --- ...age-workspace-settings-withholding.test.ts | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts diff --git a/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts new file mode 100644 index 000000000000..71833ec866bd --- /dev/null +++ b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, it } from "vitest"; +import { REDACTED_EVENT_VALUE, restoreWithheldPipelineStageConfig } from "../redaction.js"; +import { + WITHHELD_WORKSPACE_RUNTIME_VIEWER, + publicPipelineStageConfig, +} from "../routes/workspace-response.js"; + +/** + * PEN-3266 — unit coverage for the projection that closes the `pipeline_stages.config` carrier, and + * for the write-side guard that keeps that projection from destroying the values it masks. + * + * The carrier is the same `issueExecutionWorkspaceSettingsSchema` PEN-3252 withholds, declared on both + * `pipelineStageOnEnterSchema` and `pipelineStageAutomationSchema`. It crosses under a parent key in a + * `jsonb` blob rather than in a column of its own, which is why the PEN-3252 sweep — scoped to the + * `execution_workspace_settings` column — could not see it. + * + * Two properties are asserted here that a route fixture would not isolate cleanly: + * + * 1. BOTH keys are projected, and `onEnter` is the one that matters most. `withDerivedStageAutomation` + * returns the stored config verbatim when a stage has no backing routine, so the derived + * `automation` block is not reliably present while `onEnter` always is. A projection that covered + * only `automation` would mask the sometimes-absent copy and leak the always-present one. + * 2. The rest of `config` survives byte-for-byte. Stage config holds unrelated operator prose — + * `variables`, `disabledReason`, breakdown templates — so masking the blob wholesale would be + * wrong in the opposite direction. + * + * ⛔ Every value below is invented. No real credential, command or path is quoted, per the parent + * ticket's standing prohibition. Fixture names deliberately avoid the key/token/secret/password/ + * credential stems that `.github/scripts/check-pr-security.mjs` flags on 20+ character literals — a + * security change whose own fixtures report as secrets costs a reviewer a triage every round. + */ + +const COMMAND_SENTINEL = "sentinel-stage-provision-command-must-not-egress"; +const RUNTIME_SENTINEL = "sentinel-stage-runtime-must-not-egress"; +const ENTITLED_VIEWER = { revealRuntimeConfig: true }; +const ENVIRONMENT_ID = "6ba7b810-9dad-11d1-80b4-00c04fd430c8"; + +function settings() { + return { + mode: "isolated_workspace", + environmentId: ENVIRONMENT_ID, + workspaceStrategy: { + type: "git_worktree", + baseRef: "main", + provisionCommand: COMMAND_SENTINEL, + teardownCommand: `${COMMAND_SENTINEL}-teardown`, + worktreeParentDir: `${COMMAND_SENTINEL}-parent-dir`, + runScope: "per_issue", + }, + workspaceRuntime: { + services: [{ name: "web", command: RUNTIME_SENTINEL }], + }, + }; +} + +function stageConfig() { + return { + // Unrelated operator prose that must round-trip intact. + variables: [{ name: "customer", label: "Customer name" }], + disabledReason: "Paused while the intake form is rewritten", + onEnter: { type: "run_routine", routineId: "r-1", executionWorkspaceSettings: settings() }, + automation: { routineId: "r-1", executionWorkspaceSettings: settings() }, + }; +} + +function withheld(config: unknown) { + return publicPipelineStageConfig(config, WITHHELD_WORKSPACE_RUNTIME_VIEWER) as Record; +} + +describe("PEN-3266 pipeline stage config withholding", () => { + it("masks the operator-authored commands under onEnter", () => { + const strategy = withheld(stageConfig()).onEnter.executionWorkspaceSettings.workspaceStrategy; + expect(strategy.provisionCommand).toBe(REDACTED_EVENT_VALUE); + expect(strategy.teardownCommand).toBe(REDACTED_EVENT_VALUE); + expect(strategy.worktreeParentDir).toBe(REDACTED_EVENT_VALUE); + }); + + it("masks the derived automation copy of the same bytes", () => { + const strategy = withheld(stageConfig()).automation.executionWorkspaceSettings.workspaceStrategy; + expect(strategy.provisionCommand).toBe(REDACTED_EVENT_VALUE); + }); + + it("does not egress the workspaceRuntime record through either key", () => { + const projected = JSON.stringify(withheld(stageConfig())); + expect(projected).not.toContain(RUNTIME_SENTINEL); + expect(projected).not.toContain(COMMAND_SENTINEL); + }); + + it("leaves the non-workspace parts of the stage config byte-for-byte intact", () => { + const projected = withheld(stageConfig()); + expect(projected.variables).toEqual(stageConfig().variables); + expect(projected.disabledReason).toBe(stageConfig().disabledReason); + // The routing keys the editor needs are not workspace-runtime material. + expect(projected.onEnter.type).toBe("run_routine"); + expect(projected.onEnter.routineId).toBe("r-1"); + expect(projected.onEnter.executionWorkspaceSettings.mode).toBe("isolated_workspace"); + expect(projected.onEnter.executionWorkspaceSettings.environmentId).toBe(ENVIRONMENT_ID); + }); + + it("hands an entitled viewer the config unchanged", () => { + const config = stageConfig(); + expect(publicPipelineStageConfig(config, ENTITLED_VIEWER)).toEqual(config); + }); + + it("passes through a stage config that is absent or not an object", () => { + expect(withheld(null)).toBeNull(); + expect(withheld(undefined)).toBeUndefined(); + expect(publicPipelineStageConfig("not-an-object", WITHHELD_WORKSPACE_RUNTIME_VIEWER)).toBe("not-an-object"); + }); + + it("is inert on a stage config that carries neither key", () => { + const plain = { variables: [], requireApproval: true }; + expect(withheld(plain)).toEqual(plain); + }); +}); + +/** + * The half of the fix without which the half above would be a regression. + * + * The pipeline editor round-trips this field, so a caller that read the stage WITHOUT + * `workspace_runtime:read` holds the sentinel where the command is. Saving any unrelated field would + * otherwise persist the sentinel over the real command — a silent destructive write, not a disclosure. + */ +describe("PEN-3266 pipeline stage config write-back guard", () => { + const stored = stageConfig(); + + it("restores a masked command instead of persisting the sentinel", () => { + const roundTripped = withheld(stageConfig()); + roundTripped.disabledReason = "Edited something unrelated"; + + const restored = restoreWithheldPipelineStageConfig(roundTripped, stored) as Record; + expect(restored.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe(COMMAND_SENTINEL); + expect(restored.onEnter.executionWorkspaceSettings.workspaceStrategy.teardownCommand) + .toBe(`${COMMAND_SENTINEL}-teardown`); + expect(restored.automation.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe(COMMAND_SENTINEL); + // The genuine edit still lands. + expect(restored.disabledReason).toBe("Edited something unrelated"); + }); + + it("restores the nested workspaceRuntime record the mask walked", () => { + const roundTripped = withheld(stageConfig()); + const restored = restoreWithheldPipelineStageConfig(roundTripped, stored) as Record; + expect(restored.onEnter.executionWorkspaceSettings.workspaceRuntime) + .toEqual(stored.onEnter.executionWorkspaceSettings.workspaceRuntime); + }); + + it("does not resurrect a value the caller genuinely changed", () => { + const edited = withheld(stageConfig()); + edited.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand = "a-real-new-command"; + edited.onEnter.executionWorkspaceSettings.mode = "shared_workspace"; + + const restored = restoreWithheldPipelineStageConfig(edited, stored) as Record; + expect(restored.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe("a-real-new-command"); + expect(restored.onEnter.executionWorkspaceSettings.mode).toBe("shared_workspace"); + }); + + it("catches a sentinel embedded in a longer string, not only an exact match", () => { + const edited = withheld(stageConfig()); + edited.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand = + `https://user:${REDACTED_EVENT_VALUE}@example.invalid/provision`; + + const restored = restoreWithheldPipelineStageConfig(edited, stored) as Record; + expect(restored.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe(COMMAND_SENTINEL); + }); + + it("is inert when there is no stored config to restore from", () => { + const incoming = { onEnter: { type: "run_routine", routineId: "r-2" } }; + expect(restoreWithheldPipelineStageConfig(incoming, null)).toEqual(incoming); + }); + + it("passes through an incoming config that is not an object", () => { + expect(restoreWithheldPipelineStageConfig(null, stored)).toBeNull(); + expect(restoreWithheldPipelineStageConfig("x", stored)).toBe("x"); + }); +}); From 31a81f81361d3038d55b7fb29ecc8725403ed0ce Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Tue, 22 Sep 2026 21:23:42 +0000 Subject: [PATCH 3/5] security(pipelines): fix the write guard's restore target and its array alignment (PEN-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three findings from the consolidated review at head `7ca388a8`. **The `automation` branch restored from a key no stored row can carry.** `persistedStageConfig` (`services/pipelines.ts`) destructures `automation` out before every write, so `existingRecord["automation"]` was `undefined` on every real row. `restoreWithheldValue` then returned `undefined` for each sentinel-bearing string rather than the stored command — and `upsertStageAutomationRoutine` rebuilds `onEnter` from that stripped context (`onEnter: { type, routineId, ...input.executionContext }`), overwriting the copy the `onEnter` branch had just restored correctly. Net effect before this commit: an editor holding `pipelines:write` without `workspace_runtime:read` who saved a stage NAME destroyed `provisionCommand`, `teardownCommand`, `worktreeParentDir` and `workspaceRuntime`. That is the exact destructive write the guard was written to prevent, inside the guard itself. Both keys now restore from `existing.onEnter.executionWorkspaceSettings`. The two are not symmetric: `onEnter` is the only persisted copy of this carrier and `automation` is derived from it on read. **The fixture that hid it.** The write-back suite passed the full `stageConfig()` as the stored side, and that fixture carries an `automation` block no stored row can have. An impossible fixture cannot fail, which is why the stated mutation check missed the bug. `stored` is now built by stripping `automation`, with a guard test pinning that shape so it cannot creep back. **Array restore aligned by index.** `workspaceRuntime` holds arrays (`services`, and the `commands`/`jobs` siblings the read mask walks), every command masked for an unentitled reader. A read-modify-write that removed or reordered an element restored the sentinel at index *i* from the stored element at index *i*, silently writing one service's real command onto another. Restore now keys on `WORKSPACE_RUNTIME_IDENTITY_KEYS` — the mask's own set, so an identity key added there has to be honoured here in the same commit — and fails closed: no identity, or an identity matching zero or several stored elements, leaves the incoming value untouched rather than guessing a neighbour. Index alignment survives only for identity-less elements in an array that demonstrably did not change length. Related: a sentinel with nothing stored to restore now stays a sentinel instead of becoming `undefined`. Stripping the key is the destructive write; a literal sentinel is at least visibly wrong rather than silently gone. `null` is a real stored value ("no command configured") and still restores normally. Mutation check, run against the unmodified `redaction.ts` at `7ca388a8`: five assertions fail, each on the assertion it was written for — the pre-existing `automation` restore (now that its fixture is possible), the new restore-from-`onEnter` case, the strip-vs-sentinel case, and both array- alignment cases. All 18 pass with the fix. Also moves the cross-reference docstring above `publicExecutionWorkspace`, where it read as that function's own doc comment, up under `publicPipelineStageConfig` which it actually describes. Refs PEN-3266 Signed-off-by: Security Engineer --- ...age-workspace-settings-withholding.test.ts | 118 +++++++++++++++++- server/src/redaction.ts | 74 +++++++++-- server/src/routes/workspace-response.ts | 11 +- 3 files changed, 187 insertions(+), 16 deletions(-) diff --git a/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts index 71833ec866bd..c488dd378e1b 100644 --- a/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts +++ b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts @@ -63,6 +63,21 @@ function stageConfig() { }; } +/** + * What a stage row can ACTUALLY hold, and the distinction the first version of this suite missed. + * + * `persistedStageConfig` (`services/pipelines.ts`) destructures `automation` out before every write, + * so no stored config can carry one — `onEnter` is the only persisted copy of this carrier and + * `automation` is derived from it on read. Passing the full `stageConfig()` as the stored side was an + * impossible fixture, and it is precisely what made the `automation` restore assertions green while + * the production path stripped the command to `undefined` and then overwrote the real value with it. + * A stored fixture that cannot exist cannot fail, which is why the stated mutation check missed it. + */ +function storedStageConfig() { + const { automation: _automation, ...rest } = stageConfig(); + return rest; +} + function withheld(config: unknown) { return publicPipelineStageConfig(config, WITHHELD_WORKSPACE_RUNTIME_VIEWER) as Record; } @@ -122,7 +137,14 @@ describe("PEN-3266 pipeline stage config withholding", () => { * otherwise persist the sentinel over the real command — a silent destructive write, not a disclosure. */ describe("PEN-3266 pipeline stage config write-back guard", () => { - const stored = stageConfig(); + const stored = storedStageConfig(); + + it("pins the stored fixture to a shape a stage row can actually hold", () => { + // Guards the fixture itself: if `automation` creeps back in here, every assertion below that + // exercises the derived-copy branch silently stops testing the production path. + expect("automation" in stored).toBe(false); + expect(stored.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe(COMMAND_SENTINEL); + }); it("restores a masked command instead of persisting the sentinel", () => { const roundTripped = withheld(stageConfig()); @@ -168,6 +190,100 @@ describe("PEN-3266 pipeline stage config write-back guard", () => { expect(restoreWithheldPipelineStageConfig(incoming, null)).toEqual(incoming); }); + /** + * The derived `automation` block must restore from the STORED `onEnter`, because that is the only + * place the value is kept. Keying it on `existing.automation` found `undefined` on every real row + * and stripped the command instead of restoring it — and `upsertStageAutomationRoutine` then + * rebuilds `onEnter` from that stripped context, overwriting the copy the `onEnter` branch had just + * restored correctly. Net effect: renaming a stage destroyed its provision command. + */ + it("restores the derived automation copy from the stored onEnter, not from a stored automation key", () => { + const roundTripped = withheld(stageConfig()); + roundTripped.name = "Renamed stage"; + + const restored = restoreWithheldPipelineStageConfig(roundTripped, stored) as Record; + const automationSettings = restored.automation.executionWorkspaceSettings; + + // The precise production failure: stripped to `undefined` rather than left as a sentinel. + expect(automationSettings).toBeDefined(); + expect(automationSettings.workspaceStrategy.provisionCommand).toBe(COMMAND_SENTINEL); + expect(automationSettings.workspaceStrategy.teardownCommand).toBe(`${COMMAND_SENTINEL}-teardown`); + expect(automationSettings.workspaceRuntime).toEqual(stored.onEnter.executionWorkspaceSettings.workspaceRuntime); + }); + + it("leaves a sentinel in place rather than deleting the field when nothing is stored to restore", () => { + // `undefined` here would strip the key entirely, which is the destructive write the guard exists + // to stop. A literal sentinel is visibly wrong; a vanished command is silently gone. + const incoming = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { workspaceStrategy: { provisionCommand: REDACTED_EVENT_VALUE } }, + }, + }; + const restored = restoreWithheldPipelineStageConfig(incoming, { onEnter: { type: "run_routine" } }) as Record; + expect("provisionCommand" in restored.onEnter.executionWorkspaceSettings.workspaceStrategy).toBe(true); + expect(restored.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand).toBe(REDACTED_EVENT_VALUE); + }); + + /** + * `workspaceRuntime` holds arrays (`services`, and the `commands`/`jobs` siblings the mask walks), + * every command value masked for an unentitled reader. Restoring them by index means a + * read-modify-write that removes or reorders an element writes one service's real command onto + * another — a silent integrity fault, not a disclosure. `maskWorkspaceRuntimeForRead` deliberately + * lets identity keys (`id`/`name`/`label`/`title`) through on those entries, so there is a stable + * key to align on. + */ + it("aligns array elements by identity, not position, when an element is removed", () => { + const storedRuntime = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { + workspaceRuntime: { + services: [ + { name: "api", command: `${COMMAND_SENTINEL}-api` }, + { name: "web", command: `${COMMAND_SENTINEL}-web` }, + ], + }, + }, + }, + }; + // The caller dropped "api" and kept the masked "web" entry — now at index 0. + const incoming = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { + workspaceRuntime: { services: [{ name: "web", command: REDACTED_EVENT_VALUE }] }, + }, + }, + }; + + const restored = restoreWithheldPipelineStageConfig(incoming, storedRuntime) as Record; + const services = restored.onEnter.executionWorkspaceSettings.workspaceRuntime.services; + expect(services).toHaveLength(1); + // Index alignment would have handed "web" the command belonging to "api". + expect(services[0].command).toBe(`${COMMAND_SENTINEL}-web`); + }); + + it("does not guess a neighbour when an identity-less array element cannot be aligned", () => { + const storedRuntime = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { workspaceRuntime: { commands: ["stored-first", "stored-second"] } }, + }, + }; + const incoming = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { workspaceRuntime: { commands: [REDACTED_EVENT_VALUE] } }, + }, + }; + + const restored = restoreWithheldPipelineStageConfig(incoming, storedRuntime) as Record; + // Length changed and there is no identity key, so the sentinel stays rather than picking up + // whichever string happens to sit at index 0. + expect(restored.onEnter.executionWorkspaceSettings.workspaceRuntime.commands).toEqual([REDACTED_EVENT_VALUE]); + }); + it("passes through an incoming config that is not an object", () => { expect(restoreWithheldPipelineStageConfig(null, stored)).toBeNull(); expect(restoreWithheldPipelineStageConfig("x", stored)).toBe("x"); diff --git a/server/src/redaction.ts b/server/src/redaction.ts index 2727c8ecfb1d..ba8e2aa3fc04 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -961,32 +961,88 @@ export function restoreWithheldPipelineStageConfig(incoming: unknown, existing: if (!isPlainObject(incoming)) return incoming; const existingRecord = isPlainObject(existing) ? existing : {}; + // BOTH incoming keys restore from the SAME stored block, and that asymmetry is the whole + // point: `persistedStageConfig` (`services/pipelines.ts`) destructures `automation` out + // before every write, so no stored row can carry one. Keying the `automation` branch on + // `existing.automation` therefore always found `undefined` and stripped the command instead + // of restoring it — and `upsertStageAutomationRoutine` then rebuilds `onEnter` from that + // stripped context (`onEnter: { type, routineId, ...input.executionContext }`), overwriting + // the copy the `onEnter` branch had just restored correctly. `onEnter` is the only persisted + // copy of this carrier; `automation` is derived from it on read. + const storedBlock = isPlainObject(existingRecord.onEnter) ? existingRecord.onEnter : {}; + const storedSettings = storedBlock.executionWorkspaceSettings; + const restored: Record = { ...incoming }; for (const key of ["onEnter", "automation"]) { const block = restored[key]; if (!isPlainObject(block)) continue; if (!("executionWorkspaceSettings" in block)) continue; - const existingBlock = isPlainObject(existingRecord[key]) - ? (existingRecord[key] as Record) - : {}; restored[key] = { ...block, - executionWorkspaceSettings: restoreWithheldValue( - block.executionWorkspaceSettings, - existingBlock.executionWorkspaceSettings, - ), + executionWorkspaceSettings: restoreWithheldValue(block.executionWorkspaceSettings, storedSettings), }; } return restored; } +function readArrayElementIdentity(value: unknown): { key: string; value: string } | null { + if (!isPlainObject(value)) return null; + // The mask's OWN set, not a copy: an identity key added there must start being honoured here in + // the same commit, or the restore silently loses the ability to align on it. + for (const key of WORKSPACE_RUNTIME_IDENTITY_KEYS) { + const candidate = value[key]; + if (typeof candidate === "string" && candidate.length > 0) return { key, value: candidate }; + } + return null; +} + +/** + * PEN-3266. Which stored element an incoming array element should restore from. + * + * Aligning by index alone is wrong for a read-modify-write that removes or reorders an + * element: the sentinel at index *i* would be restored from the stored element at index *i*, + * silently writing one service's real command onto another. `workspaceRuntime` holds exactly + * such arrays (`services`, and the `commands`/`jobs` siblings `maskWorkspaceRuntimeForRead` + * walks), and that mask deliberately lets identity keys through on those entries — so an + * unentitled round-tripper still holds a real `id`/`name` to key on. + * + * Ambiguity fails closed: no identity, or an identity matching zero or several stored + * elements, yields `undefined`, and the caller then leaves the incoming value untouched + * rather than guessing a neighbour. Index alignment survives only as the fallback for + * identity-less elements in an array that demonstrably did not change length. + */ +function matchExistingArrayElement( + incoming: unknown, + index: number, + incomingLength: number, + existingArray: readonly unknown[], +): unknown { + const identity = readArrayElementIdentity(incoming); + if (identity) { + const matches = existingArray.filter((candidate) => { + const candidateIdentity = readArrayElementIdentity(candidate); + return candidateIdentity?.key === identity.key && candidateIdentity?.value === identity.value; + }); + return matches.length === 1 ? matches[0] : undefined; + } + return existingArray.length === incomingLength ? existingArray[index] : undefined; +} + function restoreWithheldValue(incoming: unknown, existing: unknown): unknown { if (typeof incoming === "string") { - return incoming.includes(REDACTED_EVENT_VALUE) ? existing : incoming; + if (!incoming.includes(REDACTED_EVENT_VALUE)) return incoming; + // Nothing stored to put back. Leave the sentinel rather than returning `undefined`, which + // would strip the key entirely — that is the destructive write this guard exists to stop, + // and a literal sentinel in the config is at least visibly wrong instead of silently gone. + // `null` is a real stored value ("no command configured") and is restored normally. + return existing === undefined ? incoming : existing; } if (Array.isArray(incoming)) { const existingArray = Array.isArray(existing) ? existing : []; - return incoming.map((value, index) => restoreWithheldValue(value, existingArray[index])); + return incoming.map((value, index) => restoreWithheldValue( + value, + matchExistingArrayElement(value, index, incoming.length, existingArray), + )); } if (!isPlainObject(incoming)) return incoming; diff --git a/server/src/routes/workspace-response.ts b/server/src/routes/workspace-response.ts index 73356a7e3763..9c1f1c38aeae 100644 --- a/server/src/routes/workspace-response.ts +++ b/server/src/routes/workspace-response.ts @@ -368,6 +368,11 @@ export function publicIssueExecutionWorkspaceSettings( * Enumerated over a walk on purpose: `config` holds unrelated operator prose (`variables`, * `disabledReason`, breakdown templates) that is not workspace-runtime material and must survive * intact, so a blanket mask over the blob would be wrong in the other direction. + * + * This read projection is only half of the fix; the write-side counterpart that keeps it from + * destroying the values it masks is `restoreWithheldPipelineStageConfig` in `../redaction.ts`. + * That lives there rather than here because `services/pipelines.ts` is what has to call it, and + * services do not import from the route layer. */ export function publicPipelineStageConfig(config: unknown, viewer: WorkspaceRuntimeViewer): unknown { if (viewer.revealRuntimeConfig) return config; @@ -386,12 +391,6 @@ export function publicPipelineStageConfig(config: unknown, viewer: WorkspaceRunt return projected; } -/** - * PEN-3266. The read projection above is only half of the fix; the write-side counterpart that keeps - * it from destroying the values it masks is `restoreWithheldPipelineStageConfig` in `../redaction.ts`. - * It lives there rather than here because `services/pipelines.ts` is what has to call it, and services - * do not import from the route layer. - */ export function publicExecutionWorkspace( workspace: ExecutionWorkspace, viewer: WorkspaceRuntimeViewer, From 9ea62760c4e6df08e2b188af699c18463dc9b44e Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Tue, 22 Sep 2026 21:24:13 +0000 Subject: [PATCH 4/5] security(pipelines): project stage config on the case read surface and the review-cases exit (PEN-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consolidated review at head `7ca388a8` named four unprojected case-route exits. An independent sweep of every `pipelineStages` egress in the route and service layers confirmed those four and found a fifth the review did not name. **The four case exits** (all gated by `assertCaseAccess` → company scope only, the same gate class as the `GET /pipelines/:pipelineId` exit already closed): - `GET /pipelines/:pipelineId/cases` — `stage` selected as the whole table. - `GET /cases/:caseId/children` — `...row` spreads `stage`. - `getCaseDetail` — `stage` and `parentCase.stage`. - `allowedNextStages` — the widest: EVERY stage config in the pipeline, returned from a case-detail read and re-exported as `allowedTransitions`. `getCaseDetail` is a bare function, so the viewer is threaded in from its three callers. Projecting inside it rather than at each `res.json` means a future caller that returns the detail wholesale is masked by construction. `POST /cases/:caseId/open-conversation` passes the WITHHELD viewer rather than the caller's: that detail never reaches a response — it feeds `buildCaseContextMarkdown`, which projects the stage to `{id,key,name,kind}` — and the markdown becomes an agent-visible issue description, so the most restrictive viewer is the correct one. **The fifth, found by the sweep: `GET /companies/:companyId/review-cases`.** It carries the commands TWICE on one response, and masking the obvious carrier alone would have left them egressing on the sibling: - `stage` — the whole `pipeline_stages` row, spread by `...row` in `listReviewCases`, nothing overwriting it. - `reviewConfig` — `reviewConfigForStage` spreads `normalizeStageConfig(...)`, which strips only `automation`, `assigneeAgentId` and `reviewerKind`. `onEnter` survives it intact, so the same `executionWorkspaceSettings` rides out a second time under a key whose name suggests it only carries review-routing fields. Projected in the route rather than the service because `services/` does not import the route layer. **Negative controls, recorded so the next sweep does not re-derive them.** `POST /companies/:companyId/pipelines` does return whole stage rows with no viewer resolved, and is deliberately left unprojected: every value in that config is the caller's own request body, and the only other source, `DEFAULT_STAGES`, carries no `onEnter`. There is nothing there the caller did not just supply. A comment at the site says so, and says what would make it a real carrier. Also verified clear: `getStagesByKey`, `getChildOutcomeSummaries`, `loadBuiltFromAutomation`, the intake-form and health routes, `issues.ts` `listIssueLinkedCases`, all of `pipelines-aggregation.ts`, and every realtime channel — the live-events/WS fan-out ships no stage row, and the only `config` written into a case-event payload is the narrowed breakdown config. **Tests.** The existing suite is unit-level: it proves the projection is correct, not that any route applies it. Three route cases now drive the real router against the real `accessService`, so the entitlement decision is the production one rather than a mock. Mutation check against the unmodified `routes/pipelines.ts` at `7ca388a8`: both withholding cases fail on their intended assertions, while the entitled-reader positive control passes in both worlds — which is what makes the `not.toContain` assertions mean something rather than passing because the fixture never reached the response. Also hoists `resolveWorkspaceRuntimeViewer` out of the `res.json(...)` argument on the two stage-mutation routes, matching every other call site and keeping the access decision off the far side of the write, where a throw would land in the `codedConflictForUnique` catch after the mutation had already happened. Refs PEN-3266 Signed-off-by: Security Engineer --- server/src/__tests__/pipelines-routes.test.ts | 187 ++++++++++++++++++ server/src/routes/pipelines.ts | 69 +++++-- 2 files changed, 244 insertions(+), 12 deletions(-) diff --git a/server/src/__tests__/pipelines-routes.test.ts b/server/src/__tests__/pipelines-routes.test.ts index c386f6eed596..8fdcef0d14de 100644 --- a/server/src/__tests__/pipelines-routes.test.ts +++ b/server/src/__tests__/pipelines-routes.test.ts @@ -1282,4 +1282,191 @@ describeEmbeddedPostgres("pipeline routes", () => { expect(res.body.details.version).toBe(2); expect(res.body.details.stage.key).toBe("intake"); }); + + /** + * PEN-3266. Route-level coverage for the `pipeline_stages.config` carrier. + * + * The sibling suite (`pipeline-stage-workspace-settings-withholding.test.ts`) exercises + * `publicPipelineStageConfig` directly, which proves the projection is CORRECT but not that any + * given route APPLIES it. Every exit below returns a whole `pipeline_stages` row — some by + * `...row` spread — so the wiring is exactly the thing that can be forgotten, and a unit test of + * the helper cannot see it. These cases drive the real router against the real + * `accessService`, so the entitlement decision is the production one rather than a mock. + * + * ⛔ Every planted value is invented. No real command or path is quoted, per PEN-2370. + */ + describe("PEN-3266 stage config withholding across the case read surface", () => { + const STAGE_COMMAND_SENTINEL = "sentinel-route-provision-must-not-egress"; + const STAGE_RUNTIME_SENTINEL = "sentinel-route-runtime-must-not-egress"; + + function plantedStageConfig(existing: unknown) { + const base = existing && typeof existing === "object" && !Array.isArray(existing) + ? existing as Record + : {}; + // MERGED, not replaced: the seeded review stage already carries the `approveToStageKey` / + // `rejectToStageKey` that `normalizeStageConfig` requires, and dropping them would make + // `reviewConfigForStage` throw — turning a withholding test into a 500-response test. + return { + ...base, + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { + mode: "isolated_workspace", + workspaceStrategy: { + type: "git_worktree", + provisionCommand: STAGE_COMMAND_SENTINEL, + teardownCommand: `${STAGE_COMMAND_SENTINEL}-teardown`, + }, + workspaceRuntime: { services: [{ name: "web", command: STAGE_RUNTIME_SENTINEL }] }, + }, + }, + }; + } + + /** + * Planted with a direct UPDATE rather than through `POST /pipelines`, on purpose: an operator + * authors this config, and going through the create route would drag in + * `validateStageAutomationConfig` and a backing routine that have nothing to do with the + * carrier. This models the stored row, which is what every route below reads. + * + * Planted AFTER the transition for the same reason — an `onEnter` block present during a + * stage entry is automation input, and this test is about the read path, not that machinery. + */ + async function seedCarrier() { + const company = await seedCompany(); + const boardHttp = request(app(boardActor)); + const pipeline = await boardHttp + .post(`/api/companies/${company.id}/pipelines`) + .send({ key: "carrier", name: "Carrier" }) + .expect(201); + const parent = await boardHttp + .post(`/api/pipelines/${pipeline.body.id}/cases`) + .send({ caseKey: "carrier-parent", title: "Carrier parent" }) + .expect(201); + const child = await boardHttp + .post(`/api/pipelines/${pipeline.body.id}/cases`) + .send({ caseKey: "carrier-child", title: "Carrier child", parentCaseId: parent.body.case.id }) + .expect(201); + // `listReviewCases` only returns cases sitting in a `review`-kind stage. Without this the + // review-cases body is `[]` and every assertion against it is vacuous. + await boardHttp + .post(`/api/cases/${child.body.case.id}/transition`) + .send({ toStageKey: "review", expectedVersion: 1 }) + .expect(200); + + // Every stage, so `allowedNextStages` — the widest exit, which returns every stage in the + // pipeline rather than just the case's own — carries it too. + const stages = await db.select().from(pipelineStages).where(eq(pipelineStages.pipelineId, pipeline.body.id)); + for (const stage of stages) { + await db + .update(pipelineStages) + .set({ config: plantedStageConfig(stage.config) }) + .where(eq(pipelineStages.id, stage.id)); + } + + const [agent] = await db.insert(agents).values({ + companyId: company.id, + name: "Ordinary Agent", + role: "engineer", + adapterType: "codex_local", + }).returning(); + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "agent", + principalId: agent!.id, + status: "active", + membershipRole: "member", + }); + // Deliberately NO `workspace_runtime:read` grant: this models the actor class the ticket + // names — an ordinary same-company agent. + const unentitledActor: Express.Request["actor"] = { + type: "agent", + agentId: agent!.id, + companyId: company.id, + runId: randomUUID(), + source: "agent_key", + }; + + return { + company, + pipelineId: pipeline.body.id as string, + parentCaseId: parent.body.case.id as string, + childCaseId: child.body.case.id as string, + unentitled: request(app(unentitledActor)), + entitled: boardHttp, + }; + } + + it("withholds the commands from an unentitled reader on every case read exit", async () => { + const seeded = await seedCarrier(); + + const responses = await Promise.all([ + seeded.unentitled.get(`/api/companies/${seeded.company.id}/review-cases`).expect(200), + seeded.unentitled.get(`/api/pipelines/${seeded.pipelineId}/cases`).expect(200), + seeded.unentitled.get(`/api/cases/${seeded.parentCaseId}/children`).expect(200), + seeded.unentitled.get(`/api/cases/${seeded.childCaseId}`).expect(200), + seeded.unentitled.get(`/api/cases/${seeded.childCaseId}/context-pack`).expect(200), + ]); + + for (const response of responses) { + const body = JSON.stringify(response.body); + expect(body).not.toContain(STAGE_COMMAND_SENTINEL); + expect(body).not.toContain(STAGE_RUNTIME_SENTINEL); + } + }); + + /** + * Positive control for the assertions above, in the same run. `not.toContain` passes just as + * happily when the fixture never reached the response at all — a stage without the planted + * config, a route that 404s, a serializer that drops the key. This proves the sentinel IS + * reachable through these exits and that the withholding above is the reason it is absent. + */ + it("discloses the same bytes to an entitled reader, proving the sentinel reaches these exits", async () => { + const seeded = await seedCarrier(); + + const responses = await Promise.all([ + seeded.entitled.get(`/api/companies/${seeded.company.id}/review-cases`).expect(200), + seeded.entitled.get(`/api/pipelines/${seeded.pipelineId}/cases`).expect(200), + seeded.entitled.get(`/api/cases/${seeded.parentCaseId}/children`).expect(200), + seeded.entitled.get(`/api/cases/${seeded.childCaseId}`).expect(200), + ]); + + for (const response of responses) { + expect(JSON.stringify(response.body)).toContain(STAGE_COMMAND_SENTINEL); + } + }); + + /** + * `reviewConfig` is a SECOND carrier on the review-cases response, under a key whose name + * suggests it only holds review-routing fields. It is built by `reviewConfigForStage` + * (`services/pipelines.ts`), which spreads `normalizeStageConfig(...)` — and that strips only + * `automation`, `assigneeAgentId` and `reviewerKind`, so `onEnter` survives it intact. Masking + * `stage` alone would have left the commands egressing here, which is why this is pinned + * separately rather than folded into the sweep above. + */ + it("masks the reviewConfig copy, not only the stage row, on the review-cases exit", async () => { + const seeded = await seedCarrier(); + + const withheldRes = await seeded.unentitled + .get(`/api/companies/${seeded.company.id}/review-cases`) + .expect(200); + const revealedRes = await seeded.entitled + .get(`/api/companies/${seeded.company.id}/review-cases`) + .expect(200); + + // A review-stage case has to exist, or both bodies are `[]` and every assertion is vacuous. + expect(revealedRes.body.length).toBeGreaterThan(0); + for (const row of revealedRes.body) { + expect(row.reviewConfig.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand) + .toBe(STAGE_COMMAND_SENTINEL); + // The review-routing fields the board UI reads must survive the mask. + expect(row.reviewConfig.approveToStageKey).toBe("done"); + } + for (const row of withheldRes.body) { + expect(row.reviewConfig.onEnter.executionWorkspaceSettings.workspaceStrategy.provisionCommand) + .not.toBe(STAGE_COMMAND_SENTINEL); + expect(row.reviewConfig.approveToStageKey).toBe("done"); + } + }); + }); }); diff --git a/server/src/routes/pipelines.ts b/server/src/routes/pipelines.ts index 50ed10ca8fb4..b6f83e3baa47 100644 --- a/server/src/routes/pipelines.ts +++ b/server/src/routes/pipelines.ts @@ -72,6 +72,7 @@ import { import { publicPipelineStageConfig, resolveWorkspaceRuntimeViewer, + WITHHELD_WORKSPACE_RUNTIME_VIEWER, type WorkspaceRuntimeViewer, } from "./workspace-response.js"; import { documentAnnotationService } from "../services/document-annotations.js"; @@ -945,6 +946,13 @@ export function pipelineRoutes(db: Db, options: Parameters ({ + ...row, + stage: publicPipelineStage(row.stage, stageViewer), + reviewConfig: publicPipelineStageConfig(row.reviewConfig, stageViewer), + }))); }); router.post("/companies/:companyId/review-cases/bulk", validate(bulkReviewSchema), async (req, res) => { @@ -1234,7 +1259,8 @@ export function pipelineRoutes(db: Db, options: Parameters row.case.id); - const [activeWork, descendantActiveWorkCounts] = await Promise.all([ + const [activeWork, descendantActiveWorkCounts, stageViewer] = await Promise.all([ loadActiveWorkForCases(db, companyId, caseIds), loadDescendantActiveWorkCountsForCases(db, companyId, caseIds), + resolveWorkspaceRuntimeViewer(access, req, companyId), ]); res.json(rows.map((row) => ({ case: row.case, - stage: row.stage, + stage: publicPipelineStage(row.stage, stageViewer), parentCase: row.parentCase?.id && row.parentPipeline?.id ? { case: row.parentCase, @@ -1594,7 +1622,7 @@ export function pipelineRoutes(db: Db, options: Parameters { const caseId = req.params.caseId as string; const companyId = await assertCaseAccess(db, req, caseId); - const detail = await getCaseDetail(db, companyId, caseId); + const detail = await getCaseDetail(db, companyId, caseId, await resolveWorkspaceRuntimeViewer(access, req, companyId)); res.json(detail); }); @@ -1938,12 +1966,14 @@ export function pipelineRoutes(db: Db, options: Parameters row.case.id); - const [activeWork, descendantActiveWorkCounts] = await Promise.all([ + const [activeWork, descendantActiveWorkCounts, stageViewer] = await Promise.all([ loadActiveWorkForCases(db, companyId, caseIds), loadDescendantActiveWorkCountsForCases(db, companyId, caseIds), + resolveWorkspaceRuntimeViewer(access, req, companyId), ]); res.json(rows.map((row) => ({ ...row, + stage: publicPipelineStage(row.stage, stageViewer), activeWork: activeWork.get(row.case.id) ?? null, descendantActiveWorkCount: descendantActiveWorkCounts.get(row.case.id) ?? 0, }))); @@ -2049,7 +2079,11 @@ export function pipelineRoutes(db: Db, options: Parameters summarizePipelineCaseOutputsForContext(outputs)), @@ -2216,7 +2250,7 @@ export function pipelineRoutes(db: Db, options: Parameters { const caseId = req.params.caseId as string; const companyId = await assertCaseAccess(db, req, caseId); - const detail = await getCaseDetail(db, companyId, caseId); + const detail = await getCaseDetail(db, companyId, caseId, await resolveWorkspaceRuntimeViewer(access, req, companyId)); const [events, outputs, childOutcomes] = await Promise.all([ svc.listCaseEventsPage(companyId, caseId, { limit: PIPELINE_CONTEXT_PACK_EVENT_LIMIT, @@ -2307,7 +2341,15 @@ export function pipelineRoutes(db: Db, options: Parameters publicPipelineStage(stage, viewer)), links, blockers, blocks, @@ -2376,7 +2419,9 @@ async function getCaseDetail(db: Db, companyId: string, caseId: string) { liveness, conversationSource, builtFromAutomation, - parentCase, + parentCase: parentCase === null + ? null + : { ...parentCase, stage: publicPipelineStage(parentCase.stage, viewer) }, pendingSuggestion: row.case.pendingSuggestion, }; } From 426c186dd90ebd223bae149c2be9b93ad3985c34 Mon Sep 17 00:00:00 2001 From: Security Engineer Date: Thu, 24 Sep 2026 12:07:23 +0000 Subject: [PATCH 5/5] security(pipelines): stop the write guard destroying commands whose identity the mask redacted (PEN-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write guard added earlier in this PR aligned array elements by identity so a read-modify-write could not write one service's command onto another. It accepted ANY non-empty string as an identity — including the sentinel the read mask had just put there. `maskWorkspaceRuntimeForRead` honours identity keys (`id`/`name`/`label`/`title`) only on an entry sitting directly inside a top-level `commands`/`services`/`jobs` array, and only when the value is already a string. Everywhere else the identity comes back redacted. `readArrayElementIdentity` then matched zero stored elements, `matchExistingArrayElement` read that as ambiguous and failed closed, and the caller persisted `***REDACTED***` over the operator's real value — on an UNMODIFIED round-trip. That is the exact destructive write this guard exists to prevent, one nesting level down. Three shapes reproduced against the production functions, all now lossless: - a nested array (`services[].env[]`) whose identity the mask redacted - a top-level array the parser does not bless (`containers`, `volumes`, …) - a blessed entry whose identity value is not a string (`id: 1`) Fix: a masked identity is not an identity. Skipping it falls through to the existing length-guarded index alignment, which is correct whenever the editor did not add or remove elements. A duplicate stored identity is left fail-closed deliberately — genuinely ambiguous, and guessing writes one service's command onto another — but it is now pinned by a test and the docstring states plainly that "fails closed" persists the sentinel and loses the stored value, rather than implying it is a no-op. Tests: the new cases drive the REAL mask and assert the round-trip changes no stored byte, because every pre-existing fixture was hand-written and so pinned only the one array shape that happened to work. Mutation-checked: removing the sentinel guard fails exactly those three tests on their losslessness assertions. Also corrects `upsertStageAutomationRoutine` in the guard's rationale and in the test docblock — no such symbol exists. The function that rebuilds `onEnter` from the stripped context is `syncPipelineStageAutomation` (`services/pipelines.ts:2818`, rebuild at `:2886` and `:2924`). Refs PEN-3266 Signed-off-by: Security Engineer --- ...age-workspace-settings-withholding.test.ts | 122 +++++++++++++++++- server/src/redaction.ts | 41 +++++- 2 files changed, 154 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts index c488dd378e1b..2af03c8351be 100644 --- a/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts +++ b/server/src/__tests__/pipeline-stage-workspace-settings-withholding.test.ts @@ -193,8 +193,9 @@ describe("PEN-3266 pipeline stage config write-back guard", () => { /** * The derived `automation` block must restore from the STORED `onEnter`, because that is the only * place the value is kept. Keying it on `existing.automation` found `undefined` on every real row - * and stripped the command instead of restoring it — and `upsertStageAutomationRoutine` then - * rebuilds `onEnter` from that stripped context, overwriting the copy the `onEnter` branch had just + * and stripped the command instead of restoring it — and `syncPipelineStageAutomation` + * (`services/pipelines.ts:2818`) then rebuilds `onEnter` from that stripped context (`:2886`, + * `:2924`), overwriting the copy the `onEnter` branch had just * restored correctly. Net effect: renaming a stage destroyed its provision command. */ it("restores the derived automation copy from the stored onEnter, not from a stored automation key", () => { @@ -289,3 +290,120 @@ describe("PEN-3266 pipeline stage config write-back guard", () => { expect(restoreWithheldPipelineStageConfig("x", stored)).toBe("x"); }); }); + +/** + * PEN-3266 — the round-trip property, driven through the REAL mask rather than a hand-written + * "masked" fixture. + * + * Every assertion above builds its masked input by hand, which silently pins the ONE array shape + * the identity alignment happened to handle: a top-level `services` array whose entries carry a + * unique STRING `name`. Four neighbouring shapes destroyed the operator's stored value on an + * *unmodified* round-trip, and no hand-written fixture could have caught it, because the bug lives + * in the disagreement between what `maskWorkspaceRuntimeForRead` emits and what + * `readArrayElementIdentity` accepts — and a hand-written fixture is the author asserting that + * they agree. + * + * So these drive `publicPipelineStageConfig` → `restoreWithheldPipelineStageConfig` and assert the + * only property that matters: **an unentitled reader who saves back exactly what it was handed + * must not change a single stored byte.** + */ +describe("PEN-3266 write guard — mask/restore round-trip is lossless for an unedited save", () => { + function roundTrip(storedRuntime: unknown) { + const storedConfig = { + onEnter: { + type: "run_routine", + executionWorkspaceSettings: { workspaceRuntime: storedRuntime }, + }, + }; + // Exactly what an unentitled editor receives, then saves back untouched. + const masked = publicPipelineStageConfig(storedConfig, WITHHELD_WORKSPACE_RUNTIME_VIEWER) as Record; + const restored = restoreWithheldPipelineStageConfig(masked, storedConfig) as Record; + return { + masked: masked.onEnter.executionWorkspaceSettings.workspaceRuntime, + restored: restored.onEnter.executionWorkspaceSettings.workspaceRuntime, + }; + } + + /** The shape the hand-written fixtures already cover — a positive control for this harness. */ + it("control: blessed services[] with unique string names round-trips losslessly", () => { + const storedRuntime = { + services: [ + { name: "web", command: `${COMMAND_SENTINEL}-web` }, + { name: "api", command: `${COMMAND_SENTINEL}-api` }, + ], + }; + const { masked, restored } = roundTrip(storedRuntime); + expect(masked.services[0].command).toBe(REDACTED_EVENT_VALUE); + expect(restored).toEqual(storedRuntime); + }); + + /** + * `identityScope` is true only for entries sitting DIRECTLY inside a top-level + * `commands`/`services`/`jobs` array, so a nested `env` array has its `name` masked. The restore + * then read that sentinel as a legitimate identity, matched zero stored elements, and persisted + * the sentinel over the real value. + */ + it("restores a nested array whose identity key the mask replaced with a sentinel", () => { + const storedRuntime = { + services: [{ + name: "web", + command: `${COMMAND_SENTINEL}-web`, + env: [{ name: "REGION", value: `${RUNTIME_SENTINEL}-region` }], + }], + }; + const { masked, restored } = roundTrip(storedRuntime); + // The mask does redact the nested identity — that half is intended and stays asserted. + expect(masked.services[0].env[0].name).toBe(REDACTED_EVENT_VALUE); + expect(restored).toEqual(storedRuntime); + }); + + /** A top-level array the parser does not bless gets NO identity honoured at all. */ + it("restores a top-level array that is not commands/services/jobs", () => { + const storedRuntime = { + containers: [ + { name: "web", command: `${COMMAND_SENTINEL}-a` }, + { name: "api", command: `${COMMAND_SENTINEL}-b` }, + ], + }; + const { masked, restored } = roundTrip(storedRuntime); + expect(masked.containers[0].name).toBe(REDACTED_EVENT_VALUE); + expect(restored).toEqual(storedRuntime); + }); + + /** + * `maskEntry` emits the sentinel for a non-string identity, so mask and restore disagreed about + * which key was the identity and every command in the array was destroyed. + */ + it("restores a blessed array whose identity value is not a string", () => { + const storedRuntime = { + services: [ + { id: 1, command: `${COMMAND_SENTINEL}-1` }, + { id: 2, command: `${COMMAND_SENTINEL}-2` }, + ], + }; + const { restored } = roundTrip(storedRuntime); + expect(restored).toEqual(storedRuntime); + }); + + /** + * ⚠️ Pins a KNOWN, DELIBERATE loss rather than a fix. A duplicate stored identity is genuinely + * ambiguous — there is no way to tell a reorder from a no-op — and writing one service's real + * command onto another is the silent fault the identity alignment exists to prevent. So this + * branch keeps the sentinel, which loses the stored value VISIBLY. + * + * It is pinned so the cost is a recorded choice. If this test ever starts failing because + * someone made the index fallback unconditional, that is a decision to take deliberately, not a + * green diff. + */ + it("known loss: a duplicate stored identity keeps the sentinel rather than guessing", () => { + const storedRuntime = { + services: [ + { name: "web", command: `${COMMAND_SENTINEL}-1` }, + { name: "web", command: `${COMMAND_SENTINEL}-2` }, + ], + }; + const { restored } = roundTrip(storedRuntime); + expect(restored.services[0].command).toBe(REDACTED_EVENT_VALUE); + expect(restored.services[1].command).toBe(REDACTED_EVENT_VALUE); + }); +}); diff --git a/server/src/redaction.ts b/server/src/redaction.ts index ba8e2aa3fc04..8022d90ce09d 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -965,8 +965,9 @@ export function restoreWithheldPipelineStageConfig(incoming: unknown, existing: // point: `persistedStageConfig` (`services/pipelines.ts`) destructures `automation` out // before every write, so no stored row can carry one. Keying the `automation` branch on // `existing.automation` therefore always found `undefined` and stripped the command instead - // of restoring it — and `upsertStageAutomationRoutine` then rebuilds `onEnter` from that - // stripped context (`onEnter: { type, routineId, ...input.executionContext }`), overwriting + // of restoring it — and `syncPipelineStageAutomation` (`services/pipelines.ts:2818`) then + // rebuilds `onEnter` from that stripped context (`:2886`, `:2924`: + // `onEnter: { type, routineId, ...input.executionContext }`), overwriting // the copy the `onEnter` branch had just restored correctly. `onEnter` is the only persisted // copy of this carrier; `automation` is derived from it on read. const storedBlock = isPlainObject(existingRecord.onEnter) ? existingRecord.onEnter : {}; @@ -991,7 +992,20 @@ function readArrayElementIdentity(value: unknown): { key: string; value: string // the same commit, or the restore silently loses the ability to align on it. for (const key of WORKSPACE_RUNTIME_IDENTITY_KEYS) { const candidate = value[key]; - if (typeof candidate === "string" && candidate.length > 0) return { key, value: candidate }; + if (typeof candidate !== "string" || candidate.length === 0) continue; + // A MASKED identity is not an identity. `maskWorkspaceRuntimeForRead` honours these keys only + // on an entry sitting directly inside a top-level `commands`/`services`/`jobs` array + // (`identityScope`), and only when the value is already a string. Everywhere else — a nested + // `env` array inside a service, a top-level array the parser does not bless (`containers`, + // `volumes`, …), or a blessed entry whose `id` is a NUMBER — the identity key comes back as the + // sentinel. Accepting that sentinel as an identity made this function match zero stored + // elements, which `matchExistingArrayElement` reads as "ambiguous" and fails closed on, so the + // caller persisted the sentinel OVER the operator's real value on an UNMODIFIED round-trip — + // the exact destructive write this guard exists to prevent, one nesting level down. + // Treating it as absent instead falls through to the length-guarded index alignment below, + // which is correct whenever the editor did not add or remove elements. + if (candidate.includes(REDACTED_EVENT_VALUE)) continue; + return { key, value: candidate }; } return null; } @@ -1006,10 +1020,23 @@ function readArrayElementIdentity(value: unknown): { key: string; value: string * walks), and that mask deliberately lets identity keys through on those entries — so an * unentitled round-tripper still holds a real `id`/`name` to key on. * - * Ambiguity fails closed: no identity, or an identity matching zero or several stored - * elements, yields `undefined`, and the caller then leaves the incoming value untouched - * rather than guessing a neighbour. Index alignment survives only as the fallback for - * identity-less elements in an array that demonstrably did not change length. + * Ambiguity fails closed: an identity matching zero or several stored elements yields + * `undefined`, and the caller then leaves the incoming value untouched rather than guessing a + * neighbour. ⚠️ Be precise about what that costs — "fails closed" here is NOT a no-op. The + * incoming value under a sentinel IS the sentinel, so leaving it untouched persists + * `***REDACTED***` and the operator's stored value is lost. It is a *visible* loss rather than a + * silent one (a literal sentinel in the config is obviously wrong, whereas a neighbour's real + * command is not), and that is the only sense in which it is the safer branch. + * + * The one case that genuinely reaches it is a DUPLICATE identity in the stored array — two + * services both named `web`. That is left fail-closed deliberately: with the identity ambiguous + * there is no way to tell a reorder from a no-op, and silently writing one service's command onto + * another is the failure this alignment was introduced to stop. It is pinned by a test so the + * cost is a recorded choice rather than an accident. + * + * Index alignment survives as the fallback for elements with no USABLE identity — including one + * whose identity the mask replaced with a sentinel — in an array that demonstrably did not change + * length. */ function matchExistingArrayElement( incoming: unknown,