diff --git a/src/auth/security.ts b/src/auth/security.ts index 8f2b2d14fb..6808ce877d 100644 --- a/src/auth/security.ts +++ b/src/auth/security.ts @@ -141,21 +141,53 @@ export function parseGitHubLoginList(value: string | undefined): Set { ); } -/** Is `repoFullName` within the operator's MCP_ACTUATION_REPO_ALLOWLIST? The static `mcp` identity is minted from - * a single shared secret (GITTENSORY_MCP_TOKEN) that is documented as an ordinary end-user CLI credential — unlike - * `api`/`internal`, it is not operator-only, so unlike those it must NOT be unconditionally trusted for every - * installed repo. Unset/empty ⇒ deny (fail closed: an operator must explicitly opt a repo in). `*`/`all` ⇒ every - * repo, an explicit escape hatch for an operator who wants the old unscoped-trust behavior. (#2253) */ -export function isMcpActuationRepoAllowed(value: string | undefined, repoFullName: string): boolean { - const entries = (value ?? "") +/** Shared CSV/whitespace allowlist parse for the MCP repo-allowlist env vars — both the actuation (write) and + * read allowlists use the identical fail-closed/wildcard parsing, just gate a different security boundary at + * their respective call sites. */ +function parseMcpRepoAllowlistEntries(value: string | undefined): string[] { + return (value ?? "") .split(/[\s,]+/) .map((entry) => entry.trim().toLowerCase()) .filter(Boolean); +} + +/** Does an allowlist value grant `repoFullName`? Unset/empty ⇒ deny (fail closed). `*`/`all` ⇒ every repo, an + * explicit escape hatch for an operator who wants unscoped trust. */ +function matchesMcpRepoAllowlist(value: string | undefined, repoFullName: string): boolean { + const entries = parseMcpRepoAllowlistEntries(value); if (entries.length === 0) return false; if (entries.includes("*") || entries.includes("all")) return true; return entries.includes(repoFullName.toLowerCase()); } +/** Is `repoFullName` within the operator's MCP_ACTUATION_REPO_ALLOWLIST? The static `mcp` identity is minted from + * a single shared secret (GITTENSORY_MCP_TOKEN) that is documented as an ordinary end-user CLI credential — unlike + * `api`/`internal`, it is not operator-only, so unlike those it must NOT be unconditionally trusted for every + * installed repo. Unset/empty ⇒ deny (fail closed: an operator must explicitly opt a repo in). `*`/`all` ⇒ every + * repo, an explicit escape hatch for an operator who wants the old unscoped-trust behavior. (#2253) */ +export function isMcpActuationRepoAllowed(value: string | undefined, repoFullName: string): boolean { + return matchesMcpRepoAllowlist(value, repoFullName); +} + +/** Is `repoFullName` within the operator's MCP_READ_REPO_ALLOWLIST? Same fail-closed/wildcard model as + * isMcpActuationRepoAllowed, kept as a SEPARATE allowlist so an operator can grant broad read access without + * also granting actuation (merge/close/approve) trust, or the reverse. Gates the static `mcp` identity's + * read-only MCP tools: repo context, issue quality, watch subscriptions, and (via isMcpReadUnscoped below) the + * non-repo-scoped contributor/operator tools. (#2455) */ +export function isMcpReadRepoAllowed(value: string | undefined, repoFullName: string): boolean { + return matchesMcpRepoAllowlist(value, repoFullName); +} + +/** Is MCP_READ_REPO_ALLOWLIST set to the full `*`/`all` wildcard? Contributor-login-scoped tools (another + * contributor's decision pack/profile/notifications) and operator-scoped tools (fleet analytics) have no single + * repo to check a scoped allowlist entry against, so — unlike the repo-scoped read tools above — they only + * unlock for the static `mcp` identity via the full wildcard opt-in: a repo-scoped allowlist does not imply a + * right to read an ARBITRARY other contributor's private data or cross-instance operator-only analytics. (#2455) */ +export function isMcpReadUnscoped(value: string | undefined): boolean { + const entries = parseMcpRepoAllowlistEntries(value); + return entries.includes("*") || entries.includes("all"); +} + type CookieOptions = { maxAge: number; path: string; diff --git a/src/env.d.ts b/src/env.d.ts index 9169af8c3b..97d5a0c465 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -144,6 +144,11 @@ declare global { * list, or `*`/`all` for every repo). Unset ⇒ none — GITTENSORY_MCP_TOKEN is a shared, end-user-obtainable * credential, so it must not implicitly actuate on every installed repo (#2253). */ MCP_ACTUATION_REPO_ALLOWLIST?: string; + /** Repos the shared GITTENSORY_MCP_TOKEN may READ via MCP tools (repo context, issue quality, watch + * subscriptions) — comma/whitespace `owner/repo` list, or `*`/`all` for every repo AND for the + * non-repo-scoped contributor/operator tools (another contributor's private data, fleet analytics). Unset + * ⇒ none. A separate allowlist from MCP_ACTUATION_REPO_ALLOWLIST so read and write trust can differ (#2455). */ + MCP_READ_REPO_ALLOWLIST?: string; /** Shared bearer secret required by the hosted Orb ingest collector. */ ORB_INGEST_TOKEN?: string; /** AES-256-GCM master secret for maintainer BYOK provider keys (encrypt/decrypt at rest). A Worker/self-host diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 265390021b..6d79c80aae 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -4,7 +4,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import type { RequestHandlerExtra } from "@modelcontextprotocol/sdk/shared/protocol.js"; import { ElicitResultSchema, type ServerNotification, type ServerRequest } from "@modelcontextprotocol/sdk/types.js"; import { z } from "zod"; -import { authenticatePrivateToken, extractBearerToken, isMcpActuationRepoAllowed, type AuthIdentity } from "../auth/security"; +import { authenticatePrivateToken, extractBearerToken, isMcpActuationRepoAllowed, isMcpReadRepoAllowed, isMcpReadUnscoped, type AuthIdentity } from "../auth/security"; import { canLoginAccessRepo, canWatchRepo, loadControlPanelAccessScope, loadControlPanelRoleSummary, type ControlPanelAccessScope } from "../services/control-panel-roles"; import { countOpenIssues, @@ -1770,6 +1770,13 @@ export class GittensoryMcp { if (this.identity.kind === "session" && this.identity.actor.toLowerCase() !== login.toLowerCase()) { throw new Error("Forbidden: session can only access the authenticated GitHub login."); } + // The static `mcp` identity must not read an ARBITRARY other contributor's private decision pack, profile, + // or notifications by default — GITTENSORY_MCP_TOKEN is a shared, end-user-obtainable CLI credential, not an + // operator-only secret (see requireRepoManageAccess). There is no per-login allowlist, so only the full + // MCP_READ_REPO_ALLOWLIST wildcard opt-in unlocks this, matching requireOperatorAccess below. (#2455) + if (this.identity.kind === "static" && this.identity.actor === "mcp" && !isMcpReadUnscoped(this.env.MCP_READ_REPO_ALLOWLIST)) { + throw new Error("Forbidden: this MCP token is not authorized to read another contributor's data."); + } } private async requireRepoAccess(repoFullName: string): Promise { @@ -1842,6 +1849,9 @@ export class GittensoryMcp { // Issue-watch gate (#699 path B). Sessions may only watch repos they can SEE: any gittensory-tracked PUBLIC // repo (the miner use case) or a PRIVATE repo they can access — never an arbitrary/private repo they cannot, // so private-repo issues never fan out to them. Non-session (private-token) identities are trusted. + // Its only caller (watchIssues) already gates the static `mcp` identity via requireContributorAccess's + // unscoped-MCP_READ_REPO_ALLOWLIST-wildcard-only check first, which is strictly stronger than any repo-scoped + // check this function could add — a static mcp caller can only ever reach here already fully trusted. (#2455) private async requireWatchableRepo(login: string, repoFullName: string): Promise { if (this.identity.kind !== "session") return; if (await canWatchRepo(this.env, login, repoFullName)) return; @@ -2031,8 +2041,15 @@ export class GittensoryMcp { } private async canAccessRepo(fullName: string): Promise { - if (this.identity.kind !== "session") return true; - return canLoginAccessRepo(this.env, this.identity.actor, fullName); + if (this.identity.kind === "session") return canLoginAccessRepo(this.env, this.identity.actor, fullName); + // The static `mcp` identity is a shared, end-user-obtainable CLI credential — scope it to the operator's + // MCP_READ_REPO_ALLOWLIST instead of trusting it for every installed repo, mirroring requireRepoManageAccess's + // MCP_ACTUATION_REPO_ALLOWLIST scoping for writes. api/internal static identities remain trusted (operator-only + // Worker secrets, never handed to end users). (#2455) + if (this.identity.kind === "static" && this.identity.actor === "mcp") { + return isMcpReadRepoAllowed(this.env.MCP_READ_REPO_ALLOWLIST, fullName); + } + return true; } private async getRepoOutcomePatterns(input: { owner: string; repo: string }): Promise { @@ -2065,12 +2082,19 @@ export class GittensoryMcp { } // Operator-only gate: the fleet view aggregates ALL self-hosters' calibration, so a session must be an - // operator; private-token / static identities are trusted (the same model as the other measurement tools). + // operator. api/internal static identities are trusted (operator-only Worker secrets). The static `mcp` + // identity is NOT trusted by default — it is a shared, end-user-obtainable CLI credential, and fleet analytics + // has no single repo to scope a MCP_READ_REPO_ALLOWLIST entry against, so only the full wildcard opt-in + // (mirroring requireContributorAccess) unlocks it. (#2455) private async requireOperatorAccess(): Promise { - if (this.identity.kind !== "session") return; - const scope = await this.loadSessionAccessScope(); - if (scope.operator) return; - throw new Error("Forbidden: operator authority is required for fleet analytics."); + if (this.identity.kind === "session") { + const scope = await this.loadSessionAccessScope(); + if (scope.operator) return; + throw new Error("Forbidden: operator authority is required for fleet analytics."); + } + if (this.identity.kind === "static" && this.identity.actor === "mcp" && !isMcpReadUnscoped(this.env.MCP_READ_REPO_ALLOWLIST)) { + throw new Error("Forbidden: this MCP token is not authorized for operator-only fleet analytics."); + } } private async getFleetAnalytics(input: { windowDays?: number | undefined }): Promise { diff --git a/test/helpers/d1.ts b/test/helpers/d1.ts index 5d6e3946b4..3ca21cd0b2 100644 --- a/test/helpers/d1.ts +++ b/test/helpers/d1.ts @@ -97,6 +97,7 @@ export function createTestEnv(overrides: Partial = {}): Env { GITHUB_APP_PRIVATE_KEY: "test-private-key", ADMIN_GITHUB_LOGINS: "jsonbored", MCP_ACTUATION_REPO_ALLOWLIST: "*", + MCP_READ_REPO_ALLOWLIST: "*", SELFHOST_TRANSIENT_CACHE: { async get(key: string) { return transientCache.get(key) ?? null; diff --git a/test/unit/issue-watch.test.ts b/test/unit/issue-watch.test.ts index 98bfb78594..1444a9c01c 100644 --- a/test/unit/issue-watch.test.ts +++ b/test/unit/issue-watch.test.ts @@ -216,4 +216,14 @@ describe("MCP gittensory_watch_issues", () => { expect(result.isError).toBe(true); expect(JSON.stringify(result.content)).toContain("authenticated GitHub login"); }); + + // Regression test for #2455: the shared, end-user-obtainable GITTENSORY_MCP_TOKEN must not manage an + // ARBITRARY login's watch subscriptions by default. "" overrides createTestEnv's own + // MCP_READ_REPO_ALLOWLIST: "*" default back to unset, exercising the real deny-by-default behavior. + it("forbids the static mcp identity without an MCP_READ_REPO_ALLOWLIST wildcard opt-in (#2455)", async () => { + const client = await connect(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "" })); + const result = await client.callTool({ name: "gittensory_watch_issues", arguments: { login: "miner", action: "list" } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toMatch(/not authorized to read another contributor's data/i); + }); }); diff --git a/test/unit/mcp-fleet-analytics.test.ts b/test/unit/mcp-fleet-analytics.test.ts index 3ac63d3796..96c29b60b1 100644 --- a/test/unit/mcp-fleet-analytics.test.ts +++ b/test/unit/mcp-fleet-analytics.test.ts @@ -59,4 +59,19 @@ describe("gittensory_get_fleet_analytics MCP tool", () => { expect(result.isError).toBeTruthy(); expect(JSON.stringify(result.content)).toMatch(/operator authority/i); }); + + // Regression test for #2455: the shared, end-user-obtainable GITTENSORY_MCP_TOKEN must not read + // cross-instance operator-only fleet analytics by default. "" overrides createTestEnv's own + // MCP_READ_REPO_ALLOWLIST: "*" default back to unset, exercising the real deny-by-default behavior. + it("forbids the static mcp identity without an MCP_READ_REPO_ALLOWLIST wildcard opt-in (#2455)", async () => { + const result = await (await connect(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "" }))).callTool({ name: "gittensory_get_fleet_analytics", arguments: {} }); + expect(result.isError).toBeTruthy(); + expect(JSON.stringify(result.content)).toMatch(/not authorized for operator-only fleet analytics/i); + }); + + it("forbids the static mcp identity when MCP_READ_REPO_ALLOWLIST is scoped to specific repos, not the wildcard (#2455)", async () => { + const result = await (await connect(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "acme/widgets" }))).callTool({ name: "gittensory_get_fleet_analytics", arguments: {} }); + expect(result.isError).toBeTruthy(); + expect(JSON.stringify(result.content)).toMatch(/not authorized for operator-only fleet analytics/i); + }); }); diff --git a/test/unit/mcp-output-schemas.test.ts b/test/unit/mcp-output-schemas.test.ts index 51be3e9645..b8895672bb 100644 --- a/test/unit/mcp-output-schemas.test.ts +++ b/test/unit/mcp-output-schemas.test.ts @@ -188,6 +188,33 @@ describe("MCP tool calls return schema-valid structured content", () => { expect(data.repoFullName).toBe("octo/demo"); }); + // Regression test for #2455: api/internal static identities are operator-only Worker secrets (never handed to + // end users, unlike the shared GITTENSORY_MCP_TOKEN), so canAccessRepo must remain unconditionally trusted for + // them even with MCP_READ_REPO_ALLOWLIST unset — mirroring the existing api/internal-trusted tests for the + // write-side MCP_ACTUATION_REPO_ALLOWLIST guards. + it("gittensory_get_repo_context trusts the api static identity unconditionally, regardless of MCP_READ_REPO_ALLOWLIST (#2455)", async () => { + const { client } = await connectTestClient(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "" }), { kind: "static", actor: "api" }); + const result = await client.callTool({ name: "gittensory_get_repo_context", arguments: { owner: "octo", repo: "demo" } }); + expect(result.isError).toBeFalsy(); + const data = result.structuredContent as Record; + expect(data.repoFullName).toBe("octo/demo"); + }); + + // Regression test for #2455: the shared, end-user-obtainable GITTENSORY_MCP_TOKEN must not read an arbitrary + // repo's context by default. + it("gittensory_get_repo_context forbids the static mcp identity without an MCP_READ_REPO_ALLOWLIST wildcard/scoped opt-in (#2455)", async () => { + const { client } = await connectTestClient(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "" })); + const result = await client.callTool({ name: "gittensory_get_repo_context", arguments: { owner: "octo", repo: "demo" } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toMatch(/cannot access this repository/i); + }); + + it("gittensory_get_repo_context allows the static mcp identity once the repo is explicitly allowlisted (#2455)", async () => { + const { client } = await connectTestClient(createTestEnv({ MCP_READ_REPO_ALLOWLIST: "octo/demo" })); + const result = await client.callTool({ name: "gittensory_get_repo_context", arguments: { owner: "octo", repo: "demo" } }); + expect(result.isError).toBeFalsy(); + }); + it("gittensory_get_maintainer_noise returns a structured noise triage report for a repo", async () => { const env = createTestEnv(); await upsertRepositoryFromGitHub(env, { name: "demo", full_name: "octo/demo", private: false, owner: { login: "octo" }, default_branch: "main" });