From 5d1f5a88ee2cca72ac0063a3270cf0d250274e2b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 23:06:09 +0200 Subject: [PATCH] sec(auth): rate-limit MFA lifecycle endpoints per IP and per user The four /api/auth/mfa/* routes (setup, enable, disable, regenerate-recovery-codes) verify a password, TOTP or recovery code but never called the rate limiter, so a stolen session could brute-force the second factor without limit. Add an "mfa" bucket (10 attempts / 15 min) and apply it through requireMFASession, which checks the bucket per IP and per session user, both fail-closed like the other credential endpoints. Closes #94 --- internal/api/handler.go | 1 + internal/api/handler_auth.go | 8 +-- internal/api/handler_auth_test.go | 85 +++++++++++++++++++++++++++++++ internal/api/middleware.go | 28 ++++++++++ internal/api/rate_limiter.go | 4 ++ 5 files changed, 122 insertions(+), 4 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 52bac12cb..cb4ad18c1 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -212,6 +212,7 @@ func NewHandler(cfg HandlerConfig) *Handler { "change_password", "register", "approve_cancel_public", + "mfa", ) return h diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 3e3c89224..a244ee3d0 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -552,7 +552,7 @@ func mapMFAServiceError(err error) error { // the body (base64-encoded, same convention as login). Returns the // generated secret + otpauth provisioning URI for QR display. func (h *Handler) mfaSetup(ctx context.Context, req *events.LambdaFunctionURLRequest) (*MFASetupResponse, error) { - session, err := h.requireSession(ctx, req) + session, err := h.requireMFASession(ctx, req) if err != nil { return nil, err } @@ -576,7 +576,7 @@ func (h *Handler) mfaSetup(ctx context.Context, req *events.LambdaFunctionURLReq // TOTP code against the pending secret + flips MFAEnabled=true on // success. Returns the plaintext recovery codes once. func (h *Handler) mfaEnable(ctx context.Context, req *events.LambdaFunctionURLRequest) (*MFAEnableResponse, error) { - session, err := h.requireSession(ctx, req) + session, err := h.requireMFASession(ctx, req) if err != nil { return nil, err } @@ -597,7 +597,7 @@ func (h *Handler) mfaEnable(ctx context.Context, req *events.LambdaFunctionURLRe // code). On success: clears the secret + recovery codes, flips // MFAEnabled=false. func (h *Handler) mfaDisable(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { - session, err := h.requireSession(ctx, req) + session, err := h.requireMFASession(ctx, req) if err != nil { return nil, err } @@ -622,7 +622,7 @@ func (h *Handler) mfaDisable(ctx context.Context, req *events.LambdaFunctionURLR // recovery codes with a fresh batch. Requires a current TOTP code // (NOT a recovery code). func (h *Handler) mfaRegenerateRecoveryCodes(ctx context.Context, req *events.LambdaFunctionURLRequest) (*MFARegenerateResponse, error) { - session, err := h.requireSession(ctx, req) + session, err := h.requireMFASession(ctx, req) if err != nil { return nil, err } diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 639a82f21..c85904715 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1635,3 +1635,88 @@ func TestMapMFAServiceError_AuthFailed_Is401(t *testing.T) { assert.Equal(t, 401, ce.code, "ErrMFAAuthFailed must map to HTTP 401") assert.Contains(t, ce.Error(), auth.ErrMFAAuthFailed.Error()) } + +// Issue #94: every MFA route that checks a password, TOTP or recovery code +// must be throttled per IP and per session user. +func TestHandler_mfaEndpoints_RateLimited(t *testing.T) { + limit := getDefaultRateLimits()["mfa"].MaxAttempts + invalid := auth.ErrMFAInvalidCode + cases := []struct { + name string + method string + args []any + ret []any + call func(h *Handler, req *events.LambdaFunctionURLRequest) error + body string + }{ + {"setup", "MFASetupAPI", []any{"user-1", "pw"}, []any{"", "", invalid}, + func(h *Handler, r *events.LambdaFunctionURLRequest) error { + _, err := h.mfaSetup(context.Background(), r) + return err + }, + `{"password":"` + b64("pw") + `"}`}, + {"enable", "MFAEnableAPI", []any{"user-1", "000000"}, []any{[]string(nil), invalid}, + func(h *Handler, r *events.LambdaFunctionURLRequest) error { + _, err := h.mfaEnable(context.Background(), r) + return err + }, + `{"code":"000000"}`}, + {"disable", "MFADisableAPI", []any{"user-1", "pw", "000000"}, []any{invalid}, + func(h *Handler, r *events.LambdaFunctionURLRequest) error { + _, err := h.mfaDisable(context.Background(), r) + return err + }, + `{"password":"` + b64("pw") + `","code":"000000"}`}, + {"regenerate", "MFARegenerateRecoveryCodesAPI", []any{"user-1", "000000"}, []any{[]string(nil), invalid}, + func(h *Handler, r *events.LambdaFunctionURLRequest) error { + _, err := h.mfaRegenerateRecoveryCodes(context.Background(), r) + return err + }, + `{"code":"000000"}`}, + } + for _, tc := range cases { + for _, rotateIP := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/rotateIP=%v", tc.name, rotateIP), func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", mock.Anything, "tok"). + Return(&Session{UserID: "user-1", Email: "u@x.com"}, nil) + mockAuth.On(tc.method, append([]any{mock.Anything}, tc.args...)...). + Return(tc.ret...).Times(limit) + h := &Handler{auth: mockAuth, rateLimiter: NewInMemoryRateLimiter()} + + for i := 0; i <= limit; i++ { + req := authedReq("tok", tc.body) + req.RequestContext.HTTP.SourceIP = "198.51.100.1" + if rotateIP { + req.RequestContext.HTTP.SourceIP = fmt.Sprintf("198.51.100.%d", i+1) + } + err := tc.call(h, req) + ce, ok := IsClientError(err) + require.True(t, ok, "attempt %d: %v", i+1, err) + if i < limit { + assert.Equal(t, 400, ce.code, "attempt %d", i+1) + } else { + assert.Equal(t, 429, ce.code, "attempt %d must be throttled", i+1) + } + } + }) + } + } +} + +func TestHandler_mfaEnable_RateLimiterErrorFailsClosed(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", mock.Anything, "tok").Return(&Session{UserID: "user-1"}, nil) + rl := new(MockRateLimiter) + t.Cleanup(func() { rl.AssertExpectations(t) }) + rl.On("AllowWithIP", mock.Anything, mock.Anything, "mfa").Return(true, nil) + rl.On("AllowWithUser", mock.Anything, "user-1", "mfa").Return(false, errors.New("db down")) + h := &Handler{auth: mockAuth, rateLimiter: rl} + + _, err := h.mfaEnable(context.Background(), authedReq("tok", `{"code":"000000"}`)) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 503, ce.code) +} diff --git a/internal/api/middleware.go b/internal/api/middleware.go index c68b5d399..95f3abbc2 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -569,3 +569,31 @@ func (h *Handler) checkRateLimitStrict(ctx context.Context, req *events.LambdaFu } return nil } + +// requireMFASession is requireSession plus the "mfa" bucket applied per IP and +// then per session user, both fail-closed, so a stolen session cannot reset its +// guessing budget by rotating IPs (#94). +func (h *Handler) requireMFASession(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { + const endpoint = "mfa" + if err := h.checkRateLimitStrict(ctx, req, endpoint); err != nil { + return nil, err + } + session, err := h.requireSession(ctx, req) + if err != nil { + return nil, err + } + if h.rateLimiter == nil { + return session, nil + } + allowed, err := h.rateLimiter.AllowWithUser(ctx, session.UserID, endpoint) + if err != nil { + logging.Errorf("ALERT: rate limiter error on credential endpoint %s for user %s; request denied (02-M1): %v", + endpoint, session.UserID, err) + return nil, NewClientError(503, "service temporarily unavailable, please try again later") + } + if !allowed { + logging.Warnf("Rate limit exceeded for %s for user: %s", endpoint, session.UserID) + return nil, NewClientError(429, "too many requests, please try again later") + } + return session, nil +} diff --git a/internal/api/rate_limiter.go b/internal/api/rate_limiter.go index 722c644e5..65bd4ba93 100644 --- a/internal/api/rate_limiter.go +++ b/internal/api/rate_limiter.go @@ -40,6 +40,10 @@ func getDefaultRateLimits() map[string]RateLimitConfig { // api_general (300/min), enabling bulk table-flooding and inbox-spam. Limit // to 5 per 15 minutes per IP, matching setup_admin severity. "register": NewRateLimitConfig(5, 15*60), // 5 attempts / 15 minutes / IP (#1016) + // mfa covers every /api/auth/mfa/* route that checks a password, TOTP or + // recovery code. Applied per IP AND per session user (#94), so rotating + // source IPs does not reset one account's guessing budget. + "mfa": NewRateLimitConfig(10, 15*60), // 10 attempts / 15 minutes / IP and / user (#94) } }