Skip to content

Commit eb3d641

Browse files
JSONboredJSONbored
authored
fix(commands): require an open, non-draft PR for chat's pr_author grant (#5094)
The per-PR chat rate-limit counter (repoFullName#issueNumber#command) never checks PR state and never resets -- a contributor could keep a fresh chat allowance indefinitely by reopening/reusing a closed PR or spamming cheap draft PRs, since each PR number gets its own independent, permanent counter. Requires the PR to be open and not draft, enforced in evaluateCommandAuthorization alongside the existing commandRateLimitPolicy: hold requirement. Scoped only to the pr_author tier; maintainers/collaborators are unaffected regardless of PR state. Closes #5092
1 parent e04e847 commit eb3d641

8 files changed

Lines changed: 172 additions & 29 deletions

File tree

‎packages/gittensory-engine/src/settings/command-authorization.ts‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: {
129129
* `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting
130130
* never grants contributor chat access no matter what `chat`'s configured roles say. */
131131
commandRateLimitPolicy?: "off" | "hold" | undefined;
132+
/** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in
133+
* {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`)
134+
* never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by
135+
* reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" &&
136+
* !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false`
137+
* denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless
138+
* (this bounds the less-trusted pr_author tier, not already-trusted roles). */
139+
pullRequestOpenAndNotDraft?: boolean | undefined;
132140
}): CommandAuthorizationDecision {
133141
const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName);
134142
const roles = actorRoles(args);
135143
const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null;
136-
const prAuthorRateLimitGated =
137-
matchedRole === "pr_author" &&
138-
PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) &&
139-
args.commandRateLimitPolicy !== "hold";
140-
if (matchedRole && !prAuthorRateLimitGated) {
144+
const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName));
145+
if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") {
146+
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
147+
}
148+
if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) {
149+
return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles };
150+
}
151+
if (matchedRole) {
141152
return {
142153
authorized: true,
143154
reason: authorizationReason(matchedRole),
@@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: {
146157
allowedRoles,
147158
};
148159
}
149-
if (prAuthorRateLimitGated) {
150-
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
151-
}
152160
const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin);
153161
if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) {
154162
return {

‎src/github/commands.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,9 @@ export function isAuthorizedCommandActor(args: {
385385
/** #5084: required (must be `"hold"`) for a PR author to be authorized for `chat` -- see
386386
* PR_AUTHOR_RATE_LIMITED_COMMANDS in settings/command-authorization.ts. */
387387
commandRateLimitPolicy?: "off" | "hold" | undefined;
388+
/** #5092: ALSO required (must be `true`) for a PR author to be authorized for `chat` -- the caller-computed
389+
* `pr.state === "open" && !pr.isDraft`, since the per-PR rate-limit counter never checks PR state on its own. */
390+
pullRequestOpenAndNotDraft?: boolean | undefined;
388391
}): { authorized: boolean; reason: string; actorKind: "maintainer" | "author" | "none" } {
389392
const decision = evaluateCommandAuthorization({
390393
policy: args.commandAuthorizationPolicy,
@@ -394,6 +397,7 @@ export function isAuthorizedCommandActor(args: {
394397
pullRequestAuthorLogin: args.pullRequestAuthorLogin,
395398
minerStatus: args.officialAuthorDetection?.status,
396399
commandRateLimitPolicy: args.commandRateLimitPolicy,
400+
pullRequestOpenAndNotDraft: args.pullRequestOpenAndNotDraft,
397401
});
398402
return { authorized: decision.authorized, reason: decision.reason, actorKind: decision.actorKind };
399403
}

‎src/queue/processors.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12561,6 +12561,10 @@ async function maybeProcessGittensoryMentionCommand(
1256112561
officialAuthorDetection: official,
1256212562
commandAuthorizationPolicy: settings.commandAuthorization,
1256312563
commandRateLimitPolicy: settings.commandRateLimitPolicy,
12564+
// #5092: the per-PR rate-limit counter below never checks PR state on its own (a closed/merged PR keeps
12565+
// its own counter forever; a brand-new PR gets a fresh one) -- without this, a contributor could keep a
12566+
// fresh chat allowance indefinitely by reopening/reusing a closed PR or spamming cheap draft PRs.
12567+
pullRequestOpenAndNotDraft: cachedPullRequest?.state === "open" && cachedPullRequest?.isDraft !== true,
1256412568
});
1256512569
if (!authorization.authorized) {
1256612570
await recordAuditEvent(env, {

‎src/settings/command-authorization.ts‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,15 +129,26 @@ export function evaluateCommandAuthorization(args: {
129129
* `pr_author` weren't in the allowed-roles list at all, so a repo that hasn't turned on rate limiting
130130
* never grants contributor chat access no matter what `chat`'s configured roles say. */
131131
commandRateLimitPolicy?: "off" | "hold" | undefined;
132+
/** #5092: ALSO required (must be `true`) for a bare `pr_author` match to authorize a command in
133+
* {@link PR_AUTHOR_RATE_LIMITED_COMMANDS} -- the per-PR rate-limit counter (`repoFullName#issueNumber#command`)
134+
* never resets or checks PR state, so without this a contributor could keep a fresh allowance forever by
135+
* reopening/reusing a closed PR or spamming cheap draft PRs. Caller-computed (e.g. `pr.state === "open" &&
136+
* !pr.isDraft`) so this function doesn't need to know GitHub's own state-string conventions. Unset/`false`
137+
* denies exactly like a missing rate-limit policy -- maintainers/collaborators are unaffected regardless
138+
* (this bounds the less-trusted pr_author tier, not already-trusted roles). */
139+
pullRequestOpenAndNotDraft?: boolean | undefined;
132140
}): CommandAuthorizationDecision {
133141
const allowedRoles = commandAuthorizationAllowedRoles(args.policy, args.commandName);
134142
const roles = actorRoles(args);
135143
const matchedRole = roles.find((role) => allowedRoles.includes(role)) ?? null;
136-
const prAuthorRateLimitGated =
137-
matchedRole === "pr_author" &&
138-
PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName)) &&
139-
args.commandRateLimitPolicy !== "hold";
140-
if (matchedRole && !prAuthorRateLimitGated) {
144+
const prAuthorGatedCommand = matchedRole === "pr_author" && PR_AUTHOR_RATE_LIMITED_COMMANDS.has(normalizeCommandName(args.commandName));
145+
if (prAuthorGatedCommand && args.commandRateLimitPolicy !== "hold") {
146+
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
147+
}
148+
if (prAuthorGatedCommand && args.pullRequestOpenAndNotDraft !== true) {
149+
return { authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null, allowedRoles };
150+
}
151+
if (matchedRole) {
141152
return {
142153
authorized: true,
143154
reason: authorizationReason(matchedRole),
@@ -146,9 +157,6 @@ export function evaluateCommandAuthorization(args: {
146157
allowedRoles,
147158
};
148159
}
149-
if (prAuthorRateLimitGated) {
150-
return { authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null, allowedRoles };
151-
}
152160
const ownPrAuthor = isSameLogin(args.commenterLogin, args.pullRequestAuthorLogin);
153161
if (ownPrAuthor && allowedRoles.includes("confirmed_miner")) {
154162
return {

‎test/unit/command-authorization-engine.test.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -195,33 +195,52 @@ describe("repo command authorization policy", () => {
195195
it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => {
196196
// No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the
197197
// generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all".
198+
// pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically.
198199
expect(
199-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }),
200+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }),
200201
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null });
201202
// Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check.
202203
expect(
203-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }),
204+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }),
204205
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
205206
// "hold" -- the PR's own author is authorized.
206207
expect(
207-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
208+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
208209
).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" });
209210
// A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not
210211
// confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not
211212
// the separate confirmed_miner exception "review" gets.
212213
expect(
213-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }),
214+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }),
214215
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
215216
expect(
216-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }),
217+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
217218
).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
218219
// A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author,
219220
// rate limiting or not.
220221
expect(
221-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
222+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
222223
).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" });
223224
});
224225

226+
it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => {
227+
// commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The
228+
// per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so
229+
// without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR
230+
// or spamming cheap draft PRs.
231+
const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const };
232+
// Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial.
233+
expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null });
234+
// Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check.
235+
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" });
236+
// Open and not draft -- authorized.
237+
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
238+
// Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the
239+
// less-trusted pr_author tier, never already-trusted roles.
240+
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" });
241+
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" });
242+
});
243+
225244
it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => {
226245
const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } });
227246
expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat.");

‎test/unit/command-authorization.test.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -166,33 +166,52 @@ describe("repo command authorization policy", () => {
166166
it("#5084: a chat pr_author match is only granted when commandRateLimitPolicy is \"hold\" for the repo", () => {
167167
// No rate-limit policy passed at all (the undefined branch) -- denied, with a distinct reason from the
168168
// generic denials so an operator can tell "rate limiting isn't on" apart from "not authorized at all".
169+
// pullRequestOpenAndNotDraft: true throughout, so this test isolates the rate-limit gate specifically.
169170
expect(
170-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author" }),
171+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", pullRequestOpenAndNotDraft: true }),
171172
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting", actorKind: "author", matchedRole: null });
172173
// Explicitly "off" (not just unset) -- same denial, covering both falsy branches of the `!== "hold"` check.
173174
expect(
174-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off" }),
175+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "off", pullRequestOpenAndNotDraft: true }),
175176
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
176177
// "hold" -- the PR's own author is authorized.
177178
expect(
178-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
179+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
179180
).toMatchObject({ authorized: true, reason: "allowed_pr_author", actorKind: "author", matchedRole: "pr_author" });
180181
// A confirmed miner acting on their OWN PR matches pr_author first (chat's roles list has pr_author, not
181182
// confirmed_miner) -- so a miner is gated by the SAME rate-limit requirement as any other PR author, not
182183
// the separate confirmed_miner exception "review" gets.
183184
expect(
184-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed" }),
185+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", pullRequestOpenAndNotDraft: true }),
185186
).toMatchObject({ authorized: false, reason: "pr_author_requires_rate_limiting" });
186187
expect(
187-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold" }),
188+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "miner", pullRequestAuthorLogin: "miner", minerStatus: "confirmed", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
188189
).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
189190
// A commenter on someone ELSE's PR is still denied outright -- pr_author never matches for a non-author,
190191
// rate limiting or not.
191192
expect(
192-
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" }),
193+
evaluateCommandAuthorization({ commandName: "chat", commenterLogin: "other", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold", pullRequestOpenAndNotDraft: true }),
193194
).toMatchObject({ authorized: false, reason: "not_maintainer_or_pr_author" });
194195
});
195196

197+
it("#5092: a chat pr_author match is ALSO only granted when the PR is open and not draft", () => {
198+
// commandRateLimitPolicy: "hold" throughout, so this test isolates the PR-state gate specifically. The
199+
// per-PR rate-limit counter (repoFullName#issueNumber#command) never checks PR state on its own, so
200+
// without this a contributor could keep a fresh chat allowance forever by reopening/reusing a closed PR
201+
// or spamming cheap draft PRs.
202+
const base = { commandName: "chat", commenterLogin: "author", pullRequestAuthorLogin: "author", commandRateLimitPolicy: "hold" as const };
203+
// Unset (the undefined branch) -- denied, distinct reason from the rate-limit denial.
204+
expect(evaluateCommandAuthorization(base)).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr", actorKind: "author", matchedRole: null });
205+
// Explicitly false (not just unset) -- same denial, covering both falsy branches of the `!== true` check.
206+
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: false })).toMatchObject({ authorized: false, reason: "pr_author_requires_open_pr" });
207+
// Open and not draft -- authorized.
208+
expect(evaluateCommandAuthorization({ ...base, pullRequestOpenAndNotDraft: true })).toMatchObject({ authorized: true, reason: "allowed_pr_author", matchedRole: "pr_author" });
209+
// Maintainers/collaborators are completely unaffected by PR state -- the check only bounds the
210+
// less-trusted pr_author tier, never already-trusted roles.
211+
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "OWNER" })).toMatchObject({ authorized: true, reason: "maintainer_invocation" });
212+
expect(evaluateCommandAuthorization({ commandName: "chat", commenterAssociation: "COLLABORATOR" })).toMatchObject({ authorized: true, reason: "collaborator_invocation" });
213+
});
214+
196215
it("#5084: a maintainer's yml override restating chat's own default (incl. pr_author) is not clamped away", () => {
197216
const restated = normalizeCommandAuthorizationPolicy({ commands: { chat: ["collaborator", "pr_author"] } });
198217
expect(restated.warnings).not.toContain("Ignored author command authorization roles for maintainer-only command: chat.");

0 commit comments

Comments
 (0)