diff --git a/client/ssh/client/client_test.go b/client/ssh/client/client_test.go index 96d2c8a25..53cde8bef 100644 --- a/client/ssh/client/client_test.go +++ b/client/ssh/client/client_test.go @@ -515,18 +515,19 @@ func getCurrentUsername() string { return "test-user" } -// isCI checks if we're running in a CI environment +// isCI checks if we're running in GitHub Actions CI func isCI() bool { - ciEnvVars := []string{ - "CI", "CONTINUOUS_INTEGRATION", "GITHUB_ACTIONS", - "GITLAB_CI", "JENKINS_URL", "BUILDKITE", "CIRCLECI", + // Check standard CI environment variables + if os.Getenv("GITHUB_ACTIONS") == "true" || os.Getenv("CI") == "true" { + return true } - for _, envVar := range ciEnvVars { - if os.Getenv(envVar) != "" { - return true - } + // Check for GitHub Actions runner hostname pattern (when running as SYSTEM) + hostname, err := os.Hostname() + if err == nil && strings.HasPrefix(hostname, "runner") { + return true } + return false } diff --git a/client/ssh/server/compatibility_test.go b/client/ssh/server/compatibility_test.go index fa342283e..ab7838798 100644 --- a/client/ssh/server/compatibility_test.go +++ b/client/ssh/server/compatibility_test.go @@ -744,16 +744,17 @@ func getTestUsername(t *testing.T) string { // isCI checks if we're running in a CI environment func isCI() bool { - ciEnvVars := []string{ - "CI", "CONTINUOUS_INTEGRATION", "GITHUB_ACTIONS", - "GITLAB_CI", "JENKINS_URL", "BUILDKITE", "CIRCLECI", + // Check standard CI environment variables + if os.Getenv("GITHUB_ACTIONS") == "true" || os.Getenv("CI") == "true" { + return true } - for _, envVar := range ciEnvVars { - if os.Getenv(envVar) != "" { - return true - } + // Check for GitHub Actions runner hostname pattern (when running as SYSTEM) + hostname, err := os.Hostname() + if err == nil && strings.HasPrefix(hostname, "runner") { + return true } + return false } diff --git a/client/ssh/server/user_utils_test.go b/client/ssh/server/user_utils_test.go index abcd1f24f..637dc10d0 100644 --- a/client/ssh/server/user_utils_test.go +++ b/client/ssh/server/user_utils_test.go @@ -544,14 +544,18 @@ func TestIsSameUser(t *testing.T) { } } -func TestUsernameValidation(t *testing.T) { +func TestUsernameValidation_Unix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix-specific username validation tests") + } + tests := []struct { name string username string wantErr bool errMsg string }{ - // Valid usernames + // Valid usernames (Unix/POSIX) {"valid_alphanumeric", "user123", false, ""}, {"valid_with_dots", "user.name", false, ""}, {"valid_with_hyphens", "user-name", false, ""}, @@ -560,29 +564,88 @@ func TestUsernameValidation(t *testing.T) { {"valid_starting_with_digit", "123user", false, ""}, {"valid_starting_with_dot", ".hidden", false, ""}, - // Invalid usernames + // Invalid usernames (Unix/POSIX) {"empty_username", "", true, "username cannot be empty"}, {"username_too_long", "thisusernameiswaytoolongandexceedsthe32characterlimit", true, "username too long"}, - {"username_starting_with_hyphen", "-user", true, "invalid characters"}, + {"username_starting_with_hyphen", "-user", true, "invalid characters"}, // POSIX restriction {"username_with_spaces", "user name", true, "invalid characters"}, {"username_with_shell_metacharacters", "user;rm", true, "invalid characters"}, {"username_with_command_injection", "user`rm -rf /`", true, "invalid characters"}, {"username_with_pipe", "user|rm", true, "invalid characters"}, {"username_with_ampersand", "user&rm", true, "invalid characters"}, {"username_with_quotes", "user\"name", true, "invalid characters"}, - {"username_with_backslash", "user\\name", true, "invalid characters"}, {"username_with_newline", "user\nname", true, "invalid characters"}, {"reserved_dot", ".", true, "cannot be '.' or '..'"}, {"reserved_dotdot", "..", true, "cannot be '.' or '..'"}, + {"username_with_at_symbol", "user@domain", true, "invalid characters"}, // Not allowed in bare Unix usernames + {"username_with_backslash", "user\\name", true, "invalid characters"}, // Not allowed in Unix usernames } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Skip hyphen test on Windows - Windows allows usernames starting with hyphens - if tt.name == "username_starting_with_hyphen" && runtime.GOOS == "windows" { - t.Skip("Windows allows usernames starting with hyphens") + err := validateUsername(tt.username) + if tt.wantErr { + assert.Error(t, err, "Should reject invalid username") + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg, "Error message should contain expected text") + } + } else { + assert.NoError(t, err, "Should accept valid username") } + }) + } +} +func TestUsernameValidation_Windows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Windows-specific username validation tests") + } + + tests := []struct { + name string + username string + wantErr bool + errMsg string + }{ + // Valid usernames (Windows) + {"valid_alphanumeric", "user123", false, ""}, + {"valid_with_dots", "user.name", false, ""}, + {"valid_with_hyphens", "user-name", false, ""}, + {"valid_with_underscores", "user_name", false, ""}, + {"valid_uppercase", "UserName", false, ""}, + {"valid_starting_with_digit", "123user", false, ""}, + {"valid_starting_with_dot", ".hidden", false, ""}, + {"valid_starting_with_hyphen", "-user", false, ""}, // Windows allows this + {"valid_domain_username", "DOMAIN\\user", false, ""}, // Windows domain format + {"valid_email_username", "user@domain.com", false, ""}, // Windows email format + {"valid_machine_username", "MACHINE\\user", false, ""}, // Windows machine format + + // Invalid usernames (Windows) + {"empty_username", "", true, "username cannot be empty"}, + {"username_too_long", "thisusernameiswaytoolongandexceedsthe32characterlimit", true, "username too long"}, + {"username_with_spaces", "user name", true, "invalid characters"}, + {"username_with_shell_metacharacters", "user;rm", true, "invalid characters"}, + {"username_with_command_injection", "user`rm -rf /`", true, "invalid characters"}, + {"username_with_pipe", "user|rm", true, "invalid characters"}, + {"username_with_ampersand", "user&rm", true, "invalid characters"}, + {"username_with_quotes", "user\"name", true, "invalid characters"}, + {"username_with_newline", "user\nname", true, "invalid characters"}, + {"username_with_brackets", "user[name]", true, "invalid characters"}, + {"username_with_colon", "user:name", true, "invalid characters"}, + {"username_with_semicolon", "user;name", true, "invalid characters"}, + {"username_with_equals", "user=name", true, "invalid characters"}, + {"username_with_comma", "user,name", true, "invalid characters"}, + {"username_with_plus", "user+name", true, "invalid characters"}, + {"username_with_asterisk", "user*name", true, "invalid characters"}, + {"username_with_question", "user?name", true, "invalid characters"}, + {"username_with_angles", "user", true, "invalid characters"}, + {"reserved_dot", ".", true, "cannot be '.' or '..'"}, + {"reserved_dotdot", "..", true, "cannot be '.' or '..'"}, + {"username_ending_with_period", "user.", true, "cannot end with a period"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { err := validateUsername(tt.username) if tt.wantErr { assert.Error(t, err, "Should reject invalid username") @@ -684,11 +747,11 @@ func TestCheckPrivileges_ActualPlatform(t *testing.T) { switch { case actualOS == "windows": - // Windows should deny user switching - assert.False(t, result.Allowed, "Windows should deny user switching") + // Windows supports user switching but should fail on nonexistent user + assert.False(t, result.Allowed, "Windows should deny nonexistent user") assert.True(t, result.RequiresUserSwitching, "Should indicate switching is needed") - assert.Contains(t, result.Error.Error(), "user switching not supported", - "Should indicate user switching not supported") + assert.Contains(t, result.Error.Error(), "not found", + "Should indicate user not found") case !actualIsPrivileged: // Non-privileged Unix processes should fallback to current user assert.True(t, result.Allowed, "Non-privileged Unix process should fallback to current user") diff --git a/client/ssh/server/userswitching_windows.go b/client/ssh/server/userswitching_windows.go index d65eb02c1..3c9a93a46 100644 --- a/client/ssh/server/userswitching_windows.go +++ b/client/ssh/server/userswitching_windows.go @@ -56,7 +56,7 @@ func validateUsernameLength(username string) error { // validateUsernameCharacters checks for invalid characters in Windows usernames func validateUsernameCharacters(username string) error { - invalidChars := []rune{'"', '/', '\\', '[', ']', ':', ';', '|', '=', ',', '+', '*', '?', '<', '>', ' ', '`', '&', '\n'} + invalidChars := []rune{'"', '/', '[', ']', ':', ';', '|', '=', ',', '+', '*', '?', '<', '>', ' ', '`', '&', '\n'} for _, char := range username { for _, invalid := range invalidChars { if char == invalid { @@ -72,14 +72,14 @@ func validateUsernameCharacters(username string) error { // validateUsernameFormat checks for invalid username formats and patterns func validateUsernameFormat(username string) error { - if strings.HasSuffix(username, ".") { - return fmt.Errorf("username cannot end with a period") - } - if username == "." || username == ".." { return fmt.Errorf("username cannot be '.' or '..'") } + if strings.HasSuffix(username, ".") { + return fmt.Errorf("username cannot end with a period") + } + return nil }