Merge pull request #1932 from basecamp/flavorjones-validate-email

Validate Identity email address

app/helpers/html_helper.rb

@@ -9,7 +9,7 @@ module HtmlHelper
 
   private
     EXCLUDED_ELEMENTS = %w[ a figcaption pre code ]
-    EMAIL_REGEXP = /\b[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\b/
+    EMAIL_AUTOLINK_REGEXP = /\b[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\b/
     URL_REGEXP = URI::DEFAULT_PARSER.make_regexp(%w[http https])
 
     def auto_link(fragment)
@@ -48,7 +48,7 @@ module HtmlHelper
     end
 
     def auto_link_emails(text)
-      text.gsub!(EMAIL_REGEXP) do |match|
+      text.gsub!(EMAIL_AUTOLINK_REGEXP) do |match|
         %(<a href="mailto:#{match}">#{match}</a>)
       end
     end

app/models/identity.rb

@@ -10,6 +10,7 @@ class Identity < ApplicationRecord
 
   before_destroy :deactivate_users
 
+  validates :email_address, format: { with: URI::MailTo::EMAIL_REGEXP }
   normalizes :email_address, with: ->(value) { value.strip.downcase.presence }
 
   def send_magic_link(**attributes)

test/controllers/signups_controller_test.rb

@@ -34,6 +34,18 @@ 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" } }
+        end
+      end
+
+      assert_response :unprocessable_entity
+    end
+  end
+
   test "create for an authenticated user" do
     identity = identities(:kevin)
     sign_in_as identity

test/models/identity_test.rb

@@ -13,6 +13,23 @@ class IdentityTest < ActiveSupport::TestCase
     end
   end
 
+  test "email address format validation" do
+    invalid_emails = [
+      "sam smith@example.com",       # space in local part
+      "@example.com",                # missing local part
+      "test@",                       # missing domain
+      "test",                        # missing @ and domain
+      "<script>@example.com",        # angle brackets
+      "test@example.com\nX-Inject:" # newline (header injection attempt)
+    ]
+
+    invalid_emails.each do |email|
+      identity = Identity.new(email_address: email)
+      assert_not identity.valid?, "expected #{email.inspect} to be invalid"
+      assert identity.errors[:email_address].any?, "expected error on email_address for #{email.inspect}"
+    end
+  end
+
   test "join" do
     identity = identities(:david)
     account = accounts(:initech)