From bd002c80a15f01cc3ecc37401d4599f5e8d092bf Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 12 Sep 2026 20:05:59 -0700 Subject: [PATCH] fix(update): preserve env shorthand after service reinstall (#146660) A systemd Gateway whose unit lists a managed environment key and whose config uses `${VAR}` shorthand can start again after the 2026.9.4 service reinstall. Startup, service planning, and secrets audit share the existing config-resolution provenance to recognize authored environment references after substitution; escaped literals remain literal, stale keys are still removed, and configuration files remain unchanged. Fixes #146612 Reported by @foxsky (#146612). Validation: focused startup, installer, Doctor, config, audit, and runtime suites passed; built Linux before/after startup proof retained the key and reached readiness with unchanged config bytes. Exact-head CI was reverified green. A complete published-updater service-regeneration run remains a documented proof gap. --- docs/help/environment.md | 6 ++ .../gateway-cli/pre-bootstrap.process.test.ts | 98 +++++++++++++++++++ src/cli/gateway-cli/pre-bootstrap.ts | 2 +- src/commands/daemon-install-helpers.test.ts | 43 +++++--- src/commands/daemon-install-helpers.ts | 45 ++++----- src/config/resolution-facts.ts | 39 +++++++- src/config/types.secrets.test.ts | 7 +- src/config/types.secrets.ts | 22 ----- src/secrets/audit.test.ts | 52 ++++++++-- src/secrets/audit.ts | 15 +-- 10 files changed, 245 insertions(+), 84 deletions(-) diff --git a/docs/help/environment.md b/docs/help/environment.md index 7c96e32708aa..2b1535cce68a 100644 --- a/docs/help/environment.md +++ b/docs/help/environment.md @@ -10,6 +10,12 @@ title: "Environment variables" OpenClaw pulls environment variables from multiple sources. The normal rule is **never override existing values**. For an OpenClaw-installed systemd service, the global `.env` may replace only service values that OpenClaw recorded as managed. Operator-owned service values still take precedence. Workspace `.env` files are a lower-trust source: OpenClaw ignores provider credentials and protected runtime controls from workspace `.env` before applying precedence. +Systemd startup preserves managed process values referenced by config, including +`${VAR}` and `$VAR` SecretRef shorthand in `$include` files. This also covers +values supplied by an operator `EnvironmentFile=`. Managed values absent from +both trusted dotenv files and current config references are removed from the +Gateway process environment. + ## Precedence (highest to lowest) 1. **Process environment** (what the Gateway process already has from the parent shell/daemon). diff --git a/src/cli/gateway-cli/pre-bootstrap.process.test.ts b/src/cli/gateway-cli/pre-bootstrap.process.test.ts index 553c6f2fc0b3..f9a1c6cf4f3b 100644 --- a/src/cli/gateway-cli/pre-bootstrap.process.test.ts +++ b/src/cli/gateway-cli/pre-bootstrap.process.test.ts @@ -34,6 +34,104 @@ function stateManifest(root: string): Record { } describe("Gateway config selection before migration admission", () => { + it.each([ + { name: "managed template", apiKey: "${REPRO_PROVIDER_KEY}", managed: true, included: false }, + { + name: "unmanaged template", + apiKey: "${REPRO_PROVIDER_KEY}", + managed: false, + included: false, + }, + { name: "included template", apiKey: "${REPRO_PROVIDER_KEY}", managed: true, included: true }, + { name: "managed shorthand", apiKey: "$REPRO_PROVIDER_KEY", managed: true, included: false }, + { + name: "managed object", + apiKey: { source: "env", provider: "default", id: "REPRO_PROVIDER_KEY" }, + managed: true, + included: false, + }, + ])( + "preserves $name through startup without writing config", + async ({ apiKey, managed, included }) => { + const root = fs.realpathSync(tempDirs.make("openclaw-managed-env-selection-")); + const runtimeRoot = createSourceRuntime(runtimeParent); + const stateDir = path.join(root, "state"); + fs.mkdirSync(stateDir); + const configPath = path.join(stateDir, "openclaw.json"); + const providers = { + minimax: { + baseUrl: "https://api.minimax.io/anthropic", + api: "anthropic-messages", + apiKey, + models: [], + }, + }; + if (included) { + fs.writeFileSync(path.join(stateDir, "providers.json"), JSON.stringify(providers)); + } + fs.writeFileSync( + configPath, + JSON.stringify({ + gateway: { mode: "local" }, + plugins: { enabled: false }, + messages: { responsePrefix: "$${STALE_KEY}" }, + models: { providers: included ? { $include: "providers.json" } : providers }, + }), + ); + const before = stateManifest(stateDir); + const result = await runIsolatedModuleScript( + { + PATH: process.env.PATH, + TMPDIR: childTempDir, + TEMP: childTempDir, + TMP: childTempDir, + HOME: root, + USERPROFILE: root, + OPENCLAW_HOME: root, + OPENCLAW_STATE_DIR: stateDir, + OPENCLAW_CONFIG_PATH: configPath, + OPENCLAW_WORKSPACE_DIR: path.join(root, "workspace"), + OPENCLAW_DISABLE_BUNDLED_PLUGINS: "1", + OPENCLAW_BUNDLED_PLUGINS_DIR: path.join(root, "bundled"), + INVOCATION_ID: "repro", + REPRO_PROVIDER_KEY: "repro-not-a-real-key", + STALE_KEY: "removed-service-value", + OPENCLAW_SERVICE_MANAGED_ENV_KEYS: managed ? "REPRO_PROVIDER_KEY,STALE_KEY" : "STALE_KEY", + }, + ` + Object.defineProperty(process, "platform", { value: "linux" }); + const { selectGatewayRunEnvironment, prepareGatewayRunBootstrap, recheckGatewayRunBootstrap } = await import("./src/cli/gateway-cli/pre-bootstrap.ts"); + const { ExitError } = await import("./src/runtime.ts"); + const runtime = { log() {}, error: console.error, exit(code) { throw new ExitError(code); } }; + let admitted = false; + try { + if (await selectGatewayRunEnvironment({ opts: {}, runtime }) && + await prepareGatewayRunBootstrap({ opts: {}, runtime })) { + admitted = await recheckGatewayRunBootstrap({ opts: {}, runtime }); + } + } catch (error) { + if (!(error instanceof ExitError)) throw error; + } + console.log("__RESULT__" + JSON.stringify({ admitted, + keyPresent: process.env.REPRO_PROVIDER_KEY === "repro-not-a-real-key", + stalePresent: process.env.STALE_KEY !== undefined, + })); + `, + { runtimeRoot, timeoutMs: 60_000 }, + ); + const output = `${result.stdout}\n${result.stderr}`; + const line = result.stdout.split("\n").find((entry) => entry.startsWith("__RESULT__")); + expect(line, output).toBeDefined(); + expect(JSON.parse(line!.slice("__RESULT__".length)), output).toEqual({ + admitted: true, + keyPresent: true, + stalePresent: false, + }); + expect(stateManifest(stateDir)).toEqual(before); + }, + 75_000, + ); + it.each([ { name: "future backup before reset", code: 1 }, { name: "future current config", code: 1 }, diff --git a/src/cli/gateway-cli/pre-bootstrap.ts b/src/cli/gateway-cli/pre-bootstrap.ts index 2459c812ddd8..6ef6715cb181 100644 --- a/src/cli/gateway-cli/pre-bootstrap.ts +++ b/src/cli/gateway-cli/pre-bootstrap.ts @@ -270,7 +270,7 @@ async function guardGatewayRunSelectedConfig( import("../../infra/env.js"), import("../../config/paths.js"), import("../../utils.js"), - import("../../config/types.secrets.js"), + import("../../config/resolution-facts.js"), import("../../daemon/service-managed-env.js"), ]); const invocationDestructiveOverride = resolveInvocationDestructiveOverride(); diff --git a/src/commands/daemon-install-helpers.test.ts b/src/commands/daemon-install-helpers.test.ts index 5a86014b5096..7afe13e51c79 100644 --- a/src/commands/daemon-install-helpers.test.ts +++ b/src/commands/daemon-install-helpers.test.ts @@ -4,8 +4,11 @@ import os from "node:os"; import path from "node:path"; import { Writable } from "node:stream"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { coerceConfig, resolveConfigForRead } from "../config/io.read-helpers.js"; +import { setConfigResolutionFacts } from "../config/resolution-facts.js"; import { writeStateDirDotEnv } from "../config/test-helpers.js"; import type { OpenClawConfig } from "../config/types.js"; +import type { SecretInput } from "../config/types.secrets.js"; import { buildLaunchAgentPlist, readLaunchAgentProgramArgumentsFromFile, @@ -862,32 +865,44 @@ describe("buildGatewayInstallPlan", () => { }, ); - it("renders config env SecretRefs as file-backed managed values on Linux", async () => { + it.each<{ name: string; token: SecretInput; resolved: boolean; managed: boolean }>([ + { + name: "structured reference", + token: { source: "env", provider: "default", id: "DISCORD_BOT_TOKEN" }, + resolved: false, + managed: true, + }, + { name: "raw shorthand", token: "${DISCORD_BOT_TOKEN}", resolved: false, managed: true }, + { name: "resolved shorthand", token: "${DISCORD_BOT_TOKEN}", resolved: true, managed: true }, + { name: "pending shorthand", token: "$DISCORD_BOT_TOKEN", resolved: true, managed: true }, + { name: "escaped literal", token: "$${DISCORD_BOT_TOKEN}", resolved: true, managed: false }, + ])("renders Linux service env for $name", async ({ token, resolved, managed }) => { mockNodeGatewayPlanFixture({ serviceEnvironment: { OPENCLAW_PORT: "3000", }, }); + const env = isolatedPlanEnv({ DISCORD_BOT_TOKEN: "discord-test-token" }); + let config: OpenClawConfig = { channels: { discord: { token } } }; + if (resolved) { + const read = resolveConfigForRead(config, env); + config = coerceConfig(read.resolvedConfigRaw); + setConfigResolutionFacts(config, read.resolutionFacts); + } const plan = await buildGatewayInstallPlan({ - env: isolatedPlanEnv({ - DISCORD_BOT_TOKEN: "discord-test-token", - }), + env, port: 3000, runtime: "node", platform: "linux", - config: { - channels: { - discord: { - token: { source: "env", provider: "default", id: "DISCORD_BOT_TOKEN" }, - }, - }, - }, + config, }); - expect(plan.environment.DISCORD_BOT_TOKEN).toBe("discord-test-token"); - expect(plan.environmentValueSources?.DISCORD_BOT_TOKEN).toBe("file"); - expect(plan.environment.OPENCLAW_SERVICE_MANAGED_ENV_KEYS).toBe("DISCORD_BOT_TOKEN"); + expect(plan.environment.DISCORD_BOT_TOKEN).toBe(managed ? "discord-test-token" : undefined); + expect(plan.environmentValueSources?.DISCORD_BOT_TOKEN).toBe(managed ? "file" : undefined); + expect(plan.environment.OPENCLAW_SERVICE_MANAGED_ENV_KEYS).toBe( + managed ? "DISCORD_BOT_TOKEN" : undefined, + ); }); it("retains config env SecretRefs for Windows task scripts", async () => { diff --git a/src/commands/daemon-install-helpers.ts b/src/commands/daemon-install-helpers.ts index 888ffc47c54c..8b1e982cc04a 100644 --- a/src/commands/daemon-install-helpers.ts +++ b/src/commands/daemon-install-helpers.ts @@ -5,9 +5,10 @@ import path from "node:path"; import type { AuthProfileStore } from "../agents/auth-profiles/types.js"; import { formatCliCommand } from "../cli/command-format.js"; import { resolveConfigWidePluginManifestRegistry } from "../config/io.plugin-metadata.js"; +import { collectEnvSecretRefIds, resolveConfigSecretRef } from "../config/resolution-facts.js"; import { collectDurableServiceEnvVarSources } from "../config/state-dir-dotenv.js"; import type { OpenClawConfig } from "../config/types.js"; -import { coerceSecretRef, resolveSecretInputRef, type SecretRef } from "../config/types.secrets.js"; +import { resolveSecretInputRef, type SecretRef } from "../config/types.secrets.js"; import { resolveGatewayLaunchAgentLabel } from "../daemon/constants.js"; import { resolveGatewayStateDir, resolveGatewayTaskScriptPath } from "../daemon/paths.js"; import { @@ -46,6 +47,7 @@ import { } from "../secrets/provider-integrations.js"; import { collectPluginConfigAssignments } from "../secrets/runtime-config-collectors-plugins.js"; import { evaluateGatewayAuthSurfaceStates } from "../secrets/runtime-gateway-auth-surfaces.js"; +import { hasSecretRefCandidate } from "../secrets/runtime-secret-scan.js"; import { createResolverContext } from "../secrets/runtime-shared.js"; import { discoverConfigSecretTargets } from "../secrets/target-registry.js"; import { createLazyPromise } from "../shared/lazy-runtime.js"; @@ -72,27 +74,6 @@ const NON_PERSISTED_CONFIG_SECRET_ENV_TARGET_IDS = new Set([ ]); const EXEC_SECRET_REF_PASS_ENV_ALLOWED_OVERRIDE_ONLY_KEYS = new Set(["HOME"]); -function configContainsSecretRef(config: OpenClawConfig | undefined): boolean { - if (!config) { - return false; - } - const pending: unknown[] = [config]; - const seen = new Set(); - const defaults = config.secrets?.defaults; - while (pending.length > 0) { - const value = pending.pop(); - if (coerceSecretRef(value, defaults)) { - return true; - } - if (!value || typeof value !== "object" || seen.has(value)) { - continue; - } - seen.add(value); - pending.push(...Object.values(value)); - } - return false; -} - function isBlockedExecSecretRefPassEnvKey(key: string): boolean { if (isDangerousHostEnvVarName(key)) { return true; @@ -288,7 +269,13 @@ function collectConfigSecretRefServiceEnvSources(params: { continue; } const { ref } = resolveSecretInputRef({ - value: target.value, + value: resolveConfigSecretRef({ + config: params.config, + path: target.path, + value: target.value, + defaults: params.config.secrets?.defaults, + includeResolved: true, + }), refValue: target.refValue, defaults: params.config.secrets?.defaults, }); @@ -351,7 +338,13 @@ function collectExecSecretRefPassEnvServiceEnvVars(params: { continue; } const { ref } = resolveSecretInputRef({ - value: target.value, + value: resolveConfigSecretRef({ + config: params.config, + path: target.path, + value: target.value, + defaults: params.config.secrets?.defaults, + includeResolved: true, + }), refValue: target.refValue, defaults: params.config.secrets?.defaults, }); @@ -696,7 +689,9 @@ async function buildGatewayInstallEnvironment(params: { config: params.config, }); // Full target discovery materializes plugin metadata; configs without refs do not need it. - const containsConfigSecretRef = configContainsSecretRef(params.config); + const containsConfigSecretRef = + hasSecretRefCandidate(params.config, params.config?.secrets?.defaults) || + collectEnvSecretRefIds(params.config).size > 0; const { keys: configSecretRefKeys, environment: configSecretRefEnvironment } = collectConfigSecretRefServiceEnvSources({ env: params.env, diff --git a/src/config/resolution-facts.ts b/src/config/resolution-facts.ts index 07b9c54efe3a..7809e0540834 100644 --- a/src/config/resolution-facts.ts +++ b/src/config/resolution-facts.ts @@ -1,5 +1,10 @@ import type { EnvSubstitutionWarning } from "./env-substitution.js"; -import { coerceSecretRef, DEFAULT_SECRET_PROVIDER_ALIAS, type SecretRef } from "./types.secrets.js"; +import { + coerceSecretRef, + DEFAULT_SECRET_PROVIDER_ALIAS, + isValidEnvSecretRefId, + type SecretRef, +} from "./types.secrets.js"; /** `null` means this value has not passed through authoritative config env substitution. */ export type ConfigResolutionFacts = ReadonlySet | null; @@ -149,15 +154,45 @@ export function getResolvedConfigEnvSecretRef(target: unknown, path: string): Se return fact?.state === "resolved" ? fact.ref : null; } +/** Collect authored env references, including shorthand already materialized by config loading. */ +export function collectEnvSecretRefIds(value: unknown): Set { + const facts = getConfigResolutionFacts(value); + const ids = new Set(); + for (const { ref } of (facts && envSecretRefsByFacts.get(facts))?.values() ?? []) { + ids.add(ref.id); + } + const seen = new WeakSet(); + const visit = (candidate: unknown): void => { + // Loaded strings are decoded literals; only their recorded provenance can name a reference. + const ref = typeof candidate === "string" && facts !== null ? null : coerceSecretRef(candidate); + if (ref?.source === "env" && isValidEnvSecretRefId(ref.id)) { + ids.add(ref.id); + return; + } + if (typeof candidate !== "object" || candidate === null || seen.has(candidate)) { + return; + } + seen.add(candidate); + for (const child of Array.isArray(candidate) ? candidate : Object.values(candidate)) { + visit(child); + } + }; + visit(value); + return ids; +} + /** Reads inline references from authored facts and structured references from their values. */ export function resolveConfigSecretRef(params: { config: unknown; path: string; value: unknown; defaults?: Parameters[1]; + /** Authoring and audit consumers also need the source of materialized values. */ + includeResolved?: boolean; }): SecretRef | null { return typeof params.value === "string" && getConfigResolutionFacts(params.config) !== null - ? getAuthoredConfigSecretRef(params.config, params.path) + ? (getAuthoredConfigSecretRef(params.config, params.path) ?? + (params.includeResolved ? getResolvedConfigEnvSecretRef(params.config, params.path) : null)) : coerceSecretRef(params.value, params.defaults); } diff --git a/src/config/types.secrets.test.ts b/src/config/types.secrets.test.ts index 47b35d164c30..4a20579a4e3b 100644 --- a/src/config/types.secrets.test.ts +++ b/src/config/types.secrets.test.ts @@ -1,10 +1,7 @@ // Verifies secret config type guards and normalization helpers. import { describe, expect, it } from "vitest"; -import { - coerceSecretRef, - collectEnvSecretRefIds, - parseEnvTemplateSecretRef, -} from "./types.secrets.js"; +import { collectEnvSecretRefIds } from "./resolution-facts.js"; +import { coerceSecretRef, parseEnvTemplateSecretRef } from "./types.secrets.js"; describe("parseEnvTemplateSecretRef", () => { it("parses ${VAR} template syntax", () => { diff --git a/src/config/types.secrets.ts b/src/config/types.secrets.ts index 047d242119a2..6f5b839d3f41 100644 --- a/src/config/types.secrets.ts +++ b/src/config/types.secrets.ts @@ -82,28 +82,6 @@ export function parseEnvTemplateSecretRef( }; } -/** Collect env ids from supported SecretRef shapes anywhere in a config tree. */ -export function collectEnvSecretRefIds(value: unknown): Set { - const ids = new Set(); - const seen = new WeakSet(); - const visit = (candidate: unknown): void => { - const ref = coerceSecretRef(candidate); - if (ref?.source === "env" && isValidEnvSecretRefId(ref.id)) { - ids.add(ref.id); - return; - } - if (typeof candidate !== "object" || candidate === null || seen.has(candidate)) { - return; - } - seen.add(candidate); - for (const child of Array.isArray(candidate) ? candidate : Object.values(candidate)) { - visit(child); - } - }; - visit(value); - return ids; -} - /** Detect retired env SecretRef marker strings for migration and explicit rejection. */ export function isLegacySecretRefEnvMarker(value: unknown): value is string { if (typeof value !== "string") { diff --git a/src/secrets/audit.test.ts b/src/secrets/audit.test.ts index 79d6f93bdaa6..cf7b24720621 100644 --- a/src/secrets/audit.test.ts +++ b/src/secrets/audit.test.ts @@ -341,11 +341,19 @@ describe("secrets audit", () => { apiKey: { source: "store", provider: "default", id: "STORED_API_KEY" }, models: [{ id: "fixture", name: "fixture" }], }, + envReferenced: { + baseUrl: "https://env-referenced.example.test/v1", + api: "openai-completions", + apiKey: "${AUDIT_STORE_VALUE}", + models: [{ id: "fixture", name: "fixture" }], + }, }, }, }); - const report = await runSecretsAudit({ env: fixture.env }); + const report = await runSecretsAudit({ + env: { ...fixture.env, AUDIT_STORE_VALUE: "shared-store-value" }, + }); expect(report.summary.storeResidueCount).toBe(1); expect(report.findings.find((entry) => entry.code === "STORE_PLAINTEXT_RESIDUE")).toMatchObject( { @@ -836,12 +844,37 @@ describe("secrets audit", () => { ).toBe(true); }); - it("exempts only known openclaw.json model provider apiKey markers", async () => { - for (const { apiKey, isPlaintext } of [ - { apiKey: "lmstudio-local", isPlaintext: false }, - { apiKey: "ollama-local", isPlaintext: false }, - { apiKey: "sk-real-plaintext", isPlaintext: true }, - ]) { + it.each([ + { name: "lmstudio marker", apiKey: "lmstudio-local", isPlaintext: false, refsChecked: 0 }, + { name: "ollama marker", apiKey: "ollama-local", isPlaintext: false, refsChecked: 0 }, + { name: "plaintext", apiKey: "sk-real-plaintext", isPlaintext: true, refsChecked: 0 }, + { + name: "resolved shorthand", + apiKey: "${OPENAI_API_KEY}", + isPlaintext: false, + refsChecked: 1, + }, + { + name: "pending shorthand", + apiKey: "$OPENAI_API_KEY", + isPlaintext: false, + refsChecked: 1, + }, + { + name: "structured reference", + apiKey: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, + isPlaintext: false, + refsChecked: 1, + }, + { + name: "escaped literal", + apiKey: "$${OPENAI_API_KEY}", + isPlaintext: true, + refsChecked: 0, + }, + ])( + "classifies config provider credentials from $name", + async ({ apiKey, isPlaintext, refsChecked }) => { await writeJsonFile(fixture.configPath, { models: { providers: { @@ -865,8 +898,9 @@ describe("secrets audit", () => { entry.jsonPath === "models.providers.openai.apiKey", ), ).toBe(isPlaintext); - } - }); + expect(report.resolution.refsChecked).toBe(refsChecked); + }, + ); it("scans .env in legacy .clawdbot state directory via automatic fallback", async () => { // Do NOT set OPENCLAW_STATE_DIR or OPENCLAW_CONFIG_PATH — rely on diff --git a/src/secrets/audit.ts b/src/secrets/audit.ts index ff9989d10423..bae402bcd21d 100644 --- a/src/secrets/audit.ts +++ b/src/secrets/audit.ts @@ -15,6 +15,7 @@ import { } from "../agents/model-auth-markers.js"; import { normalizeProviderId } from "../agents/model-selection.js"; import { resolveStateDir, type OpenClawConfig } from "../config/config.js"; +import { resolveConfigSecretRef } from "../config/resolution-facts.js"; import { coerceSecretRef, resolveSecretInputRef, type SecretRef } from "../config/types.secrets.js"; import { formatErrorMessage } from "../infra/errors.js"; import { resolveUserPath } from "../utils.js"; @@ -196,15 +197,17 @@ function collectConfigSecrets(params: { if (!target.entry.includeInAudit) { continue; } - const { ref } = resolveSecretInputRef({ + const inlineRef = resolveConfigSecretRef({ + config: params.config, + path: target.path, value: target.value, - refValue: target.refValue, defaults, + includeResolved: true, }); - const hasPlaintext = hasConfiguredPlaintextSecretValue( - target.value, - target.entry.expectedResolvedValue, - ); + const ref = coerceSecretRef(target.refValue, defaults) ?? inlineRef; + const hasPlaintext = + inlineRef === null && + hasConfiguredPlaintextSecretValue(target.value, target.entry.expectedResolvedValue); const isNonSecretHeader = target.entry.id === "models.providers.*.headers.*" && !isLikelySensitiveModelProviderHeaderName(target.pathSegments.at(-1) ?? "");