Merge pull request rails#49858 from skipkayhil/hm-dont-assign-internal-variables

tenderlove · web-flow · commit 0915a3eed8d1 · 2023-12-11T09:06:05.000-08:00
Prevent assigning internal ivars to AV::Base
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`