This commit is contained in:
Dennis Rende 2026-09-13 17:20:41 +08:00 committed by GitHub
commit b8bcc9a9a9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 303 additions and 27 deletions

View File

@ -4368,6 +4368,73 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
mockAdapterExecute.mockClear();
});
it("keeps a disallowed agent-owned low-trust secret as a terminal pre-dispatch setup failure", async () => {
const { companyId, agentId, runId, issueId } =
await seedQueuedIssueRunFixture();
const secrets = secretService(db);
const secret = await secrets.create(companyId, {
name: `agent-owned-negative-probe-${randomUUID()}`,
provider: "local_encrypted",
value: "must-never-reach-the-adapter",
});
const adapterConfig = {
env: {
REVIP5787_NEGATIVE_SETUP_PROBE: {
type: "secret_ref" as const,
secretId: secret.id,
version: "latest" as const,
},
},
};
await secrets.syncEnvBindingsForTarget(
companyId,
{ targetType: "agent", targetId: agentId },
adapterConfig.env,
);
await db
.update(agents)
.set({
adapterConfig,
permissions: {
trustPreset: "low_trust_review",
authorizationPolicy: {
trustPreset: "low_trust_review",
reviewPreset: {
id: "low_trust_review",
version: 1,
rawOutputDisposition: "quarantine",
},
trustBoundary: {
mode: "low_trust_review",
companyId,
issueIds: [issueId],
allowedToolClasses: ["git.read", "tests.local"],
allowedSecretBindingIds: [],
outputPromotionTarget: { type: "issue", issueId },
},
},
},
})
.where(eq(agents.id, agentId));
const heartbeat = heartbeatService(db);
await heartbeat.resumeQueuedRuns();
const failedRun = await waitForRunToSettle(heartbeat, runId);
expect(failedRun).toMatchObject({
status: "failed",
errorCode: "setup_failed",
resultJson: {
executionRecovery: { kind: "bootstrap", providerWorkStarted: false },
},
});
expect(failedRun?.error).toContain(
"Secret binding is outside the active low-trust boundary",
);
expect(mockAdapterExecute).not.toHaveBeenCalled();
});
it("classifies only the installed-but-not-ready sandbox provider plugin message as a configuration gap", () => {
expect(
parseSandboxProviderPluginNotReadyFailureMessage(

View File

@ -330,6 +330,13 @@ describe("resolveExecutionRunAdapterConfig", () => {
expect(resolveEnvBindings.mock.calls[2]?.[2]).toMatchObject({
allowedBindingIds: ["binding-1"],
});
// Inherited scopes (environment/project/routine) ask resolveEnvBindings to
// omit a disallowed binding rather than throw. The agent's own
// adapterConfig.env goes through resolveAdapterConfigForRuntime instead,
// which has no such option and stays hard-fail.
expect(resolveEnvBindings.mock.calls[0]?.[3]).toEqual({ omitDisallowedBindings: true });
expect(resolveEnvBindings.mock.calls[1]?.[3]).toEqual({ omitDisallowedBindings: true });
expect(resolveEnvBindings.mock.calls[2]?.[3]).toEqual({ omitDisallowedBindings: true });
});
it("does not project brokered GitHub credentials across a low-trust boundary", async () => {

View File

@ -1517,6 +1517,70 @@ describeEmbeddedPostgres("secretService", () => {
expect(resolved.manifest[0]?.bindingId).toBe(binding!.id);
});
// REVIP-5787: a low-trust reviewer with allowedSecretBindingIds: [] used to
// hard-fail setup for every project carrying a project-wide env secret, even
// though the reviewer never declared that secret itself. Inherited bindings
// (environment/project/routine) must be omitted, not fatal; only the agent's
// own adapterConfig.env binding keeps the hard failure from the test above.
it("omits an inherited project secret binding outside the low-trust boundary instead of throwing, when instructed", async () => {
const companyId = await seedCompany();
const svc = secretService(db);
const secret = await svc.create(companyId, {
name: `project-inherited-${randomUUID()}`,
provider: "local_encrypted",
value: "aral-bp-invoices",
});
const env = {
ARAL_BP_RECHNUNGEN: { type: "secret_ref" as const, secretId: secret.id, version: "latest" as const },
PROJECT_PLAIN: "still-here",
};
await svc.syncEnvBindingsForTarget(companyId, { targetType: "project", targetId: "project-1" }, env);
const [binding] = await svc.listBindings(companyId, secret.id);
expect(binding?.id).toBeTruthy();
const resolved = await svc.resolveEnvBindings(
companyId,
env,
{
consumerType: "project",
consumerId: "project-1",
actorType: "agent",
actorId: "low-trust-reviewer",
// Empty allowlist, exactly like a reviewOnly class-1 agent with no
// secret bindings of its own.
allowedBindingIds: [],
},
{ omitDisallowedBindings: true },
);
expect(resolved.env).toEqual({ PROJECT_PLAIN: "still-here" });
expect(resolved.secretKeys.has("ARAL_BP_RECHNUNGEN")).toBe(false);
expect(resolved.manifest).toEqual([]);
expect(JSON.stringify(resolved)).not.toContain("aral-bp-invoices");
const [denialEvent] = await svc.listAccessEvents(companyId, secret.id);
expect(denialEvent).toMatchObject({
consumerType: "project",
consumerId: "project-1",
configPath: "env.ARAL_BP_RECHNUNGEN",
outcome: "failure",
errorCode: "binding_not_allowed",
});
expect(JSON.stringify(denialEvent)).not.toContain("aral-bp-invoices");
// Without the opt-in the same call still hard-fails — omission is
// per-call, not a change to the default enforcement.
await expect(
svc.resolveEnvBindings(companyId, env, {
consumerType: "project",
consumerId: "project-1",
actorType: "agent",
actorId: "low-trust-reviewer",
allowedBindingIds: [],
}),
).rejects.toMatchObject({ status: 422, details: { code: "binding_not_allowed" } });
});
it("fails closed at runtime for class-3 env lease rows outside the allowlist", async () => {
const companyId = await seedCompany();
const svc = secretService(db);
@ -1608,6 +1672,61 @@ describeEmbeddedPostgres("secretService", () => {
expect(resolved.manifest[0]?.bindingId).toBe(declaration!.id);
});
it("omits an inherited user-secret declaration outside the low-trust boundary instead of throwing, when instructed", async () => {
const companyId = await seedCompany();
await seedCompanyMember(companyId, "user-1", "owner");
const svc = secretService(db);
const definition = await svc.createUserSecretDefinition(companyId, {
key: "github_token",
name: "GitHub token",
provider: "local_encrypted",
});
const env = {
GITHUB_TOKEN: { type: "user_secret_ref" as const, key: "github_token", version: "latest" as const },
ROUTINE_PLAIN: "still-here",
};
await svc.syncEnvBindingsForTarget(companyId, { targetType: "routine", targetId: "routine-1" }, env);
const userSecret = await svc.createCurrentUserSecretValue(companyId, "user-1", {
definitionKey: "github_token",
value: "user-one-secret",
});
const resolved = await svc.resolveEnvBindings(
companyId,
env,
{
consumerType: "routine",
consumerId: "routine-1",
actorType: "agent",
actorId: "low-trust-reviewer",
responsibleUserId: "user-1",
allowedBindingIds: [],
},
{ omitDisallowedBindings: true },
);
expect(resolved.env).toEqual({ ROUTINE_PLAIN: "still-here" });
expect(resolved.secretKeys.has("GITHUB_TOKEN")).toBe(false);
expect(JSON.stringify(resolved)).not.toContain("user-one-secret");
const [denialEvent] = await svc.listAccessEvents(companyId, userSecret.id);
expect(denialEvent).toMatchObject({
secretId: userSecret.id,
userSecretDefinitionId: definition.id,
secretScope: "user",
responsibleUserId: "user-1",
credentialOwnerUserId: "user-1",
credentialSubjectType: "user",
credentialSubjectId: "user-1",
consumerType: "routine",
consumerId: "routine-1",
configPath: "env.GITHUB_TOKEN",
outcome: "failure",
errorCode: "binding_not_allowed",
});
expect(JSON.stringify(denialEvent)).not.toContain("user-one-secret");
});
it("resolves routine env secret refs through routine bindings and records value-free access metadata", async () => {
const companyId = await seedCompany();
const svc = secretService(db);

View File

@ -1718,6 +1718,12 @@ export async function resolveExecutionRunAdapterConfig(input: {
: {}),
}
: undefined,
// Inherited bindings (environment/project/routine): low-trust containment
// means omitting a disallowed value, not aborting the run. Only the
// agent's own adapterConfig.env (below) stays hard-fail — a misconfigured
// agent-owned binding is the operator's mistake to fix, not something to
// silently drop.
lowTrustAllowedBindingIds !== undefined ? { omitDisallowedBindings: true } : undefined,
)
: { env: {}, secretKeys: new Set<string>(), manifest: [] };
const {
@ -1770,6 +1776,7 @@ export async function resolveExecutionRunAdapterConfig(input: {
: {}),
}
: undefined,
lowTrustAllowedBindingIds !== undefined ? { omitDisallowedBindings: true } : undefined,
)
: { env: {}, secretKeys: new Set<string>(), manifest: [] };
if (Object.keys(projectEnvResolution.env).length > 0) {
@ -1799,6 +1806,7 @@ export async function resolveExecutionRunAdapterConfig(input: {
: {}),
}
: undefined,
lowTrustAllowedBindingIds !== undefined ? { omitDisallowedBindings: true } : undefined,
)
: { env: {}, secretKeys: new Set<string>(), manifest: [] };
if (Object.keys(routineEnvResolution.env).length > 0) {

View File

@ -712,6 +712,7 @@ type RuntimeSecretResolution = {
};
type SecretResolutionErrorCode =
| "binding_not_allowed"
| "binding_missing"
| "secret_deleted"
| "secret_inactive"
@ -808,11 +809,21 @@ function defaultProviderConfigStatus(provider: SecretProvider): SecretProviderCo
return COMING_SOON_SECRET_PROVIDERS.has(provider) ? "coming_soon" : "ready";
}
// True only for the low-trust-boundary rejection raised by assertBindingContext /
// resolveUserSecretValue (context.allowedBindingIds set and the binding isn't in
// it). Distinguishing this from other resolution failures (missing binding,
// inactive secret, ...) lets a caller choose to omit just this class of binding
// instead of aborting the whole resolution.
function isBindingNotAllowedError(error: unknown): boolean {
return error instanceof HttpError && asRecord(error.details)?.code === "binding_not_allowed";
}
function secretResolutionErrorCode(error: unknown): SecretResolutionErrorCode {
if (isSecretProviderClientError(error)) return "provider_error";
if (error instanceof HttpError) {
const details = asRecord(error.details);
switch (details?.code) {
case "binding_not_allowed":
case "binding_missing":
case "secret_deleted":
case "secret_inactive":
@ -1058,6 +1069,24 @@ export function secretService(db: Db | DbTransaction) {
.then((rows) => rows[0] ?? null);
}
async function getUserSecretValueId(input: {
companyId: string;
ownerUserId: string;
definitionId: string;
}) {
return db
.select({ id: companySecrets.id })
.from(companySecrets)
.where(and(
eq(companySecrets.companyId, input.companyId),
eq(companySecrets.scope, "user"),
eq(companySecrets.ownerUserId, input.ownerUserId),
eq(companySecrets.userSecretDefinitionId, input.definitionId),
ne(companySecrets.status, "deleted"),
))
.then((rows) => rows[0] ?? null);
}
async function getUserSecretValueById(companyId: string, ownerUserId: string, secretId: string) {
const secret = await getById(secretId);
if (!secret || secret.status === "deleted" || secret.scope !== "user") {
@ -4157,6 +4186,27 @@ export function secretService(db: Db | DbTransaction) {
Array.isArray(context?.allowedBindingIds) &&
(!declaration || !context.allowedBindingIds.includes(declaration.id))
) {
const deniedSecret = await getUserSecretValueId({
companyId,
ownerUserId: responsibleUserId,
definitionId: definition.id,
});
if (deniedSecret) {
await recordAccessEvent({
companyId,
secretId: deniedSecret.id,
userSecretDefinitionId: definition.id,
secretScope: "user",
version: null,
provider: definition.provider as SecretProvider,
context: context ? { ...context, responsibleUserId } : undefined,
credentialOwnerUserId: responsibleUserId,
credentialSubjectType: "user",
credentialSubjectId: responsibleUserId,
outcome: "failure",
errorCode: "binding_not_allowed",
}).catch(() => undefined);
}
throw unprocessable(
"User secret declaration is outside the active low-trust boundary",
{ code: "binding_not_allowed" },
@ -5101,6 +5151,7 @@ export function secretService(db: Db | DbTransaction) {
companyId: string,
envValue: unknown,
context?: Omit<SecretBindingContext, "configPath">,
opts?: { omitDisallowedBindings?: boolean },
): Promise<{ env: Record<string, string>; secretKeys: Set<string>; manifest: RuntimeSecretManifestEntry[] }> => {
const record = asRecord(envValue);
if (!record) return { env: {} as Record<string, string>, secretKeys: new Set<string>(), manifest: [] };
@ -5120,37 +5171,61 @@ export function secretService(db: Db | DbTransaction) {
if (binding.type === "plain") {
resolved[key] = binding.value;
} else if (binding.type === "secret_ref") {
const secretResolution = await resolveSecretValueInternal(
companyId,
binding.secretId,
binding.version,
context
? {
bindingContext: { ...context, configPath: `env.${key}` },
accessContext: { ...context, configPath: `env.${key}` },
}
: undefined,
);
let secretResolution: RuntimeSecretResolution;
try {
secretResolution = await resolveSecretValueInternal(
companyId,
binding.secretId,
binding.version,
context
? {
bindingContext: { ...context, configPath: `env.${key}` },
accessContext: { ...context, configPath: `env.${key}` },
}
: undefined,
);
} catch (err) {
if (opts?.omitDisallowedBindings && isBindingNotAllowedError(err)) {
logger.warn(
{ envKey: key, consumerType: context?.consumerType, consumerId: context?.consumerId },
"omitting inherited secret binding outside the active low-trust boundary",
);
continue;
}
throw err;
}
resolved[key] = secretResolution.value;
manifest.push(secretResolution.manifestEntry);
secretKeys.add(key);
} else {
const secretResolution = await secretService(db).resolveUserSecretValue(
companyId,
{
definitionKey: binding.key,
version: binding.version,
required: binding.required,
allowMissingOverride: binding.allowMissingOverride,
},
context
? {
...context,
configPath: `env.${key}`,
responsibleUserId: context.responsibleUserId ?? null,
}
: undefined,
);
let secretResolution: RuntimeSecretResolution | null;
try {
secretResolution = await secretService(db).resolveUserSecretValue(
companyId,
{
definitionKey: binding.key,
version: binding.version,
required: binding.required,
allowMissingOverride: binding.allowMissingOverride,
},
context
? {
...context,
configPath: `env.${key}`,
responsibleUserId: context.responsibleUserId ?? null,
}
: undefined,
);
} catch (err) {
if (opts?.omitDisallowedBindings && isBindingNotAllowedError(err)) {
logger.warn(
{ envKey: key, consumerType: context?.consumerType, consumerId: context?.consumerId },
"omitting inherited user-secret binding outside the active low-trust boundary",
);
continue;
}
throw err;
}
if (secretResolution) {
resolved[key] = secretResolution.value;
manifest.push(secretResolution.manifestEntry);