diff --git a/src/config/zod-schema.core.ts b/src/config/zod-schema.core.ts index b30df9d0069..df330b88901 100644 --- a/src/config/zod-schema.core.ts +++ b/src/config/zod-schema.core.ts @@ -140,7 +140,7 @@ const SecretsExecProviderSchema = z }) .strict(); -const SecretsProviderSchema = z.discriminatedUnion("source", [ +export const SecretProviderSchema = z.discriminatedUnion("source", [ SecretsEnvProviderSchema, SecretsFileProviderSchema, SecretsExecProviderSchema, @@ -152,7 +152,7 @@ export const SecretsConfigSchema = z .object({ // Keep this as a record so users can define multiple providers per source. }) - .catchall(SecretsProviderSchema) + .catchall(SecretProviderSchema) .optional(), defaults: z .object({ diff --git a/src/secrets/apply.test.ts b/src/secrets/apply.test.ts index 9763594c42c..3e5b456b2d6 100644 --- a/src/secrets/apply.test.ts +++ b/src/secrets/apply.test.ts @@ -171,12 +171,75 @@ describe("secrets apply", () => { const first = await runSecretsApply({ plan, env, write: true }); expect(first.changed).toBe(true); + // Second apply should be a true no-op and avoid file writes entirely. + await fs.chmod(configPath, 0o400); + await fs.chmod(authStorePath, 0o400); + const second = await runSecretsApply({ plan, env, write: true }); expect(second.mode).toBe("write"); expect(second.changed).toBe(false); expect(second.changedFiles).toEqual([]); }); + it("applies targets safely when map keys contain dots", async () => { + await fs.writeFile( + configPath, + `${JSON.stringify( + { + models: { + providers: { + "openai.dev": { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: "sk-openai-plaintext", + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + }, + }, + }, + null, + 2, + )}\n`, + "utf8", + ); + + const plan: SecretsApplyPlan = { + version: 1, + protocolVersion: 1, + generatedAt: new Date().toISOString(), + generatedBy: "manual", + targets: [ + { + type: "models.providers.apiKey", + path: "models.providers.openai.dev.apiKey", + pathSegments: ["models", "providers", "openai.dev", "apiKey"], + providerId: "openai.dev", + ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" }, + }, + ], + options: { + scrubEnv: false, + scrubAuthProfilesForProviderTargets: false, + scrubLegacyAuthJson: false, + }, + }; + + const result = await runSecretsApply({ plan, env, write: true }); + expect(result.changed).toBe(true); + + const nextConfig = JSON.parse(await fs.readFile(configPath, "utf8")) as { + models?: { + providers?: Record; + }; + }; + expect(nextConfig.models?.providers?.["openai.dev"]?.apiKey).toEqual({ + source: "env", + provider: "default", + id: "OPENAI_API_KEY", + }); + expect(nextConfig.models?.providers?.openai).toBeUndefined(); + }); + it("applies provider upserts and deletes from plan", async () => { await fs.writeFile( configPath, diff --git a/src/secrets/apply.ts b/src/secrets/apply.ts index 005de3e9f26..8c4e2c69c2a 100644 --- a/src/secrets/apply.ts +++ b/src/secrets/apply.ts @@ -11,7 +11,11 @@ import type { ConfigWriteOptions } from "../config/io.js"; import type { SecretProviderConfig } from "../config/types.secrets.js"; import { resolveConfigDir, resolveUserPath } from "../utils.js"; import { createSecretsConfigIO } from "./config-io.js"; -import { type SecretsApplyPlan, normalizeSecretsPlanOptions } from "./plan.js"; +import { + type SecretsApplyPlan, + type SecretsPlanTarget, + normalizeSecretsPlanOptions, +} from "./plan.js"; import { listKnownSecretEnvVarNames } from "./provider-env-vars.js"; import { resolveSecretRefValue } from "./resolve.js"; import { prepareSecretsRuntimeSnapshot } from "./runtime.js"; @@ -52,8 +56,10 @@ function parseDotPath(pathname: string): string[] { return pathname.split(".").filter(Boolean); } -function getByDotPath(root: unknown, pathLabel: string): unknown { - const segments = parseDotPath(pathLabel); +function getByPathSegments(root: unknown, segments: string[]): unknown { + if (segments.length === 0) { + return undefined; + } let cursor: unknown = root; for (const segment of segments) { if (!isRecord(cursor)) { @@ -64,8 +70,7 @@ function getByDotPath(root: unknown, pathLabel: string): unknown { return cursor; } -function setByDotPath(root: OpenClawConfig, pathLabel: string, value: unknown): boolean { - const segments = parseDotPath(pathLabel); +function setByPathSegments(root: OpenClawConfig, segments: string[], value: unknown): boolean { if (segments.length === 0) { throw new Error("Target path is empty."); } @@ -88,8 +93,7 @@ function setByDotPath(root: OpenClawConfig, pathLabel: string, value: unknown): return changed; } -function deleteByDotPath(root: OpenClawConfig, pathLabel: string): boolean { - const segments = parseDotPath(pathLabel); +function deleteByPathSegments(root: OpenClawConfig, segments: string[]): boolean { if (segments.length === 0) { return false; } @@ -109,6 +113,18 @@ function deleteByDotPath(root: OpenClawConfig, pathLabel: string): boolean { return true; } +function resolveTargetPathSegments(target: SecretsPlanTarget): string[] { + const explicit = target.pathSegments; + if ( + Array.isArray(explicit) && + explicit.length > 0 && + explicit.every((segment) => typeof segment === "string" && segment.trim().length > 0) + ) { + return [...explicit]; + } + return parseDotPath(target.path); +} + function parseEnvValue(raw: string): string { const trimmed = raw.trim(); if ( @@ -203,11 +219,13 @@ function collectAuthJsonPaths(stateDir: string): string[] { return out; } -function resolveGoogleChatRefPath(pathLabel: string): string { - if (pathLabel.endsWith(".serviceAccount")) { - return `${pathLabel}Ref`; +function resolveGoogleChatRefPathSegments(pathSegments: string[]): string[] { + if (pathSegments.at(-1) === "serviceAccount") { + return [...pathSegments.slice(0, -1), "serviceAccountRef"]; } - throw new Error(`Google Chat target path must end with ".serviceAccount": ${pathLabel}`); + throw new Error( + `Google Chat target path must end with "serviceAccount": ${pathSegments.join(".")}`, + ); } function applyProviderPlanMutations(params: { @@ -280,25 +298,26 @@ async function projectPlanState(params: { } for (const target of params.plan.targets) { + const targetPathSegments = resolveTargetPathSegments(target); if (target.type === "channels.googlechat.serviceAccount") { - const previous = getByDotPath(nextConfig, target.path); + const previous = getByPathSegments(nextConfig, targetPathSegments); if (isNonEmptyString(previous)) { scrubbedValues.add(previous.trim()); } - const refPath = resolveGoogleChatRefPath(target.path); - const wroteRef = setByDotPath(nextConfig, refPath, target.ref); - const deletedLegacy = deleteByDotPath(nextConfig, target.path); + const refPathSegments = resolveGoogleChatRefPathSegments(targetPathSegments); + const wroteRef = setByPathSegments(nextConfig, refPathSegments, target.ref); + const deletedLegacy = deleteByPathSegments(nextConfig, targetPathSegments); if (wroteRef || deletedLegacy) { changedFiles.add(configPath); } continue; } - const previous = getByDotPath(nextConfig, target.path); + const previous = getByPathSegments(nextConfig, targetPathSegments); if (isNonEmptyString(previous)) { scrubbedValues.add(previous.trim()); } - const wroteRef = setByDotPath(nextConfig, target.path, target.ref); + const wroteRef = setByPathSegments(nextConfig, targetPathSegments, target.ref); if (wroteRef) { changedFiles.add(configPath); } @@ -510,6 +529,15 @@ export async function runSecretsApply(params: { warnings: projected.warnings, }; } + if (changedFiles.length === 0) { + return { + mode: "write", + changed: false, + changedFiles: [], + warningCount: projected.warnings.length, + warnings: projected.warnings, + }; + } const io = createSecretsConfigIO({ env }); const snapshots = new Map(); diff --git a/src/secrets/audit.test.ts b/src/secrets/audit.test.ts index ecfbfa2a73c..6f0fcf8475b 100644 --- a/src/secrets/audit.test.ts +++ b/src/secrets/audit.test.ts @@ -105,4 +105,77 @@ describe("secrets audit", () => { await expect(fs.stat(authJsonPath)).resolves.toBeTruthy(); await expect(fs.stat(authStorePath)).rejects.toMatchObject({ code: "ENOENT" }); }); + + it("reports malformed sidecar JSON as findings instead of crashing", async () => { + await fs.writeFile(authStorePath, "{invalid-json", "utf8"); + await fs.writeFile(authJsonPath, "{invalid-json", "utf8"); + + const report = await runSecretsAudit({ env }); + expect(report.findings.some((entry) => entry.file === authStorePath)).toBe(true); + expect(report.findings.some((entry) => entry.file === authJsonPath)).toBe(true); + expect(report.findings.some((entry) => entry.code === "REF_UNRESOLVED")).toBe(true); + }); + + it("batches ref resolution per provider during audit", async () => { + const execLogPath = path.join(rootDir, "exec-calls.log"); + const execScriptPath = path.join(rootDir, "resolver.mjs"); + await fs.writeFile( + execScriptPath, + [ + "#!/usr/bin/env node", + "import fs from 'node:fs';", + "const req = JSON.parse(fs.readFileSync(0, 'utf8'));", + `fs.appendFileSync(${JSON.stringify(execLogPath)}, 'x\\n');`, + "const values = Object.fromEntries((req.ids ?? []).map((id) => [id, `value:${id}`]));", + "process.stdout.write(JSON.stringify({ protocolVersion: 1, values }));", + ].join("\n"), + { encoding: "utf8", mode: 0o700 }, + ); + + await fs.writeFile( + configPath, + `${JSON.stringify( + { + secrets: { + providers: { + execmain: { + source: "exec", + command: execScriptPath, + jsonOnly: true, + passEnv: ["PATH"], + }, + }, + }, + models: { + providers: { + openai: { + baseUrl: "https://api.openai.com/v1", + api: "openai-completions", + apiKey: { source: "exec", provider: "execmain", id: "providers/openai/apiKey" }, + models: [{ id: "gpt-5", name: "gpt-5" }], + }, + moonshot: { + baseUrl: "https://api.moonshot.cn/v1", + api: "openai-completions", + apiKey: { source: "exec", provider: "execmain", id: "providers/moonshot/apiKey" }, + models: [{ id: "moonshot-v1-8k", name: "moonshot-v1-8k" }], + }, + }, + }, + }, + null, + 2, + )}\n`, + "utf8", + ); + await fs.rm(authStorePath, { force: true }); + await fs.writeFile(envPath, "", "utf8"); + + const report = await runSecretsAudit({ env }); + expect(report.summary.unresolvedRefCount).toBe(0); + + const callLog = await fs.readFile(execLogPath, "utf8"); + const callCount = callLog.split("\n").filter((line) => line.trim().length > 0).length; + expect(callCount).toBe(1); + }); }); diff --git a/src/secrets/audit.ts b/src/secrets/audit.ts index e73b24bde6d..2566b5871a3 100644 --- a/src/secrets/audit.ts +++ b/src/secrets/audit.ts @@ -9,7 +9,12 @@ import { coerceSecretRef, type SecretRef } from "../config/types.secrets.js"; import { resolveConfigDir, resolveUserPath } from "../utils.js"; import { createSecretsConfigIO } from "./config-io.js"; import { listKnownSecretEnvVarNames } from "./provider-env-vars.js"; -import { resolveSecretRefValue, type SecretRefResolveCache } from "./resolve.js"; +import { secretRefKey } from "./ref-contract.js"; +import { + resolveSecretRefValue, + resolveSecretRefValues, + type SecretRefResolveCache, +} from "./resolve.js"; import { isNonEmptyString, isRecord } from "./shared.js"; export type SecretsAuditCode = @@ -154,16 +159,26 @@ function collectEnvPlaintext(params: { envPath: string; collector: AuditCollecto } } -function readJsonObject(filePath: string): Record | null { +function readJsonObject(filePath: string): { + value: Record | null; + error?: string; +} { if (!fs.existsSync(filePath)) { - return null; + return { value: null }; } - const raw = fs.readFileSync(filePath, "utf8"); - const parsed = JSON.parse(raw) as unknown; - if (!isRecord(parsed)) { - return null; + try { + const raw = fs.readFileSync(filePath, "utf8"); + const parsed = JSON.parse(raw) as unknown; + if (!isRecord(parsed)) { + return { value: null }; + } + return { value: parsed }; + } catch (err) { + return { + value: null, + error: err instanceof Error ? err.message : String(err), + }; } - return parsed; } function collectConfigSecrets(params: { @@ -322,7 +337,18 @@ function collectAuthStoreSecrets(params: { return; } params.collector.filesScanned.add(params.authStorePath); - const parsed = readJsonObject(params.authStorePath); + const parsedResult = readJsonObject(params.authStorePath); + if (parsedResult.error) { + addFinding(params.collector, { + code: "REF_UNRESOLVED", + severity: "error", + file: params.authStorePath, + jsonPath: "", + message: `Invalid JSON in auth-profiles store: ${parsedResult.error}`, + }); + return; + } + const parsed = parsedResult.value; if (!parsed || !isRecord(parsed.profiles)) { return; } @@ -420,7 +446,18 @@ function collectAuthJsonResidue(params: { stateDir: string; collector: AuditColl continue; } params.collector.filesScanned.add(authJsonPath); - const parsed = readJsonObject(authJsonPath); + const parsedResult = readJsonObject(authJsonPath); + if (parsedResult.error) { + addFinding(params.collector, { + code: "REF_UNRESOLVED", + severity: "error", + file: authJsonPath, + jsonPath: "", + message: `Invalid JSON in legacy auth.json: ${parsedResult.error}`, + }); + continue; + } + const parsed = parsedResult.value; if (!parsed) { continue; } @@ -448,27 +485,99 @@ async function collectUnresolvedRefFindings(params: { env: NodeJS.ProcessEnv; }): Promise { const cache: SecretRefResolveCache = {}; + const refsByProvider = new Map>(); for (const assignment of params.collector.refAssignments) { + const providerKey = `${assignment.ref.source}:${assignment.ref.provider}`; + let refsForProvider = refsByProvider.get(providerKey); + if (!refsForProvider) { + refsForProvider = new Map(); + refsByProvider.set(providerKey, refsForProvider); + } + refsForProvider.set(secretRefKey(assignment.ref), assignment.ref); + } + + const resolvedByRefKey = new Map(); + const errorsByRefKey = new Map(); + + for (const refsForProvider of refsByProvider.values()) { + const refs = [...refsForProvider.values()]; try { - const resolved = await resolveSecretRefValue(assignment.ref, { + const resolved = await resolveSecretRefValues(refs, { config: params.config, env: params.env, cache, }); - if (assignment.expected === "string") { - if (!isNonEmptyString(resolved)) { - throw new Error("resolved value is not a non-empty string"); - } - } else if (!(isNonEmptyString(resolved) || isRecord(resolved))) { - throw new Error("resolved value is not a string/object"); + for (const [key, value] of resolved.entries()) { + resolvedByRefKey.set(key, value); } - } catch (err) { + continue; + } catch { + // Fall back to per-ref resolution for provider-specific pinpoint errors. + } + + for (const ref of refs) { + const key = secretRefKey(ref); + try { + const resolved = await resolveSecretRefValue(ref, { + config: params.config, + env: params.env, + cache, + }); + resolvedByRefKey.set(key, resolved); + } catch (err) { + errorsByRefKey.set(key, err); + } + } + } + + for (const assignment of params.collector.refAssignments) { + const key = secretRefKey(assignment.ref); + const resolveErr = errorsByRefKey.get(key); + if (resolveErr) { addFinding(params.collector, { code: "REF_UNRESOLVED", severity: "error", file: assignment.file, jsonPath: assignment.path, - message: `Failed to resolve ${assignment.ref.source}:${assignment.ref.provider}:${assignment.ref.id} (${String(err)}).`, + message: `Failed to resolve ${assignment.ref.source}:${assignment.ref.provider}:${assignment.ref.id} (${describeUnknownError(resolveErr)}).`, + provider: assignment.provider, + }); + continue; + } + + if (!resolvedByRefKey.has(key)) { + addFinding(params.collector, { + code: "REF_UNRESOLVED", + severity: "error", + file: assignment.file, + jsonPath: assignment.path, + message: `Failed to resolve ${assignment.ref.source}:${assignment.ref.provider}:${assignment.ref.id} (resolved value is missing).`, + provider: assignment.provider, + }); + continue; + } + + const resolved = resolvedByRefKey.get(key); + if (assignment.expected === "string") { + if (!isNonEmptyString(resolved)) { + addFinding(params.collector, { + code: "REF_UNRESOLVED", + severity: "error", + file: assignment.file, + jsonPath: assignment.path, + message: `Failed to resolve ${assignment.ref.source}:${assignment.ref.provider}:${assignment.ref.id} (resolved value is not a non-empty string).`, + provider: assignment.provider, + }); + } + continue; + } + if (!(isNonEmptyString(resolved) || isRecord(resolved))) { + addFinding(params.collector, { + code: "REF_UNRESOLVED", + severity: "error", + file: assignment.file, + jsonPath: assignment.path, + message: `Failed to resolve ${assignment.ref.source}:${assignment.ref.provider}:${assignment.ref.id} (resolved value is not a string/object).`, provider: assignment.provider, }); } @@ -495,6 +604,21 @@ function collectShadowingFindings(collector: AuditCollector): void { } } +function describeUnknownError(err: unknown): string { + if (err instanceof Error && err.message.trim().length > 0) { + return err.message; + } + if (typeof err === "string" && err.trim().length > 0) { + return err; + } + try { + const serialized = JSON.stringify(err); + return serialized ?? "unknown error"; + } catch { + return "unknown error"; + } +} + function summarizeFindings(findings: SecretsAuditFinding[]): SecretsAuditReport["summary"] { return { plaintextCount: findings.filter((entry) => entry.code === "PLAINTEXT_FOUND").length, diff --git a/src/secrets/configure.ts b/src/secrets/configure.ts index 334d4c1d0b1..518f95926d9 100644 --- a/src/secrets/configure.ts +++ b/src/secrets/configure.ts @@ -13,6 +13,7 @@ import { isRecord } from "./shared.js"; type ConfigureCandidate = { type: "models.providers.apiKey" | "skills.entries.apiKey" | "channels.googlechat.serviceAccount"; path: string; + pathSegments: string[]; label: string; providerId?: string; accountId?: string; @@ -134,6 +135,7 @@ function buildCandidates(config: OpenClawConfig): ConfigureCandidate[] { out.push({ type: "models.providers.apiKey", path: `models.providers.${providerId}.apiKey`, + pathSegments: ["models", "providers", providerId, "apiKey"], label: `Provider API key: ${providerId}`, providerId, }); @@ -149,6 +151,7 @@ function buildCandidates(config: OpenClawConfig): ConfigureCandidate[] { out.push({ type: "skills.entries.apiKey", path: `skills.entries.${entryId}.apiKey`, + pathSegments: ["skills", "entries", entryId, "apiKey"], label: `Skill API key: ${entryId}`, }); } @@ -159,6 +162,7 @@ function buildCandidates(config: OpenClawConfig): ConfigureCandidate[] { out.push({ type: "channels.googlechat.serviceAccount", path: "channels.googlechat.serviceAccount", + pathSegments: ["channels", "googlechat", "serviceAccount"], label: "Google Chat serviceAccount (default)", }); const accounts = googlechat.accounts; @@ -170,6 +174,7 @@ function buildCandidates(config: OpenClawConfig): ConfigureCandidate[] { out.push({ type: "channels.googlechat.serviceAccount", path: `channels.googlechat.accounts.${accountId}.serviceAccount`, + pathSegments: ["channels", "googlechat", "accounts", accountId, "serviceAccount"], label: `Google Chat serviceAccount (${accountId})`, accountId, }); @@ -845,6 +850,7 @@ export async function runSecretsConfigureInteractive( targets: [...selectedByPath.values()].map((entry) => ({ type: entry.type, path: entry.path, + pathSegments: [...entry.pathSegments], ref: entry.ref, ...(entry.providerId ? { providerId: entry.providerId } : {}), ...(entry.accountId ? { accountId: entry.accountId } : {}), diff --git a/src/secrets/plan.ts b/src/secrets/plan.ts index 088c27fc4fb..9a39f0fa292 100644 --- a/src/secrets/plan.ts +++ b/src/secrets/plan.ts @@ -1,4 +1,5 @@ import type { SecretProviderConfig, SecretRef } from "../config/types.secrets.js"; +import { SecretProviderSchema } from "../config/zod-schema.core.js"; export type SecretsPlanTargetType = | "models.providers.apiKey" @@ -12,6 +13,11 @@ export type SecretsPlanTarget = { * Example: "models.providers.openai.apiKey" */ path: string; + /** + * Canonical path segments used for safe mutation. + * Example: ["models", "providers", "openai", "apiKey"] + */ + pathSegments?: string[]; ref: SecretRef; /** * For provider targets, used to scrub auth-profile/static residues. @@ -44,70 +50,8 @@ function isObjectRecord(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); } -function isStringArray(value: unknown): value is string[] { - return Array.isArray(value) && value.every((entry) => typeof entry === "string"); -} - function isSecretProviderConfigShape(value: unknown): value is SecretProviderConfig { - if (!isObjectRecord(value) || typeof value.source !== "string") { - return false; - } - - if (value.source === "env") { - if (value.allowlist !== undefined && !isStringArray(value.allowlist)) { - return false; - } - return true; - } - - if (value.source === "file") { - if (typeof value.path !== "string" || value.path.trim().length === 0) { - return false; - } - if (value.mode !== undefined && value.mode !== "json" && value.mode !== "singleValue") { - return false; - } - return true; - } - - if (value.source === "exec") { - if (typeof value.command !== "string" || value.command.trim().length === 0) { - return false; - } - if (value.args !== undefined && !isStringArray(value.args)) { - return false; - } - if ( - value.passEnv !== undefined && - (!Array.isArray(value.passEnv) || !value.passEnv.every((entry) => typeof entry === "string")) - ) { - return false; - } - if ( - value.trustedDirs !== undefined && - (!Array.isArray(value.trustedDirs) || - !value.trustedDirs.every((entry) => typeof entry === "string")) - ) { - return false; - } - if (value.allowInsecurePath !== undefined && typeof value.allowInsecurePath !== "boolean") { - return false; - } - if (value.allowSymlinkCommand !== undefined && typeof value.allowSymlinkCommand !== "boolean") { - return false; - } - if (value.env !== undefined) { - if (!isObjectRecord(value.env)) { - return false; - } - if (!Object.values(value.env).every((entry) => typeof entry === "string")) { - return false; - } - } - return true; - } - - return false; + return SecretProviderSchema.safeParse(value).success; } export function isSecretsApplyPlan(value: unknown): value is SecretsApplyPlan { @@ -130,6 +74,12 @@ export function isSecretsApplyPlan(value: unknown): value is SecretsApplyPlan { candidate.type !== "channels.googlechat.serviceAccount") || typeof candidate.path !== "string" || !candidate.path.trim() || + (candidate.pathSegments !== undefined && + (!Array.isArray(candidate.pathSegments) || + candidate.pathSegments.length === 0 || + candidate.pathSegments.some( + (segment) => typeof segment !== "string" || segment.trim().length === 0, + ))) || !ref || typeof ref !== "object" || (ref.source !== "env" && ref.source !== "file" && ref.source !== "exec") ||