Force case insensitivity for new emails, existing.

There is a legacy bug I just caught, where many accounts would have the
same email but then have different casing. In extreme scenarios, this
would lead to them creating a new user with the same email, or having
issues with password reset and username lookup.

This doesn't merge in the existing duplicates, but makes sure to only
allow insensitive lowercase emails from here on out. It also will check
for emails in a case insensitive way for such things as resets and
logins if the sensitive lookup doesn't work.

The implementation was not wrong per se - email is supposed to be case sensitive
for usernames. But of course, nobody (nor do most/all email servers) treat
them that way, leading to confusion situations where the user sometimes
camelcases their email and then switches to lowercase later.
This commit is contained in:
Kyle Drake 2017-12-08 22:13:27 -08:00
parent 33054a8298
commit d467e9be96
5 changed files with 52 additions and 7 deletions

View file

@ -139,7 +139,7 @@ post '/forgot_username' do
redirect '/forgot_username'
end
sites = Site.where(email: params[:email]).all
sites = Site.get_recovery_sites_with_email params[:email]
sites.each do |site|
body = <<-EOT

View file

@ -9,7 +9,7 @@ post '/send_password_reset' do
redirect '/password_reset'
end
sites = Site.filter(email: params[:email]).all
sites = Site.get_recovery_sites_with_email params[:email]
if sites.length > 0
token = SecureRandom.uuid.gsub('-', '')

View file

@ -295,14 +295,23 @@ class Site < Sequel::Model
def get_with_identifier(username_or_email)
if username_or_email =~ /@/
site = self.where(email: username_or_email).where(parent_site_id: nil).first
site = get_with_email username_or_email
else
site = self[username: username_or_email]
site = self[username: username_or_email.downcase]
end
return nil if site.nil? || site.is_banned || site.owner.is_banned
site
end
def get_recovery_sites_with_email(email)
self.where('lower(email) = ?', email.downcase).all
end
def get_with_email(email)
query = self.where(parent_site_id: nil)
query.where(email: email).first || query.where('lower(email) = ?', email.downcase).first
end
def ip_create_limit?(ip)
Site.where('created_at > ?', Date.today.to_time).where(ip: ip).count > IP_CREATE_LIMIT ||
Site.where(ip: ip).count > TOTAL_IP_CREATE_LIMIT
@ -844,7 +853,7 @@ class Site < Sequel::Model
def email=(email)
@original_email = values[:email] unless new?
super
super(email.nil? ? nil : email.downcase)
end
def can_email?(col=nil)
@ -972,12 +981,12 @@ class Site < Sequel::Model
# Check for existing email if new or changing email.
if new? || @original_email
email_check = self.class.select(:id).filter(email: values[:email])
email_check = self.class.select(:id).filter('lower(email)=?', values[:email])
email_check = email_check.exclude(id: self.id) unless new?
email_check = email_check.first
if parent? && email_check && email_check.id != self.id
errors.add :email, 'This email address already exists on Neocities, please use your existing account instead of creating a new one.'
errors.add :email, 'This email address already exists on Neocities.'
end
end

View file

@ -60,6 +60,17 @@ describe 'signin' do
page.must_have_content 'Your Feed'
end
it 'signs in with invalid case username' do
pass = SecureRandom.hex
@site = Fabricate :site, password: pass
visit '/'
click_link 'Sign In'
fill_in 'username', with: @site.username.upcase
fill_in 'password', with: pass
click_button 'Sign In'
page.must_have_content 'Your Feed'
end
it 'signs in with email' do
pass = SecureRandom.hex
@site = Fabricate :site, password: pass
@ -70,4 +81,15 @@ describe 'signin' do
click_button 'Sign In'
page.must_have_content 'Your Feed'
end
it 'signs in with invalid case email' do
pass = SecureRandom.hex
@site = Fabricate :site, password: pass
visit '/'
click_link 'Sign In'
fill_in 'username', with: @site.email.upcase
fill_in 'password', with: pass
click_button 'Sign In'
page.must_have_content 'Your Feed'
end
end

View file

@ -140,6 +140,20 @@ describe 'signup' do
page.must_have_content /email.+exists/
end
it 'fails with existing email even if case sensitive' do
email = Fabricate.attributes_for(:site)[:email]
fill_in_valid
fill_in 'email', with: email
click_signup_button
site_created?.must_equal true
Capybara.reset_sessions!
visit_signup
fill_in_valid
fill_in 'email', with: email.upcase
click_signup_button
page.must_have_content /email.+exists/
end
it 'succeeds with no tags' do
fill_in_valid
fill_in 'new_tags_string', with: ''