feat(auth): dynamic signup/login ban lists via AWS AppConfig#4911
feat(auth): dynamic signup/login ban lists via AWS AppConfig#4911TheodoreSpeaks wants to merge 1 commit into
Conversation
- Move blocked-domain/allowlist/MX gating from env vars into AWS AppConfig (queried at runtime via the AppConfig Data SDK with a 30s in-process cache); env vars remain a fallback for self-hosted/OSS. - Add a new bannedEmails denylist that blocks a specific address at both sign-in and sign-up. - Generic, profile-agnostic AppConfig client so future config (feature flags) reuses the same plumbing; AppConfig client shares the same credential resolution as the S3 client. - Defense in depth: authenticateApiKeyFromHeader now rejects keys belonging to banned users.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Auth hooks and the user-create hook now call Reviewed by Cursor Bugbot for commit e0c19ea. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@greptile review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e0c19ea. Configure here.
| throw new APIError('FORBIDDEN', { | ||
| message: 'Access restricted. Please contact your administrator.', | ||
| }) | ||
| } |
There was a problem hiding this comment.
Email OTP skips banned list
High Severity
The new bannedEmails check runs only when ctx.path starts with /sign-in or /sign-up, but email OTP routes are handled under /email-otp (also listed as an email-password path). A denied address can still request or complete OTP sign-in outside the ban.
Reviewed by Cursor Bugbot for commit e0c19ea. Configure here.
|
|
||
| // Cold: no value yet — fetch synchronously so the caller gets real data. | ||
| if (entry.value === null) { | ||
| return poll(ids, parse, entry) |
There was a problem hiding this comment.
Empty AppConfig never warms cache
Medium Severity
When GetLatestConfiguration succeeds but returns an empty payload, entry.value stays null, so every fetchAppConfigProfile call keeps taking the cold path and awaits another AWS round trip. Auth middleware calls this on each sign-in/sign-up while AppConfig is enabled but unseeded.
Reviewed by Cursor Bugbot for commit e0c19ea. Configure here.
Greptile SummaryThis PR moves signup/login gating lists (blocked domains, email allowlists, MX host denylist) from static env vars into AWS AppConfig with a 30-second stale-while-revalidate cache, adds a new
Confidence Score: 3/5The new bannedEmails denylist does not block existing users who authenticate via OAuth — a user banned after account creation can still log in via Google or GitHub. The core AppConfig integration, credential extraction, and MX validation refactor are clean. However, the bannedEmails denylist has a real gap: the Pay close attention to Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant BetterAuth as BetterAuth (onRequest hook)
participant AC as access-control.ts
participant AppCfg as appconfig.ts
participant AWS as AWS AppConfig
participant DB as Database
Client->>BetterAuth: "POST /sign-in/email {email, password}"
BetterAuth->>AC: getAccessControlConfig()
AC->>AppCfg: "fetchAppConfigProfile({app, env, profile})"
alt "Cache warm (< 30s TTL)"
AppCfg-->>AC: cached value
else Cache cold or stale
AppCfg->>AWS: GetLatestConfiguration
AWS-->>AppCfg: config JSON
AppCfg-->>AC: parsed config
end
AC-->>BetterAuth: AccessControlConfig
BetterAuth->>BetterAuth: check bannedEmails (requestEmail present)
BetterAuth->>BetterAuth: check allowedLoginEmails/Domains
BetterAuth->>DB: verify credentials
DB-->>BetterAuth: user record
BetterAuth-->>Client: session
Note over Client,DB: OAuth flow — bannedEmails NOT checked for existing users
Client->>BetterAuth: "POST /sign-in/social {provider: google}"
BetterAuth->>BetterAuth: "onRequest: requestEmail = undefined, skip bannedEmails"
BetterAuth->>AWS: OAuth redirect
AWS-->>BetterAuth: callback with email
BetterAuth->>DB: find/update user (no bannedEmails check for existing users)
DB-->>BetterAuth: existing user record
BetterAuth-->>Client: session (banned user allowed through)
Reviews (1): Last reviewed commit: "feat(auth): dynamic signup/login ban lis..." | Re-trigger Greptile |
| if (isSignIn || isSignUp) { | ||
| const accessControl = await getAccessControlConfig() | ||
| const requestEmail = ctx.body?.email?.toLowerCase() | ||
|
|
||
| if (!isAllowed && env.ALLOWED_LOGIN_DOMAINS) { | ||
| const allowedDomains = env.ALLOWED_LOGIN_DOMAINS.split(',').map((domain) => | ||
| domain.trim().toLowerCase() | ||
| ) | ||
| const emailDomain = requestEmail.split('@')[1] | ||
| isAllowed = emailDomain && allowedDomains.includes(emailDomain) | ||
| } | ||
| if (requestEmail && accessControl.bannedEmails.includes(requestEmail)) { | ||
| throw new APIError('FORBIDDEN', { | ||
| message: 'Access restricted. Please contact your administrator.', | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
The banned-email guard in the onRequest hook fires only when ctx.body?.email is populated, but for social sign-in (/sign-in/social), ctx.body carries provider, not email — so requestEmail is undefined and the check is silently skipped. The databaseHooks.user.create.before guard blocks new OAuth registrations, but an existing user whose address is subsequently added to bannedEmails can continue to authenticate via Google or GitHub with no friction. The PR description says "blocks a specific address at both sign-in and sign-up," but that guarantee only holds for email/password flows today. A BetterAuth session.before hook (which receives the resolved user record) or a user.update check would be needed to cover the OAuth sign-in path.
|
|
||
| const response = await dataClient.send( | ||
| new GetLatestConfigurationCommand({ ConfigurationToken: entry.nextToken }) | ||
| ) | ||
| entry.nextToken = response.NextPollConfigurationToken ?? entry.nextToken | ||
|
|
||
| if (response.Configuration && response.Configuration.length > 0) { | ||
| const text = new TextDecoder().decode(response.Configuration) | ||
| entry.value = parse(JSON.parse(text)) | ||
| } | ||
|
|
||
| entry.expiresAt = Date.now() + DEFAULT_TTL_MS |
There was a problem hiding this comment.
NextPollIntervalInSeconds ignored — may trigger AppConfig throttling
GetLatestConfiguration returns NextPollIntervalInSeconds (typically 15–300 s depending on the deployment), which AppConfig uses to communicate the minimum safe polling cadence. The code ignores this field and unconditionally resets entry.expiresAt to Date.now() + DEFAULT_TTL_MS (30 s). If AppConfig signals a longer interval, repeated polls at 30 s will generate BadRequestException (expired token) errors and force constant session re-establishment, eroding the stale-while-revalidate reliability. Respecting the returned interval — Math.max(DEFAULT_TTL_MS, nextIntervalMs) — would fix this.
| const key = cacheKey(ids) | ||
| const entry = (cache.get(key) as CacheEntry<T> | undefined) ?? { | ||
| value: null, | ||
| nextToken: undefined, | ||
| expiresAt: 0, | ||
| refreshing: false, | ||
| } | ||
| cache.set(key, entry) | ||
|
|
||
| // Cold: no value yet — fetch synchronously so the caller gets real data. | ||
| if (entry.value === null) { | ||
| return poll(ids, parse, entry) |
There was a problem hiding this comment.
The cold-path guard (entry.value === null) is checked synchronously, before any await. If two requests arrive before the first poll resolves, both read the freshly created entry from cache, both see value === null, and both call poll() concurrently on the same object. The writes to entry.nextToken and entry.value from the two concurrent polls race — last writer wins — and two StartConfigurationSession + GetLatestConfiguration pairs are sent. The existing refreshing flag prevents this for the warm-stale path; a similar in-flight promise (e.g., storing the pending poll() Promise on the entry) would prevent duplicate cold fetches.
Greptile SummaryThis PR moves signup/login gating lists (blocked domains, login allowlists, blocked MX hosts) from static env vars into AWS AppConfig with a 30-second in-process stale-while-revalidate cache, and adds a new
Confidence Score: 3/5The existing auth paths are largely preserved and the env-var fallback is solid, but the new bannedEmails feature has a meaningful coverage gap for OAuth users that should be addressed before relying on it in production. The bannedEmails check in the request middleware only fires when ctx.body?.email is present, which is true for email/password flows but not for OAuth (Google, Microsoft) sign-ins. An existing user whose address is added to the AppConfig bannedEmails list can continue to authenticate via OAuth indefinitely — the databaseHooks guard only covers new registrations. The AppConfig cache client also has a cold-start concurrency gap and resets session tokens on parse errors, but those issues are operationally self-healing. The rest of the change — credential extraction, MX-host list injection, API-key banned-user check — is clean and well-tested. apps/sim/lib/auth/auth.ts (bannedEmails OAuth bypass) and apps/sim/lib/core/config/appconfig.ts (cold-fetch concurrency, token reset on parse error) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant AuthMiddleware as hooks.before (auth.ts)
participant AccessControl as getAccessControlConfig
participant AppConfig as AWS AppConfig
participant Cache as In-process Cache
participant DBHook as databaseHooks.user.create.before
Client->>AuthMiddleware: POST /sign-in/email or /sign-up/email
AuthMiddleware->>AccessControl: getAccessControlConfig()
AccessControl->>Cache: cache.get(key)
alt "Cache cold (value === null)"
Cache-->>AccessControl: null
AccessControl->>AppConfig: StartConfigurationSession
AppConfig-->>AccessControl: InitialConfigurationToken
AccessControl->>AppConfig: GetLatestConfiguration(token)
AppConfig-->>AccessControl: config JSON + NextToken
AccessControl->>Cache: "cache.set(key, parsed value, TTL=30s)"
else Cache warm
Cache-->>AccessControl: cached value
else Cache stale
Cache-->>AccessControl: cached value (served immediately)
AccessControl->>AppConfig: poll in background (void)
end
AccessControl-->>AuthMiddleware: AccessControlConfig
AuthMiddleware->>AuthMiddleware: check bannedEmails (ctx.body?.email only)
AuthMiddleware->>AuthMiddleware: check allowedLoginEmails/Domains
AuthMiddleware->>AuthMiddleware: check blockedSignupDomains (sign-up)
AuthMiddleware->>AuthMiddleware: validateSignupEmailMx(blockedMxHosts)
AuthMiddleware-->>Client: 403 FORBIDDEN (if blocked)
Note over AuthMiddleware,DBHook: For sign-up, user creation proceeds if hooks.before passes
AuthMiddleware->>DBHook: user.create.before(user)
DBHook->>AccessControl: getAccessControlConfig() [warm cache]
DBHook->>DBHook: check bannedEmails + blockedSignupDomains
DBHook-->>Client: Error (if blocked by DB hook)
Reviews (2): Last reviewed commit: "feat(auth): dynamic signup/login ban lis..." | Re-trigger Greptile |
| if (requestEmail && accessControl.bannedEmails.includes(requestEmail)) { | ||
| throw new APIError('FORBIDDEN', { | ||
| message: 'Access restricted. Please contact your administrator.', | ||
| }) | ||
| } |
There was a problem hiding this comment.
The bannedEmails ban check relies on ctx.body?.email, which is only populated for email/password flows. For OAuth sign-in (Google, Microsoft, etc.), the email arrives as part of the identity-provider token later in the flow — ctx.body?.email is absent, so the check is silently skipped. An existing user whose address is added to the bannedEmails AppConfig list can still authenticate via OAuth. Only new OAuth registrations are blocked (by databaseHooks.user.create.before), not returning OAuth users. The PR description claims this "blocks a specific address at both sign-in and sign-up," which is not true for OAuth sign-ins by existing accounts.
| // Cold: no value yet — fetch synchronously so the caller gets real data. | ||
| if (entry.value === null) { | ||
| return poll(ids, parse, entry) | ||
| } |
There was a problem hiding this comment.
The stale-while-revalidate path correctly guards with entry.refreshing, but the cold path (entry.value === null) has no equivalent guard. If two requests arrive before any value is cached, both run synchronously through cache.set(key, entry) and then both call poll(ids, parse, entry) concurrently on the same shared entry. Inside poll, both see !entry.nextToken and each fires StartConfigurationSessionCommand. The winner writes entry.nextToken; the loser then calls GetLatestConfigurationCommand with a stale or already-consumed token, which may trigger an InvalidConfigurationTokenException on the second poll — the error handler resets the token and self-heals, but it results in unnecessary session creation and a brief window of failed fetches.
| if (response.Configuration && response.Configuration.length > 0) { | ||
| const text = new TextDecoder().decode(response.Configuration) | ||
| entry.value = parse(JSON.parse(text)) | ||
| } |
There was a problem hiding this comment.
When JSON.parse(text) or the caller-supplied parse() callback throws, execution falls through to the catch block which unconditionally sets entry.nextToken = undefined. This discards a perfectly valid session token — the network round-trip succeeded; only the response parsing failed. The next poll will unnecessarily start a brand-new session, incurring an extra API call and a higher chance of a cold-path race. Narrowing the catch (or saving the new token before calling parse) would preserve the session on deserialization errors.
Summary
bannedEmailsdenylist that blocks a specific address at both sign-in and sign-up (previously you could only block whole domains).lib/core/config/appconfig.ts) so future config (feature flags) reuses the same session/cache/IAM plumbing.getAwsCredentialsFromEnv), so it works wherever S3 does (ECS task role or explicit keys).authenticateApiKeyFromHeadernow rejects API keys belonging to banned users (banning already deletes keys; this catches any survivor).Pairs with infra PR simstudioai/infra#201 (creates the AppConfig app/env/
access-controlprofile, IAM, and injectsAPPCONFIG_APPLICATION/APPCONFIG_ENVIRONMENT). Falls back to env vars until that's deployed + the profile is seeded.Type of Change
Testing
Tested manually. 29 unit tests pass (new
appconfig,access-control, banned-user API key case, updated MX validation).tsc --noEmitclean, fullbun run lint:checkclean,bun run check:api-validation:strictpassed.Checklist