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 cdabc95 commit 18269d2
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 4 deletions.
17 changes: 17 additions & 0 deletions actionpack/lib/action_view/template/resolver.rb
Expand Up @@ -110,6 +110,9 @@ def initialize(pattern=nil)
super()
end

cattr_accessor :allow_external_files, instance_reader: false, instance_writer: false

This comment has been minimized.

Copy link
@simi

simi Jan 25, 2016

Contributor

This kills Ruby 1.8.7 compatibility. :( Is PR welcomed?

This comment has been minimized.

Copy link
@cntrytwist

cntrytwist Jan 25, 2016

For me it means I'll have to stay on the 3.2.22 version till I can get away from Ruby 1.8.7. It is in the works, but not within the next month.

This comment has been minimized.

Copy link
@maclover7

maclover7 Jan 25, 2016

Contributor

commented on #23248

self.allow_external_files = false

private

def find_templates(name, prefix, partial, details)
Expand All @@ -122,6 +125,10 @@ def query(path, details, formats)

template_paths = find_template_paths query

unless self.class.allow_external_files
template_paths = reject_files_external_to_app(template_paths)
end

template_paths.map { |template|
handler, format = extract_handler_and_format(template, formats)
contents = File.binread template
Expand All @@ -133,6 +140,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 @@ -153,6 +164,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
18 changes: 14 additions & 4 deletions actionpack/test/controller/new_base/render_file_test.rb
Expand Up @@ -72,13 +72,23 @@ class TestBasic < Rack::TestCase
end

test "rendering a relative path" do
get :relative_path
assert_response "The secret is in the sauce\n"
begin
ActionView::PathResolver.allow_external_files = true
get :relative_path
assert_response "The secret is in the sauce\n"
ensure
ActionView::PathResolver.allow_external_files = false
end
end

test "rendering a relative path with dot" do
get :relative_path_with_dot
assert_response "The secret is in the sauce\n"
begin
ActionView::PathResolver.allow_external_files = true
get :relative_path_with_dot
assert_response "The secret is in the sauce\n"
ensure
ActionView::PathResolver.allow_external_files = false
end
end

test "rendering a Pathname" do
Expand Down
31 changes: 31 additions & 0 deletions actionpack/test/controller/render_test.rb
Expand Up @@ -60,6 +60,16 @@ def conditional_hello_with_record
end
end

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

def dynamic_render_with_file
# This is extremely bad, but should be possible to do.
file = params[:id] # => String, Hash
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 @@ -235,6 +245,27 @@ def accessing_action_name_in_template
render :inline => "<%= action_name %>"
end

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 accessing_controller_name_in_template
render :inline => "<%= controller_name %>"
end
Expand Down
7 changes: 7 additions & 0 deletions actionpack/test/template/render_test.rb
Expand Up @@ -126,6 +126,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 18269d2

Please sign in to comment.