Skip to content

Commit

Permalink
fix: check password max length in checkPasswordStrength (supabase#1659)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Check if password exceeds max length in `checkPasswordStrength`

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
  • Loading branch information
kangmingtay authored and LashaJini committed Nov 13, 2024
1 parent 383cefe commit 7275c8d
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 23 deletions.
5 changes: 5 additions & 0 deletions internal/api/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import (
"github.com/fatih/structs"
"github.com/go-chi/chi/v5"
"github.com/gofrs/uuid"
"github.com/pkg/errors"
"github.com/sethvargo/go-password/password"
"github.com/supabase/auth/internal/api/provider"
"github.com/supabase/auth/internal/models"
"github.com/supabase/auth/internal/observability"
"github.com/supabase/auth/internal/storage"
"golang.org/x/crypto/bcrypt"
)

type AdminUserParams struct {
Expand Down Expand Up @@ -382,6 +384,9 @@ func (a *API) adminUserCreate(w http.ResponseWriter, r *http.Request) error {
}

if err != nil {
if errors.Is(err, bcrypt.ErrPasswordTooLong) {
return badRequestError(ErrorCodeValidationFailed, err.Error())
}
return internalServerError("Error creating user").WithInternalError(err)
}

Expand Down
7 changes: 7 additions & 0 deletions internal/api/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import (
"github.com/sirupsen/logrus"
)

// BCrypt hashed passwords have a 72 character limit
const MaxPasswordLength = 72

// WeakPasswordError encodes an error that a password does not meet strength
// requirements. It is handled specially in errors.go as it gets transformed to
// a HTTPError with a special weak_password field that encodes the Reasons
Expand All @@ -24,6 +27,10 @@ func (e *WeakPasswordError) Error() string {
func (a *API) checkPasswordStrength(ctx context.Context, password string) error {
config := a.config

if len(password) > MaxPasswordLength {
return badRequestError(ErrorCodeValidationFailed, fmt.Sprintf("Password cannot be longer than %v characters", MaxPasswordLength))
}

var messages, reasons []string

if len(password) < config.Password.MinLength {
Expand Down
16 changes: 13 additions & 3 deletions internal/api/password_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,12 @@ func TestPasswordStrengthChecks(t *testing.T) {
Password: "abc123",
Reasons: nil,
},
{
MinLength: 6,
RequiredCharacters: []string{},
Password: "zZgXb5gzyCNrV36qwbOSbKVQsVJd28mC1TwRpeB0y6sFNICJyjD6bILKJMsjyKDzBdaY5tmi8zY9BWJYmt3vULLmyafjIDLYjy8qhETu0mS2jj1uQBgSAzJn9Zjm8EFa",
Reasons: nil,
},
}

for i, example := range examples {
Expand All @@ -98,10 +104,14 @@ func TestPasswordStrengthChecks(t *testing.T) {
}

err := api.checkPasswordStrength(context.Background(), example.Password)
if example.Reasons == nil {

switch e := err.(type) {
case *WeakPasswordError:
require.Equal(t, e.Reasons, example.Reasons, "Example %d failed with wrong reasons", i)
case *HTTPError:
require.Equal(t, e.ErrorCode, ErrorCodeValidationFailed, "Example %d failed with wrong error code", i)
default:
require.NoError(t, err, "Example %d failed with error", i)
} else {
require.Equal(t, err.(*WeakPasswordError).Reasons, example.Reasons, "Example %d failed with wrong reasons", i)
}
}
}
7 changes: 0 additions & 7 deletions internal/crypto/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ const (
// useful for tests only.
QuickHashCost HashCost = iota

// BCrypt hashed passwords have a 72 character limit
MaxPasswordLength = 72

Argon2Prefix = "$argon2"
)

Expand Down Expand Up @@ -214,10 +211,6 @@ func CompareHashAndPassword(ctx context.Context, hash, password string) error {
func GenerateFromPassword(ctx context.Context, password string) (string, error) {
var hashCost int

if len(password) > MaxPasswordLength {
return "", fmt.Errorf("password cannot be longer than %d characters", MaxPasswordLength)
}

switch PasswordHashCost {
case QuickHashCost:
hashCost = bcrypt.MinCost
Expand Down
13 changes: 0 additions & 13 deletions internal/models/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package models

import (
"context"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -369,18 +368,6 @@ func (ts *UserTestSuite) TestUpdateUserEmailFailure() {
require.Equal(ts.T(), primaryIdentity.GetEmail(), userA.GetEmail())
}

func (ts *UserTestSuite) TestSetPasswordTooLong() {
user, err := NewUser("", "", strings.Repeat("a", crypto.MaxPasswordLength), "", nil)
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.db.Create(user))

err = user.SetPassword(ts.db.Context(), strings.Repeat("a", crypto.MaxPasswordLength+1), false, "", "")
require.Error(ts.T(), err)

err = user.SetPassword(ts.db.Context(), strings.Repeat("a", crypto.MaxPasswordLength), false, "", "")
require.NoError(ts.T(), err)
}

func (ts *UserTestSuite) TestNewUserWithPasswordHashSuccess() {
cases := []struct {
desc string
Expand Down

0 comments on commit 7275c8d

Please sign in to comment.