Fix #3813: Favorite limit can be bypassed.

app/controllers/favorites_controller.rb

@@ -1,6 +1,6 @@
 class FavoritesController < ApplicationController
   before_action :member_only, except: [:index]
-  respond_to :html, :xml, :json
+  respond_to :html, :xml, :json, :js
   skip_before_action :api_check
 
   def index
@@ -24,23 +24,10 @@ class FavoritesController < ApplicationController
   end
 
   def create
-    if CurrentUser.favorite_limit.nil? || CurrentUser.favorite_count < CurrentUser.favorite_limit
+    @post = Post.find(params[:post_id])
-      @post = Post.find(params[:post_id])
+    @post.add_favorite!(CurrentUser.user)
-      @post.add_favorite!(CurrentUser.user)
-    else
-      @error_msg = "You can only keep up to #{CurrentUser.favorite_limit} favorites. Upgrade your account to save more."
-    end
 
-    respond_with(@post) do |format|
+    respond_with(@post)
-      format.js
-      format.json do
-        if @post
-          render :json => {:success => true}.to_json
-        else
-          render :json => {:success => false, :reason => @error_msg}.to_json, :status => 422
-        end
-      end
-    end
   end
 
   def destroy
@@ -52,11 +39,6 @@ class FavoritesController < ApplicationController
       Favorite.remove(post_id: params[:id], user: CurrentUser.user)
     end
 
-    respond_with(@post) do |format|
+    respond_with(@post)
-      format.js
-      format.json do
-        render :json => {:success => true}.to_json
-      end
-    end
   end
 end

app/models/favorite.rb

@@ -1,4 +1,6 @@
 class Favorite < ApplicationRecord
+  class Error < Exception ; end
+
   belongs_to :post
   belongs_to :user
   scope :for_user, ->(user_id) {where("user_id % 100 = #{user_id.to_i % 100} and user_id = #{user_id.to_i}")}
@@ -7,7 +9,12 @@ class Favorite < ApplicationRecord
     Favorite.transaction do
       User.where(:id => user.id).select("id").lock("FOR UPDATE NOWAIT").first
 
-      return if Favorite.for_user(user.id).where(:user_id => user.id, :post_id => post.id).exists?
+      if user.favorite_count >= user.favorite_limit
+        raise Error, "You can only keep up to #{user.favorite_limit} favorites. Upgrade your account to save more."
+      elsif Favorite.for_user(user.id).where(:user_id => user.id, :post_id => post.id).exists?
+        raise Error, "You have already favorited this post"
+      end
+
       Favorite.create!(:user_id => user.id, :post_id => post.id)
       Post.where(:id => post.id).update_all("fav_count = fav_count + 1")
       post.append_user_to_fav_string(user.id)

app/models/user.rb

@@ -605,7 +605,7 @@ class User < ApplicationRecord
 
     def favorite_limit
       if is_platinum?
-        nil
+        Float::INFINITY
       elsif is_gold?
         20_000
       else

app/views/favorites/create.js.erb

@@ -1,17 +1,13 @@
-<% if @error_msg %>
+$("#add-to-favorites").hide();
-  $(window).trigger("danbooru:error", "<%= @error_msg %>");
+$("#add-fav-button").hide();
-<% else %>
+$("#remove-from-favorites").show();
-  $("#add-to-favorites").hide();
+$("#remove-fav-button").show();
-  $("#add-fav-button").hide();
+$("#score-for-post-<%= @post.id %>").html(<%= @post.score %>);
-  $("#remove-from-favorites").show();
+$("#favcount-for-post-<%= @post.id %>").html(<%= @post.fav_count %>);
-  $("#remove-fav-button").show();
+<% if CurrentUser.is_gold? %>
-  $("#score-for-post-<%= @post.id %>").html(<%= @post.score %>);
+  $("#favlist").html("<%= escape_javascript(post_favlist(@post)) %>");
-  $("#favcount-for-post-<%= @post.id %>").html(<%= @post.fav_count %>);
+  if (!$("#favlist").is(":visible")) {
-  <% if CurrentUser.is_gold? %>
+    $("#show-favlist-link").show();
-    $("#favlist").html("<%= escape_javascript(post_favlist(@post)) %>");
+  }
-    if (!$("#favlist").is(":visible")) {
-      $("#show-favlist-link").show();
-    }
-  <% end %>
-  $(window).trigger("danbooru:notice", "You have favorited this post");
 <% end %>
+$(window).trigger("danbooru:notice", "You have favorited this post");

app/views/static/error.js.erb

@@ -0,0 +1,2 @@
+var message = <%= raw @error_message.try(:to_json) || @exception.message.to_json %>;
+Danbooru.Utility.error(message);

test/unit/favorite_test.rb

@@ -2,10 +2,13 @@ require 'test_helper'
 
 class FavoriteTest < ActiveSupport::TestCase
   setup do
-    user = FactoryBot.create(:user)
+    @user1 = FactoryBot.create(:user)
-    CurrentUser.user = user
+    @user2 = FactoryBot.create(:user)
+    @p1 = FactoryBot.create(:post)
+    @p2 = FactoryBot.create(:post)
+
+    CurrentUser.user = @user1
     CurrentUser.ip_addr = "127.0.0.1"
-    Favorite # need to force loading the favorite model
   end
 
   teardown do
@@ -15,44 +18,41 @@ class FavoriteTest < ActiveSupport::TestCase
 
   context "A favorite" do
     should "delete from all tables" do
-      user1 = FactoryBot.create(:user)
+      @p1.add_favorite!(@user1)
-      p1 = FactoryBot.create(:post)
+      assert_equal(1, @user1.favorite_count)
 
-      p1.add_favorite!(user1)
+      Favorite.where(:user_id => @user1.id, :post_id => @p1.id).delete_all
-      assert_equal(1, Favorite.count)
-
-      Favorite.where(:user_id => user1.id, :post_id => p1.id).delete_all
       assert_equal(0, Favorite.count)
     end
 
     should "know which table it belongs to" do
-      user1 = FactoryBot.create(:user)
+      @p1.add_favorite!(@user1)
-      user2 = FactoryBot.create(:user)
+      @p2.add_favorite!(@user1)
-      p1 = FactoryBot.create(:post)
+      @p1.add_favorite!(@user2)
-      p2 = FactoryBot.create(:post)
 
-      p1.add_favorite!(user1)
+      favorites = @user1.favorites.order("id desc")
-      p2.add_favorite!(user1)
-      p1.add_favorite!(user2)
-
-      favorites = user1.favorites.order("id desc")
       assert_equal(2, favorites.count)
-      assert_equal(p2.id, favorites[0].post_id)
+      assert_equal(@p2.id, favorites[0].post_id)
-      assert_equal(p1.id, favorites[1].post_id)
+      assert_equal(@p1.id, favorites[1].post_id)
 
-      favorites = user2.favorites.order("id desc")
+      favorites = @user2.favorites.order("id desc")
       assert_equal(1, favorites.count)
-      assert_equal(p1.id, favorites[0].post_id)
+      assert_equal(@p1.id, favorites[0].post_id)
     end
 
     should "not allow duplicates" do
-      user1 = FactoryBot.create(:user)
+      @p1.add_favorite!(@user1)
-      p1 = FactoryBot.create(:post)
+      error = assert_raises(Favorite::Error) { @p1.add_favorite!(@user1) }
-      p2 = FactoryBot.create(:post)
-      p1.add_favorite!(user1)
-      p1.add_favorite!(user1)
 
-      assert_equal(1, user1.favorites.count)
+      assert_equal("You have already favorited this post", error.message)
+      assert_equal(1, @user1.favorite_count)
+    end
+
+    should "not allow exceeding the user's favorite limit" do
+      @user1.stubs(:favorite_limit).returns(0)
+      error = assert_raises(Favorite::Error) { @p1.add_favorite!(@user1) }
+
+      assert_equal("You can only keep up to 0 favorites. Upgrade your account to save more.", error.message)
     end
   end
 end