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); + }); });