Merge pull request #1077 from publify/validate-string-lengths

Validate lengths of string attributes

publify_core/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

publify_core/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

publify_core/app/models/comment.rb

@@ -41,18 +41,15 @@ def send_notifications
   private
 
   def article_allows_feedback?
-    return true if article.allow_comments?
-
-    errors.add(:article, "Article is not open to comments")
-    false
+    article.allow_comments?
   end
 
   def blog_allows_feedback?
     true
   end
 
-  def check_article_closed_for_feedback
-    errors.add(:article, "Comment are closed") if article.comments_closed?
+  def article_closed_for_feedback?
+    article.comments_closed?
   end
 
   def originator

publify_core/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

publify_core/app/models/config_manager.rb

@@ -1,8 +1,7 @@
 # frozen_string_literal: true
 
 module ConfigManager
-  def self.append_features(base)
-    super
+  def self.included(base)
     base.extend(ClassMethods)
   end
 

publify_core/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

publify_core/app/models/feedback.rb

@@ -10,11 +10,16 @@ class Feedback < ApplicationRecord
 
   include PublifyGuid
   include ContentBase
+  include StringLengthLimit
 
-  validate :article_allows_this_feedback, on: :create
-  validate :feedback_not_closed, on: :create
+  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
 
@@ -99,8 +104,20 @@ def correct_url
     self.url = "http://#{url}" unless %r{^https?://}.match?(url)
   end
 
-  def article_allows_this_feedback
-    article && blog_allows_feedback? && article_allows_feedback?
+  def feedback_allowed
+    return unless article
+
+    unless blog_allows_feedback?
+      errors.add(:base, "#{plural_model_name} are disabled")
+      return
+    end
+
+    unless article_allows_feedback?
+      errors.add(:article, "Article is not open for #{plural_model_name.downcase}")
+      return
+    end
+
+    errors.add(:article, "#{plural_model_name} are closed") if article_closed_for_feedback?
   end
 
   def akismet_options
@@ -200,10 +217,6 @@ def report_as_ham
     end
   end
 
-  def feedback_not_closed
-    check_article_closed_for_feedback
-  end
-
   def send_notifications
     nil
   end
@@ -242,4 +255,8 @@ def akismet_client
   def blog_id
     article.blog_id if article.present?
   end
+
+  def plural_model_name
+    self.class.model_name.human.pluralize
+  end
 end

publify_core/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

publify_core/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

publify_core/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

publify_core/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

publify_core/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
 

publify_core/app/models/trackback.rb

@@ -14,24 +14,6 @@ def process_trackback
     end
   end
 
-  def article_allows_feedback?
-    return true if article.allow_pings?
-
-    errors.add(:article, "Article is not pingable")
-    false
-  end
-
-  def blog_allows_feedback?
-    return true unless blog.global_pings_disable
-
-    errors.add(:base, "Pings are disabled")
-    false
-  end
-
-  def check_article_closed_for_feedback
-    errors.add(:article, "Pings are closed") if article.pings_closed?
-  end
-
   def originator
     blog_name
   end
@@ -47,4 +29,18 @@ def body=(newval)
   def feed_title
     "Trackback from #{blog_name}: #{title} on #{article.title}"
   end
+
+  private
+
+  def article_allows_feedback?
+    article.allow_pings?
+  end
+
+  def blog_allows_feedback?
+    !blog.global_pings_disable
+  end
+
+  def article_closed_for_feedback?
+    article.pings_closed?
+  end
 end

publify_core/app/models/user.rb

@@ -14,12 +14,15 @@ 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
+  validates :name, length: { maximum: 2048 }
 
   belongs_to :resource, optional: true
   has_many :notifications, foreign_key: "notify_user_id"

publify_core/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

publify_core/spec/models/comment_spec.rb

@@ -8,9 +8,29 @@
 
   let(:published_article) { build_stubbed(:article, published_at: 1.hour.ago, blog: blog) }
 
-  def valid_comment(options = {})
-    Comment.new({ author: "Bob", article: published_article, body: "nice post",
-                  ip: "1.2.3.4" }.merge(options))
+  describe "validations" do
+    let(:comment) { described_class.new }
+
+    it "allows an article with open comment window" do
+      article = Article.new(blog: blog, allow_comments: true, state: "published",
+                            published_at: 1.day.ago)
+
+      expect(comment).to allow_value(article).for(:article)
+    end
+
+    it "requires article comment window to be open" do
+      article = Article.new(blog: blog, allow_comments: true)
+
+      expect(comment).not_to allow_value(article).for(:article).
+        with_message("Comments are closed")
+    end
+
+    it "requires article to be open to comments" do
+      article = Article.new(blog: blog, allow_comments: false)
+
+      expect(comment).not_to allow_value(article).for(:article).
+        with_message("Article is not open for comments")
+    end
   end
 
   describe "#permalink_url" do
@@ -88,6 +108,11 @@ def valid_comment(options = {})
   end
 
   describe "#classify_content" do
+    def valid_comment(options = {})
+      Comment.new({ author: "Bob", article: published_article, body: "nice post",
+                    ip: "1.2.3.4" }.merge(options))
+    end
+
     it "rejects spam rbl" do
       comment = valid_comment(
         author: "Spammer",

publify_core/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