Skip to content

ENG-3466: Add username character validation on user creation#7953

Merged
erosselli merged 5 commits intomainfrom
erosselli/ENG-3466
Apr 17, 2026
Merged

ENG-3466: Add username character validation on user creation#7953
erosselli merged 5 commits intomainfrom
erosselli/ENG-3466

Conversation

@erosselli
Copy link
Copy Markdown
Contributor

@erosselli erosselli commented Apr 17, 2026

Ticket ENG-3466

Description Of Changes

Usernames were only validated to reject spaces, allowing special characters like &, =, <, > that break invite links when interpolated into the email template URL. This tightens the backend Pydantic validator on UserCreate to restrict usernames to [a-zA-Z0-9._-]{1,100}.

This is a creation-only change — existing users are unaffected since the validator only runs on UserCreate, not on login (UserLogin), read (UserResponse), or update (UserUpdate) paths.

The email template logic will be updated in a separate PR.

Code Changes

  • Replaced the spaces-only check in UserCreate.validate_username with re.fullmatch against a USERNAME_PATTERN constant ([a-zA-Z0-9._-]{1,100})
  • Added parametrized tests for invalid usernames (spaces, &, =, <, empty, 101 chars) and valid usernames (plain, hyphen, underscore, mixed, single char, 100 chars)

Steps to Confirm

  1. Create a user with a valid username (e.g. test-user_123) — should succeed
  2. Create a user with an invalid username (e.g. user&name, user name, empty) — should return a validation error
  3. Existing users can still log in and view their profile without errors

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Restrict usernames to alphanumeric characters, underscores, and hyphens
(1-100 chars) to prevent special characters from breaking invite links.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Apr 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Apr 17, 2026 4:10pm
fides-privacy-center Ignored Ignored Apr 17, 2026 4:10pm

Request Review

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.04%. Comparing base (3bf2e3e) to head (41476d2).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7953      +/-   ##
==========================================
+ Coverage   85.03%   85.04%   +0.01%     
==========================================
  Files         629      629              
  Lines       40971    40997      +26     
  Branches     4763     4769       +6     
==========================================
+ Hits        34838    34865      +27     
  Misses       5052     5052              
+ Partials     1081     1080       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

erosselli and others added 2 commits April 17, 2026 12:24
@erosselli erosselli marked this pull request as ready for review April 17, 2026 15:50
@erosselli erosselli requested a review from a team as a code owner April 17, 2026 15:50
@erosselli erosselli requested review from nreyes-dev and removed request for a team April 17, 2026 15:50
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: ENG-3466 — Username character validation

The change is well-scoped: replace a space-only check with a strict allowlist regex, add parametrized tests for both valid and invalid cases. The logic is correct and the approach is sound. Three findings below.


Critical (Must Fix)

Error message does not match the allowed pattern.
USERNAME_PATTERN is [a-zA-Z0-9._-]{1,100}, which allows dots. The error message says "may only contain letters, numbers, underscores, and hyphens" — dots are absent. john.doe is tested as a valid username but a user reading the error would not know to try it. The message and pattern must agree. See inline comment on line 18 of user.py.


Suggestions

Missing follow-up issue for the email template fix.
The PR description says the template URL-encoding fix will come in a separate PR, but the pre-merge checklist has "No followup issues" checked and no issue linked. The root-cause template (user_invite.html) still interpolates {{username}} raw into the URL. The new validation blocks the worst offenders (&, =, <, >), but the fix is incomplete — dots are now allowed and are URL-safe, but this is fragile reasoning. A tracked follow-up issue should exist before this merges. See inline comment on the template file.

Invalid-username test does not assert on the error message.
pytest.raises(ValueError) without a match= argument passes for any ValueError, including one from an unrelated field. Add match="Usernames must be" to make the test specifically verify username validation is firing. See inline comment on line 46 of the test file.

re.fullmatch with a string constant recompiles on each call.
Minor: prefer re.compile(USERNAME_PATTERN) at module level and call .fullmatch() on the compiled object. CPython's regex cache means this is not a performance problem in practice, but it makes the intent explicit. See inline comment on line 44 of user.py.


Nice to Have

The parametrized valid-username test asserts user.username == username. That's correct and sufficient. No issues there.


🔬 Codegraph: connected (46831 nodes) — note: graph snapshot reflects main (795eb71), not the PR head, so cross-repo impact analysis reflects pre-PR state. No architecture violations detected for this change.


💡 Write /code-review in a comment to re-run this review.

Comment thread src/fides/api/schemas/user.py
Comment thread src/fides/api/schemas/user.py
Comment thread tests/lib/test_oauth_schemas_user.py Outdated
- Include periods in the validation error message to match the pattern
- Add match="Usernames must be" to pytest.raises for specificity

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@erosselli erosselli added this pull request to the merge queue Apr 17, 2026
Merged via the queue into main with commit fb01292 Apr 17, 2026
69 checks passed
@erosselli erosselli deleted the erosselli/ENG-3466 branch April 17, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants