fix(mint): reject cross-org installation mismatch#1332
Conversation
… org When the same GitHub App is installed on multiple orgs sharing a mint, findInstallation could return an installation belonging to a different org than the one authenticated via OIDC. The mint used this installation without verifying ownership, potentially minting tokens scoped to the wrong org's repos. Assert that the installation's Account.Login matches the expected org derived from the OIDC token. If they differ, reject the request with a 502 and a clear log message rather than silently minting a cross-org token. Refs: #1321, #737, #672 Assisted-by: Claude claude-opus-4-6 <[email protected]> Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Site previewPreview: https://2a57f99b-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsNo findings. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad (10 agents) — 3 MEDIUM findings
Sound defense-in-depth security fix. The EqualFold check and error handling are correct (error never leaks to HTTP client). Main concerns: (1) PR description overstates the threat — GitHub API should not return cross-org installations given the URL construction, so this is a guardrail, not a fix for a routinely exploitable path; (2) missing a case-variation test to document the EqualFold intent; (3) security event only logged via log.Printf with no structured alerting.
| // Verify the installation belongs to the expected org. When the same | ||
| // GitHub App is installed on multiple orgs, the API could return an | ||
| // installation for the wrong org. Accepting it would mint a token | ||
| // scoped to a different org's repos. |
There was a problem hiding this comment.
MEDIUM — Threat model description overstates the risk
The comment says "the API could return an installation for the wrong org," but GET /repos/{owner}/{repo}/installation constructs the URL using org from the OIDC-validated repository_owner claim, so GitHub should only return the installation for that specific org (or 404). The scenario where Account.Login differs from {owner} is not documented GitHub behavior — realistic vectors are more like repo transfers or org renames, not routine cross-org installation leakage.
The defense-in-depth check is still valuable and worth keeping, but consider clarifying the comment to say this guards against hypothetical API inconsistencies rather than a known, routinely exploitable path.
6/10 review agents flagged this
| // installation for the wrong org. Accepting it would mint a token | ||
| // scoped to a different org's repos. | ||
| if !strings.EqualFold(inst.Account.Login, org) { | ||
| log.Printf("installation org mismatch: expected %s, got %s (installation %d for %s/%s)", |
There was a problem hiding this comment.
MEDIUM — No structured alerting for this security-relevant event
A cross-org installation mismatch is a security event (misconfigured App or potential exploitation attempt), but it's only logged via log.Printf — easily lost in Cloud Functions log volume. This is the sole detection layer for this attack path.
Consider tagging the log with a severity prefix (e.g., "SECURITY: installation org mismatch") that Cloud Logging alerting policies can match on, or incrementing a custom metric (mint_cross_org_mismatch_total) so a threshold-based alert fires.
2/10 review agents flagged this
| w.WriteHeader(http.StatusCreated) | ||
| json.NewEncoder(w).Encode(installationTokenResponse{ | ||
| Token: "ghs_correct_org_token", | ||
| ExpiresAt: "2026-05-07T12:00:00Z", |
There was a problem hiding this comment.
MEDIUM — Missing test for case-mismatch between org and Account.Login
The code uses strings.EqualFold but tests only use exact-case matches ("org-a" vs "org-b", "org-a" vs "org-a"). GitHub returns canonical-cased logins (e.g., "Org-A") while org is lowercased at line 508. Adding a test where this mock returns Login: "Org-A" (mixed case) and verifying minting still succeeds (200) would document the EqualFold intent and guard against a future regression if someone changes it to ==.
4/10 review agents flagged this
2 participants
Summary
findInstallationcould return an installation belonging to a different org than the OIDC-authenticated one. The mint used it without verifying ownership.findInstallationthatinst.Account.Loginmatches the expected org. Mismatches now return 502 with a clear log message instead of silently minting a cross-org token.TestHandler_CrossOrgInstallationMismatchreproduces the Triage agent should not label ready-to-code when it identifies an existing PR that addresses the issue #1321 bug and confirms the fix rejects it.Refs: #1321, #737, #672
Test plan
TestHandler_CrossOrgInstallationMismatch— fails before fix, passes afterTestHandler_CrossOrgInstallation_SameOrgPasses— confirms happy path still worksmintsrc/main.go.embed🤖 Generated with Claude Code