Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
allow :file to be outside rails root, but anything else must be insid…
…e the rails view directory

Conflicts:
	actionpack/test/controller/render_test.rb
	actionview/lib/action_view/template/resolver.rb

CVE-2016-0752
  • Loading branch information
tenderlove committed Jan 22, 2016
1 parent 2cb4663 commit 0c5c32a
Show file tree
Hide file tree
Showing 9 changed files with 93 additions and 16 deletions.
8 changes: 7 additions & 1 deletion actionpack/lib/abstract_controller/rendering.rb
Expand Up @@ -77,7 +77,13 @@ def view_assigns
# render "foo/bar" to render :file => "foo/bar".
# :api: plugin
def _normalize_args(action=nil, options={})
if action.is_a? Hash
case action
when ActionController::Parameters
unless action.permitted?
raise ArgumentError, "render parameters are not permitted"
end
action
when Hash
action
else
options
Expand Down
31 changes: 31 additions & 0 deletions actionpack/test/controller/render_test.rb
Expand Up @@ -58,6 +58,16 @@ def conditional_hello_with_record
end
end

def dynamic_render
render params[:id] # => String, AC:Params
end

def dynamic_render_with_file
# This is extremely bad, but should be possible to do.
file = params[:id] # => String, AC:Params
render file: file
end

def conditional_hello_with_public_header
if stale?(:last_modified => Time.now.utc.beginning_of_day, :etag => [:foo, 123], :public => true)
render :action => 'hello_world'
Expand Down Expand Up @@ -273,6 +283,27 @@ def accessing_logger_in_template
class ExpiresInRenderTest < ActionController::TestCase
tests TestController

def test_dynamic_render_with_file
# This is extremely bad, but should be possible to do.
assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb'))
response = get :dynamic_render_with_file, { id: '../\\../test/abstract_unit.rb' }
assert_equal File.read(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')),
response.body
end

def test_dynamic_render
assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb'))
assert_raises ActionView::MissingTemplate do
get :dynamic_render, { id: '../\\../test/abstract_unit.rb' }
end
end

def test_dynamic_render_file_hash
assert_raises ArgumentError do
get :dynamic_render, { id: { file: '../\\../test/abstract_unit.rb' } }
end
end

def test_expires_in_header
get :conditional_hello_with_expires_in
assert_equal "max-age=60, private", @response.headers["Cache-Control"]
Expand Down
4 changes: 4 additions & 0 deletions actionview/lib/action_view/lookup_context.rb
Expand Up @@ -122,6 +122,10 @@ def find(name, prefixes = [], partial = false, keys = [], options = {})
end
alias :find_template :find

def find_file(name, prefixes = [], partial = false, keys = [], options = {})
@view_paths.find_file(*args_for_lookup(name, prefixes, partial, keys, options))
end

def find_all(name, prefixes = [], partial = false, keys = [], options = {})
@view_paths.find_all(*args_for_lookup(name, prefixes, partial, keys, options))
end
Expand Down
26 changes: 19 additions & 7 deletions actionview/lib/action_view/path_set.rb
Expand Up @@ -46,23 +46,35 @@ def find(*args)
find_all(*args).first || raise(MissingTemplate.new(self, *args))
end

def find_file(path, prefixes = [], *args)
_find_all(path, prefixes, args, true).first || raise(MissingTemplate.new(self, path, prefixes, *args))
end

def find_all(path, prefixes = [], *args)
_find_all path, prefixes, args, false
end

def exists?(path, prefixes, *args)
find_all(path, prefixes, *args).any?
end

private

def _find_all(path, prefixes, args, outside_app)
prefixes = [prefixes] if String === prefixes
prefixes.each do |prefix|
paths.each do |resolver|
templates = resolver.find_all(path, prefix, *args)
if outside_app
templates = resolver.find_all_anywhere(path, prefix, *args)
else
templates = resolver.find_all(path, prefix, *args)
end
return templates unless templates.empty?
end
end
[]
end

def exists?(path, prefixes, *args)
find_all(path, prefixes, *args).any?
end

private

def typecast(paths)
paths.map do |path|
case path
Expand Down
2 changes: 1 addition & 1 deletion actionview/lib/action_view/renderer/abstract_renderer.rb
Expand Up @@ -15,7 +15,7 @@ module ActionView
# that new object is called in turn. This abstracts the setup and rendering
# into a separate classes for partials and templates.
class AbstractRenderer #:nodoc:
delegate :find_template, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context
delegate :find_template, :find_file, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context

def initialize(lookup_context)
@lookup_context = lookup_context
Expand Down
2 changes: 1 addition & 1 deletion actionview/lib/action_view/renderer/template_renderer.rb
Expand Up @@ -29,7 +29,7 @@ def determine_template(options)
elsif options.key?(:html)
Template::HTML.new(options[:html], formats.first)
elsif options.key?(:file)
with_fallbacks { find_template(options[:file], nil, false, keys, @details) }
with_fallbacks { find_file(options[:file], nil, false, keys, @details) }
elsif options.key?(:inline)
handler = Template.handler_for_extension(options[:type] || "erb")
Template.new(options[:inline], "inline template", handler, :locals => keys)
Expand Down
25 changes: 21 additions & 4 deletions actionview/lib/action_view/template/resolver.rb
Expand Up @@ -113,7 +113,13 @@ def clear_cache
# Normalizes the arguments and passes it on to find_templates.
def find_all(name, prefix=nil, partial=false, details={}, key=nil, locals=[])
cached(key, [name, prefix, partial], details, locals) do
find_templates(name, prefix, partial, details)
find_templates(name, prefix, partial, details, false)
end
end

def find_all_anywhere(name, prefix, partial=false, details={}, key=nil, locals=[])
cached(key, [name, prefix, partial], details, locals) do
find_templates(name, prefix, partial, details, true)
end
end

Expand Down Expand Up @@ -174,15 +180,16 @@ def initialize(pattern=nil)

private

def find_templates(name, prefix, partial, details)
def find_templates(name, prefix, partial, details, outside_app_allowed = false)
path = Path.build(name, prefix, partial)
query(path, details, details[:formats])
query(path, details, details[:formats], outside_app_allowed)
end

def query(path, details, formats)
def query(path, details, formats, outside_app_allowed)
query = build_query(path, details)

template_paths = find_template_paths query
template_paths = reject_files_external_to_app(template_paths) unless outside_app_allowed

template_paths.map { |template|
handler, format, variant = extract_handler_and_format_and_variant(template, formats)
Expand All @@ -197,6 +204,10 @@ def query(path, details, formats)
}
end

def reject_files_external_to_app(files)
files.reject { |filename| !inside_path?(@path, filename) }
end

if RUBY_VERSION >= '2.2.0'
def find_template_paths(query)
Dir[query].reject { |filename|
Expand All @@ -217,6 +228,12 @@ def find_template_paths(query)
end
end

def inside_path?(path, filename)
filename = File.expand_path(filename)
path = File.join(path, '')
filename.start_with?(path)
end

# Helper for building query glob string based on resolver's pattern.
def build_query(path, details)
query = @pattern.dup
Expand Down
4 changes: 2 additions & 2 deletions actionview/lib/action_view/testing/resolvers.rb
Expand Up @@ -19,7 +19,7 @@ def to_s

private

def query(path, exts, formats)
def query(path, exts, formats, _)
query = ""
EXTENSIONS.each_key do |ext|
query << '(' << exts[ext].map {|e| e && Regexp.escape(".#{e}") }.join('|') << '|)'
Expand All @@ -44,7 +44,7 @@ def query(path, exts, formats)
end

class NullResolver < PathResolver
def query(path, exts, formats)
def query(path, exts, formats, _)
handler, format, variant = extract_handler_and_format_and_variant(path, formats)
[ActionView::Template.new("Template generated by Null Resolver", path, handler, :virtual_path => path, :format => format, :variant => variant)]
end
Expand Down
7 changes: 7 additions & 0 deletions actionview/test/template/render_test.rb
Expand Up @@ -141,6 +141,13 @@ def test_render_partial_from_default
assert_equal "only partial", @view.render("test/partial_only")
end

def test_render_outside_path
assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb'))
assert_raises ActionView::MissingTemplate do
@view.render(:template => "../\\../test/abstract_unit.rb")
end
end

def test_render_partial
assert_equal "only partial", @view.render(:partial => "test/partial_only")
end
Expand Down

0 comments on commit 0c5c32a

Please sign in to comment.