SECURITY: Limit name field length of TOTP authenticators and security keys

app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.hbs

@@ -27,6 +27,7 @@
             @value={{this.securityKeyName}}
             id="security-key-name"
             placeholder="security key name"
+            maxlength={{this.maxSecondFactorNameLength}}
           />
         </div>
       </div>

app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.js

@@ -8,6 +8,7 @@ import I18n from "I18n";
 import { inject as service } from "@ember/service";
 import { action } from "@ember/object";
 import { tracked } from "@glimmer/tracking";
+import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user";
 
 export default class SecondFactorAddSecurityKey extends Component {
   @service capabilities;
@@ -16,6 +17,8 @@ export default class SecondFactorAddSecurityKey extends Component {
   @tracked errorMessage = null;
   @tracked securityKeyName;
 
+  maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH;
+
   get webauthnUnsupported() {
     return !isWebauthnSupported();
   }
@@ -30,7 +33,7 @@ export default class SecondFactorAddSecurityKey extends Component {
     } else {
       key = "user.second_factor.security_key.default_name";
     }
-    this.securityKeyName = key;
+    this.securityKeyName = I18n.t(key);
 
     this.loading = true;
     this.args.model.secondFactor

app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.hbs

@@ -46,6 +46,7 @@
           }}</label>
         <div class="controls">
           <SecondFactorInput
+            maxlength={{this.maxSecondFactorNameLength}}
             @value={{this.secondFactorName}}
             @inputId="second-factor-name"
             @placeholder={{i18n "user.second_factor.totp.default_name"}}

app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.js

@@ -2,6 +2,7 @@ import Component from "@glimmer/component";
 import { action } from "@ember/object";
 import { tracked } from "@glimmer/tracking";
 import I18n from "I18n";
+import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user";
 
 export default class SecondFactorAddTotp extends Component {
   @tracked loading = false;
@@ -11,6 +12,8 @@ export default class SecondFactorAddTotp extends Component {
   @tracked errorMessage;
   @tracked secondFactorToken;
 
+  maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH;
+
   @action
   totpRequested() {
     this.args.model.secondFactor

app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.hbs

@@ -10,6 +10,7 @@
           }}</label>
         <Input
           name="security-key-name"
+          maxlength={{this.maxSecondFactorNameLength}}
           @type="text"
           @value={{@model.securityKey.name}}
         />

app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.js

@@ -1,10 +1,13 @@
 import Component from "@glimmer/component";
 import { action } from "@ember/object";
 import { tracked } from "@glimmer/tracking";
+import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user";
 
 export default class SecondFactorEditSecurityKey extends Component {
   @tracked loading = false;
 
+  maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH;
+
   @action
   editSecurityKey() {
     this.loading = true;

app/assets/javascripts/discourse/app/components/modal/second-factor-edit.hbs

@@ -9,6 +9,7 @@
         }}</label>
       <Input
         name="authenticator-name"
+        maxlength={{this.maxSecondFactorNameLength}}
         @type="text"
         @value={{@model.secondFactor.name}}
       />

app/assets/javascripts/discourse/app/components/modal/second-factor-edit.js

@@ -1,10 +1,13 @@
 import Component from "@glimmer/component";
 import { action } from "@ember/object";
 import { tracked } from "@glimmer/tracking";
+import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user";
 
 export default class SecondFactorEdit extends Component {
   @tracked loading = false;
 
+  maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH;
+
   @action
   editSecondFactor() {
     this.loading = true;

app/assets/javascripts/discourse/app/components/second-factor-input.hbs

@@ -11,4 +11,5 @@
   autocapitalize="off"
   autocorrect="off"
   autofocus="autofocus"
+  ...attributes
 />

app/assets/javascripts/discourse/app/models/user.js

@@ -45,6 +45,8 @@ export const SECOND_FACTOR_METHODS = {
   SECURITY_KEY: 3,
 };
 
+export const MAX_SECOND_FACTOR_NAME_LENGTH = 300;
+
 const TEXT_SIZE_COOKIE_NAME = "text_size";
 const COOKIE_EXPIRY_DAYS = 365;
 

app/controllers/users_controller.rb

@@ -1551,6 +1551,11 @@ class UsersController < ApplicationController
   end
 
   def create_second_factor_security_key
+    if current_user.all_security_keys.count >= UserSecurityKey::MAX_KEYS_PER_USER
+      render_json_error(I18n.t("login.too_many_security_keys"), status: 422)
+      return
+    end
+
     challenge_session = DiscourseWebauthn.stage_challenge(current_user, secure_session)
     render json:
              success_json.merge(

app/models/user.rb

@@ -96,6 +96,7 @@ class User < ActiveRecord::Base
   has_many :directory_items
   has_many :email_logs
   has_many :security_keys, -> { where(enabled: true) }, class_name: "UserSecurityKey"
+  has_many :all_security_keys, class_name: "UserSecurityKey"
 
   has_many :badges, through: :user_badges
   has_many :default_featured_user_badges,

app/models/user_second_factor.rb

@@ -2,6 +2,10 @@
 
 class UserSecondFactor < ActiveRecord::Base
   include SecondFactorManager
+
+  MAX_TOTPS_PER_USER = 50
+  MAX_NAME_LENGTH = 300
+
   belongs_to :user
 
   scope :backup_codes, -> { where(method: UserSecondFactor.methods[:backup_codes], enabled: true) }
@@ -10,6 +14,10 @@ class UserSecondFactor < ActiveRecord::Base
 
   scope :all_totps, -> { where(method: UserSecondFactor.methods[:totp]) }
 
+  validates :name, length: { maximum: MAX_NAME_LENGTH }, if: :name_changed?
+
+  validate :count_per_user_does_not_exceed_limit, on: :create
+
   def self.methods
     @methods ||= Enum.new(totp: 1, backup_codes: 2, security_key: 3)
   end
@@ -21,6 +29,16 @@ class UserSecondFactor < ActiveRecord::Base
   def totp_provisioning_uri
     totp_object.provisioning_uri(user.email)
   end
+
+  private
+
+  def count_per_user_does_not_exceed_limit
+    if self.method == UserSecondFactor.methods[:totp]
+      if self.class.where(method: self.method, user_id: self.user_id).count >= MAX_TOTPS_PER_USER
+        errors.add(:base, I18n.t("login.too_many_authenticators"))
+      end
+    end
+  end
 end
 
 # == Schema Information
@@ -35,7 +53,7 @@ end
 #  last_used  :datetime
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
-#  name       :string
+#  name       :string(300)
 #
 # Indexes
 #

app/models/user_security_key.rb

@@ -2,13 +2,26 @@
 
 class UserSecurityKey < ActiveRecord::Base
   belongs_to :user
+  MAX_KEYS_PER_USER = 50
+  MAX_NAME_LENGTH = 300
 
   scope :second_factors,
         -> { where(factor_type: UserSecurityKey.factor_types[:second_factor], enabled: true) }
 
+  validates :name, length: { maximum: MAX_NAME_LENGTH }, if: :name_changed?
+  validate :count_per_user_does_not_exceed_limit, on: :create
+
   def self.factor_types
     @factor_types ||= Enum.new(second_factor: 0, first_factor: 1, multi_factor: 2)
   end
+
+  private
+
+  def count_per_user_does_not_exceed_limit
+    if UserSecurityKey.where(user_id: self.user_id).count >= MAX_KEYS_PER_USER
+      errors.add(:base, I18n.t("login.too_many_security_keys"))
+    end
+  end
 end
 
 # == Schema Information
@@ -21,7 +34,7 @@ end
 #  public_key    :string           not null
 #  factor_type   :integer          default(0), not null
 #  enabled       :boolean          default(TRUE), not null
-#  name          :string           not null
+#  name          :string(300)      not null
 #  last_used     :datetime
 #  created_at    :datetime         not null
 #  updated_at    :datetime         not null

config/locales/server.en.yml

@@ -2674,6 +2674,8 @@ en:
     invalid_security_key: "Invalid security key."
     missing_second_factor_name: "Please provide a name."
     missing_second_factor_code: "Please provide a code."
+    too_many_authenticators: "Sorry, you can't have more than 50 authenticators. Please remove an existing one and try again."
+    too_many_security_keys: "Sorry, you can't have more than 50 security keys. Please remove an existing one and try again."
     second_factor_toggle:
       totp: "Use an authenticator app or security key instead"
       backup_code: "Use a backup code instead"

db/post_migrate/20230823095931_add_limit_to_user_second_factor_name.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class AddLimitToUserSecondFactorName < ActiveRecord::Migration[7.0]
+  def up
+    execute(<<~SQL)
+      UPDATE user_second_factors
+      SET name = LEFT(name, 300)
+      WHERE name IS NOT NULL AND LENGTH(name) > 300
+    SQL
+    change_column :user_second_factors, :name, :string, limit: 300
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/post_migrate/20230823100627_add_limit_to_user_security_key_name.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class AddLimitToUserSecurityKeyName < ActiveRecord::Migration[7.0]
+  def up
+    execute(<<~SQL)
+      UPDATE user_security_keys
+      SET name = LEFT(name, 300)
+      WHERE name IS NOT NULL AND LENGTH(name) > 300
+    SQL
+    change_column :user_security_keys, :name, :string, limit: 300
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/webauthn/security_key_registration_service.rb

@@ -109,7 +109,7 @@ module DiscourseWebauthn
       #     then register the new credential with the account that was denoted in options.user, by
       #     associating it with the credentialId and credentialPublicKey in the attestedCredentialData
       #     in authData, as appropriate for the Relying Party's system.
-      UserSecurityKey.create(
+      UserSecurityKey.create!(
         user: @current_user,
         credential_id: encoded_credential_id,
         public_key: endcoded_public_key,

spec/models/user_second_factor_spec.rb

@@ -1,10 +1,83 @@
 # frozen_string_literal: true
 
 RSpec.describe UserSecondFactor do
+  fab!(:user) { Fabricate(:user) }
+
   describe ".methods" do
     it "should retain the right order" do
       expect(described_class.methods[:totp]).to eq(1)
       expect(described_class.methods[:backup_codes]).to eq(2)
     end
   end
+
+  describe "name length validation" do
+    it "allows the name to be nil" do
+      Fabricate(:user_second_factor_totp, user: user, name: nil)
+    end
+
+    it "doesn't allow the name to be longer than the limit" do
+      expect do
+        Fabricate(
+          :user_second_factor_totp,
+          user: user,
+          name: "a" * (described_class::MAX_NAME_LENGTH + 1),
+        )
+      end.to raise_error(ActiveRecord::RecordInvalid) do |error|
+        expect(error.message).to include(
+          I18n.t("activerecord.errors.messages.too_long", count: described_class::MAX_NAME_LENGTH),
+        )
+      end
+    end
+
+    it "allows a name that is equal to or less than the limit" do
+      expect do
+        Fabricate(
+          :user_second_factor_totp,
+          user: user,
+          name: "a" * described_class::MAX_NAME_LENGTH,
+        )
+      end.not_to raise_error
+    end
+  end
+
+  describe "per-user count validation" do
+    it "doesn't allow a user to have more authenticators than the limit allows" do
+      stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do
+        Fabricate(:user_second_factor_totp, user: user)
+        expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error(
+          ActiveRecord::RecordInvalid,
+        ) do |error|
+          expect(error.message).to include(I18n.t("login.too_many_authenticators"))
+        end
+      end
+    end
+
+    it "doesn't count backup codes in the authenticators limit" do
+      user.generate_backup_codes
+      expect(user.user_second_factors.backup_codes.count).to eq(10)
+
+      stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do
+        Fabricate(:user_second_factor_totp, user: user)
+        expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error(
+          ActiveRecord::RecordInvalid,
+        ) do |error|
+          expect(error.message).to include(I18n.t("login.too_many_authenticators"))
+        end
+      end
+    end
+
+    it "doesn't count authenticators from other users" do
+      another_user = Fabricate(:user)
+      Fabricate(:user_second_factor_totp, user: another_user)
+
+      stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do
+        Fabricate(:user_second_factor_totp, user: user)
+        expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error(
+          ActiveRecord::RecordInvalid,
+        ) do |error|
+          expect(error.message).to include(I18n.t("login.too_many_authenticators"))
+        end
+      end
+    end
+  end
 end

spec/models/user_security_key_spec.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+RSpec.describe UserSecurityKey do
+  fab!(:user) { Fabricate(:user) }
+
+  describe "name length validation" do
+    it "doesn't allow the name to be longer than the limit" do
+      expect do
+        Fabricate(
+          :user_security_key_with_random_credential,
+          user: user,
+          name: "b" * (described_class::MAX_NAME_LENGTH + 1),
+        )
+      end.to raise_error(ActiveRecord::RecordInvalid) do |error|
+        expect(error.message).to include(
+          I18n.t("activerecord.errors.messages.too_long", count: described_class::MAX_NAME_LENGTH),
+        )
+      end
+    end
+
+    it "allows a name that's under the limit" do
+      expect do
+        Fabricate(
+          :user_security_key_with_random_credential,
+          user: user,
+          name: "b" * described_class::MAX_NAME_LENGTH,
+        )
+      end.not_to raise_error
+    end
+  end
+
+  describe "per-user count validation" do
+    it "doesn't allow a user to have more security keys than the limit allows" do
+      stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do
+        Fabricate(:user_security_key_with_random_credential, user: user)
+        expect do
+          Fabricate(:user_security_key_with_random_credential, user: user)
+        end.to raise_error(ActiveRecord::RecordInvalid) do |error|
+          expect(error.message).to include(I18n.t("login.too_many_security_keys"))
+        end
+      end
+    end
+
+    it "doesn't count security keys from other users" do
+      another_user = Fabricate(:user)
+      Fabricate(:user_security_key_with_random_credential, user: another_user)
+
+      stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do
+        Fabricate(:user_security_key_with_random_credential, user: user)
+        expect do
+          Fabricate(:user_security_key_with_random_credential, user: user)
+        end.to raise_error(ActiveRecord::RecordInvalid) do |error|
+          expect(error.message).to include(I18n.t("login.too_many_security_keys"))
+        end
+      end
+    end
+  end
+end

spec/requests/users_controller_spec.rb

@@ -5539,6 +5539,46 @@ RSpec.describe UsersController do
         expect(response.parsed_body["error"]).to eq(I18n.t("login.missing_second_factor_code"))
       end
     end
+
+    it "doesn't allow creating too many TOTPs" do
+      Fabricate(:user_second_factor_totp, user: user1)
+
+      create_totp
+      staged_totp_key = read_secure_session["staged-totp-#{user1.id}"]
+      token = ROTP::TOTP.new(staged_totp_key).now
+
+      stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do
+        post "/users/enable_second_factor_totp.json",
+             params: {
+               name: "test",
+               second_factor_token: token,
+             }
+      end
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_authenticators"))
+
+      expect(user1.user_second_factors.count).to eq(1)
+    end
+
+    it "doesn't allow the TOTP name to exceed the limit" do
+      create_totp
+      staged_totp_key = read_secure_session["staged-totp-#{user1.id}"]
+      token = ROTP::TOTP.new(staged_totp_key).now
+
+      post "/users/enable_second_factor_totp.json",
+           params: {
+             name: "a" * (UserSecondFactor::MAX_NAME_LENGTH + 1),
+             second_factor_token: token,
+           }
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to include(
+        "Name is too long (maximum is 300 characters)",
+      )
+
+      expect(user1.user_second_factors.count).to eq(0)
+    end
   end
 
   describe "#update_second_factor" do
@@ -5716,6 +5756,13 @@ RSpec.describe UsersController do
       )
     end
 
+    it "doesn't create a challenge if the user has the maximum number allowed of security keys" do
+      Fabricate(:user_security_key_with_random_credential, user: user1)
+      stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) { create_second_factor_security_key }
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_security_keys"))
+    end
+
     context "if the user has security key credentials already" do
       fab!(:user_security_key) { Fabricate(:user_security_key_with_random_credential, user: user1) }
 
@@ -5745,6 +5792,43 @@ RSpec.describe UsersController do
         )
         expect(user1.security_keys.last.name).to eq(valid_security_key_create_post_data[:name])
       end
+
+      it "doesn't allow creating too many security keys" do
+        simulate_localhost_webauthn_challenge
+        create_second_factor_security_key
+        _response_parsed = response.parsed_body
+
+        Fabricate(:user_security_key_with_random_credential, user: user1)
+
+        stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do
+          post "/u/register_second_factor_security_key.json",
+               params: valid_security_key_create_post_data
+        end
+
+        expect(response.status).to eq(422)
+        expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_security_keys"))
+
+        expect(user1.security_keys.count).to eq(1)
+      end
+
+      it "doesn't allow the security key name to exceed the limit" do
+        simulate_localhost_webauthn_challenge
+        create_second_factor_security_key
+        _response_parsed = response.parsed_body
+
+        post "/u/register_second_factor_security_key.json",
+             params:
+               valid_security_key_create_post_data.merge(
+                 name: "a" * (UserSecurityKey::MAX_NAME_LENGTH + 1),
+               )
+
+        expect(response.status).to eq(422)
+        expect(response.parsed_body["errors"]).to include(
+          "Name is too long (maximum is 300 characters)",
+        )
+
+        expect(user1.security_keys.count).to eq(0)
+      end
     end
 
     context "when the creation parameters are invalid" do