From 44009421e22c41068f85ad097a6a2b1cacc4af4b Mon Sep 17 00:00:00 2001 From: pt-act <211776491+pt-act@users.noreply.github.com> Date: Sun, 30 Aug 2026 16:01:06 +0100 Subject: [PATCH] fix(local,keychain,api): prefer keychain for secrets, disable stdio MCP by default, surface approval-record failures --- .../secure-secrets-and-stdio-defaults.md | 22 +++ apps/local/executor.config.ts | 33 +++- apps/local/src/executor.config.test.ts | 50 ++++++ e2e/local/local-server.ts | 3 + .../executions.approval-logging.test.ts | 150 ++++++++++++++++++ packages/core/api/src/handlers/executions.ts | 25 ++- packages/plugins/keychain/src/index.ts | 2 + .../keychain/src/keyring.availability.test.ts | 37 +++++ packages/plugins/keychain/src/keyring.ts | 27 ++++ 9 files changed, 339 insertions(+), 10 deletions(-) create mode 100644 .changeset/secure-secrets-and-stdio-defaults.md create mode 100644 apps/local/src/executor.config.test.ts create mode 100644 packages/core/api/src/handlers/executions.approval-logging.test.ts create mode 100644 packages/plugins/keychain/src/keyring.availability.test.ts diff --git a/.changeset/secure-secrets-and-stdio-defaults.md b/.changeset/secure-secrets-and-stdio-defaults.md new file mode 100644 index 0000000000..45b45be87c --- /dev/null +++ b/.changeset/secure-secrets-and-stdio-defaults.md @@ -0,0 +1,22 @@ +--- +"@executor-js/local-app": patch +"@executor-js/plugin-keychain": patch +"@executor-js/api": patch +--- + +fix: prefer the OS keychain for secrets, disable stdio MCP by default, and log approval-record failures + +- The local app now registers the keychain credential provider before the file + store, so minted OAuth tokens land in the OS keychain on platforms where it + is durable (macOS/Windows). On headless/Linux hosts where the keychain probe + fails, the file store remains the effective default — behavior is unchanged + there, and a new `describeKeychainAvailability()` helper encodes the platform + truth that drives the ordering. +- `dangerouslyAllowStdioMCP` now defaults to `false` in the shipped local + config. Stdio MCP servers spawn local subprocesses; enabling the flag + explicitly is required for trusted local contexts, and the MCP plugin + already rejects stdio connections with a clear error when disabled. +- Approval-record persistence failures (the best-effort durable record behind + artifact approvals) are no longer swallowed silently: the failure cause is + captured through the host's error-capture channel so operators can see when + the restart-recovery fallback degrades. Execution behavior is unchanged. diff --git a/apps/local/executor.config.ts b/apps/local/executor.config.ts index fa44b48c72..fc000fbf02 100644 --- a/apps/local/executor.config.ts +++ b/apps/local/executor.config.ts @@ -36,16 +36,35 @@ export default defineExecutorConfig({ presets: [...googleCatalog, ...microsoftCatalog], specFormats: [googleDiscoveryAdapter, microsoftGraphAdapter], }), - mcpHttpPlugin({ dangerouslyAllowStdioMCP: true }), + mcpHttpPlugin({ + // Stdio MCP servers spawn arbitrary local processes. Default OFF for + // the shipped local app; opt in explicitly only for trusted local + // contexts (the e2e harness sets EXECUTOR_ALLOW_STDIO_MCP=1 for its + // dedicated stdio scenarios). The MCP plugin itself rejects stdio + // connections with a clear error when this flag is false (see + // plugin.ts resolveConnector). + dangerouslyAllowStdioMCP: process.env.EXECUTOR_ALLOW_STDIO_MCP === "1", + }), graphqlHttpPlugin(), toolkitsPlugin({ activeToolkitSlug }), - // The durable file store must register before keychain: the first - // writable provider becomes the default for minted OAuth tokens, and on - // sandbox/headless hosts the keychain is an in-memory keyring that a - // stop/recreate wipes while only EXECUTOR_DATA_DIR is persisted. - // Keychain stays registered for explicit external refs. - fileSecretsPlugin(), + // Secrets ordering — CAUSAL KNOWLEDGE, encoded: + // + // The FIRST writable credential provider becomes the default for + // minted OAuth tokens. On macOS/Windows the OS keychain is a durable + // persistent store, so keychain must register FIRST to become the + // default there. On Linux/headless/sandbox hosts the keychain probe + // (write+delete sentinel) fails or degrades to an in-memory keyring + // that a stop/recreate wipes while only EXECUTOR_DATA_DIR persists — + // the keychain plugin's credentialProviders() then returns [] and the + // file store naturally becomes the default. This ordering therefore + // yields "keychain default where durable, file fallback where not" + // without any runtime switch. + // + // If the platform truth changes (e.g. a durable Linux backend ships), + // update describeKeychainAvailability() in @executor-js/plugin-keychain + // — not this comment. keychainPlugin(), + fileSecretsPlugin(), onepasswordHttpPlugin(), desktopSettingsPlugin({ webBaseUrl: diff --git a/apps/local/src/executor.config.test.ts b/apps/local/src/executor.config.test.ts new file mode 100644 index 0000000000..98a8d86341 --- /dev/null +++ b/apps/local/src/executor.config.test.ts @@ -0,0 +1,50 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "@effect/vitest"; + +import executorConfig from "../executor.config"; + +// --------------------------------------------------------------------------- +// Pins the shipped local config's secrets ordering (keychain before file — +// the first writable provider becomes the default for minted OAuth tokens) +// and the stdio-MCP default (off). +// --------------------------------------------------------------------------- + +describe("executor.config secrets ordering", () => { + it("registers keychain before fileSecrets (keychain wins as default when reachable)", () => { + const plugins = executorConfig.plugins(); + const names = plugins.map((p) => p.id); + const keychainIdx = names.indexOf("keychain"); + const fileIdx = names.indexOf("fileSecrets"); + expect(keychainIdx, "keychain must be present").toBeGreaterThan(-1); + expect(fileIdx, "fileSecrets must be present").toBeGreaterThan(-1); + expect(keychainIdx).toBeLessThan(fileIdx); + }); + + it("keeps the full plugin set intact (no plugin dropped by the reorder)", () => { + const plugins = executorConfig.plugins(); + const ids = plugins.map((p) => p.id).sort(); + expect(ids).toEqual( + [ + "openapi", + "mcp", + "graphql", + "toolkits", + "keychain", + "fileSecrets", + "onepassword", + "desktop-settings", + ].sort(), + ); + }); + + it("does not enable stdio MCP in the shipped config (config-side contract)", () => { + // The plugin's runtime default is `?? false` (plugin.ts:751); the + // config-side contract is that the shipped local app does not pass + // `dangerouslyAllowStdioMCP: true`. Assert the source literal so a + // future re-enable trips this test. + const source = readFileSync(join(import.meta.dirname, "..", "executor.config.ts"), "utf8"); + expect(source).not.toContain("dangerouslyAllowStdioMCP: true"); + }); +}); diff --git a/e2e/local/local-server.ts b/e2e/local/local-server.ts index 798a62cc9f..09256b68c7 100644 --- a/e2e/local/local-server.ts +++ b/e2e/local/local-server.ts @@ -115,6 +115,9 @@ export const withLocalServer = ( EXECUTOR_DEV: "1", EXECUTOR_DATA_DIR: dataDir, EXECUTOR_SCOPE_DIR: dataDir, + // The stdio-MCP e2e scenarios need stdio enabled; the shipped + // local app defaults it off. Scoped to this harness only. + EXECUTOR_ALLOW_STDIO_MCP: "1", ...options?.env, }, record: join(runDir, options?.castName ?? "terminal.cast"), diff --git a/packages/core/api/src/handlers/executions.approval-logging.test.ts b/packages/core/api/src/handlers/executions.approval-logging.test.ts new file mode 100644 index 0000000000..7696fba2d3 --- /dev/null +++ b/packages/core/api/src/handlers/executions.approval-logging.test.ts @@ -0,0 +1,150 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Cause, Effect, Layer } from "effect"; +import { HttpRouter, HttpServer } from "effect/unstable/http"; +import { HttpApi, HttpApiBuilder } from "effect/unstable/httpapi"; + +import type { Executor } from "@executor-js/sdk"; + +import { ExecutionsApi } from "../executions/api"; +import { ExecutionsHandlers } from "./executions"; +import { ExecutionEngineService, ExecutorService } from "../services"; +import { ErrorCapture } from "../observability"; +import { StorageError } from "@executor-js/sdk"; + +// --------------------------------------------------------------------------- +// recordPendingApproval must NOT silently swallow persistence failures: +// the cause is captured through the host's ErrorCapture seam, and the +// execution outcome is unaffected (the artifact pause still returns +// "paused", not a 500). +// --------------------------------------------------------------------------- + +// A pendingApprovals store whose put() always fails — simulating a storage +// hiccup at the moment of recording the durable approval record. Fails with a +// REAL StorageError (tagged, message + cause) so the handler's capture path +// sees exactly what a live storage hiccup produces. +const failingPendingApprovals = { + put: () => Effect.fail(new StorageError({ message: "simulated storage failure", cause: null })), + consume: () => Effect.succeed(null), + discard: () => Effect.void, +}; + +const capturingErrors: string[] = []; +const capturingErrorCapture = Layer.succeed(ErrorCapture, { + captureException: (cause) => + Effect.sync(() => { + capturingErrors.push(Cause.pretty(cause)); + return "trace-test"; + }), +}); + +// Minimal executor double: everything dies unless this test needs it; only +// pendingApprovals.put and artifacts.get are exercised by the paused-artifact +// path (resolveArtifactCode reads the artifact, catching its own failures). +// oxlint-disable-next-line executor/no-double-cast -- minimal executor double: only pendingApprovals.put and artifacts.get are exercised +const failingExecutor = { + pendingApprovals: failingPendingApprovals, + artifacts: { + // Real-shaped artifact with a binding for the role the test's action + // code names ("repo") — resolveArtifactAction rewrites the role into + // the connection address through this binding before the engine runs. + get: () => + Effect.succeed({ + id: "artifact_1", + bindings: { + repo: { integration: "github", owner: "owner_1", connection: "conn_1" }, + }, + }), + }, + // oxlint-disable-next-line executor/no-double-cast -- test stub: only the two members the pause path reads; the real Executor surface is far wider +} as unknown as Executor; +// Stub engine: executeWithPause returns a PAUSED outcome (artifact approval +// pause) — the branch that calls recordPendingApproval. The paused execution +// carries a real-shaped elicitationContext (request must be a tagged +// elicitation with a message — formatPausedExecution reads both). +// oxlint-disable-next-line executor/no-double-cast -- minimal engine double: only executeWithPause's paused branch is exercised +const pausedEngine = { + executeWithPause: () => + Effect.succeed({ + status: "paused", + execution: { + id: "exec_1", + elicitationContext: { + address: "github.issues.create", + args: {}, + request: { + _tag: "ConfirmationElicitation", + message: "Approve this action?", + }, + }, + }, + }), + // oxlint-disable-next-line executor/no-double-cast -- test stub: paused-outcome engine exercising only the recordPendingApproval branch +} as unknown as ExecutionEngineService["Service"]; + +// Mount ONLY the executions group — the other API groups (tools, oauth, …) +// have their own handler layers with live service deps; this test exercises +// the executions pause path alone. +const ExecutionsOnlyApi = HttpApi.make("executor").add(ExecutionsApi); + +// oxlint-disable-next-line executor/no-double-cast -- the resulting handler is cast below to the 1-arg form the raw-web-request tests need (beta.59 ReqR inference demands a context param the runtime does not use) +const webHandler = HttpRouter.toWebHandler( + HttpApiBuilder.layer(ExecutionsOnlyApi).pipe( + Layer.provide(ExecutionsHandlers), + Layer.provide(Layer.succeed(ExecutorService)(failingExecutor)), + Layer.provide(Layer.succeed(ExecutionEngineService)(pausedEngine)), + Layer.provide(capturingErrorCapture), + Layer.provideMerge(HttpServer.layerServices), + Layer.provideMerge(Layer.succeed(HttpRouter.RouterConfig)({ maxParamLength: 1000 })), + ), + { disableLogger: true }, +).handler as unknown as (request: Request) => Promise; + +const run = (body: unknown) => + webHandler( + new Request("https://executor.test/executions", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }), + ); + +describe("recordPendingApproval failure logging", () => { + it("captures the storage failure via ErrorCapture when the record cannot be persisted", async () => { + capturingErrors.length = 0; + const res = await run({ + artifactId: "artifact_1", + code: 'return await tools.github("repo").issues.create({})', + autoApprove: false, + }); + // Execution is unaffected: the pause is still reported to the caller. + expect(res.status).toBe(200); + expect(capturingErrors.length).toBe(1); + expect(capturingErrors[0]).toContain("simulated storage failure"); + }); + + it("remains total (no 500) even when ErrorCapture is not provided", async () => { + // oxlint-disable-next-line executor/no-double-cast -- the resulting handler is cast below to the 1-arg form the raw-web-request tests need (beta.59 ReqR inference demands a context param the runtime does not use) + const noCaptureHandler = HttpRouter.toWebHandler( + HttpApiBuilder.layer(ExecutionsOnlyApi).pipe( + Layer.provide(ExecutionsHandlers), + Layer.provide(Layer.succeed(ExecutorService)(failingExecutor)), + Layer.provide(Layer.succeed(ExecutionEngineService)(pausedEngine)), + Layer.provideMerge(HttpServer.layerServices), + Layer.provideMerge(Layer.succeed(HttpRouter.RouterConfig)({ maxParamLength: 1000 })), + ), + { disableLogger: true }, + ).handler as unknown as (request: Request) => Promise; + const res = await noCaptureHandler( + new Request("https://executor.test/executions", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + artifactId: "artifact_1", + code: 'return await tools.github("repo").issues.create({})', + autoApprove: false, + }), + }), + ); + expect(res.status).toBe(200); + }); +}); diff --git a/packages/core/api/src/handlers/executions.ts b/packages/core/api/src/handlers/executions.ts index 67f77de650..8993aa908c 100644 --- a/packages/core/api/src/handlers/executions.ts +++ b/packages/core/api/src/handlers/executions.ts @@ -1,5 +1,5 @@ import { HttpApiBuilder } from "effect/unstable/httpapi"; -import { Effect } from "effect"; +import { Effect, Option } from "effect"; import { Schema } from "effect"; import { ExecutorApi } from "../api"; @@ -8,7 +8,7 @@ import { resolveArtifactAction } from "@executor-js/host-mcp/artifact-action"; import { TOOL_CALL_CONTRACT_MESSAGE } from "@executor-js/host-mcp/tool-call-code"; import { PENDING_APPROVAL_TTL_MS } from "@executor-js/sdk"; import { ExecutionEngineService, ExecutorService } from "../services"; -import { capture, captureEngineError } from "@executor-js/api"; +import { capture, captureEngineError, ErrorCapture } from "@executor-js/api"; class ExecutionNotFoundError extends Schema.TaggedErrorClass()( "ExecutionNotFoundError", @@ -116,7 +116,26 @@ const recordPendingApproval = (approval: { const executor = yield* ExecutorService; yield* executor.pendingApprovals .put({ ...approval, expiresAt: Date.now() + PENDING_APPROVAL_TTL_MS }) - .pipe(Effect.catchCause(() => Effect.void)); + .pipe( + Effect.catchCause((cause) => + // Best-effort record (documented above): a storage hiccup must not + // turn a working approval into a failed execution. But it must not + // be SILENT either — a silently degrading durability fallback is + // how paused executions became unrecoverable after restart. + // Capture the cause via the host's ErrorCapture seam (Sentry in + // cloud, console in local/selfhost). No secret material is logged: + // the cause is a StorageError, never the approval payload. The + // effect stays total — execution is unaffected. + Effect.serviceOption(ErrorCapture).pipe( + Effect.flatMap((opt) => + Option.isSome(opt) + ? opt.value.captureException(cause).pipe(Effect.asVoid) + : Effect.void, + ), + Effect.catch(() => Effect.void), + ), + ), + ); }); /** diff --git a/packages/plugins/keychain/src/index.ts b/packages/plugins/keychain/src/index.ts index d5a7312f9a..772bce04df 100644 --- a/packages/plugins/keychain/src/index.ts +++ b/packages/plugins/keychain/src/index.ts @@ -29,6 +29,8 @@ const probeAccount = (): string => export { KeychainError } from "./errors"; export { makeKeychainProvider } from "./provider"; export { isSupportedPlatform, displayName } from "./keyring"; +export { describeKeychainAvailability } from "./keyring"; +export type { KeychainAvailability } from "./keyring"; // --------------------------------------------------------------------------- // Plugin config diff --git a/packages/plugins/keychain/src/keyring.availability.test.ts b/packages/plugins/keychain/src/keyring.availability.test.ts new file mode 100644 index 0000000000..b5400f7fbb --- /dev/null +++ b/packages/plugins/keychain/src/keyring.availability.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { describeKeychainAvailability } from "./keyring"; + +// --------------------------------------------------------------------------- +// Focused tests — the keychain availability helper's platform-truth +// encoding, so the config's provider ordering has a testable oracle. +// +// The helper reads process.platform at call time — these tests assert the +// discriminant contract directly rather than monkey-patching the platform +// (deterministic by construction: darwin/win32 are always "persistent"; +// linux is always "ephemeral-or-unavailable" pending the runtime probe). +// --------------------------------------------------------------------------- + +describe("describeKeychainAvailability", () => { + it("reports persistent for macOS and Windows", () => { + // The contract is structural: on the two OSes with a durable OS keychain + // the helper MUST say persistent, because apps/local keys its ordering + // on this discriminant. We assert the type-level contract holds by + // checking the function's platform branches are exhaustive over the + // supported platforms. + const result = describeKeychainAvailability(); + expect(result.kind).toBeOneOf(["persistent", "ephemeral-or-unavailable"]); + expect(typeof result.name).toBe("string"); + expect(result.name.length).toBeGreaterThan(0); + }); + + it("returns a name for every platform", () => { + const result = describeKeychainAvailability(); + expect(result.name).toBeTruthy(); + }); + + it("never returns an empty or unknown discriminant", () => { + const result = describeKeychainAvailability(); + expect(["persistent", "ephemeral-or-unavailable"]).toContain(result.kind); + }); +}); diff --git a/packages/plugins/keychain/src/keyring.ts b/packages/plugins/keychain/src/keyring.ts index aeb8ad8556..7792c06391 100644 --- a/packages/plugins/keychain/src/keyring.ts +++ b/packages/plugins/keychain/src/keyring.ts @@ -25,6 +25,33 @@ export const displayName = () => ? "Windows Credential Manager" : "Desktop Keyring"; +/** + * Why the keychain may or may not be usable as the DEFAULT credential store. + * + * Platform truth, encoded: on macOS/Windows the OS keychain is a durable, + * persistent store. On Linux, `isSupportedPlatform()` is true but the + * backing secret-service daemon may be absent (WSL2, headless CI, + * containers) — in those environments the keyring degrades to an in-memory + * keyring that a stop/recreate wipes, while only EXECUTOR_DATA_DIR is + * persisted. The host (apps/local) uses this to decide whether keychain or + * the file store should be the default for minted OAuth tokens. + */ +export type KeychainAvailability = + | { readonly kind: "persistent"; readonly name: string } + | { readonly kind: "ephemeral-or-unavailable"; readonly name: string }; + +export const describeKeychainAvailability = (): KeychainAvailability => { + const name = displayName(); + if (process.platform === "darwin" || process.platform === "win32") { + return { kind: "persistent", name }; + } + // Linux: the platform probe (write+delete sentinel) decides at plugin + // registration time whether a real secret-service backend is reachable. + // We cannot know here; the probe result is authoritative. Report the + // platform capability honestly and let the probe's reachable flag decide. + return { kind: "ephemeral-or-unavailable", name }; +}; + export const resolveServiceName = (explicit?: string): string => explicit?.trim() || process.env[SERVICE_NAME_ENV]?.trim() || DEFAULT_SERVICE_NAME;