Skip to content

Commit

Permalink
Merge branch '14552-signup-password-leak' into 'master'
Browse files Browse the repository at this point in the history
Don't populate the password field on signup validation errors

- Previously, we were pulling `params[:user][:password]` as the default
  value for the password field. This is incorrect; we should be pulling
  it from `@user.password` or the like.

[Closes #14552]

See merge request !3691
  • Loading branch information
rymai committed Apr 18, 2016
2 parents e9f20f5 + 38557ec commit 6d899f4
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG
Expand Up @@ -56,6 +56,7 @@ v 8.7.0 (unreleased)
- Decouple membership and notifications
- Fix creation of merge requests for orphaned branches (Stan Hu)
- API: Ability to retrieve a single tag (Robert Schilling)
- While signing up, don't persist the user password across form redisplays
- Fall back to `In-Reply-To` and `References` headers when sub-addressing is not available (David Padilla)
- Remove "Congratulations!" tweet button on newly-created project. (Connor Shea)
- Fix admin/projects when using visibility levels on search (PotHix)
Expand Down
9 changes: 4 additions & 5 deletions app/views/devise/shared/_signup_box.html.haml
Expand Up @@ -6,18 +6,17 @@
.login-heading
%h3 Create an account
.login-body
- user = params[:user].present? ? params[:user] : {}
= form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f|
.devise-errors
= devise_error_messages!
%div
= f.text_field :name, class: "form-control top", value: user[:name], placeholder: "Name", required: true
= f.text_field :name, class: "form-control top", placeholder: "Name", required: true
%div
= f.text_field :username, class: "form-control middle", value: user[:username], placeholder: "Username", required: true
= f.text_field :username, class: "form-control middle", placeholder: "Username", required: true
%div
= f.email_field :email, class: "form-control middle", value: user[:email], placeholder: "Email", required: true
= f.email_field :email, class: "form-control middle", placeholder: "Email", required: true
.form-group.append-bottom-20#password-strength
= f.password_field :password, class: "form-control bottom", value: user[:password], id: "user_password_sign_up", placeholder: "Password", required: true
= f.password_field :password, class: "form-control bottom", id: "user_password_sign_up", placeholder: "Password", required: true
%div
- if current_application_settings.recaptcha_enabled
= recaptcha_tags
Expand Down
55 changes: 55 additions & 0 deletions spec/features/signup_spec.rb
@@ -0,0 +1,55 @@
require 'spec_helper'

feature 'Signup', feature: true do
describe 'signup with no errors' do
it 'creates the user account and sends a confirmation email' do
user = build(:user)

visit root_path

fill_in 'user_name', with: user.name
fill_in 'user_username', with: user.username
fill_in 'user_email', with: user.email
fill_in 'user_password_sign_up', with: user.password
click_button "Sign up"

expect(current_path).to eq user_session_path
expect(page).to have_content("A message with a confirmation link has been sent to your email address.")
end
end

describe 'signup with errors' do
it "displays the errors" do
existing_user = create(:user)
user = build(:user)

visit root_path

fill_in 'user_name', with: user.name
fill_in 'user_username', with: user.username
fill_in 'user_email', with: existing_user.email
fill_in 'user_password_sign_up', with: user.password
click_button "Sign up"

expect(current_path).to eq user_registration_path
expect(page).to have_content("error prohibited this user from being saved")
expect(page).to have_content("Email has already been taken")
end

it 'does not redisplay the password' do
existing_user = create(:user)
user = build(:user)

visit root_path

fill_in 'user_name', with: user.name
fill_in 'user_username', with: user.username
fill_in 'user_email', with: existing_user.email
fill_in 'user_password_sign_up', with: user.password
click_button "Sign up"

expect(current_path).to eq user_registration_path
expect(page.body).not_to match(/#{user.password}/)
end
end
end

0 comments on commit 6d899f4

Please sign in to comment.