diff --git a/examples/single-repo-team-bot/.paseo/workflows/github.yml b/examples/single-repo-team-bot/.paseo/workflows/github.yml index 1e413791..29fa146d 100644 --- a/examples/single-repo-team-bot/.paseo/workflows/github.yml +++ b/examples/single-repo-team-bot/.paseo/workflows/github.yml @@ -3,7 +3,7 @@ on: github.issue_comment max_runtime: 2h filters: repo: your-org/your-repo - from_users: [your-github-login] + from_teams: [your-org/your-team] contains: "@your-bot" steps: - id: classify diff --git a/examples/single-repo-team-bot/README.md b/examples/single-repo-team-bot/README.md index 21c8d149..6cbb15b7 100644 --- a/examples/single-repo-team-bot/README.md +++ b/examples/single-repo-team-bot/README.md @@ -15,11 +15,14 @@ Copy `.paseo` to your repository root, then replace these values: | `your-github-connection` | GitHub connection slug in Hub | | `YOUR_DISCORD_USER_ID` | Discord user allowed to trigger runs | | `YOUR_SLACK_USER_ID` | Slack user allowed to trigger runs | -| `your-github-login` | GitHub user allowed to trigger runs | +| `your-org/your-team` | GitHub team allowed to trigger runs | | `@your-bot` | Mention that starts the GitHub workflow | -Connect Discord, Slack, and GitHub to the project before enabling their triggers. Keep the user -allowlists narrow; wildcards are not supported. +Connect Discord, Slack, and GitHub to the project before enabling their triggers. GitHub team +filters use `organization/team-slug` and require the GitHub App's organization **Members** +permission with read access. If membership cannot be checked, Hub does not start a run. A GitHub +`from_users` allowlist can be combined with `from_teams`; either grants access. Keep allowlists +narrow; wildcards are not supported. ## Deploy diff --git a/src/config/compiler.test.ts b/src/config/compiler.test.ts index a48637d1..514fb42f 100644 --- a/src/config/compiler.test.ts +++ b/src/config/compiler.test.ts @@ -325,6 +325,45 @@ describe("workflow compiler", () => { }); }); + it("supports GitHub team trigger allowlists and rejects invalid team filter uses", () => { + const raw = configuration(); + const trigger = raw.triggers[0]!; + const githubTrigger = { + ...trigger, + on: "github.issue_comment", + filters: { from_teams: ["getpaseo/maintainers"] }, + }; + + assert.deepEqual(compileHubConfig({ ...raw, triggers: [githubTrigger] }).triggers[0]?.filters, { + from_teams: ["getpaseo/maintainers"], + }); + assert.throws( + () => + compileHubConfig({ + ...raw, + triggers: [{ ...trigger, on: "slack.mention", filters: githubTrigger.filters }], + }), + /filters\.from_teams only for GitHub events/iu, + ); + assert.throws( + () => + compileHubConfig({ + ...raw, + triggers: [ + { + ...githubTrigger, + filters: { from_teams: ["maintainers"] }, + }, + ], + }), + /organization\/team-slug/iu, + ); + assert.throws( + () => compileHubConfig({ ...raw, triggers: [{ ...trigger, on: "github.issue_comment" }] }), + /filters\.from_users or filters\.from_teams/iu, + ); + }); + it("allows a project-scoped Linear scout but keeps reactive Linear triggers actor-allowlisted", () => { const raw = configuration(); const trigger = raw.triggers[0]!; diff --git a/src/config/compiler.ts b/src/config/compiler.ts index e5be558f..a23da521 100644 --- a/src/config/compiler.ts +++ b/src/config/compiler.ts @@ -32,6 +32,7 @@ const EVENT_NAME = /^[a-z][a-z0-9_-]*\.[a-z][a-z0-9_-]*$/u; const DURATION = /^([1-9][0-9]*)(ms|s|m|h)$/u; const MAX_DURATION_MS = 24 * 60 * 60_000; const INPUT_NAME = /^[a-z][a-z0-9_-]*$/u; +const GITHUB_TEAM_REFERENCE = /^[^/\s]+\/[^/\s]+$/u; const DYNAMIC_INPUT_REFERENCE = /^\$\{\{\s*paseo\.inputs\.([a-z][a-z0-9_-]*)\s*\}\}$/u; const EXPRESSION_START = "${{"; const EXPRESSION_END = "}}"; @@ -102,6 +103,9 @@ const AuthoredTriggerFilterSchema = z assignees: z.array(z.string().min(1)).min(1).optional(), channels: z.array(z.string().min(1)).optional(), from_users: z.array(z.string().min(1)).optional(), + from_teams: z + .array(z.string().regex(GITHUB_TEAM_REFERENCE, "must be formatted as organization/team-slug")) + .optional(), inputs: z.record(z.string(), InputValueSchema).optional(), connection: z .string() @@ -264,10 +268,11 @@ export type CompiledSteps = readonly CompiledStep[]; export type CompiledTriggerFilter = Readonly< Omit< AuthoredTriggerFilter, - "channels" | "from_users" | "states" | "labels" | "exclude_labels" | "assignees" + "channels" | "from_users" | "from_teams" | "states" | "labels" | "exclude_labels" | "assignees" > & { channels?: readonly string[] | undefined; from_users?: readonly string[] | undefined; + from_teams?: readonly string[] | undefined; states?: readonly string[] | undefined; labels?: readonly string[] | undefined; exclude_labels?: readonly string[] | undefined; @@ -1377,6 +1382,10 @@ function validateAuthoredIds(config: AuthoredHubConfig): void { } function validateTriggerLaunchSecurity(trigger: CompiledTrigger): void { + const fromTeams = trigger.filters?.from_teams ?? []; + if (fromTeams.length > 0 && !trigger.on.startsWith("github.")) { + throw new Error(`trigger ${trigger.name} may use filters.from_teams only for GitHub events`); + } if (isEditorEvent(trigger.on) && eventDefinition(trigger.on).origin === "hub") return; // A project scout is an intentionally autonomous, project-scoped policy. Every other // externally-originated Linear action remains actor-allowlisted below. @@ -1388,9 +1397,13 @@ function validateTriggerLaunchSecurity(trigger: CompiledTrigger): void { } return; } - if ((trigger.filters?.from_users?.length ?? 0) === 0) { + const fromUsers = trigger.filters?.from_users ?? []; + if (fromUsers.length === 0 && fromTeams.length === 0) { + const allowlist = trigger.on.startsWith("github.") + ? "filters.from_users or filters.from_teams" + : "filters.from_users"; throw new Error( - `trigger ${trigger.name} requires a non-empty filters.from_users allowlist for externally sourced events`, + `trigger ${trigger.name} requires a non-empty ${allowlist} allowlist for externally sourced events`, ); } } diff --git a/src/provider-applications/guides.test.ts b/src/provider-applications/guides.test.ts index 668c35a0..80b0781b 100644 --- a/src/provider-applications/guides.test.ts +++ b/src/provider-applications/guides.test.ts @@ -208,6 +208,7 @@ test("GitHub renders permissions as a mapping and events as a list, never as pro { name: "Issues", access: "Read and write" }, { name: "Pull requests", access: "Read and write" }, { name: "Metadata", access: "Read-only" }, + { name: "Members", access: "Read-only" }, ]); assert.deepEqual( steps.flatMap((step) => step.events ?? []), diff --git a/src/provider-applications/guides.ts b/src/provider-applications/guides.ts index c46053ab..f4e48eba 100644 --- a/src/provider-applications/guides.ts +++ b/src/provider-applications/guides.ts @@ -266,6 +266,14 @@ export const GITHUB_GUIDE: ProviderGuide = { "Push", ], }, + { + segments: [ + { kind: "text", value: "If a workflow uses a GitHub team allowlist, under " }, + { kind: "term", value: "Organization permissions" }, + { kind: "text", value: ", grant:" }, + ], + permissions: [{ name: "Members", access: "Read-only" }], + }, ], fields: [ { diff --git a/src/providers/github/index.ts b/src/providers/github/index.ts index 9ac06a25..791196c6 100644 --- a/src/providers/github/index.ts +++ b/src/providers/github/index.ts @@ -25,6 +25,7 @@ import { createGitHubTriggerProvider, type GitHubReactionClient, } from "../../triggers/github/provider.js"; +import { createGitHubTeamMembershipClient } from "../../triggers/github/team-membership.js"; import { organizationBillingUrl } from "../../triggers/failure-notice.js"; import { createWebhookSource } from "../../triggers/github/webhook.js"; import { @@ -90,6 +91,7 @@ export function createGitHubRegistration( const appAuth = options.appAuth ?? createGitHubAuth({ appId: configuration.appId, privateKey: configuration.privateKey }); + const teamMemberships = createGitHubTeamMembershipClient(appAuth); const client = options.connectionClient ?? createGitHubConnectionClient({ @@ -249,6 +251,7 @@ export function createGitHubRegistration( return createGitHubTriggerProvider({ configurationStoreForProject, reactions, + teamMemberships, comments, billingUrlForOrganization: (organizationId) => organizationBillingUrl(database, options.publicBaseUrl!, organizationId), diff --git a/src/triggers/github/classification.ts b/src/triggers/github/classification.ts index e315676a..159f0a44 100644 --- a/src/triggers/github/classification.ts +++ b/src/triggers/github/classification.ts @@ -4,6 +4,7 @@ import { PullRequestPayloadSchema, PullRequestReviewCommentPayloadSchema, PullRequestReviewPayloadSchema, + PushPayloadSchema, } from "../../auth/github-events.js"; import type { NormalizedGitHubEvent } from "../../auth/github-events.js"; @@ -52,9 +53,19 @@ export function classifyGitHubEvent(event: NormalizedGitHubEvent): GitHubClassif if (event.type === "issue_comment") return classifyIssueComment(event); if (event.type === "pull_request_review") return classifyReview(event); if (event.type === "pull_request_review_comment") return classifyReviewComment(event); + if (event.type === "push") return classifyPush(event); return emptyClassification(); } +function classifyPush(event: NormalizedGitHubEvent): GitHubClassifiedEvent { + const parsed = PushPayloadSchema.safeParse(event.payload); + if (!parsed.success) return emptyClassification(); + return { + ...emptyClassification(), + actor: parsed.data.sender?.login ?? "", + }; +} + function classifyIssue(event: NormalizedGitHubEvent): GitHubClassifiedEvent { const payload = IssuesPayloadSchema.parse(event.payload); const item = payload.issue === undefined ? null : itemFor("issue", payload.issue); diff --git a/src/triggers/github/match.test.ts b/src/triggers/github/match.test.ts index f959eb85..dfcfa2aa 100644 --- a/src/triggers/github/match.test.ts +++ b/src/triggers/github/match.test.ts @@ -3,6 +3,7 @@ import { describe, it } from "vitest"; import { compileHubConfig } from "../../config/index.js"; import type { NormalizedGitHubEvent } from "../../auth/github-events.js"; import { matchTriggers, readGitHubInvocationMessage } from "./match.js"; +import type { GitHubTeamMembershipClient } from "./team-membership.js"; describe("GitHub trigger matching", () => { it.each([ @@ -155,7 +156,7 @@ describe("GitHub trigger matching", () => { event: eventFor("issue_comment", { action: "edited", issue: issue(), comment: comment() }), expected: 1, }, - ])("$acceptance", ({ on, filters, event, expected, connectionId }) => { + ])("$acceptance", async ({ on, filters, event, expected, connectionId }) => { const config = configFor({ from_users: ["boudra"], ...filters, @@ -172,7 +173,15 @@ describe("GitHub trigger matching", () => { }, ], }; - assert.equal(matchTriggers(configured, event, connectionId).length, expected); + assert.equal( + ( + await matchTriggers(configured, event, { + teamMemberships: denyTeams, + ...(connectionId === undefined ? {} : { connectionId }), + }) + ).length, + expected, + ); }); it("allows every actor only when the wildcard is explicit", () => { @@ -231,54 +240,72 @@ describe("GitHub trigger matching", () => { }, ])( "keeps legacy $name actor, text, and action-agnostic matching", - ({ event, text, anotherAction }) => { + async ({ event, text, anotherAction }) => { const config = configFor({ contains: text, from_users: ["boudra"] }); const trigger = config.triggers[0]!; const configured = { ...config, triggers: [{ ...trigger, on: `github.${event.type}` }] }; assert.equal(readGitHubInvocationMessage(event), text); - assert.equal(matchTriggers(configured, event).length, 1); assert.equal( - matchTriggers(configured, { - ...event, - payload: { ...event.payload, sender: { login: "someone-else" } }, - }).length, + (await matchTriggers(configured, event, { teamMemberships: denyTeams })).length, + 1, + ); + assert.equal( + ( + await matchTriggers( + configured, + { + ...event, + payload: { ...event.payload, sender: { login: "someone-else" } }, + }, + { teamMemberships: denyTeams }, + ) + ).length, 0, ); assert.equal( - matchTriggers(configured, { - ...event, - payload: { ...event.payload, action: anotherAction }, - }).length, + ( + await matchTriggers( + configured, + { + ...event, + payload: { ...event.payload, action: anotherAction }, + }, + { teamMemberships: denyTeams }, + ) + ).length, 1, ); }, ); - it("matches the compiled one-step trigger by repository, text, and actor", () => { + it("matches the compiled one-step trigger by repository, text, and actor", async () => { const config = configFor({ repo: "boudra/faro", contains: "@paseo", from_users: ["boudra"] }); - const matches = matchTriggers(config, createEvent()); + const matches = await matchTriggers(config, createEvent(), { teamMemberships: denyTeams }); assert.equal(matches.length, 1); assert.equal(matches[0]?.trigger.name, "github-comment"); }); - it("matches pattern-less comments while preserving the provider allowlist", () => { + it("matches pattern-less comments while preserving the provider allowlist", async () => { const config = configFor({ repo: "boudra/faro", from_users: ["boudra"] }); assert.equal( - matchTriggers( - config, - createEvent({ - payload: { - comment: { id: 123, body: "please explain", user: { login: "boudra" } }, - sender: { login: "boudra" }, - }, - }), + ( + await matchTriggers( + config, + createEvent({ + payload: { + comment: { id: 123, body: "please explain", user: { login: "boudra" } }, + sender: { login: "boudra" }, + }, + }), + { teamMemberships: denyTeams }, + ) ).length, 1, ); assert.deepEqual( - matchTriggers( + await matchTriggers( config, createEvent({ payload: { @@ -286,28 +313,37 @@ describe("GitHub trigger matching", () => { sender: { login: "stranger" }, }, }), + { teamMemberships: denyTeams }, ), [], ); }); - it("keeps repository and pattern filters literal", () => { + it("keeps repository and pattern filters literal", async () => { const config = configFor({ repo: "boudra/faro", pattern: "@paseo", from_users: ["boudra"] }); assert.equal( - matchTriggers( - config, - createEvent({ - payload: { - comment: { id: 123, body: "@paseo please explain", user: { login: "boudra" } }, - sender: { login: "boudra" }, - }, - }), + ( + await matchTriggers( + config, + createEvent({ + payload: { + comment: { id: 123, body: "@paseo please explain", user: { login: "boudra" } }, + sender: { login: "boudra" }, + }, + }), + { teamMemberships: denyTeams }, + ) ).length, 1, ); - assert.deepEqual(matchTriggers(config, createEvent({ repo: "elsewhere/repo" })), []); assert.deepEqual( - matchTriggers( + await matchTriggers(config, createEvent({ repo: "elsewhere/repo" }), { + teamMemberships: denyTeams, + }), + [], + ); + assert.deepEqual( + await matchTriggers( config, createEvent({ payload: { @@ -315,12 +351,13 @@ describe("GitHub trigger matching", () => { sender: { login: "boudra" }, }, }), + { teamMemberships: denyTeams }, ), [], ); }); - it("applies the same security filters to pull-request review comments", () => { + it("applies the same security filters to pull-request review comments", async () => { const config = compileHubConfig({ environments: [{ name: "runner", kind: "daemon", daemon: "runner", cwd: "/repo" }], triggers: [ @@ -351,10 +388,111 @@ describe("GitHub trigger matching", () => { pull_request: { head: { ref: "topic" } }, }, }; - assert.equal(matchTriggers(config, event).length, 1); + assert.equal((await matchTriggers(config, event, { teamMemberships: denyTeams })).length, 1); + }); + + it("allows active GitHub team members and fails closed when membership is unavailable", async () => { + const config = configFor({ + repo: "boudra/faro", + contains: "@paseo", + from_teams: ["boudra/maintainers"], + }); + const activeTeams = new TestTeamMemberships(true); + + assert.equal( + (await matchTriggers(config, createEvent(), { teamMemberships: activeTeams })).length, + 1, + ); + assert.deepEqual(activeTeams.checks, [ + { + installationId: 42, + organization: "boudra", + teamSlug: "maintainers", + username: "boudra", + }, + ]); + assert.deepEqual( + await matchTriggers(config, createEvent(), { teamMemberships: denyTeams }), + [], + ); + }); + + it("applies team authorization to semantic GitHub triggers", async () => { + const base = configFor({ + from_teams: ["boudra/maintainers"], + label: "ready-for-agent", + }); + const trigger = base.triggers[0]!; + const config = { + ...base, + triggers: [{ ...trigger, on: "github.issue_label_added" }], + }; + const event = eventFor("issues", { + action: "labeled", + issue: issue({ labels: [{ name: "ready-for-agent" }] }), + label: { name: "ready-for-agent" }, + }); + + assert.equal( + (await matchTriggers(config, event, { teamMemberships: new TestTeamMemberships(true) })) + .length, + 1, + ); + }); + + it("uses the push payload sender for team authorization", async () => { + const base = configFor({ from_teams: ["boudra/maintainers"] }); + const trigger = base.triggers[0]!; + const config = { ...base, triggers: [{ ...trigger, on: "github.push" }] }; + const memberships = new TestTeamMemberships(true); + const event = eventFor("push", { + after: "commit-sha", + ref: "refs/heads/main", + }); + + assert.equal((await matchTriggers(config, event, { teamMemberships: memberships })).length, 1); + assert.deepEqual(memberships.checks, [ + { + installationId: 42, + organization: "boudra", + teamSlug: "maintainers", + username: "boudra", + }, + ]); + }); + + it("treats direct-user and team allowlists as alternatives", async () => { + const config = configFor({ + repo: "boudra/faro", + contains: "@paseo", + from_users: ["boudra"], + from_teams: ["boudra/maintainers"], + }); + const memberships = new TestTeamMemberships(false); + + assert.equal( + (await matchTriggers(config, createEvent(), { teamMemberships: memberships })).length, + 1, + ); + assert.deepEqual(memberships.checks, []); }); }); +const denyTeams: GitHubTeamMembershipClient = { + isActiveMember: async () => false, +}; + +class TestTeamMemberships implements GitHubTeamMembershipClient { + readonly checks: Array[0]> = []; + + constructor(private readonly active: boolean) {} + + async isActiveMember(input: Parameters[0]) { + this.checks.push(input); + return this.active; + } +} + function configFor(filters: Record) { return compileHubConfig({ environments: [{ name: "runner", kind: "daemon", daemon: "runner", cwd: "/repo" }], diff --git a/src/triggers/github/match.ts b/src/triggers/github/match.ts index 74ed3107..c3488bc4 100644 --- a/src/triggers/github/match.ts +++ b/src/triggers/github/match.ts @@ -4,6 +4,7 @@ import type { } from "../../config/index.js"; import type { NormalizedGitHubEvent } from "../../auth/github-events.js"; import { classifyGitHubEvent } from "./classification.js"; +import type { GitHubTeamMembershipClient } from "./team-membership.js"; type MatchedTriggerDefinition = Pick; @@ -12,6 +13,11 @@ export interface MatchedTriggerEvent { trigger: MatchedTriggerDefinition; } +export interface GitHubTriggerMatchOptions { + connectionId?: string | null; + teamMemberships: GitHubTeamMembershipClient; +} + export function readGitHubInvocationMessage(event: NormalizedGitHubEvent): string { return classifyGitHubEvent(event).text; } @@ -41,81 +47,178 @@ export function matchTriggers( config: { triggers: readonly MatchedTriggerDefinition[] }, event: NormalizedGitHubEvent, connectionId?: string | null, -): MatchedTriggerEvent[] { +): MatchedTriggerEvent[]; +export function matchTriggers( + config: { triggers: readonly MatchedTriggerDefinition[] }, + event: NormalizedGitHubEvent, + options: GitHubTriggerMatchOptions, +): Promise; +export function matchTriggers( + config: { triggers: readonly MatchedTriggerDefinition[] }, + event: NormalizedGitHubEvent, + input?: string | null | GitHubTriggerMatchOptions, +): MatchedTriggerEvent[] | Promise { const classified = classifyGitHubEvent(event); const eventNames = new Set([`github.${event.type}`, classified.semanticEvent]); + + if (typeof input !== "object" || input === null) { + return matchTriggersWithoutTeams(config, event, classified, eventNames, input); + } + return matchTriggersWithTeams(config, event, classified, eventNames, input); +} + +function matchTriggersWithoutTeams( + config: { triggers: readonly MatchedTriggerDefinition[] }, + event: NormalizedGitHubEvent, + classified: ReturnType, + eventNames: ReadonlySet, + connectionId: string | null | undefined, +): MatchedTriggerEvent[] { const matches: MatchedTriggerEvent[] = []; for (const trigger of config.triggers) { if ( !eventNames.has(trigger.on) || - !matchesFilter(classified, trigger.filters, event, connectionId) + !matchesStaticFilter(classified, trigger.filters, event, connectionId) || + !matchesUserFilter(trigger.filters, classified.actor) ) { continue; } + matches.push({ event, trigger }); + } + return matches; +} + +async function matchTriggersWithTeams( + config: { triggers: readonly MatchedTriggerDefinition[] }, + event: NormalizedGitHubEvent, + classified: ReturnType, + eventNames: ReadonlySet, + options: GitHubTriggerMatchOptions, +): Promise { + const matches: MatchedTriggerEvent[] = []; + const membershipChecks = new Map>(); + + for (const trigger of config.triggers) { + if (!eventNames.has(trigger.on)) continue; + if (!matchesStaticFilter(classified, trigger.filters, event, options.connectionId)) continue; + if (matchesUserFilter(trigger.filters, classified.actor)) { + matches.push({ event, trigger }); + continue; + } + if (trigger.filters === undefined || classified.actor.length === 0) continue; + if ( + !(await matchesTeamFilter( + event, + classified.actor, + trigger.filters.from_teams, + options.teamMemberships, + membershipChecks, + )) + ) { + continue; + } matches.push({ event, trigger }); } return matches; } -function matchesFilter( +function matchesStaticFilter( classified: ReturnType, filter: TriggerFilter | undefined, event: NormalizedGitHubEvent, - connectionId?: string | null, + connectionId: string | null | undefined, ): boolean { - if (filter === undefined) { - return false; - } + if (filter === undefined) return false; + return ( + hasIdentityAllowlist(filter) && + matchesConnection(filter, connectionId) && + matchesRepository(filter, event) && + matchesText(filter, classified.text) && + matchesLabels(filter, classified) + ); +} - if (filter.from_users === undefined || filter.from_users.length === 0) { - return false; - } +function hasIdentityAllowlist(filter: TriggerFilter): boolean { + return (filter.from_users?.length ?? 0) > 0 || (filter.from_teams?.length ?? 0) > 0; +} - if (filter.connectionId !== undefined && filter.connectionId !== connectionId) { - return false; - } +function matchesConnection( + filter: TriggerFilter, + connectionId: string | null | undefined, +): boolean { + return filter.connectionId === undefined || filter.connectionId === connectionId; +} +function matchesRepository(filter: TriggerFilter, event: NormalizedGitHubEvent): boolean { const repo = filter["repo"]; - if (typeof repo === "string" && repo !== event.repo) { - return false; - } + if (typeof repo === "string" && repo !== event.repo) return false; const resourceId = filter["resourceId"]; - if (typeof resourceId === "string" && resourceId !== String(event.repositoryId)) { - return false; - } + return typeof resourceId !== "string" || resourceId === String(event.repositoryId); +} +function matchesText(filter: TriggerFilter, text: string): boolean { const pattern = readStringFilter(filter, "pattern"); - if (pattern !== undefined && !classified.text.startsWith(pattern)) { - return false; - } + if (pattern !== undefined && !text.startsWith(pattern)) return false; const contains = readStringFilter(filter, "contains"); - if (contains !== undefined && !classified.text.includes(contains)) { - return false; - } - - if (!filter.from_users.includes("*") && !filter.from_users.includes(classified.actor)) { - return false; - } + return contains === undefined || text.includes(contains); +} +function matchesLabels( + filter: TriggerFilter, + classified: ReturnType, +): boolean { const label = readStringFilter(filter, "label"); - if (label !== undefined && !sameLabel(label, classified.changedLabel)) { - return false; - } + if (label !== undefined && !sameLabel(label, classified.changedLabel)) return false; - const labels = filter.labels; - if ( - labels !== undefined && - !labels.every((labelName) => classified.labels.some((current) => sameLabel(labelName, current))) - ) { - return false; + return ( + filter.labels === undefined || + filter.labels.every((labelName) => + classified.labels.some((current) => sameLabel(labelName, current)), + ) + ); +} + +function matchesUserFilter(filter: TriggerFilter | undefined, actor: string): boolean { + const fromUsers = filter?.from_users; + return fromUsers?.includes("*") === true || fromUsers?.includes(actor) === true; +} + +async function matchesTeamFilter( + event: NormalizedGitHubEvent, + actor: string, + references: readonly string[] | undefined, + teamMemberships: GitHubTeamMembershipClient, + membershipChecks: Map>, +): Promise { + if (references === undefined) return false; + + for (const reference of references) { + const team = parseTeamReference(reference); + if (team === undefined) continue; + const key = [event.installationId, team.organization, team.slug, actor].join("\u0000"); + let membership = membershipChecks.get(key); + if (membership === undefined) { + membership = teamMemberships.isActiveMember({ + installationId: event.installationId, + organization: team.organization, + teamSlug: team.slug, + username: actor, + }); + membershipChecks.set(key, membership); + } + try { + if (await membership) return true; + } catch { + // Membership checks are authorization decisions. An unavailable client denies the trigger. + } } - return true; + return false; } function readStringFilter( @@ -132,3 +235,18 @@ function sameLabel(expected: string, actual: string | undefined): boolean { expected.localeCompare(actual, undefined, { sensitivity: "accent" }) === 0 ); } + +function parseTeamReference(value: string): { organization: string; slug: string } | undefined { + const parts = value.split("/"); + const organization = parts.length === 2 ? parts[0] : undefined; + const slug = parts.length === 2 ? parts[1] : undefined; + if ( + organization === undefined || + organization.length === 0 || + slug === undefined || + slug.length === 0 + ) { + return undefined; + } + return { organization, slug }; +} diff --git a/src/triggers/github/provider.test.ts b/src/triggers/github/provider.test.ts index df3314e0..e1bb98b1 100644 --- a/src/triggers/github/provider.test.ts +++ b/src/triggers/github/provider.test.ts @@ -5,8 +5,9 @@ import { createMemoryDatabase } from "../../db/memory.js"; import type { DurableProviderEvent } from "../../db/types.js"; import { createActiveProjectConfiguration } from "../../test-utils/project-configuration.js"; import { createDurableWorkflowHandler } from "../../workflows/engine.js"; -import type { GitHubReactionClient } from "./provider.js"; import { createGitHubReactionClient, createGitHubTriggerProvider } from "./provider.js"; +import type { GitHubReactionClient } from "./provider.js"; +import type { GitHubTeamMembershipClient } from "./team-membership.js"; import type { NormalizedGitHubEvent } from "../../auth/github-events.js"; import type { GitHubAuth } from "../../auth/github.js"; import { isAcceptedTriggerProviderMatch } from "../index.js"; @@ -29,6 +30,7 @@ describe("GitHub Phase 1 trigger provider", () => { const provider = createGitHubTriggerProvider({ configurationStoreForProject: () => store, reactions, + teamMemberships: new TestTeamMemberships(), billingUrlForOrganization: async () => "https://hub.paseo.sh/o/acme/settings/billing", comments: { createIssueComment: async (input) => { @@ -161,6 +163,50 @@ describe("GitHub Phase 1 trigger provider", () => { assert.equal(wrongActor, "trigger_filters_rejected"); }); + it("allows active GitHub team members and fails closed when their membership cannot be checked", async () => { + const base = githubConfiguration(); + const trigger = base.triggers[0]!; + const teamFilterBase = { + repo: trigger.filters.repo, + contains: trigger.filters.contains, + }; + const { project, revision, store } = await activeConfiguration({ + ...base, + triggers: [ + { + ...trigger, + filters: { + ...teamFilterBase, + from_teams: ["boudra/maintainers"], + }, + }, + ], + }); + const activeTeams = new TestTeamMemberships(true); + const activeProvider = createProvider(store, new TestReactions(), activeTeams); + + const active = await activeProvider.match( + external(project.id, revision.id, createEvent({ actor: "maintainer" })), + ); + assert.ok(Array.isArray(active)); + assert.equal(active.length, 1); + assert.deepEqual(activeTeams.checks, [ + { + installationId: 42, + organization: "boudra", + teamSlug: "maintainers", + username: "maintainer", + }, + ]); + + const denied = await createProvider( + store, + new TestReactions(), + new TestTeamMemberships(false), + ).match(external(project.id, revision.id, createEvent({ actor: "maintainer" }))); + assert.equal(denied, "trigger_filters_rejected"); + }); + it("routes a semantic trigger from the legacy GitHub webhook source", async () => { const configuration = githubConfiguration(); const trigger = configuration.triggers[0]!; @@ -491,14 +537,16 @@ describe("GitHub Phase 1 trigger provider", () => { function createProvider( store: Awaited>["store"], reactions: TestReactions, + teamMemberships: GitHubTeamMembershipClient = new TestTeamMemberships(), ) { return createGitHubTriggerProvider({ configurationStoreForProject: () => store, reactions, + teamMemberships, }); } -async function activeConfiguration(rawConfiguration = githubConfiguration()) { +async function activeConfiguration(rawConfiguration: unknown = githubConfiguration()) { return createActiveProjectConfiguration(createMemoryDatabase(), rawConfiguration); } @@ -671,3 +719,14 @@ class TestReactions implements GitHubReactionClient { this.deleted.push(input); } } + +class TestTeamMemberships implements GitHubTeamMembershipClient { + readonly checks: Array[0]> = []; + + constructor(private readonly active = false) {} + + async isActiveMember(input: Parameters[0]) { + this.checks.push(input); + return this.active; + } +} diff --git a/src/triggers/github/provider.ts b/src/triggers/github/provider.ts index 3fa2ba87..b36f509f 100644 --- a/src/triggers/github/provider.ts +++ b/src/triggers/github/provider.ts @@ -19,6 +19,7 @@ import { readGitHubInvocationParserMessage, readGitHubMention, } from "./match.js"; +import type { GitHubTeamMembershipClient } from "./team-membership.js"; import { matchesInputFilters, parseInvocation } from "../invocation.js"; import { IssueCommentPayloadSchema, @@ -148,6 +149,7 @@ interface GitHubReactionState { export function createGitHubTriggerProvider(options: { configurationStoreForProject: (projectId: string) => ProjectConfigurationStore; reactions: GitHubReactionClient; + teamMemberships: GitHubTeamMembershipClient; comments?: GitHubReplyClient; billingUrlForOrganization?: (organizationId: string) => Promise; }): TriggerProvider<"github", GitHubTriggerContext> { @@ -168,11 +170,12 @@ export function createGitHubTriggerProvider(options: { return "no_trigger_for_source"; const matches: TriggerProviderMatch[] = []; - for (const match of matchTriggers( - stored.configuration, - event, - externalTrigger.connectionId, - )) { + for (const match of await matchTriggers(stored.configuration, event, { + teamMemberships: options.teamMemberships, + ...(externalTrigger.connectionId === undefined + ? {} + : { connectionId: externalTrigger.connectionId }), + })) { const compiledTrigger = stored.configuration.triggers.find( (candidate) => candidate.name === match.trigger.name, ); diff --git a/src/triggers/github/team-membership.test.ts b/src/triggers/github/team-membership.test.ts new file mode 100644 index 00000000..a9371268 --- /dev/null +++ b/src/triggers/github/team-membership.test.ts @@ -0,0 +1,70 @@ +import assert from "node:assert/strict"; +import { describe, it } from "vitest"; +import { + createGitHubTeamMembershipClient, + type GitHubTeamMembershipAuth, +} from "./team-membership.js"; + +describe("GitHub team membership client", () => { + it("accepts only active memberships", async () => { + const harness = createHarness({ state: "active" }); + const client = createGitHubTeamMembershipClient(harness.auth); + + assert.equal( + await client.isActiveMember({ + installationId: 42, + organization: "getpaseo", + teamSlug: "maintainers", + username: "michael", + }), + true, + ); + assert.deepEqual(harness.requests, [ + { + route: "GET /orgs/{org}/teams/{team_slug}/memberships/{username}", + parameters: { + org: "getpaseo", + team_slug: "maintainers", + username: "michael", + headers: { "x-github-api-version": "2026-03-10" }, + }, + }, + ]); + }); + + it.each([ + ["pending", { state: "pending" }], + ["missing", httpError(404)], + ["permission denied", httpError(403)], + ])("fails closed for a %s team membership result", async (_name, result) => { + const client = createGitHubTeamMembershipClient(createHarness(result).auth); + + assert.equal( + await client.isActiveMember({ + installationId: 42, + organization: "getpaseo", + teamSlug: "maintainers", + username: "michael", + }), + false, + ); + }); +}); + +function createHarness(result: unknown) { + const requests: Array<{ route: string; parameters: Record }> = []; + const auth = { + createInstallationOctokit: async () => ({ + request: async (route: string, parameters: Record) => { + requests.push({ route, parameters }); + if (result instanceof Error) throw result; + return { data: result }; + }, + }), + } satisfies GitHubTeamMembershipAuth; + return { auth, requests }; +} + +function httpError(status: number): Error & { status: number } { + return Object.assign(new Error(`GitHub returned ${status}`), { status }); +} diff --git a/src/triggers/github/team-membership.ts b/src/triggers/github/team-membership.ts new file mode 100644 index 00000000..11f19b20 --- /dev/null +++ b/src/triggers/github/team-membership.ts @@ -0,0 +1,76 @@ +import { GITHUB_APP_PERMISSION_VOCABULARY } from "../../config/github-authority.js"; +import { logger } from "../../logger.js"; +import { z } from "zod"; + +const TeamMembershipSchema = z.object({ state: z.enum(["active", "pending"]) }).passthrough(); + +export interface GitHubTeamMembershipClient { + isActiveMember(input: GitHubTeamMembershipCheck): Promise; +} + +export interface GitHubTeamMembershipCheck { + installationId: number; + organization: string; + teamSlug: string; + username: string; +} + +export interface GitHubTeamMembershipAuth { + createInstallationOctokit(installationId: number): Promise; +} + +interface GitHubTeamMembershipOctokit { + request( + route: "GET /orgs/{org}/teams/{team_slug}/memberships/{username}", + parameters: { + org: string; + team_slug: string; + username: string; + headers: { "x-github-api-version": string }; + }, + ): Promise<{ data: unknown }>; +} + +export function createGitHubTeamMembershipClient( + auth: GitHubTeamMembershipAuth, +): GitHubTeamMembershipClient { + return { + async isActiveMember(input) { + try { + const octokit = await auth.createInstallationOctokit(input.installationId); + const response = await octokit.request( + "GET /orgs/{org}/teams/{team_slug}/memberships/{username}", + { + org: input.organization, + team_slug: input.teamSlug, + username: input.username, + headers: { + "x-github-api-version": GITHUB_APP_PERMISSION_VOCABULARY.apiVersion, + }, + }, + ); + return TeamMembershipSchema.parse(response.data).state === "active"; + } catch (error) { + if (hasHttpStatus(error, 404)) return false; + logger.warn( + { + err: error, + installationId: input.installationId, + organization: input.organization, + teamSlug: input.teamSlug, + username: input.username, + }, + hasHttpStatus(error, 403) + ? "GitHub team trigger filter access was denied; denying trigger" + : "GitHub team trigger filter check failed; denying trigger", + ); + return false; + } + }, + }; +} + +function hasHttpStatus(error: unknown, status: number): boolean { + if (error === null || typeof error !== "object") return false; + return Reflect.get(error, "status") === status; +} diff --git a/src/workflows/reactions.test.ts b/src/workflows/reactions.test.ts index 0a8f248f..5d673e6c 100644 --- a/src/workflows/reactions.test.ts +++ b/src/workflows/reactions.test.ts @@ -20,6 +20,7 @@ import type { DiscordBotClient } from "../triggers/discord/bot.js"; import { createDiscordTriggerProvider } from "../triggers/discord/provider.js"; import type { GitHubReactionClient } from "../triggers/github/provider.js"; import { createGitHubTriggerProvider } from "../triggers/github/provider.js"; +import type { GitHubTeamMembershipClient } from "../triggers/github/team-membership.js"; import type { SlackBotClient } from "../triggers/slack/client.js"; import { createSlackTriggerProvider } from "../triggers/slack/provider.js"; import type { TriggerProvider } from "../triggers/index.js"; @@ -207,6 +208,7 @@ function createGitHubReactionFixture() { configurationStoreForProject: () => new ProjectConfigurationStore(createMemoryDatabase(), "unused"), reactions, + teamMemberships: new DenyingGitHubTeamMemberships(), }); return { provider, @@ -225,6 +227,12 @@ function createGitHubReactionFixture() { }; } +class DenyingGitHubTeamMemberships implements GitHubTeamMembershipClient { + async isActiveMember(): Promise { + return false; + } +} + type WorkflowOutcome = "step_failure" | "timeout" | "prelaunch_failure"; interface WorkflowScenarioOptions {