mirror of https://github.com/openclaw/openclaw.git
fix: doctor gracefully skips channels with unresolved SecretRefs (closes #46154)
noteSecurityWarnings called resolveDefaultChannelAccountContext which threw on unresolved SecretRefs (e.g. env-based Discord tokens), making openclaw doctor completely unusable. Now catches the error and skips the channel plugin, matching the documented read-only degradation behavior for doctor flows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
9616d1e8ba
commit
0d59d6401f
File diff suppressed because it is too large
Load Diff
|
|
@ -386,7 +386,7 @@ export default function register(api: OpenClawPluginApi) {
|
|||
return { text: "Pairing request not found." };
|
||||
}
|
||||
const approved = await approveDevicePairing(pending.requestId);
|
||||
if (!approved) {
|
||||
if (!approved || approved.status === "forbidden") {
|
||||
return { text: "Pairing request not found." };
|
||||
}
|
||||
const label = approved.device.displayName?.trim() || approved.device.deviceId;
|
||||
|
|
|
|||
|
|
@ -158,7 +158,7 @@ async function approvePairingWithFallback(
|
|||
defaultRuntime.log(theme.warn(FALLBACK_NOTICE));
|
||||
}
|
||||
const approved = await approveDevicePairing(requestId);
|
||||
if (!approved) {
|
||||
if (!approved || approved.status === "forbidden") {
|
||||
return null;
|
||||
}
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -189,8 +189,20 @@ export async function noteSecurityWarnings(cfg: OpenClawConfig) {
|
|||
if (!plugin.security) {
|
||||
continue;
|
||||
}
|
||||
const { defaultAccountId, account, enabled, configured } =
|
||||
await resolveDefaultChannelAccountContext(plugin, cfg);
|
||||
let ctx: Awaited<ReturnType<typeof resolveDefaultChannelAccountContext>>;
|
||||
try {
|
||||
ctx = await resolveDefaultChannelAccountContext(plugin, cfg);
|
||||
} catch {
|
||||
// Unresolved SecretRefs (e.g. env-based tokens) are expected in
|
||||
// read-only doctor flows — skip the plugin but surface a warning
|
||||
// so the user knows security diagnostics were incomplete.
|
||||
const label = plugin.meta.label ?? plugin.id;
|
||||
warnings.push(
|
||||
`- ${label}: skipped security checks (account could not be resolved — likely unresolved SecretRef).`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
const { defaultAccountId, account, enabled, configured } = ctx;
|
||||
if (!enabled) {
|
||||
continue;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,15 +7,28 @@ import type { ContextEngine } from "./types.js";
|
|||
* Supports async creation for engines that need DB connections etc.
|
||||
*/
|
||||
export type ContextEngineFactory = () => ContextEngine | Promise<ContextEngine>;
|
||||
export type ContextEngineRegistrationResult = { ok: true } | { ok: false; existingOwner: string };
|
||||
|
||||
type RegisterContextEngineForOwnerOptions = {
|
||||
allowSameOwnerRefresh?: boolean;
|
||||
};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Registry (module-level singleton)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const CONTEXT_ENGINE_REGISTRY_STATE = Symbol.for("openclaw.contextEngineRegistryState");
|
||||
const CORE_CONTEXT_ENGINE_OWNER = "core";
|
||||
const PUBLIC_CONTEXT_ENGINE_OWNER = "public-sdk";
|
||||
|
||||
type ContextEngineRegistryState = {
|
||||
engines: Map<string, ContextEngineFactory>;
|
||||
engines: Map<
|
||||
string,
|
||||
{
|
||||
factory: ContextEngineFactory;
|
||||
owner: string;
|
||||
}
|
||||
>;
|
||||
};
|
||||
|
||||
// Keep context-engine registrations process-global so duplicated dist chunks
|
||||
|
|
@ -26,24 +39,69 @@ function getContextEngineRegistryState(): ContextEngineRegistryState {
|
|||
};
|
||||
if (!globalState[CONTEXT_ENGINE_REGISTRY_STATE]) {
|
||||
globalState[CONTEXT_ENGINE_REGISTRY_STATE] = {
|
||||
engines: new Map<string, ContextEngineFactory>(),
|
||||
engines: new Map(),
|
||||
};
|
||||
}
|
||||
return globalState[CONTEXT_ENGINE_REGISTRY_STATE];
|
||||
}
|
||||
|
||||
function requireContextEngineOwner(owner: string): string {
|
||||
const normalizedOwner = owner.trim();
|
||||
if (!normalizedOwner) {
|
||||
throw new Error(
|
||||
`registerContextEngineForOwner: owner must be a non-empty string, got ${JSON.stringify(owner)}`,
|
||||
);
|
||||
}
|
||||
return normalizedOwner;
|
||||
}
|
||||
|
||||
/**
|
||||
* Register a context engine implementation under the given id.
|
||||
* Register a context engine implementation under an explicit trusted owner.
|
||||
*/
|
||||
export function registerContextEngine(id: string, factory: ContextEngineFactory): void {
|
||||
getContextEngineRegistryState().engines.set(id, factory);
|
||||
export function registerContextEngineForOwner(
|
||||
id: string,
|
||||
factory: ContextEngineFactory,
|
||||
owner: string,
|
||||
opts?: RegisterContextEngineForOwnerOptions,
|
||||
): ContextEngineRegistrationResult {
|
||||
const normalizedOwner = requireContextEngineOwner(owner);
|
||||
const registry = getContextEngineRegistryState().engines;
|
||||
const existing = registry.get(id);
|
||||
if (
|
||||
id === defaultSlotIdForKey("contextEngine") &&
|
||||
normalizedOwner !== CORE_CONTEXT_ENGINE_OWNER
|
||||
) {
|
||||
return { ok: false, existingOwner: CORE_CONTEXT_ENGINE_OWNER };
|
||||
}
|
||||
if (existing && existing.owner !== normalizedOwner) {
|
||||
return { ok: false, existingOwner: existing.owner };
|
||||
}
|
||||
if (existing && opts?.allowSameOwnerRefresh !== true) {
|
||||
return { ok: false, existingOwner: existing.owner };
|
||||
}
|
||||
registry.set(id, { factory, owner: normalizedOwner });
|
||||
return { ok: true };
|
||||
}
|
||||
|
||||
/**
|
||||
* Public SDK entry point for third-party registrations.
|
||||
*
|
||||
* This path is intentionally unprivileged: it cannot claim core-owned ids and
|
||||
* it cannot safely refresh an existing registration because the caller's
|
||||
* identity is not authenticated.
|
||||
*/
|
||||
export function registerContextEngine(
|
||||
id: string,
|
||||
factory: ContextEngineFactory,
|
||||
): ContextEngineRegistrationResult {
|
||||
return registerContextEngineForOwner(id, factory, PUBLIC_CONTEXT_ENGINE_OWNER);
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the factory for a registered engine, or undefined.
|
||||
*/
|
||||
export function getContextEngineFactory(id: string): ContextEngineFactory | undefined {
|
||||
return getContextEngineRegistryState().engines.get(id);
|
||||
return getContextEngineRegistryState().engines.get(id)?.factory;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -73,13 +131,13 @@ export async function resolveContextEngine(config?: OpenClawConfig): Promise<Con
|
|||
? slotValue.trim()
|
||||
: defaultSlotIdForKey("contextEngine");
|
||||
|
||||
const factory = getContextEngineRegistryState().engines.get(engineId);
|
||||
if (!factory) {
|
||||
const entry = getContextEngineRegistryState().engines.get(engineId);
|
||||
if (!entry) {
|
||||
throw new Error(
|
||||
`Context engine "${engineId}" is not registered. ` +
|
||||
`Available engines: ${listContextEngineIds().join(", ") || "(none)"}`,
|
||||
);
|
||||
}
|
||||
|
||||
return factory();
|
||||
return entry.factory();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -78,7 +78,7 @@ export const deviceHandlers: GatewayRequestHandlers = {
|
|||
undefined,
|
||||
);
|
||||
},
|
||||
"device.pair.approve": async ({ params, respond, context }) => {
|
||||
"device.pair.approve": async ({ params, respond, context, client }) => {
|
||||
if (!validateDevicePairApproveParams(params)) {
|
||||
respond(
|
||||
false,
|
||||
|
|
@ -93,11 +93,22 @@ export const deviceHandlers: GatewayRequestHandlers = {
|
|||
return;
|
||||
}
|
||||
const { requestId } = params as { requestId: string };
|
||||
const approved = await approveDevicePairing(requestId);
|
||||
const callerScopes = Array.isArray(client?.connect?.scopes) ? client.connect.scopes : [];
|
||||
|
||||
// Scope validation and approval happen atomically inside the same lock.
|
||||
const approved = await approveDevicePairing(requestId, { callerScopes });
|
||||
if (!approved) {
|
||||
respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "unknown requestId"));
|
||||
return;
|
||||
}
|
||||
if (approved.status === "forbidden") {
|
||||
respond(
|
||||
false,
|
||||
undefined,
|
||||
errorShape(ErrorCodes.INVALID_REQUEST, `missing scope: ${approved.missingScope}`),
|
||||
);
|
||||
return;
|
||||
}
|
||||
context.logGateway.info(
|
||||
`device pairing approved device=${approved.device.deviceId} role=${approved.device.role ?? "unknown"}`,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,132 @@
|
|||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, test } from "vitest";
|
||||
import { WebSocket } from "ws";
|
||||
import {
|
||||
loadOrCreateDeviceIdentity,
|
||||
publicKeyRawBase64UrlFromPem,
|
||||
type DeviceIdentity,
|
||||
} from "../infra/device-identity.js";
|
||||
import {
|
||||
approveDevicePairing,
|
||||
getPairedDevice,
|
||||
requestDevicePairing,
|
||||
rotateDeviceToken,
|
||||
} from "../infra/device-pairing.js";
|
||||
import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES } from "../utils/message-channel.js";
|
||||
import {
|
||||
connectOk,
|
||||
installGatewayTestHooks,
|
||||
rpcReq,
|
||||
startServerWithClient,
|
||||
trackConnectChallengeNonce,
|
||||
} from "./test-helpers.js";
|
||||
|
||||
installGatewayTestHooks({ scope: "suite" });
|
||||
|
||||
function resolveDeviceIdentityPath(name: string): string {
|
||||
const root = process.env.OPENCLAW_STATE_DIR ?? process.env.HOME ?? os.tmpdir();
|
||||
return path.join(root, "test-device-identities", `${name}.json`);
|
||||
}
|
||||
|
||||
function loadDeviceIdentity(name: string): {
|
||||
identityPath: string;
|
||||
identity: DeviceIdentity;
|
||||
publicKey: string;
|
||||
} {
|
||||
const identityPath = resolveDeviceIdentityPath(name);
|
||||
const identity = loadOrCreateDeviceIdentity(identityPath);
|
||||
return {
|
||||
identityPath,
|
||||
identity,
|
||||
publicKey: publicKeyRawBase64UrlFromPem(identity.publicKeyPem),
|
||||
};
|
||||
}
|
||||
|
||||
async function issuePairingScopedOperator(name: string): Promise<{
|
||||
identityPath: string;
|
||||
deviceId: string;
|
||||
token: string;
|
||||
}> {
|
||||
const loaded = loadDeviceIdentity(name);
|
||||
const request = await requestDevicePairing({
|
||||
deviceId: loaded.identity.deviceId,
|
||||
publicKey: loaded.publicKey,
|
||||
role: "operator",
|
||||
scopes: ["operator.admin"],
|
||||
clientId: GATEWAY_CLIENT_NAMES.TEST,
|
||||
clientMode: GATEWAY_CLIENT_MODES.TEST,
|
||||
});
|
||||
await approveDevicePairing(request.request.requestId);
|
||||
const rotated = await rotateDeviceToken({
|
||||
deviceId: loaded.identity.deviceId,
|
||||
role: "operator",
|
||||
scopes: ["operator.pairing"],
|
||||
});
|
||||
expect(rotated).not.toBeNull();
|
||||
const token = rotated?.token ?? "";
|
||||
return {
|
||||
identityPath: loaded.identityPath,
|
||||
deviceId: loaded.identity.deviceId,
|
||||
token: String(token),
|
||||
};
|
||||
}
|
||||
|
||||
async function openTrackedWs(port: number): Promise<WebSocket> {
|
||||
const ws = new WebSocket(`ws://127.0.0.1:${port}`);
|
||||
trackConnectChallengeNonce(ws);
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
const timer = setTimeout(() => reject(new Error("timeout waiting for ws open")), 5_000);
|
||||
ws.once("open", () => {
|
||||
clearTimeout(timer);
|
||||
resolve();
|
||||
});
|
||||
ws.once("error", (error) => {
|
||||
clearTimeout(timer);
|
||||
reject(error);
|
||||
});
|
||||
});
|
||||
return ws;
|
||||
}
|
||||
|
||||
describe("gateway device.pair.approve caller scope guard", () => {
|
||||
test("rejects approving device scopes above the caller session scopes", async () => {
|
||||
const started = await startServerWithClient("secret");
|
||||
const approver = await issuePairingScopedOperator("approve-attacker");
|
||||
const pending = loadDeviceIdentity("approve-target");
|
||||
|
||||
let pairingWs: WebSocket | undefined;
|
||||
try {
|
||||
const request = await requestDevicePairing({
|
||||
deviceId: pending.identity.deviceId,
|
||||
publicKey: pending.publicKey,
|
||||
role: "operator",
|
||||
scopes: ["operator.admin"],
|
||||
clientId: GATEWAY_CLIENT_NAMES.TEST,
|
||||
clientMode: GATEWAY_CLIENT_MODES.TEST,
|
||||
});
|
||||
|
||||
pairingWs = await openTrackedWs(started.port);
|
||||
await connectOk(pairingWs, {
|
||||
skipDefaultAuth: true,
|
||||
deviceToken: approver.token,
|
||||
deviceIdentityPath: approver.identityPath,
|
||||
scopes: ["operator.pairing"],
|
||||
});
|
||||
|
||||
const approve = await rpcReq(pairingWs, "device.pair.approve", {
|
||||
requestId: request.request.requestId,
|
||||
});
|
||||
expect(approve.ok).toBe(false);
|
||||
expect(approve.error?.message).toBe("missing scope: operator.admin");
|
||||
|
||||
const paired = await getPairedDevice(pending.identity.deviceId);
|
||||
expect(paired).toBeNull();
|
||||
} finally {
|
||||
pairingWs?.close();
|
||||
started.ws.close();
|
||||
await started.server.close();
|
||||
started.envSnapshot.restore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -745,7 +745,7 @@ export function attachGatewayWsMessageHandler(params: {
|
|||
const context = buildRequestContext();
|
||||
if (pairing.request.silent === true) {
|
||||
const approved = await approveDevicePairing(pairing.request.requestId);
|
||||
if (approved) {
|
||||
if (approved && approved.status === "approved") {
|
||||
logGateway.info(
|
||||
`device pairing auto-approved device=${approved.device.deviceId} role=${approved.device.role ?? "unknown"}`,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -78,6 +78,16 @@ type DevicePairingStateFile = {
|
|||
|
||||
const PENDING_TTL_MS = 5 * 60 * 1000;
|
||||
|
||||
export type ApproveDevicePairingResult =
|
||||
| { status: "approved"; requestId: string; device: PairedDevice }
|
||||
| { status: "forbidden"; missingScope: string }
|
||||
| null;
|
||||
|
||||
type ApprovedDevicePairingResult = Extract<
|
||||
NonNullable<ApproveDevicePairingResult>,
|
||||
{ status: "approved" }
|
||||
>;
|
||||
|
||||
const withLock = createAsyncLock();
|
||||
|
||||
async function loadState(baseDir?: string): Promise<DevicePairingStateFile> {
|
||||
|
|
@ -302,16 +312,60 @@ export async function requestDevicePairing(
|
|||
});
|
||||
}
|
||||
|
||||
function resolveMissingRequestedScope(params: {
|
||||
role: string;
|
||||
requestedScopes: readonly string[];
|
||||
callerScopes: readonly string[];
|
||||
}): string | null {
|
||||
for (const scope of params.requestedScopes) {
|
||||
if (
|
||||
!roleScopesAllow({
|
||||
role: params.role,
|
||||
requestedScopes: [scope],
|
||||
allowedScopes: params.callerScopes,
|
||||
})
|
||||
) {
|
||||
return scope;
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
export async function approveDevicePairing(
|
||||
requestId: string,
|
||||
baseDir?: string,
|
||||
): Promise<{ requestId: string; device: PairedDevice } | null> {
|
||||
): Promise<ApprovedDevicePairingResult | null>;
|
||||
export async function approveDevicePairing(
|
||||
requestId: string,
|
||||
options: { callerScopes?: readonly string[] },
|
||||
baseDir?: string,
|
||||
): Promise<ApproveDevicePairingResult>;
|
||||
export async function approveDevicePairing(
|
||||
requestId: string,
|
||||
optionsOrBaseDir?: { callerScopes?: readonly string[] } | string,
|
||||
maybeBaseDir?: string,
|
||||
): Promise<ApproveDevicePairingResult> {
|
||||
const options =
|
||||
typeof optionsOrBaseDir === "string" || optionsOrBaseDir === undefined
|
||||
? undefined
|
||||
: optionsOrBaseDir;
|
||||
const baseDir = typeof optionsOrBaseDir === "string" ? optionsOrBaseDir : maybeBaseDir;
|
||||
return await withLock(async () => {
|
||||
const state = await loadState(baseDir);
|
||||
const pending = state.pendingById[requestId];
|
||||
if (!pending) {
|
||||
return null;
|
||||
}
|
||||
if (pending.role && options?.callerScopes) {
|
||||
const missingScope = resolveMissingRequestedScope({
|
||||
role: pending.role,
|
||||
requestedScopes: normalizeDeviceAuthScopes(pending.scopes),
|
||||
callerScopes: options.callerScopes,
|
||||
});
|
||||
if (missingScope) {
|
||||
return { status: "forbidden", missingScope };
|
||||
}
|
||||
}
|
||||
const now = Date.now();
|
||||
const existing = state.pairedByDeviceId[pending.deviceId];
|
||||
const roles = mergeRoles(existing?.roles, existing?.role, pending.roles, pending.role);
|
||||
|
|
@ -364,7 +418,7 @@ export async function approveDevicePairing(
|
|||
delete state.pendingById[requestId];
|
||||
state.pairedByDeviceId[device.deviceId] = device;
|
||||
await persistState(state, baseDir);
|
||||
return { requestId, device };
|
||||
return { status: "approved" as const, requestId, device };
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue