TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Always reject files external to app

Browse Source 
Previously, when using `render file:`, it was possible to render files
not only at an absolute path or relative to the current directory, but
relative to ANY view paths. This was probably done for absolutely
maximum compatibility when addressing CVE-2016-0752, but I think is
unlikely to be used in practice.

Tihs commit removes the ability to `render file:` with a path relative
to a non-fallback view path.

Make FallbackResolver.new private

To ensure nobody is making FallbackResolvers other than "/" and "".

Make reject_files_external_... no-op for fallbacks

Because there are only two values used for path: "" and "/", and
File.join("", "") == File.join("/", "") == "/", this method was only
testing that the absolute paths started at "/" (which of course all do).

This commit doesn't change any behaviour, but it makes it explicit that
the FallbackFileSystemResolver works this way.

Remove outside_app_allowed argument

Deprecate find_all_anywhere

This is now equivalent to find_all

Remove outside_app argument

Deprecate find_file for find

Both LookupContext#find_file and PathSet#find_file are now equivalent to
their respective #find methods.
This commit is contained in:
John Hawthorn 2019-04-01 16:35:07 -07:00
parent beb0bc9907
commit eb52904eb5
9 changed files with 42 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
actionpack/test/controller/new_base/render_file_test.rb
View File

@ -17,12 +17,12 @@ def with_instance_variables
def relative_path
@secret = "in the sauce"
render file: "../../fixtures/test/render_file_with_ivar"
render file: "../actionpack/test/fixtures/test/render_file_with_ivar"
end
def relative_path_with_dot
@secret = "in the sauce"
render file: "../../fixtures/test/dot.directory/render_file_with_ivar"
render file: "../actionpack/test/fixtures/test/dot.directory/render_file_with_ivar"
end
def pathname

17
actionpack/test/controller/render_test.rb
View File

@ -323,11 +323,12 @@ def setup
end
def test_dynamic_render_with_file
# This is extremely bad, but should be possible to do.
assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__))
response = assert_deprecated { get :dynamic_render_with_file, params: { id: '../\\../test/abstract_unit.rb' } }
assert_equal File.read(File.expand_path("../../test/abstract_unit.rb", __dir__)),
response.body
assert_deprecated do
assert_raises ActionView::MissingTemplate do
get :dynamic_render_with_file, params: { id: '../\\../test/abstract_unit.rb' }
end
end
end
def test_dynamic_render_with_absolute_path
@ -351,9 +352,11 @@ def test_dynamic_render
def test_permitted_dynamic_render_file_hash
assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__))
response = assert_deprecated { get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } } }
assert_equal File.read(File.expand_path("../../test/abstract_unit.rb", __dir__)),
response.body
assert_deprecated do
assert_raises ActionView::MissingTemplate do
get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } }
end
end
end
def test_dynamic_render_file_hash

5
actionview/lib/action_view/lookup_context.rb
View File

@ -130,9 +130,8 @@ 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
alias :find_file :find
deprecate :find_file
def find_all(name, prefixes = [], partial = false, keys = [], options = {})
@view_paths.find_all(*args_for_lookup(name, prefixes, partial, keys, options))

15
actionview/lib/action_view/path_set.rb
View File

@ -48,12 +48,11 @@ 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
alias :find_file :find
deprecate :find_file
def find_all(path, prefixes = [], *args)
_find_all path, prefixes, args, false
_find_all path, prefixes, args
end
def exists?(path, prefixes, *args)
@ -71,15 +70,11 @@ def find_all_with_query(query) # :nodoc:
private
def _find_all(path, prefixes, args, outside_app)
def _find_all(path, prefixes, args)
prefixes = [prefixes] if String === prefixes
prefixes.each do |prefix|
paths.each do |resolver|
if outside_app
templates = resolver.find_all_anywhere(path, prefix, *args)
else
templates = resolver.find_all(path, prefix, *args)
end
templates = resolver.find_all(path, prefix, *args)
return templates unless templates.empty?
end
end

2
actionview/lib/action_view/renderer/template_renderer.rb
View File

@ -30,7 +30,7 @@ def determine_template(options)
Template::File.new(options[:file])
else
ActiveSupport::Deprecation.warn "render file: should be given the absolute path to a file"
@lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details)
@lookup_context.with_fallbacks.find_template(options[:file], nil, false, keys, @details)
end
elsif options.key?(:inline)
handler = Template.handler_for_extension(options[:type] || "erb")

29
actionview/lib/action_view/template/resolver.rb
View File

@ -118,17 +118,12 @@ def find_all(name, prefix = nil, partial = false, details = {}, key = nil, local
locals = locals.map(&:to_s).sort!.freeze
cached(key, [name, prefix, partial], details, locals) do
find_templates(name, prefix, partial, details, false, locals)
find_templates(name, prefix, partial, details, locals)
end
end
def find_all_anywhere(name, prefix, partial = false, details = {}, key = nil, locals = [])
locals = locals.map(&:to_s).sort!.freeze
cached(key, [name, prefix, partial], details, locals) do
find_templates(name, prefix, partial, details, true, locals)
end
end
alias :find_all_anywhere :find_all
deprecate :find_all_anywhere
def find_all_with_query(query) # :nodoc:
@cache.cache_query(query) { find_template_paths(File.join(@path, query)) }
@ -141,8 +136,8 @@ def find_all_with_query(query) # :nodoc:
# This is what child classes implement. No defaults are needed
# because Resolver guarantees that the arguments are present and
# normalized.
def find_templates(name, prefix, partial, details, outside_app_allowed = false, locals = [])
raise NotImplementedError, "Subclasses must implement a find_templates(name, prefix, partial, details, outside_app_allowed = false, locals = []) method"
def find_templates(name, prefix, partial, details, locals = [])
raise NotImplementedError, "Subclasses must implement a find_templates(name, prefix, partial, details, locals = []) method"
end
# Handles templates caching. If a key is given and caching is on
@ -179,14 +174,14 @@ def initialize(pattern = nil)
private
def find_templates(name, prefix, partial, details, outside_app_allowed = false, locals)
def find_templates(name, prefix, partial, details, locals)
path = Path.build(name, prefix, partial)
query(path, details, details[:formats], outside_app_allowed, locals)
query(path, details, details[:formats], locals)
end
def query(path, details, formats, outside_app_allowed, locals)
def query(path, details, formats, locals)
template_paths = find_template_paths_from_details(path, details)
template_paths = reject_files_external_to_app(template_paths) unless outside_app_allowed
template_paths = reject_files_external_to_app(template_paths)
template_paths.map do |template|
build_template(template, path.virtual, locals)
@ -360,6 +355,8 @@ def build_regex(path, details)
# The same as FileSystemResolver but does not allow templates to store
# a virtual path since it is invalid for such resolvers.
class FallbackFileSystemResolver < FileSystemResolver #:nodoc:
private_class_method :new
def self.instances
[new(""), new("/")]
end
@ -367,5 +364,9 @@ def self.instances
def build_template(template, virtual_path, locals)
super(template, nil, locals)
end
def reject_files_external_to_app(files)
files
end
end
end

4
actionview/lib/action_view/testing/resolvers.rb
View File

@ -23,7 +23,7 @@ def to_s
private
def query(path, exts, _, _, locals)
def query(path, exts, _, locals)
query = +""
EXTENSIONS.each_key do |ext|
query << "(" << exts[ext].map { |e| e && Regexp.escape(".#{e}") }.join("|") << "|)"
@ -47,7 +47,7 @@ def query(path, exts, _, _, locals)
end
class NullResolver < PathResolver
def query(path, exts, _, _, locals)
def query(path, exts, _, locals)
handler, format, variant = extract_handler_and_format_and_variant(path)
[ActionView::Template.new("Template generated by Null Resolver", path.virtual, handler, virtual_path: path.virtual, format: format, variant: variant, locals: locals)]
end

2
actionview/test/template/fallback_file_system_resolver_test.rb
View File

@ -4,7 +4,7 @@
class FallbackFileSystemResolverTest < ActiveSupport::TestCase
def setup
@root_resolver = ActionView::FallbackFileSystemResolver.new("/")
@root_resolver = ActionView::FallbackFileSystemResolver.send(:new, "/")
end
def test_should_have_no_virtual_path

8
actionview/test/template/lookup_context_test.rb
View File

@ -143,16 +143,16 @@ def teardown
assert_deprecated do
@lookup_context.with_fallbacks do
assert_equal 3, @lookup_context.view_paths.size
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("")
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("/")
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.instances[0]
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.instances[1]
end
end
@lookup_context = @lookup_context.with_fallbacks
assert_equal 3, @lookup_context.view_paths.size
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("")
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.new("/")
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.instances[0]
assert_includes @lookup_context.view_paths, ActionView::FallbackFileSystemResolver.instances[1]
end
test "add fallbacks just once in nested fallbacks calls" do