From b876e02c3a894ccb7df89d8cfab40a197795b5fb Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Mon, 20 Apr 2026 18:17:50 +1000 Subject: [PATCH] Hold TOTP enrollment secret server-side and email user on activation TOTP enrollment previously round-tripped the generated secret through a hidden form field and saved whatever the client submitted, letting an attacker with session access enroll a 2FA device they control by posting their own secret plus a matching code. Stash the secret in the session at GET /totp/new, read it only from the session at POST /totp, and drop the hidden field from the view. Notify the user by email on successful enrollment so unauthorized activations are visible even if a new vector appears later. Co-Authored-By: Claude Opus 4 --- app/controllers/totp_controller.rb | 15 +++++- app/mailers/totp_mailer.rb | 7 +++ app/views/totp/new.html.erb | 2 - app/views/totp_mailer/enabled.html.erb | 16 ++++++ app/views/totp_mailer/enabled.text.erb | 9 ++++ test/controllers/totp_security_test.rb | 73 ++++++++++++++++++++++++++ test/mailers/totp_mailer_test.rb | 19 +++++++ 7 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 app/mailers/totp_mailer.rb create mode 100644 app/views/totp_mailer/enabled.html.erb create mode 100644 app/views/totp_mailer/enabled.text.erb create mode 100644 test/mailers/totp_mailer_test.rb diff --git a/app/controllers/totp_controller.rb b/app/controllers/totp_controller.rb index 75d1513..39774b7 100644 --- a/app/controllers/totp_controller.rb +++ b/app/controllers/totp_controller.rb @@ -12,6 +12,10 @@ class TotpController < ApplicationController @totp_secret = ROTP::Base32.random @provisioning_uri = ROTP::TOTP.new(@totp_secret, issuer: "Clinch").provisioning_uri(@user.email_address) + # Hold the secret server-side until the user confirms it with a valid code. + # The view no longer round-trips it through a hidden form field. + session[:pending_totp_secret] = @totp_secret + # Generate QR code require "rqrcode" @qr_code = RQRCode::QRCode.new(@provisioning_uri) @@ -19,9 +23,14 @@ class TotpController < ApplicationController # POST /totp - Verify TOTP code and enable 2FA def create - totp_secret = params[:totp_secret] + totp_secret = session[:pending_totp_secret] code = params[:code] + unless totp_secret + redirect_to new_totp_path, alert: "Your TOTP setup session expired. Please start again." + return + end + # Verify the code works totp = ROTP::TOTP.new(totp_secret) if totp.verify(code, drift_behind: 30, drift_ahead: 30) @@ -30,6 +39,10 @@ class TotpController < ApplicationController plain_codes = @user.send(:generate_backup_codes) # Use private method from User model @user.save! + # Consume the pending secret and notify the user that 2FA is now active + session.delete(:pending_totp_secret) + TotpMailer.enabled(@user).deliver_later + # Store plain codes temporarily in session for display after redirect session[:temp_backup_codes] = plain_codes diff --git a/app/mailers/totp_mailer.rb b/app/mailers/totp_mailer.rb new file mode 100644 index 0000000..0514186 --- /dev/null +++ b/app/mailers/totp_mailer.rb @@ -0,0 +1,7 @@ +class TotpMailer < ApplicationMailer + def enabled(user) + @user = user + mail subject: "Two-factor authentication enabled on your account", + to: user.email_address + end +end diff --git a/app/views/totp/new.html.erb b/app/views/totp/new.html.erb index 63756dc..276e5d5 100644 --- a/app/views/totp/new.html.erb +++ b/app/views/totp/new.html.erb @@ -35,8 +35,6 @@

Step 2: Verify

<%= form_with url: totp_path, method: :post, class: "space-y-4" do |form| %> - <%= hidden_field_tag :totp_secret, @totp_secret %> -
<%= label_tag :code, "Verification Code", class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %> <%= text_field_tag :code, diff --git a/app/views/totp_mailer/enabled.html.erb b/app/views/totp_mailer/enabled.html.erb new file mode 100644 index 0000000..6bbc92a --- /dev/null +++ b/app/views/totp_mailer/enabled.html.erb @@ -0,0 +1,16 @@ +

Hello,

+ +

+ Two-factor authentication was just enabled on the Clinch account for + <%= @user.email_address %>. +

+ +

+ If you did this, you can ignore this email. +

+ +

+ If you did not do this, your account may have been + accessed by someone else. Reset your password immediately and contact + your administrator. +

diff --git a/app/views/totp_mailer/enabled.text.erb b/app/views/totp_mailer/enabled.text.erb new file mode 100644 index 0000000..0fcb18c --- /dev/null +++ b/app/views/totp_mailer/enabled.text.erb @@ -0,0 +1,9 @@ +Hello, + +Two-factor authentication was just enabled on the Clinch account for +<%= @user.email_address %>. + +If you did this, you can ignore this email. + +If you did NOT do this, your account may have been accessed by someone +else. Reset your password immediately and contact your administrator. diff --git a/test/controllers/totp_security_test.rb b/test/controllers/totp_security_test.rb index 43d4068..9d1fa74 100644 --- a/test/controllers/totp_security_test.rb +++ b/test/controllers/totp_security_test.rb @@ -257,6 +257,79 @@ class TotpSecurityTest < ActionDispatch::IntegrationTest # TOTP RECOVERY FLOW TESTS # ==================== + # ==================== + # TOTP ENROLLMENT — SECRET BINDING TESTS (H-1) + # ==================== + + test "enrollment uses the server-issued secret, not any client-submitted one" do + user = User.create!(email_address: "totp_enroll_binding@example.com", password: "password123") + post signin_path, params: {email_address: "totp_enroll_binding@example.com", password: "password123"} + assert_response :redirect + + # Start enrollment: server generates a secret and stores it in the session + get new_totp_path + assert_response :success + + # Attacker-supplied secret + a code that's valid for THAT secret must not + # succeed. The server should only ever consider its own session-stored secret. + attacker_secret = ROTP::Base32.random + attacker_code = ROTP::TOTP.new(attacker_secret).now + + assert_no_enqueued_emails do + post totp_path, params: {totp_secret: attacker_secret, code: attacker_code} + end + + user.reload + assert_nil user.totp_secret, "attacker-chosen secret must not be saved" + assert_not user.totp_enabled? + + user.sessions.delete_all + user.destroy + end + + test "enrollment succeeds, clears pending secret, and notifies the user by email" do + user = User.create!(email_address: "totp_enroll_success@example.com", password: "password123") + post signin_path, params: {email_address: "totp_enroll_success@example.com", password: "password123"} + assert_response :redirect + + get new_totp_path + assert_response :success + + # Pull the session-stored secret and produce a valid code for it + pending_secret = session[:pending_totp_secret] + assert pending_secret.present?, "new action should stash the secret in session" + valid_code = ROTP::TOTP.new(pending_secret).now + + assert_enqueued_email_with TotpMailer, :enabled, args: [user] do + post totp_path, params: {code: valid_code} + end + + user.reload + assert_equal pending_secret, user.totp_secret + assert user.totp_enabled? + assert_nil session[:pending_totp_secret], "pending secret must be cleared after enrollment" + + user.sessions.delete_all + user.destroy + end + + test "enrollment without a prior GET new is rejected" do + user = User.create!(email_address: "totp_enroll_no_session@example.com", password: "password123") + post signin_path, params: {email_address: "totp_enroll_no_session@example.com", password: "password123"} + assert_response :redirect + + # Skip GET /totp/new — no session[:pending_totp_secret] is set + post totp_path, params: {code: "123456"} + assert_redirected_to new_totp_path + + user.reload + assert_nil user.totp_secret + assert_not user.totp_enabled? + + user.sessions.delete_all + user.destroy + end + test "user can sign in with backup code when TOTP device is lost" do user = User.create!(email_address: "totp_recovery_test@example.com", password: "password123") diff --git a/test/mailers/totp_mailer_test.rb b/test/mailers/totp_mailer_test.rb new file mode 100644 index 0000000..97683fe --- /dev/null +++ b/test/mailers/totp_mailer_test.rb @@ -0,0 +1,19 @@ +require "test_helper" + +class TotpMailerTest < ActionMailer::TestCase + test "enabled email addresses the user and names the event" do + user = User.create!(email_address: "totp_mailer_test@example.com", password: "password123") + + email = TotpMailer.enabled(user) + + assert_equal ["totp_mailer_test@example.com"], email.to + assert_equal "Two-factor authentication enabled on your account", email.subject + text_body = email.text_part.body.to_s + html_body = email.html_part.body.to_s + assert_match "totp_mailer_test@example.com", text_body + assert_match "totp_mailer_test@example.com", html_body + assert_match(/Reset your password/i, text_body) + + user.destroy + end +end