From 075f2d9eefa26cd0dec3900c5f506f3c5de4ba08 Mon Sep 17 00:00:00 2001 From: Michael Ryan Date: Fri, 18 May 2018 18:57:37 -0400 Subject: [PATCH 1/2] LG-283 Fix password reset links sent to unconfirmed email address **Why**: User could lose account access if password reset link sent to incorrect or inaccessible address **How**: Remove the Devise mail helper initializer added to keep unencrypted email addresses from log files, now superfluous since all job log entries have standardized output via our custom `ActiveJob::Logging::LogSubscriber` --- app/mailers/user_mailer.rb | 3 +- config/initializers/devise_mailer_helpers.rb | 27 -------------- .../active_job_logger_patch_spec.rb | 36 +++++++++++++++++++ spec/features/users/user_edit_spec.rb | 9 +++++ .../visitors/password_recovery_spec.rb | 25 +++++++++++++ 5 files changed, 72 insertions(+), 28 deletions(-) delete mode 100644 config/initializers/devise_mailer_helpers.rb create mode 100644 spec/config/initializers/active_job_logger_patch_spec.rb diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 1f41ae6dee3..330388e7f38 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -2,7 +2,8 @@ class UserMailer < ActionMailer::Base include Mailable include LocaleHelper before_action :attach_images - default from: email_with_name(Figaro.env.email_from, Figaro.env.email_from) + default from: email_with_name(Figaro.env.email_from, Figaro.env.email_from), + reply_to: email_with_name(Figaro.env.email_from, Figaro.env.email_from) def email_changed(old_email) mail(to: old_email, subject: t('mailer.email_change_notice.subject')) diff --git a/config/initializers/devise_mailer_helpers.rb b/config/initializers/devise_mailer_helpers.rb deleted file mode 100644 index aa1cb85a475..00000000000 --- a/config/initializers/devise_mailer_helpers.rb +++ /dev/null @@ -1,27 +0,0 @@ -# This overrides the Devise mailer headers so that the recipient -# is determined based on whether or not an unconfirmed_email is present, -# as opposed to passing in the email as an argument to the job, which -# might expose it in some logs. -module Devise - module Mailers - module Helpers - def headers_for(action, opts) - headers = { - subject: subject_for(action), - to: recipient, - template_path: template_paths, - template_name: action, - }.merge(opts) - - @email = headers[:to] - headers - end - - private - - def recipient - resource.unconfirmed_email.presence || resource.email - end - end - end -end diff --git a/spec/config/initializers/active_job_logger_patch_spec.rb b/spec/config/initializers/active_job_logger_patch_spec.rb new file mode 100644 index 00000000000..ba9c12866ed --- /dev/null +++ b/spec/config/initializers/active_job_logger_patch_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +# Covers config/initializers/active_job_logger_patch.rb, which overrides +# ActiveJob::Logging::LogSubscriber to standardize output and prevent sensitive +# user data from being logged. +describe ActiveJob::Logging::LogSubscriber do + it 'overrides the default job logger to output only specified parameters in JSON format' do + class FakeJob < ActiveJob::Base + def perform(sensitive_param:); end + end + + # This list corresponds to the initializer's output + permitted_attributes = %w( + timestamp + event_type + job_class + job_queue + job_id + duration + ) + + # In this case, we need to assert before the action which logs, block-style to + # match the initializer + expect(Rails.logger).to receive(:info) do |&blk| + output = JSON.parse(blk.call) + + # [Sidenote: The nested assertions don't seem to be reflected in the spec + # count--perhaps because of the uncommon block format?--but reversing them + # will show them failing as expected.] + output.keys.each { |k| expect(permitted_attributes).to include(k) } + expect(output.keys).to_not include('sensitive_param') + end + + FakeJob.perform_later(sensitive_param: '111-22-3333') + end +end diff --git a/spec/features/users/user_edit_spec.rb b/spec/features/users/user_edit_spec.rb index cc162c14cd1..e2b9095c8bc 100644 --- a/spec/features/users/user_edit_spec.rb +++ b/spec/features/users/user_edit_spec.rb @@ -15,6 +15,15 @@ expect(page).to have_current_path manage_email_path end + + scenario 'user receives confirmation message at new address' do + fill_in 'Email', with: 'new_email@test.com' + click_button 'Update' + + open_last_email + click_email_link_matching(/confirmation_token/) + expect(page).to have_content('new_email@test.com') + end end context 'editing 2FA phone number' do diff --git a/spec/features/visitors/password_recovery_spec.rb b/spec/features/visitors/password_recovery_spec.rb index 4a0db3975b1..b7a0e753f79 100644 --- a/spec/features/visitors/password_recovery_spec.rb +++ b/spec/features/visitors/password_recovery_spec.rb @@ -253,4 +253,29 @@ to(include('style-src \'self\' \'unsafe-inline\'')) end end + + context 'user resets password with unconfirmed email address edit' do + let(:original_email) { 'original_email@test.com' } + let(:user) { create(:user, :signed_up, email: original_email) } + + before do + sign_in_and_2fa_user(user) + visit manage_email_path + end + + it 'receives password reset message at original address' do + fill_in 'Email', with: 'new_email@test.com' + click_button 'Update' + + expect(page).to have_content t('devise.registrations.email_update_needs_confirmation') + + visit sign_out_url + + click_link t('links.passwords.forgot') + fill_in 'password_reset_email_form_email', with: original_email + click_button t('forms.buttons.continue') + + expect(open_last_email).to be_delivered_to(original_email) + end + end end From 501b14579a5f87f86067ed2d7e2d6622226e67e0 Mon Sep 17 00:00:00 2001 From: Michael Ryan Date: Fri, 8 Jun 2018 09:32:49 -0400 Subject: [PATCH 2/2] LG-283 Fix password reset links sent to unconfirmed email address **Why**: User could lose account access if password reset link sent to incorrect or inaccessible address **How**: Remove the Devise mail helper initializer added to keep unencrypted email addresses from log files, now superfluous since all job log entries have standardized output via our custom `ActiveJob::Logging::LogSubscriber` --- spec/features/users/user_edit_spec.rb | 7 +++++-- spec/features/visitors/password_recovery_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/features/users/user_edit_spec.rb b/spec/features/users/user_edit_spec.rb index e2b9095c8bc..9e80b4fa87d 100644 --- a/spec/features/users/user_edit_spec.rb +++ b/spec/features/users/user_edit_spec.rb @@ -4,6 +4,8 @@ let(:user) { create(:user, :signed_up) } context 'editing email' do + let(:new_email) { 'new_email@test.com' } + before do sign_in_and_2fa_user(user) visit manage_email_path @@ -17,12 +19,13 @@ end scenario 'user receives confirmation message at new address' do - fill_in 'Email', with: 'new_email@test.com' + fill_in 'Email', with: new_email click_button 'Update' open_last_email click_email_link_matching(/confirmation_token/) - expect(page).to have_content('new_email@test.com') + + expect(page).to have_content(new_email) end end diff --git a/spec/features/visitors/password_recovery_spec.rb b/spec/features/visitors/password_recovery_spec.rb index b7a0e753f79..bb7d8bae458 100644 --- a/spec/features/visitors/password_recovery_spec.rb +++ b/spec/features/visitors/password_recovery_spec.rb @@ -256,6 +256,7 @@ context 'user resets password with unconfirmed email address edit' do let(:original_email) { 'original_email@test.com' } + let(:new_email) { 'new_email@test.com' } let(:user) { create(:user, :signed_up, email: original_email) } before do @@ -264,13 +265,12 @@ end it 'receives password reset message at original address' do - fill_in 'Email', with: 'new_email@test.com' + fill_in 'Email', with: new_email click_button 'Update' expect(page).to have_content t('devise.registrations.email_update_needs_confirmation') visit sign_out_url - click_link t('links.passwords.forgot') fill_in 'password_reset_email_form_email', with: original_email click_button t('forms.buttons.continue')