fix(plugin): address scoped config review findings

This commit is contained in:
Alkim Ake Gozen 2026-06-04 11:53:05 +09:00
parent db0ef46900
commit 06a9428a36
6 changed files with 19844 additions and 93 deletions

File diff suppressed because it is too large Load diff

View file

@ -419,8 +419,10 @@ export function startWorkerRpcHost(options: WorkerRpcHostOptions): WorkerRpcHost
config: { config: {
async get(params) { async get(params) {
const companyId = if (params && "companyId" in params) {
params?.companyId ?? runtimeCompanyContext.getStore()?.companyId ?? null; return callHost("config.get", { companyId: params.companyId ?? null });
}
const companyId = runtimeCompanyContext.getStore()?.companyId ?? null;
return callHost("config.get", companyId ? { companyId } : {}); return callHost("config.get", companyId ? { companyId } : {});
}, },
}, },
@ -572,7 +574,9 @@ export function startWorkerRpcHost(options: WorkerRpcHostOptions): WorkerRpcHost
secrets: { secrets: {
async resolve(secretRef: string, companyId?: string | null): Promise<string> { async resolve(secretRef: string, companyId?: string | null): Promise<string> {
const scopedCompanyId = companyId ?? runtimeCompanyContext.getStore()?.companyId ?? null; const scopedCompanyId = arguments.length >= 2
? companyId ?? null
: runtimeCompanyContext.getStore()?.companyId ?? null;
return callHost("secrets.resolve", { secretRef, companyId: scopedCompanyId }); return callHost("secrets.resolve", { secretRef, companyId: scopedCompanyId });
}, },
}, },

View file

@ -355,6 +355,7 @@ describe("startWorkerRpcHost runtime company context", () => {
const host = startWorkerRpcHost({ plugin, stdin, stdout }); const host = startWorkerRpcHost({ plugin, stdin, stdout });
try {
writeMessage(stdin, { writeMessage(stdin, {
jsonrpc: "2.0", jsonrpc: "2.0",
id: 1, id: 1,
@ -413,7 +414,104 @@ describe("startWorkerRpcHost runtime company context", () => {
id: 2, id: 2,
result: { content: "company-config:company-secret" }, result: { content: "company-config:company-secret" },
}); });
} finally {
host.stop(); host.stop();
stdin.destroy();
stdout.destroy();
}
});
it("preserves explicit null company context in config and secret host calls", async () => {
const stdin = new PassThrough();
const stdout = new PassThrough();
const nextMessage = collectJsonLines(stdout);
const plugin = definePlugin({
async setup(ctx) {
ctx.tools.register(
"check-explicit-null",
{
displayName: "Check Explicit Null",
description: "Checks explicit null runtime context propagation",
parametersSchema: { type: "object", properties: {} },
},
async () => {
const config = await ctx.config.get({ companyId: null });
const token = await ctx.secrets.resolve(
"77777777-7777-4777-8777-777777777777",
null,
);
return { content: `${config.mode}:${token}` };
},
);
},
});
const host = startWorkerRpcHost({ plugin, stdin, stdout });
try {
writeMessage(stdin, {
jsonrpc: "2.0",
id: 1,
method: "initialize",
params: {
manifest: { id: "test-plugin", name: "test-plugin", version: "1.0.0" },
config: {},
instanceInfo: { instanceId: "inst-1", hostVersion: "0.0.0-test" },
apiVersion: 1,
},
});
await expect(nextMessage()).resolves.toMatchObject({ id: 1, result: { ok: true } });
writeMessage(stdin, {
jsonrpc: "2.0",
id: 2,
method: "executeTool",
params: {
toolName: "check-explicit-null",
parameters: {},
runContext: {
agentId: "agent-1",
runId: "run-1",
companyId: "company-1",
projectId: "project-1",
},
},
});
const configRequest = await nextMessage();
expect(configRequest).toMatchObject({
method: "config.get",
params: { companyId: null },
});
writeMessage(stdin, {
jsonrpc: "2.0",
id: configRequest.id,
result: { mode: "global-config" },
});
const secretRequest = await nextMessage();
expect(secretRequest).toMatchObject({
method: "secrets.resolve",
params: {
secretRef: "77777777-7777-4777-8777-777777777777",
companyId: null,
},
});
writeMessage(stdin, {
jsonrpc: "2.0",
id: secretRequest.id,
result: "global-secret",
});
await expect(nextMessage()).resolves.toMatchObject({
id: 2,
result: { content: "global-config:global-secret" },
});
} finally {
host.stop();
stdin.destroy();
stdout.destroy();
}
}); });
}); });

View file

@ -2169,23 +2169,24 @@ export function pluginRoutes(
res.status(400).json({ error: '"configJson" is required and must be an object' }); res.status(400).json({ error: '"configJson" is required and must be an object' });
return; return;
} }
const configJson = body.configJson;
const companyId = resolvePluginConfigCompanyId(req); const companyId = resolvePluginConfigCompanyId(req);
// Strip devUiUrl unless the caller is an instance admin. devUiUrl activates // Strip devUiUrl unless the caller is an instance admin. devUiUrl activates
// a dev-proxy in the static file route that could be abused for SSRF if any // a dev-proxy in the static file route that could be abused for SSRF if any
// board-level user were allowed to set it. // board-level user were allowed to set it.
if ( if (
"devUiUrl" in body.configJson && "devUiUrl" in configJson &&
!(req.actor.type === "board" && req.actor.isInstanceAdmin) !(req.actor.type === "board" && req.actor.isInstanceAdmin)
) { ) {
delete body.configJson.devUiUrl; delete configJson.devUiUrl;
} }
// Validate configJson against the plugin's instanceConfigSchema (if declared). // Validate configJson against the plugin's instanceConfigSchema (if declared).
// This ensures CLI/API callers get the same validation the UI performs client-side. // This ensures CLI/API callers get the same validation the UI performs client-side.
const schema = plugin.manifestJson?.instanceConfigSchema; const schema = plugin.manifestJson?.instanceConfigSchema;
if (schema && Object.keys(schema).length > 0) { if (schema && Object.keys(schema).length > 0) {
const validation = validateInstanceConfig(body.configJson, schema); const validation = validateInstanceConfig(configJson, schema);
if (!validation.valid) { if (!validation.valid) {
res.status(400).json({ res.status(400).json({
error: "Configuration does not match the plugin's instanceConfigSchema", error: "Configuration does not match the plugin's instanceConfigSchema",
@ -2196,30 +2197,45 @@ export function pluginRoutes(
} }
try { try {
const secretRefsByPath = extractSecretRefPathsFromConfig(body.configJson, schema); const secretRefsByPath = extractSecretRefPathsFromConfig(configJson, schema);
if (secretRefsByPath.size > 0 && !companyId) { if (secretRefsByPath.size > 0 && !companyId) {
res.status(422).json({ error: "Plugin secret references require companyId" }); res.status(422).json({ error: "Plugin secret references require companyId" });
return; return;
} }
if (companyId) {
const refs = [...secretRefsByPath.entries()].flatMap(([secretId, paths]) => const refs = [...secretRefsByPath.entries()].flatMap(([secretId, paths]) =>
[...paths].map((configPath) => ({ secretId, configPath })), [...paths].map((configPath) => ({ secretId, configPath })),
); );
await secrets.syncSecretRefsForTarget( const persistConfig = async (
companyId, scopedSecrets: typeof secrets,
{ targetType: "plugin", targetId: plugin.id }, scopedRegistry: typeof registry,
refs, secretDb?: Db,
); ) => {
if (companyId) {
const target = { targetType: "plugin" as const, targetId: plugin.id };
if (secretDb) {
await scopedSecrets.syncSecretRefsForTarget(companyId, target, refs, { db: secretDb });
} else {
await scopedSecrets.syncSecretRefsForTarget(companyId, target, refs);
} }
}
const result = await registry.upsertConfig(plugin.id, { return scopedRegistry.upsertConfig(plugin.id, {
configJson: body.configJson, configJson,
}, companyId); }, companyId);
};
const result = typeof db.transaction === "function"
? await db.transaction((tx) =>
persistConfig(
secretService(tx as unknown as Db),
pluginRegistryService(tx as unknown as Db),
tx as unknown as Db,
)
)
: await persistConfig(secrets, registry);
await logPluginMutationActivity(req, "plugin.config.updated", plugin.id, { await logPluginMutationActivity(req, "plugin.config.updated", plugin.id, {
pluginId: plugin.id, pluginId: plugin.id,
pluginKey: plugin.pluginKey, pluginKey: plugin.pluginKey,
companyId, companyId,
configKeyCount: Object.keys(body.configJson).length, configKeyCount: Object.keys(configJson).length,
}); });
// Only legacy/global config is still pushed into the process-global worker state. // Only legacy/global config is still pushed into the process-global worker state.
@ -2229,7 +2245,7 @@ export function pluginRoutes(
await bridgeDeps.workerManager.call( await bridgeDeps.workerManager.call(
plugin.id, plugin.id,
"configChanged", "configChanged",
{ config: body.configJson }, { config: configJson },
); );
} catch (rpcErr) { } catch (rpcErr) {
if ( if (

View file

@ -44,7 +44,7 @@ function isPluginKeyConflict(error: unknown): boolean {
return err.code === "23505" && constraint === "plugins_plugin_key_idx"; return err.code === "23505" && constraint === "plugins_plugin_key_idx";
} }
function pluginConfigScopeCondition(pluginId: string, companyId?: string | null) { function pluginConfigExactScopeCondition(pluginId: string, companyId?: string | null) {
return and( return and(
eq(pluginConfig.pluginId, pluginId), eq(pluginConfig.pluginId, pluginId),
companyId ? eq(pluginConfig.companyId, companyId) : isNull(pluginConfig.companyId), companyId ? eq(pluginConfig.companyId, companyId) : isNull(pluginConfig.companyId),
@ -288,12 +288,22 @@ export function pluginRegistryService(db: Db) {
// ----- Config --------------------------------------------------------- // ----- Config ---------------------------------------------------------
/** Retrieve a plugin's company-scoped config, or the legacy global fallback. */ /** Retrieve a plugin's company-scoped config, or the legacy global fallback. */
getConfig: (pluginId: string, companyId?: string | null) => getConfig: async (pluginId: string, companyId?: string | null) => {
db if (companyId) {
const scoped = await db
.select() .select()
.from(pluginConfig) .from(pluginConfig)
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.then((rows) => rows[0] ?? null), .then((rows) => rows[0] ?? null);
if (scoped) return scoped;
}
return db
.select()
.from(pluginConfig)
.where(pluginConfigExactScopeCondition(pluginId, null))
.then((rows) => rows[0] ?? null);
},
/** /**
* Create or fully replace a plugin's instance configuration. * Create or fully replace a plugin's instance configuration.
@ -307,7 +317,7 @@ export function pluginRegistryService(db: Db) {
const existing = await db const existing = await db
.select() .select()
.from(pluginConfig) .from(pluginConfig)
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.then((rows) => rows[0] ?? null); .then((rows) => rows[0] ?? null);
if (existing) { if (existing) {
@ -318,7 +328,7 @@ export function pluginRegistryService(db: Db) {
lastError: null, lastError: null,
updatedAt: new Date(), updatedAt: new Date(),
}) })
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.returning() .returning()
.then((rows) => rows[0]); .then((rows) => rows[0]);
} }
@ -345,7 +355,7 @@ export function pluginRegistryService(db: Db) {
const existing = await db const existing = await db
.select() .select()
.from(pluginConfig) .from(pluginConfig)
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.then((rows) => rows[0] ?? null); .then((rows) => rows[0] ?? null);
if (existing) { if (existing) {
@ -357,7 +367,7 @@ export function pluginRegistryService(db: Db) {
lastError: null, lastError: null,
updatedAt: new Date(), updatedAt: new Date(),
}) })
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.returning() .returning()
.then((rows) => rows[0]); .then((rows) => rows[0]);
} }
@ -381,7 +391,7 @@ export function pluginRegistryService(db: Db) {
const rows = await db const rows = await db
.update(pluginConfig) .update(pluginConfig)
.set({ lastError, updatedAt: new Date() }) .set({ lastError, updatedAt: new Date() })
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.returning(); .returning();
if (rows.length === 0) throw notFound("Plugin config not found"); if (rows.length === 0) throw notFound("Plugin config not found");
@ -392,7 +402,7 @@ export function pluginRegistryService(db: Db) {
deleteConfig: async (pluginId: string, companyId?: string | null) => { deleteConfig: async (pluginId: string, companyId?: string | null) => {
const rows = await db const rows = await db
.delete(pluginConfig) .delete(pluginConfig)
.where(pluginConfigScopeCondition(pluginId, companyId)) .where(pluginConfigExactScopeCondition(pluginId, companyId))
.returning(); .returning();
return rows[0] ?? null; return rows[0] ?? null;

View file

@ -2034,6 +2034,7 @@ export function secretService(db: Db) {
required?: boolean; required?: boolean;
label?: string | null; label?: string | null;
}>, }>,
options?: { db?: SecretBindingDb },
) => { ) => {
const normalizedRefs: Array<{ const normalizedRefs: Array<{
secretId: string; secretId: string;
@ -2042,8 +2043,9 @@ export function secretService(db: Db) {
required: boolean; required: boolean;
label: string | null; label: string | null;
}> = []; }> = [];
const bindingDb = options?.db ?? db;
for (const ref of refs) { for (const ref of refs) {
await assertSecretInCompany(companyId, ref.secretId); await assertSecretInCompany(companyId, ref.secretId, bindingDb);
normalizedRefs.push({ normalizedRefs.push({
secretId: ref.secretId, secretId: ref.secretId,
configPath: ref.configPath, configPath: ref.configPath,
@ -2055,10 +2057,10 @@ export function secretService(db: Db) {
const pathPrefixes = [...new Set(normalizedRefs.map((ref) => ref.configPath.split(".")[0]))]; const pathPrefixes = [...new Set(normalizedRefs.map((ref) => ref.configPath.split(".")[0]))];
await db.transaction(async (tx) => { const writeBindings = async (targetDb: SecretBindingDb) => {
if (pathPrefixes.length > 0) { if (pathPrefixes.length > 0) {
for (const pathPrefix of pathPrefixes) { for (const pathPrefix of pathPrefixes) {
await tx await targetDb
.delete(companySecretBindings) .delete(companySecretBindings)
.where( .where(
and( and(
@ -2070,7 +2072,7 @@ export function secretService(db: Db) {
); );
} }
} else { } else {
await tx await targetDb
.delete(companySecretBindings) .delete(companySecretBindings)
.where( .where(
and( and(
@ -2081,7 +2083,7 @@ export function secretService(db: Db) {
); );
} }
if (normalizedRefs.length === 0) return; if (normalizedRefs.length === 0) return;
await tx.insert(companySecretBindings).values( await targetDb.insert(companySecretBindings).values(
normalizedRefs.map((ref) => ({ normalizedRefs.map((ref) => ({
companyId, companyId,
secretId: ref.secretId, secretId: ref.secretId,
@ -2093,7 +2095,13 @@ export function secretService(db: Db) {
label: ref.label, label: ref.label,
})), })),
); );
}); };
if (options?.db) {
await writeBindings(options.db);
} else {
await db.transaction(async (tx) => writeBindings(tx));
}
return normalizedRefs; return normalizedRefs;
}, },