From c0b2765227abfc8da0e1fbb27272cefae84494c5 Mon Sep 17 00:00:00 2001 From: Nick Malcolm Date: Thu, 4 Apr 2024 11:11:15 +1300 Subject: [PATCH] Adds an option to strictly enforce single recipients for emails Devise sends email containing sensitive values such as confirmation URLs, password reset URLs, and unlock URLs. In most (all?) cases, these should only be sent to a single person so that they alone can click the link. If the email is sent to multiple addresses, another person could click the link. Set `Devise.strict_single_recipient_emails` to an array of actions to raise an error when the email would be sent to more than one email address. By default Devise is secure: - `Devise.email_regexp` will reject email addresses containing separators (`,;`) - Devise gets a single email address from `record.email` However, when using `opts`, and particularly if providing untrusted user input to `opts`, multiple values could be present in `to:`, `cc:`, or `bcc:`. Example: ```ruby # POST https://your-app.com/customised-reset-password?email[]="attacker@example.com"&email[]="victim@example.com" # Returns the victim's user user = User.find_by(email: params[:email]) # unsafe, will send the link to two addresses: Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]}) # safe, devise will use the user's email address Devise.reset_password_instructions(user, 'fake-token') # safe, will raise error: Devise.strict_single_recipient_emails = [ :confirmation_instructions, :reset_password_instructions, :unlock_instructions ] Devise.mailer.reset_password_instructions(user, 'fake-token', {to: params[:email]}) ``` --- lib/devise.rb | 11 +++ lib/devise/mailers/helpers.rb | 44 +++++++++++- test/controllers/internal_helpers_test.rb | 2 +- test/devise_test.rb | 2 +- test/mailers/helpers/helpers_test.rb | 86 +++++++++++++++++++++++ 5 files changed, 142 insertions(+), 3 deletions(-) create mode 100644 test/mailers/helpers/helpers_test.rb diff --git a/lib/devise.rb b/lib/devise.rb index 3847e190c..2c3e09ed8 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -308,6 +308,17 @@ module Test mattr_accessor :sign_in_after_change_password @@sign_in_after_change_password = true + # When sending an email to an action set below, raise an error if + # there is more than one recipient. For example: + # @@strict_single_recipient_emails = [ + # :confirmation_instructions, + # :reset_password_instructions, + # :unlock_instructions + # ] + # By default any action can be sent to multiple recipients. + mattr_accessor :strict_single_recipient_emails + @@strict_single_recipient_emails = [] + # Default way to set up Devise. Run rails generate devise_install to create # a fresh initializer with all configuration values. def self.setup diff --git a/lib/devise/mailers/helpers.rb b/lib/devise/mailers/helpers.rb index 29a491970..e7bcaa6a9 100644 --- a/lib/devise/mailers/helpers.rb +++ b/lib/devise/mailers/helpers.rb @@ -5,6 +5,8 @@ module Mailers module Helpers extend ActiveSupport::Concern + MultipleRecipientsError = Class.new(ArgumentError) + included do include Devise::Controllers::ScopedViews end @@ -16,7 +18,11 @@ module Helpers # Configure default email options def devise_mail(record, action, opts = {}, &block) initialize_from_record(record) - mail headers_for(action, opts), &block + + headers = headers_for(action, opts) + validate_single_recipient_in_headers!(headers) if Devise.strict_single_recipient_emails.include?(action) + + mail headers, &block end def initialize_from_record(record) @@ -82,6 +88,42 @@ def subject_for(key) I18n.t(:"#{devise_mapping.name}_subject", scope: [:devise, :mailer, key], default: [:subject, key.to_s.humanize]) end + + # It is possible to send email to one or more recipients in one + # email by setting a list of emails in the :to key, or by :cc or + # :bcc-ing recipients. + # https://guides.rubyonrails.org/action_mailer_basics.html#sending-email-to-multiple-recipients + # + # This method ensures the headers contain a single recipient. + def validate_single_recipient_in_headers!(headers) + return unless headers + + symbolized_headers = headers.symbolize_keys + + if headers.keys.length != symbolized_headers.keys.length + raise MultipleRecipientsError, "headers has colliding key names" + end + + if symbolized_headers[:cc] || symbolized_headers[:bcc] + raise MultipleRecipientsError, 'headers[:cc] and headers[:bcc] are not allowed' + end + + if symbolized_headers[:to] && !validate_single_recipient_in_email(symbolized_headers[:to]) + raise MultipleRecipientsError, 'headers[:to] must be a string not containing ; or ,' + end + + true + end + + # Returns true if email is a String not containing email + # separators: commas (RFC5322) or semicolons (RFC1485). + # + # Unlike Devise.email_regexp (which can be overridden), it does + # not validate that the email is valid. + def validate_single_recipient_in_email(email) + email.is_a?(String) && !email.match(/[;,]/) + end + end end end diff --git a/test/controllers/internal_helpers_test.rb b/test/controllers/internal_helpers_test.rb index 124c8df06..e71ea0f34 100644 --- a/test/controllers/internal_helpers_test.rb +++ b/test/controllers/internal_helpers_test.rb @@ -5,7 +5,7 @@ class MyController < DeviseController end -class HelpersTest < Devise::ControllerTestCase +class InternalHelpersTest < Devise::ControllerTestCase tests MyController def setup diff --git a/test/devise_test.rb b/test/devise_test.rb index 532aa57dc..bb7f2b36b 100644 --- a/test/devise_test.rb +++ b/test/devise_test.rb @@ -97,7 +97,7 @@ class DeviseTest < ActiveSupport::TestCase test 'Devise.email_regexp should match valid email addresses' do valid_emails = ["test@example.com", "jo@jo.co", "f4$_m@you.com", "testing.example@example.com.ua", "test@tt", "test@valid---domain.com"] - non_valid_emails = ["rex", "test user@example.com", "test_user@example server.com"] + non_valid_emails = ["rex", "test user@example.com", "test_user@example server.com", "test_user@example,rex@server.com", "test_user@example;rex@server.com"] valid_emails.each do |email| assert_match Devise.email_regexp, email diff --git a/test/mailers/helpers/helpers_test.rb b/test/mailers/helpers/helpers_test.rb new file mode 100644 index 000000000..f791f3b6a --- /dev/null +++ b/test/mailers/helpers/helpers_test.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'test_helper' + +class HelpersTest < ActiveSupport::TestCase + def setup + Devise.strict_single_recipient_emails = [] + end + + def user + @user ||= create_user + end + + def send_mail(opts={}) + Devise.mailer.reset_password_instructions( + user, + 'fake-token', + opts, + ).deliver + end + + test "an email passed in opts is used instead of the record's email" do + mail = send_mail({to: "test2@example.com"}) + + assert_equal ["test2@example.com"], mail.to + end + + test "multiple recipients are permitted when setting configured to empty array" do + mail = send_mail({ + to: ["test1@example.com", "test2@example.com"], + bcc: ["admin@example.com"] + }) + + assert_equal ["test1@example.com", "test2@example.com"], mail.to + assert_equal ["admin@example.com"], mail.bcc + end + + test "multiple recipients are permitted setting configured to another action" do + Devise.strict_single_recipient_emails = [:unlock_instructions] + mail = send_mail({ + to: ["test1@example.com", "test2@example.com"], + bcc: ["admin@example.com"] + }) + + assert_equal ["test1@example.com", "test2@example.com"], mail.to + assert_equal ["admin@example.com"], mail.bcc + end + + test "single to recipient does not raises an error when action has strict enforcement" do + Devise.strict_single_recipient_emails = [:reset_password_instructions] + mail = send_mail + + assert_equal [user.email], mail.to + end + + test "multiple to recipients raises an error when action has strict enforcement" do + + Devise.strict_single_recipient_emails = [:reset_password_instructions] + + [ + { to: 'test1@example.com', 'to' => 'test2@example.com' }, + { to: 'test1@example.com,test2@example.com' }, + { to: 'test1@example.com;test2@example.com' }, + { to: ['test1@example.com', 'test2@example.com'] } + ].each do |headers| + assert_raises(Devise::Mailers::Helpers::MultipleRecipientsError) do + send_mail(headers) + end + end + end + + test "bcc raises an error when action has strict enforcement" do + Devise.strict_single_recipient_emails = [:reset_password_instructions] + assert_raises(Devise::Mailers::Helpers::MultipleRecipientsError) do + send_mail({bcc: ["admin@example.com"]}) + end + end + + test "cc raises an error when action has strict enforcement" do + Devise.strict_single_recipient_emails = [:reset_password_instructions] + assert_raises(Devise::Mailers::Helpers::MultipleRecipientsError) do + send_mail({cc: ["admin@example.com"]}) + end + end + +end \ No newline at end of file