Fix: OAuth signups crash when generated values exceed User column widths - #274
Open
carlosplanchon wants to merge 3 commits into
Open
Fix: OAuth signups crash when generated values exceed User column widths#274carlosplanchon wants to merge 3 commits into
carlosplanchon wants to merge 3 commits into
Conversation
crudauth's OAuth provisioning sanitizes the provider username and caps it at USERNAME_MAX_LENGTH = 32, but User.username was String(20), so any OAuth signup whose sanitized username exceeded 20 chars crashed the callback with StringDataRightTruncationError. Found live with a self-hosted OIDC provider: preferred_username values shaped like user@org.domain sanitize to 32 chars. Google never trips it (no preferred_username, falls back to the short given name), which is why it went unnoticed. The column is now String(32), matching the generator's cap. Same class of bug one field over: NewUserContext.suggested_name returns the provider's full display name unbounded while User.name is String(30), so new_user_fields now truncates it to the column width. Adds a regression test exercising a long provider username + display name against the real DB (fails without the fix, with the exact truncation error seen in the field). Note: existing databases need `ALTER TABLE "user" ALTER COLUMN username TYPE VARCHAR(32);` since create_all only covers fresh databases. Signed-off-by: Carlos Andrés Planchón Prestes <carlosandresplanchonprestes@gmail.com>
The username column is String(32) and crudauth's OAuth generator emits up to 32 chars with underscores, but UserBase, UserRead and UserUpdate still declared max_length=20 and ^[a-z0-9]+$. Widening the column alone is not enough: crudauth writes the row directly, so the signup succeeds and every later read through UserRead fails with a ResponseValidationError. The constraints now live in USERNAME_MAX_LENGTH / USERNAME_PATTERN and are reused by all three schemas. Co-authored-by: Nicolás Gutiérrez <ngutierreztassano@gmail.com> Signed-off-by: Carlos Andrés Planchón Prestes <carlosandresplanchonprestes@gmail.com>
The user guide still documented max_length=20 and ^[a-z0-9]+$, which contradicted the code after the schemas moved to USERNAME_MAX_LENGTH / USERNAME_PATTERN. database/schemas.md mirrors the real module including the shared constants, authentication/user-management.md points at where they are defined, and the simplified snippet in database/index.md just carries the new bound. Signed-off-by: Carlos Andrés Planchón Prestes <carlosandresplanchonprestes@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
crudauth caps generated OAuth usernames at 32 chars but User.username is String(20), so providers whose sanitized username exceeds 20 chars (e.g. OIDC preferred_username shaped like user@org.domain) crash the callback with StringDataRightTruncationError. Also bounds the provider's unbounded display name to User.name's String(30). Found live with a self-hosted Zitadel instance; Google never trips it, which is why it went unnoticed. Includes a regression test that fails without the fix. Note for existing DBs: ALTER TABLE "user" ALTER COLUMN username TYPE VARCHAR(32).