diff --git a/backend/handler/auth/base_handler.py b/backend/handler/auth/base_handler.py index 94c60bcf07..e5807d1185 100644 --- a/backend/handler/auth/base_handler.py +++ b/backend/handler/auth/base_handler.py @@ -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, diff --git a/backend/tests/handler/auth/test_oidc.py b/backend/tests/handler/auth/test_oidc.py index a285cafa72..f0cc45f8e5 100644 --- a/backend/tests/handler/auth/test_oidc.py +++ b/backend/tests/handler/auth/test_oidc.py @@ -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, diff --git a/backend/tests/utils/test_validation.py b/backend/tests/utils/test_validation.py index b55720579a..3954ff9cc7 100644 --- a/backend/tests/utils/test_validation.py +++ b/backend/tests/utils/test_validation.py @@ -4,6 +4,7 @@ from utils.validation import ( ValidationError, + sanitize_username, validate_ascii_only, validate_email, validate_password, @@ -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.""" @@ -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.""" @@ -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: diff --git a/backend/utils/validation.py b/backend/utils/validation.py index 27ddff568a..e4aecb227b 100644 --- a/backend/utils/validation.py +++ b/backend/utils/validation.py @@ -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,}$") @@ -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. diff --git a/frontend/src/locales/en_GB/common.json b/frontend/src/locales/en_GB/common.json index 4cbf3da3d0..ba80d13bbf 100644 --- a/frontend/src/locales/en_GB/common.json +++ b/frontend/src/locales/en_GB/common.json @@ -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", diff --git a/frontend/src/locales/en_US/common.json b/frontend/src/locales/en_US/common.json index 4cbf3da3d0..ba80d13bbf 100644 --- a/frontend/src/locales/en_US/common.json +++ b/frontend/src/locales/en_US/common.json @@ -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", diff --git a/frontend/src/stores/users.ts b/frontend/src/stores/users.ts index 668bc62ca9..788d34db68 100644 --- a/frontend/src/stores/users.ts +++ b/frontend/src/stores/users.ts @@ -12,7 +12,8 @@ const usernameLength = (v: string) => (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 + /^[\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");