Skip to content

[fix] sync hardening: cross-tenant repo mounts, prune wipes, forged webhooks - #8

Merged
Lanznx merged 1 commit into
mainfrom
fix/sync-hardening
Aug 26, 2026
Merged

Lanznx merged 1 commit into
mainfrom
fix/sync-hardening

Conversation

@yui0303

@yui0303 yui0303 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Three independent problems in the GitHub sync path. None is reachable today only because the App is not registered yet — which is also why this is the moment to fix them. Refs #1.

1. Any member could mount another workspace's private repo

GET /api/github/installations returned every installation of the App to any signed-in user, along with the repo names inside each. POST /api/sources then accepted whatever repo + installationId it was handed, validating neither.

list installations  ->  see installation 42 (another tenant) + its private repos
POST /api/sources   ->  { org: <mine>, repo: <theirs/private>, installationId: "42" }
                    ->  synced into my workspace, readable by me

Nothing in the schema binds an installation to a workspace, and adding that binding is a migration. So installations are now claimed first-come by the workspace that first mounts through them:

  • installationOwner() — rejects a second workspace claiming an installation already in use
  • installationCanSee() — rejects a repo the installation genuinely cannot read
  • the installations list is filtered to exactly what POST would accept, so the picker stops leaking other tenants' repo names

This is a containment fix, not the ideal one. A real organization_id column on the installation is better and should replace it — deliberately deferred so this ships without a schema change.

2. A sync that matched nothing deleted everything

seen.length ? notInArray(schema.document.path, seen) : sql`true`

When a sync produced no files, the prune's predicate became true — deleting every document the source owned. A mistyped folder, a branch with no .html, or a truncated tree all trigger it. Data loss, silent, no error.

An empty result is now treated as the misconfiguration it nearly always is: the prune is skipped and reported back as prunedSkipped. A sync that legitimately finds files still prunes normally.

Related, and its own data-loss path: GitHub sets truncated on trees too large for one call, and the flag was never read. A partial listing fed a prune that deletes whatever the listing omits — so a big repo would delete real documents. It now throws.

3. An unset webhook secret accepted forged pushes

new TextEncoder().encode(process.env.GITHUB_APP_WEBHOOK_SECRET ?? "")

With the secret unset, the HMAC was computed with an empty key — a signature anyone can reproduce, so /api/github/webhook accepted forged pushes and would sync on them. Per issue #1 the secret is not yet set in production, so this is the live configuration.

Now refuses outright when the secret is missing, and compares with crypto.subtle.verify (constant time) instead of string equality.

Scope

No schema change, no migration. tsc --noEmit clean, next build clean.

Not verifiable end-to-end until the GitHub App exists — these paths need an installation to exercise. Reviewed by reading; the prune guard and the webhook rejection are both straightforward to re-check by inspection.

…ebhooks

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.
@github-actions

Copy link
Copy Markdown

❌ SonarQube Quality Gate ERROR — pathorsAI_pensieve

failed condition value threshold
new_violations 3 ≤ 0

3 open issues on this PR:

  • MINOR typescript:S1128 — Remove this unused import of 'eq'. (app/api/github/installations/route.ts:2)
  • CRITICAL typescript:S3776 — Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed. (app/api/sources/route.ts:20)
  • MINOR typescript:S7773 — Prefer Number.parseInt over parseInt. (lib/github.ts:155)

@Lanznx
Lanznx merged commit 57d8acb into main Aug 26, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants