fix(secrets): harden apply and audit plan handling

This commit is contained in:
joshavant
2026-02-26 00:02:02 -06:00
committed by Peter Steinberger
parent ea1ccf4896
commit d879c7c641
7 changed files with 345 additions and 101 deletions

View File

@@ -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<string, { apiKey?: unknown }>;
};
};
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,

View File

@@ -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<string, FileSnapshot>();

View File

@@ -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);
});
});

View File

@@ -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<string, unknown> | null {
function readJsonObject(filePath: string): {
value: Record<string, unknown> | 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: "<root>",
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: "<root>",
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<void> {
const cache: SecretRefResolveCache = {};
const refsByProvider = new Map<string, Map<string, SecretRef>>();
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<string, SecretRef>();
refsByProvider.set(providerKey, refsForProvider);
}
refsForProvider.set(secretRefKey(assignment.ref), assignment.ref);
}
const resolvedByRefKey = new Map<string, unknown>();
const errorsByRefKey = new Map<string, unknown>();
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,

View File

@@ -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 } : {}),

View File

@@ -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<string, unknown> {
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") ||