From 0160f215f2f35eb060ce408799e088094429cef8 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 6 Dec 2025 16:52:16 -0500 Subject: [PATCH] Validate email before creating identity during sign-up and sign-in Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form field validation that should normally prevent this from occurring, so we just return 422 without validation error messages. Also: - extract redirect_to_session_magic_link helper - some controller refactoring and cleanup --- app/controllers/concerns/authentication.rb | 6 +++++ app/controllers/join_codes_controller.rb | 16 ++++------- app/controllers/sessions_controller.rb | 17 ++++++------ app/controllers/signups_controller.rb | 9 ++++--- test/controllers/sessions_controller_test.rb | 28 +++++++++++++++----- test/controllers/signups_controller_test.rb | 16 ++++++----- test/test_helper.rb | 9 +++++++ 7 files changed, 64 insertions(+), 37 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 89eda5887..4cdbbdb69 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -97,6 +97,12 @@ module Authentication end end + def redirect_to_session_magic_link(magic_link, return_to: nil) + serve_development_magic_link(magic_link) + session[:return_to_after_authenticating] = return_to if return_to + redirect_to session_magic_link_url(script_name: nil) + end + def serve_development_magic_link(magic_link) if Rails.env.development? flash[:magic_link_code] = magic_link&.code diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb index eb328df18..99298880c 100644 --- a/app/controllers/join_codes_controller.rb +++ b/app/controllers/join_codes_controller.rb @@ -20,8 +20,11 @@ class JoinCodesController < ApplicationController elsif identity == Current.identity redirect_to new_users_verification_url(script_name: @join_code.account.slug) else - logout_and_send_new_magic_link(identity) - redirect_to session_magic_link_url(script_name: nil) + terminate_session if Current.identity + + redirect_to_session_magic_link \ + identity.send_magic_link, + return_to: new_users_verification_url(script_name: @join_code.account.slug) end end @@ -37,13 +40,4 @@ class JoinCodesController < ApplicationController render :inactive, status: :gone end end - - def logout_and_send_new_magic_link(identity) - terminate_session if Current.identity - - magic_link = identity.send_magic_link - serve_development_magic_link(magic_link) - - session[:return_to_after_authenticating] = new_users_verification_url(script_name: @join_code.account.slug) - end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5335f58f3..fb9fe0617 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -9,17 +9,16 @@ class SessionsController < ApplicationController end def create - identity = Identity.find_by_email_address(email_address) - - magic_link = if identity - identity.send_magic_link + if identity = Identity.find_by_email_address(email_address) + redirect_to_session_magic_link identity.send_magic_link else - Signup.new(email_address: email_address).create_identity + signup = Signup.new(email_address: email_address) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end - - serve_development_magic_link(magic_link) - - redirect_to session_magic_link_path end def destroy diff --git a/app/controllers/signups_controller.rb b/app/controllers/signups_controller.rb index 217221378..776421201 100644 --- a/app/controllers/signups_controller.rb +++ b/app/controllers/signups_controller.rb @@ -11,9 +11,12 @@ class SignupsController < ApplicationController end def create - magic_link = Signup.new(signup_params).create_identity - serve_development_magic_link(magic_link) - redirect_to session_magic_link_path + signup = Signup.new(signup_params) + if signup.valid?(:identity_creation) + redirect_to_session_magic_link signup.create_identity + else + head :unprocessable_entity + end end private diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index f2318bf46..7ebe4d518 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -36,15 +36,29 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest end end - private - test "destroy" do - sign_in_as :kevin - + test "create with invalid email address" do + # Avoid Sentry exceptions when attackers try to stuff invalid emails. The browser performs form + # field validation that should normally prevent this from occurring, so I'm not worried about + # returning proper validation errors. + without_action_dispatch_exception_handling do untenanted do - delete session_path + assert_no_difference -> { Identity.count } do + post session_path, params: { email_address: "not-a-valid-email" } + end - assert_redirected_to new_session_path - assert_not cookies[:session_token].present? + assert_response :unprocessable_entity end end + end + + test "destroy" do + sign_in_as :kevin + + untenanted do + delete session_path + + assert_redirected_to new_session_path + assert_not cookies[:session_token].present? + end + end end diff --git a/test/controllers/signups_controller_test.rb b/test/controllers/signups_controller_test.rb index 83fea8882..17314ee90 100644 --- a/test/controllers/signups_controller_test.rb +++ b/test/controllers/signups_controller_test.rb @@ -34,15 +34,17 @@ class SignupsControllerTest < ActionDispatch::IntegrationTest end end - test "create with email address containing blanks" do - untenanted do - assert_no_difference -> { Identity.count } do - assert_no_difference -> { MagicLink.count } do - post signup_path, params: { signup: { email_address: "sam smith@example.com" } } + test "create with invalid email address" do + without_action_dispatch_exception_handling do + untenanted do + assert_no_difference -> { Identity.count } do + assert_no_difference -> { MagicLink.count } do + post signup_path, params: { signup: { email_address: "not-a-valid-email" } } + end end - end - assert_response :unprocessable_entity + assert_response :unprocessable_entity + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 03cda99db..4cec836fd 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -61,6 +61,15 @@ class ActionDispatch::IntegrationTest setup do integration_session.default_url_options[:script_name] = "/#{ActiveRecord::FixtureSet.identify("37signals")}" end + + private + def without_action_dispatch_exception_handling + original = Rails.application.config.action_dispatch.show_exceptions + Rails.application.config.action_dispatch.show_exceptions = :none + yield + ensure + Rails.application.config.action_dispatch.show_exceptions = original + end end class ActionDispatch::SystemTestCase