From 8a47fe350525fba65bc390d02a6d84a7769e3341 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 28 Oct 2020 01:39:21 +0100
Subject: [PATCH] Avoid a brakeman security warning

Although it wasn't a real security concern because we were only calling
a `find` method based on the user input, it's a good practice to avoid
using constants based on user parameters.

Since we don't use the `find` method anymore but we still need to check
the associated record exists, we're changing the `followable` validation
in the `Follow` model to do exactly that.
---
 app/controllers/follows_controller.rb |  6 +++---
 app/models/follow.rb                  |  3 +--
 spec/models/follow_spec.rb            | 12 ++++++++++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/app/controllers/follows_controller.rb b/app/controllers/follows_controller.rb
index 325b38fb3..1d5d3d331 100644
--- a/app/controllers/follows_controller.rb
+++ b/app/controllers/follows_controller.rb
@@ -3,7 +3,7 @@ class FollowsController < ApplicationController
   load_and_authorize_resource
 
   def create
-    @follow = Follow.create!(user: current_user, followable: find_followable)
+    @follow = current_user.follows.create!(follow_params)
     flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.create.notice")
     render :refresh_follow_button
   end
@@ -17,8 +17,8 @@ class FollowsController < ApplicationController
 
   private
 
-    def find_followable
-      params[:followable_type].constantize.find(params[:followable_id])
+    def follow_params
+      params.permit(:followable_type, :followable_id)
     end
 
     def followable_translation_key(followable)
diff --git a/app/models/follow.rb b/app/models/follow.rb
index 8062ffecc..f67adcefd 100644
--- a/app/models/follow.rb
+++ b/app/models/follow.rb
@@ -3,6 +3,5 @@ class Follow < ApplicationRecord
   belongs_to :followable, polymorphic: true
 
   validates :user_id, presence: true
-  validates :followable_id, presence: true
-  validates :followable_type, presence: true
+  validates :followable, presence: true
 end
diff --git a/spec/models/follow_spec.rb b/spec/models/follow_spec.rb
index 60dbfe288..feaa2e8cd 100644
--- a/spec/models/follow_spec.rb
+++ b/spec/models/follow_spec.rb
@@ -21,4 +21,16 @@ describe Follow do
     follow.followable_type = nil
     expect(follow).not_to be_valid
   end
+
+  it "is not valid with an invalid followable_type" do
+    follow.followable_type = "NotARealModel"
+
+    expect { follow.valid? }.to raise_exception "uninitialized constant NotARealModel"
+  end
+
+  it "is not valid with the ID of a non-existent record" do
+    follow.followable_id = Proposal.last.id + 1
+
+    expect(follow).not_to be_valid
+  end
 end