Skip to content

fix(commands): require an open, non-draft PR for chat's pr_author grant #5094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 11, 2026
Merged

fix(commands): require an open, non-draft PR for chat's pr_author grant #5094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions packages/gittensory-engine/src/settings/command-authorization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: {
* `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting
* never grants contributor chat access no matter what `chat`'s configured roles say. */
commandRateLimitPolicy?: "off" | "hold" | undefined;
/** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in
* {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`)
* never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by
* reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" &&
* !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false`
* denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless
* (this bounds the less-trusted pr_author tier, not already-trusted roles). */
pullRequestOpenAndNotDraft?: boolean | undefined;
}): CommandAuthorizationDecision {
const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName);
const roles = actorRoles(args);
const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null;
const prAuthorRateLimitGated =
matchedRole === "pr_author" &&
PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) &&
args.commandRateLimitPolicy !== "hold";
if (matchedRole && !prAuthorRateLimitGated) {
const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName));
if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") {
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
}
if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) {
return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles };
}
if (matchedRole) {
return {
authorized: true,
reason: authorizationReason(matchedRole),
Expand All @@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: {
allowedRoles,
};
}
if (prAuthorRateLimitGated) {
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
}
const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin);
if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) {
return {
Expand Down
4 changes: 4 additions & 0 deletions src/github/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,9 @@ export function isAuthorizedCommandActor(args: {
/** #5084: required (must be `"hold"`) for a PR author to be authorized for `chat` -- see
* PR_AUTHOR_RATE_LIMITED_COMMANDS in settings/command-authorization.ts. */
commandRateLimitPolicy?: "off" | "hold" | undefined;
/** #5092: ALSO required (must be `true`) for a PR author to be authorized for `chat` -- the caller-computed
* `pr.state === "open" && !pr.isDraft`, since the per-PR rate-limit counter never checks PR state on its own. */
pullRequestOpenAndNotDraft?: boolean | undefined;
}): { authorized: boolean; reason: string; actorKind: "maintainer" | "author" | "none" } {
const decision = evaluateCommandAuthorization({
policy: args.commandAuthorizationPolicy,
Expand All @@ -394,6 +397,7 @@ export function isAuthorizedCommandActor(args: {
pullRequestAuthorLogin: args.pullRequestAuthorLogin,
minerStatus: args.officialAuthorDetection?.status,
commandRateLimitPolicy: args.commandRateLimitPolicy,
pullRequestOpenAndNotDraft: args.pullRequestOpenAndNotDraft,
});
return { authorized: decision.authorized, reason: decision.reason, actorKind: decision.actorKind };
}
Expand Down
4 changes: 4 additions & 0 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12561,6 +12561,10 @@ async function maybeProcessGittensoryMentionCommand(
officialAuthorDetection: official,
commandAuthorizationPolicy: settings.commandAuthorization,
commandRateLimitPolicy: settings.commandRateLimitPolicy,
// #5092: the per-PR rate-limit counter below never checks PR state on its own (a closed/merged PR keeps
// its own counter forever; a brand-new PR gets a fresh one) -- without this, a contributor could keep a
// fresh chat allowance indefinitely by reopening/reusing a closed PR or spamming cheap draft PRs.
pullRequestOpenAndNotDraft: cachedPullRequest?.state === "open" && cachedPullRequest?.isDraft !== true,
});
if (!authorization.authorized) {
await recordAuditEvent(env, {
Expand Down
24 changes: 16 additions & 8 deletions src/settings/command-authorization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: {
* `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting
* never grants contributor chat access no matter what `chat`'s configured roles say. */
commandRateLimitPolicy?: "off" | "hold" | undefined;
/** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in
* {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`)
* never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by
* reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" &&
* !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false`
* denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless
* (this bounds the less-trusted pr_author tier, not already-trusted roles). */
pullRequestOpenAndNotDraft?: boolean | undefined;
}): CommandAuthorizationDecision {
const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName);
const roles = actorRoles(args);
const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null;
const prAuthorRateLimitGated =
matchedRole === "pr_author" &&
PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) &&
args.commandRateLimitPolicy !== "hold";
if (matchedRole && !prAuthorRateLimitGated) {
const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName));
if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") {
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
}
if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) {
return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles };
}
if (matchedRole) {
return {
authorized: true,
reason: authorizationReason(matchedRole),
Expand All @@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: {
allowedRoles,
};
}
if (prAuthorRateLimitGated) {
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
}
const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin);
if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) {
return {
Expand Down
31 changes: 25 additions & 6 deletions test/unit/command-authorization-engine.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,33 +195,52 @@ describe("repo command authorization policy", () => {
it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => {
// No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the
// generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all".
// pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null });
// Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
// "hold" -- the PR's own author is authorized.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" });
// A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not
// confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not
// the separate confirmed_miner exception "review" gets.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
// A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author,
// rate limiting or not.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" });
});

it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => {
// commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The
// per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so
// without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR
// or spamming cheap draft PRs.
const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const };
// Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial.
expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null });
// Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check.
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" });
// Open and not draft -- authorized.
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
// Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the
// less-trusted pr_author tier, never already-trusted roles.
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" });
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" });
});

it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => {
const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } });
expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat.");
Expand Down
31 changes: 25 additions & 6 deletions test/unit/command-authorization.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,33 +166,52 @@ describe("repo command authorization policy", () => {
it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => {
// No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the
// generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all".
// pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null });
// Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
// "hold" -- the PR's own author is authorized.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" });
// A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not
// confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not
// the separate confirmed_miner exception "review" gets.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
// A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author,
// rate limiting or not.
expect(
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" });
});

it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => {
// commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The
// per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so
// without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR
// or spamming cheap draft PRs.
const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const };
// Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial.
expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null });
// Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check.
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" });
// Open and not draft -- authorized.
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
// Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the
// less-trusted pr_author tier, never already-trusted roles.
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" });
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" });
});

it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => {
const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } });
expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat.");
Expand Down
Loading