Skip to content

feat(auth): dynamic signup/login ban lists via AWS AppConfig#4911

Merged
TheodoreSpeaks merged 4 commits into
stagingfrom
feat/faster-ban-user
Jun 9, 2026
Merged

feat(auth): dynamic signup/login ban lists via AWS AppConfig#4911
TheodoreSpeaks merged 4 commits into
stagingfrom
feat/faster-ban-user

Conversation

@TheodoreSpeaks

@TheodoreSpeaks TheodoreSpeaks commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Move signup/login gating lists (blocked domains, login allowlist, blocked MX hosts) out of env vars into AWS AppConfig, queried at runtime via the AppConfig Data SDK with a 30s in-process cache + stale-while-revalidate. Env vars remain a fallback so self-hosted/OSS works with no AWS.
  • Generic, profile-agnostic AppConfig client (lib/core/config/appconfig.ts) so future config (feature flags) reuses the same session/cache/IAM plumbing.
  • AppConfig client shares the same credential resolution as the S3 client (extracted getAwsCredentialsFromEnv), so it works wherever S3 does (ECS task role or explicit keys).
  • Defense in depth: authenticateApiKeyFromHeader now 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-control profile, IAM, and injects APPCONFIG_APPLICATION/APPCONFIG_ENVIRONMENT). Falls back to env vars until that's deployed + the profile is seeded.

Type of Change

  • New feature

Testing

Tested manually. 29 unit tests pass (new appconfig, access-control, banned-user API key case, updated MX validation). tsc --noEmit clean, full bun run lint:check clean, bun run check:api-validation:strict passed.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

- 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.
@vercel

vercel Bot commented Jun 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 8, 2026 11:56pm

Request Review

@cursor

cursor Bot commented Jun 8, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Touches sign-in/sign-up enforcement and API key authentication; misconfigured AppConfig or cache/fallback behavior could widen or tighten access until lists are seeded and infra is deployed.

Overview
Moves signup/login gating lists (blocked signup domains, login allowlist, blocked MX hosts) from startup env parsing to getAccessControlConfig(), which loads the access-control AppConfig profile on hosted deployments (via new fetchAppConfigProfile with ~30s cache and stale-while-revalidate) and falls back to the same env CSV vars when AppConfig is off or fetch fails.

Auth hooks and user-create hooks now await that config for domain denylist, login allowlist, and MX signup checks; validateSignupEmailMx takes the MX denylist as an argument instead of reading env internally.

Adds shared getAwsCredentialsFromEnv (used by S3 and AppConfig), APPCONFIG_* env and isAppConfigEnabled, plus @aws-sdk/client-appconfigdata.

API key auth left-joins the user row and treats keys owned by banned users as invalid (defense in depth).

Reviewed by Cursor Bugbot for commit c66a2a4. Bugbot is set up for automated code reviews on this repo. Configure here.

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

@greptile review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment thread apps/sim/lib/auth/auth.ts Outdated
Comment thread apps/sim/lib/core/config/appconfig.ts Outdated
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves signup/login gating lists (blocked domains, login allowlist, blocked MX hosts) from static env-var parsing into a dynamic AWS AppConfig source with a 30 s in-process cache and stale-while-revalidate, while keeping env vars as a fallback for self-hosted deployments. It also adds a defense-in-depth check in authenticateApiKeyFromHeader that rejects API keys whose owning user is banned.

  • appconfig.ts — Generic profile-agnostic AppConfig client; properly preserves the session token on parse errors (split try/catch), deduplicates concurrent cold-path polls via a shared inflight promise (??=), and respects NextPollIntervalInSeconds with Math.max.
  • access-control.ts — New abstraction layer that reads from AppConfig when enabled and falls back to fromEnv(); normalizeList trims, lowercases, and deduplicates all entries.
  • service.ts — LEFT JOIN on user to surface the banned flag; orphaned keys (no matching user row) return null, which is correctly treated as non-banned.

Confidence Score: 4/5

Safe to merge with awareness that the bannedEmails denylist and the OAuth allowlist bypass noted in prior review rounds remain open; those gaps are pre-existing or out of scope for this PR.

The AppConfig client correctly handles token rotation, parse errors, concurrent cold fetches, and the NextPollIntervalInSeconds back-off — all issues raised in prior review rounds are addressed. The defense-in-depth banned-user check in the API key path is clean. The blockedMxHosts comparison relies on caller-supplied pre-lowercased values without enforcing this at the comparison site, which is a minor contract gap. The overall change delivers its stated goal reliably, but the open items from prior threads limit full confidence in the feature's stated guarantees.

apps/sim/lib/auth/auth.ts and apps/sim/lib/auth/access-control.ts for the open gaps noted in prior review threads; the new AppConfig plumbing files look solid.

Important Files Changed

Filename Overview
apps/sim/lib/core/config/appconfig.ts New generic AppConfig client with in-process TTL cache, stale-while-revalidate, in-flight deduplication via ??=, and proper token preservation on parse errors — all previous review concerns are addressed.
apps/sim/lib/auth/access-control.ts New access-control abstraction: reads from AppConfig when enabled, falls back to env vars. normalizeList correctly lowercases and deduplicates. AccessControlConfig interface lacks bannedEmails (flagged in prior review thread).
apps/sim/lib/auth/auth.ts Signup/login gating migrated from module-level static env parsing to async getAccessControlConfig(). MX check correctly threads blockedEmailMxHosts from access control config. Allowlist check gates on requestEmail presence — pre-existing OAuth bypass unchanged.
apps/sim/lib/api-key/service.ts Adds LEFT JOIN on user table to surface banned flag; rejects keys belonging to banned users. null from the LEFT JOIN (orphaned key, no matching user) is correctly treated as non-banned.
apps/sim/lib/core/config/aws.ts Extracts shared getAwsCredentialsFromEnv() from S3 client; clean refactor with no logic changes.
apps/sim/lib/messaging/email/validation.server.ts Removes env-reading from validateSignupEmailMx; blockedMxHosts is now a caller-supplied parameter. The function assumes entries are pre-lowercased but this is undocumented.
apps/sim/lib/core/config/feature-flags.ts Adds isAppConfigEnabled flag gated on isHosted + both AppConfig env vars being set. Clean and correct.
apps/sim/lib/core/config/appconfig.test.ts Comprehensive tests covering cold fetch, parse errors, empty payload, concurrent deduplication, and cache behavior. Uses unique IDs per test to avoid module-level cache bleed.

Sequence Diagram

sequenceDiagram
    participant Client
    participant BetterAuth as BetterAuth onRequest
    participant AC as getAccessControlConfig
    participant AppConfig as AWS AppConfig
    participant Cache as In-process Cache

    Client->>BetterAuth: POST /sign-in or /sign-up
    BetterAuth->>AC: getAccessControlConfig()
    AC->>Cache: check entry (loaded?)
    alt cold (never polled)
        Cache-->>AC: "entry.loaded = false"
        AC->>AppConfig: StartConfigurationSession
        AppConfig-->>AC: InitialConfigurationToken
        AC->>AppConfig: GetLatestConfiguration
        AppConfig-->>AC: payload + NextPollConfigurationToken
        AC->>Cache: store value, mark loaded, set expiresAt
    else warm and fresh
        Cache-->>AC: return cached value
    else warm and stale (background)
        Cache-->>AC: return cached value
        AC->>AppConfig: poll in background (async)
    end
    AC-->>BetterAuth: AccessControlConfig (or env fallback)
    BetterAuth->>BetterAuth: allowlist / blockedDomains / MX checks
    BetterAuth-->>Client: 200 OK or 403 FORBIDDEN
Loading

Reviews (5): Last reviewed commit: "fix(appconfig): preserve session token o..." | Re-trigger Greptile

Comment thread apps/sim/lib/auth/auth.ts Outdated
Comment thread apps/sim/lib/core/config/appconfig.ts Outdated
Comment thread apps/sim/lib/core/config/appconfig.ts Outdated
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This 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 bannedEmails per-address denylist. It also adds a defense-in-depth check that rejects API keys belonging to banned users at the database query layer.

  • New appconfig.ts client: Generic, profile-agnostic AppConfig poller with TTL cache; self-heals on fetch errors by serving the last known value and resetting the session token.
  • New access-control.ts: Resolves the four gating lists + bannedEmails from AppConfig when APPCONFIG_APPLICATION/APPCONFIG_ENVIRONMENT are set, otherwise falls back to the existing env vars — making this fully backwards-compatible for self-hosted deployments.
  • authenticateApiKeyFromHeader hardening: Left-joins the user table and rejects keys whose owner has banned = true in the DB.

Confidence Score: 3/5

The 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

Filename Overview
apps/sim/lib/core/config/appconfig.ts New generic AppConfig polling client with stale-while-revalidate cache; cold-path has a concurrency gap (no guard against parallel cold fetches) and discards valid session tokens on parse errors
apps/sim/lib/auth/auth.ts Auth middleware updated to fetch access control config dynamically; bannedEmails check relies on ctx.body?.email which is absent for OAuth sign-in flows, leaving existing OAuth users unblocked
apps/sim/lib/auth/access-control.ts New access control config resolver with AppConfig + env-var fallback; clean normalisation, well-typed, and safe fallback behaviour on fetch failure
apps/sim/lib/core/config/aws.ts Extracts shared AWS credential resolution into a single helper used by both S3 and AppConfig clients
apps/sim/lib/api-key/service.ts Left-joins user table to surface the banned flag and rejects API keys belonging to banned users as a defense-in-depth measure
apps/sim/lib/messaging/email/validation.server.ts MX validation refactored to accept blockedMxHosts as a parameter instead of reading from env, decoupling it from env access and enabling AppConfig-sourced lists
apps/sim/lib/core/config/env.ts Adds two optional env vars (APPCONFIG_APPLICATION, APPCONFIG_ENVIRONMENT) that activate AppConfig sourcing when both are present
apps/sim/lib/uploads/providers/s3/client.ts Switches S3 credential construction to the shared getAwsCredentialsFromEnv helper, reducing duplication

Sequence Diagram

sequenceDiagram
    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)
Loading

Reviews (2): Last reviewed commit: "feat(auth): dynamic signup/login ban lis..." | Re-trigger Greptile

Comment thread apps/sim/lib/auth/auth.ts Outdated
Comment thread apps/sim/lib/core/config/appconfig.ts Outdated
Comment thread apps/sim/lib/core/config/appconfig.ts Outdated
- Remove bannedEmails sign-in check: better-auth's admin plugin already blocks banned users at sign-in (session.create.before, all providers). bannedEmails is now a signup-only denylist via user.create.before, which also closed the OAuth/email-OTP sign-in bypass the bots flagged.
- AppConfig cache: track a 'loaded' flag so an empty/unseeded profile warms the cache instead of re-polling on every request; honor NextPollIntervalInSeconds to avoid throttling; dedupe concurrent cold fetches behind one in-flight poll to avoid racing the rotating session token.
@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings in 9ee5b1f:

bannedEmails sign-in bypass (Bugbot "Email OTP skips banned list", Greptile P1 "bypassed for OAuth")
Removed the bannedEmails check from the request middleware entirely. better-auth's admin plugin already blocks banned users at sign-in via a session.create.before hook (admin.mjs) that resolves the user by id and runs on every session creation — so it covers all providers (email/password, OAuth, OTP) and auto-unbans on banExpires. bannedEmails is now a signup-only denylist, enforced in databaseHooks.user.create.before, which sees the resolved user.email and therefore also covers OAuth/OTP signup. Banning an existing account stays better-auth's job (which also deletes their API keys); the denylists are a registration gate only.

Empty AppConfig never warms cache (Bugbot, medium)
Added a loaded flag — the cache is now keyed on "has any poll completed" rather than "value !== null", so an empty/unseeded profile warms the cache instead of re-polling AWS on every request.

NextPollIntervalInSeconds ignored (Greptile P2)
poll() now sets the TTL to max(30s, NextPollIntervalInSeconds), so we never poll faster than AppConfig allows.

No cold-start dedup (Greptile)
Concurrent cold fetches now share one in-flight poll, which also avoids racing the rotating session token.

All gates green: 30 unit tests, tsc clean, full lint:check clean, check:api-validation:strict passed.

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

@greptile review

- Remove the bannedEmails denylist entirely (better-auth banning + blockedSignupDomains cover the cases).
- Move isAppConfigEnabled into feature-flags.ts and gate it on isHosted, so AppConfig is hosted-only; self-hosted/OSS always uses the env-var fallback and never constructs the AWS client.
@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

Pushed 19631cf:

  • Removed bannedEmails entirely — better-auth banning (existing accounts) + blockedSignupDomains (registration) cover what we need; the per-email denylist was redundant.
  • isAppConfigEnabled moved to feature-flags.ts and gated on isHosted — AppConfig is now hosted-only. Self-hosted/OSS always use the env-var fallback and never construct the AWS client.

All gates green: 30 tests, tsc clean, lint:check clean, check:api-validation:strict passed.

Note: the bannedEmails key can be dropped from the infra access-control profile seed/schema (infra#201) as a harmless follow-up — the app ignores unknown keys.

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

@greptile review

Comment thread apps/sim/lib/core/config/appconfig.ts
Comment thread apps/sim/lib/auth/access-control.ts
Narrow the token-resetting catch to only the network calls. A JSON/parse failure no longer discards the already-rotated session token (the round trip succeeded), so the next poll reuses it instead of opening a new StartConfigurationSession.
@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

Fixed in c66a2a4 — valid P1.

Narrowed the token-resetting catch to wrap only the network calls (StartConfigurationSession / GetLatestConfiguration). Decode + parse() now run in a separate try afterward, so a JSON/parse failure no longer discards the already-rotated NextPollConfigurationToken. The next poll reuses the valid session token instead of opening a fresh one. On a parse error we keep the last good value and back off to the normal interval (a malformed profile won't be fixed by an immediate retry).

Added a test asserting a parse error doesn't throw, warms the cache, and starts exactly one session. 31 tests, tsc/lint/api-validation all clean.

@TheodoreSpeaks

Copy link
Copy Markdown
Collaborator Author

@greptile review

@TheodoreSpeaks TheodoreSpeaks merged commit 7677479 into staging Jun 9, 2026
14 checks passed
@TheodoreSpeaks TheodoreSpeaks deleted the feat/faster-ban-user branch June 9, 2026 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant