From b6edb93c472541033245104949894d860687362a Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Thu, 11 Dec 2025 17:18:47 +0100 Subject: [PATCH 1/5] Pin sign in attempts to the current session This makes it way more difficult to brute-force a magic link. Original implementation by udiudi. Ref: https://github.com/udiudi/fizzy/pull/1/files Discussion: https://github.com/basecamp/fizzy/discussions/1981 Co-Authored-By: Udi --- app/controllers/concerns/authentication.rb | 12 ++++++++++++ .../sessions/magic_links_controller.rb | 12 ++++++++++-- app/controllers/sessions_controller.rb | 4 ++-- .../sessions/magic_links_controller_test.rb | 16 ++++++++++++++++ test/test_helpers/session_test_helper.rb | 1 + 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 30f54820b..fbe2c6b79 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -107,8 +107,20 @@ module Authentication end end + def email_address_pending_authentication_matches?(email_address) + pending_email_address = session.fetch(:email_address_pending_authentication, "") + + if ActiveSupport::SecurityUtils.secure_compare(email_address, pending_email_address) + session.delete(:email_address_pending_authentication) + true + else + false + end + end + def redirect_to_session_magic_link(magic_link, return_to: nil) serve_development_magic_link(magic_link) + session[:email_address_pending_authentication] = magic_link.identity.email_address if magic_link session[:return_to_after_authenticating] = return_to if return_to redirect_to session_magic_link_url(script_name: nil) end diff --git a/app/controllers/sessions/magic_links_controller.rb b/app/controllers/sessions/magic_links_controller.rb index 71fa1b9ac..1608e9fbb 100644 --- a/app/controllers/sessions/magic_links_controller.rb +++ b/app/controllers/sessions/magic_links_controller.rb @@ -10,14 +10,22 @@ class Sessions::MagicLinksController < ApplicationController def create if magic_link = MagicLink.consume(code) - start_new_session_for magic_link.identity - redirect_to after_sign_in_url(magic_link) + authenticate_with magic_link else redirect_to session_magic_link_path, flash: { shake: true } end end private + def authenticate_with(magic_link) + if email_address_pending_authentication_matches?(magic_link.identity.email_address) + start_new_session_for magic_link.identity + redirect_to after_sign_in_url(magic_link) + else + redirect_to new_session_path, alert: "Authentication failed. Please try again." + end + end + def code params.expect(:code) end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index e32cf3829..f5044bd2a 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -14,8 +14,8 @@ class SessionsController < ApplicationController else signup = Signup.new(email_address: email_address) if signup.valid?(:identity_creation) - identity = signup.create_identity if Account.accepting_signups? - redirect_to_session_magic_link identity + magic_link = signup.create_identity if Account.accepting_signups? + redirect_to_session_magic_link magic_link else head :unprocessable_entity end diff --git a/test/controllers/sessions/magic_links_controller_test.rb b/test/controllers/sessions/magic_links_controller_test.rb index dbde3723e..f2316ed51 100644 --- a/test/controllers/sessions/magic_links_controller_test.rb +++ b/test/controllers/sessions/magic_links_controller_test.rb @@ -14,6 +14,7 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest magic_link = MagicLink.create!(identity: identity) untenanted do + post session_path, params: { email_address: identity.email_address } post session_magic_link_url, params: { code: magic_link.code } assert_response :redirect @@ -28,6 +29,7 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest magic_link = MagicLink.create!(identity: identity, purpose: :sign_up) untenanted do + post session_path, params: { email_address: identity.email_address } post session_magic_link_url, params: { code: magic_link.code } assert_response :redirect @@ -37,6 +39,20 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest end end + test "create with cross-user code" do + identity = identities(:kevin) + other_identity = identities(:jason) + magic_link = MagicLink.create!(identity: other_identity) + + untenanted do + post session_path, params: { email_address: identity.email_address } + post session_magic_link_url, params: { code: magic_link.code } + + assert_redirected_to new_session_path + assert_not cookies[:session_token].present? + end + end + test "create with invalid code" do identity = identities(:kevin) magic_link = MagicLink.create!(identity: identity) diff --git a/test/test_helpers/session_test_helper.rb b/test/test_helpers/session_test_helper.rb index 57d697f75..d07d6612a 100644 --- a/test/test_helpers/session_test_helper.rb +++ b/test/test_helpers/session_test_helper.rb @@ -18,6 +18,7 @@ module SessionTestHelper magic_link = identity.magic_links.order(id: :desc).first untenanted do + post session_path, params: { email_address: identity.email_address } post session_magic_link_url, params: { code: magic_link.code } end From abaed12124467141452636405153de2d4ed6fefb Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Thu, 11 Dec 2025 19:17:39 +0100 Subject: [PATCH 2/5] Prohibit access to magic links unless an email address --- app/controllers/concerns/authentication.rb | 9 ++++++--- app/controllers/sessions/magic_links_controller.rb | 7 +++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index fbe2c6b79..f8ee24642 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -6,6 +6,7 @@ module Authentication before_action :require_authentication after_action :ensure_development_magic_link_not_leaked helper_method :authenticated? + helper_method :email_address_pending_authentication etag { Current.identity.id if authenticated? } @@ -108,9 +109,7 @@ module Authentication end def email_address_pending_authentication_matches?(email_address) - pending_email_address = session.fetch(:email_address_pending_authentication, "") - - if ActiveSupport::SecurityUtils.secure_compare(email_address, pending_email_address) + if ActiveSupport::SecurityUtils.secure_compare(email_address, email_address_pending_authentication || "") session.delete(:email_address_pending_authentication) true else @@ -118,6 +117,10 @@ module Authentication end end + def email_address_pending_authentication + session[:email_address_pending_authentication] + end + def redirect_to_session_magic_link(magic_link, return_to: nil) serve_development_magic_link(magic_link) session[:email_address_pending_authentication] = magic_link.identity.email_address if magic_link diff --git a/app/controllers/sessions/magic_links_controller.rb b/app/controllers/sessions/magic_links_controller.rb index 1608e9fbb..c0632407a 100644 --- a/app/controllers/sessions/magic_links_controller.rb +++ b/app/controllers/sessions/magic_links_controller.rb @@ -2,6 +2,7 @@ class Sessions::MagicLinksController < ApplicationController disallow_account_scope require_unauthenticated_access rate_limit to: 10, within: 15.minutes, only: :create, with: -> { redirect_to session_magic_link_path, alert: "Wait 15 minutes, then try again" } + before_action :ensure_that_email_address_pending_authentication_exists layout "public" @@ -17,6 +18,12 @@ class Sessions::MagicLinksController < ApplicationController end private + def ensure_that_email_address_pending_authentication_exists + unless email_address_pending_authentication.present? + redirect_to new_session_path, alert: "Enter your email address to sign in." + end + end + def authenticate_with(magic_link) if email_address_pending_authentication_matches?(magic_link.identity.email_address) start_new_session_for magic_link.identity From 8a66369d641be115d7cc0ee8a7032d59776c111c Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Thu, 11 Dec 2025 19:18:01 +0100 Subject: [PATCH 3/5] Show the email address you are signing in with --- app/views/sessions/magic_links/show.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/sessions/magic_links/show.html.erb b/app/views/sessions/magic_links/show.html.erb index 947b3c426..a08372af8 100644 --- a/app/views/sessions/magic_links/show.html.erb +++ b/app/views/sessions/magic_links/show.html.erb @@ -9,6 +9,7 @@ <%= form_with url: session_magic_link_path, method: :post, html: { data: { controller: "magic-link" } } do |form| %> +

<%= email_address_pending_authentication %>

<%= form.text_field :code, required: true, class: "input center txt-align-enter txt-large", autofocus: true, autocorrect: "off", autocapitalize: "off", spellcheck: "false", "data-1p-ignore": true, autocomplete: "one-time-code", maxlength: "6", placeholder: "••••••", value: params[:code], From 0dccd7e19c0d8c932c7b5f1cacbe2c3fc6d2f220 Mon Sep 17 00:00:00 2001 From: Jason Zimdars Date: Thu, 11 Dec 2025 19:31:20 -0600 Subject: [PATCH 4/5] Move email address into hint line --- app/views/sessions/magic_links/show.html.erb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/sessions/magic_links/show.html.erb b/app/views/sessions/magic_links/show.html.erb index a08372af8..26a28da9d 100644 --- a/app/views/sessions/magic_links/show.html.erb +++ b/app/views/sessions/magic_links/show.html.erb @@ -9,14 +9,13 @@ <%= form_with url: session_magic_link_path, method: :post, html: { data: { controller: "magic-link" } } do |form| %> -

<%= email_address_pending_authentication %>

<%= form.text_field :code, required: true, class: "input center txt-align-enter txt-large", autofocus: true, autocorrect: "off", autocapitalize: "off", spellcheck: "false", "data-1p-ignore": true, autocomplete: "one-time-code", maxlength: "6", placeholder: "••••••", value: params[:code], data: { magic_link_target: "input", action: "keydown.enter->magic-link#submitOnEnter paste->magic-link#submitOnPaste" } %> <% end %> -

The code you receive will work for <%= distance_of_time_in_words(MagicLink::EXPIRATION_TIME) %>.

+

The code sent to <%= email_address_pending_authentication %> will work for <%= distance_of_time_in_words(MagicLink::EXPIRATION_TIME) %>.

<% if Rails.env.development? && flash[:magic_link_code].present? %> From f4ef9c95808c16326e415324d72854eefef23e1d Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Fri, 12 Dec 2025 07:16:07 +0100 Subject: [PATCH 5/5] Test that you can't go to the magic link screen without an email --- test/controllers/sessions/magic_links_controller_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/controllers/sessions/magic_links_controller_test.rb b/test/controllers/sessions/magic_links_controller_test.rb index f2316ed51..5a4a48e51 100644 --- a/test/controllers/sessions/magic_links_controller_test.rb +++ b/test/controllers/sessions/magic_links_controller_test.rb @@ -5,6 +5,14 @@ class Sessions::MagicLinksControllerTest < ActionDispatch::IntegrationTest untenanted do get session_magic_link_url + assert_response :redirect, "Without an email address pending authentication, should redirect" + assert_redirected_to new_session_path + end + + untenanted do + post session_path, params: { email_address: "test@example.com" } + get session_magic_link_url + assert_response :success end end