From 194d9a4177c57316c5f84ea80b617d673134360c Mon Sep 17 00:00:00 2001 From: pathors Date: Wed, 26 Aug 2026 02:31:02 +0800 Subject: [PATCH] [fix] sync hardening: cross-tenant repo mounts, prune wipes, forged webhooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent problems in the GitHub sync path, none reachable yet only because the App is not registered. Any member could mount another workspace's private repo /api/github/installations returned every installation of the App to any signed-in user, including the repo names inside each one, and POST /api/sources accepted whatever repo + installationId it was handed without checking either. Pairing an id seen in the first with any repo listed there mounted that repo — private included — into a workspace of your own. Nothing binds an installation to a workspace, and adding that binding is a schema change. Instead an installation is now claimed first-come by the workspace that first mounts through it: installationOwner() rejects a second claimant, installationCanSee() rejects a repo the installation cannot read, and the installations list is filtered to what POST would actually accept, so the picker stops leaking other tenants' repo names. Worth replacing with a real org<->installation column later. A sync that matched nothing deleted everything The prune deleted every row the source owned when the sync produced no files (`seen.length ? notInArray(...) : sql`true``). A mistyped folder, a branch with no .html, or a truncated tree therefore wiped the whole mount. An empty result is now treated as the misconfiguration it almost always is: the prune is skipped and reported as prunedSkipped. The truncated tree deserved its own fix — GitHub sets `truncated` on large repos and the flag was never read, so a partial listing fed a prune that deletes whatever the listing omits. It now throws. An unset webhook secret accepted forged pushes verifyWebhook fell back to `?? ""`, computing the HMAC with an empty key — a signature anyone can reproduce. It now refuses outright when the secret is missing, and compares via subtle.verify rather than string equality. --- app/api/github/installations/route.ts | 30 ++++++-- app/api/sources/route.ts | 18 ++++- app/o/[slug]/settings/settings-client.tsx | 2 +- lib/github.ts | 91 +++++++++++++++++------ 4 files changed, 108 insertions(+), 33 deletions(-) diff --git a/app/api/github/installations/route.ts b/app/api/github/installations/route.ts index 4d0cbde..14650a4 100644 --- a/app/api/github/installations/route.ts +++ b/app/api/github/installations/route.ts @@ -1,17 +1,31 @@ import { NextResponse } from "next/server"; -import { headers } from "next/headers"; -import { auth } from "@/lib/auth"; +import { eq } from "drizzle-orm"; +import { db } from "@/lib/db"; +import * as schema from "@/lib/schema"; +import { requireMember } from "@/lib/access"; import { appJwt } from "@/lib/github"; // Lists the GitHub App's installations and their repos, so the settings UI can // offer a repo picker instead of hand-typed ids. -// NOTE: session-gated but app-wide — fine for a self-hosted/team deployment, -// too broad for an open multi-tenant SaaS (see README trust model). -export async function GET() { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session) return NextResponse.json({ error: "forbidden" }, { status: 403 }); +// +// Scoped to one workspace: the App's installation list is global, and the repo +// names inside it are the private inventory of whoever installed it. Only +// installations this workspace already uses, or that no workspace has claimed +// yet, are returned — matching what POST /api/sources will actually accept. +export async function GET(req: Request) { + const slug = new URL(req.url).searchParams.get("org") ?? ""; + const access = await requireMember(slug); + if (!access) return NextResponse.json({ error: "forbidden" }, { status: 403 }); if (!process.env.GITHUB_APP_ID) return NextResponse.json({ installations: [], appMissing: true }); + const claimed = new Map(); + for (const r of await db + .select({ installationId: schema.syncSource.installationId, + organizationId: schema.syncSource.organizationId }) + .from(schema.syncSource)) { + if (r.installationId) claimed.set(r.installationId, r.organizationId); + } + const jwt = await appJwt(); const gh = { Accept: "application/vnd.github+json", "User-Agent": "pensieve" }; const appRes = await fetch("https://api.github.com/app", { headers: { ...gh, Authorization: `Bearer ${jwt}` } }); @@ -23,6 +37,8 @@ export async function GET() { const out = []; for (const inst of insts) { + const claimant = claimed.get(String(inst.id)); + if (claimant && claimant !== access.org.id) continue; const tok = await fetch(`https://api.github.com/app/installations/${inst.id}/access_tokens`, { method: "POST", headers: { ...gh, Authorization: `Bearer ${jwt}` } }); if (!tok.ok) continue; diff --git a/app/api/sources/route.ts b/app/api/sources/route.ts index a4691be..0e43e48 100644 --- a/app/api/sources/route.ts +++ b/app/api/sources/route.ts @@ -3,7 +3,7 @@ import { eq } from "drizzle-orm"; import { db } from "@/lib/db"; import * as schema from "@/lib/schema"; import { requireMember } from "@/lib/access"; -import { syncGithubSource } from "@/lib/github"; +import { syncGithubSource, installationOwner, installationCanSee } from "@/lib/github"; export async function GET(req: Request) { const slug = new URL(req.url).searchParams.get("org") ?? ""; @@ -58,11 +58,25 @@ export async function POST(req: Request) { } } + // create. repo and installationId arrive from the client, so neither can be + // trusted: /api/github/installations can see every installation of the App, + // and without these checks any member could mount another workspace's private + // repo into their own by pairing its id with that installation. + if (!body.repo || !body.installationId) + return NextResponse.json({ error: "repo and installationId are required" }, { status: 400 }); + + const owner = await installationOwner(body.installationId); + if (owner && owner !== access.org.id) + return NextResponse.json({ error: "that installation is in use by another workspace" }, { status: 403 }); + + if (!(await installationCanSee(body.installationId, body.repo))) + return NextResponse.json({ error: "that installation cannot access that repo" }, { status: 403 }); + const id = crypto.randomUUID(); await db.insert(schema.syncSource).values({ id, organizationId: access.org.id, type: "github", repo: body.repo, branch: body.branch || "main", folder: body.folder || "", - mount: body.mount || "/", installationId: body.installationId || null, + mount: body.mount || "/", installationId: body.installationId, }); return NextResponse.json({ ok: true, id }); } diff --git a/app/o/[slug]/settings/settings-client.tsx b/app/o/[slug]/settings/settings-client.tsx index 41cc4b3..1e8750c 100644 --- a/app/o/[slug]/settings/settings-client.tsx +++ b/app/o/[slug]/settings/settings-client.tsx @@ -65,7 +65,7 @@ export function SettingsClient({ slug, orgName }: Readonly<{ slug: string; orgNa const load = () => { fetch(`/api/sources?org=${slug}`).then((r) => r.json()).then((d) => setSources(d.sources ?? [])); - fetch("/api/github/installations").then((r) => r.json()).then((d) => { + fetch(`/api/github/installations?org=${encodeURIComponent(slug)}`).then((r) => r.json()).then((d) => { setInsts(d.installations ?? []); setAppMissing(!!d.appMissing); setAppSlug(d.appSlug ?? null); }); }; diff --git a/lib/github.ts b/lib/github.ts index 08d1023..8f35492 100644 --- a/lib/github.ts +++ b/lib/github.ts @@ -46,7 +46,12 @@ export async function syncGithubSource(source: typeof schema.syncSource.$inferSe const branch = source.branch || "main"; const treeRes = await gh(`https://api.github.com/repos/${source.repo}/git/trees/${branch}?recursive=1`); if (!treeRes.ok) throw new Error(`tree: ${treeRes.status}`); - const tree = (await treeRes.json() as { tree: { path: string; type: string; sha: string }[] }).tree; + const treeBody = await treeRes.json() as { tree: { path: string; type: string; sha: string }[]; truncated?: boolean }; + // A truncated listing is a PARTIAL view of the repo, and the prune below + // deletes whatever this listing does not mention — so continuing here would + // delete documents that still exist upstream. Fail instead. + if (treeBody.truncated) throw new Error("github returned a truncated tree; narrow the source's folder"); + const tree = treeBody.tree; const folder = (source.folder ?? "").replaceAll(/^\/|\/$/g, ""); const prefix = folder ? folder + "/" : ""; @@ -115,30 +120,70 @@ export async function syncGithubSource(source: typeof schema.syncSource.$inferSe source: sql`excluded.source`, updatedAt: new Date() }, }); } - const seenAssets = assets.map((a) => a.path); - await db.delete(schema.asset).where(and( - eq(schema.asset.organizationId, source.organizationId), - eq(schema.asset.source, label), - seenAssets.length ? notInArray(schema.asset.path, seenAssets) : sql`true`, - )); - - // prune everything this source owns that is no longer in the repo (or moved mount) - const seen = docs.map((d) => d.path); - await db.delete(schema.document).where(and( - eq(schema.document.organizationId, source.organizationId), - eq(schema.document.source, label), - seen.length ? notInArray(schema.document.path, seen) : sql`true`, - )); + // Prune everything this source owns that is no longer in the repo (or moved + // mount) — but a sync that matched NOTHING at all is far more likely a + // misconfiguration (wrong branch, mistyped folder) than a repo that genuinely + // emptied. Pruning on that would wipe the source's entire mount, so skip and + // report it instead. + const matchedNothing = !docs.length && !assets.length; + if (!matchedNothing) { + const seenAssets = assets.map((a) => a.path); + await db.delete(schema.asset).where(and( + eq(schema.asset.organizationId, source.organizationId), + eq(schema.asset.source, label), + seenAssets.length ? notInArray(schema.asset.path, seenAssets) : sql`true`, + )); + const seen = docs.map((d) => d.path); + await db.delete(schema.document).where(and( + eq(schema.document.organizationId, source.organizationId), + eq(schema.document.source, label), + seen.length ? notInArray(schema.document.path, seen) : sql`true`, + )); + } await db.update(schema.syncSource).set({ lastSyncAt: new Date() }).where(eq(schema.syncSource.id, source.id)); - return { synced: docs.length, assets: assets.length }; + return { synced: docs.length, assets: assets.length, prunedSkipped: matchedNothing }; } export async function verifyWebhook(req: Request, body: string): Promise { - const sig = req.headers.get("x-hub-signature-256") ?? ""; - const key = await crypto.subtle.importKey("raw", - new TextEncoder().encode(process.env.GITHUB_APP_WEBHOOK_SECRET ?? ""), - { name: "HMAC", hash: "SHA-256" }, false, ["sign"]); - const mac = await crypto.subtle.sign("HMAC", key, new TextEncoder().encode(body)); - const expect = "sha256=" + [...new Uint8Array(mac)].map((b) => b.toString(16).padStart(2, "0")).join(""); - return sig.length === expect.length && sig === expect; + const secret = process.env.GITHUB_APP_WEBHOOK_SECRET; + // Without a secret there is nothing to verify against. The old empty-string + // fallback still produced a signature — one anyone could recompute — so an + // unconfigured deployment accepted forged pushes. Refuse instead. + if (!secret) return false; + const m = /^sha256=([a-f0-9]{64})$/i.exec(req.headers.get("x-hub-signature-256") ?? ""); + if (!m) return false; + const sig = Uint8Array.from(m[1].match(/../g)!, (h) => parseInt(h, 16)); + const key = await crypto.subtle.importKey("raw", new TextEncoder().encode(secret), + { name: "HMAC", hash: "SHA-256" }, false, ["verify"]); + // subtle.verify compares in constant time; the old string === did not + return crypto.subtle.verify("HMAC", key, sig, new TextEncoder().encode(body)); +} + +/** + * Which workspace, if any, already mounts a repo through this installation. + * + * Nothing else binds an installation to a workspace: /api/github/installations + * can see every installation of the App, and a sync source stores whatever + * installationId it was handed. So an installation is claimed first-come by the + * workspace that first mounts through it, and this lookup is what stops a second + * workspace pointing at someone else's private repos. + */ +export async function installationOwner(installationId: string): Promise { + const rows = await db + .select({ organizationId: schema.syncSource.organizationId }) + .from(schema.syncSource) + .where(eq(schema.syncSource.installationId, installationId)) + .limit(1); + return rows[0]?.organizationId ?? null; +} + +/** Whether the installation can actually see the repo it is being asked to mount. */ +export async function installationCanSee(installationId: string, repo: string): Promise { + const token = await installationToken(installationId); + const res = await fetch("https://api.github.com/installation/repositories?per_page=100", { + headers: { Authorization: `Bearer ${token}`, Accept: "application/vnd.github+json", "User-Agent": "pensieve" }, + }); + if (!res.ok) return false; + const { repositories } = await res.json() as { repositories: { full_name: string }[] }; + return repositories.some((r) => r.full_name.toLowerCase() === repo.toLowerCase()); }