Repository navigation
[PROJ-1816] Login allowlist + waitlist intake (dark behind LOGIN_ALLOWLIST_ENABLED) - #353
Merged
AndrewNordstrom merged 4 commits intoJul 13, 2026
Merged
Conversation
… waitlist intake
Dark behind LOGIN_ALLOWLIST_ENABLED (default false). When enabled, login
requires the DID to be an admin (BOT_ADMIN_DIDS) or an active
approved_participants row; the deny path invalidates the just-minted
session and returns 403 {error:NotApproved, waitlist:true} with the
discriminator declared in the response schema.
Waitlist intake: POST /api/governance/waitlist stores a normalized handle
(+ optional note <=500) with ON CONFLICT DO NOTHING and an identical
generic 200 for every state, so the endpoint cannot be used to probe
approval status. Rate-limited at the login tier (10/min/IP), placed above
the governance mutation catch-all.
Admin review: GET /api/admin/waitlist, POST /:id/approve (resolves handle
to DID via the shared resolve-handle helper extracted from participants,
upserts approved_participants, invalidates the participant cache so
approval is immediate, audit-logs), POST /:id/reject. Unresolvable
handles leave the row pending; participants API remains the DID escape
hatch. Admin status now reports loginAllowlistEnabled.
Migration 033 creates waitlist_requests. Tests: 25 new across three
suites (normalization, enumeration-proof responses, flag-off regression,
admin bypass, session invalidation on deny, fail-closed, approve/reject
lifecycle). Full suite 135 files / 1460 tests green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughAdds a public voting-pilot waitlist, admin approval and rejection routes, login allowlist enforcement behind a default-disabled flag, waitlist persistence, rate limiting, status reporting, shared handle resolution, and route and authentication tests. ChangesWaitlist and login allowlist
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
…ction Review follow-up. The approve handler read the row, checked pending, then did an unconditional UPDATE — a TOCTOU where two concurrent approves could both pass the check, both resolve the handle, and both write an audit row. And its four writes weren't transactional, so a partial failure could leave the account approved in approved_participants while the queue still showed pending. Now: a fast 404/409 pre-check (skips the network resolve for bad ids), then the pending-claim UPDATE ... WHERE id = $1 AND status = 'pending' RETURNING runs inside a transaction with the participant upsert and audit insert — atomic claim (no double-approve) and no cross-table drift. Cache invalidation moved after commit. Reject already used the atomic pattern. Adds a rate-limit-config regression test asserting the public waitlist POST matches the login-tier IP-keyed branch (guards the ordering vs the generic governance catch-all). Full suite 135 files / 1462 tests green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@src/admin/routes/resolve-handle.ts`:
- Around line 9-13: Update resolveHandleToDid to create an AbortController,
abort the signal after the configured timeout, and pass that signal to
agent.resolveHandle so hung bsky.social requests are bounded. Add coverage
verifying both explicit abort handling and a slow upstream exceeding the
timeout.
In `@src/admin/routes/waitlist.ts`:
- Around line 190-201: Extract the duplicated approved-participant upsert into a
shared upsertApprovedParticipant helper accepting a database client and { did,
handle, addedBy, notes }. Replace the inline SQL in both the waitlist route and
participants route with this helper, preserving the existing conflict handling,
handle COALESCE behavior, and transaction client usage.
- Around line 237-292: Make the reject handler’s status update and governance
audit insertion atomic by wrapping both queries in a transaction, following the
transaction pattern used by the approve handler. Update the `waitlist_requests`
and `governance_audit_log` operations in the `/waitlist/:id/reject` route to use
the same transaction client, committing only after both succeed and rolling back
on failure; preserve the existing not-found and conflict handling.
In `@src/governance/routes/auth.ts`:
- Around line 141-161: Update the denied-account branch in the authentication
handler to let invalidateSession(session.accessJwt) failures propagate to the
existing outer error handling instead of catching and logging them locally.
Preserve the 403 response only when invalidation succeeds, and ensure
SessionStoreUnavailableError follows the established 503 SessionStoreUnavailable
behavior used elsewhere in the handler.
In `@tests/admin-waitlist.test.ts`:
- Around line 105-192: The approval tests cover the lost-claim path but not
errors after the transaction begins or invalid route IDs. Extend the approve
test suite around the existing transaction tests to reject the participant
upsert or audit query after a successful claim, then assert a non-2xx response,
ROLLBACK, no cache invalidation, and exactly one client release; also add
400-status cases for non-numeric, zero, and negative IDs to cover the route’s
positive integer validation.
In `@tests/governance-auth-allowlist.test.ts`:
- Around line 126-133: Update the deny-path cookie assertion in the governance
authorization test to require that the response has no set-cookie header at all,
rather than conditionally inspecting cookie contents. Remove the conditional
iteration and assert direct absence on response.headers['set-cookie'],
preserving the existing deny-path behavior.
- Around line 136-147: Update the “flag on: allowlist lookup failure” test
around login(buildApp()) to make isParticipantApprovedMock reject with a storage
error instead of resolving false. Keep the existing 403 status and NotApproved
assertions so the route’s catch path is verified to fail closed, while leaving
the separate unapproved-account test unchanged.
In `@tests/governance-waitlist-route.test.ts`:
- Around line 65-79: Extend the governance waitlist validation tests with a
successful boundary case: submit a valid 253-character handle and a note exactly
500 characters long, then assert the request succeeds and the expected query
path is invoked. Keep the existing malformed and oversized-input rejection
coverage unchanged.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b058a651-5a5d-4c50-b0bb-02ca85a60464
📒 Files selected for processing (15)
src/admin/routes/index.tssrc/admin/routes/participants.tssrc/admin/routes/resolve-handle.tssrc/admin/routes/status.tssrc/admin/routes/waitlist.tssrc/config.tssrc/db/migrations/033_waitlist.sqlsrc/feed/rate-limit-config.tssrc/governance/routes/auth.tssrc/governance/routes/waitlist.tssrc/governance/server.tstests/admin-waitlist.test.tstests/governance-auth-allowlist.test.tstests/governance-waitlist-route.test.tstests/rate-limit-config.test.ts
…transactional reject, harden deny - resolve-handle: bound the external bsky.social resolveHandle with a 10s AbortController timeout so a hung upstream can't pin admin requests open. - Extract upsertApprovedParticipant() shared by the participants and waitlist approve routes — the allowlist write path no longer has two copies to drift. - Make the reject handler transactional (status flip + audit commit together), matching approve; a rejected request can't lose its audit row on partial failure. - Login deny: if invalidating the just-minted session throws, return 503 (consistent with this file's SessionStoreUnavailable handling) instead of a 403 that would leave a live session — the 'token must be dead' invariant holds. - Tests: mid-transaction-throw rollback coverage; unconditional cookie-absence assertion; a real fail-closed case (isParticipantApproved throws) + the new invalidation-failure 503 path; note/handle boundary-pass cases. Full suite 135 files / 1465 tests green.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/admin/routes/participants.ts (1)
162-182: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winParticipant add: upsert + audit insert aren't atomic — same bug class just fixed for waitlist reject.
This is the identical shape of issue that was fixed elsewhere in this PR ("audit log can silently be lost" for
waitlist.tsreject): the upsert (163-168) and thegovernance_audit_loginsert (174-182) are two independentdb.querycalls. If the audit insert throws after the upsert commits (constraint violation, pool exhaustion, network blip), the participant is live inapproved_participantswith zero audit trail, and the caller gets an uncaught error despite the write having actually succeeded. TheDELETE /participants/:didhandler below (235-268) has the same pattern.🔒 Proposed fix: commit upsert + audit together
- // Insert (or re-activate if previously removed) - await upsertApprovedParticipant(db, { - did: resolvedDid, - handle: resolvedHandle, - addedBy: adminDid, - notes: notes ?? null, - }); - - // Invalidate cache so next feed request picks up the change - await invalidateParticipantCache(resolvedDid); - - // Audit log - await db.query( - `INSERT INTO governance_audit_log (action, actor_did, details) - VALUES ($1, $2, $3)`, - [ - 'participant_added', - adminDid, - JSON.stringify({ did: resolvedDid, handle: resolvedHandle, notes }), - ] - ); + // Upsert + audit commit together, or not at all. + const client = await db.connect(); + try { + await client.query('BEGIN'); + await upsertApprovedParticipant(client, { + did: resolvedDid, + handle: resolvedHandle, + addedBy: adminDid, + notes: notes ?? null, + }); + await client.query( + `INSERT INTO governance_audit_log (action, actor_did, details) + VALUES ($1, $2, $3)`, + ['participant_added', adminDid, JSON.stringify({ did: resolvedDid, handle: resolvedHandle, notes })] + ); + await client.query('COMMIT'); + } catch (err) { + await client.query('ROLLBACK').catch(() => {}); + throw err; + } finally { + client.release(); + } + + // Invalidate cache so next feed request picks up the change + await invalidateParticipantCache(resolvedDid);🤖 Prompt for 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. In `@src/admin/routes/participants.ts` around lines 162 - 182, Wrap the upsertApprovedParticipant call and the governance_audit_log insert in a single transaction using db.connect()/BEGIN/COMMIT with ROLLBACK-on-error and client.release() in finally, mirroring the pattern used in src/admin/routes/waitlist.ts's approve/reject handlers. Keep invalidateParticipantCache after the commit, outside the transaction.🤖 Prompt for 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. In `@src/admin/routes/participants.ts` around lines 162 - 182, Wrap upsertApprovedParticipant and the participant_added governance_audit_log insert in the add-participant handler’s single transaction using db.connect(), BEGIN, COMMIT, rollback on errors, and client.release() in finally, mirroring the waitlist transaction pattern. Keep invalidateParticipantCache(resolvedDid) after the transaction commits and outside it; apply the same atomicity fix to the DELETE /participants/:did handler’s participant removal and audit insert.Source: Path instructions
src/governance/routes/auth.ts (1)
141-167: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApproval-check throwing skips session invalidation entirely — the "must be dead" invariant only covers half the failure surface.
The fix for the
invalidateSessioncatch (Lines 151-158) is correct and matches the file's establishedSessionStoreUnavailablepattern — that part resolves the prior review comment.But
invalidateSessionis only reachable afterisParticipantApprovedresolves tofalse(Line 146-148). IfisParticipantApproved(session.did)itself throws, execution jumps straight to the outer catch (500/503) andinvalidateSessionis never called — the just-minted session for that account stays live in the store. That directly contradicts the comment on Lines 141-144: "the minted token must be dead, not merely unreturned." The client never gets the cookie either way, but a valid, undead session for an unapproved/undetermined account now sits in the store until its natural TTL.🔒 Proposed fix: fail closed on the approval-check itself, not just on invalidation
if (config.LOGIN_ALLOWLIST_ENABLED && !isAdmin(session.did)) { - const approved = await isParticipantApproved(session.did); + let approved: boolean; + try { + approved = await isParticipantApproved(session.did); + } catch (approvalErr) { + // Fail closed here too — don't leave a freshly-minted session + // alive just because we couldn't decide whether to deny it. + try { + await invalidateSession(session.accessJwt); + } catch (invalidateErr) { + logger.error({ err: invalidateErr, did: session.did }, 'Failed to invalidate session after approval-check failure'); + } + throw approvalErr; + } if (!approved) {Suggested test in
tests/governance-auth-allowlist.test.ts: in the existing "allowlist lookup that throws" case, additionally assertdeleteSessionMock/invalidateSessionwas called with the mintedaccessJwt— today that assertion would fail, exposing this exact gap.🤖 Prompt for 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. In `@src/governance/routes/auth.ts` around lines 141 - 167, Wrap the isParticipantApproved(session.did) call in its own try/catch so that if it throws, the handler attempts to invalidate the just-minted session (session.accessJwt) before rethrowing, logging any invalidation failure at error level. Preserve the existing behavior for the resolved-false branch (invalidate → 403/503) unchanged.🤖 Prompt for 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. In `@src/governance/routes/auth.ts` around lines 141 - 167, Wrap the isParticipantApproved(session.did) call in its own try/catch within the allowlist gate; when approval lookup throws, attempt invalidateSession(session.accessJwt), log any invalidation failure at error level, then rethrow the original approval error so existing outer handling applies. Preserve the resolved-false path’s current invalidation and 403/503 behavior.Source: Path instructions
src/admin/routes/waitlist.ts (1)
213-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPost-commit cache-invalidation failure turns a successful approval into a false 500.
By Line 220 the DB transaction has already committed —
waitlist_requestsisapproved,approved_participantsis upserted, and the audit row exists.invalidateParticipantCache(did)isn't wrapped in try/catch, so if it throws (Redis blip), the whole request 500s even though the approval fully succeeded. Per the comment on Line 219, the account then stays cache-locked-out of login for up to 300s, and a retrying admin hits409 Request already decidedwith no way to tell the approval actually worked.🔒 Proposed fix: don't fail the response for a stale-cache-only failure
// After commit: a login attempt made before approval leaves a cached // negative for up to 300s, so clear it or the account stays locked out. - await invalidateParticipantCache(did); + // The DB write already committed — a cache-invalidation hiccup shouldn't + // present a successful approval to the admin as a failure. + try { + await invalidateParticipantCache(did); + } catch (err) { + logger.error({ err, did }, 'Waitlist approve: cache invalidation failed after commit'); + }Suggested test: mock
invalidateParticipantCacheto reject and assert the response is still200 { success: true, did, handle }.🤖 Prompt for 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. In `@src/admin/routes/waitlist.ts` around lines 213 - 224, Wrap the invalidateParticipantCache(did) call in try/catch, logging a warning/error on failure instead of letting it propagate, since the approval transaction has already committed by this point. The route should still return the 200 success response even if cache invalidation fails.🤖 Prompt for 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. In `@src/admin/routes/waitlist.ts` around lines 213 - 224, Wrap the post-commit invalidateParticipantCache(did) call in a try/catch so cache failures are logged as a warning or error without propagating. Keep the successful approval response through the existing logger.info and reply.send path, ensuring the route still returns 200 after the database transaction commits.Source: Path instructions
🤖 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 `@tests/admin-waitlist.test.ts`:
- Around line 215-231: Add a reject-path test alongside the existing reject
test, configuring clientQueryMock so the returning UPDATE in the reject handler
succeeds while the governance_audit_log query rejects. Assert the request
returns a 500-or-greater status, the recorded SQL contains ROLLBACK but not
COMMIT, and releaseMock is called exactly once.
In `@tests/governance-waitlist-route.test.ts`:
- Around line 65-78: Update the boundary test around longHandle to construct an
exactly 253-character valid handle, assert its length is 253, and verify the
second queryMock call received the normalized lowercase handle with a null note.
Keep the existing success assertion and first submission checks unchanged.
---
Outside diff comments:
In `@src/admin/routes/participants.ts`:
- Around line 162-182: Wrap upsertApprovedParticipant and the participant_added
governance_audit_log insert in the add-participant handler’s single transaction
using db.connect(), BEGIN, COMMIT, rollback on errors, and client.release() in
finally, mirroring the waitlist transaction pattern. Keep
invalidateParticipantCache(resolvedDid) after the transaction commits and
outside it; apply the same atomicity fix to the DELETE /participants/:did
handler’s participant removal and audit insert.
In `@src/admin/routes/waitlist.ts`:
- Around line 213-224: Wrap the post-commit invalidateParticipantCache(did) call
in a try/catch so cache failures are logged as a warning or error without
propagating. Keep the successful approval response through the existing
logger.info and reply.send path, ensuring the route still returns 200 after the
database transaction commits.
In `@src/governance/routes/auth.ts`:
- Around line 141-167: Wrap the isParticipantApproved(session.did) call in its
own try/catch within the allowlist gate; when approval lookup throws, attempt
invalidateSession(session.accessJwt), log any invalidation failure at error
level, then rethrow the original approval error so existing outer handling
applies. Preserve the resolved-false path’s current invalidation and 403/503
behavior.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23dbef94-71c9-4811-ad49-3b92874529ef
📒 Files selected for processing (8)
src/admin/routes/participant-upsert.tssrc/admin/routes/participants.tssrc/admin/routes/resolve-handle.tssrc/admin/routes/waitlist.tssrc/governance/routes/auth.tstests/admin-waitlist.test.tstests/governance-auth-allowlist.test.tstests/governance-waitlist-route.test.ts
…andle boundary - Add the reject-side mirror of the approve mid-transaction-throw test: force the audit insert to fail after a winning claim, assert ROLLBACK / no COMMIT / release once. - Boundary test now builds valid domain-format handles of exactly 253 (pass) and 254 (400) chars, so a .max() regression on the handle schema is caught instead of only exercising ~77% of the cap.
AndrewNordstrom
deleted the
dev/PROJ-1816-login-allowlist-waitlist-backend
branch
July 13, 2026 05:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Turns Corgi voting into an approved-accounts pilot, dark behind
LOGIN_ALLOWLIST_ENABLED(default false) — merging this changes no behavior until the flag is flipped on the VPS.src/governance/routes/auth.ts): after credentials verify, admins (BOT_ADMIN_DIDS) bypass; everyone else needs an activeapproved_participantsrow (isParticipantApproved, fail-closed, 300s Redis cache). Deny invalidates the just-minted Redis session (it's persisted before the gate can run) and returns 403{error:"NotApproved", waitlist:true}— the discriminator is declared in the response schema so Fastify's serializer doesn't strip it. No session cookie on deny.POST /api/governance/waitlist, public): zod-normalized handle (trim, strip@, lowercase, domain regex; DIDs rejected), optional note ≤500.ON CONFLICT (handle) DO NOTHING— sticky rejections, first-note-wins. Identical generic 200 for new/duplicate/approved/rejected so the endpoint can't be used to probe who's approved. Rate-limited at the login tier (10/min/IP) via a branch placed above the governance-mutation catch-all inrate-limit-config.ts(which would otherwise key it by DID with vote limits)./api/admin/waitlist, inheritsrequireAdmin): list (status filter, oldest-first queue), approve (resolve handle→DID viaresolve-handle.ts— extracted from participants.ts, both now share it; upsert intoapproved_participantswith re-activation;invalidateParticipantCacheso approval takes effect immediately; auditwaitlist_approved), reject (auditwaitlist_rejected). 404/409/400; unresolvable handles leave the row pending — the participants API remains the add-by-DID escape hatch.033_waitlist.sql:waitlist_requestswith unique normalized handle + status CHECK.GET /api/admin/statusnow reportsloginAllowlistEnabled(for the upcoming admin Access panel).Why
Security review found the only real gap in the auth story: any Bluesky account with an app password could log in and vote. The transparency surfaces stay public by design; this scopes participation to a pilot allowlist with a public waitlist, reusing the existing
approved_participantsinfra (migration 016, cache, admin CRUD, audit log) rather than new machinery.Linear: Fixes PROJ-1816
Testing performed
@Alice.Example.COM→alice.example.com), custom domains, enumeration-proof duplicate body, flag-off regression (unapproved still logs in), admin bypass without a participant lookup, 403 body + minted-session invalidation + no-cookie-leak assertion, fail-closed lookup, approve lifecycle (upsert + cache invalidation + audit), reject, 404/409/400 paths..env.exampleenv, same as CI).tsc --noEmitclean.Reviewer focus
auth.ts— session invalidation ordering and the 403 schema.waitlist.ts(intentionally never varies by row state).rate-limit-config.ts(must precede the governance catch-all).Deployment notes (post-merge, before flag flip)
deploy.yml does not run migrations —
npm run migrateon the VPS applies 033. Flag flip happens only after the frontend packet (waitlist UX) lands: verifyBOT_ADMIN_DIDScontains the founder DID, then setLOGIN_ALLOWLIST_ENABLED=true+ restart. Rollback = flag off + restart. (.env.exampleline omitted — local policy hook blocks.env*edits; the flag is documented inconfig.ts.)