From dc28f851827935013d93c1cc31a1382053197176 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 03:31:09 -0700 Subject: [PATCH] fix(review): derive four hardcoded gittensory literals from their config (#4615) Small, independent hardcoding cleanups found by the review-stack architecture audit: isOwnReviewThreadAuthor now derives its own-bot match from GITHUB_APP_SLUG instead of a literal "gittensory" regex, matching every other self-authorship check in the codebase; isProtectedAutomationAuthor gains an additive PROTECTED_AUTOCLOSE_AUTHORS_EXTRA env override for a self-hoster running a different automation stack; the Orb OAuth landing page's dashboard link now follows PUBLIC_SITE_ORIGIN (falling back to the public dashboard), mirroring maintainerControlPanelUrl one file-family over; and the OAuth returnTo allowlist drops a redundant hardcoded cloud origin entry that was dead weight once a self-hoster sets their own PUBLIC_SITE_ORIGIN. --- src/auth/github-oauth.ts | 5 ++++- src/env.d.ts | 4 ++++ src/github/backfill.ts | 24 +++++++++++++++------- src/orb/oauth.ts | 29 ++++++++++++++++---------- src/settings/agent-actions.ts | 16 +++++++++++++-- test/integration/orb-oauth.test.ts | 18 ++++++++++++++++ test/unit/agent-actions.test.ts | 21 +++++++++++++++++++ test/unit/auth.test.ts | 21 +++++++++++++++++++ test/unit/backfill.test.ts | 33 +++++++++++++++++++++++++----- 9 files changed, 145 insertions(+), 26 deletions(-) diff --git a/src/auth/github-oauth.ts b/src/auth/github-oauth.ts index 283842d37f..ed37eebd18 100644 --- a/src/auth/github-oauth.ts +++ b/src/auth/github-oauth.ts @@ -241,9 +241,12 @@ function normalizeReturnTo(env: Env, value: string | undefined): string { if (!value) return fallback; try { const url = new URL(value, siteOrigin); + // siteOrigin already IS "https://gittensory.aethereal.dev" when PUBLIC_SITE_ORIGIN is unset (the fallback + // two lines up), so a separate hardcoded entry here was dead weight once a self-hoster sets their own + // PUBLIC_SITE_ORIGIN -- it kept accepting the cloud origin as a valid redirect target even for a self-host + // instance that never uses it (#4615). Rely solely on siteOrigin. const allowedOrigins = new Set([ siteOrigin.replace(/\/$/, ""), - "https://gittensory.aethereal.dev", "http://localhost:3000", "http://localhost:4173", "http://localhost:5173", diff --git a/src/env.d.ts b/src/env.d.ts index caeabe922d..9e8dabfdba 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -177,6 +177,10 @@ declare global { SCORING_TIME_DECAY_ENABLED?: string; /** #776 agent-layer GLOBAL kill-switch — when truthy, halts ALL agent actions across every repo. */ AGENT_ACTIONS_PAUSED?: string; + /** #4615: comma-separated logins ADDED to the built-in auto-close protection allowlist + * (github-actions[bot]/dependabot[bot]/renovate[bot] — settings/agent-actions.ts), for a self-hoster + * running a different automation stack (e.g. mergify[bot], snyk-bot). Additive only. */ + PROTECTED_AUTOCLOSE_AUTHORS_EXTRA?: string; /** Self-host instance-wide write switch: "dry-run" | "disabled" forces EVERY installation write to be * suppressed regardless of per-repo mode (the cloud→self-host parallel-run kill switch). Unset = live. */ SELFHOST_DEPLOYMENT_MODE?: string; diff --git a/src/github/backfill.ts b/src/github/backfill.ts index fb05163367..b568e85385 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3799,7 +3799,7 @@ async function isAuthorizedReviewThreadAuthor( login: string | null | undefined, association: string | null | undefined, ): Promise { - if (isOwnReviewThreadAuthor(login)) return false; + if (isOwnReviewThreadAuthor(env, login)) return false; if (isTrustedScannerReviewThreadAuthor(env, login)) return true; if (isMaintainerReviewThreadAuthor(association)) return true; return isVerifiedMemberReviewThreadAuthor(env, repoFullName, token, memberPermissionCache, admissionKey, login, association); @@ -3862,12 +3862,22 @@ function isTrustedScannerReviewThreadAuthor(env: Env, login: string | null | und .includes(normalized); } -// Match only OUR OWN app bot login (a `gittensory` / `gittensory-orb[bot]` PREFIX), never a third-party slug -// that merely ENDS in `-gittensory[bot]`. Anchored to `^`: a `\b` boundary also fires after a hyphen, so the -// prior `\bgittensory…` misclassified e.g. `evil-gittensory[bot]` as our own author and dropped its -// review-thread comment as a self-authored non-blocker (fail-open) instead of evaluating it as external. -export function isOwnReviewThreadAuthor(login: string | null | undefined): boolean { - return /^gittensory[-\w]*\[bot\]$/i.test(login ?? "") || /^(gittensory|gittensory-orb)$/i.test(login ?? ""); +// Match only OUR OWN app bot login (a `${GITHUB_APP_SLUG}` / `${GITHUB_APP_SLUG}-orb[bot]` PREFIX), never a +// third-party slug that merely ENDS in `-${GITHUB_APP_SLUG}[bot]`. Anchored to `^`: a `\b` boundary also fires +// after a hyphen, so the prior `\bgittensory…` misclassified e.g. `evil-gittensory[bot]` as our own author and +// dropped its review-thread comment as a self-authored non-blocker (fail-open) instead of evaluating it as +// external. Derived from `env.GITHUB_APP_SLUG` (#4615) rather than a hardcoded "gittensory" literal -- every +// other "is this our own bot" check in the codebase already does this (self-authored.ts, pr-actions.ts, +// comments.ts, processors.ts) -- so a self-hoster who renamed their App still recognizes its own comments. +export function isOwnReviewThreadAuthor(env: Env, login: string | null | undefined): boolean { + const slug = env.GITHUB_APP_SLUG.trim().toLowerCase(); + if (!slug) return false; + const escapedSlug = escapeRegExpForOwnAuthorSlug(slug); + return new RegExp(`^${escapedSlug}[-\\w]*\\[bot\\]$`, "i").test(login ?? "") || new RegExp(`^(${escapedSlug}|${escapedSlug}-orb)$`, "i").test(login ?? ""); +} + +function escapeRegExpForOwnAuthorSlug(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } /** The deterministic linked-issue facts the hard-rule evaluator needs (labels / assignees / open-state), plus diff --git a/src/orb/oauth.ts b/src/orb/oauth.ts index 1fba42ad53..a1746482de 100644 --- a/src/orb/oauth.ts +++ b/src/orb/oauth.ts @@ -14,6 +14,7 @@ // surface). import type { Context } from "hono"; import { timeoutFetch } from "../github/client"; +import { GITTENSORY_SITE_URL } from "../github/footer"; import { isOrbBrokerEnabled, issueOrbEnrollment } from "./broker"; type GitHubUser = { login: string; id?: number }; @@ -68,20 +69,20 @@ export async function verifyInstallationAdmin( async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, installationId: number): Promise { const token = await exchangeOrbOAuthCode(c.env, code); - if (!token) return c.html(landingPage("Couldn't verify your GitHub identity", "The authorization didn't complete — re-run the install from GitHub and try again."), 400); + if (!token) return c.html(landingPage(c.env, "Couldn't verify your GitHub identity", "The authorization didn't complete — re-run the install from GitHub and try again."), 400); const user = await fetchOrbOAuthUser(token); - if (!user) return c.html(landingPage("Couldn't verify your GitHub identity", "We couldn't read your GitHub account — try the install again."), 400); + if (!user) return c.html(landingPage(c.env, "Couldn't verify your GitHub identity", "We couldn't read your GitHub account — try the install again."), 400); const install = await c.env.DB.prepare("SELECT account_login, account_type, account_id, registered, self_enrollment_disabled, suspended_at, removed_at FROM orb_github_installations WHERE installation_id = ?") .bind(installationId) .first<{ account_login: string | null; account_type: string | null; account_id: number | null; registered: number; self_enrollment_disabled: number; suspended_at: string | null; removed_at: string | null }>(); - if (!install) return c.html(landingPage("Installation not recognized", "We haven't recorded this installation yet — give it a moment after installing, then retry."), 404); + if (!install) return c.html(landingPage(c.env, "Installation not recognized", "We haven't recorded this installation yet — give it a moment after installing, then retry."), 404); // The admin-of-installation check is the authorization gate — it runs BEFORE we reveal or change any state, so a // non-admin learns nothing about the install and can never enroll someone else's. It binds to the immutable // GitHub account id (logins can be renamed/reused), so a stale account_login can never grant access. const isAdmin = await verifyInstallationAdmin(token, user.login, user.id, install.account_login, install.account_type, install.account_id); - if (!isAdmin) return c.html(landingPage("Admin access required", "You must be an admin of this installation's account to enroll it for self-host."), 403); - if (install.removed_at !== null || install.suspended_at !== null) return c.html(landingPage("Installation not active", "This installation is suspended or uninstalled — re-install the Orb App, then retry."), 403); - if (install.self_enrollment_disabled === 1) return c.html(landingPage("Installation disabled", "This installation was disabled by the operator — contact the operator to re-enable self-host enrollment."), 403); + if (!isAdmin) return c.html(landingPage(c.env, "Admin access required", "You must be an admin of this installation's account to enroll it for self-host."), 403); + if (install.removed_at !== null || install.suspended_at !== null) return c.html(landingPage(c.env, "Installation not active", "This installation is suspended or uninstalled — re-install the Orb App, then retry."), 403); + if (install.self_enrollment_disabled === 1) return c.html(landingPage(c.env, "Installation disabled", "This installation was disabled by the operator — contact the operator to re-enable self-host enrollment."), 403); // Zero-touch self-service: a verified admin of an ACTIVE, non-disabled install self-registers it (registered=1). // installation_id stays bound server-side in the enrollment, so brokered tokens remain scoped to this install. if (install.registered !== 1) { @@ -90,7 +91,7 @@ async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, const result = await issueOrbEnrollment(c.env, installationId, { login: user.login, githubId: user.id ?? null }); /* v8 ignore next -- defensive: the existence + admin + active checks above passed and we just set registered=1, so issueOrbEnrollment (which re-checks existence + registered) cannot return an error here; kept to degrade safely. */ - if ("error" in result) return c.html(landingPage("Couldn't issue an enrollment", "Please retry, or contact the operator."), 409); + if ("error" in result) return c.html(landingPage(c.env, "Couldn't issue an enrollment", "Please retry, or contact the operator."), 409); return c.html(secretPage(result.secret)); } @@ -104,8 +105,8 @@ export async function handleOrbOAuthCallback(c: Context<{ Bindings: Env }>): Pro const updated = c.req.query("setup_action") === "update"; return c.html( updated - ? landingPage("Gittensory Orb updated", "Your repository selection was updated — the dashboard reflects the change shortly.") - : landingPage("Gittensory Orb connected", "Your repositories are linked. Their review activity now flows to the global Gittensory dashboard."), + ? landingPage(c.env, "Gittensory Orb updated", "Your repository selection was updated — the dashboard reflects the change shortly.") + : landingPage(c.env, "Gittensory Orb connected", "Your repositories are linked. Their review activity now flows to the global Gittensory dashboard."), ); } @@ -113,8 +114,14 @@ function shell(heading: string, inner: string): string { return `${heading}

${heading}

${inner}
`; } -function landingPage(heading: string, message: string): string { - return shell(heading, `

${message}

Open the dashboard`); +// The Orb App itself stays a single centrally-hosted hub (broker.ts: gittensory holds the App key centrally +// and mints tokens on demand) -- that part is architecturally fixed. This link is just where the browser lands +// after OAuth, so it follows the SAME self-hoster-configurable pattern as maintainerControlPanelUrl one +// file-family over (github/footer.ts): env.PUBLIC_SITE_ORIGIN when set, else the public gittensory dashboard +// (#4615). +function landingPage(env: Env, heading: string, message: string): string { + const dashboardOrigin = (env.PUBLIC_SITE_ORIGIN ?? GITTENSORY_SITE_URL).replace(/\/$/, ""); + return shell(heading, `

${message}

Open the dashboard`); } /** Show the freshly-issued enrollment secret ONCE. The secret is a generated opaque token (no user input), safe diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index e2465e0a90..240b463e03 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -57,8 +57,20 @@ export const AGENT_LABEL_MIGRATION_COLLISION = "migration-collision"; // or slop heuristic — the maintainer owns its lifecycle. (reviewbot wrongly auto-closed such an accumulator, // awesome-claude #4192.) Still eligible for auto-merge when clean + passing. const PROTECTED_AUTOCLOSE_AUTHORS = new Set(["github-actions[bot]", "dependabot[bot]", "renovate[bot]"]); -export function isProtectedAutomationAuthor(login: string | null | undefined): boolean { - return login != null && PROTECTED_AUTOCLOSE_AUTHORS.has(login.toLowerCase()); + +// A self-hoster running a different automation stack (mergify[bot], snyk-bot, allcontributors[bot], ...) can +// extend the base allowlist with a comma-separated PROTECTED_AUTOCLOSE_AUTHORS_EXTRA env var instead of +// forking the code (#4615). Additive only -- an unset/blank value never shrinks the base set below. +function protectedAutocloseAuthors(env: Env | undefined): Set { + const extra = env?.PROTECTED_AUTOCLOSE_AUTHORS_EXTRA + ?.split(",") + .map((login) => login.trim().toLowerCase()) + .filter(Boolean); + return extra && extra.length > 0 ? new Set([...PROTECTED_AUTOCLOSE_AUTHORS, ...extra]) : PROTECTED_AUTOCLOSE_AUTHORS; +} + +export function isProtectedAutomationAuthor(login: string | null | undefined, env?: Env): boolean { + return login != null && protectedAutocloseAuthors(env).has(login.toLowerCase()); } export type PlannedAgentAction = { diff --git a/test/integration/orb-oauth.test.ts b/test/integration/orb-oauth.test.ts index a3ba1b359d..8600f09c47 100644 --- a/test/integration/orb-oauth.test.ts +++ b/test/integration/orb-oauth.test.ts @@ -34,6 +34,24 @@ describe("GET /v1/orb/oauth/callback (post-install landing)", () => { const res = await app.request("/v1/orb/ingest", { method: "POST" }, createTestEnv()); expect([400, 413]).toContain(res.status); // reached the (exempt) ingest handler, failed only on the empty body }); + + it("the dashboard link follows a self-hoster's PUBLIC_SITE_ORIGIN, not a hardcoded gittensory.aethereal.dev (#4615)", async () => { + const res = await app.request( + "/v1/orb/oauth/callback", + {}, + createTestEnv({ PUBLIC_SITE_ORIGIN: "https://my-instance.example.com/" }), + ); + const html = await res.text(); + expect(html).toContain('href="https://my-instance.example.com"'); // trailing slash stripped, matching footer.ts's siteOrigin normalization + expect(html).not.toContain("gittensory.aethereal.dev"); + }); + + it("falls back to the public gittensory dashboard when PUBLIC_SITE_ORIGIN is unset (cloud default)", async () => { + const env = createTestEnv(); + delete (env as Partial).PUBLIC_SITE_ORIGIN; + const res = await app.request("/v1/orb/oauth/callback", {}, env); + expect(await res.text()).toContain('href="https://gittensory.aethereal.dev"'); + }); }); describe("verifyInstallationAdmin (the privilege-escalation gate)", () => { diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 61d31c1b8b..072fb56387 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -9,6 +9,7 @@ import type { GateCheckConclusion } from "../../src/rules/advisory"; // actually manifest in this test file's own module graph, not just incidentally in other suites. Importing // agent-actions.ts alone (above) never exercises the OTHER direction of the cycle -- this import does. import { DEFAULT_ISSUE_DISCOVERY_SHARE } from "../../src/scoring/model"; +import { createTestEnv } from "../helpers/d1"; function input(overrides: Partial & { conclusion: GateCheckConclusion }): AgentActionPlanInput { return { @@ -1454,6 +1455,26 @@ describe("isProtectedAutomationAuthor", () => { expect(isProtectedAutomationAuthor(null)).toBe(false); expect(isProtectedAutomationAuthor(undefined)).toBe(false); }); + + it("extends the base allowlist via PROTECTED_AUTOCLOSE_AUTHORS_EXTRA (#4615), additively", () => { + const env = createTestEnv({ PROTECTED_AUTOCLOSE_AUTHORS_EXTRA: "mergify[bot], Snyk-Bot" }); + expect(isProtectedAutomationAuthor("mergify[bot]", env)).toBe(true); + expect(isProtectedAutomationAuthor("snyk-bot", env)).toBe(true); // case-insensitive + expect(isProtectedAutomationAuthor("github-actions[bot]", env)).toBe(true); // base set still honored + expect(isProtectedAutomationAuthor("some-other-bot[bot]", env)).toBe(false); + }); + + it("ignores a blank/whitespace-only PROTECTED_AUTOCLOSE_AUTHORS_EXTRA (never shrinks the base set)", () => { + const blank = createTestEnv({ PROTECTED_AUTOCLOSE_AUTHORS_EXTRA: " , ," }); + expect(isProtectedAutomationAuthor("github-actions[bot]", blank)).toBe(true); + expect(isProtectedAutomationAuthor("mergify[bot]", blank)).toBe(false); + }); + + it("behaves exactly as before when no env (or no override) is passed", () => { + expect(isProtectedAutomationAuthor("github-actions[bot]", createTestEnv())).toBe(true); + expect(isProtectedAutomationAuthor("mergify[bot]", createTestEnv())).toBe(false); + expect(isProtectedAutomationAuthor("github-actions[bot]", undefined)).toBe(true); + }); }); describe("downgradeMergeToHold — accuracy circuit-breaker (#self-improve / GAP-4)", () => { diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index 6c3ed14757..daebb66309 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -689,6 +689,27 @@ describe("private-beta auth and rate limiting", () => { await expect(createSessionFromGitHubToken(env, "github-token")).resolves.toMatchObject({ login: "jsonbored", scopes: [] }); }); + it("a self-hoster's PUBLIC_SITE_ORIGIN is the sole allowed returnTo origin -- the cloud origin is no longer a bonus entry (#4615)", async () => { + const env = createTestEnv({ + GITHUB_OAUTH_CLIENT_ID: "client-id", + GITHUB_OAUTH_CLIENT_SECRET: "client-secret", + PUBLIC_SITE_ORIGIN: "https://my-instance.example.com", + }); + + // The self-hoster's own origin is accepted. + const own = await startGitHubWebOAuth(env, "https://my-instance.example.com/v1/auth/github/start", "https://my-instance.example.com/app/workbench"); + expect(own.returnTo).toBe("https://my-instance.example.com/app/workbench"); + + // The cloud gittensory.aethereal.dev origin is no longer accepted for THIS instance -- it falls back to + // this instance's own /app, proving the dead literal is gone rather than merely redundant. + const cloud = await startGitHubWebOAuth(env, "https://my-instance.example.com/v1/auth/github/start", "https://gittensory.aethereal.dev/app/workbench"); + expect(cloud.returnTo).toBe("https://my-instance.example.com/app"); + + // Localhost dev origins remain accepted regardless of PUBLIC_SITE_ORIGIN. + const local = await startGitHubWebOAuth(env, "https://my-instance.example.com/v1/auth/github/start", "http://localhost:5173/app"); + expect(local.returnTo).toBe("http://localhost:5173/app"); + }); + it("verifies the GitHub token audience before minting a session on the token-exchange path", async () => { const env = createTestEnv({ GITHUB_OAUTH_CLIENT_ID: "client-id", GITHUB_OAUTH_CLIENT_SECRET: "client-secret" }); const introspect = (appClientId: string | undefined) => diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 7ca54bbef8..1c3b12f2db 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -6764,9 +6764,11 @@ function githubTotalsResponse(counts: { openIssues: number; openPullRequests: nu } describe("isOwnReviewThreadAuthor", () => { + const env = createTestEnv(); // GITHUB_APP_SLUG defaults to "gittensory" (test/helpers/d1.ts) + it("matches our own gittensory app bot logins by prefix", () => { for (const login of ["gittensory[bot]", "gittensory-orb[bot]", "gittensory-review[bot]", "GITTENSORY[bot]", "gittensory", "gittensory-orb"]) { - expect(isOwnReviewThreadAuthor(login)).toBe(true); + expect(isOwnReviewThreadAuthor(env, login)).toBe(true); } }); @@ -6774,13 +6776,34 @@ describe("isOwnReviewThreadAuthor", () => { // A `\b` boundary also fires after a hyphen, so the unanchored regex misclassified these external bots as // our own author and dropped their review-thread comments as self-authored non-blockers (fail-open). for (const login of ["evil-gittensory[bot]", "x-gittensory[bot]", "not-gittensory", "gittensory-fork"]) { - expect(isOwnReviewThreadAuthor(login)).toBe(false); + expect(isOwnReviewThreadAuthor(env, login)).toBe(false); } }); it("treats an absent login as not our own author", () => { - expect(isOwnReviewThreadAuthor(null)).toBe(false); - expect(isOwnReviewThreadAuthor(undefined)).toBe(false); - expect(isOwnReviewThreadAuthor("")).toBe(false); + expect(isOwnReviewThreadAuthor(env, null)).toBe(false); + expect(isOwnReviewThreadAuthor(env, undefined)).toBe(false); + expect(isOwnReviewThreadAuthor(env, "")).toBe(false); + }); + + it("derives the match from GITHUB_APP_SLUG (#4615), not a hardcoded literal", () => { + const renamed = createTestEnv({ GITHUB_APP_SLUG: "acme-review" }); + expect(isOwnReviewThreadAuthor(renamed, "acme-review[bot]")).toBe(true); + expect(isOwnReviewThreadAuthor(renamed, "acme-review-orb[bot]")).toBe(true); + expect(isOwnReviewThreadAuthor(renamed, "acme-review")).toBe(true); + // The OLD slug no longer matches once an operator renames their App -- proves the literal is gone. + expect(isOwnReviewThreadAuthor(renamed, "gittensory[bot]")).toBe(false); + }); + + it("a slug containing regex metacharacters is escaped, not interpreted (defensive)", () => { + const weird = createTestEnv({ GITHUB_APP_SLUG: "acme.bot" }); + expect(isOwnReviewThreadAuthor(weird, "acme.bot[bot]")).toBe(true); + expect(isOwnReviewThreadAuthor(weird, "acmexbot[bot]")).toBe(false); // "." must not act as a wildcard + }); + + it("fails closed when GITHUB_APP_SLUG is blank (misconfiguration)", () => { + const blank = createTestEnv({ GITHUB_APP_SLUG: "" }); + expect(isOwnReviewThreadAuthor(blank, "gittensory[bot]")).toBe(false); + expect(isOwnReviewThreadAuthor(blank, "")).toBe(false); }); });