aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJohn Hawthorn <john@hawthorn.email>2019-03-18 17:17:11 -0700
committerJohn Hawthorn <john@hawthorn.email>2019-03-27 15:51:25 -0700
commitc7820d8124c854760a4d288334f185de2fb99446 (patch)
tree08b50d4f926f0fb19e4b7d8c45f963ce6a84546c
parent93dbbe3a81bee6da2f1e88ca6971299b462cad93 (diff)
downloadrails-c7820d8124c854760a4d288334f185de2fb99446.tar.gz
rails-c7820d8124c854760a4d288334f185de2fb99446.tar.bz2
rails-c7820d8124c854760a4d288334f185de2fb99446.zip
Introduce Template::File as new render file:
The previous behaviour of render file: was essentially the same as render template:, except that templates can be specified as an absolute path on the filesystem. This makes sense for historic reasons, but now render file: is almost exclusively used to render raw files (not .erb) like public/404.html. In addition to complicating the code in template/resolver.rb, I think the current behaviour is surprising to developers. This commit deprecates the existing "lookup a template from anywhere" behaviour and replaces it with "render this file exactly as it is on disk". Handlers will no longer be used (it will render the same as if the :raw handler was used), but formats (.html, .xml, etc) will still be detected (and will default to :plain). The existing render file: behaviour was the path through which Rails apps were vulnerable in the recent CVE-2019-5418. Although the vulnerability has been patched in a fully backwards-compatible way, I think it's a strong hint that we should drop the existing previously-vulnerable behaviour if it isn't a benefit to developers.
-rw-r--r--actionpack/test/controller/new_base/render_file_test.rb24
-rw-r--r--actionpack/test/controller/render_test.rb4
-rw-r--r--actionpack/test/controller/renderer_test.rb2
-rw-r--r--actionview/lib/action_view/file_template.rb2
-rw-r--r--actionview/lib/action_view/renderer/template_renderer.rb7
-rw-r--r--actionview/lib/action_view/template.rb3
-rw-r--r--actionview/lib/action_view/template/error.rb2
-rw-r--r--actionview/lib/action_view/template/file.rb28
-rw-r--r--actionview/test/actionpack/abstract/render_test.rb2
-rw-r--r--actionview/test/actionpack/controller/render_test.rb44
-rw-r--r--actionview/test/template/log_subscriber_test.rb13
-rw-r--r--actionview/test/template/render_test.rb36
-rw-r--r--actionview/test/template/streaming_render_test.rb4
13 files changed, 131 insertions, 40 deletions
diff --git a/actionpack/test/controller/new_base/render_file_test.rb b/actionpack/test/controller/new_base/render_file_test.rb
index de8af029e0..82325c5bb2 100644
--- a/actionpack/test/controller/new_base/render_file_test.rb
+++ b/actionpack/test/controller/new_base/render_file_test.rb
@@ -40,32 +40,44 @@ module RenderFile
testing RenderFile::BasicController
test "rendering simple template" do
- get :index
+ assert_deprecated do
+ get :index
+ end
assert_response "Hello world!"
end
test "rendering template with ivar" do
- get :with_instance_variables
+ assert_deprecated do
+ get :with_instance_variables
+ end
assert_response "The secret is in the sauce\n"
end
test "rendering a relative path" do
- get :relative_path
+ assert_deprecated do
+ get :relative_path
+ end
assert_response "The secret is in the sauce\n"
end
test "rendering a relative path with dot" do
- get :relative_path_with_dot
+ assert_deprecated do
+ get :relative_path_with_dot
+ end
assert_response "The secret is in the sauce\n"
end
test "rendering a Pathname" do
- get :pathname
+ assert_deprecated do
+ get :pathname
+ end
assert_response "The secret is in the sauce\n"
end
test "rendering file with locals" do
- get :with_locals
+ assert_deprecated do
+ get :with_locals
+ end
assert_response "The secret is in the sauce\n"
end
end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 4750093c5c..6d198ca42f 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -325,7 +325,7 @@ class ExpiresInRenderTest < ActionController::TestCase
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 = get :dynamic_render_with_file, params: { id: '../\\../test/abstract_unit.rb' }
+ 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
end
@@ -351,7 +351,7 @@ class ExpiresInRenderTest < ActionController::TestCase
def test_permitted_dynamic_render_file_hash
assert File.exist?(File.expand_path("../../test/abstract_unit.rb", __dir__))
- response = get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } }
+ 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
end
diff --git a/actionpack/test/controller/renderer_test.rb b/actionpack/test/controller/renderer_test.rb
index ae8330e029..3d5161f207 100644
--- a/actionpack/test/controller/renderer_test.rb
+++ b/actionpack/test/controller/renderer_test.rb
@@ -40,7 +40,7 @@ class RendererTest < ActiveSupport::TestCase
test "rendering with an instance renderer" do
renderer = ApplicationController.renderer.new
- content = renderer.render file: "test/hello_world"
+ content = assert_deprecated { renderer.render file: "test/hello_world" }
assert_equal "Hello world!", content
end
diff --git a/actionview/lib/action_view/file_template.rb b/actionview/lib/action_view/file_template.rb
index dea02176eb..e0dc7da3b6 100644
--- a/actionview/lib/action_view/file_template.rb
+++ b/actionview/lib/action_view/file_template.rb
@@ -11,7 +11,7 @@ module ActionView
end
def source
- File.binread @filename
+ ::File.binread @filename
end
def refresh(_)
diff --git a/actionview/lib/action_view/renderer/template_renderer.rb b/actionview/lib/action_view/renderer/template_renderer.rb
index 9548fe12c4..60cb2ceafa 100644
--- a/actionview/lib/action_view/renderer/template_renderer.rb
+++ b/actionview/lib/action_view/renderer/template_renderer.rb
@@ -26,7 +26,12 @@ module ActionView
elsif options.key?(:html)
Template::HTML.new(options[:html], formats.first)
elsif options.key?(:file)
- @lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details)
+ if File.exist?(options[:file])
+ 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)
+ end
elsif options.key?(:inline)
handler = Template.handler_for_extension(options[:type] || "erb")
format = if handler.respond_to?(:default_format)
diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb
index 6e3af1536a..2230de2ca2 100644
--- a/actionview/lib/action_view/template.rb
+++ b/actionview/lib/action_view/template.rb
@@ -113,6 +113,7 @@ module ActionView
eager_autoload do
autoload :Error
+ autoload :File
autoload :Handlers
autoload :HTML
autoload :Inline
@@ -139,7 +140,7 @@ module ActionView
@virtual_path = virtual_path
@variable = if @virtual_path
- base = @virtual_path[-1] == "/" ? "" : File.basename(@virtual_path)
+ base = @virtual_path[-1] == "/" ? "" : ::File.basename(@virtual_path)
base =~ /\A_?(.*?)(?:\.\w+)*\z/
$1.to_sym
end
diff --git a/actionview/lib/action_view/template/error.rb b/actionview/lib/action_view/template/error.rb
index 4e3c02e05e..e6d9e35815 100644
--- a/actionview/lib/action_view/template/error.rb
+++ b/actionview/lib/action_view/template/error.rb
@@ -104,7 +104,7 @@ module ActionView
def line_number
@line_number ||=
if file_name
- regexp = /#{Regexp.escape File.basename(file_name)}:(\d+)/
+ regexp = /#{Regexp.escape ::File.basename(file_name)}:(\d+)/
$1 if message =~ regexp || backtrace.find { |line| line =~ regexp }
end
end
diff --git a/actionview/lib/action_view/template/file.rb b/actionview/lib/action_view/template/file.rb
new file mode 100644
index 0000000000..487e5735cf
--- /dev/null
+++ b/actionview/lib/action_view/template/file.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module ActionView #:nodoc:
+ # = Action View File Template
+ class Template #:nodoc:
+ class File #:nodoc:
+ attr_accessor :type, :format
+
+ def initialize(filename)
+ @filename = filename.to_s
+ extname = ::File.extname(filename).delete(".")
+ @type = Template::Types[extname] || Template::Types[:text]
+ @format = @type.symbol
+ end
+
+ def identifier
+ @filename
+ end
+
+ def render(*args)
+ ::File.read(@filename)
+ end
+
+ def formats; Array(format); end
+ deprecate :formats
+ end
+ end
+end
diff --git a/actionview/test/actionpack/abstract/render_test.rb b/actionview/test/actionpack/abstract/render_test.rb
index d863548a5c..e4e8ac93b2 100644
--- a/actionview/test/actionpack/abstract/render_test.rb
+++ b/actionview/test/actionpack/abstract/render_test.rb
@@ -26,7 +26,7 @@ module AbstractController
end
def file
- render file: "some/file"
+ ActiveSupport::Deprecation.silence { render file: "some/file" }
end
def inline
diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
index 52c3c54d96..c8ce7366d1 100644
--- a/actionview/test/actionpack/controller/render_test.rb
+++ b/actionview/test/actionpack/controller/render_test.rb
@@ -872,48 +872,64 @@ class RenderTest < ActionController::TestCase
# :ported:
def test_render_file_with_instance_variables
- get :render_file_with_instance_variables
+ assert_deprecated do
+ get :render_file_with_instance_variables
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
def test_render_file
- get :hello_world_file
+ assert_deprecated do
+ get :hello_world_file
+ end
assert_equal "Hello world!", @response.body
end
# :ported:
def test_render_file_not_using_full_path
- get :render_file_not_using_full_path
+ assert_deprecated do
+ get :render_file_not_using_full_path
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
# :ported:
def test_render_file_not_using_full_path_with_dot_in_path
- get :render_file_not_using_full_path_with_dot_in_path
+ assert_deprecated do
+ get :render_file_not_using_full_path_with_dot_in_path
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
# :ported:
def test_render_file_using_pathname
- get :render_file_using_pathname
+ assert_deprecated do
+ get :render_file_using_pathname
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
# :ported:
def test_render_file_with_locals
- get :render_file_with_locals
+ assert_deprecated do
+ get :render_file_with_locals
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
# :ported:
def test_render_file_as_string_with_locals
- get :render_file_as_string_with_locals
+ assert_deprecated do
+ get :render_file_as_string_with_locals
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
# :assessed:
def test_render_file_from_template
- get :render_file_from_template
+ assert_deprecated do
+ get :render_file_from_template
+ end
assert_equal "The secret is in the sauce\n", @response.body
end
@@ -1133,11 +1149,19 @@ class RenderTest < ActionController::TestCase
end
def test_bad_render_to_string_still_throws_exception
- assert_raise(ActionView::MissingTemplate) { get :render_to_string_with_exception }
+ assert_deprecated do
+ assert_raise(ActionView::MissingTemplate) do
+ get :render_to_string_with_exception
+ end
+ end
end
def test_render_to_string_that_throws_caught_exception_doesnt_break_assigns
- assert_nothing_raised { get :render_to_string_with_caught_exception }
+ assert_deprecated do
+ assert_nothing_raised do
+ get :render_to_string_with_caught_exception
+ end
+ end
assert_equal "i'm before the render", @controller.instance_variable_get(:@before)
assert_equal "i'm after the render", @controller.instance_variable_get(:@after)
end
diff --git a/actionview/test/template/log_subscriber_test.rb b/actionview/test/template/log_subscriber_test.rb
index 85735139c1..8b160a7336 100644
--- a/actionview/test/template/log_subscriber_test.rb
+++ b/actionview/test/template/log_subscriber_test.rb
@@ -51,9 +51,20 @@ class AVLogSubscriberTest < ActiveSupport::TestCase
def @view.combined_fragment_cache_key(*); "ahoy `controller` dependency"; end
end
+ def test_render_template_template
+ Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do
+ @view.render(template: "test/hello_world")
+ wait
+
+ assert_equal 2, @logger.logged(:info).size
+ assert_match(/Rendering test\/hello_world\.erb/, @logger.logged(:info).first)
+ assert_match(/Rendered test\/hello_world\.erb/, @logger.logged(:info).last)
+ end
+ end
+
def test_render_file_template
Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do
- @view.render(file: "test/hello_world")
+ @view.render(file: "#{FIXTURE_LOAD_PATH}/test/hello_world.erb")
wait
assert_equal 2, @logger.logged(:info).size
diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb
index f0fed601f8..3cf3903a58 100644
--- a/actionview/test/template/render_test.rb
+++ b/actionview/test/template/render_test.rb
@@ -53,15 +53,20 @@ module RenderTestCases
assert_match(/You invoked render but did not give any of (.+) option\./, e.message)
end
+ def test_render_template
+ assert_equal "Hello world!", @view.render(template: "test/hello_world")
+ end
+
+
def test_render_file
- assert_equal "Hello world!", @view.render(file: "test/hello_world")
+ assert_equal "Hello world!", assert_deprecated { @view.render(file: "test/hello_world") }
end
# Test if :formats, :locale etc. options are passed correctly to the resolvers.
def test_render_file_with_format
- assert_match "<h1>No Comment</h1>", @view.render(file: "comments/empty", formats: [:html])
- assert_match "<error>No Comment</error>", @view.render(file: "comments/empty", formats: [:xml])
- assert_match "<error>No Comment</error>", @view.render(file: "comments/empty", formats: :xml)
+ assert_match "<h1>No Comment</h1>", assert_deprecated { @view.render(file: "comments/empty", formats: [:html]) }
+ assert_match "<error>No Comment</error>", assert_deprecated { @view.render(file: "comments/empty", formats: [:xml]) }
+ assert_match "<error>No Comment</error>", assert_deprecated { @view.render(file: "comments/empty", formats: :xml) }
end
def test_render_template_with_format
@@ -94,8 +99,8 @@ module RenderTestCases
end
def test_render_file_with_locale
- assert_equal "<h1>Kein Kommentar</h1>", @view.render(file: "comments/empty", locale: [:de])
- assert_equal "<h1>Kein Kommentar</h1>", @view.render(file: "comments/empty", locale: :de)
+ assert_equal "<h1>Kein Kommentar</h1>", assert_deprecated { @view.render(file: "comments/empty", locale: [:de]) }
+ assert_equal "<h1>Kein Kommentar</h1>", assert_deprecated { @view.render(file: "comments/empty", locale: :de) }
end
def test_render_template_with_locale
@@ -107,8 +112,8 @@ module RenderTestCases
end
def test_render_file_with_handlers
- assert_equal "<h1>No Comment</h1>\n", @view.render(file: "comments/empty", handlers: [:builder])
- assert_equal "<h1>No Comment</h1>\n", @view.render(file: "comments/empty", handlers: :builder)
+ assert_equal "<h1>No Comment</h1>\n", assert_deprecated { @view.render(file: "comments/empty", handlers: [:builder]) }
+ assert_equal "<h1>No Comment</h1>\n", assert_deprecated { @view.render(file: "comments/empty", handlers: :builder) }
end
def test_render_template_with_handlers
@@ -156,22 +161,27 @@ module RenderTestCases
assert_equal "Elastica", @view.render(template: "/shared")
end
- def test_render_file_with_full_path
+ def test_render_file_with_full_path_no_extension
template_path = File.expand_path("../fixtures/test/hello_world", __dir__)
+ assert_equal "Hello world!", assert_deprecated { @view.render(file: template_path) }
+ end
+
+ def test_render_file_with_full_path
+ template_path = File.expand_path("../fixtures/test/hello_world.erb", __dir__)
assert_equal "Hello world!", @view.render(file: template_path)
end
def test_render_file_with_instance_variables
- assert_equal "The secret is in the sauce\n", @view.render(file: "test/render_file_with_ivar")
+ assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/render_file_with_ivar") }
end
def test_render_file_with_locals
locals = { secret: "in the sauce" }
- assert_equal "The secret is in the sauce\n", @view.render(file: "test/render_file_with_locals", locals: locals)
+ assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/render_file_with_locals", locals: locals) }
end
def test_render_file_not_using_full_path_with_dot_in_path
- assert_equal "The secret is in the sauce\n", @view.render(file: "test/dot.directory/render_file_with_ivar")
+ assert_equal "The secret is in the sauce\n", assert_deprecated { @view.render(file: "test/dot.directory/render_file_with_ivar") }
end
def test_render_partial_from_default
@@ -286,7 +296,7 @@ module RenderTestCases
end
def test_render_file_with_errors
- e = assert_raises(ActionView::Template::Error) { @view.render(file: File.expand_path("test/_raise", FIXTURE_LOAD_PATH)) }
+ e = assert_raises(ActionView::Template::Error) { assert_deprecated { @view.render(file: File.expand_path("test/_raise", FIXTURE_LOAD_PATH)) } }
assert_match %r!method.*doesnt_exist!, e.message
assert_equal "", e.sub_template_message
assert_equal "1", e.line_number
diff --git a/actionview/test/template/streaming_render_test.rb b/actionview/test/template/streaming_render_test.rb
index a5b59a700e..a5e673e71e 100644
--- a/actionview/test/template/streaming_render_test.rb
+++ b/actionview/test/template/streaming_render_test.rb
@@ -47,12 +47,12 @@ class FiberedTest < SetupFiberedBase
end
def test_render_file
- assert_equal "Hello world!", buffered_render(file: "test/hello_world")
+ assert_equal "Hello world!", assert_deprecated { buffered_render(file: "test/hello_world") }
end
def test_render_file_with_locals
locals = { secret: "in the sauce" }
- assert_equal "The secret is in the sauce\n", buffered_render(file: "test/render_file_with_locals", locals: locals)
+ assert_equal "The secret is in the sauce\n", assert_deprecated { buffered_render(file: "test/render_file_with_locals", locals: locals) }
end
def test_render_partial