From 49588c2f1faea89f59c2667013338697b6a90645 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:49:38 +0900 Subject: [PATCH] fix(engine): deep-copy and freeze the default command-authorization policy normalizeCommandAuthorizationPolicy's two exit paths disagreed about ownership of the returned object. The non-record path deep-copies via clonePolicy; the record path seeded commands with a SHALLOW spread of DEFAULT_COMMAND_AUTHORIZATION_POLICY, so every un-overridden command's role array was the SAME instance the module-level default holds. Only explicitly-overridden commands got a fresh array. A caller that pushed a role through a returned policy would widen that command for every repo in the same isolate, permanently, with no config change and no audit trail -- the default is the security vocabulary for the whole command surface. Seed the record path from clonePolicy too, so every returned role array is freshly allocated on every input, and freeze DEFAULT_COMMAND_AUTHORIZATION_POLICY (object, default array, commands record, and every role array) so a future aliasing regression fails loudly instead of silently corrupting the vocabulary. Values, warnings, key validation, the maintainer-only clamp, and every evaluateCommandAuthorization decision are unchanged. Closes #9998 --- .../src/settings/command-authorization.ts | 16 ++++++- .../test/command-authorization.test.ts | 47 +++++++++++++++++++ .../unit/command-authorization-engine.test.ts | 18 +++++++ 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 packages/loopover-engine/test/command-authorization.test.ts diff --git a/packages/loopover-engine/src/settings/command-authorization.ts b/packages/loopover-engine/src/settings/command-authorization.ts index af9cacd334..9b0554f8dd 100644 --- a/packages/loopover-engine/src/settings/command-authorization.ts +++ b/packages/loopover-engine/src/settings/command-authorization.ts @@ -43,6 +43,15 @@ export const DEFAULT_COMMAND_AUTHORIZATION_POLICY: RepositoryCommandAuthorizatio }, }; +// #9998: freeze the security vocabulary at runtime -- the object, its `default` array, its `commands` record, +// and every role array inside it -- so a future aliasing regression fails loudly (a strict-mode TypeError on +// the offending `push`) instead of silently widening a command for every repo in the isolate. Values are +// unchanged; normalizeCommandAuthorizationPolicy always hands callers a fresh deep copy to mutate. +for (const roles of Object.values(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands)) Object.freeze(roles); +Object.freeze(DEFAULT_COMMAND_AUTHORIZATION_POLICY.default); +Object.freeze(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands); +Object.freeze(DEFAULT_COMMAND_AUTHORIZATION_POLICY); + const COMMAND_AUTHORIZATION_ROLES = new Set(["maintainer", "collaborator", "pr_author", "confirmed_miner"]); // Roles that may remain configured on a maintainer-only command. The clamp drops only the spoofable // plain `pr_author` role; `confirmed_miner` survives so a detected miner can self-trigger reruns (#824). @@ -70,7 +79,12 @@ export function normalizeCommandAuthorizationPolicy(input: unknown): { policy: R } const defaultRoles = normalizeRoleList(input.default, DEFAULT_COMMAND_AUTHORIZATION_POLICY.default, "default", warnings); - const commands: Record = { ...DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands }; + // #9998: DEEP-copy the default command arrays, not a shallow spread. A shallow `{ ...DEFAULT...commands }` + // shares every un-overridden command's role array with the module-level (now frozen) default, so a caller + // that mutated a returned array would corrupt the security vocabulary for every repo in the isolate. This + // reuses `clonePolicy` -- the same deep copy the non-record exit already returns -- so both paths hand back + // arrays the caller solely owns. + const commands: Record = clonePolicy(DEFAULT_COMMAND_AUTHORIZATION_POLICY).commands; if (input.commands !== undefined) { if (isRecord(input.commands)) { for (const [command, roles] of Object.entries(input.commands)) { diff --git a/packages/loopover-engine/test/command-authorization.test.ts b/packages/loopover-engine/test/command-authorization.test.ts new file mode 100644 index 0000000000..5693f91eb1 --- /dev/null +++ b/packages/loopover-engine/test/command-authorization.test.ts @@ -0,0 +1,47 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + commandAuthorizationAllowedRoles, + DEFAULT_COMMAND_AUTHORIZATION_POLICY, + evaluateCommandAuthorization, + normalizeCommandAuthorizationPolicy, +} from "../dist/settings/command-authorization.js"; + +// #9998: the record path seeded `commands` with a SHALLOW spread of DEFAULT_COMMAND_AUTHORIZATION_POLICY, +// so every un-overridden command's role array was the same instance the module-level default holds. A caller +// that mutated a returned array would corrupt the security vocabulary for every repo in the isolate. The +// record path now deep-copies (via clonePolicy) and the default is frozen, matching the non-record exit. +test("#9998: normalizeCommandAuthorizationPolicy({}) returns fresh role arrays, deep-equal but not aliased", () => { + const policy = normalizeCommandAuthorizationPolicy({}).policy; + assert.notStrictEqual(policy.commands["review"], DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"]); + assert.deepEqual(policy.commands["review"], DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"]); + assert.deepEqual(policy.commands, DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands); + assert.deepEqual(policy.default, DEFAULT_COMMAND_AUTHORIZATION_POLICY.default); +}); + +test("#9998: the two exit paths agree — null and an override both return non-aliased arrays for un-overridden commands", () => { + const fromNull = normalizeCommandAuthorizationPolicy(null).policy; + assert.notStrictEqual(fromNull.commands["review"], DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"]); + + // Overriding `plan` must not leave the un-overridden `pause` aliased to the default. + const fromOverride = normalizeCommandAuthorizationPolicy({ commands: { plan: ["maintainer"] } }).policy; + assert.notStrictEqual(fromOverride.commands["pause"], DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["pause"]); + assert.deepEqual(fromOverride.commands["pause"], DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["pause"]); +}); + +test("#9998: mutating a returned role array does not change the default, which is frozen", () => { + const review = normalizeCommandAuthorizationPolicy({}).policy.commands["review"]; + assert.ok(review !== undefined); + review.push("pr_author"); + assert.deepEqual(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"], ["maintainer", "collaborator", "confirmed_miner"]); + assert.equal(Object.isFrozen(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["generate-tests"]), true); +}); + +test("#9998: preserved behaviour — generate-tests stays maintainer-only and denies a COLLABORATOR", () => { + assert.deepEqual(commandAuthorizationAllowedRoles(null, "generate-tests"), ["maintainer"]); + assert.equal( + evaluateCommandAuthorization({ commandName: "generate-tests", commenterAssociation: "COLLABORATOR" }).authorized, + false, + ); +}); diff --git a/test/unit/command-authorization-engine.test.ts b/test/unit/command-authorization-engine.test.ts index 0d6c7d1bd5..5ef7a521e5 100644 --- a/test/unit/command-authorization-engine.test.ts +++ b/test/unit/command-authorization-engine.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { commandAuthorizationAllowedRoles, commandAuthorizationNeedsMinerDetection, + DEFAULT_COMMAND_AUTHORIZATION_POLICY, evaluateCommandAuthorization, normalizeCommandAuthorizationPolicy, summarizeCommandAuthorizationPolicy, @@ -292,4 +293,21 @@ describe("repo command authorization policy", () => { expect(malformedCommands.warnings).toContain("commandAuthorization.commands must be an object; using command defaults."); expect(malformedCommands.policy.commands["queue-summary"]).toEqual(["maintainer", "collaborator"]); }); + + it("#9998: returns fresh, non-aliased role arrays on every input and freezes the default", () => { + // The record path used a shallow spread, sharing every un-overridden command's array with the module-level + // default; a caller mutating a returned array would corrupt the vocabulary for every repo in the isolate. + for (const input of [{}, null, { commands: { plan: ["maintainer"] } }] as const) { + const policy = normalizeCommandAuthorizationPolicy(input).policy; + // `pause` is never overridden by any of these inputs, so it exercises the deep-copied default path. + expect(policy.commands["pause"]).not.toBe(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["pause"]); + expect(policy.commands["pause"]).toEqual(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["pause"]); + } + const review = normalizeCommandAuthorizationPolicy({}).policy.commands["review"]; + expect(review).toBeDefined(); + review?.push("pr_author"); + expect(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["review"]).toEqual(["maintainer", "collaborator", "confirmed_miner"]); + expect(Object.isFrozen(DEFAULT_COMMAND_AUTHORIZATION_POLICY.commands["generate-tests"])).toBe(true); + expect(Object.isFrozen(DEFAULT_COMMAND_AUTHORIZATION_POLICY)).toBe(true); + }); });