Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make username sanitization case-insensitive (#284) #285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hydrandt
Copy link

Description

Fixes #284, makes username sanitization case-insensitive to avoid potential conflicts when creating account using oauth providers.

Performance impact

Should be minimal (using lower() 3x in sanitization loop, potentially could be optimized by only running it once and assigning to a variable, is it worth it?

Security impact

None.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

@benjie
Copy link
Member

benjie commented Dec 10, 2021

Is this actually required? Username is citext so it should already be compared case insensitively…

@benjie
Copy link
Member

benjie commented Dec 10, 2021

Ah the comparison is text due to concat; we should case the result of concat back to citext.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Could you try a different approach:

where users.username = (
  case
  when i = 0 then v_username
  else (v_username || i::text)::citext
  end
)

this should solve the issue but also means we can continue to use the username unique index.

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.

username sanitization is case sensitive; column is citext
2 participants