Skip to content

fix(integration): detect GitHub App rotation, and revoke what it orphaned - #4

Open
pranav8764 wants to merge 1 commit into
Golden-Incodent-Corpusfrom
worktree-agent-a87d3c66685612289
Open

pranav8764 wants to merge 1 commit into
Golden-Incodent-Corpusfrom
worktree-agent-a87d3c66685612289

Conversation

@pranav8764

Copy link
Copy Markdown
Owner

The failure

A repository added through the archiatros-sre GitHub App never appeared in the frontend.

integration-service: "failed to generate installation token","error":"unexpected status code fetching token: 404","installation_id":142620052
api-gateway:         "list repositories failed","error":"rpc error: code = Internal desc = failed to authorize with github"

Installation 142620052 was created under a previous GitHub App; .env now carries GITHUB_APP_ID=4464433. A GitHub App can only mint installation tokens for its own installations, so the token endpoint 404s. Nothing in the stack detected this, so the row stayed status='active' forever and the only symptom was an opaque HTTP 500.

What changed

App binding is now recorded and enforced. githubInstallationMeta dropped GitHub's app_id at decode, so github_installations never knew which App owned a row. It is captured and persisted now. A NULL app_id is not a mismatch — rows predating migration 008 never observed a binding, and treating unknown as wrong would revoke every legacy installation on first read. Those fall through to the token mint, which is authoritative, and a reconnect backfills the binding.

404 is a typed, terminal error. getInstallationToken collapsed every non-201 into one bare fmt.Errorf. It now returns ErrInstallationNotFound on 404 only; callers branch with errors.Is. A 404 can never succeed on retry, so ListRepositories / AttachRepository revoke the installation and its repository_connections and return codes.FailedPrecondition with "installation belongs to a different GitHub App; reconnect required". The stored-app_id mismatch path does the same without ever calling GitHub.

Revocation is reversible. Status only — the 532 indexed chunks, ingestion history and connection metadata all survive, and reattaching clears revoked_at.

Reconcile-on-read enforces "remove a repo from the App and it disappears from the frontend". The App ships a disabled example.com webhook, so webhooks do not fire locally and a read is the only reliable moment to notice. ListRepositories diffs the live list against stored connections and revokes the absent ones. It fails open: an errored or timed-out GitHub call is not an empty list, so on failure it logs, serves the stored rows, and revokes nothing. Bounded by a 10s context. ListRepositoryConnections now filters revoked rows, which is what actually removes them from the dashboard.

Webhook actions. HandleGitHubWebhook verified the HMAC, wrote one audit row, and returned without ever reading req.EventType. It now handles installation_repositories/removed and installation/deleted|suspend. Per docs/ARCHITECTURE.md, payload prose stays untrusted: it selects existing rows by numeric id only, every workspace scope is read from the stored installation rather than the body, and no webhook may restore access. Audit reasons are fixed server-side constants.

Session sweep. Expired unconsumed github_install_sessions were never swept, so abandoned installs accumulated forever behind migration 005's partial index. CreateGitHubInstallSession now deletes them opportunistically, after authorization, tolerating failure. No background goroutine.

Testing

IntegrationServer.db is now a narrow dbConn interface rather than *pgxpool.Pool — the revocation and reconciliation branches are pure database effects and were otherwise reachable only against a live cluster. A scripted fake makes them testable. 24 new tests cover the 404→revoked path, the app-ID mismatch path, NULL app_id not being a mismatch, reconcile-on-read (including the empty-list case), fail-open on outage, every webhook branch, and the invariants that no destructive write occurs and no payload prose reaches a query argument.

cd services/integration-service && go test ./... passes; the whole Go suite (gateway, auth, workspace, project, integration) passes. Every new statement was additionally prepared and executed against the running database inside a rolled-back transaction; the live baseline is unchanged (installation active, both connections connected, 532 chunks).

Notes for the coordinator

  • Depends on migration 008_installation_app_binding.sql (U1). Already applied to the live database.
  • TestCredentialEncryptionRoundTripAndTamperDetection in credentials_test.go is a pre-existing flake, unrelated to this change and outside this unit's file scope. It fails roughly 1 run in 40. It flips the last base64 character of the ciphertext; when that character carries unused low bits, several characters decode to the same byte, so the "tampered" ciphertext is byte-identical and decrypts fine. The fix is to flip a byte of the decoded ciphertext instead.
  • Browser e2e skipped per instructions; a sibling unit owns that flow.

🤖 Generated with Claude Code

A repository added through archiatros-sre never reached the frontend. The
installation, 142620052, was created under a previous GitHub App; .env now
carries GITHUB_APP_ID=4464433. A GitHub App can only mint installation tokens
for its own installations, so POST /app/installations/{id}/access_tokens 404s.
Nothing in the stack noticed. The row stayed status='active' forever and the
only symptom was an opaque 500 with "failed to authorize with github".

Three things made that failure invisible, and all three are fixed here.

github_installations never recorded which App an installation belongs to,
because githubInstallationMeta dropped GitHub's app_id at decode. It is
captured and persisted now (migration 008 adds the column), so a rotation is
detectable locally, before a round trip. NULL stays meaningful: rows written
before the migration never observed a binding, and treating unknown as wrong
would revoke every legacy installation on first read. Those fall through to the
token mint, which is authoritative, and a reconnect backfills the binding.

getInstallationToken collapsed every non-201 into one bare fmt.Errorf, so a 404
was indistinguishable from a 500. It now returns ErrInstallationNotFound on 404
only, and callers branch on it with errors.Is. A 404 can never succeed on
retry, so ListRepositories and AttachRepository mark the installation and its
repository_connections 'revoked' and return FailedPrecondition with "reconnect
required" rather than Internal. Same treatment when the stored app_id is
non-NULL and differs from the configured one, except that case never reaches
GitHub at all. Revocation is status only -- the 532 indexed chunks and the
connection history survive, and reattaching clears revoked_at.

Reconcile-on-read is what enforces "remove a repo from the App and it
disappears from the frontend". The App ships a disabled example.com webhook, so
webhooks do not fire in local development and a read is the only reliable
moment to notice. ListRepositories diffs the live list against stored
connections and revokes the absent ones. It fails open: an errored or timed-out
GitHub call is not an empty list, so on failure it logs, serves the stored rows
and revokes nothing. An outage must never be read as "the user removed
everything". The call is bounded by a 10s context so a slow GitHub degrades to
stored data instead of holding the caller for the full client timeout.
ListRepositoryConnections now filters revoked rows, which is what actually
removes them from the dashboard.

HandleGitHubWebhook verified the HMAC, wrote one audit row and returned without
ever reading req.EventType. It now handles installation_repositories/removed
and installation/deleted|suspend. The payload stays untrusted data, not
instructions: it is used solely to select rows that already exist, by numeric
id, every workspace scope is read from the stored installation rather than the
body, and no webhook may restore access -- only a reconnect through the
authenticated install flow can. Audit reasons are fixed server-side constants
so payload prose cannot reach audit_logs.

Expired unconsumed github_install_sessions were never swept; only the DDL, one
INSERT and the claiming UPDATE referenced the table, so abandoned installs
accumulated forever behind migration 005's partial index. CreateGitHubInstallSession
now deletes them opportunistically, after authorization, and tolerates failure.

IntegrationServer.db is a narrow dbConn interface rather than *pgxpool.Pool.
The revocation and reconciliation branches are pure database effects and were
otherwise reachable only against a live cluster; a scripted fake makes them
testable. Every new statement was also prepared and executed against the
running database inside a rolled-back transaction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant