Skip to content

Move username sanitization to model and make it less strict#2150

Merged
Boy132 merged 1 commit intomainfrom
boy132/fix-username-sanitization
Jan 31, 2026
Merged

Move username sanitization to model and make it less strict#2150
Boy132 merged 1 commit intomainfrom
boy132/fix-username-sanitization

Conversation

@Boy132
Copy link
Member

@Boy132 Boy132 commented Jan 29, 2026 •
edited
Loading

Closes #2138

Before:
Backend silently removed . and - from username, made it lowercase and max length was 64.
Frontend did not check these restrictions.

Now:
Username is only made lowercase. (to allow case insensitive login) . and - are allowed. (will cause no problems with sftp)
Frontend and backend limits for max length match.

@Boy132 Boy132 self-assigned this Jan 29, 2026
@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes modify username normalization behavior by removing the character sanitization chain that stripped special characters during user creation, while simplifying lowercase enforcement to occur uniformly at the model persistence layer for both username and email fields.

Changes

Cohort / File(s) Summary
Model Persistence Layer
app/Models/User.php		 Modified the booted saving behavior to lowercase both username and email using Str pipeline during save, consolidating case normalization at persistence time instead of relying solely on the existing email-only lowercasing via mb_strtolower.
User Creation Service
app/Services/Users/UserCreationService.php		 Removed the username normalization chain that previously sanitized usernames by replacing periods and hyphens, converting to ASCII, and truncating to 64 characters. The generated default username now bypasses these transformations.

Possibly related PRs

  • #1731: Modifies username normalization in app/Models/User.php by enforcing lowercase/trim on username fields
  • #1702: Removes the Username validation rule and related UI, complementing the shift away from character sanitization enforcement
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: sanitization logic is relocated to the model and constraints are relaxed.
Description check ✅ Passed The description clearly explains the before/after behavior and addresses the issue: less strict sanitization and frontend-backend alignment.
Linked Issues check ✅ Passed The code changes meet issue #2138 requirements by allowing dashes and preserving case via lowercase-only sanitization, directly addressing the reported problems.
Out of Scope Changes check ✅ Passed Both file changes directly address the linked issue: removing strict sanitization from the service and moving lowercase-only normalization to the model layer.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Boy132 Boy132 merged commit a477c89 into main Jan 31, 2026
32 checks passed
@Boy132 Boy132 deleted the boy132/fix-username-sanitization branch January 31, 2026 23:07
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@notAreYouScared notAreYouScared notAreYouScared approved these changes

Assignees

@Boy132 Boy132

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New User Usernames are Unexpectedly Sanitized

2 participants

@Boy132 @notAreYouScared