AMS should error when no serializer can be found

[cantino]

I think the default behavior of AMS should be to error if a serializer cannot be found. Otherwise, it's easy to stumble into a major security hole.

Imagine you have a controller, maybe something like:

class RecentUsersController < ApplicationController
  def index
    render json: User.recent.limit(5)
  end
end

and a serializer:

class UserSerializer < ActiveModel::Serializer
  attribute :first_name
  attribute :user_since
end

Now, imagine you either put that serializer at the wrong path by accident, or you named it RecentUserSerializer, but forgot to specify it by name in the render call. Either way, you're now calling as_json on the models and handing hashed passwords out over your API.

I'm using something like this to fix the issue, but I think it should be default behavior:

module ErrorOnMissingSerializer
  module AsJSONOverride
    def as_json(*_)
      serializable_resource = ActiveModelSerializers::SerializableResource.new(self)
      if serializable_resource.serializer
        serializable_resource.as_json
      else
        raise NotImplementedError, "No serializer was found for #{self.class.name}"
      end
    end
  end

  class Railtie < Rails::Railtie
    ActiveSupport.on_load :active_record do
      ActiveRecord::Base.include ErrorOnMissingSerializer::AsJSONOverride
    end
  end
end

[bf4]

@cantino I think there is room for a 'strict mode', that raises an MissingSerializer = Class.new(ArgumentError) instead of returning nil when no serializer is found. The complication is that we'd need to ensure that the object respond to read_attribute_for_serialization or something so that it's not a string, hash, array, etc. I'd rather not modify ActiveRecord or rely on that.

Relevant code https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer.rb#L81-L87

    # @api private
    # Find a serializer from a class and caches the lookup.
    # Preferentially returns:
    #   1. class name appended with "Serializer"
    #   2. try again with superclass, if present
-    #   3. nil
+    #   3. config.models_require_serializer? ? fail(MissingSerializer) : nil
    def self.get_serializer_for(klass)
      return nil unless config.serializer_lookup_enabled
      serializers_cache.fetch_or_store(klass) do
        # NOTE(beauby): When we drop 1.9.3 support we can lazify the map for perfs.
        serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }

        if serializer_class
          serializer_class
        elsif klass.superclass
          get_serializer_for(klass.superclass)
+        elsif klass.respond_to?(:read_attribute_for_serialization) && config.models_require_serializer?
+           fail MissingSerializer
+        else
+          nil
        end
      end
    end

[bf4]

FWIW, if you use a a JSON-Schema to validate your requests/responses in tests, you can be assured you're not rendering a model without a serializer...

[GregPK]

I've taken a stab a this by a monkey patch and would like to backport it here.

However, the above solution had to be altered for me. The spec is:

RSpec.describe 'looking for a serializer' do
  class ModelWOSer < ActiveRecord::Base ; end

  it 'explicitly fails when no serializer found' do
    expect{ActiveModel::Serializer.get_serializer_for(ModelWOSer)}.to raise_error ArgumentError
  end
end

and the monkey patch is:

ActiveModelSerializers.config.on_serializer_not_found = -> (klass) { fail ArgumentError, "Serializer not defined for #{klass}" }

ActiveModel::Serializer.class_eval do
  def self.get_serializer_for(klass)
    return nil unless config.serializer_lookup_enabled
    return nil if klass == NilClass

    serializers_cache.fetch_or_store(klass) do
      # NOTE(beauby): When we drop 1.9.3 support we can lazify the map for perfs.
      serializer_class = serializer_lookup_chain_for(klass).map(&:safe_constantize).find { |x| x && x < ActiveModel::Serializer }

      if serializer_class
        serializer_class
      elsif klass.superclass && ![BasicObject, Object].include?(klass.superclass)
        get_serializer_for(klass.superclass)
      elsif config.on_serializer_not_found.respond_to?(:call) && klass.respond_to?(:serialized_attributes)
        config.on_serializer_not_found.call(klass)
      else
        nil
      end
    end
  end
end

A few notes:

• defined a on_serializer_not_found policy instead of a boolean flag (more flexible) - this could be a no-op by default.
• :read_attribute_for_serialization from OP did not work for me - ActiveRecord::Base does not respond to that in this case. Can I expect arcane problems with :serialized_attributes?

[NullVoxPopuli]

@GregPK if you want to turn that in to a PR, we can get some more formal discussions on it? :-)

[GregPK]

@NullVoxPopuli True - let's discuss with a concrete example ☝️

[bf4]

somewhat related:

• Explicit serializer_lookup_enabled in controller doesn't retain Adapter for entire render  #1500
• Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813
• Replace fail/rescue CollectionSerializer::NoSerializerError with throw/catch :no_serializer #1767
• [RFC] Automatic serializer lookup #1442
• configurable serializer_lookup option #1757
• (WIP) Fix Serializer Lookup Chain for Namespaces #1883
• Overriding serializer lookup when rendering #1743
• Serializer for association does not look for siblings #1337
• Controller namespace lookup. #1436
• [WIP] Allow users to opt-out from the ActionController extensions #1636

[rromanchuk]

I just got burned by this, still trying to figure out why serializer_for is being called four times, twice with an empty model, but this caused my User object to call as_json leaking internal data, without any indication.

https://gist.github.com/rromanchuk/36701818f977c6b1b0325db519048754

[mhluska]

The as_json hack above doesn't seem to work in the latest version (0.10.7). How can I enforce specifying a serializer for a model today?

[bf4]

@mhluska

How can I enforce specifying a serializer for a model today?

That's actually a different feature from OP's "AMS should error when no serializer can be found"

I don't remember when self.get_serializer_for(klass) is called, but you could play with prepending it with a module

def self.get_serializer_for(klass)
+   fail ArgumentError, "Serializer lookup is disabled" unless config.serializer_lookup_enabled
  super

feel free to open a new issue for that.