varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

Rails 4.2.5.1 broke godmin #174

Closed anderscarling closed 8 years ago

anderscarling commented 8 years ago

Seems like rails 4.2.5.1 broken Godmin::Resolver. Would guess the fixes for CVE-2016-0752 (Possible Information Leak Vulnerability in Action View) is the cause.

ArgumentError (wrong number of arguments (given 5, expected 4)):
  godmin (1.1.0) lib/godmin/resolver.rb:16:in `find_templates'
  actionview (4.2.5.1) lib/action_view/template/resolver.rb:116:in `block in find_all'
  actionview (4.2.5.1) lib/action_view/template/resolver.rb:152:in `block in cached'
  actionview (4.2.5.1) lib/action_view/template/resolver.rb:63:in `cache'
  actionview (4.2.5.1) lib/action_view/template/resolver.rb:151:in `cached'
  actionview (4.2.5.1) lib/action_view/template/resolver.rb:115:in `find_all'
  actionview (4.2.5.1) lib/action_view/path_set.rb:70:in `block (2 levels) in _find_all'
anderscarling commented 8 years ago
    def find_templates(name, prefix, partial, details, outside_app_allowed = false)

The added param is outside_app_allowed, and unless its true it basically tacks on a filter inside ActionView::PathResolver that makes sure that the resolved path exists inside the @path variable that's assigned through PathResolvers initializer. The common case is of course that @path is File.expand_path(Rails.root.join("app/views")).

anderscarling commented 8 years ago

I'm quite sure it should be OK to just pass along the param through Godmin::GodminResolver and Godmin::EngineResolver, but that would of course break backwards compatibility when trying to call super with 1 arg to much.

Do you have an nice strategy for handling compatibility in this kinda of issues?

The ugly quick fix kinda sucks..

def self.has_outside_app_allowed_param?
  @has_outside_app_allowed_param = ActionView::PathResolver.instance_method(:find_templates).arity.abs == 5 unless defined?(@has_outside_app_allowed_param)
  @has_outside_app_allowed_param
end

def find_templates(.., outside_app_allowed = false)
  # ...

  templates = if self.class.has_outside_app_allowed_param?
    super(name, path, partial, details, outside_app_allowed)
  else
    super(name, path, partial, details)
  end

  # ...
end
anderscarling commented 8 years ago

Rolled up ugly fix in f3a258ad45 if anyone else feels urgent need to have both security patches and godmin, while a nicer fix is being worked out.

jensljungblad commented 8 years ago

@anderscarling How about

def find_templates(name, prefix, *args)
  ...
  templates = super(name, path, *args)
end
anderscarling commented 8 years ago

Yeah, that seems like the way to go about it. :)

jensljungblad commented 8 years ago

Released a new version with a fix for this one