On destroy, Integer returned instead of object

andreierdoss commented 2 years ago

I migrated from Ruby 2.6.6 to 3.1.2 and merit gem 3.0.3 to 4.03 and now, on destroy action, inside the block, the parameter passed in (vote) is an integer (always the same number - 181), not a Vote object. The other actions work as expected.

score 2, on: 'votes#destroy', to: :voteable_user do |vote|
         vote.direction == 'down' && vote.voteable_type == 'Question'
end

tute commented 2 years ago

Can you paste the contents of that controller action?

On Wednesday, November 16, 2022, Andrei Erdoss @.***> wrote:

I migrated from Ruby 2.6.6 to 3.1.2 and merit gem 3.0.3 to 4.03 and now, on destroy action, inside the block, the parameter passed in (vote) is an integer (always the same number - 181), not a Vote object. The other actions work as expected.

score 2, on: 'votes#destroy', to: :voteable_user do |vote| vote.direction == 'down' && vote.voteable_type == 'Question' end

— Reply to this email directly, view it on GitHub https://github.com/merit-gem/merit/issues/365, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAANH5FMSDOR6D65J2YBKOTWITVSTANCNFSM6AAAAAASCJMGQA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

andreierdoss commented 2 years ago

  def destroy
    authorize! vote_action(vote_params[:direction]), @voteable
    @vote = Vote.find_by(voteable_id: @voteable.id,
                      voteable_type: @voteable.class.name,
                      user_id: current_user.id,
                      direction: Vote.directions[vote_params[:direction]])
    @vote.destroy
    @voteable.reload
  end

tute commented 2 years ago

Thanks! And can you post the logs for the action? Particularly I want to see what target_data holds. TY.

andreierdoss commented 2 years ago

Here are the logs on DELETE of vote

Started DELETE "/votes/up/question/158" for ::1 at 2022-11-18 14:13:50 +0200
Processing by VotesController#destroy as JS
  Parameters: {"direction"=>"up", "voteable_type"=>"question", "voteable_id"=>"158"}
  User Load (0.2ms)  SELECT "users".* FROM "users" WHERE "users"."id" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["id", 1], ["LIMIT", 1]]
  Question Load (5.4ms)  SELECT "questions".* FROM "questions" WHERE "questions"."id" = $1 LIMIT $2  [["id", 158], ["LIMIT", 1]]
  ↳ app/controllers/votes_controller.rb:26:in `find_voteable'
  Shooter Load (0.3ms)  SELECT "shooters".* FROM "shooters" WHERE "shooters"."user_id" = $1 LIMIT $2  [["user_id", 1], ["LIMIT", 1]]
  ↳ app/models/ability.rb:7:in `initialize'
  Coach Load (0.3ms)  SELECT "coaches".* FROM "coaches" WHERE "coaches"."user_id" = $1 LIMIT $2  [["user_id", 1], ["LIMIT", 1]]
  ↳ app/models/ability.rb:8:in `initialize'
  Merit::Sash Load (0.2ms)  SELECT "sashes".* FROM "sashes" WHERE "sashes"."id" = $1 LIMIT $2  [["id", 1], ["LIMIT", 1]]
  ↳ app/models/ability.rb:137:in `initialize'
  Merit::Score Load (0.3ms)  SELECT "merit_scores".* FROM "merit_scores" WHERE "merit_scores"."sash_id" = $1  [["sash_id", 1]]
  ↳ app/models/ability.rb:137:in `initialize'
   (0.4ms)  SELECT SUM("merit_score_points"."num_points") AS sum_num_points, "merit_score_points"."score_id" AS merit_score_points_score_id FROM "merit_score_points" WHERE "merit_score_points"."score_id" = $1 GROUP BY "merit_score_points"."score_id"  [["score_id", 1]]
  ↳ app/models/ability.rb:137:in `initialize'
  CACHE  (0.0ms)  SELECT SUM("merit_score_points"."num_points") AS sum_num_points, "merit_score_points"."score_id" AS merit_score_points_score_id FROM "merit_score_points" WHERE "merit_score_points"."score_id" = $1 GROUP BY "merit_score_points"."score_id"  [["score_id", 1]]
  ↳ app/models/ability.rb:138:in `initialize'
  CACHE  (0.0ms)  SELECT SUM("merit_score_points"."num_points") AS sum_num_points, "merit_score_points"."score_id" AS merit_score_points_score_id FROM "merit_score_points" WHERE "merit_score_points"."score_id" = $1 GROUP BY "merit_score_points"."score_id"  [["score_id", 1]]
  ↳ app/models/ability.rb:141:in `initialize'
  CACHE  (0.0ms)  SELECT SUM("merit_score_points"."num_points") AS sum_num_points, "merit_score_points"."score_id" AS merit_score_points_score_id FROM "merit_score_points" WHERE "merit_score_points"."score_id" = $1 GROUP BY "merit_score_points"."score_id"  [["score_id", 1]]
  ↳ app/models/ability.rb:142:in `initialize'
  User Load (0.6ms)  SELECT "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2  [["id", 2026], ["LIMIT", 1]]
  ↳ app/controllers/votes_controller.rb:11:in `destroy'
  Vote Load (0.4ms)  SELECT "votes".* FROM "votes" WHERE "votes"."voteable_id" = $1 AND "votes"."voteable_type" = $2 AND "votes"."user_id" = $3 AND "votes"."direction" = $4 LIMIT $5  [["voteable_id", 158], ["voteable_type", "Question"], ["user_id", 1], ["direction", 1], ["LIMIT", 1]]
  ↳ app/controllers/votes_controller.rb:12:in `destroy'
  TRANSACTION (0.2ms)  BEGIN
  ↳ app/controllers/votes_controller.rb:16:in `destroy'
  Vote Destroy (0.4ms)  DELETE FROM "votes" WHERE "votes"."id" = $1  [["id", 224]]
  ↳ app/controllers/votes_controller.rb:16:in `destroy'
  Question Load (0.4ms)  SELECT "questions".* FROM "questions" WHERE "questions"."id" = $1 LIMIT $2  [["id", 158], ["LIMIT", 1]]
  ↳ app/models/vote.rb:32:in `update_voteable_votes_counters'
  User Load (0.3ms)  SELECT "users".* FROM "users" WHERE "users"."id" = $1 LIMIT $2  [["id", 2026], ["LIMIT", 1]]
  ↳ app/models/vote.rb:32:in `update_voteable_votes_counters'
  Question Update (0.7ms)  UPDATE "questions" SET "up_votes" = $1, "votes_sum" = $2, "updated_at" = $3 WHERE "questions"."id" = $4  [["up_votes", 0], ["votes_sum", 0], ["updated_at", "2022-11-18 12:13:50.609765"], ["id", 158]]
  ↳ app/models/vote.rb:32:in `update_voteable_votes_counters'
  TRANSACTION (39.6ms)  COMMIT
  ↳ app/controllers/votes_controller.rb:16:in `destroy'
  Question Load (0.3ms)  SELECT "questions".* FROM "questions" WHERE "questions"."id" = $1 LIMIT $2  [["id", 158], ["LIMIT", 1]]
  ↳ app/controllers/votes_controller.rb:17:in `destroy'
  Rendering votes/destroy.js.haml
   (0.6ms)  SELECT "users"."name" FROM "users" INNER JOIN "votes" ON "users"."id" = "votes"."user_id" WHERE "votes"."voteable_id" = $1 AND "votes"."voteable_type" = $2  [["voteable_id", 158], ["voteable_type", "Question"]]
  ↳ app/helpers/votes_helper.rb:46:in `votes_options'
  Rendered votes/destroy.js.haml (Duration: 13.5ms | Allocations: 7940)
  TRANSACTION (0.2ms)  BEGIN
  Merit::Action Create (2.2ms)  INSERT INTO "merit_actions" ("user_id", "action_method", "target_model", "target_id", "target_data", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id"  [["user_id", 1], ["action_method", "destroy"], ["target_model", "votes"], ["target_id", 224], ["target_data", "--- !ruby/object:Vote\nconcise_attributes:\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: id\n  value_before_type_cast: 224\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: voteable_id\n  value_before_type_cast: 158\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: voteable_type\n  value_before_type_cast: Question\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: direction\n  value_before_type_cast: 1\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: created_at\n  value_before_type_cast: 2022-11-18 12:13:37.250735000 Z\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: updated_at\n  value_before_type_cast: 2022-11-18 12:13:37.250735000 Z\n- !ruby/object:ActiveModel::Attribute::FromDatabase\n  name: user_id\n  value_before_type_cast: 1\nnew_record: false\nactive_record_yaml_version: 2\n"], ["created_at", "2022-11-18 12:13:50.683021"], ["updated_at", "2022-11-18 12:13:50.683021"]]
  TRANSACTION (0.6ms)  COMMIT
  Merit::Action Load (0.5ms)  SELECT "merit_actions".* FROM "merit_actions" WHERE "merit_actions"."processed" = $1 ORDER BY "merit_actions"."id" ASC LIMIT $2  [["processed", false], ["LIMIT", 1000]]
  TRANSACTION (0.2ms)  BEGIN
  Merit::Action Update (0.4ms)  UPDATE "merit_actions" SET "processed" = $1, "updated_at" = $2 WHERE "merit_actions"."id" = $3  [["processed", true], ["updated_at", "2022-11-18 12:13:50.711079"], ["id", 146]]
  TRANSACTION (0.6ms)  COMMIT
  Vote Load (0.5ms)  SELECT "votes".* FROM "votes" WHERE "votes"."id" = $1 LIMIT $2  [["id", 224], ["LIMIT", 1]]
[merit] no target found: Tried to load unspecified class: Vote. /Users/andrei/.frum/versions/3.1.2/lib/ruby/gems/3.1.0/gems/merit-4.0.3/lib/merit/base_target_finder.rb:12:in `find'
Completed 500  in 227ms (Views: 14.1ms | ActiveRecord: 55.5ms | Allocations: 122699)

NoMethodError - undefined method `direction' for 181:Integer
         vote.direction == 'down' && vote.voteable_type == 'Question'
             ^^^^^^^^^^:
  app/models/merit/point_rules.rb:24:in `block in initialize'

tute commented 2 years ago

There we go! I cut the relevant snippet:

[merit] no target found: Tried to load unspecified class: Vote. /Users/andrei/.frum/versions/3.1.2/lib/ruby/gems/3.1.0/gems/merit-4.0.3/lib/merit/base_target_finder.rb:12:infind'`

I've never seen this one before, found the following (links to the CVE and a potential workraound): https://stackoverflow.com/questions/72970170/upgrading-to-rails-6-1-6-1-causes-psychdisallowedclass-tried-to-load-unspecif

Can you retry adding config.active_record.yaml_column_permitted_classes = [Symbol, Vote] to your config/application.rb file?

tute commented 2 years ago

That would be one workaround, but the solution I think would be to refactor out the YAML.load call within merit.

andreierdoss commented 2 years ago

I made the change as requested and then ran a test:

 web git:(master) ✗ rspec spec/features/votes_spec.rb

An error occurred while loading ./spec/features/votes_spec.rb.
Failure/Error: require File.expand_path('../../config/environment', __FILE__)

NameError:
  uninitialized constant Web::Application::Vote
# ./config/application.rb:21:in `<class:Application>'
# ./config/application.rb:11:in `<module:Web>'
# ./config/application.rb:9:in `<top (required)>'
# ./config/environment.rb:2:in `require_relative'
# ./config/environment.rb:2:in `<top (required)>'
# ./spec/rails_helper.rb:12:in `<top (required)>'
# ./spec/features/votes_spec.rb:1:in `<top (required)>'
No examples found.

Finished in 0.00019 seconds (files took 5.04 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

tute commented 2 years ago

Two questions:

Did yaml_column_permitted_classes already appear in the file? If so, you might need to edit the previous one rather than add the line as I suggested
Is Vote an ActiveRecord model?

andreierdoss commented 2 years ago

yaml_column_permitted_classes does appear in the file, but I just edited it to include Vote. Like this:

config.active_record.yaml_column_permitted_classes = [Symbol, Hash, Array, ActiveSupport::HashWithIndifferentAccess,  Vote]

Yes, Vote is an ActiveRecord model

tute commented 2 years ago

Can't reference models yet in config/application.rb. Anyhow it's not the real solution; we'll need to change how merit works for destroy to avoid that CVE and restriction. This is a bug, thanks for reporting.

tute commented 4 months ago

Just skipped three related unit tests related to this bug: https://github.com/merit-gem/merit/commit/08e65bd9a44de753100df23d62e1d24ec93f2ef8#diff-a2eb130f0d1ee0b07aa5562e4ae246cdaaa12ebcb16355e301c05b30b24828c8

merit-gem / merit

On destroy, Integer returned instead of object #365