From a5f99202bff065e84e5efd6cd181b3352cfe3382 Mon Sep 17 00:00:00 2001 From: Mike Date: Sun, 4 Aug 2024 20:38:20 -0400 Subject: [PATCH] implement screen name/password validation --- foodgroup/admin.go | 15 +++-- server/http/mgmt_api.go | 9 ++- server/http/mgmt_api_test.go | 116 +++++++++++++++++++++------------- state/user.go | 71 +++++++++++++++++++++ state/user_test.go | 118 +++++++++++++++++++++++++++++++++++ wire/user.go | 63 ------------------- wire/user_test.go | 108 -------------------------------- 7 files changed, 277 insertions(+), 223 deletions(-) create mode 100644 state/user_test.go delete mode 100644 wire/user_test.go diff --git a/foodgroup/admin.go b/foodgroup/admin.go index 4000cf2a..8c57b194 100644 --- a/foodgroup/admin.go +++ b/foodgroup/admin.go @@ -154,18 +154,21 @@ func (s AdminService) InfoChangeRequest(ctx context.Context, sess *state.Session // validateProposedName ensures that the name is valid var validateProposedName = func(name state.DisplayScreenName) (ok bool, errorCode uint16) { - // proposed name is too long - if len(name) > 16 { + err := name.ValidateAIMHandle() + switch { + case errors.Is(err, state.ErrAIMHandleLength): + // proposed name is too long return false, wire.AdminInfoErrorInvalidNickNameLength + case errors.Is(err, state.ErrAIMHandleInvalidFormat): + // character or spacing issues + return false, wire.AdminInfoErrorInvalidNickName } + // proposed name does not match session name (e.g. malicious client) if name.IdentScreenName() != sess.IdentScreenName() { return false, wire.AdminInfoErrorValidateNickName } - // proposed name ends in a space - if name[len(name)-1] == 32 { - return false, wire.AdminInfoErrorInvalidNickName - } + return true, 0 } diff --git a/server/http/mgmt_api.go b/server/http/mgmt_api.go index 848f6885..d5f64472 100644 --- a/server/http/mgmt_api.go +++ b/server/http/mgmt_api.go @@ -201,6 +201,12 @@ func postUserHandler(w http.ResponseWriter, r *http.Request, userManager UserMan } sn := state.DisplayScreenName(input.ScreenName) + + if err := sn.ValidateAIMHandle(); err != nil { + http.Error(w, fmt.Sprintf("invalid screen name: %s", err), http.StatusBadRequest) + return + } + user := state.User{ AuthKey: newUUID().String(), DisplayScreenName: sn, @@ -208,8 +214,7 @@ func postUserHandler(w http.ResponseWriter, r *http.Request, userManager UserMan } if err := user.HashPassword(input.Password); err != nil { - logger.Error("error hashing user password in POST /user", "err", err.Error()) - http.Error(w, "internal server error", http.StatusInternalServerError) + http.Error(w, fmt.Sprintf("invalid password: %s", err), http.StatusBadRequest) return } diff --git a/server/http/mgmt_api_test.go b/server/http/mgmt_api_test.go index 3f7c7725..655f3b30 100644 --- a/server/http/mgmt_api_test.go +++ b/server/http/mgmt_api_test.go @@ -149,35 +149,41 @@ func TestUserHandler_GET(t *testing.T) { } func TestUserHandler_POST(t *testing.T) { + type insertUserParams struct { + user state.User + err error + } tt := []struct { - name string - body string - UUID uuid.UUID - user state.User - userHandlerErr error - want string - statusCode int + name string + body string + UUID uuid.UUID + insertUserParams []insertUserParams + want string + statusCode int }{ { name: "with valid user", body: `{"screen_name":"userA", "password":"thepassword"}`, UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"), - user: func() state.User { - user := state.User{ - AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), - DisplayScreenName: "userA", - IdentScreenName: state.NewIdentScreenName("userA"), - } - assert.NoError(t, user.HashPassword("thepassword")) - return user - }(), + insertUserParams: []insertUserParams{ + { + user: func() state.User { + user := state.User{ + AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), + DisplayScreenName: "userA", + IdentScreenName: state.NewIdentScreenName("userA"), + } + assert.NoError(t, user.HashPassword("thepassword")) + return user + }(), + }, + }, want: `User account created successfully.`, statusCode: http.StatusCreated, }, { name: "with malformed body", body: `{"screen_name":"userA", "password":"thepassword"`, - user: state.User{}, want: `malformed input`, statusCode: http.StatusBadRequest, }, @@ -185,35 +191,57 @@ func TestUserHandler_POST(t *testing.T) { name: "user handler error", body: `{"screen_name":"userA", "password":"thepassword"}`, UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"), - user: func() state.User { - user := state.User{ - AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), - DisplayScreenName: "userA", - IdentScreenName: state.NewIdentScreenName("userA"), - } - assert.NoError(t, user.HashPassword("thepassword")) - return user - }(), - userHandlerErr: io.EOF, - want: `internal server error`, - statusCode: http.StatusInternalServerError, + insertUserParams: []insertUserParams{ + { + user: func() state.User { + user := state.User{ + AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), + DisplayScreenName: "userA", + IdentScreenName: state.NewIdentScreenName("userA"), + } + assert.NoError(t, user.HashPassword("thepassword")) + return user + }(), + err: io.EOF, + }, + }, + want: `internal server error`, + statusCode: http.StatusInternalServerError, }, { name: "duplicate user", body: `{"screen_name":"userA", "password":"thepassword"}`, UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"), - user: func() state.User { - user := state.User{ - AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), - DisplayScreenName: "userA", - IdentScreenName: state.NewIdentScreenName("userA"), - } - assert.NoError(t, user.HashPassword("thepassword")) - return user - }(), - userHandlerErr: state.ErrDupUser, - want: `user already exists`, - statusCode: http.StatusConflict, + insertUserParams: []insertUserParams{ + { + user: func() state.User { + user := state.User{ + AuthKey: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(), + DisplayScreenName: "userA", + IdentScreenName: state.NewIdentScreenName("userA"), + } + assert.NoError(t, user.HashPassword("thepassword")) + return user + }(), + err: state.ErrDupUser, + }, + }, + want: `user already exists`, + statusCode: http.StatusConflict, + }, + { + name: "invalid AIM screen name", + body: `{"screen_name":"a", "password":"thepassword"}`, + UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"), + want: `invalid screen name: screen name must be between 3 and 16 characters`, + statusCode: http.StatusBadRequest, + }, + { + name: "invalid AIM password", + body: `{"screen_name":"userA", "password":"1"}`, + UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"), + want: `invalid password: password length must be between 4-16 characters`, + statusCode: http.StatusBadRequest, }, } @@ -223,10 +251,10 @@ func TestUserHandler_POST(t *testing.T) { responseRecorder := httptest.NewRecorder() userManager := newMockUserManager(t) - if tc.user.IdentScreenName.String() != "" { + for _, params := range tc.insertUserParams { userManager.EXPECT(). - InsertUser(tc.user). - Return(tc.userHandlerErr) + InsertUser(params.user). + Return(params.err) } newUUID := func() uuid.UUID { return tc.UUID } diff --git a/state/user.go b/state/user.go index 72f7d4f5..3f38a1cd 100644 --- a/state/user.go +++ b/state/user.go @@ -3,7 +3,9 @@ package state import ( "bytes" "errors" + "strconv" "strings" + "unicode" "github.com/google/uuid" @@ -58,6 +60,50 @@ func NewIdentScreenName(screenName string) IdentScreenName { // This includes the original casing and spacing as defined by the user. type DisplayScreenName string +var ( + ErrAIMHandleLength = errors.New("screen name must be between 3 and 16 characters") + ErrAIMHandleInvalidFormat = errors.New("screen name must start with a letter, cannot end with a space, and must contain only letters, numbers, and spaces") + ErrICQUINInvalidFormat = errors.New("uin must be a number in the range 10000-2147483646") +) + +// ValidateAIMHandle returns an error if the instance is not a valid AIM screen name. +// Possible errors: +// - ErrAIMHandleLength: if the screen name is not between 3 and 16 +// characters +// - ErrAIMHandleInvalidFormat: if the screen name does not start with a +// letter, ends with a space, or contains invalid characters +func (s DisplayScreenName) ValidateAIMHandle() error { + if len(s) < 3 || len(s) > 16 { + return ErrAIMHandleLength + } + + // Must start with a letter, cannot end with a space, + // and must contain only letters, numbers, and spaces + if !unicode.IsLetter(rune(s[0])) || s[len(s)-1] == ' ' { + return ErrAIMHandleInvalidFormat + } + + for _, ch := range s { + if !unicode.IsLetter(ch) && !unicode.IsDigit(ch) && ch != ' ' { + return ErrAIMHandleInvalidFormat + } + } + + return nil +} + +// ValidateICQHandle returns an error if the instance is not a valid ICQ UIN. +// Possible errors: +// - ErrICQUINInvalidFormat: if the UIN is not a number or is not in the valid +// range +func (s DisplayScreenName) ValidateICQHandle() error { + uin, err := strconv.Atoi(string(s)) + if err != nil || uin < 10000 || uin > 2147483646 { + return ErrICQUINInvalidFormat + } + return nil +} + // IdentScreenName converts the DisplayScreenName to an IdentScreenName by applying // the normalization process defined in NewIdentScreenName. func (s DisplayScreenName) IdentScreenName() IdentScreenName { @@ -118,7 +164,32 @@ func (u *User) ValidateRoastedPass(roastedPass []byte) bool { // HashPassword computes MD5 hashes of the user's password. It computes both // weak and strong variants and stores them in the struct. func (u *User) HashPassword(passwd string) error { + if err := validateAIMPassword(passwd); err != nil { + return err + } u.WeakMD5Pass = wire.WeakMD5PasswordHash(passwd, u.AuthKey) u.StrongMD5Pass = wire.StrongMD5PasswordHash(passwd, u.AuthKey) return nil } + +// validateAIMPassword returns an error if the AIM password is invalid. +// A valid password is 4-16 characters long. The minimum password length is +// set here for software preservation purposes; operators should set more +// stringent password requirements. +func validateAIMPassword(pass string) error { + if len(pass) < 4 || len(pass) > 16 { + return errors.New("password length must be between 4-16 characters") + } + return nil +} + +// validateICQPassword returns an error if the ICQ password is invalid. +// A valid password is 1-8 characters long. The minimum password length is set +// here for software preservation purposes; operators should set more stringent +// password requirements. +func validateICQPassword(pass string) error { + if len(pass) < 1 || len(pass) > 8 { + return errors.New("password must be between 1 and 8 characters") + } + return nil +} diff --git a/state/user_test.go b/state/user_test.go new file mode 100644 index 00000000..04132070 --- /dev/null +++ b/state/user_test.go @@ -0,0 +1,118 @@ +package state + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mk6i/retro-aim-server/wire" +) + +func TestUser_HashPassword(t *testing.T) { + tests := []struct { + name string + user User + password string + expectedWeakMD5 []byte + expectedStrongMD5 []byte + wantError bool + }{ + { + name: "Valid password", + user: User{AuthKey: "someAuthKey"}, + password: "validPassword", + expectedWeakMD5: wire.WeakMD5PasswordHash("validPassword", "someAuthKey"), + expectedStrongMD5: wire.StrongMD5PasswordHash("validPassword", "someAuthKey"), + wantError: false, + }, + { + name: "Empty password", + user: User{AuthKey: "someAuthKey"}, + password: "", + expectedWeakMD5: nil, + expectedStrongMD5: nil, + wantError: true, + }, + { + name: "Password too short", + user: User{AuthKey: "someAuthKey"}, + password: "abc", + expectedWeakMD5: nil, + expectedStrongMD5: nil, + wantError: true, + }, + { + name: "Password too long", + user: User{AuthKey: "someAuthKey"}, + password: "thispasswordistoolong", + expectedWeakMD5: nil, + expectedStrongMD5: nil, + wantError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.user.HashPassword(tt.password) + + if tt.wantError { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.expectedWeakMD5, tt.user.WeakMD5Pass) + assert.Equal(t, tt.expectedStrongMD5, tt.user.StrongMD5Pass) + } + }) + } +} + +func TestDisplayScreenName_ValidateAIMHandle(t *testing.T) { + tests := []struct { + name string + input DisplayScreenName + wantErr error + }{ + {"Valid handle", "User123", nil}, + {"Too short", "Us", ErrAIMHandleLength}, + {"Too long", "ThisIsAReallyLongScreenName", ErrAIMHandleLength}, + {"Starts with number", "1User", ErrAIMHandleInvalidFormat}, + {"Ends with space", "User123 ", ErrAIMHandleInvalidFormat}, + {"Contains invalid character", "User@123", ErrAIMHandleInvalidFormat}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.input.ValidateAIMHandle() + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr, "ValidateAIMHandle() error = %v, wantErr %v", err, tt.wantErr) + } else { + assert.NoError(t, err, "ValidateAIMHandle() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestDisplayScreenName_ValidateICQHandle(t *testing.T) { + tests := []struct { + name string + input DisplayScreenName + wantErr error + }{ + {"Valid UIN", "123456", nil}, + {"Too low", "9999", ErrICQUINInvalidFormat}, + {"Too high", "2147483647", ErrICQUINInvalidFormat}, + {"Non-numeric", "abcd", ErrICQUINInvalidFormat}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.input.ValidateICQHandle() + if tt.wantErr != nil { + assert.ErrorIs(t, err, tt.wantErr, "ValidateICQHandle() error = %v, wantErr %v", err, tt.wantErr) + } else { + assert.NoError(t, err, "ValidateICQHandle() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/wire/user.go b/wire/user.go index 7939e069..8346ee4d 100644 --- a/wire/user.go +++ b/wire/user.go @@ -2,9 +2,7 @@ package wire import ( "crypto/md5" - "errors" "io" - "unicode" ) // WeakMD5PasswordHash hashes password and authKey for AIM v3.5-v4.7. @@ -45,64 +43,3 @@ func RoastPassword(roastedPass []byte) []byte { } return clearPass } - -// ValidateAIMHandle returns an error if the AIM screen name is invalid. -// A valid screen name meets the following criteria: -// - 3-16 letters/numbers -// - must start with a letter -// - doesn't end with a space -func ValidateAIMHandle(screenName string) error { - if len(screenName) < 3 || len(screenName) > 16 { - return errors.New("screen name must be between 3 and 16 characters") - } - - // Must start with a letter - if !unicode.IsLetter(rune(screenName[0])) { - return errors.New("screen name must start with a letter") - } - - // Cannot end with a space - if screenName[len(screenName)-1] == ' ' { - return errors.New("screen name cannot end with a space") - } - - // Must contain only letters, numbers, and spaces - for _, ch := range screenName { - if !unicode.IsLetter(ch) && !unicode.IsDigit(ch) && ch != ' ' { - return errors.New("screen name must contain only letters, numbers, and spaces") - } - } - - return nil -} - -// ValidateAIMPassword returns an error if the AIM password is invalid. -// A valid password is 4-16 characters long. The minimum password length is -// set here for software preservation purposes; operators should set more -// stringent password requirements. -func ValidateAIMPassword(pass string) error { - if len(pass) < 4 || len(pass) > 16 { - return errors.New("password must be between 4 and 16 characters") - } - return nil -} - -// ValidateICQHandle returns an error if the ICQ UIN is invalid. -// A valid UIN is a number in the range 10000-2147483646. -func ValidateICQHandle(UIN uint32) error { - if UIN < 10000 || UIN > 2147483646 { - return errors.New("UIN must be between 10000 and 2147483646") - } - return nil -} - -// ValidateICQPassword returns an error if the ICQ password is invalid. -// A valid password is 1-8 characters long. The minimum password length is set -// here for software preservation purposes; operators should set more stringent -// password requirements. -func ValidateICQPassword(pass string) error { - if len(pass) < 1 || len(pass) > 8 { - return errors.New("password must be between 1 and 8 characters") - } - return nil -} diff --git a/wire/user_test.go b/wire/user_test.go deleted file mode 100644 index 9516e93f..00000000 --- a/wire/user_test.go +++ /dev/null @@ -1,108 +0,0 @@ -package wire - -import ( - "errors" - "fmt" - "testing" -) - -func TestValidateAIMHandle(t *testing.T) { - tests := []struct { - screenName string - expected error - }{ - {"ValidName", nil}, - {"Valid Name", nil}, - {"Val1dName", nil}, - {"Va", errors.New("screen name must be between 3 and 16 characters")}, - {"ThisIsAVeryLongScreenName", errors.New("screen name must be between 3 and 16 characters")}, - {"1Invalid", errors.New("screen name must start with a letter")}, - {"Invalid ", errors.New("screen name cannot end with a space")}, - {"Inval!dName", errors.New("screen name must contain only letters, numbers, and spaces")}, - } - - for _, tt := range tests { - t.Run(tt.screenName, func(t *testing.T) { - err := ValidateAIMHandle(tt.screenName) - if err != nil && err.Error() != tt.expected.Error() { - t.Errorf("got %v, want %v", err, tt.expected) - } - if err == nil && tt.expected != nil { - t.Errorf("got %v, want %v", err, tt.expected) - } - }) - } -} - -func TestValidateAIMPassword(t *testing.T) { - tests := []struct { - password string - expected error - }{ - {"pass1234", nil}, - {"1234", nil}, - {"pa", errors.New("password must be between 4 and 16 characters")}, - {"thisisaverylongpassword", errors.New("password must be between 4 and 16 characters")}, - } - - for _, tt := range tests { - t.Run(tt.password, func(t *testing.T) { - err := ValidateAIMPassword(tt.password) - if err != nil && err.Error() != tt.expected.Error() { - t.Errorf("got %v, want %v", err, tt.expected) - } - if err == nil && tt.expected != nil { - t.Errorf("got %v, want %v", err, tt.expected) - } - }) - } -} - -func TestValidateICQHandle(t *testing.T) { - tests := []struct { - UIN uint32 - expected error - }{ - {123456, nil}, - {10000, nil}, - {2147483646, nil}, - {9999, errors.New("UIN must be between 10000 and 2147483646")}, - {2147483647, errors.New("UIN must be between 10000 and 2147483646")}, - } - - for _, tt := range tests { - t.Run(fmt.Sprintf("%d", tt.UIN), func(t *testing.T) { - err := ValidateICQHandle(tt.UIN) - if err != nil && err.Error() != tt.expected.Error() { - t.Errorf("got %v, want %v", err, tt.expected) - } - if err == nil && tt.expected != nil { - t.Errorf("got %v, want %v", err, tt.expected) - } - }) - } -} - -func TestValidateICQPassword(t *testing.T) { - tests := []struct { - password string - expected error - }{ - {"pass", nil}, - {"12345678", nil}, - {"", errors.New("password must be between 1 and 8 characters")}, - {"123456789", errors.New("password must be between 1 and 8 characters")}, - } - - for _, tt := range tests { - t.Run(tt.password, func(t *testing.T) { - err := ValidateICQPassword(tt.password) - if err != nil && err.Error() != tt.expected.Error() { - t.Errorf("got %v, want %v", err, tt.expected) - } - if err == nil && tt.expected != nil { - t.Errorf("got %v, want %v", err, tt.expected) - } - }) - } -}