security(workspaces): scrub secrets on the workspace-operation write path (PEN-3205)

server/src/__tests__/workspace-operation-secret-scrub.test.ts

@@ -0,0 +1,141 @@
+import { describe, expect, it, vi, beforeEach } from "vitest";
+
+// PEN-3205: the workspace-operation write path applied only `redactCurrentUserText`
+// (username censoring) to captured command output, so a secret-shaped chunk reached
+// BOTH durable sinks in the clear: the `stdoutExcerpt`/`stderrExcerpt` columns and the
+// log-store body. The amplifier is `buildWorkspaceCommandEnv`, which hands operator
+// commands `{ ...process.env }` wholesale — a command under `set -x`, or any tool that
+// dumps its environment on failure, writes the server environment into a durable
+// company-readable channel.
+//
+// These tests assert BOTH sinks on purpose. Scrubbing one and not the other is the
+// exact half-control this row exists to close, and a test that only reads the excerpt
+// would have passed against a fix that left the log body unscrubbed.
+
+const capturedAppends: { stream: string; chunk: string }[] = [];
+const capturedUpdates: Record<string, unknown>[] = [];
+const capturedInserts: Record<string, unknown>[] = [];
+
+vi.mock("../services/instance-settings.js", () => ({
+  instanceSettingsService: () => ({
+    getGeneral: async () => ({ censorUsernameInLogs: false }),
+  }),
+}));
+
+vi.mock("../services/workspace-operation-log-store.js", () => ({
+  getWorkspaceOperationLogStore: () => ({
+    begin: async () => ({ store: "local_file", logRef: "test/log.ndjson" }),
+    append: async (_handle: unknown, event: { stream: string; chunk: string }) => {
+      capturedAppends.push({ stream: event.stream, chunk: event.chunk });
+    },
+    finalize: async () => ({ bytes: 0, sha256: "sha", compressed: false }),
+    read: async () => ({ content: "" }),
+  }),
+}));
+
+const { workspaceOperationService } = await import("../services/workspace-operations.js");
+
+function makeFakeDb() {
+  const thenableWhere = (payload: Record<string, unknown>) => {
+    const row = {
+      id: "op-1",
+      companyId: "company-1",
+      phase: "provision",
+      status: "succeeded",
+      logCompressed: false,
+      startedAt: new Date(),
+      createdAt: new Date(),
+      updatedAt: new Date(),
+      ...payload,
+    };
+    return {
+      returning: () => Promise.resolve([row]),
+      then: (resolve: (v: unknown) => unknown) => Promise.resolve([row]).then(resolve),
+    };
+  };
+
+  return {
+    insert: () => ({
+      values: async (payload: Record<string, unknown>) => {
+        capturedInserts.push(payload);
+      },
+    }),
+    update: () => ({
+      set: (payload: Record<string, unknown>) => {
+        capturedUpdates.push(payload);
+        return { where: () => thenableWhere(payload) };
+      },
+    }),
+    select: () => ({ from: () => ({ where: () => Promise.resolve([]) }) }),
+  } as never;
+}
+
+const SECRET = "fake-pen3205-workspace-op-secret";
+const SECRET_CHUNK = `PAPERCLIP_SYNTHETIC_TOKEN=${SECRET}\nSAFE_ENV_NAME=visible\n`;
+
+async function runOperation(result: { stdout?: string; stderr?: string }) {
+  capturedAppends.length = 0;
+  capturedUpdates.length = 0;
+  capturedInserts.length = 0;
+
+  const svc = workspaceOperationService(makeFakeDb());
+  const recorder = svc.createRecorder({ companyId: "company-1" });
+  await recorder.recordOperation({
+    phase: "provision" as never,
+    command: "bash -lc 'set -x; env'",
+    cwd: "/workspace",
+    run: async () => ({ stdout: result.stdout ?? null, stderr: result.stderr ?? null }),
+  });
+
+  const finalUpdate = capturedUpdates.at(-1) ?? {};
+  return {
+    stdoutExcerpt: (finalUpdate.stdoutExcerpt as string | null) ?? "",
+    stderrExcerpt: (finalUpdate.stderrExcerpt as string | null) ?? "",
+    logBody: capturedAppends.map((a) => a.chunk).join(""),
+  };
+}
+
+describe("workspace-operation captured output is secret-scrubbed at write time", () => {
+  beforeEach(() => {
+    capturedAppends.length = 0;
+    capturedUpdates.length = 0;
+    capturedInserts.length = 0;
+  });
+
+  it("scrubs a secret-shaped stdout chunk in the excerpt column", async () => {
+    const { stdoutExcerpt } = await runOperation({ stdout: SECRET_CHUNK });
+
+    expect(stdoutExcerpt).not.toContain(SECRET);
+    expect(stdoutExcerpt).toContain("PAPERCLIP_SYNTHETIC_TOKEN=***REDACTED***");
+  });
+
+  it("scrubs the same chunk in the durable log-store body", async () => {
+    const { logBody } = await runOperation({ stdout: SECRET_CHUNK });
+
+    expect(logBody).not.toContain(SECRET);
+    expect(logBody).toContain("PAPERCLIP_SYNTHETIC_TOKEN=***REDACTED***");
+  });
+
+  it("scrubs secret-shaped stderr too — the failure path is where env dumps land", async () => {
+    const { stderrExcerpt, logBody } = await runOperation({ stderr: SECRET_CHUNK });
+
+    expect(stderrExcerpt).not.toContain(SECRET);
+    expect(logBody).not.toContain(SECRET);
+  });
+
+  // Discriminates scrubbing from blanking: a fix that simply dropped the output
+  // would satisfy every "not.toContain" above. This is the control that fails it.
+  it("passes a non-secret chunk through unchanged in both sinks", async () => {
+    const benign = "Cloning into 'repo'...\nremote: Enumerating objects: 42, done.\n";
+    const { stdoutExcerpt, logBody } = await runOperation({ stdout: benign });
+
+    expect(stdoutExcerpt).toBe(benign);
+    expect(logBody).toBe(benign);
+  });
+
+  it("keeps the non-secret line beside a scrubbed one", async () => {
+    const { stdoutExcerpt } = await runOperation({ stdout: SECRET_CHUNK });
+
+    expect(stdoutExcerpt).toContain("SAFE_ENV_NAME=visible");
+  });
+});

server/src/__tests__/workspace-runtime-response-withholding.test.ts

@@ -1,6 +1,7 @@
 import express from "express";
 import request from "supertest";
 import { readFileSync } from "node:fs";
+import os from "node:os";
 import { fileURLToPath } from "node:url";
 import path from "node:path";
 import { beforeEach, describe, expect, it, vi } from "vitest";
@@ -182,6 +183,24 @@ vi.mock("../routes/workspace-runtime-service-authz.js", () => ({
   assertCanManageExecutionWorkspaceRuntimeServices: vi.fn(async () => undefined),
 }));
 
+/**
+ * PEN-3205: the workspace-operations route now reads `censorUsernameInLogs` per request, to apply
+ * the username censoring the sibling list route in `routes/agents.ts` already applied. These apps
+ * mount `{}` as `db`, so the real service's `getGeneral()` throws and the route answers 500 —
+ * stubbing it is what keeps the withholding boundary below drivable at all.
+ *
+ * Default OFF so the censor is a no-op: the withholding assertions in this file measure
+ * `publicWorkspaceOperation`, and a censor running underneath them could mask a sentinel and make
+ * a withholding test pass for the wrong reason. The censor has its own coverage below, which
+ * turns it on explicitly and pairs it with the off-case as the discriminator.
+ */
+const mockInstanceGeneralSettings = vi.hoisted(() => ({ censorUsernameInLogs: false }));
+vi.mock("../services/instance-settings.js", () => ({
+  instanceSettingsService: () => ({
+    getGeneral: async () => ({ ...mockInstanceGeneralSettings }),
+  }),
+}));
+
 function runtimeBlob(): Record<string, unknown> {
   return {
     services: [{ name: "web", command: SECRET_SENTINEL, env: { TOKEN_FIXTURE: SECOND_SENTINEL } }],
@@ -327,6 +346,7 @@ function createApp(mount: "execution-workspaces" | "projects") {
 describe("workspace runtime withholding boundary (PEN-2852)", () => {
   beforeEach(() => {
     vi.clearAllMocks();
+    mockInstanceGeneralSettings.censorUsernameInLogs = false;
     decideAsUnprivilegedReader();
     mockExecutionWorkspaceService.getById.mockResolvedValue(executionWorkspaceFixture());
     mockExecutionWorkspaceService.list.mockResolvedValue([executionWorkspaceFixture()]);
@@ -647,6 +667,51 @@ describe("workspace runtime withholding boundary (PEN-2852)", () => {
       expect(res.body[0].cwd).toBe(OPERATION_CWD_SENTINEL);
     });
 
+    /**
+     * PEN-3205, read side. `publicWorkspaceOperation` masks `command`/`cwd`/`metadata` and spreads
+     * the rest, so `stdoutExcerpt` crosses this route UNMASKED by design — the username censor is
+     * the only control standing over it here, and `routes/agents.ts` was already applying it on
+     * the sibling list route while this one answered with a bare `res.json`.
+     *
+     * The home directory comes from `os.homedir()` rather than a literal because that is the same
+     * value `defaultHomeDirs` derives its (module-cached) candidate list from, so this is
+     * deterministic on any runner without reaching into that cache. The pair is the point: the
+     * setting is the sole discriminator between the two cases, so neither passes if the censor is
+     * dropped from the route, and neither passes if it is replaced by blanket blanking.
+     */
+    it("censors the current user's home directory in the excerpt when the setting is on", async () => {
+      mockInstanceGeneralSettings.censorUsernameInLogs = true;
+      const homeDir = os.homedir();
+      mockWorkspaceOperationService.listForExecutionWorkspace.mockResolvedValue([
+        workspaceOperationFixture({ stdoutExcerpt: `cloned into ${homeDir}/checkout` }),
+      ]);
+
+      const res = await request(createApp("execution-workspaces")).get(
+        "/api/execution-workspaces/workspace-1/workspace-operations",
+      );
+
+      expect(res.status).toBe(200);
+      expect(res.body[0].stdoutExcerpt).not.toContain(homeDir);
+      // Censored, not blanked: the surrounding line survives so the excerpt stays readable.
+      expect(res.body[0].stdoutExcerpt).toContain("cloned into ");
+      expect(res.body[0].stdoutExcerpt).toContain("/checkout");
+    });
+
+    it("leaves the excerpt alone when the setting is off", async () => {
+      mockInstanceGeneralSettings.censorUsernameInLogs = false;
+      const homeDir = os.homedir();
+      mockWorkspaceOperationService.listForExecutionWorkspace.mockResolvedValue([
+        workspaceOperationFixture({ stdoutExcerpt: `cloned into ${homeDir}/checkout` }),
+      ]);
+
+      const res = await request(createApp("execution-workspaces")).get(
+        "/api/execution-workspaces/workspace-1/workspace-operations",
+      );
+
+      expect(res.status).toBe(200);
+      expect(res.body[0].stdoutExcerpt).toBe(`cloned into ${homeDir}/checkout`);
+    });
+
     /**
      * Driven rather than source-pinned, at Ally's ask on the review of `024a330c`, and it is the
      * one that most deserved it: this exact handler is where the door was found. It masked

server/src/routes/execution-workspaces.ts

@@ -13,6 +13,7 @@ import {
 import type { ExecutionWorkspace, ExecutionWorkspaceSummary, WorkspaceRuntimeDesiredState, WorkspaceRuntimeServiceStateMap } from "@paperclipai/shared";
 import { validate } from "../middleware/validate.js";
 import { accessService, executionWorkspaceService, heartbeatService, logActivity, workspaceOperationService } from "../services/index.js";
+import { instanceSettingsService } from "../services/instance-settings.js";
 import { mergeExecutionWorkspaceConfig, readExecutionWorkspaceConfig } from "../services/execution-workspaces.js";
 import { parseProjectExecutionWorkspacePolicy } from "../services/execution-workspace-policy.js";
 import { readProjectWorkspaceRuntimeConfig } from "../services/project-workspace-runtime-config.js";
@@ -32,6 +33,7 @@ import {
   collectExecutionWorkspaceCommandPaths,
 } from "./workspace-command-authz.js";
 import { assertCanManageExecutionWorkspaceRuntimeServices } from "./workspace-runtime-service-authz.js";
+import { redactCurrentUserValue } from "../log-redaction.js";
 import {
   publicExecutionWorkspace,
   publicExecutionWorkspaces,
@@ -51,6 +53,13 @@ export function executionWorkspaceRoutes(db: Db, opts: { pluginWorkerManager?: P
   const svc = executionWorkspaceService(db);
   const access = accessService(db);
   const workspaceOperationsSvc = workspaceOperationService(db);
+  const instanceSettings = instanceSettingsService(db);
+
+  async function getCurrentUserRedactionOptions() {
+    return {
+      enabled: (await instanceSettings.getGeneral()).censorUsernameInLogs,
+    };
+  }
   const heartbeat = heartbeatService(db, {
     pluginWorkerManager: opts.pluginWorkerManager,
   });
@@ -151,7 +160,15 @@ export function executionWorkspaceRoutes(db: Db, opts: { pluginWorkerManager?: P
     if (!(await assertExecutionWorkspaceReadAllowed(req, res, workspace.companyId))) return;
     const operations = await workspaceOperationsSvc.listForExecutionWorkspace(id);
     const viewer = await resolveWorkspaceRuntimeViewer(access, req, workspace.companyId);
-    res.json(publicWorkspaceOperations(operations, viewer));
+    // PEN-3205: same username censoring as the sibling list route on `routes/agents.ts`. Both
+    // answer with the same historical `WorkspaceOperation` rows including `stdoutExcerpt` /
+    // `stderrExcerpt`, which `publicWorkspaceOperation` deliberately does NOT withhold, so
+    // censoring on one route and not the other left the same bytes legible one URL over.
+    // New rows are censored at write time as well; this still covers rows stored before that.
+    res.json(redactCurrentUserValue(
+      publicWorkspaceOperations(operations, viewer),
+      await getCurrentUserRedactionOptions(),
+    ));
   });
 
   async function handleExecutionWorkspaceRuntimeCommand(req: Request, res: Response) {

server/src/services/heartbeat.ts

@@ -571,7 +571,6 @@ const TERMINAL_RUN_STATUSES = new Set(["succeeded", "failed", "cancelled", "time
 import { serverVersion } from "../version.js";
 
 const MAX_LIVE_LOG_CHUNK_BYTES = 8 * 1024;
-const MAX_PERSISTED_LOG_CHUNK_CHARS = 64 * 1024;
 const MAX_RUN_EVENT_PAYLOAD_STRING_CHARS = 16 * 1024;
 const MAX_RUN_EVENT_PAYLOAD_ARRAY_ITEMS = 50;
 
@@ -2145,7 +2144,6 @@ const activeRunExecutions = new Set<string>();
 // flips to "running"; this grace prevents reaping that startup race. Mirrors
 // the 5-min grace in cleanupManagedJobsWithoutRun.
 const ORPHANED_MANAGED_POD_REAP_GRACE_MS = 5 * 60 * 1000;
-const INLINE_BASE64_IMAGE_DATA_RE = /("type":"image","source":\{"type":"base64","data":")([A-Za-z0-9+/=]{1024,})(")/g;
 const SESSION_ISOLATION_KEY_PARAM = "paperclipIsolationKey";
 
 type RuntimeConfigSecretResolver = Pick<
@@ -4116,30 +4114,13 @@ export function boundHeartbeatRunEventPayloadForStorage(payload: Record<string,
   return parseObject(bounded) ?? { _truncated: true };
 }
 
-function redactInlineBase64ImageData(chunk: string) {
-  return chunk.replace(INLINE_BASE64_IMAGE_DATA_RE, (_match, prefix: string, data: string, suffix: string) =>
-    `${prefix}[omitted base64 image data: ${data.length} chars]${suffix}`,
-  );
-}
-
-export function compactRunLogChunk(chunk: string, maxChars = MAX_PERSISTED_LOG_CHUNK_CHARS) {
-  const normalized = redactSensitiveText(redactInlineBase64ImageData(chunk));
-  if (normalized.length <= maxChars) return normalized;
-
-  const headChars = Math.max(0, Math.floor(maxChars * 0.6));
-  const tailChars = Math.max(0, Math.floor(maxChars * 0.25));
-  const omittedChars = Math.max(0, normalized.length - headChars - tailChars);
-  const marker = `\n[paperclip truncated run log chunk: omitted ${omittedChars} chars]\n`;
-  return `${normalized.slice(0, headChars)}${marker}${normalized.slice(normalized.length - tailChars)}`;
-}
-
-export function sanitizeRunLogChunkForStorage(
-  chunk: string,
-  currentUserRedactionOptions: Parameters<typeof redactCurrentUserText>[1],
-  maxChars = MAX_PERSISTED_LOG_CHUNK_CHARS,
-) {
-  return compactRunLogChunk(redactCurrentUserText(chunk, currentUserRedactionOptions), maxChars);
-}
+// PEN-3205: `compactRunLogChunk` / `sanitizeRunLogChunkForStorage` now live in
+// `./log-chunk-sanitizer.js` so the workspace-operation write path can share the one
+// definition (importing them from here would close a cycle — see that module's header).
+// Imported for local use below and re-exported so existing importers and tests keep
+// their current entry point.
+import { compactRunLogChunk, sanitizeRunLogChunkForStorage } from "./log-chunk-sanitizer.js";
+export { compactRunLogChunk, sanitizeRunLogChunkForStorage };
 
 const SYNTHETIC_KEEPALIVE_RUN_LOG_LINE_RE =
   /^\[paperclip\] keepalive\b.*\bjob\b.*\brunning \(\d+s since last output\)$/;

server/src/services/log-chunk-sanitizer.ts

@@ -0,0 +1,48 @@
+import { redactCurrentUserText } from "../log-redaction.js";
+import { redactSensitiveText } from "../redaction.js";
+
+/**
+ * Shared write-time sanitizer for captured command/agent output.
+ *
+ * PEN-3205: this used to live inside `heartbeat.ts`, which made it unreachable from
+ * `workspace-operations.ts` — `heartbeat.ts` already imports that module, so importing
+ * back would close a cycle. The workspace-operation write path therefore grew its own
+ * weaker transform (`redactCurrentUserText` alone, i.e. username censoring with no
+ * secret scrub) and persisted unscrubbed output to both the excerpt columns and the
+ * log-store body.
+ *
+ * Extracting it here rather than re-deriving an equivalent in the second caller is
+ * deliberate: two independent sanitizers would be two oracles that can silently drift,
+ * and the weaker one would decide what a reviewer believes is covered. `heartbeat.ts`
+ * re-exports these so its existing importers and tests keep their current entry points.
+ */
+
+export const MAX_PERSISTED_LOG_CHUNK_CHARS = 64 * 1024;
+
+const INLINE_BASE64_IMAGE_DATA_RE =
+  /("type":"image","source":\{"type":"base64","data":")([A-Za-z0-9+/=]{1024,})(")/g;
+
+function redactInlineBase64ImageData(chunk: string) {
+  return chunk.replace(INLINE_BASE64_IMAGE_DATA_RE, (_match, prefix: string, data: string, suffix: string) =>
+    `${prefix}[omitted base64 image data: ${data.length} chars]${suffix}`,
+  );
+}
+
+export function compactRunLogChunk(chunk: string, maxChars = MAX_PERSISTED_LOG_CHUNK_CHARS) {
+  const normalized = redactSensitiveText(redactInlineBase64ImageData(chunk));
+  if (normalized.length <= maxChars) return normalized;
+
+  const headChars = Math.max(0, Math.floor(maxChars * 0.6));
+  const tailChars = Math.max(0, Math.floor(maxChars * 0.25));
+  const omittedChars = Math.max(0, normalized.length - headChars - tailChars);
+  const marker = `\n[paperclip truncated run log chunk: omitted ${omittedChars} chars]\n`;
+  return `${normalized.slice(0, headChars)}${marker}${normalized.slice(normalized.length - tailChars)}`;
+}
+
+export function sanitizeRunLogChunkForStorage(
+  chunk: string,
+  currentUserRedactionOptions: Parameters<typeof redactCurrentUserText>[1],
+  maxChars = MAX_PERSISTED_LOG_CHUNK_CHARS,
+) {
+  return compactRunLogChunk(redactCurrentUserText(chunk, currentUserRedactionOptions), maxChars);
+}