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.
rails · Feb 29, 2016 · 8a1d3ea · 8a1d3ea
1 parent 31ab3aa
commit 8a1d3ea
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 49 deletions.
diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb
@@ -13,15 +13,6 @@ def with_instance_variables
       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 @@ def with_locals
       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 @@ class TestBasic < Rack::TestCase
       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 @@ class TestBasic < Rack::TestCase
       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
@@ -45,6 +45,10 @@ def with_locals
       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 @@ class TestWithoutLayout < Rack::TestCase
       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 "<html>\n  <p>Hello</p>\n</html>\n"

diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
@@ -261,6 +261,11 @@ def accessing_logger_in_template
 class ExpiresInRenderTest < ActionController::TestCase
   tests TestController
 
+  def setup
+    super
+    ActionController::Base.view_paths.paths.each(&:clear_cache)
+  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'))
@@ -269,6 +274,18 @@ def test_dynamic_render_with_file
       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
     assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb'))
     assert_raises ActionView::MissingTemplate do

diff --git a/actionview/CHANGELOG.md b/actionview/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*
+
+
 ## Rails 4.1.14 (November 12, 2015) ##
 
 *   Fix `mail_to` when called with `nil` as argument.

diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb
@@ -107,7 +107,7 @@ def _process_format(format, options = {}) #:nodoc:
       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: private
       def _normalize_args(action=nil, options={})
         options = super(action, options)
@@ -117,7 +117,7 @@ def _normalize_args(action=nil, options={})
           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/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
@@ -91,17 +91,17 @@ def hello_world_file
 
   # :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:
@@ -111,7 +111,7 @@ def render_template_in_top_directory
 
   # :deprecated:
   def render_template_in_top_directory_with_slash
-    render :template => '/shared'
+    render '/shared'
   end
 
   # :ported:
@@ -159,13 +159,6 @@ def render_file_with_instance_variables
     render :file => path
   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'
@@ -194,7 +187,7 @@ def render_file_with_locals
 
   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
@@ -780,12 +773,6 @@ def test_render_file
     assert_equal "Hello world!", @response.body
   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