Validate lengths of string attributes

mvz · mvz · commit 34f6e9c98e0e · 2022-08-16T22:03:43.000+02:00
When using the MySQL database, Rails automatically sets a limit of 255
characters for string columns. This changes ensures this limit is also
enforced in the validations. For several columns with a different limit,
that different limit is enforced instead.

For the PostgreSQL database, there is no limit in the database, but it
is useful to set a limit anyway to prevent absurdly large values from
being submitted.
diff --git a/Manifest.txt b/Manifest.txt
@@ -160,6 +160,7 @@ app/models/article.rb
 app/models/article/factory.rb
 app/models/blog.rb
 app/models/comment.rb
+app/models/concerns/string_length_limit.rb
 app/models/config_manager.rb
 app/models/content.rb
 app/models/content_base.rb
diff --git a/app/models/blog.rb b/app/models/blog.rb
@@ -9,6 +9,8 @@
 #
 class Blog < ApplicationRecord
   include ConfigManager
+  include StringLengthLimit
+
   include Rails.application.routes.url_helpers
 
   has_many :contents
@@ -139,6 +141,7 @@ class Blog < ApplicationRecord
 
   validate :permalink_has_identifier
   # validates :base_url, presence: true
+  validates_default_string_length :base_url
 
   # Find the Blog that matches a specific base URL. If no Blog object is found
   # that matches, then grab the first blog. If *that* fails, then create a new
diff --git a/app/models/concerns/string_length_limit.rb b/app/models/concerns/string_length_limit.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module StringLengthLimit
+  # Default string length limit for model attributes. When running on MySQL,
+  # this is equal to the default string length in the database as set by Rails.
+  STRING_LIMIT = 255
+
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def validates_default_string_length(*names)
+      names.each do |name|
+        validates name, length: { maximum: STRING_LIMIT }
+      end
+    end
+  end
+end
diff --git a/app/models/content.rb b/app/models/content.rb
@@ -5,6 +5,7 @@
 
 class Content < ApplicationRecord
   include ContentBase
+  include StringLengthLimit
 
   belongs_to :user, optional: true, touch: true
   belongs_to :blog
@@ -38,6 +39,9 @@ class Content < ApplicationRecord
 
   serialize :whiteboard
 
+  validates_default_string_length :title, :author, :permalink, :name,
+                                  :post_type, :text_filter_name
+
   def author=(user)
     if user.respond_to?(:login)
       self[:author] = user.login
diff --git a/app/models/feedback.rb b/app/models/feedback.rb
@@ -10,10 +10,16 @@ class Feedback < ApplicationRecord
 
   include PublifyGuid
   include ContentBase
+  include StringLengthLimit
 
   validate :feedback_allowed, on: :create
   validates :article, presence: true
 
+  validates_default_string_length :title, :author, :email, :url, :blog_name,
+                                  :user_agent, :text_filter_name
+
+  validates :ip, length: { maximum: 40 }
+
   before_save :correct_url, :classify_content
   before_create :create_guid
 
diff --git a/app/models/ping.rb b/app/models/ping.rb
@@ -1,5 +1,8 @@
 # frozen_string_literal: true
 
 class Ping < ApplicationRecord
+  include StringLengthLimit
+
   belongs_to :article
+  validates_default_string_length :url
 end
diff --git a/app/models/post_type.rb b/app/models/post_type.rb
@@ -1,9 +1,13 @@
 # frozen_string_literal: true
 
 class PostType < ApplicationRecord
+  include StringLengthLimit
+
   validates :name, uniqueness: true
   validates :name, presence: true
   validate :name_is_not_read
+  validates_default_string_length :name, :permalink, :description
+
   before_save :sanitize_title
 
   def name_is_not_read
diff --git a/app/models/redirect.rb b/app/models/redirect.rb
@@ -1,13 +1,17 @@
 # frozen_string_literal: true
 
 class Redirect < ApplicationRecord
+  include StringLengthLimit
+
   belongs_to :content, optional: true, touch: true
   belongs_to :blog
 
   validates :from_path, uniqueness: true
   validates :to_path, presence: true
   validates :blog, presence: true
 
+  validates_default_string_length :from_path, :to_path
+
   def full_to_path
     path = to_path
     # FIXME: Unify HTTP URI matchers
diff --git a/app/models/resource.rb b/app/models/resource.rb
@@ -4,9 +4,12 @@
 require "carrierwave/orm/activerecord"
 
 class Resource < ApplicationRecord
+  include StringLengthLimit
   belongs_to :blog
   belongs_to :content, optional: true
 
   mount_uploader :upload, ResourceUploader
   validates :upload, presence: true
+
+  validates_default_string_length :mime
 end
diff --git a/app/models/tag.rb b/app/models/tag.rb
@@ -1,12 +1,15 @@
 # frozen_string_literal: true
 
 class Tag < ApplicationRecord
+  include StringLengthLimit
+
   belongs_to :blog
   has_and_belongs_to_many :contents, order: "created_at DESC"
 
   validates :name, uniqueness: { scope: :blog_id }
   validates :blog, presence: true
   validates :name, presence: true
+  validates_default_string_length :display_name
 
   before_validation :ensure_naming_conventions
 
diff --git a/app/models/user.rb b/app/models/user.rb
@@ -14,12 +14,14 @@ class User < ApplicationRecord
   devise :database_authenticatable, :registerable,
          :recoverable, :rememberable, :trackable, :validatable
   include ConfigManager
+  include StringLengthLimit
 
   before_validation :set_default_profile
 
   validates :login, uniqueness: true
   validates :email, :login, presence: true
   validates :login, length: { in: 3..40 }
+  validates_default_string_length :email, :text_filter_name
 
   belongs_to :resource, optional: true
   has_many :notifications, foreign_key: "notify_user_id"
diff --git a/spec/models/blog_spec.rb b/spec/models/blog_spec.rb
@@ -113,6 +113,10 @@
   describe "validations" do
     let(:blog) { described_class.new }
 
+    it "requires base url to not be too long" do
+      expect(blog).to validate_length_of(:base_url).is_at_most(255)
+    end
+
     it "requires blog name to not be too long" do
       expect(blog).to validate_length_of(:blog_name).is_at_most(256)
     end
diff --git a/spec/models/content_spec.rb b/spec/models/content_spec.rb
@@ -144,4 +144,32 @@
       it { expect(content.author_name).to eq(author.login) }
     end
   end
+
+  describe "validations" do
+    let(:content) { described_class.new }
+
+    it "requires title to not be too long" do
+      expect(content).to validate_length_of(:title).is_at_most(255)
+    end
+
+    it "requires author to not be too long" do
+      expect(content).to validate_length_of(:author).is_at_most(255)
+    end
+
+    it "requires permalink to not be too long" do
+      expect(content).to validate_length_of(:permalink).is_at_most(255)
+    end
+
+    it "requires name to not be too long" do
+      expect(content).to validate_length_of(:name).is_at_most(255)
+    end
+
+    it "requires post_type to not be too long" do
+      expect(content).to validate_length_of(:post_type).is_at_most(255)
+    end
+
+    it "requires text_filter_name to not be too long" do
+      expect(content).to validate_length_of(:text_filter_name).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/feedback_spec.rb b/spec/models/feedback_spec.rb
@@ -178,4 +178,40 @@ def classify
       assert !@comment.published?
     end
   end
+
+  describe "validations" do
+    let(:feedback) { described_class.new }
+
+    it "requires title to not be too long" do
+      expect(feedback).to validate_length_of(:title).is_at_most(255)
+    end
+
+    it "requires author to not be too long" do
+      expect(feedback).to validate_length_of(:author).is_at_most(255)
+    end
+
+    it "requires email to not be too long" do
+      expect(feedback).to validate_length_of(:email).is_at_most(255)
+    end
+
+    it "requires url to not be too long" do
+      expect(feedback).to validate_length_of(:url).is_at_most(255)
+    end
+
+    it "requires ip to not be too long" do
+      expect(feedback).to validate_length_of(:ip).is_at_most(40)
+    end
+
+    it "requires blog_name to not be too long" do
+      expect(feedback).to validate_length_of(:blog_name).is_at_most(255)
+    end
+
+    it "requires user_agent to not be too long" do
+      expect(feedback).to validate_length_of(:user_agent).is_at_most(255)
+    end
+
+    it "requires text_filter_name to not be too long" do
+      expect(feedback).to validate_length_of(:text_filter_name).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/ping_spec.rb b/spec/models/ping_spec.rb
@@ -3,4 +3,11 @@
 require "rails_helper"
 
 describe Ping, type: :model do
+  describe "validations" do
+    let(:ping) { described_class.new }
+
+    it "requires url to not be too long" do
+      expect(ping).to validate_length_of(:url).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/post_type_spec.rb b/spec/models/post_type_spec.rb
@@ -29,4 +29,20 @@
     expect(test_type).not_to be_valid
     expect(test_type.errors[:name]).to eq(["has already been taken"])
   end
+
+  describe "validations" do
+    let(:post_type) { described_class.new }
+
+    it "requires name to not be too long" do
+      expect(post_type).to validate_length_of(:name).is_at_most(255)
+    end
+
+    it "requires permalink to not be too long" do
+      expect(post_type).to validate_length_of(:permalink).is_at_most(255)
+    end
+
+    it "requires description to not be too long" do
+      expect(post_type).to validate_length_of(:description).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/redirect_spec.rb b/spec/models/redirect_spec.rb
@@ -20,4 +20,16 @@
       expect(redirect.from_url).to eq "#{blog.base_url}/right/here"
     end
   end
+
+  describe "validations" do
+    let(:redirect) { described_class.new }
+
+    it "requires from_path to not be too long" do
+      expect(redirect).to validate_length_of(:from_path).is_at_most(255)
+    end
+
+    it "requires to_path to not be too long" do
+      expect(redirect).to validate_length_of(:to_path).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/resource_spec.rb b/spec/models/resource_spec.rb
@@ -33,4 +33,12 @@
       expect(img_resource.upload_url(:thumb)).to eq "/files/resource/1/thumb_testfile.png"
     end
   end
+
+  describe "validations" do
+    let(:resource) { described_class.new }
+
+    it "requires mime to not be too long" do
+      expect(resource).to validate_length_of(:mime).is_at_most(255)
+    end
+  end
 end
diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb
@@ -12,9 +12,15 @@
     expect(test_tag.errors[:name]).to eq(["has already been taken"])
   end
 
-  describe "validation" do
+  describe "validations" do
+    let(:tag) { described_class.new }
+
     it "requires name to be present" do
-      expect(blog.tags.build(name: "")).not_to be_valid
+      expect(tag).to validate_presence_of(:name)
+    end
+
+    it "requires display_name to not be too long" do
+      expect(tag).to validate_length_of(:display_name).is_at_most(255)
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
@@ -45,6 +45,10 @@
   describe "validations" do
     let(:user) { described_class.new }
 
+    it "requires email to not be too long" do
+      expect(user).to validate_length_of(:email).is_at_most(255)
+    end
+
     it "requires first name to not be too long" do
       expect(user).to validate_length_of(:firstname).is_at_most(256)
     end
@@ -73,6 +77,10 @@
       expect(user).to validate_presence_of(:login)
     end
 
+    it "requires text_filter_name to not be too long" do
+      expect(user).to validate_length_of(:text_filter_name).is_at_most(255)
+    end
+
     it "does not allow duplicate logins when updating a user" do
       create :user, login: "foo"
       bar = create :user, login: "bar"
