Safe Prefix Decode for find_by_prefix_id

[bryanrite]

When using the finder methods, either the overridden find or find_by_prefix_id and find_by_prefix_id!, the value we are trying to find is eventually delegated to HashIds to decode (https://github.com/excid3/prefixed_ids/blob/master/lib/prefixed_ids/prefix_id.rb#L21). If this value contains any characters outside of the specific HashId alphabet, HashId will raise a Hashids::InputError with the message unable to unhash.

In a lot of web-apps, the id parameter to this find action would come from user-input, likely from the URL. If the user makes any typo, such as adding a dash instead of an underscore, or happens to use UUID-based IDs (which contain a dash), then the finder method will raise and cause a 500... even for the non-bang find methods.

class MyModel < ActiveRecord::Base
  has_prefix_id :foo, minimum_length: 4
end
MyModel._prefix_id.encode("1")             # =>  "foo_5eGU1"
MyModel._prefix_id.decode("foo_5eGU1")     # => 1
MyModel._prefix_id.decode("foo5eGU1")      # => nil
MyModel._prefix_id.decode("foo-5eGU1")     # => unable to unhash (Hashids::InputError)
MyModel._prefix_id.decode("foo_5eGU1!")    # => unable to unhash (Hashids::InputError)

Could be solved in app with a rescue_from in the Controller, or validating the format of id but for a finder method specifically, it seems more appropriate to return nil or raise ActiveRecord::RecordNotFound rather than raise Hashids::InputError.

I've monkey patched my app using the following:

module PrefixedIds
  module Attribute
    class_methods do
      def safe_decode_prefix_id(id)
        _prefix_id.decode(id)
      rescue Hashids::InputError
        nil
      end

      def find_by_prefix_id(id)
        find_by(id: safe_decode_prefix_id(id))
      end

      def find_by_prefix_id!(id)
        find_by!(id: safe_decode_prefix_id(id))
      end
    end
  end
end

If this is something you'd be interested in accepting, I can throw together a PR.

Cheers!

[excid3]

Hmm, good question.

Letting the user know these are invalid is useful and needs to raise an error to let the user know. That can help debugging when things go wrong like something manipulated the ID on accident. Returning nil in that case would mask the issue.

[bryanrite]

Yes, that was my reasoning for leaving it untouched in the other methods. My thought was that find_by_prefix_id is going to be used overwhelmingly with params[:id] for the majority of controller actions... having to check/rescue it every time kinda defeats the purpose, and doing so with a rescue_from in the controller can break situations where you expect to recover from ActiveRecord::RecordNotFound.

IMHO its too brittle if a typo/any user input can cause a 500 and too common a pattern to expect someone to guard each use.

[kaka-ruto]

I just got this on my app as well. The user updated their username to foo-bar, with a dash.

Does it make sense to validate against dashes for something like this? Why are dashes not allowed with hash ids?