From cb88db5eef0c3dcfee104310f73a246aaf07295e Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Fri, 20 Sep 2024 15:18:58 -0700 Subject: [PATCH] Add length validations for user-supplied string attributes Signed-off-by: Samuel Giddins --- app/models/gem_name_reservation.rb | 2 +- app/models/gem_typo_exception.rb | 2 +- app/models/linkset.rb | 2 ++ app/models/oidc/api_key_role.rb | 2 +- .../oidc/trusted_publisher/github_action.rb | 8 +++----- app/models/user.rb | 5 ++--- app/models/webauthn_credential.rb | 6 +++--- app/models/webauthn_verification.rb | 2 +- test/models/gem_name_reservation_test.rb | 1 + test/models/gem_typo_exception_test.rb | 1 + test/models/linkset_test.rb | 7 +++++++ test/models/oidc/api_key_role_test.rb | 2 ++ .../trusted_publisher/github_action_test.rb | 20 +++++++++++++++---- test/models/user_test.rb | 3 ++- test/models/webauthn_credential_test.rb | 4 ++++ test/unit/webauthn_verification_test.rb | 2 ++ 16 files changed, 49 insertions(+), 20 deletions(-) diff --git a/app/models/gem_name_reservation.rb b/app/models/gem_name_reservation.rb index 4e3e69eb566..cb4e685a20a 100644 --- a/app/models/gem_name_reservation.rb +++ b/app/models/gem_name_reservation.rb @@ -1,5 +1,5 @@ class GemNameReservation < ApplicationRecord - validates :name, uniqueness: { case_sensitive: false }, presence: true + validates :name, uniqueness: { case_sensitive: false }, presence: true, length: { maximum: Gemcutter::MAX_FIELD_LENGTH } validate :downcase_name_check def self.reserved?(name) diff --git a/app/models/gem_typo_exception.rb b/app/models/gem_typo_exception.rb index 98db3b92cde..668c5e3830b 100644 --- a/app/models/gem_typo_exception.rb +++ b/app/models/gem_typo_exception.rb @@ -1,5 +1,5 @@ class GemTypoException < ApplicationRecord - validates :name, presence: true, uniqueness: { case_sensitive: false } + validates :name, presence: true, uniqueness: { case_sensitive: false }, length: { maximum: Gemcutter::MAX_FIELD_LENGTH } validate :rubygems_name private diff --git a/app/models/linkset.rb b/app/models/linkset.rb index dc0be7a6d3d..ccea8016728 100644 --- a/app/models/linkset.rb +++ b/app/models/linkset.rb @@ -11,6 +11,8 @@ class Linkset < ApplicationRecord allow_nil: true, allow_blank: true, message: "does not appear to be a valid URL" + + validates url, length: { maximum: Gemcutter::MAX_FIELD_LENGTH } end def empty? diff --git a/app/models/oidc/api_key_role.rb b/app/models/oidc/api_key_role.rb index 1f7edf17a85..9111cc723e6 100644 --- a/app/models/oidc/api_key_role.rb +++ b/app/models/oidc/api_key_role.rb @@ -22,7 +22,7 @@ class OIDC::ApiKeyRole < ApplicationRecord scope :deleted, -> { where.not(deleted_at: nil) } scope :active, -> { where(deleted_at: nil) } - validates :name, presence: true, length: { maximum: 255 }, uniqueness: { scope: :user_id } + validates :name, presence: true, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, uniqueness: { scope: :user_id } attribute :api_key_permissions, Types::JsonDeserializable.new(OIDC::ApiKeyPermissions) validates :api_key_permissions, presence: true, nested: true diff --git a/app/models/oidc/trusted_publisher/github_action.rb b/app/models/oidc/trusted_publisher/github_action.rb index dd68c8132ec..11f02a61ff1 100644 --- a/app/models/oidc/trusted_publisher/github_action.rb +++ b/app/models/oidc/trusted_publisher/github_action.rb @@ -8,11 +8,9 @@ class OIDC::TrustedPublisher::GitHubAction < ApplicationRecord before_validation :find_github_repository_owner_id - validates :repository_owner, presence: true - validates :repository_name, presence: true - validates :workflow_filename, presence: true - validates :environment, presence: true, allow_nil: true - validates :repository_owner_id, presence: true + validates :repository_owner, :repository_name, :workflow_filename, :repository_owner_id, + presence: true, length: { maximum: Gemcutter::MAX_FIELD_LENGTH } + validates :environment, presence: true, allow_nil: true, length: { maximum: Gemcutter::MAX_FIELD_LENGTH } validate :unique_publisher validate :workflow_filename_format diff --git a/app/models/user.rb b/app/models/user.rb index dbf55be8aa2..a740033e4a3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -68,7 +68,7 @@ class User < ApplicationRecord has_many :organizations, through: :memberships validates :email, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: URI::MailTo::EMAIL_REGEXP }, presence: true, -uniqueness: { case_sensitive: false } + uniqueness: { case_sensitive: false } validates :unconfirmed_email, length: { maximum: Gemcutter::MAX_FIELD_LENGTH }, format: { with: URI::MailTo::EMAIL_REGEXP }, allow_blank: true validates :handle, uniqueness: { case_sensitive: false }, allow_nil: true, if: :handle_changed? @@ -78,9 +78,8 @@ class User < ApplicationRecord validates :twitter_username, format: { with: /\A[a-zA-Z0-9_]*\z/, message: "can only contain letters, numbers, and underscores" - }, allow_nil: true + }, allow_nil: true, length: { within: 0..20 } - validates :twitter_username, length: { within: 0..20 }, allow_nil: true validates :password, length: { within: 10..200 }, unpwn: true, diff --git a/app/models/webauthn_credential.rb b/app/models/webauthn_credential.rb index 0d948384839..99ca6b3ce34 100644 --- a/app/models/webauthn_credential.rb +++ b/app/models/webauthn_credential.rb @@ -1,9 +1,9 @@ class WebauthnCredential < ApplicationRecord belongs_to :user - validates :external_id, uniqueness: true, presence: true - validates :public_key, presence: true - validates :nickname, presence: true, uniqueness: { scope: :user_id } + validates :external_id, uniqueness: true, presence: true, length: { maximum: 512 } + validates :public_key, presence: true, length: { maximum: 512 } + validates :nickname, presence: true, uniqueness: { scope: :user_id }, length: { maximum: 64 } validates :sign_count, presence: true, numericality: { greater_than_or_equal_to: 0 } after_create :send_creation_email diff --git a/app/models/webauthn_verification.rb b/app/models/webauthn_verification.rb index 052d8b97925..95b19ae1485 100644 --- a/app/models/webauthn_verification.rb +++ b/app/models/webauthn_verification.rb @@ -2,7 +2,7 @@ class WebauthnVerification < ApplicationRecord belongs_to :user validates :user_id, uniqueness: true - validates :path_token, presence: true, uniqueness: true + validates :path_token, presence: true, uniqueness: true, length: { maximum: 128 } validates :path_token_expires_at, presence: true def expire_path_token diff --git a/test/models/gem_name_reservation_test.rb b/test/models/gem_name_reservation_test.rb index 2a46647f449..aab29c8a16f 100644 --- a/test/models/gem_name_reservation_test.rb +++ b/test/models/gem_name_reservation_test.rb @@ -13,6 +13,7 @@ class GemNameReservationTest < ActiveSupport::TestCase should_not allow_value("Abc").for(:name) should allow_value("abc").for(:name) should validate_uniqueness_of(:name).case_insensitive + should validate_length_of(:name).is_at_most(Gemcutter::MAX_FIELD_LENGTH) end context "#reserved?" do diff --git a/test/models/gem_typo_exception_test.rb b/test/models/gem_typo_exception_test.rb index f9d4acef3ac..1dc77b7fe9a 100644 --- a/test/models/gem_typo_exception_test.rb +++ b/test/models/gem_typo_exception_test.rb @@ -3,6 +3,7 @@ class GemTypoExceptionTest < ActiveSupport::TestCase context "name validations" do should validate_uniqueness_of(:name).case_insensitive + should validate_length_of(:name).is_at_most(Gemcutter::MAX_FIELD_LENGTH) should "be a valid factory" do assert_predicate build(:gem_typo_exception), :valid? diff --git a/test/models/linkset_test.rb b/test/models/linkset_test.rb index 261890a1bcc..a57d2c58db9 100644 --- a/test/models/linkset_test.rb +++ b/test/models/linkset_test.rb @@ -36,4 +36,11 @@ class LinksetTest < ActiveSupport::TestCase assert_equal @spec.homepage, @linkset.home end end + + context "validations" do + %w[home code docs wiki mail bugs].each do |link| + should allow_value("http://example.com").for(link.to_sym) + should validate_length_of(link).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + end + end end diff --git a/test/models/oidc/api_key_role_test.rb b/test/models/oidc/api_key_role_test.rb index 790f6a1ee16..bce620fd32b 100644 --- a/test/models/oidc/api_key_role_test.rb +++ b/test/models/oidc/api_key_role_test.rb @@ -8,6 +8,8 @@ class OIDC::ApiKeyRoleTest < ActiveSupport::TestCase should have_many :id_tokens should validate_presence_of :api_key_permissions should validate_presence_of :access_policy + should validate_presence_of :name + should validate_length_of(:name).is_at_most(Gemcutter::MAX_FIELD_LENGTH) setup do @role = build(:oidc_api_key_role) diff --git a/test/models/oidc/trusted_publisher/github_action_test.rb b/test/models/oidc/trusted_publisher/github_action_test.rb index b87e8e63e8f..bae69432ff9 100644 --- a/test/models/oidc/trusted_publisher/github_action_test.rb +++ b/test/models/oidc/trusted_publisher/github_action_test.rb @@ -7,10 +7,22 @@ class OIDC::TrustedPublisher::GitHubActionTest < ActiveSupport::TestCase should have_many(:rubygem_trusted_publishers) should have_many(:api_keys).inverse_of(:owner) - should validate_presence_of(:repository_owner) - should validate_presence_of(:repository_name) - should validate_presence_of(:workflow_filename) - should validate_presence_of(:repository_owner_id) + context "validations" do + setup do + stub_request(:get, Addressable::Template.new("https://api.github.com/users/{user}")) + .to_return(status: 404, body: "", headers: {}) + end + should validate_presence_of(:repository_owner) + should validate_length_of(:repository_owner).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + should validate_presence_of(:repository_name) + should validate_length_of(:repository_name).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + should validate_presence_of(:workflow_filename) + should validate_length_of(:workflow_filename).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + should validate_presence_of(:repository_owner_id) + should validate_length_of(:repository_owner_id).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + + should validate_length_of(:environment).is_at_most(Gemcutter::MAX_FIELD_LENGTH) + end test "validates publisher uniqueness" do publisher = create(:oidc_trusted_publisher_github_action) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0baaab92185..67662129d1d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -17,6 +17,7 @@ class UserTest < ActiveSupport::TestCase should_not allow_value("1abcde").for(:handle) should_not allow_value("abc^%def").for(:handle) should_not allow_value("abc\n