Prevent assigning internal ivars to AV::Base

skipkayhil · byroot · jonathanhefner · skipkayhil · commit 24213d6954d0 · 2023-11-03T20:52:47.000-04:00
Previously, both the `@rendered_format` and
`@marked_for_same_origin_verification` instance variables would be
assigned to instances of `ActionView::Base`, making them accessible in
view templates. However, these instance variables are really internal to
the controller and result in extra string allocations because the `@`
gets stripped and readded when going through the assignment.

This commit prefixes the variables with an underscore to help indicate
that they are internal, and then adds them to the list of
`_protected_ivars` to prevent assigning them when rendering templates.

Co-authored-by: Jean Boussier &lt;jean.boussier@gmail.com&gt;
Co-authored-by: Jonathan Hefner &lt;jonathan@hefner.pro&gt;
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
@@ -247,6 +247,7 @@ def self.without_modules(*modules)
     PROTECTED_IVARS = AbstractController::Rendering::DEFAULT_PROTECTED_INSTANCE_VARIABLES + %i(
       @_params @_response @_request @_config @_url_options @_action_has_layout @_view_context_class
       @_view_renderer @_lookup_context @_routes @_view_runtime @_db_runtime @_helper_proxy
+      @_marked_for_same_origin_verification @_rendered_format
     )
 
     def _protected_ivars
diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -340,7 +340,7 @@ def reset(request)
 
     def initialize(...)
       super
-      @marked_for_same_origin_verification = nil
+      @_marked_for_same_origin_verification = nil
     end
 
     def reset_csrf_token(request) # :doc:
@@ -414,13 +414,13 @@ def verify_same_origin_request # :doc:
 
       # GET requests are checked for cross-origin JavaScript after rendering.
       def mark_for_same_origin_verification! # :doc:
-        @marked_for_same_origin_verification = request.get?
+        @_marked_for_same_origin_verification = request.get?
       end
 
       # If the +verify_authenticity_token+ before_action ran, verify that
       # JavaScript responses are only served to same-origin GET requests.
       def marked_for_same_origin_verification? # :doc:
-        @marked_for_same_origin_verification ||= false
+        @_marked_for_same_origin_verification ||= false
       end
 
       # Check for cross-origin JavaScript responses.
diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb
@@ -27,10 +27,10 @@ module Rendering
     extend ActiveSupport::Concern
     include ActionView::ViewPaths
 
-    attr_reader :rendered_format
+    attr_internal_reader :rendered_format
 
     def initialize
-      @rendered_format = nil
+      @_rendered_format = nil
       super
     end
 
@@ -136,7 +136,7 @@ def _render_template(options)
         end
 
         rendered_format = rendered_template.format || lookup_context.formats.first
-        @rendered_format = Template::Types[rendered_format]
+        @_rendered_format = Template::Types[rendered_format]
 
         rendered_template.body
       end
diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
@@ -126,6 +126,10 @@ def hello_world_with_layout_false
     render layout: false
   end
 
+  def render_instance_variables
+    render inline: "<%= instance_variables.sort %>"
+  end
+
   # :ported:
   def render_template_with_instance_variables
     @secret = "in the sauce"
@@ -721,6 +725,7 @@ class RenderTest < ActionController::TestCase
     get :render_hello_world_with_forward_slash, to: "test#render_hello_world_with_forward_slash"
     get :render_implicit_html_template_from_xhr_request, to: "test#render_implicit_html_template_from_xhr_request"
     get :render_implicit_js_template_without_layout, to: "test#render_implicit_js_template_without_layout"
+    get :render_instance_variables, to: "test#render_instance_variables"
     get :render_line_offset, to: "test#render_line_offset"
     get :render_nothing_with_appendix, to: "test#render_nothing_with_appendix"
     get :render_template_in_top_directory, to: "test#render_template_in_top_directory"
@@ -785,6 +790,29 @@ def test_simple_show
     assert_equal "<html>Hello world!</html>", @response.body
   end
 
+  def test_controller_does_not_leak_instance_variables
+    expected = [
+      :@_assigns, # attr_internal on ActionView::Base
+      :@_config, # attr_internal on ActionView::Base
+      :@_controller, # attr_internal on ActionView::Helpers::ControllerHelper
+      :@_default_form_builder, # attr_internal on ActionView::Helpers::FormHelper
+      :@_ivars, # ActionController::Testing::Functional (only appears inside an ActionController::TestCase)
+      :@_request, # attr_internal on ActionView::Helpers::ControllerHelper
+      :@current_template, # instance variable on ActionView::Base
+      :@lookup_context, # attr_reader on ActionView::Base
+      :@output_buffer, # attr_accessor on ActionView::Base::Context
+      :@variable_for_layout, # part of this test class
+      :@view_flow, # attr_accessor on ActionView::Base::Context
+      :@view_renderer, # attr_reader on ActionView::Base
+      :@virtual_path, # instance variable on ActionView::Base
+    ].inspect
+
+    get :render_instance_variables
+
+    assert_response 200
+    assert_equal expected, @response.body
+  end
+
   # :ported:
   def test_renders_default_template_for_missing_action
     get :'hyphen-ated'

Original file line number	Diff line number	Diff line change
`@@ -247,6 +247,7 @@ def self.without_modules(*modules)`
`247`	`247`	`PROTECTED_IVARS = AbstractController::Rendering::DEFAULT_PROTECTED_INSTANCE_VARIABLES + %i(`
`248`	`248`	`@_params @_response @_request @_config @_url_options @_action_has_layout @_view_context_class`
`249`	`249`	`@_view_renderer @_lookup_context @_routes @_view_runtime @_db_runtime @_helper_proxy`
	`250`	`+ @_marked_for_same_origin_verification @_rendered_format`
`250`	`251`	`)`
`251`	`252`
`252`	`253`	`def _protected_ivars`