diff --git a/backend/internal/common/errors.go b/backend/internal/common/errors.go index 14536541..1d6a03c6 100644 --- a/backend/internal/common/errors.go +++ b/backend/internal/common/errors.go @@ -124,6 +124,13 @@ type MissingSessionIdError struct{} func (e MissingSessionIdError) Error() string { return "Missing session id" } func (e MissingSessionIdError) HttpStatusCode() int { return http.StatusBadRequest } +type InvalidWebauthnSessionError struct{} + +func (e InvalidWebauthnSessionError) Error() string { + return "WebAuthn session is invalid or has expired" +} +func (e InvalidWebauthnSessionError) HttpStatusCode() int { return http.StatusBadRequest } + type ReservedClaimError struct { Key string } diff --git a/backend/internal/webauthn/service.go b/backend/internal/webauthn/service.go index 07a1e247..159f8f09 100644 --- a/backend/internal/webauthn/service.go +++ b/backend/internal/webauthn/service.go @@ -130,13 +130,15 @@ func (s *Service) VerifyRegistration(ctx context.Context, sessionID string, user // Load & delete the session row var storedSession WebauthnSession - err := tx. + result := tx. WithContext(ctx). Clauses(clause.Returning{}). - Delete(&storedSession, "id = ?", sessionID). - Error - if err != nil { - return model.WebauthnCredential{}, fmt.Errorf("failed to load WebAuthn session: %w", err) + Delete(&storedSession, "id = ?", sessionID) + if result.Error != nil { + return model.WebauthnCredential{}, fmt.Errorf("failed to load WebAuthn session: %w", result.Error) + } + if result.RowsAffected == 0 { + return model.WebauthnCredential{}, &common.InvalidWebauthnSessionError{} } session := gowebauthn.SessionData{ @@ -148,7 +150,7 @@ func (s *Service) VerifyRegistration(ctx context.Context, sessionID string, user } var user model.User - err = tx. + err := tx. WithContext(ctx). Find(&user, "id = ?", userID). Error @@ -238,13 +240,15 @@ func (s *Service) VerifyLogin(ctx context.Context, dbConfig *appconfig.AppConfig // Load & delete the session row var storedSession WebauthnSession - err := tx. + result := tx. WithContext(ctx). Clauses(clause.Returning{}). - Delete(&storedSession, "id = ?", sessionID). - Error - if err != nil { - return model.User{}, "", fmt.Errorf("failed to load WebAuthn session: %w", err) + Delete(&storedSession, "id = ?", sessionID) + if result.Error != nil { + return model.User{}, "", fmt.Errorf("failed to load WebAuthn session: %w", result.Error) + } + if result.RowsAffected == 0 { + return model.User{}, "", &common.InvalidWebauthnSessionError{} } session := gowebauthn.SessionData{ @@ -255,7 +259,7 @@ func (s *Service) VerifyLogin(ctx context.Context, dbConfig *appconfig.AppConfig } var user *model.User - _, err = s.webAuthn.ValidateDiscoverableLogin(func(_, userHandle []byte) (gowebauthn.User, error) { + _, err := s.webAuthn.ValidateDiscoverableLogin(func(_, userHandle []byte) (gowebauthn.User, error) { innerErr := tx. WithContext(ctx). Preload("Credentials"). @@ -442,13 +446,15 @@ func (s *Service) CreateReauthenticationTokenWithWebauthn(ctx context.Context, s // Retrieve and delete the session var storedSession WebauthnSession - err := tx. + result := tx. WithContext(ctx). Clauses(clause.Returning{}). - Delete(&storedSession, "id = ? AND expires_at > ?", sessionID, datatype.DateTime(time.Now())). - Error - if err != nil { - return "", fmt.Errorf("failed to load WebAuthn session: %w", err) + Delete(&storedSession, "id = ? AND expires_at > ?", sessionID, datatype.DateTime(time.Now())) + if result.Error != nil { + return "", fmt.Errorf("failed to load WebAuthn session: %w", result.Error) + } + if result.RowsAffected == 0 { + return "", &common.InvalidWebauthnSessionError{} } session := gowebauthn.SessionData{ @@ -460,7 +466,7 @@ func (s *Service) CreateReauthenticationTokenWithWebauthn(ctx context.Context, s // Validate the credential assertion var user *model.User - _, err = s.webAuthn.ValidateDiscoverableLogin(func(_, userHandle []byte) (gowebauthn.User, error) { + _, err := s.webAuthn.ValidateDiscoverableLogin(func(_, userHandle []byte) (gowebauthn.User, error) { innerErr := tx. WithContext(ctx). Preload("Credentials"). diff --git a/backend/internal/webauthn/service_test.go b/backend/internal/webauthn/service_test.go index 1ccfeece..b5c08915 100644 --- a/backend/internal/webauthn/service_test.go +++ b/backend/internal/webauthn/service_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/go-webauthn/webauthn/protocol" "github.com/lestrrat-go/jwx/v3/jwt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -132,6 +133,74 @@ func TestWebAuthnDisplayNameUsesRequestConfig(t *testing.T) { require.Equal(t, "Custom App", service.webAuthn.Config.RPDisplayName) } +// A ceremony that references a session which does not exist must be rejected outright +// The delete-and-return leaves the struct zero-valued when nothing matched, and a zero session has an +// empty user verification requirement, a zero expiry and an empty challenge, so letting it reach the +// library would validate the assertion with user verification and expiry enforcement silently disabled +func TestCeremoniesRejectSessionThatDoesNotExist(t *testing.T) { + const userID = "ceremony-user" + + setupService := func(t *testing.T) *Service { + t.Helper() + + db := testutils.NewDatabaseForTest(t) + require.NoError(t, db.Create(&model.User{ + Base: model.Base{ID: userID}, + Username: userID, + }).Error) + + return &Service{db: db} + } + + t.Run("registration rejects an unknown session", func(t *testing.T) { + service := setupService(t) + + _, err := service.VerifyRegistration(t.Context(), "does-not-exist", userID, nil, "127.0.0.1") + + require.Error(t, err) + assert.ErrorAs(t, err, new(*common.InvalidWebauthnSessionError)) + }) + + t.Run("login rejects an unknown session", func(t *testing.T) { + service := setupService(t) + + _, token, err := service.VerifyLogin(t.Context(), &appconfig.AppConfigModel{}, "does-not-exist", nil, "127.0.0.1", "test-agent") + + assert.Empty(t, token) + require.Error(t, err) + assert.ErrorAs(t, err, new(*common.InvalidWebauthnSessionError)) + }) + + t.Run("reauthentication rejects an unknown session", func(t *testing.T) { + service := setupService(t) + + token, err := service.CreateReauthenticationTokenWithWebauthn(t.Context(), "does-not-exist", nil) + + assert.Empty(t, token) + require.Error(t, err) + assert.ErrorAs(t, err, new(*common.InvalidWebauthnSessionError)) + }) + + // The reauthentication query filters expired rows out in SQL, so an expired session matches no row + // and previously produced the same zero-valued session as an unknown one + t.Run("reauthentication rejects an expired session", func(t *testing.T) { + service := setupService(t) + + expiredSession := WebauthnSession{ + Challenge: "expired-challenge", + ExpiresAt: datatype.DateTime(time.Now().Add(-time.Minute)), + UserVerification: string(protocol.VerificationRequired), + } + require.NoError(t, service.db.Create(&expiredSession).Error) + + token, err := service.CreateReauthenticationTokenWithWebauthn(t.Context(), expiredSession.ID, nil) + + assert.Empty(t, token) + require.Error(t, err) + assert.ErrorAs(t, err, new(*common.InvalidWebauthnSessionError)) + }) +} + func TestConsumeReauthenticationTokenReturnsTokenCreationTime(t *testing.T) { db := testutils.NewDatabaseForTest(t) service := &Service{db: db}