Search code examples
ruby-on-railsruby-on-rails-7

Rails: Prevent assignment of other models on a polymorphic select dropdown


I am implementing an app where the User model can be associated with zero or more BusinessUnit objects, and/or zero or more Location objects via an Assignment link object (as below).

class Assignment < ApplicationRecord
  belongs_to :user
  belongs_to :assignable, polymorphic: true

  validates :user_id, uniqueness: { scope: [:assignable_type, :assignable_id] }
end

I am implementing a multiple select dropdown on the User edit page to assign locations/business units. After some research, I'm using signed global IDs based on the very helpful guidance in these two guides: One, Two

In the latter guide, the author is very pointed in ensuring that only the intended classes can be assigned (Users and Teams in their example, BusinessUnits and Locations in mine). In the former example, the author asserts that this is unneccesary when using signed global ID's as:

the only records the user can set as a review’s subject are ones that we allow when we include them in the review form.

Question - which is correct? Intuitively, it seems accurate that a signed global ID (which expires, is not idempotent, and includes a timestamp) could not be faked to refer to a different arbitrary class and object, however I want to make sure that I'm not missing a vulnerability here as this is my first time using signed global IDs.

Thank you!


Solution

  • Your intuition is right, the SGIDs can't realistically be faked, AND having the recipient_ prefix in the associations also adds a significant layer of security. I'd have stopped at just having the locate_many_signed(..., only: BROADCAST_RECIPIENT_CLASSES) layer of added security in there (ie. I think the added steps of grouping, and then looping over B_R_C again is unnecessary)

    So, you may ask, given the SGIDs do we even need the only: thing in there? And to that I'd say, today - no. But apps grow and change over time, so it seems a good idea to have the added layer of protection in case something is added in future that you weren't anticipating.

    And FWIW, to that end, I'd probably have named BROADCAST_RECIPIENT_CLASSES something a bit more obvious/alarming like TAINTED_INPUT_WHITELIST.