fix: refreshGithubInstallations discovers re-installs for known accounts #68

[wonderwomancode]

Summary

• refreshGithubInstallations Phase 2 discovery: The Refresh button previously called refreshGithubInstallations which only updated metadata for installations already in the DB. If a user re-installed the GitHub App on the same account (new installation_id, same account.login), or if syncGithubInstallation was interrupted during the setup_url redirect, the Refresh button returned an empty list.

  Added Phase 2: after refreshing existing rows, call listAllInstallations() (App JWT, already in client.ts) and auto-claim any GitHub installation whose account.login matches an account already linked to this org's DB. Two safety constraints prevent reintroducing the 2026-04-22 cross-tenant claim bug:

  1. Only claim installs for account.login values already recorded under this org — never unknown accounts.
  2. Skip any installation_id already owned by another org in DB.

• listAllInstallations import: Added to the import list (it was already in client.ts but unused here).

• requireAuth return captured: Changed requireAuth(context) (return ignored) to const userId = requireAuth(context) so userId is available for installedByUserId and grantInstallationAccess in Phase 2.

Test plan

• User installs GitHub App → syncGithubInstallation runs via setup_url → install appears in picker ✅ (unchanged)
• User uninstalls + re-installs GitHub App on same GitHub account → Refresh button discovers new installation_id via account_login match
• User from org A cannot steal installs belonging to org B via Refresh (safety constraint 2)
• listAllInstallations() failure (simulate GitHub API down) → non-fatal, mutation still returns existing DB records

Closes #68

🤖 Generated with Claude Code

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[wonderwomancode]

Quinn QA Review

Verdict: REQUEST CHANGES

Note: GitHub prevents requesting changes on your own PR — posting as a comment instead.

---

Test coverage findings

BLOCKER: Zero automated test coverage for the new Phase 2 security logic.

refreshGithubInstallations has no test file anywhere in the repo (grep across all *.test.* files returns no hits). The new Phase 2 discovery path adds security-critical branch logic with no automated test coverage:

Scenario	Test exists?
Re-install for known account is auto-claimed	❌
Unknown account login is NOT claimed (Constraint 1)	❌
installation_id already owned by another org is NOT claimed (Constraint 2)	❌
listAllInstallations() failure is non-fatal — Phase 1 results still returned	❌
TOCTOU: concurrent Refresh calls don't double-claim	❌

The 2026-04-22 cross-tenant data leak is explicitly cited as the historical motivation for constraints 1 and 2. Those constraints are the entire security guarantee of this PR. Without automated tests, any future refactor that accidentally widens the knownAccountLogins check, removes the alreadyOwned guard, or changes the short-circuit order will regress silently. Existing test suite: 684 tests, zero GitHub mutations covered.

---

Regression risks

Low overall. Phase 1 behavior is unchanged (same getInstallation refresh loop, same select shape plus the new accountLogin field). The const userId = requireAuth(context) change is strictly additive — requireAuth still throws on unauthenticated callers; capturing the return value was the pre-existing latent bug. No existing code paths are altered.

One subtle limitation: Phase 2 only works when owned is non-empty (at least one prior installation exists for the org). If the very first install's setup_url redirect is interrupted and syncGithubInstallation never ran, knownAccountLogins is empty and Phase 2 is a no-op. The PR body says "or if syncGithubInstallation was interrupted" but that only applies to re-installs where a prior row exists. Not a blocker, but the comment is slightly misleading.

---

Code quality and maintainability

The logic is clean and in-code comments are thorough. The phased approach with separate error boundaries is the right structure. The source: 'refresh' tag on grantInstallationAccess is good for audit trails.

One growth note: listAllInstallations() fetches per_page=100 with no pagination. The comment says "we'll never realistically have more for a single org's UI" but this function returns installs across all tenants of the App, not one org. At 100+ total installs platform-wide, Phase 2 silently truncates with no warning logged.

---

Bugs / logic errors

None found. The TOCTOU race between findUnique + create is handled gracefully — the unique constraint on installationId means the losing concurrent request gets a constraint error caught by the inner try/catch, logs a warning, and continues. Correct behavior.

---

Security implications

Both constraints are correctly structured. Lain will go deeper — flagging the untested constraint coverage as the primary risk since the exploit this defends against already happened once.

---

Blockers

1. Add automated tests for Phase 2 security logic covering at minimum:
   • Happy path: re-install for a knownAccountLogin is discovered and claimed
   • Constraint 1: GitHub install with an unknown account.login is skipped
   • Constraint 2: GitHub install whose installation_id already exists in DB for another org is skipped
   • Non-fatal: listAllInstallations() throws → mutation still returns Phase 1 results without error

---

Suggested follow-ups (non-blocking)

• Add pagination to listAllInstallations() (or at minimum log a warning when truncation may occur) before the platform reaches ~80 total installations
• Clarify Phase 2 comment: "interrupted setup_url redirect" only helps when the org already has ≥1 prior install for that account.login