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 <noreply@anthropic.com>

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
 

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

app/views/totp/new.html.erb

@@ -35,8 +35,6 @@
       <div>
         <h3 class="text-lg font-medium text-gray-900 dark:text-gray-100 mb-4">Step 2: Verify</h3>
         <%= form_with url: totp_path, method: :post, class: "space-y-4" do |form| %>
-          <%= hidden_field_tag :totp_secret, @totp_secret %>
-
           <div>
             <%= label_tag :code, "Verification Code", class: "block text-sm font-medium text-gray-700 dark:text-gray-300" %>
             <%= text_field_tag :code,

app/views/totp_mailer/enabled.html.erb

@@ -0,0 +1,16 @@
+<p>Hello,</p>
+
+<p>
+  Two-factor authentication was just enabled on the Clinch account for
+  <strong><%= @user.email_address %></strong>.
+</p>
+
+<p>
+  If you did this, you can ignore this email.
+</p>
+
+<p>
+  If you did <strong>not</strong> do this, your account may have been
+  accessed by someone else. Reset your password immediately and contact
+  your administrator.
+</p>

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.

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")
 

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