Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion backend/handler/auth/base_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,25 @@ async def get_current_active_user_from_openid_token(self, token: Any):
"User with email '%s' not found, creating new user",
hl(email, color=CYAN),
)
# Lazy import to avoid circular dependency: utils.validation imports
# models.user which (via handler.auth.__init__) imports this module.
from utils.validation import ValidationError, sanitize_username

try:
sanitized_username = sanitize_username(preferred_username)
except ValidationError as exc:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=exc.message,
) from exc
if sanitized_username != preferred_username:
log.warning(
"OIDC username '%s' contains invalid characters; sanitized to '%s'",
preferred_username,
sanitized_username,
)
new_user = User(
username=preferred_username,
username=sanitized_username,
hashed_password=str(uuid.uuid4()),
email=email,
enabled=True,
Expand Down
52 changes: 52 additions & 0 deletions backend/tests/handler/auth/test_oidc.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,58 @@ async def test_oidc_valid_add_user(
assert mock_add_user.call_args.args[0].role == romm_role


async def test_oidc_valid_add_user_with_dotted_username(
mocker,
mock_oidc_enabled,
mock_token,
mock_openid_configuration,
):
"""Test that OIDC usernames with printable chars (dots, @, etc.) are stored as-is."""
mock_token["userinfo"]["preferred_username"] = "john.doe@example"
mock_user = MagicMock(enabled=True, role=Role.VIEWER)
mock_add_user = mocker.patch(
"handler.database.db_user_handler.add_user", return_value=mock_user
)
mocker.patch.object(
StarletteOAuth2App,
"load_server_metadata",
return_value=mock_openid_configuration,
)

oidc_handler = OpenIDHandler()
await oidc_handler.get_current_active_user_from_openid_token(mock_token)

mock_add_user.assert_called_once()
assert mock_add_user.call_args.args[0].username == "john.doe@example"
assert mock_add_user.call_args.args[0].email == mock_token["userinfo"]["email"]


async def test_oidc_valid_add_user_with_sanitized_username(
mocker,
mock_oidc_enabled,
mock_token,
mock_openid_configuration,
):
"""Test that OIDC usernames with spaces are sanitized before user creation."""
mock_token["userinfo"]["preferred_username"] = "john doe"
mock_user = MagicMock(enabled=True, role=Role.VIEWER)
mock_add_user = mocker.patch(
"handler.database.db_user_handler.add_user", return_value=mock_user
)
mocker.patch.object(
StarletteOAuth2App,
"load_server_metadata",
return_value=mock_openid_configuration,
)

oidc_handler = OpenIDHandler()
await oidc_handler.get_current_active_user_from_openid_token(mock_token)

mock_add_user.assert_called_once()
assert mock_add_user.call_args.args[0].username == "john-doe"
assert mock_add_user.call_args.args[0].email == mock_token["userinfo"]["email"]


async def test_oidc_valid_edit_user_role(
mocker,
mock_oidc_enabled,
Expand Down
65 changes: 59 additions & 6 deletions backend/tests/utils/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from utils.validation import (
ValidationError,
sanitize_username,
validate_ascii_only,
validate_email,
validate_password,
Expand Down Expand Up @@ -49,6 +50,10 @@ def test_valid_usernames(self):
validate_username("test_user")
validate_username("admin")
validate_username("user-name")
validate_username("john.doe")
validate_username("user@domain")
validate_username("user+tag")
validate_username("user/path")

def test_invalid_empty_username(self):
"""Test that empty usernames fail validation."""
Expand All @@ -74,14 +79,14 @@ def test_invalid_long_username(self):
assert "no more than 255 characters" in exc_info.value.message

def test_invalid_characters_username(self):
"""Test that usernames with invalid characters fail validation."""
"""Test that usernames with spaces or control characters fail validation."""
with pytest.raises(ValidationError) as exc_info:
validate_username("user@domain")
assert "letters, numbers, underscores, and hyphens" in exc_info.value.message
validate_username("user name")
assert "spaces or control characters" in exc_info.value.message

with pytest.raises(ValidationError) as exc_info:
validate_username("user.name")
assert True
validate_username("user\tname")
assert "spaces or control characters" in exc_info.value.message

def test_invalid_non_ascii_username(self):
"""Test that usernames with non-ASCII characters fail validation."""
Expand All @@ -91,7 +96,55 @@ def test_invalid_non_ascii_username(self):

with pytest.raises(ValidationError) as exc_info:
validate_username("résumé")
assert True
assert "ASCII characters" in exc_info.value.message


class TestSanitizeUsername:
"""Test username sanitization."""

def test_valid_username_unchanged(self):
"""Test that already-valid usernames are returned unchanged."""
assert sanitize_username("user123") == "user123"
assert sanitize_username("test_user") == "test_user"
assert sanitize_username("user-name") == "user-name"
assert sanitize_username("john.doe") == "john.doe"
assert sanitize_username("user@domain") == "user@domain"

def test_space_replaced_with_hyphen(self):
"""Test that spaces are replaced with hyphens."""
assert sanitize_username("user name") == "user-name"
assert sanitize_username("john doe smith") == "john-doe-smith"

def test_consecutive_spaces_collapsed(self):
"""Test that multiple consecutive spaces collapse to a single hyphen."""
assert sanitize_username("user name") == "user-name"

def test_leading_trailing_spaces_stripped(self):
"""Test that leading and trailing spaces are stripped after sanitization."""
assert sanitize_username(" username ") == "username"

def test_non_ascii_chars_removed(self):
"""Test that non-ASCII characters are removed."""
assert sanitize_username("naïve") == "nave"
assert sanitize_username("jöhn") == "jhn"

def test_too_short_after_sanitization_raises(self):
"""Test that a ValidationError is raised if sanitized result is too short."""
with pytest.raises(ValidationError) as exc_info:
sanitize_username(" a ") # strips to "a" after space→hyphen + strip
assert "too short" in exc_info.value.message

def test_empty_username_raises(self):
"""Test that an empty username raises a ValidationError."""
with pytest.raises(ValidationError) as exc_info:
sanitize_username("")
assert "cannot be empty" in exc_info.value.message

def test_long_username_truncated(self):
"""Test that long usernames are truncated to 255 characters."""
long_username = "a" * 300
result = sanitize_username(long_username)
assert len(result) <= 255


class TestValidatePassword:
Expand Down
49 changes: 47 additions & 2 deletions backend/utils/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ def __init__(self, message: str, field_name: str = "field"):


# Pre-compiled regex patterns for better performance
USERNAME_PATTERN = re.compile(r"^[a-zA-Z0-9_-]+$")
USERNAME_PATTERN = re.compile(r"^[\x21-\x7E]+$")
USERNAME_INVALID_CHARS_PATTERN = re.compile(r"[^\x21-\x7E]")
USERNAME_CONSECUTIVE_HYPHENS_PATTERN = re.compile(r"-{2,}")
EMAIL_PATTERN = re.compile(r"^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$")


Expand Down Expand Up @@ -68,11 +70,54 @@ def validate_username(username: str) -> None:
raise ValidationError(msg, "Username")

if not USERNAME_PATTERN.match(username):
msg = "Username can only contain letters, numbers, underscores, and hyphens"
msg = "Username must not contain spaces or control characters"
log.error(f"Validation failed: {msg} for username: {username}")
raise ValidationError(msg, "Username")


def sanitize_username(username: str) -> str:
"""Sanitize a username by replacing invalid characters with hyphens.

This is used for OIDC-provided usernames which may contain characters
not allowed by the standard username validation rules.

Args:
username (str): The username to sanitize

Returns:
str: The sanitized username

Raises:
ValidationError: If the sanitized username is still invalid (e.g., too short)
"""
if not username or not username.strip():
msg = "Username cannot be empty"
log.error(msg)
raise ValidationError(msg, "Username")

# Encode to ASCII, ignoring non-ASCII characters
ascii_username = username.encode("ascii", errors="ignore").decode("ascii")

# Replace any character not in [a-zA-Z0-9_-] with a hyphen
sanitized = USERNAME_INVALID_CHARS_PATTERN.sub("-", ascii_username)

# Collapse multiple consecutive hyphens into one
sanitized = USERNAME_CONSECUTIVE_HYPHENS_PATTERN.sub("-", sanitized)

# Strip leading and trailing hyphens
sanitized = sanitized.strip("-")

# Truncate to maximum allowed length
sanitized = sanitized[:TEXT_FIELD_LENGTH]

if len(sanitized) < 3:
msg = f"Username '{username}' could not be sanitized to a valid username (result too short)"
log.error(msg)
raise ValidationError(msg, "Username")

return sanitized


def validate_password(password: str) -> None:
"""Validate password format and content.

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/locales/en_GB/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"upload": "Upload",
"upload-files-selected": "{count} file selected | {count} files selected",
"user-interface": "User interface",
"username-chars": "Username can only contain letters, numbers, underscores, and hyphens",
"username-chars": "Username must not contain spaces or control characters",
"username-length": "Username must be between 3 and 255 characters",
"virtual-collection": "Autogenerated collection",
"virtual-collections": "Autogenerated collections",
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/locales/en_US/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"upload": "Upload",
"upload-files-selected": "{count} file selected | {count} files selected",
"user-interface": "User interface",
"username-chars": "Username can only contain letters, numbers, underscores, and hyphens",
"username-chars": "Username must not contain spaces or control characters",
"username-length": "Username must be between 3 and 255 characters",
"virtual-collection": "Autogenerated collection",
"virtual-collections": "Autogenerated collections",
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/stores/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
(v.length >= 3 && v.length <= 255) || i18n.global.t("common.username-length");

const usernameChars = (v: string) =>
/^[a-zA-Z0-9_-]*$/.test(v) || i18n.global.t("common.username-chars");
// eslint-disable-next-line no-control-regex

Check warning on line 15 in frontend/src/stores/users.ts

View workflow job for this annotation

GitHub Actions / Trunk Check

eslint

[new] Unused eslint-disable directive (no problems were reported from 'no-control-regex').
/^[\x21-\x7E]*$/.test(v) || i18n.global.t("common.username-chars");

const passwordLength = (v: string) =>
(v.length >= 6 && v.length <= 255) || i18n.global.t("common.password-length");
Expand Down
Loading