From 1a65dd1c21cb7a70db054793deeb19dea1b357cf Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 26 Jan 2016 17:06:31 -0800 Subject: [PATCH 1/2] Change render "foo" to render a template and not a file. Previously, calling `render "foo/bar"` in a controller action is equivalent to `render file: "foo/bar"`. This has been changed to mean `render template: "foo/bar"` instead. If you need to render a file, please change your code to use the explicit form (`render file: "foo/bar"`) instead. Test that we are not allowing you to grab a file with an absolute path outside of your application directory. This is dangerous because it could be used to retrieve files from the server like `/etc/passwd`. Fix CVE-2016-2097. --- .../test/controller/new_base/render_file_test.rb | 29 ---------------------- .../controller/new_base/render_template_test.rb | 9 +++++++ actionpack/test/controller/render_test.rb | 17 +++++++++++++ actionpack/CHANGELOG.md | 10 ++++++++ actionpack/lib/abstract_controller/rendering.rb | 4 +-- .../test/controller/render_test.rb | 23 ++++------------- 6 files changed, 43 insertions(+), 49 deletions(-) diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb index a961cbf..0c21bb0 100644 --- a/actionpack/test/controller/new_base/render_file_test.rb +++ b/actionpack/test/controller/new_base/render_file_test.rb @@ -13,15 +13,6 @@ module RenderFile render :file => File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') end - def without_file_key - render File.join(File.dirname(__FILE__), *%w[.. .. fixtures test hello_world]) - end - - def without_file_key_with_instance_variable - @secret = 'in the sauce' - render File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_ivar') - end - def relative_path @secret = 'in the sauce' render :file => '../../fixtures/test/render_file_with_ivar' @@ -41,11 +32,6 @@ module RenderFile path = File.join(File.dirname(__FILE__), '../../fixtures/test/render_file_with_locals') render :file => path, :locals => {:secret => 'in the sauce'} end - - def without_file_key_with_locals - path = FIXTURES.join('test/render_file_with_locals').to_s - render path, :locals => {:secret => 'in the sauce'} - end end class TestBasic < Rack::TestCase @@ -61,16 +47,6 @@ module RenderFile assert_response "The secret is in the sauce\n" end - test "rendering path without specifying the :file key" do - get :without_file_key - assert_response "Hello world!" - end - - test "rendering path without specifying the :file key with ivar" do - get :without_file_key_with_instance_variable - assert_response "The secret is in the sauce\n" - end - test "rendering a relative path" do get :relative_path assert_response "The secret is in the sauce\n" @@ -90,10 +66,5 @@ module RenderFile get :with_locals assert_response "The secret is in the sauce\n" end - - test "rendering path without specifying the :file key with locals" do - get :without_file_key_with_locals - assert_response "The secret is in the sauce\n" - end end end diff --git a/actionpack/test/controller/new_base/render_template_test.rb b/actionpack/test/controller/new_base/render_template_test.rb index b7a9cf9..b0c4efb 100644 --- a/actionpack/test/controller/new_base/render_template_test.rb +++ b/actionpack/test/controller/new_base/render_template_test.rb @@ -45,6 +45,10 @@ module RenderTemplate render :template => "locals", :locals => { :secret => 'area51' } end + def with_locals_without_key + render "locals", :locals => { :secret => 'area51' } + end + def builder_template render :template => "xml_template" end @@ -101,6 +105,11 @@ module RenderTemplate assert_response "The secret is area51" end + test "rendering a template with local variables without key" do + get :with_locals + assert_response "The secret is area51" + end + test "rendering a builder template" do get :builder_template, "format" => "xml" assert_response "\n

Hello

\n\n" diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 17a019e..0fcbb86 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -1551,7 +1551,8 @@ tests TestController def setup - @request.host = "www.nextangle.com" + super + ActionController::Base.view_paths.paths.each(&:clear_cache) end def test_dynamic_render_with_file @@ -1562,6 +1563,18 @@ response.body end + def test_dynamic_render_with_absolute_path + file = Tempfile.new('name') + file.write "secrets!" + file.flush + assert_raises ActionView::MissingTemplate do + get :dynamic_render, { id: file.path } + end + ensure + file.close + file.unlink + end + def test_dynamic_render_file_hash assert_raises ArgumentError do get :dynamic_render, { id: { file: '../\\../test/abstract_unit.rb' } } diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 80a2a5e..05bda0d 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* Changed the meaning of `render "foo/bar"`. + + Previously, calling `render "foo/bar"` in a controller action is equivalent + to `render file: "foo/bar"`. This has been changed to mean + `render template: "foo/bar"` instead. If you need to render a file, please + change your code to use the explicit form (`render file: "foo/bar"`) instead. + + *Eileen Uchitelle* + + * Ensure simple_format escapes its html attributes. This fixes CVE-2013-6416 * Deep Munge the parameters for GET and POST Fixes CVE-2013-6417 diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionview/lib/abstract_controller/rendering.rb index 017302d..6283830 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -155,7 +155,7 @@ end # Normalize args by converting render "foo" to render :action => "foo" and - # render "foo/bar" to render :file => "foo/bar". + # render "foo/bar" to render :template => "foo/bar". # :api: plugin def _normalize_args(action=nil, options={}) case action @@ -169,7 +169,7 @@ options = action when String, Symbol action = action.to_s - key = action.include?(?/) ? :file : :action + key = action.include?(?/) ? :template : :action options[key] = action else options[:partial] = action diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 45b8049..a9991fe 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -159,17 +159,17 @@ class TestController < ApplicationController # :ported: def render_hello_world - render :template => "test/hello_world" + render "test/hello_world" end def render_hello_world_with_last_modified_set response.last_modified = Date.new(2008, 10, 10).to_time - render :template => "test/hello_world" + render "test/hello_world" end # :ported: compatibility def render_hello_world_with_forward_slash - render :template => "/test/hello_world" + render "/test/hello_world" end # :ported: @@ -179,7 +179,7 @@ class TestController < ApplicationController # :deprecated: def render_template_in_top_directory_with_slash - render :template => '/shared' + render '/shared' end # :ported: @@ -228,13 +228,6 @@ class TestController < ApplicationController end # :ported: - def render_file_as_string_with_instance_variables - @secret = 'in the sauce' - path = File.expand_path(File.join(File.dirname(__FILE__), '../fixtures/test/render_file_with_ivar')) - render path - end - - # :ported: def render_file_not_using_full_path @secret = 'in the sauce' render :file => 'test/render_file_with_ivar' @@ -262,7 +255,7 @@ class TestController < ApplicationController def render_file_as_string_with_locals path = File.expand_path(File.join(File.dirname(__FILE__), '../fixtures/test/render_file_with_locals')) - render path, :locals => {:secret => 'in the sauce'} + render file: path, :locals => {:secret => 'in the sauce'} end def accessing_request_in_template @@ -897,12 +890,6 @@ class RenderTest < ActionController::TestCase end # :ported: - def test_render_file_as_string_with_instance_variables - get :render_file_as_string_with_instance_variables - assert_equal "The secret is in the sauce\n", @response.body - end - - # :ported: def test_render_file_not_using_full_path get :render_file_not_using_full_path assert_equal "The secret is in the sauce\n", @response.body -- 2.7.0