fix(secrets): make apply idempotent and keep audit read-only
This commit is contained in:
committed by
Peter Steinberger
parent
f413e314b9
commit
ba2eb583c0
@@ -418,7 +418,8 @@ function loadAuthProfileStoreForAgent(
|
||||
const mergedOAuth = mergeOAuthFileIntoStore(store);
|
||||
// Keep external CLI credentials visible in runtime even during read-only loads.
|
||||
const syncedCli = syncExternalCliCredentials(store);
|
||||
const shouldWrite = !readOnly && (legacy !== null || mergedOAuth || syncedCli);
|
||||
const forceReadOnly = process.env.OPENCLAW_AUTH_STORE_READONLY === "1";
|
||||
const shouldWrite = !readOnly && !forceReadOnly && (legacy !== null || mergedOAuth || syncedCli);
|
||||
if (shouldWrite) {
|
||||
saveJsonFile(authPath, store);
|
||||
}
|
||||
|
||||
@@ -113,4 +113,49 @@ describe("discoverAuthStorage", () => {
|
||||
await fs.rm(agentDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("preserves legacy auth.json when auth store is forced read-only", async () => {
|
||||
const agentDir = await createAgentDir();
|
||||
const previous = process.env.OPENCLAW_AUTH_STORE_READONLY;
|
||||
process.env.OPENCLAW_AUTH_STORE_READONLY = "1";
|
||||
try {
|
||||
saveAuthProfileStore(
|
||||
{
|
||||
version: 1,
|
||||
profiles: {
|
||||
"openrouter:default": {
|
||||
type: "api_key",
|
||||
provider: "openrouter",
|
||||
key: "sk-or-v1-runtime",
|
||||
},
|
||||
},
|
||||
},
|
||||
agentDir,
|
||||
);
|
||||
await fs.writeFile(
|
||||
path.join(agentDir, "auth.json"),
|
||||
JSON.stringify(
|
||||
{
|
||||
openrouter: { type: "api_key", key: "legacy-static-key" },
|
||||
},
|
||||
null,
|
||||
2,
|
||||
),
|
||||
);
|
||||
|
||||
discoverAuthStorage(agentDir);
|
||||
|
||||
const parsed = JSON.parse(await fs.readFile(path.join(agentDir, "auth.json"), "utf8")) as {
|
||||
[key: string]: unknown;
|
||||
};
|
||||
expect(parsed.openrouter).toMatchObject({ type: "api_key", key: "legacy-static-key" });
|
||||
} finally {
|
||||
if (previous === undefined) {
|
||||
delete process.env.OPENCLAW_AUTH_STORE_READONLY;
|
||||
} else {
|
||||
process.env.OPENCLAW_AUTH_STORE_READONLY = previous;
|
||||
}
|
||||
await fs.rm(agentDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -15,6 +15,9 @@ function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
}
|
||||
|
||||
function scrubLegacyStaticAuthJsonEntries(pathname: string): void {
|
||||
if (process.env.OPENCLAW_AUTH_STORE_READONLY === "1") {
|
||||
return;
|
||||
}
|
||||
if (!fs.existsSync(pathname)) {
|
||||
return;
|
||||
}
|
||||
|
||||
14
src/entry.ts
14
src/entry.ts
@@ -15,6 +15,16 @@ const ENTRY_WRAPPER_PAIRS = [
|
||||
{ wrapperBasename: "openclaw.js", entryBasename: "entry.js" },
|
||||
] as const;
|
||||
|
||||
function shouldForceReadOnlyAuthStore(argv: string[]): boolean {
|
||||
const tokens = argv.slice(2).filter((token) => token.length > 0 && !token.startsWith("-"));
|
||||
for (let index = 0; index < tokens.length - 1; index += 1) {
|
||||
if (tokens[index] === "secrets" && tokens[index + 1] === "audit") {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
// Guard: only run entry-point logic when this file is the main module.
|
||||
// The bundler may import entry.js as a shared dependency when dist/index.js
|
||||
// is the actual entry point; without this guard the top-level code below
|
||||
@@ -32,6 +42,10 @@ if (
|
||||
installProcessWarningFilter();
|
||||
normalizeEnv();
|
||||
|
||||
if (shouldForceReadOnlyAuthStore(process.argv)) {
|
||||
process.env.OPENCLAW_AUTH_STORE_READONLY = "1";
|
||||
}
|
||||
|
||||
if (process.argv.includes("--no-color")) {
|
||||
process.env.NO_COLOR = "1";
|
||||
process.env.FORCE_COLOR = "0";
|
||||
|
||||
@@ -146,4 +146,34 @@ describe("secrets apply", () => {
|
||||
expect(nextEnv).not.toContain("sk-openai-plaintext");
|
||||
expect(nextEnv).toContain("UNRELATED=value");
|
||||
});
|
||||
|
||||
it("is idempotent on repeated write applies", async () => {
|
||||
const plan: SecretsApplyPlan = {
|
||||
version: 1,
|
||||
protocolVersion: 1,
|
||||
generatedAt: new Date().toISOString(),
|
||||
generatedBy: "manual",
|
||||
targets: [
|
||||
{
|
||||
type: "models.providers.apiKey",
|
||||
path: "models.providers.openai.apiKey",
|
||||
providerId: "openai",
|
||||
ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" },
|
||||
},
|
||||
],
|
||||
options: {
|
||||
scrubEnv: true,
|
||||
scrubAuthProfilesForProviderTargets: true,
|
||||
scrubLegacyAuthJson: true,
|
||||
},
|
||||
};
|
||||
|
||||
const first = await runSecretsApply({ plan, env, write: true });
|
||||
expect(first.changed).toBe(true);
|
||||
|
||||
const second = await runSecretsApply({ plan, env, write: true });
|
||||
expect(second.mode).toBe("write");
|
||||
expect(second.changed).toBe(false);
|
||||
expect(second.changedFiles).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { isDeepStrictEqual } from "node:util";
|
||||
import { listAgentIds, resolveAgentDir } from "../agents/agent-scope.js";
|
||||
import { loadAuthProfileStoreForSecretsRuntime } from "../agents/auth-profiles.js";
|
||||
import { resolveAuthStorePath } from "../agents/auth-profiles/paths.js";
|
||||
@@ -62,36 +63,49 @@ function getByDotPath(root: unknown, pathLabel: string): unknown {
|
||||
return cursor;
|
||||
}
|
||||
|
||||
function setByDotPath(root: OpenClawConfig, pathLabel: string, value: unknown): void {
|
||||
function setByDotPath(root: OpenClawConfig, pathLabel: string, value: unknown): boolean {
|
||||
const segments = parseDotPath(pathLabel);
|
||||
if (segments.length === 0) {
|
||||
throw new Error("Target path is empty.");
|
||||
}
|
||||
let cursor: Record<string, unknown> = root as unknown as Record<string, unknown>;
|
||||
let changed = false;
|
||||
for (const segment of segments.slice(0, -1)) {
|
||||
const existing = cursor[segment];
|
||||
if (!isRecord(existing)) {
|
||||
cursor[segment] = {};
|
||||
changed = true;
|
||||
}
|
||||
cursor = cursor[segment] as Record<string, unknown>;
|
||||
}
|
||||
cursor[segments[segments.length - 1]] = value;
|
||||
const leaf = segments[segments.length - 1] ?? "";
|
||||
const previous = cursor[leaf];
|
||||
if (!isDeepStrictEqual(previous, value)) {
|
||||
cursor[leaf] = value;
|
||||
changed = true;
|
||||
}
|
||||
return changed;
|
||||
}
|
||||
|
||||
function deleteByDotPath(root: OpenClawConfig, pathLabel: string): void {
|
||||
function deleteByDotPath(root: OpenClawConfig, pathLabel: string): boolean {
|
||||
const segments = parseDotPath(pathLabel);
|
||||
if (segments.length === 0) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
let cursor: Record<string, unknown> = root as unknown as Record<string, unknown>;
|
||||
for (const segment of segments.slice(0, -1)) {
|
||||
const existing = cursor[segment];
|
||||
if (!isRecord(existing)) {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
cursor = existing;
|
||||
}
|
||||
delete cursor[segments[segments.length - 1]];
|
||||
const leaf = segments[segments.length - 1] ?? "";
|
||||
if (!Object.prototype.hasOwnProperty.call(cursor, leaf)) {
|
||||
return false;
|
||||
}
|
||||
delete cursor[leaf];
|
||||
return true;
|
||||
}
|
||||
|
||||
function parseEnvValue(raw: string): string {
|
||||
@@ -219,9 +233,11 @@ async function projectPlanState(params: {
|
||||
scrubbedValues.add(previous.trim());
|
||||
}
|
||||
const refPath = resolveGoogleChatRefPath(target.path);
|
||||
setByDotPath(nextConfig, refPath, target.ref);
|
||||
deleteByDotPath(nextConfig, target.path);
|
||||
changedFiles.add(resolveUserPath(snapshot.path));
|
||||
const wroteRef = setByDotPath(nextConfig, refPath, target.ref);
|
||||
const deletedLegacy = deleteByDotPath(nextConfig, target.path);
|
||||
if (wroteRef || deletedLegacy) {
|
||||
changedFiles.add(resolveUserPath(snapshot.path));
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -229,8 +245,10 @@ async function projectPlanState(params: {
|
||||
if (isNonEmptyString(previous)) {
|
||||
scrubbedValues.add(previous.trim());
|
||||
}
|
||||
setByDotPath(nextConfig, target.path, target.ref);
|
||||
changedFiles.add(resolveUserPath(snapshot.path));
|
||||
const wroteRef = setByDotPath(nextConfig, target.path, target.ref);
|
||||
if (wroteRef) {
|
||||
changedFiles.add(resolveUserPath(snapshot.path));
|
||||
}
|
||||
if (target.type === "models.providers.apiKey" && target.providerId) {
|
||||
providerTargets.add(normalizeProviderId(target.providerId));
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ describe("secrets audit", () => {
|
||||
let stateDir = "";
|
||||
let configPath = "";
|
||||
let authStorePath = "";
|
||||
let authJsonPath = "";
|
||||
let envPath = "";
|
||||
let env: NodeJS.ProcessEnv;
|
||||
|
||||
@@ -17,6 +18,7 @@ describe("secrets audit", () => {
|
||||
stateDir = path.join(rootDir, ".openclaw");
|
||||
configPath = path.join(stateDir, "openclaw.json");
|
||||
authStorePath = path.join(stateDir, "agents", "main", "agent", "auth-profiles.json");
|
||||
authJsonPath = path.join(stateDir, "agents", "main", "agent", "auth.json");
|
||||
envPath = path.join(stateDir, ".env");
|
||||
env = {
|
||||
...process.env,
|
||||
@@ -80,4 +82,27 @@ describe("secrets audit", () => {
|
||||
expect(report.findings.some((entry) => entry.code === "REF_SHADOWED")).toBe(true);
|
||||
expect(report.findings.some((entry) => entry.code === "PLAINTEXT_FOUND")).toBe(true);
|
||||
});
|
||||
|
||||
it("does not mutate legacy auth.json during audit", async () => {
|
||||
await fs.rm(authStorePath, { force: true });
|
||||
await fs.writeFile(
|
||||
authJsonPath,
|
||||
`${JSON.stringify(
|
||||
{
|
||||
openai: {
|
||||
type: "api_key",
|
||||
key: "sk-legacy-auth-json",
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
)}\n`,
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const report = await runSecretsAudit({ env });
|
||||
expect(report.findings.some((entry) => entry.code === "LEGACY_RESIDUE")).toBe(true);
|
||||
await expect(fs.stat(authJsonPath)).resolves.toBeTruthy();
|
||||
await expect(fs.stat(authStorePath)).rejects.toMatchObject({ code: "ENOENT" });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -510,76 +510,86 @@ export async function runSecretsAudit(
|
||||
} = {},
|
||||
): Promise<SecretsAuditReport> {
|
||||
const env = params.env ?? process.env;
|
||||
const io = createSecretsConfigIO({ env });
|
||||
const { snapshot } = await io.readConfigFileSnapshotForWrite();
|
||||
const configPath = resolveUserPath(snapshot.path);
|
||||
const defaults = snapshot.valid ? snapshot.config.secrets?.defaults : undefined;
|
||||
const previousAuthStoreReadOnly = process.env.OPENCLAW_AUTH_STORE_READONLY;
|
||||
process.env.OPENCLAW_AUTH_STORE_READONLY = "1";
|
||||
try {
|
||||
const io = createSecretsConfigIO({ env });
|
||||
const snapshot = await io.readConfigFileSnapshot();
|
||||
const configPath = resolveUserPath(snapshot.path);
|
||||
const defaults = snapshot.valid ? snapshot.config.secrets?.defaults : undefined;
|
||||
|
||||
const collector: AuditCollector = {
|
||||
findings: [],
|
||||
refAssignments: [],
|
||||
configProviderRefPaths: new Map(),
|
||||
authProviderState: new Map(),
|
||||
filesScanned: new Set([configPath]),
|
||||
};
|
||||
const collector: AuditCollector = {
|
||||
findings: [],
|
||||
refAssignments: [],
|
||||
configProviderRefPaths: new Map(),
|
||||
authProviderState: new Map(),
|
||||
filesScanned: new Set([configPath]),
|
||||
};
|
||||
|
||||
const stateDir = resolveStateDir(env, os.homedir);
|
||||
const envPath = path.join(resolveConfigDir(env, os.homedir), ".env");
|
||||
const config = snapshot.valid ? snapshot.config : ({} as OpenClawConfig);
|
||||
const stateDir = resolveStateDir(env, os.homedir);
|
||||
const envPath = path.join(resolveConfigDir(env, os.homedir), ".env");
|
||||
const config = snapshot.valid ? snapshot.config : ({} as OpenClawConfig);
|
||||
|
||||
if (snapshot.valid) {
|
||||
collectConfigSecrets({
|
||||
config,
|
||||
configPath,
|
||||
collector,
|
||||
});
|
||||
for (const authStorePath of collectAuthStorePaths(config, stateDir)) {
|
||||
collectAuthStoreSecrets({
|
||||
authStorePath,
|
||||
if (snapshot.valid) {
|
||||
collectConfigSecrets({
|
||||
config,
|
||||
configPath,
|
||||
collector,
|
||||
defaults,
|
||||
});
|
||||
for (const authStorePath of collectAuthStorePaths(config, stateDir)) {
|
||||
collectAuthStoreSecrets({
|
||||
authStorePath,
|
||||
collector,
|
||||
defaults,
|
||||
});
|
||||
}
|
||||
await collectUnresolvedRefFindings({
|
||||
collector,
|
||||
config,
|
||||
env,
|
||||
});
|
||||
collectShadowingFindings(collector);
|
||||
} else {
|
||||
addFinding(collector, {
|
||||
code: "REF_UNRESOLVED",
|
||||
severity: "error",
|
||||
file: configPath,
|
||||
jsonPath: "<root>",
|
||||
message: "Config is invalid; cannot validate secret references reliably.",
|
||||
});
|
||||
}
|
||||
await collectUnresolvedRefFindings({
|
||||
|
||||
collectEnvPlaintext({
|
||||
envPath,
|
||||
collector,
|
||||
config,
|
||||
env,
|
||||
});
|
||||
collectShadowingFindings(collector);
|
||||
} else {
|
||||
addFinding(collector, {
|
||||
code: "REF_UNRESOLVED",
|
||||
severity: "error",
|
||||
file: configPath,
|
||||
jsonPath: "<root>",
|
||||
message: "Config is invalid; cannot validate secret references reliably.",
|
||||
collectAuthJsonResidue({
|
||||
stateDir,
|
||||
collector,
|
||||
});
|
||||
|
||||
const summary = summarizeFindings(collector.findings);
|
||||
const status: SecretsAuditStatus =
|
||||
summary.unresolvedRefCount > 0
|
||||
? "unresolved"
|
||||
: collector.findings.length > 0
|
||||
? "findings"
|
||||
: "clean";
|
||||
|
||||
return {
|
||||
version: 1,
|
||||
status,
|
||||
filesScanned: [...collector.filesScanned].toSorted(),
|
||||
summary,
|
||||
findings: collector.findings,
|
||||
};
|
||||
} finally {
|
||||
if (previousAuthStoreReadOnly === undefined) {
|
||||
delete process.env.OPENCLAW_AUTH_STORE_READONLY;
|
||||
} else {
|
||||
process.env.OPENCLAW_AUTH_STORE_READONLY = previousAuthStoreReadOnly;
|
||||
}
|
||||
}
|
||||
|
||||
collectEnvPlaintext({
|
||||
envPath,
|
||||
collector,
|
||||
});
|
||||
collectAuthJsonResidue({
|
||||
stateDir,
|
||||
collector,
|
||||
});
|
||||
|
||||
const summary = summarizeFindings(collector.findings);
|
||||
const status: SecretsAuditStatus =
|
||||
summary.unresolvedRefCount > 0
|
||||
? "unresolved"
|
||||
: collector.findings.length > 0
|
||||
? "findings"
|
||||
: "clean";
|
||||
|
||||
return {
|
||||
version: 1,
|
||||
status,
|
||||
filesScanned: [...collector.filesScanned].toSorted(),
|
||||
summary,
|
||||
findings: collector.findings,
|
||||
};
|
||||
}
|
||||
|
||||
export function resolveSecretsAuditExitCode(report: SecretsAuditReport, check: boolean): number {
|
||||
|
||||
Reference in New Issue
Block a user