diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 2c9efcb88d9c..3ce60d645b00 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1,6 +1,7 @@ export { agentAdapterTypeSchema, optionalAgentAdapterTypeSchema } from "./adapter-type.js"; export { CREDENTIAL_VALUE_RES, + REDACTED_VALUE_SENTINEL, SENSITIVE_ENV_KEY_RE, isPlausiblySensitiveEnvValue, isSensitiveEnv, diff --git a/packages/shared/src/sensitive-env.ts b/packages/shared/src/sensitive-env.ts index 65b1fc674335..bb64f1d6a314 100644 --- a/packages/shared/src/sensitive-env.ts +++ b/packages/shared/src/sensitive-env.ts @@ -1,3 +1,13 @@ +/** + * The single spelling of a withheld value across the whole product. + * + * BLO-34631 review: the server masks rather than empties a withheld value, so a reader can tell + * "withheld" from "absent" — but that contract only holds if the UI recognises the same string the + * server writes. Both sides had their own literal, and nothing pinned them together: change one and + * the viewer silently falls back to rendering a withheld log as an empty one. + */ +export const REDACTED_VALUE_SENTINEL = "***REDACTED***"; + /** Env-var names that conventionally hold credentials. */ export const SENSITIVE_ENV_KEY_RE = /token(?:$|[-_])|api[-_]?key|access[-_]?token|auth(?:entication|_?token)?|authorization|bearer|secret|passwd|password|credential|jwt|private[-_]?key|cookie|connectionstring/i; diff --git a/server/src/__tests__/agent-live-run-routes.test.ts b/server/src/__tests__/agent-live-run-routes.test.ts index a8850e340b28..cb437956b4d5 100644 --- a/server/src/__tests__/agent-live-run-routes.test.ts +++ b/server/src/__tests__/agent-live-run-routes.test.ts @@ -29,8 +29,61 @@ const mockInstanceSettingsService = vi.hoisted(() => ({ })); const mockLogActivity = vi.hoisted(() => vi.fn()); +/** + * BLO-34631. `/workspace-operations/:operationId/log` was the only one of the four run/operation + * log surfaces with neither a read-time projection nor an access audit — it answered with the + * stored chunk verbatim and wrote nothing. These cover both, plus the entitled control. + * + * Every value below is invented; no real credential, command or path is quoted, per the parent + * series' standing prohibition. + */ +const mockWorkspaceOperationService = vi.hoisted(() => ({ + getById: vi.fn(), + readLog: vi.fn(), +})); + +/** + * Flippable so the entitled and unentitled readers are separate cases. The default allows + * everything, which models the board actor the rest of this file drives with; denying only + * `workspace_runtime:read` models a standard same-company agent, which holds `company_scope:read` + * and `runtime:manage` but deliberately NOT this one (PEN-2852, `allow_company_agent`). + */ +const mockAccessDecide = vi.hoisted(() => vi.fn()); + const routeAgentId = "11111111-1111-4111-8111-111111111111"; +const WORKSPACE_OPERATION_LOG_SENTINEL = + "TOKEN_FIXTURE=sentinel-operation-log-not-a-real-credential ./deploy.sh"; + +function workspaceOperationLogFixture(overrides: Record = {}) { + return { + id: "operation-1", + companyId: "company-1", + heartbeatRunId: "run-1", + logStore: "local_file", + logRef: "logs/operation-1.ndjson", + ...overrides, + }; +} + +function allowEveryAction() { + mockAccessDecide.mockImplementation(async (input: { action?: string }) => ({ + allowed: true, + action: input.action, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant.", + })); +} + +function denyWorkspaceRuntimeRead() { + mockAccessDecide.mockImplementation(async (input: { action?: string }) => ({ + allowed: input.action !== "workspace_runtime:read", + action: input.action, + reason: "test", + explanation: "Allowed by test mock.", + })); +} + function registerModuleMocks() { vi.doMock("../routes/authz.js", async () => vi.importActual("../routes/authz.js")); @@ -65,12 +118,7 @@ function registerModuleMocks() { agentInstructionsService: () => ({}), accessService: () => ({ canUser: vi.fn(async () => true), - decide: vi.fn(async (input: { action?: string }) => ({ - allowed: true, - action: input.action, - reason: "allow_explicit_grant", - explanation: "Allowed by test grant.", - })), + decide: mockAccessDecide, hasPermission: vi.fn(async () => true), }), approvalService: () => ({}), @@ -83,7 +131,7 @@ function registerModuleMocks() { logActivity: mockLogActivity, secretService: () => ({}), syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config), - workspaceOperationService: () => ({}), + workspaceOperationService: () => mockWorkspaceOperationService, })); vi.doMock("../adapters/index.js", () => ({ @@ -290,6 +338,15 @@ describe("agent live run routes", () => { registerModuleMocks(); vi.clearAllMocks(); mockLogActivity.mockResolvedValue(undefined); + allowEveryAction(); + mockWorkspaceOperationService.getById.mockResolvedValue(workspaceOperationLogFixture()); + mockWorkspaceOperationService.readLog.mockResolvedValue({ + operationId: "operation-1", + store: "local_file", + logRef: "logs/operation-1.ndjson", + content: WORKSPACE_OPERATION_LOG_SENTINEL, + nextOffset: 9, + }); mockIssueService.getByIdentifier.mockResolvedValue({ id: "issue-1", companyId: "company-1", @@ -544,6 +601,167 @@ describe("agent live run routes", () => { expect(mockLogActivity.mock.calls[0]?.[1]?.details).not.toHaveProperty("logRef"); }); + /** + * BLO-34631 AC 1/2/3/4. Four cases, one per control, so a pass names which one it proved. + * + * The sentinel is the log CONTENT rather than a row field: the route answered with + * `readLog`'s result unprojected, so a fixture whose content is inert could not show it. + */ + it("withholds workspace-operation log content from a reader without workspace_runtime:read", async () => { + denyWorkspaceRuntimeRead(); + + const res = await requestApp( + await createApp({}, { + type: "agent", + agentId: routeAgentId, + companyId: "company-1", + companyIds: ["company-1"], + source: "agent_key", + runId: "actor-run-1", + }), + (baseUrl) => request(baseUrl).get("/api/workspace-operations/operation-1/log?offset=0&limitBytes=64"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(JSON.stringify(res.body)).not.toContain(WORKSPACE_OPERATION_LOG_SENTINEL); + // Masked, not emptied: a withheld reader must still tell "logged nothing" from "withheld". + expect(res.body.content).toBe("***REDACTED***"); + // The opaque handles stay — the route they point at is the one that now withholds. + expect(res.body.logRef).toBe("logs/operation-1.ndjson"); + // AC 2 + review: the access check passed, so this is `result: "allowed"` — but nothing was + // disclosed. Without `withheld` the record is indistinguishable from a real disclosure, and + // "who read this log" over-reports. + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + action: "workspace_operation.log_accessed", + details: expect.objectContaining({ result: "allowed", withheld: true }), + })); + }); + + it("discloses workspace-operation log content to a reader holding workspace_runtime:read", async () => { + const res = await requestApp( + await createApp(), + (baseUrl) => request(baseUrl).get("/api/workspace-operations/operation-1/log?offset=7&limitBytes=64"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body.content).toBe(WORKSPACE_OPERATION_LOG_SENTINEL); + expect(mockWorkspaceOperationService.readLog).toHaveBeenCalledWith("operation-1", { + offset: 7, + limitBytes: 64, + }); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + companyId: "company-1", + actorType: "user", + actorId: "local-board", + action: "workspace_operation.log_accessed", + entityType: "workspace_operation", + entityId: "operation-1", + // The operation's own run, so an audit reader can join back to the run that produced it. + runId: "run-1", + details: expect.objectContaining({ + result: "allowed", + actorSource: "local_implicit", + offset: 7, + limitBytes: 64, + logStore: "local_file", + // Paired with the withheld case above: the flag is what separates a real disclosure from + // a masked read, so it has to be asserted on both sides or it proves nothing. + withheld: false, + }), + })); + expect(mockLogActivity.mock.calls[0]?.[1]?.details).not.toHaveProperty("content"); + }); + + it("audits denied workspace-operation log access without reading content", async () => { + const res = await requestApp( + await createApp({}, { + type: "agent", + agentId: routeAgentId, + companyId: "company-2", + companyIds: ["company-2"], + source: "agent_key", + runId: "actor-run-1", + }), + (baseUrl) => request(baseUrl).get("/api/workspace-operations/operation-1/log?offset=4&limitBytes=32"), + ); + + // Cross-tenant stays a 404 so the route is not an existence oracle, but the attempt is recorded. + expect(res.status).toBe(404); + expect(mockWorkspaceOperationService.readLog).not.toHaveBeenCalled(); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + companyId: "company-1", + actorType: "agent", + actorId: routeAgentId, + agentId: routeAgentId, + action: "workspace_operation.log_accessed", + entityType: "workspace_operation", + entityId: "operation-1", + details: expect.objectContaining({ + result: "denied", + actorSource: "agent_key", + actorRunId: "actor-run-1", + offset: 4, + limitBytes: 32, + }), + })); + }); + + /** + * AC 1. Write-time username censoring is not retroactive, so a chunk stored before it landed is + * in the store uncensored and crossed verbatim on this route only. Paired with the off-case + * below: the setting is the sole discriminator, so neither passes if the read-time censor is + * dropped, and neither passes if it is replaced by blanket blanking. + * + * `os.homedir()` rather than a literal, because that is the value `defaultHomeDirs` derives its + * module-cached candidate list from. + */ + it("censors the current user's home directory in stored log content when the setting is on", async () => { + const { default: os } = await vi.importActual("node:os"); + const homeDir = os.homedir(); + mockInstanceSettingsService.getGeneral.mockResolvedValue({ + censorUsernameInLogs: true, + feedbackDataSharingPreference: "prompt", + }); + mockWorkspaceOperationService.readLog.mockResolvedValue({ + operationId: "operation-1", + store: "local_file", + logRef: "logs/operation-1.ndjson", + content: `cloned into ${homeDir}/checkout`, + nextOffset: 9, + }); + + const res = await requestApp( + await createApp(), + (baseUrl) => request(baseUrl).get("/api/workspace-operations/operation-1/log"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body.content).not.toContain(homeDir); + // Censored, not blanked: the surrounding line survives so the log stays readable. + expect(res.body.content).toContain("cloned into "); + expect(res.body.content).toContain("/checkout"); + }); + + it("leaves stored log content alone when the censor setting is off", async () => { + const { default: os } = await vi.importActual("node:os"); + const homeDir = os.homedir(); + mockWorkspaceOperationService.readLog.mockResolvedValue({ + operationId: "operation-1", + store: "local_file", + logRef: "logs/operation-1.ndjson", + content: `cloned into ${homeDir}/checkout`, + nextOffset: 9, + }); + + const res = await requestApp( + await createApp(), + (baseUrl) => request(baseUrl).get("/api/workspace-operations/operation-1/log"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body.content).toBe(`cloned into ${homeDir}/checkout`); + }); + it("caps company live run polling by default", async () => { const rows = Array.from({ length: 75 }, (_, index) => ({ id: `run-${index}`, diff --git a/server/src/__tests__/workspace-runtime-response-withholding.test.ts b/server/src/__tests__/workspace-runtime-response-withholding.test.ts index 03445d293bc3..4e56f9aa9def 100644 --- a/server/src/__tests__/workspace-runtime-response-withholding.test.ts +++ b/server/src/__tests__/workspace-runtime-response-withholding.test.ts @@ -56,6 +56,16 @@ const OPERATION_COMMAND_SENTINEL = "TOKEN_FIXTURE=sentinel-operation-command-not const OPERATION_CWD_SENTINEL = "/fixture/sentinel-operation-cwd"; const OPERATION_METADATA_SENTINEL = "/fixture/sentinel-operation-worktree-path"; +/** + * BLO-34631. Command *output*, and distinct from the `command` sentinel on purpose: the whole + * question this ticket settled is whether withholding the command while disclosing its output is a + * boundary or a gap. It resolved to "disclosed, deliberately" on a consumer survey, so these two + * are the values an unentitled reader is expected to RECEIVE — named for the decision they pin + * rather than for egress. Both invented. + */ +const OPERATION_STDOUT_SENTINEL = "sentinel-operation-stdout-disclosed-by-design"; +const OPERATION_STDERR_SENTINEL = "sentinel-operation-stderr-disclosed-by-design"; + /** * PEN-3073. The lifecycle command scalars that sit BESIDE `workspaceRuntime` on the same config * object, and their siblings on the three nouns that carry the same strings elsewhere. Each gets its @@ -839,11 +849,48 @@ describe("workspace runtime withholding boundary (PEN-2852)", () => { expect(res.body[0].cwd).toBe(OPERATION_CWD_SENTINEL); }); + /** + * BLO-34631 AC 3, resolved as DISCLOSED — pinned by a test because it is a decision, not an + * omission, and the next reader of `publicWorkspaceOperation` will otherwise see `command` and + * `cwd` masked beside two unmasked siblings and "fix" the asymmetry. + * + * The CTO lean was to withhold, on the symmetry argument that the output of a withheld command + * discloses the command. AC 3 made that falsifiable by a consumer survey and the survey + * falsifies it: `POST /execution-workspaces/:id/runtime-services/:action` answers with this + * same projection, it is the backing call for the MCP tool + * `paperclipControlIssueWorkspaceServices`, and same-company agents deliberately lack + * `workspace_runtime:read` — so masking here hands an agent `***REDACTED***` for the output of + * the command it just triggered. + * + * The contrast in the last two assertions is the whole point: this reader is unentitled, and + * the SAME row still withholds `command`/`cwd`. So this case cannot pass by the projection + * being skipped, only by the excerpts being deliberately exempt from it. + */ + it("discloses the operation excerpts to a reader without workspace_runtime:read", async () => { + mockWorkspaceOperationService.listForExecutionWorkspace.mockResolvedValue([ + workspaceOperationFixture({ + stdoutExcerpt: OPERATION_STDOUT_SENTINEL, + stderrExcerpt: OPERATION_STDERR_SENTINEL, + }), + ]); + + 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(OPERATION_STDOUT_SENTINEL); + expect(res.body[0].stderrExcerpt).toBe(OPERATION_STDERR_SENTINEL); + expect(res.body[0].command).toBe(REDACTED_EVENT_VALUE); + expect(res.body[0].cwd).toBe(REDACTED_EVENT_VALUE); + }); + /** * 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 rest, so `stdoutExcerpt` crosses this route UNMASKED by design (BLO-34631 surveyed that + * and kept it) — 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 diff --git a/server/src/redaction.ts b/server/src/redaction.ts index b03fdb1d2679..a533fd8572c3 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -1,5 +1,5 @@ import { redactCommandText } from "@paperclipai/adapter-utils"; -import { envBindingSecretRefSchema, envBindingUserSecretRefSchema } from "@paperclipai/shared"; +import { envBindingSecretRefSchema, envBindingUserSecretRefSchema, REDACTED_VALUE_SENTINEL } from "@paperclipai/shared"; /** * Tier 1: key-name stems with no ambiguous benign reading (BLO-20810 / CEO @@ -191,7 +191,7 @@ const SECRET_TEXT_HINTS = [ "xox", "github_pat_", ] as const; -export const REDACTED_EVENT_VALUE = "***REDACTED***"; +export const REDACTED_EVENT_VALUE = REDACTED_VALUE_SENTINEL; function maybeContainsSecretText(input: string) { const lower = input.toLowerCase(); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index bc7c214efac8..7640ee4116e1 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -2,7 +2,7 @@ import { Router, type Request, type Response } from "express"; import { generateKeyPairSync, randomUUID } from "node:crypto"; import path from "node:path"; import type { Db } from "@paperclipai/db"; -import { REDACTED_EVENT_VALUE, isPlainObject, redactAgentConfigPayload, redactEventPayload } from "../redaction.js"; +import { REDACTED_EVENT_VALUE, isPlainObject, maskWorkspaceRuntimeTextForRead, redactAgentConfigPayload, redactEventPayload } from "../redaction.js"; import { diffAgentAdapterSecretBindings } from "../services/agent-secret-bindings.js"; import { agentRuntimeState, agents as agentsTable, companies, heartbeatRuns, issues as issuesTable, projects as projectsTable } from "@paperclipai/db"; import { and, asc, desc, eq, gte, inArray, not, or, sql } from "drizzle-orm"; @@ -366,33 +366,67 @@ export function agentRoutes( const instanceSettings = instanceSettingsService(db); const strictSecretsMode = process.env.PAPERCLIP_SECRETS_STRICT_MODE === "true"; - async function logRunLogAccessAudit( + /** + * BLO-34631. Shared by both raw-log surfaces. `/workspace-operations/:id/log` serves the same + * class of bytes as `/heartbeat-runs/:runId/log` — stored log content, not a projected row — and + * was the only one of the four run/operation log surfaces writing no audit record at all, so + * "who read this log" had no answer for it. Both call sites audit allowed AND denied reads. + */ + async function logLogAccessAudit( req: Request, - run: { id: string; companyId: string; logStore: string | null }, + entity: { + companyId: string; + entityType: "heartbeat_run" | "workspace_operation"; + entityId: string; + runId: string | null; + logStore: string | null; + }, result: "allowed" | "denied", - opts: { offset: number; limitBytes: number }, + opts: { offset: number; limitBytes: number; withheld?: boolean }, ) { const actor = getRunLogAuditActor(req); await logActivity(db, { - companyId: run.companyId, + companyId: entity.companyId, actorType: actor.actorType, actorId: actor.actorId, agentId: actor.agentId, - runId: run.id, - action: "heartbeat.run_log_accessed", - entityType: "heartbeat_run", - entityId: run.id, + runId: entity.runId, + action: entity.entityType === "heartbeat_run" + ? "heartbeat.run_log_accessed" + : "workspace_operation.log_accessed", + entityType: entity.entityType, + entityId: entity.entityId, details: { result, actorSource: actor.actorSource, actorRunId: actor.actorRunId, offset: opts.offset, limitBytes: opts.limitBytes, - logStore: run.logStore, + logStore: entity.logStore, + // BLO-34631 review: a withheld read and a real disclosure are both `result: "allowed"` — + // the access check decides reachability, the entitlement decides the bytes. Record which + // one happened so "who read this log" is answerable without re-deriving the reader's + // grants after the fact. Absent on surfaces that apply no read-time projection. + ...(opts.withheld === undefined ? {} : { withheld: opts.withheld }), }, }); } + async function logRunLogAccessAudit( + req: Request, + run: { id: string; companyId: string; logStore: string | null }, + result: "allowed" | "denied", + opts: { offset: number; limitBytes: number }, + ) { + await logLogAccessAudit(req, { + companyId: run.companyId, + entityType: "heartbeat_run", + entityId: run.id, + runId: run.id, + logStore: run.logStore, + }, result, opts); + } + async function assertAgentEnvironmentSelection( companyId: string, adapterType: string, @@ -5065,18 +5099,67 @@ export function agentRoutes( router.get("/workspace-operations/:operationId/log", async (req, res) => { const operationId = req.params.operationId as string; - const operation = await getAccessibleResource(req, res, workspaceOperations.getById(operationId), "Workspace operation not found"); - if (!operation) return; - const offset = Number(req.query.offset ?? 0); + const normalizedOffset = Number.isFinite(offset) ? offset : 0; const limitBytes = readRunLogLimitBytes(req.query.limitBytes); + const operation = await workspaceOperations.getById(operationId); + if (!operation) { + res.status(404).json({ error: "Workspace operation not found" }); + return; + } + + const audit = (result: "allowed" | "denied", withheld?: boolean) => logLogAccessAudit(req, { + companyId: operation.companyId, + entityType: "workspace_operation", + entityId: operation.id, + runId: operation.heartbeatRunId, + logStore: operation.logStore, + }, result, { offset: normalizedOffset, limitBytes, withheld }); + + // Same shape as `/heartbeat-runs/:runId/log` rather than `getAccessibleResource`: keep the + // cross-tenant 404 so this route is not an existence oracle, without silently dropping the + // denied access event. `getAccessibleResource`'s own doc block names audit-logged denials as + // the case that should compose `hasCompanyAccess` directly. + if (!hasCompanyAccess(req, operation.companyId)) { + await audit("denied"); + res.status(404).json({ error: "Workspace operation not found" }); + return; + } + + try { + assertCompanyAccess(req, operation.companyId); + } catch (error) { + await audit("denied"); + throw error; + } + + // Viewer first: the audit record has to say whether this read actually disclosed anything, and + // only the entitlement knows that. Resolved after the two denial paths, so a caller who never + // clears company access costs no entitlement lookup. + const viewer = await resolveWorkspaceRuntimeViewer(access, req, operation.companyId); + await audit("allowed", !viewer.revealRuntimeConfig); const result = await workspaceOperations.readLog(operationId, { - offset: Number.isFinite(offset) ? offset : 0, + offset: normalizedOffset, limitBytes, }); res.set("Cache-Control", "no-cache, no-store"); - res.json(result); + // BLO-34631. `content` is the stored chunk verbatim — the write-time sanitizer is a heuristic + // secret matcher, not a withholding boundary, so it lets host paths, repo layout and any + // operator command echoed by `set -x` through. That is the same text `publicWorkspaceOperation` + // withholds one projection over, so it is withheld on the same entitlement. Masked rather than + // emptied, matching `publicRuntimeServices`: a withheld reader can still tell "this operation + // logged nothing" from "the log was withheld". `redactCurrentUserValue` still runs for the + // entitled reader, because write-time username censoring is not retroactive. + res.json(redactCurrentUserValue( + { + ...result, + content: viewer.revealRuntimeConfig + ? result.content + : maskWorkspaceRuntimeTextForRead(result.content), + }, + await getCurrentUserRedactionOptions(), + )); }); router.get("/issues/:issueId/live-runs", async (req, res) => { diff --git a/server/src/routes/execution-workspaces.ts b/server/src/routes/execution-workspaces.ts index 98d9e4d5b7aa..929b3f9052a7 100644 --- a/server/src/routes/execution-workspaces.ts +++ b/server/src/routes/execution-workspaces.ts @@ -163,9 +163,10 @@ export function executionWorkspaceRoutes(db: Db, opts: { pluginWorkerManager?: P const viewer = await resolveWorkspaceRuntimeViewer(access, req, workspace.companyId); // 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. + // `stderrExcerpt`, which `publicWorkspaceOperation` deliberately does NOT withhold (BLO-34631 + // surveyed that and found the agent's own MCP control path reads them back), 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(), diff --git a/server/src/routes/workspace-response.ts b/server/src/routes/workspace-response.ts index 5941cffa61b6..bc9f7dbc27a8 100644 --- a/server/src/routes/workspace-response.ts +++ b/server/src/routes/workspace-response.ts @@ -117,8 +117,11 @@ import type { accessService } from "../services/index.js"; * caller already holds, while `cleanup_command` is the operator's own string. * - `ExecutionWorkspaceStrategy.type` / `.runScope` — closed enums. `.baseRef` / `.branchTemplate` * are git refs and templates the branch-naming UI renders and the agent needs to name its branch. - * - `stdoutExcerpt` / `stderrExcerpt` / `logRef` on operations — command *output*, not a copy of a - * declared-withheld value (CTO Ruling F §4, BLO-33407). Unchanged by this ticket. + * - `stdoutExcerpt` / `stderrExcerpt` / `logRef` / `logStore` on operations — command *output* and + * opaque handles. BLO-34631 surveyed the consumers and kept the excerpts disclosed: the agent's + * own MCP control path reads them back for the command it just triggered (see + * `publicWorkspaceOperation`). The route `logRef` points at does withhold its content, because + * that one has no agent consumer. * * ### PEN-3252 — the bypass this module recorded as open, now closed * @@ -442,9 +445,42 @@ export function publicRuntimeServices( * unentitled operator must still be able to see that an operation ran and how it ended — withholding * the operator's text is the point, hiding the fact of execution is not. * - * `stdoutExcerpt` / `stderrExcerpt` / `logRef` are deliberately NOT withheld here. They are command - * *output*, not a copy of a declared-withheld value, so they sit on the far side of BLO-33568's rule - * and are a product decision rather than a projection bug (CTO Ruling F §4, BLO-33407). + * `stdoutExcerpt` / `stderrExcerpt` are NOT withheld, and as of BLO-34631 that is a measured + * decision rather than the inherited "command output is not a copy of a declared-withheld value" + * reading (CTO Ruling F §4, BLO-33407). + * + * The symmetry argument for withholding them is real and was the CTO's stated lean: the output of a + * withheld command can disclose the command — shells echo, `npm` prints the script it runs, `set -x` + * prints everything — and the only control standing over the bytes is the write-time + * `redactSensitiveText` heuristic, which matches env-dump assignments, JSON secret fields and URI + * credentials and nothing else. Host paths, repo layout and an operator's `cleanupCommand` cross it + * intact. BLO-34631 AC 3 made that lean explicitly falsifiable by a consumer survey, and the survey + * falsifies it: + * + * - `POST /execution-workspaces/:id/runtime-services/:action` (`routes/execution-workspaces.ts`) and + * `POST /projects/:id/workspaces/:workspaceId/runtime-services/:action` (`routes/projects.ts`) + * answer with `operation: publicWorkspaceOperation(operation, viewer)`, where the operation is the + * one the caller just triggered and `stdout`/`stderr` are captured synchronously from it. + * - The first of those is the backing call for the MCP tool `paperclipControlIssueWorkspaceServices` + * (`packages/mcp-server/src/tools.ts`), which returns the response JSON verbatim to the calling + * agent, and both it and `GET /heartbeat-runs/:runId/workspace-operations` are on the sandbox + * callback bridge allowlist (`packages/adapter-utils/src/sandbox-callback-bridge.ts`). + * - Same-company agents deliberately lack `workspace_runtime:read` (PEN-2852), so masking here + * hands an agent `***REDACTED***` for the output of the command it just ran. `status`/`exitCode` + * survive, so it would still learn pass/fail — but not why, which is exactly the "debugging a + * failed provision" flow BLO-34631 named as the finding that settles this. + * + * So the disclosure is deliberate and recorded, not an unexamined pass-through. The residual it + * accepts: an operator's service command echoed into its own output still reaches an unentitled + * reader, and the write-time scrub is not a boundary. Narrowing it is a product decision that has to + * keep the agent's own command-result path readable — masking the read/list routes alone would split + * the same field across routes, which is the failure mode this series exists to close. + * + * `logRef` / `logStore` stay for a different reason: they are opaque handles, and the route they + * point at (`/workspace-operations/:id/log`) DOES withhold its content on this entitlement as of + * BLO-34631 — that route has no agent consumer (no MCP tool, not bridge-allowlisted; only + * `ui/src/pages/AgentDetail.tsx` and `paperclip run workspace-log` read it). Masking a pointer whose + * route still served the bytes would have been theatre. */ export function publicWorkspaceOperation( operation: WorkspaceOperation, diff --git a/ui/src/pages/AgentDetail.tsx b/ui/src/pages/AgentDetail.tsx index a7e5629bde57..0c42f1084b3b 100644 --- a/ui/src/pages/AgentDetail.tsx +++ b/ui/src/pages/AgentDetail.tsx @@ -106,6 +106,7 @@ import { type WorkspaceOperation, isResponsibleUserDenialCode, isSensitiveEnv, + REDACTED_VALUE_SENTINEL, responsibleUserLabel, } from "@paperclipai/shared"; import { ResponsibleUserDenialNotice } from "../components/ResponsibleUserDenialNotice"; @@ -133,7 +134,9 @@ const runStatusIcons: Record )} {!isLoading && !error && chunks.length === 0 && ( -
No persisted log lines.
+
+ {/* BLO-34631: the API masks withheld log content rather than emptying it, so the + viewer has to tell the two apart — `parseStoredLogContent` yields no chunks for + either. */} + {logData?.content === REDACTED_ENV_VALUE + ? "Log content withheld — requires workspace runtime access." + : "No persisted log lines."} +
)} {chunks.length > 0 && (