feat(wabe): enhance security on 2FA and auth#299
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces authentication rate limiting, multi-factor authentication (MFA) with challenge tokens, file upload security validation, dynamic cookie SameSite configuration, and production-mode safety features (disabling bucket route). The changes span authentication providers, session handling, file processing, and server configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SignInResolver
participant AuthProvider
participant RateLimiter
participant MFAManager
participant Database
Client->>SignInResolver: signIn(credentials)
SignInResolver->>RateLimiter: isRateLimited(email)
alt Rate Limited
RateLimiter-->>SignInResolver: true
SignInResolver-->>Client: Error - Invalid credentials
else Not Limited
RateLimiter-->>SignInResolver: false
SignInResolver->>AuthProvider: validate credentials
alt Invalid Credentials
AuthProvider-->>SignInResolver: false
SignInResolver->>RateLimiter: registerRateLimitFailure(email)
SignInResolver-->>Client: Error - Invalid credentials
else Valid Credentials
AuthProvider-->>SignInResolver: userId
alt MFA Required
SignInResolver->>MFAManager: createMfaChallenge(userId, provider)
MFAManager->>Database: save pendingChallenges
Database-->>MFAManager: saved
MFAManager-->>SignInResolver: challengeToken
SignInResolver->>RateLimiter: clearRateLimit(email)
SignInResolver-->>Client: { user, challengeToken }
else MFA Not Required
SignInResolver->>RateLimiter: clearRateLimit(email)
SignInResolver-->>Client: { accessToken, refreshToken, user }
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/wabe/src/authentication/security.ts (1)
48-75: Consider shared storage for rate-limit state in multi-instance deployments.The module-level
Mapis per-process, so horizontally scaled instances won’t share limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wabe/src/authentication/security.ts` around lines 48 - 75, The current module-level Map rateLimitStorage (storing RateLimitState) is process-local and won't enforce limits across multiple instances; replace it with a pluggable shared store interface (e.g., IRateLimitStore with get/set/increment/expire) and update code that references rateLimitStorage to use that interface, wiring a Redis-backed implementation (or existing app cache in the WabeContext) when available; keep getRateLimitOptions as-is but ensure the rate-limiter uses the new shared store so signIn/signUp/verifyChallenge scopes share state across instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wabe/src/authentication/providers/EmailOTP.ts`:
- Line 10: The EmailOTP provider's verifyChallenge currently pre-checks rate
limits with isRateLimited but never calls registerRateLimitFailure, so failed
attempts aren't counted; update EmailOTP.verifyChallenge to call
registerRateLimitFailure (same helper used by QRCodeOTP) whenever the user is
missing or the OTP is invalid before returning null, and call clearRateLimit on
successful verification; reference the EmailOTP class and its verifyChallenge
method and the registerRateLimitFailure/isRateLimited/clearRateLimit helpers
when making the change.
In `@packages/wabe/src/authentication/providers/EmailPassword.test.ts`:
- Around line 140-181: The test currently expects the third signIn to still
result in one more DB call; update it to assert that with
signInRateLimit.maxAttempts set to 2 the third attempt is blocked and does not
invoke the DB. Specifically, keep using emailPassword.onSignIn and
mockGetObjects, record mockGetObjects.mock.calls.length after the two failing
attempts into callsBeforeBlockedAttempt, then call emailPassword.onSignIn a
third time and assert it rejects (same error if applicable) and that
mockGetObjects.mock.calls.length === callsBeforeBlockedAttempt (i.e., no
additional DB call), verifying onSignIn short-circuits before calling
mockGetObjects.
In `@packages/wabe/src/authentication/providers/EmailPassword.ts`:
- Line 8: In EmailPassword.signIn, when credentials are invalid you must call
registerRateLimitFailure with the same key used for isRateLimited/clearRateLimit
before throwing the authentication error; update the invalid-credentials branch
to invoke registerRateLimitFailure(... ) (matching the rate-limit key logic used
earlier in signIn and the pattern from signUp and PhonePassword.ts) so failed
sign-ins are tracked by the rate limiter.
In `@packages/wabe/src/authentication/providers/PhonePassword.ts`:
- Around line 77-81: In onSignUp, the rate-limiting and database checks are
inconsistent because the rateLimitKey uses normalizedPhone but subsequent
queries use input.phone; update all usages in onSignUp (including the DB
count/query and any comparisons around lines referencing the phone check) to use
the same normalizedPhone value (normalized via input.phone.trim().toLowerCase())
so the rate-limit and existence checks match; ensure you replace input.phone
with normalizedPhone in the onSignUp function and keep the same normalization
logic used in onSignIn.
- Around line 24-28: The rate-limiting uses normalizedPhone but the DB lookup
still uses raw input.phone; update the authentication flow to use
normalizedPhone everywhere so normalization is consistent: replace uses of
input.phone in the sign-in user lookup with normalizedPhone and do the same in
the onSignUp flow (where the rateLimit key is built) so rate limiting and
database queries reference the same normalized value; ensure the symbols
affected are normalizedPhone, rateLimitKey, isRateLimited(... 'signIn' ...), and
the sign-up handler onSignUp.
In `@packages/wabe/src/authentication/resolvers/signInWithResolver.ts`:
- Line 62: The MFA branch in signInWithResolver returns { accessToken,
refreshToken, user, challengeToken } but omits the srp field, causing
inconsistent response shapes; modify the MFA return to include srp (e.g., srp:
null or the appropriate SRP object) so the resolver always returns the same keys
(accessToken, refreshToken, user, challengeToken, srp), ensuring clients can
rely on a uniform response structure.
---
Nitpick comments:
In `@packages/wabe/src/authentication/security.ts`:
- Around line 48-75: The current module-level Map rateLimitStorage (storing
RateLimitState) is process-local and won't enforce limits across multiple
instances; replace it with a pluggable shared store interface (e.g.,
IRateLimitStore with get/set/increment/expire) and update code that references
rateLimitStorage to use that interface, wiring a Redis-backed implementation (or
existing app cache in the WabeContext) when available; keep getRateLimitOptions
as-is but ensure the rate-limiter uses the new shared store so
signIn/signUp/verifyChallenge scopes share state across instances.
packages/wabe/src/authentication/resolvers/signInWithResolver.ts
Outdated
Show resolved
Hide resolved
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements