feat(github) - Authenticate By Organization Membership#664
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds optional GitHub organization membership checks for access control. Users can now restrict sign-in to members of specified GitHub organizations via ChangesGitHub Organization-based Access Control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/lib/access-control.ts`:
- Around line 91-126: Add a named exported timeout constant (e.g.,
GITHUB_MEMBERSHIP_CHECK_TIMEOUT_MS) and modify checkGitHubOrganizationAccess to
bound each fetch with an AbortController: create an AbortController for each org
check, set a timer with setTimeout to call controller.abort() after the constant
timeout, pass controller.signal to fetchImpl, and clear the timer when the
request completes or errors; you can also add an optional timeoutMs parameter to
checkGitHubOrganizationAccess (defaulting to the new constant) so callers can
override the default. Ensure you reference the existing symbols
checkGitHubOrganizationAccess, fetchImpl, allowedOrganizations and userAgent,
and keep the same error handling (catch returning false) while cleaning up
timers and not leaving dangling timeouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8b6425c-e1f3-44b7-a814-26157a58c4af
📒 Files selected for processing (13)
README.mddocs/GETTING_STARTED.mddocs/SETUP_GUIDE.mdpackages/web/.env.examplepackages/web/README.mdpackages/web/src/lib/access-control.test.tspackages/web/src/lib/access-control.tspackages/web/src/lib/auth.tsterraform/environments/production/checks.tfterraform/environments/production/terraform.tfvars.exampleterraform/environments/production/variables.tfterraform/environments/production/web-cloudflare.tfterraform/environments/production/web-vercel.tf
ColeMurray
left a comment
ColeMurray
left a comment
There was a problem hiding this comment.
[Automated Review]
Multi-perspective security review of the new GitHub-org-membership allowlist. Below is the summary of cross-cutting findings; individual issues are left as inline comments.
Blocking findings
- H1 (inline on
auth.ts:42):read:orgis granted unconditionally to every user, even whenALLOWED_GITHUB_ORGSis empty. Combined with the existing token-persistence path (seeauth.ts:67comment and the control plane'sscmTokenstorage atpackages/control-plane/src/router.ts:898), this is a permanent scope increase across every deployment, baked into encrypted tokens in D1 and per-DO SQLite. - H2 — CI workflow does not propagate
ALLOWED_GITHUB_ORGS:.github/workflows/terraform.yml:205-206and:336-337exportTF_VAR_allowed_usersandTF_VAR_allowed_email_domainsonly. The newTF_VAR_allowed_github_orgsis missing in bothplanandapplyjobs, butdocs/GETTING_STARTED.md:692instructs operators to addALLOWED_GITHUB_ORGSas a GitHub Actions secret. Operators following the docs will see Terraform silently fall back todefault = "", either failing the access-control gate or (worse) succeeding with the new allowlist unapplied becauseallowed_usersis still set. Fix the workflow before merge. - H4 (inline on
auth.ts:48): Nosession.maxAgeis configured; NextAuth defaults to 30 days. A user removed from the allowlisted org retains web access AND a usableaccessTokenfor up to a month. The PR description acknowledges this as follow-up, but it should at minimum land with a shortmaxAge(e.g. 24h) and an operator-facing doc warning. - H5 (inline on
auth.ts:72): No audit logging on sign-in. For a system that grants users sandbox-spawning rights with their GitHub tokens, denied attempts and the matching allowlist should be logged server-side (never the token itself).
Other concerns (inline)
- M1:
Promise.allfanout with no short-circuit (access-control.ts:101). - M2: All error modes (401/403/404/429/5xx/network) collapse to
falsewith no logging, making operator debugging impossible (access-control.ts:121). - M3: GitHub App "Members: Read-only" doc claim conflates OAuth scope with App installation permission (
docs/GETTING_STARTED.md:219). - M4: OR semantics across three allowlists is a footgun; needs a bold callout (
packages/web/README.md:95). - M5: Pre-existing —
user.emailfrom NextAuth's GitHub provider is not checked forverified(auth.ts:60).
Other notes (not inline)
- M6 —
packages/web/src/lib/auth.ts:35enables NextAuthdebugwhenNODE_ENV === "development"ORNEXTAUTH_DEBUG === "true". Innext-auth/core/routes/callback.js:55,logger.debug("OAUTH_CALLBACK_RESPONSE", { profile, account, ... })logs the fullaccountobject, which containsaccess_tokenandrefresh_token. With the expanded scope, those logged tokens now carryread:org. Consider gatingNEXTAUTH_DEBUGto non-production environments only. - Retracted finding — An earlier draft flagged "existing users with stale pre-PR tokens get locked out by
checkGitHubOrganizationAccessfailing 403." After verification, this is incorrect: NextAuth'ssignIncallback only runs at fresh sign-in, not on each request, so pre-existing sessions are untouched (which is actually the H4 concern in disguise). Withdrawing that one.
Things done well
- Endpoint choice (
/user/memberships/orgs/{org}) is authoritative from the user's perspective and distinguishesactivefrompending. - Fail-closed throughout (401/403/404/5xx/network/JSON-parse →
false). X-GitHub-Api-Version: 2022-11-28pinned.encodeURIComponenton the org name prevents URL/path-traversal injection.fetchImpldependency-injected for testability.- Terraform safety gate correctly extended (
checks.tf:20-26). - Three-way OR logic with all-empty guard preserved (
access-control.ts:54-62).
Recommendation
Block on H1, H2, H4, H5 before merge — each is small (conditional scope string, two workflow lines, one config line, one log statement, one doc warning) and meaningfully changes the security posture. M1–M5 are appropriate fast follow-ups.
| authorization: { | ||
| params: { | ||
| scope: "read:user user:email repo", | ||
| scope: "read:user user:email repo read:org", |
There was a problem hiding this comment.
[Automated Review] H1 (HIGH) — read:org scope granted unconditionally
This literal grants read:org to every user even when ALLOWED_GITHUB_ORGS is empty, meaning operators who only use ALLOWED_USERS / ALLOWED_EMAIL_DOMAINS now ask all their users for org-membership read access on the consent screen and persist tokens carrying that scope.
Fix: compute the scope dynamically — only append read:org when parseAllowlist(process.env.ALLOWED_GITHUB_ORGS).length > 0. NextAuth supports a function for authorization.params.
There was a problem hiding this comment.
Thanks, agreed. This is fixed in c31d15d.
read:org is now added only when ALLOWED_GITHUB_ORGS is configured. The base scope remains read:user user:email repo, and buildGitHubOAuthScope() appends read:org only when the parsed org allowlist is non-empty.
| return true; | ||
|
|
||
| const isAllowedByOrgMembership = await checkGitHubOrganizationAccess({ | ||
| accessToken: account?.access_token, |
There was a problem hiding this comment.
[Automated Review] H1 follow-on — expanded-scope token is persisted broadly
The same account.access_token used here is also stored in the JWT at auth.ts:76, forwarded to the control plane as scmToken (see packages/web/src/app/api/sessions/route.ts:56-71 and packages/web/src/app/api/sessions/[id]/ws-token/route.ts:47-48), AES-encrypted with TOKEN_ENCRYPTION_KEY at packages/control-plane/src/router.ts:898, and persisted to D1 + per-DO SQLite for the session lifetime.
A TOKEN_ENCRYPTION_KEY compromise or sandbox-side exfiltration now exposes every user's private org/team graph in addition to the existing repo blast radius. Making read:org conditional (see H1 on line 42) eliminates this for the majority of deployments.
There was a problem hiding this comment.
Fixed by the same conditional-scope change in c31d15d.
Deployments that only use ALLOWED_USERS or ALLOWED_EMAIL_DOMAINS no longer request or persist
tokens with read:org. When ALLOWED_GITHUB_ORGS is configured, the scope is still required because
the sign-in flow needs to check the signing-in user's org membership.
The follow-up auth-boundary tests in ac396b6 verify that the org-membership path uses
account.access_token for the membership request and keeps the scope conditional.
| ], | ||
| callbacks: { | ||
| async signIn({ profile, user }) { | ||
| async signIn({ account, profile, user }) { |
There was a problem hiding this comment.
[Automated Review] H4 (HIGH) — no session lifetime configured; org removal takes up to 30 days to revoke access
The PR description acknowledges this as follow-up work, but the access-control implication is significant: NextAuth's default maxAge is 30 * 24 * 60 * 60 (see next-auth/core/init.js), and this codebase doesn't override session.maxAge. A user removed from allowed_github_orgs retains web access AND a usable accessToken (in the JWT + D1) for up to 30 days.
Minimum action (this PR): set an explicit session: { maxAge: 60 * 60 * 24 } (24h) when ALLOWED_GITHUB_ORGS is used, OR re-run checkGitHubOrganizationAccess inside the jwt callback with a short cache TTL.
Minimum doc action: call this out prominently in docs/GETTING_STARTED.md near the new allowed_github_orgs instructions.
There was a problem hiding this comment.
I fully agree that this is an issue, but this existed before this PR. I would be happy (and plan to) submit an additional PR to better control and validate sessions. We should be able to periodically check whether the current user is still valid and expire the session if not.
| userAgent: process.env.NEXT_PUBLIC_APP_NAME?.trim() || DEFAULT_APP_NAME, | ||
| }); | ||
|
|
||
| return isAllowedByOrgMembership; |
There was a problem hiding this comment.
[Automated Review] H5 (HIGH) — no audit logging on sign-in decisions
Neither signIn nor checkGitHubOrganizationAccess logs anything: no record of who attempted to sign in, who was granted, who was denied, or which allowlist matched. For a system that grants users sandbox-spawning rights with their GitHub tokens, brute-force username enumeration against ALLOWED_USERS is undetectable, and operators have no audit trail for org-membership decisions.
Fix: add a server-side console.info (or your structured logger of choice) with { login, decision, reason }. Never log the access token.
There was a problem hiding this comment.
Fixed in f28f20b.
signIn now logs server-side decisions with non-secret metadata:
{ login, decision, reason }The reasons distinguish username allowlist, email-domain allowlist, org membership, no matching
policy, denied org membership, and unavailable org verification. Tokens are never logged.
ac396b6 adds tests that assert the decision log is emitted and does not include the OAuth token.
| return false; | ||
| } | ||
|
|
||
| const checks = allowedOrganizations.map(async (org) => { |
There was a problem hiding this comment.
[Automated Review] M1 (MEDIUM) — Promise.all fanout with no short-circuit and no cap
Every sign-in fires one parallel GitHub API call per allowed org. With N orgs configured, that's N requests on the user's 5000/hr token budget. There's no early-exit on first match, no cap on N, and no distinction between 429 (rate-limit) and 404 (not a member) — both collapse to false in the catch block at line 121.
Failure modes:
- A noisy operator config (e.g. 20+ orgs) measurably increases sign-in latency and rate-limit pressure.
- Transient secondary rate limits silently deny legitimate users with no operator signal.
Fix: iterate sequentially with early return, or wrap Promise.any with appropriate rejection handling. Consider a sane upper bound on allowedOrganizations.length and log a warning above it.
There was a problem hiding this comment.
Fixed in f28f20b.
Org membership checks now run sequentially and return immediately when the first active membership is
found. That avoids unnecessary calls for users who match an earlier configured org and reduces rate
limit pressure.
I did not add a hard cap on ALLOWED_GITHUB_ORGS in this PR; the sequential early-return behavior
addresses the direct fanout issue without adding a new config limit.
|
|
||
| const membership = (await response.json()) as { state?: string }; | ||
| return membership.state === "active"; | ||
| } catch { |
There was a problem hiding this comment.
[Automated Review] M2 (MEDIUM) — all error modes silently collapse to false
401 (token invalid), 403 (SAML SSO required, App lacks Members permission), 404 (not a member), 429 (rate limited), 5xx (GitHub outage), JSON parse errors, and network errors all return false indistinguishably.
An operator who configures ALLOWED_GITHUB_ORGS=acme and reports "users get Access denied" has no way to tell which root cause is in play, and SAML-SSO-enforced orgs are a particularly common silent failure.
Fix: log at least { org, status } on the non-OK branch (line 115). Do not log the access token.
if (!response.ok) {
console.warn(`[org-access] org=${org} status=${response.status}`);
return false;
}There was a problem hiding this comment.
Fixed across f28f20b and 8f01c4a.
The org check now logs non-secret diagnostics for GitHub responses and request errors, including:
- org
- HTTP status
- GitHub request id
- rate-limit headers
- retry-after
- elapsed time
It also returns a typed result so auth.ts can distinguish normal denials from operational failures:
not_memberunavailable- active membership
The sign-in decision log now reports org_membership_unavailable for 403/429/5xx, network errors,
aborts, malformed JSON, and unexpected response shapes instead of collapsing those into an ordinary
org_membership_denied.
| 7. Note the **App ID** and **Client ID** (top of page) | ||
| 8. Under **"Client secrets"**, click **"Generate a new client secret"** and note the **Client | ||
| 6. Set **Organization permissions**: | ||
| - Members: **Read-only** _(required when using `ALLOWED_GITHUB_ORGS`)_ |
There was a problem hiding this comment.
[Automated Review] M3 (MEDIUM) — docs claim conflates OAuth scope with GitHub App permission
The runtime check uses an OAuth user-to-server token (via the NextAuth GitHub provider), not the GitHub App installation token. What primarily enforces the read at runtime is the user's OAuth read:org scope, which Open-Inspect now requests automatically (packages/web/src/lib/auth.ts:42).
The "Organization permissions: Members: Read-only" requirement is correct only because the OAuth provider here is a GitHub App (App user-tokens are gated by the App's declared user-level permissions). It is not correct that the org admin must "approve" the new permission for users to sign in via OAuth in all cases, and the App does not need to be installed on the target org for the user's own membership lookup to work.
Suggested rewrite: clarify that this requirement applies because Open-Inspect uses a GitHub App as the OAuth provider, and that the user's read:org OAuth grant is what authorizes the membership check at runtime. Operators following these docs against a classic OAuth App will be confused.
There was a problem hiding this comment.
Fixed in 07a834b.
The docs now clarify that the runtime membership check uses the signing-in user's OAuth token and
that read:org is requested only when org access is configured.
They also clarify why the GitHub App permission matters here: Open-Inspect uses a GitHub App as the
OAuth provider, so operators using org access need Organization permissions: Members read-only, and
existing GitHub Apps need the permission change republished/requested/approved before testing.
| > denied unless `UNSAFE_ALLOW_ALL_USERS=true`. For Terraform-managed production deploys, Terraform | ||
| > also fails validation unless you set at least one allowlist or explicitly opt in with | ||
| > `unsafe_allow_all_users = true`. | ||
| > **Access Control**: If `ALLOWED_USERS`, `ALLOWED_EMAIL_DOMAINS`, and `ALLOWED_GITHUB_ORGS` are all |
There was a problem hiding this comment.
[Automated Review] M4 (MEDIUM) — OR semantics is a footgun; needs a bold callout
The current phrasing ("If ALLOWED_USERS, ALLOWED_EMAIL_DOMAINS, and ALLOWED_GITHUB_ORGS are all empty, sign-in is denied unless UNSAFE_ALLOW_ALL_USERS=true") doesn't make the union semantics obvious.
A reasonable operator instinct is AND: they set ALLOWED_EMAIL_DOMAINS=acme.com AND ALLOWED_GITHUB_ORGS=acme expecting "must be in the org AND on the corp domain." In reality they get a union — a GitHub account with an unverified someone@acme.com primary email (see M5) passes via the weaker email check and never reaches the server-enforced org check.
Suggested addition: a bold callout near this block: "packages/web/.env.example and docs/GETTING_STARTED.md.
There was a problem hiding this comment.
Fixed in 07a834b.
The docs now explicitly call out that allowlists use OR semantics: matching any configured GitHub
username, email domain, or active GitHub org membership grants access. This is documented in:
docs/GETTING_STARTED.mdpackages/web/README.mdpackages/web/.env.exampledocs/SETUP_GUIDE.mdterraform/environments/production/terraform.tfvars.example
|
For the M5 and M6 - I agree these are worth addressing, but I am keeping them out of this PR because they are pre-existing |
2 participants
This PR adds GitHub organization membership as a sign-in allowlist option.
Previously, access could be restricted by explicit GitHub usernames or email domains. This adds a third option:
ALLOWED_GITHUB_ORGS, which allows any user with an active membership in one of the configured GitHub organizations to sign in.This should be less tedious than managing a list of usernames or relying on email domains.
How It Works
The web app now reads a new comma-separated allowlist:
During GitHub sign-in, access is granted if any configured policy matches:
ALLOWED_USERSALLOWED_EMAIL_DOMAINSALLOWED_GITHUB_ORGSUNSAFE_ALLOW_ALL_USERS=trueThe organization check calls GitHub's authenticated membership endpoint:
Only
state: "active"is accepted. Pending membership, missing membership, denied responses, and API failures all fail closed.Org names are normalized through the same allowlist parser as users/domains, so config is case-insensitive. We should still use the canonical lowercase GitHub org slug in Terraform.
GitHub App Setup
Because the app now checks authenticated organization membership, the GitHub OAuth request includes the
read:orgscope.The GitHub App must also be updated with:
Members: Read-onlyAfter changing the app permissions, the org installation
maywill need to approve the updated permission request. Users should sign out and sign back in so GitHub grants the new OAuth scope.Terraform / Deployment
Terraform now supports:
For an org-only live test, configure:
Keeping
allowed_usersorallowed_email_domainspopulated will still allow matching users through those policies because access control is OR-based.The new env var is wired through both supported web deployment paths:
Terraform's access-control gate now treats
allowed_github_orgsas a valid allowlist, so production deploys can use org membership without setting usernames or email domains.Follow-Up Work
This PR only checks org membership during sign-in.
If a user is removed from the GitHub organization after they already have an active NextAuth session, they will keep access until that session expires. Periodic session/JWT revalidation should be handled in a follow-up PR.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores