fix(secrets): harden plan target paths and ref-only auth profiles
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import type { OpenClawConfig } from "../config/config.js";
|
||||
import type { ModelDefinitionConfig } from "../config/types.models.js";
|
||||
import { coerceSecretRef } from "../config/types.secrets.js";
|
||||
import { createSubsystemLogger } from "../logging/subsystem.js";
|
||||
import {
|
||||
DEFAULT_COPILOT_API_BASE_URL,
|
||||
@@ -357,10 +358,24 @@ function resolveApiKeyFromProfiles(params: {
|
||||
continue;
|
||||
}
|
||||
if (cred.type === "api_key") {
|
||||
return cred.key;
|
||||
if (cred.key?.trim()) {
|
||||
return cred.key;
|
||||
}
|
||||
const keyRef = coerceSecretRef(cred.keyRef);
|
||||
if (keyRef?.source === "env" && keyRef.id.trim()) {
|
||||
return keyRef.id.trim();
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (cred.type === "token") {
|
||||
return cred.token;
|
||||
if (cred.token?.trim()) {
|
||||
return cred.token;
|
||||
}
|
||||
const tokenRef = coerceSecretRef(cred.tokenRef);
|
||||
if (tokenRef?.source === "env" && tokenRef.id.trim()) {
|
||||
return tokenRef.id.trim();
|
||||
}
|
||||
continue;
|
||||
}
|
||||
}
|
||||
return undefined;
|
||||
@@ -1017,7 +1032,13 @@ export async function resolveImplicitCopilotProvider(params: {
|
||||
const profileId = listProfilesForProvider(authStore, "github-copilot")[0];
|
||||
const profile = profileId ? authStore.profiles[profileId] : undefined;
|
||||
if (profile && profile.type === "token") {
|
||||
selectedGithubToken = profile.token;
|
||||
selectedGithubToken = profile.token?.trim() ?? "";
|
||||
if (!selectedGithubToken) {
|
||||
const tokenRef = coerceSecretRef(profile.tokenRef);
|
||||
if (tokenRef?.source === "env" && tokenRef.id.trim()) {
|
||||
selectedGithubToken = (env[tokenRef.id] ?? process.env[tokenRef.id] ?? "").trim();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -3,6 +3,7 @@ import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { captureEnv } from "../test-utils/env.js";
|
||||
import { upsertAuthProfile } from "./auth-profiles.js";
|
||||
import { resolveImplicitProviders } from "./models-config.providers.js";
|
||||
|
||||
describe("Volcengine and BytePlus providers", () => {
|
||||
@@ -37,4 +38,40 @@ describe("Volcengine and BytePlus providers", () => {
|
||||
envSnapshot.restore();
|
||||
}
|
||||
});
|
||||
|
||||
it("includes providers when auth profiles are env keyRef-only", async () => {
|
||||
const agentDir = mkdtempSync(join(tmpdir(), "openclaw-test-"));
|
||||
const envSnapshot = captureEnv(["VOLCANO_ENGINE_API_KEY", "BYTEPLUS_API_KEY"]);
|
||||
delete process.env.VOLCANO_ENGINE_API_KEY;
|
||||
delete process.env.BYTEPLUS_API_KEY;
|
||||
|
||||
upsertAuthProfile({
|
||||
profileId: "volcengine:default",
|
||||
credential: {
|
||||
type: "api_key",
|
||||
provider: "volcengine",
|
||||
keyRef: { source: "env", provider: "default", id: "VOLCANO_ENGINE_API_KEY" },
|
||||
},
|
||||
agentDir,
|
||||
});
|
||||
upsertAuthProfile({
|
||||
profileId: "byteplus:default",
|
||||
credential: {
|
||||
type: "api_key",
|
||||
provider: "byteplus",
|
||||
keyRef: { source: "env", provider: "default", id: "BYTEPLUS_API_KEY" },
|
||||
},
|
||||
agentDir,
|
||||
});
|
||||
|
||||
try {
|
||||
const providers = await resolveImplicitProviders({ agentDir });
|
||||
expect(providers?.volcengine?.apiKey).toBe("VOLCANO_ENGINE_API_KEY");
|
||||
expect(providers?.["volcengine-plan"]?.apiKey).toBe("VOLCANO_ENGINE_API_KEY");
|
||||
expect(providers?.byteplus?.apiKey).toBe("BYTEPLUS_API_KEY");
|
||||
expect(providers?.["byteplus-plan"]?.apiKey).toBe("BYTEPLUS_API_KEY");
|
||||
} finally {
|
||||
envSnapshot.restore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -76,4 +76,38 @@ describe("models-config", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it("uses tokenRef env var when github-copilot profile omits plaintext token", async () => {
|
||||
await withTempHome(async (home) => {
|
||||
await withUnsetCopilotTokenEnv(async () => {
|
||||
const fetchMock = mockCopilotTokenExchangeSuccess();
|
||||
const agentDir = path.join(home, "agent-profiles");
|
||||
await fs.mkdir(agentDir, { recursive: true });
|
||||
process.env.COPILOT_REF_TOKEN = "token-from-ref-env";
|
||||
await fs.writeFile(
|
||||
path.join(agentDir, "auth-profiles.json"),
|
||||
JSON.stringify(
|
||||
{
|
||||
version: 1,
|
||||
profiles: {
|
||||
"github-copilot:default": {
|
||||
type: "token",
|
||||
provider: "github-copilot",
|
||||
tokenRef: { source: "env", provider: "default", id: "COPILOT_REF_TOKEN" },
|
||||
},
|
||||
},
|
||||
},
|
||||
null,
|
||||
2,
|
||||
),
|
||||
);
|
||||
|
||||
await ensureOpenClawModelsJson({ models: { providers: {} } }, agentDir);
|
||||
|
||||
const [, opts] = fetchMock.mock.calls[0] as [string, { headers?: Record<string, string> }];
|
||||
expect(opts?.headers?.Authorization).toBe("Bearer token-from-ref-env");
|
||||
delete process.env.COPILOT_REF_TOKEN;
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -320,6 +320,49 @@ describe("secrets apply", () => {
|
||||
expect(rawConfig).not.toContain("sk-skill-plaintext");
|
||||
});
|
||||
|
||||
it("rejects plan targets that do not match allowed secret-bearing paths", async () => {
|
||||
const plan: SecretsApplyPlan = {
|
||||
version: 1,
|
||||
protocolVersion: 1,
|
||||
generatedAt: new Date().toISOString(),
|
||||
generatedBy: "manual",
|
||||
targets: [
|
||||
{
|
||||
type: "models.providers.apiKey",
|
||||
path: "models.providers.openai.baseUrl",
|
||||
pathSegments: ["models", "providers", "openai", "baseUrl"],
|
||||
providerId: "openai",
|
||||
ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" },
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
await expect(runSecretsApply({ plan, env, write: false })).rejects.toThrow(
|
||||
"Invalid plan target path",
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects plan targets with forbidden prototype-like path segments", async () => {
|
||||
const plan: SecretsApplyPlan = {
|
||||
version: 1,
|
||||
protocolVersion: 1,
|
||||
generatedAt: new Date().toISOString(),
|
||||
generatedBy: "manual",
|
||||
targets: [
|
||||
{
|
||||
type: "skills.entries.apiKey",
|
||||
path: "skills.entries.__proto__.apiKey",
|
||||
pathSegments: ["skills", "entries", "__proto__", "apiKey"],
|
||||
ref: { source: "env", provider: "default", id: "OPENAI_API_KEY" },
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
await expect(runSecretsApply({ plan, env, write: false })).rejects.toThrow(
|
||||
"Invalid plan target path",
|
||||
);
|
||||
});
|
||||
|
||||
it("applies provider upserts and deletes from plan", async () => {
|
||||
await fs.writeFile(
|
||||
configPath,
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
type SecretsApplyPlan,
|
||||
type SecretsPlanTarget,
|
||||
normalizeSecretsPlanOptions,
|
||||
resolveValidatedTargetPathSegments,
|
||||
} from "./plan.js";
|
||||
import { listKnownSecretEnvVarNames } from "./provider-env-vars.js";
|
||||
import { resolveSecretRefValue } from "./resolve.js";
|
||||
@@ -52,10 +53,6 @@ export type SecretsApplyResult = {
|
||||
warnings: string[];
|
||||
};
|
||||
|
||||
function parseDotPath(pathname: string): string[] {
|
||||
return pathname.split(".").filter(Boolean);
|
||||
}
|
||||
|
||||
function getByPathSegments(root: unknown, segments: string[]): unknown {
|
||||
if (segments.length === 0) {
|
||||
return undefined;
|
||||
@@ -114,15 +111,11 @@ function deleteByPathSegments(root: OpenClawConfig, segments: string[]): boolean
|
||||
}
|
||||
|
||||
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];
|
||||
const resolved = resolveValidatedTargetPathSegments(target);
|
||||
if (!resolved) {
|
||||
throw new Error(`Invalid plan target path for ${target.type}: ${target.path}`);
|
||||
}
|
||||
return parseDotPath(target.path);
|
||||
return resolved;
|
||||
}
|
||||
|
||||
function parseEnvValue(raw: string): string {
|
||||
|
||||
@@ -45,6 +45,15 @@ export type SecretsApplyPlan = {
|
||||
};
|
||||
|
||||
const PROVIDER_ALIAS_PATTERN = /^[a-z][a-z0-9_-]{0,63}$/;
|
||||
const FORBIDDEN_PATH_SEGMENTS = new Set(["__proto__", "prototype", "constructor"]);
|
||||
|
||||
function isSecretsPlanTargetType(value: unknown): value is SecretsPlanTargetType {
|
||||
return (
|
||||
value === "models.providers.apiKey" ||
|
||||
value === "skills.entries.apiKey" ||
|
||||
value === "channels.googlechat.serviceAccount"
|
||||
);
|
||||
}
|
||||
|
||||
function isObjectRecord(value: unknown): value is Record<string, unknown> {
|
||||
return Boolean(value) && typeof value === "object" && !Array.isArray(value);
|
||||
@@ -54,6 +63,104 @@ function isSecretProviderConfigShape(value: unknown): value is SecretProviderCon
|
||||
return SecretProviderSchema.safeParse(value).success;
|
||||
}
|
||||
|
||||
function parseDotPath(pathname: string): string[] {
|
||||
return pathname
|
||||
.split(".")
|
||||
.map((segment) => segment.trim())
|
||||
.filter((segment) => segment.length > 0);
|
||||
}
|
||||
|
||||
function hasForbiddenPathSegment(segments: string[]): boolean {
|
||||
return segments.some((segment) => FORBIDDEN_PATH_SEGMENTS.has(segment));
|
||||
}
|
||||
|
||||
function hasMatchingPathShape(
|
||||
candidate: Pick<SecretsPlanTarget, "type" | "providerId" | "accountId">,
|
||||
segments: string[],
|
||||
): boolean {
|
||||
if (candidate.type === "models.providers.apiKey") {
|
||||
if (
|
||||
segments.length !== 4 ||
|
||||
segments[0] !== "models" ||
|
||||
segments[1] !== "providers" ||
|
||||
segments[3] !== "apiKey"
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
return (
|
||||
candidate.providerId === undefined ||
|
||||
candidate.providerId.trim().length === 0 ||
|
||||
candidate.providerId === segments[2]
|
||||
);
|
||||
}
|
||||
if (candidate.type === "skills.entries.apiKey") {
|
||||
return (
|
||||
segments.length === 4 &&
|
||||
segments[0] === "skills" &&
|
||||
segments[1] === "entries" &&
|
||||
segments[3] === "apiKey"
|
||||
);
|
||||
}
|
||||
if (
|
||||
segments.length === 3 &&
|
||||
segments[0] === "channels" &&
|
||||
segments[1] === "googlechat" &&
|
||||
segments[2] === "serviceAccount"
|
||||
) {
|
||||
return candidate.accountId === undefined || candidate.accountId.trim().length === 0;
|
||||
}
|
||||
if (
|
||||
segments.length === 5 &&
|
||||
segments[0] === "channels" &&
|
||||
segments[1] === "googlechat" &&
|
||||
segments[2] === "accounts" &&
|
||||
segments[4] === "serviceAccount"
|
||||
) {
|
||||
return (
|
||||
candidate.accountId === undefined ||
|
||||
candidate.accountId.trim().length === 0 ||
|
||||
candidate.accountId === segments[3]
|
||||
);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
export function resolveValidatedTargetPathSegments(candidate: {
|
||||
type?: SecretsPlanTargetType;
|
||||
path?: string;
|
||||
pathSegments?: string[];
|
||||
providerId?: string;
|
||||
accountId?: string;
|
||||
}): string[] | null {
|
||||
if (!isSecretsPlanTargetType(candidate.type)) {
|
||||
return null;
|
||||
}
|
||||
const path = typeof candidate.path === "string" ? candidate.path.trim() : "";
|
||||
if (!path) {
|
||||
return null;
|
||||
}
|
||||
const segments =
|
||||
Array.isArray(candidate.pathSegments) && candidate.pathSegments.length > 0
|
||||
? candidate.pathSegments.map((segment) => String(segment).trim()).filter(Boolean)
|
||||
: parseDotPath(path);
|
||||
if (
|
||||
segments.length === 0 ||
|
||||
hasForbiddenPathSegment(segments) ||
|
||||
path !== segments.join(".") ||
|
||||
!hasMatchingPathShape(
|
||||
{
|
||||
type: candidate.type,
|
||||
providerId: candidate.providerId,
|
||||
accountId: candidate.accountId,
|
||||
},
|
||||
segments,
|
||||
)
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
return segments;
|
||||
}
|
||||
|
||||
export function isSecretsApplyPlan(value: unknown): value is SecretsApplyPlan {
|
||||
if (!value || typeof value !== "object" || Array.isArray(value)) {
|
||||
return false;
|
||||
@@ -74,12 +181,14 @@ 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,
|
||||
))) ||
|
||||
(candidate.pathSegments !== undefined && !Array.isArray(candidate.pathSegments)) ||
|
||||
!resolveValidatedTargetPathSegments({
|
||||
type: candidate.type,
|
||||
path: candidate.path,
|
||||
pathSegments: candidate.pathSegments,
|
||||
providerId: candidate.providerId,
|
||||
accountId: candidate.accountId,
|
||||
}) ||
|
||||
!ref ||
|
||||
typeof ref !== "object" ||
|
||||
(ref.source !== "env" && ref.source !== "file" && ref.source !== "exec") ||
|
||||
|
||||
Reference in New Issue
Block a user