diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 6e437101c1b..3b82651bd13 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -54,6 +54,8 @@ class SessionController < ApplicationController params.require(:login) params.require(:password) + return invalid_credentials if params[:password].length > User.max_password_length + login = params[:login].strip login = login[1..-1] if login[0] == "@" diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2cd93c9aacd..a7b8c361bbd 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -151,6 +151,11 @@ class UsersController < ApplicationController return end + if params[:password] && params[:password].length > User.max_password_length + render json: { success: false, message: I18n.t("login.password_too_long") } + return + end + user = User.new(user_params) authentication = UserAuthenticator.new(user, session) @@ -221,12 +226,17 @@ class UsersController < ApplicationController if !@user flash[:error] = I18n.t('password_reset.no_token') elsif request.put? - raise Discourse::InvalidParameters.new(:password) unless params[:password].present? - @user.password = params[:password] - @user.password_required! - if @user.save - Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore - logon_after_password_reset + @invalid_password = params[:password].blank? || params[:password].length > User.max_password_length + + if @invalid_password + @user.errors.add(:password, :invalid) + else + @user.password = params[:password] + @user.password_required! + if @user.save + Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore + logon_after_password_reset + end end end render layout: 'no_js' diff --git a/app/models/user.rb b/app/models/user.rb index c24671b053c..10b6b03a30d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -111,6 +111,10 @@ class User < ActiveRecord::Base LAST_VISIT = -2 end + def self.max_password_length + 200 + end + def self.username_length SiteSetting.min_username_length.to_i..SiteSetting.max_username_length.to_i end @@ -669,6 +673,7 @@ class User < ActiveRecord::Base end def hash_password(password, salt) + raise "password is too long" if password.size > User.max_password_length Pbkdf2.hash_password(password, salt, Rails.configuration.pbkdf2_iterations, Rails.configuration.pbkdf2_algorithm) end diff --git a/app/views/users/password_reset.html.erb b/app/views/users/password_reset.html.erb index 928bdaa0951..c8cbd0f170e 100644 --- a/app/views/users/password_reset.html.erb +++ b/app/views/users/password_reset.html.erb @@ -36,7 +36,8 @@ <%=form_tag({}, method: :put) do %>

- + +

<%=submit_tag( @user.has_password? ? t('password_reset.update') : t('password_reset.save'), class: 'btn')%> diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 838f0784980..707a02ac9e0 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1088,6 +1088,7 @@ en: omniauth_error: "Sorry, there was an error authorizing your %{strategy} account. Perhaps you did not approve authorization?" omniauth_error_unknown: "Something went wrong processing your log in, please try again." new_registrations_disabled: "New account registrations are not allowed at this time." + password_too_long: "Passwords are limited to 200 characters." user: username: diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index e40ebef55d5..196ef8299f9 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -186,6 +186,14 @@ describe SessionController do end end + describe 'invalid password' do + it "should return an error with an invalid password if too long" do + User.any_instance.expects(:confirm_password?).never + xhr :post, :create, login: user.username, password: ('s' * (User.max_password_length + 1)) + ::JSON.parse(response.body)['error'].should be_present + end + end + describe 'suspended user' do it 'should return an error' do User.any_instance.stubs(:suspended?).returns(true) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 053eb2391e8..1060338053d 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -287,14 +287,28 @@ describe UsersController do EmailToken.expects(:confirm).with(token).returns(user) end + it "fails when the password is blank" do + put :password_reset, token: token, password: '' + assigns(:user).errors.should be_present + session[:current_user_id].should be_blank + end + + it "fails when the password is too long" do + put :password_reset, token: token, password: ('x' * (User.max_password_length + 1)) + assigns(:user).errors.should be_present + session[:current_user_id].should be_blank + end + it "logs in the user" do put :password_reset, token: token, password: 'newpassword' + assigns(:user).errors.should be_blank session[:current_user_id].should be_present end it "doesn't log in the user when not approved" do SiteSetting.expects(:must_approve_users?).returns(true) put :password_reset, token: token, password: 'newpassword' + assigns(:user).errors.should be_blank session[:current_user_id].should be_blank end end @@ -508,6 +522,11 @@ describe UsersController do include_examples 'failed signup' end + context 'when password is too long' do + let(:create_params) { {name: @user.name, username: @user.username, password: "x" * (User.max_password_length + 1), email: @user.email} } + include_examples 'failed signup' + end + context 'when password param is missing' do let(:create_params) { {name: @user.name, username: @user.username, email: @user.email} } include_examples 'failed signup' diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bcfcea9a6af..b0a92267a92 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1150,4 +1150,30 @@ describe User do end end + describe "hash_passwords" do + + let(:too_long) { "x" * (User.max_password_length + 1) } + + def hash(password, salt) + User.new.send(:hash_password, password, salt) + end + + it "returns the same hash for the same password and salt" do + hash('poutine', 'gravy').should == hash('poutine', 'gravy') + end + + it "returns a different hash for the same salt and different password" do + hash('poutine', 'gravy').should_not == hash('fries', 'gravy') + end + + it "returns a different hash for the same password and different salt" do + hash('poutine', 'gravy').should_not == hash('poutine', 'cheese') + end + + it "raises an error when passwords are too long" do + -> { hash(too_long, 'gravy') }.should raise_error + end + + end + end