-
Notifications
You must be signed in to change notification settings - Fork 37
Restrict username characters to those allowed in Ubuntu/Debian #1452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
c127159
51722b9
6b53ffa
d3a0215
395b917
ee16c0a
b8eb81b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| FIRST CALL: | ||
| access: | ||
| data: | ||
| err: provided userinfo is invalid: username "-invalid_username" is not valid: it must match ^[a-z_][-a-z0-9_.@]*[$]?$ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| FIRST CALL: | ||
| access: | ||
| msg: | ||
| err: provided userinfo is invalid: username "-invalid_username" is not valid: it must match ^[a-z_][-a-z0-9_.@]*[$]?$ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| users: [] | ||
| groups: [] | ||
| users_to_groups: [] | ||
| schema_version: 2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,3 +136,46 @@ func TestUserInfoEquals(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestValidateUsername(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := map[string]struct { | ||
| username string | ||
| wantErr bool | ||
| }{ | ||
| // Valid usernames | ||
| "Valid_lowercase_name": {username: "user"}, | ||
| "Valid_name_starting_with_underscore": {username: "_user"}, | ||
| "Valid_name_with_hyphen": {username: "user-name"}, | ||
| "Valid_name_with_underscore": {username: "user_name"}, | ||
| "Valid_name_with_digits": {username: "user1"}, | ||
| "Valid_name_with_trailing_dollar_sign": {username: "user$"}, | ||
| "Valid_single_character_name": {username: "a"}, | ||
| "Valid_name_with_mixed_allowed_chars": {username: "a-b_c0"}, | ||
| "Valid_email_style_name": {username: "user@example.com"}, | ||
| "Valid_email_style_name_with_subdomain": {username: "user@sub.example.com"}, | ||
| "Valid_name_with_dot": {username: "first.last"}, | ||
|
|
||
| // Invalid usernames | ||
| "Error_on_empty_username": {username: "", wantErr: true}, | ||
| "Error_on_uppercase_character": {username: "User", wantErr: true}, | ||
| "Error_on_uppercase_email": {username: "User@example.com", wantErr: true}, | ||
| "Error_on_name_starting_with_digit": {username: "1user", wantErr: true}, | ||
| "Error_on_name_starting_with_hyphen": {username: "-user", wantErr: true}, | ||
| "Error_on_name_with_dollar_not_at_end": {username: "user$name", wantErr: true}, | ||
| "Error_on_name_with_space": {username: "user name", wantErr: true}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check if a name has
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot ^^
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in b8eb81b. Added explicit |
||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| err := types.ValidateUsername(tc.username) | ||
| if tc.wantErr { | ||
| require.Error(t, err, "ValidateUsername should return an error for %q, but did not", tc.username) | ||
| return | ||
| } | ||
| require.NoError(t, err, "ValidateUsername should not return an error for %q, but did", tc.username) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how likely it is for this to cause regressions, i.e. there being existing authd users with characters that are allowed by this change, causing login to fail for those users.
We might want to allow all characters that are allowed in email addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But email addresses allow
/which opens the door to path traversal again. Maybe we just shouldn't try to validate the username and keep accepting what the broker gives us?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3v1n0 WDYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and new users have the same issue if their email address contains characters not allowed by this change. they will have to change their email address to be able to log in successfully. and in the current implementation, they are only told so after successful device auth, which makes the UX even worse.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just deny-list some relevant chars such as all the ones included in these tests, which even if used by someone, really seems wrong.
Likely including any white-space and non-printable character in general