diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 7902ebcff..fc093f7e6 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -200,6 +200,10 @@ def evaluate(object) object.respond_to?(:evaluate) ? object.evaluate(self) : object end + def self_drop + @self_drop ||= SelfDrop.new(self) + end + # Fetches an object starting at the local scope and then moving up the hierachy def find_variable(key, raise_on_not_found: true) # This was changed from find() to find_index() because this is a very hot @@ -208,7 +212,7 @@ def find_variable(key, raise_on_not_found: true) # `self` resolves to a SelfDrop (enabling `self['var']` lookups), # but only when it hasn't been explicitly assigned as a local variable. - return SelfDrop.new(self) if key == Expression::SELF && !index + return self_drop if key == Expression::SELF && !index variable = if index lookup_and_evaluate(@scopes[index], key, raise_on_not_found: raise_on_not_found) diff --git a/lib/liquid/self_drop.rb b/lib/liquid/self_drop.rb index 357814653..30b81f857 100644 --- a/lib/liquid/self_drop.rb +++ b/lib/liquid/self_drop.rb @@ -16,23 +16,42 @@ module Liquid # then the local value takes precedence over the `self` object. # @liquid_access global class SelfDrop < Drop + attr_accessor :bound_self + def initialize(context) super() @context = context + @bound_self = nil end def [](key) - @context.find_variable(key) + if @bound_self && bound_has?(key) + bound_lookup(key) + else + @context.find_variable(key) + end rescue UndefinedVariable nil end def key?(key) - @context.variable_defined?(key) + (@bound_self && bound_has?(key)) || @context.variable_defined?(key) end def to_liquid self end + + private + + def bound_has?(key) + @bound_self.respond_to?(:key?) && @bound_self.key?(key) + end + + def bound_lookup(key) + return unless @bound_self.respond_to?(:[]) + + @bound_self[key] + end end end diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 6e1559cc2..01bef807b 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -66,7 +66,12 @@ def render_tag(context, output) inner_context['forloop'] = forloop if forloop @attributes.each do |key, value| - inner_context[key] = context.evaluate(value) + evaluated = context.evaluate(value) + if key == Expression::SELF + inner_context.self_drop.bound_self = evaluated + else + inner_context[key] = evaluated + end end inner_context[context_variable_name] = var unless var.nil? partial.render_to_output_buffer(inner_context, output) diff --git a/test/integration/self_drop_render_test.rb b/test/integration/self_drop_render_test.rb new file mode 100644 index 000000000..40e0805a4 --- /dev/null +++ b/test/integration/self_drop_render_test.rb @@ -0,0 +1,234 @@ +# frozen_string_literal: true + +require 'test_helper' + +# Tests for self[var] lookup behavior across {% render %} boundaries, +# including the `self:` bound-parameter shape used by the rewriter. +class SelfDropRenderTest < Minitest::Test + include Liquid + + # Snippet body using the rewriter's `self[name_var]` form. `item_1_title` + # is a template-local assign; `name` is built at runtime; the rewriter + # produces `self[name]` because bare brackets are forbidden in :strict2. + REWRITTEN_SNIPPET = <<~LIQUID + {%- liquid + assign item_1_title = 'Cookware Set' + -%} + {%- for i in (1..1) -%} + {%- liquid + assign name = 'item_' | append: i | append: '_title' + assign title = self[name] + -%} + [{{ title }}] + {%- endfor -%} + LIQUID + + # Original (pre-rewrite) snippet body using bare-bracket lookup. Rejected + # at parse time by :strict2 -- which is why the rewriter exists. + ORIGINAL_SNIPPET = <<~LIQUID + {%- liquid + assign item_1_title = 'Cookware Set' + -%} + {%- for i in (1..1) -%} + {%- liquid + assign name = 'item_' | append: i | append: '_title' + assign title = [name] + -%} + [{{ title }}] + {%- endfor -%} + LIQUID + + EXPECTED_OUTPUT = '[Cookware Set]' + + # Baseline: parent does NOT pass `self:` to the snippet. The SelfDrop is + # returned by find_variable (no scope has the `self` key), and its `[]` + # walks back through the scope chain to find the for-loop-local + # `item_1_title`. This is the parity-safe case for the rewriter's + # transform; passing today. + def test_rewritten_self_lookup_without_self_named_param_resolves_local_assign + assert_template_result( + EXPECTED_OUTPUT, + "{% render 'snippet' %}", + partials: { 'snippet' => REWRITTEN_SNIPPET }, + error_mode: :strict2, + ) + end + + # PRODUCTION FAILURE SHAPE. + # + # Parent passes `self:` as a named render parameter. Render's + # `inner_context[key] = context.evaluate(value)` (render.rb:68-70) + # writes `my_obj` to `inner_context['self']`, which lands in + # @scopes[0] (context.rb:172-174). Now find_variable's check at + # context.rb:209-213 sees `self` defined in scope[0] and skips the + # SelfDrop fallthrough -- `self[name]` becomes a literal key-access + # against `my_obj`, which has no `item_1_title` key, returning nil. + # Output is empty. + # + # This test asserts the INTENDED behavior (output should be the + # snippet-local title). It FAILS today. It should pass once the + # rewriter's transform is corrected to preserve scope-chain semantics + # across `{% render 'snippet', self: ... %}` boundaries (or, less + # likely, once SelfDrop's lookup precedence is changed in + # find_variable). + # + # Failure message reads: + # Expected: "[Cookware Set]" + # Actual: "[]" + # which directly says "the snippet's template-local item_1_title was + # not found via self[name] when self: was bound on render". + def test_rewritten_self_lookup_with_self_named_param_loses_local_assign + assert_template_result( + EXPECTED_OUTPUT, + "{% render 'snippet', self: my_obj %}", + { 'my_obj' => { 'unrelated_key' => 'foo' } }, + partials: { 'snippet' => REWRITTEN_SNIPPET }, + error_mode: :strict2, + ) + end + + # Pins the prohibition that motivates the rewriter migration: + # bare-bracket access must raise at parse time in :strict2. Documents + # WHY the rewriter rewrites `[name]` to `self[name]` in the first + # place. Passing today; serves as a guard against accidental + # regression of PR #2060's strict2 enforcement. + def test_original_bare_bracket_lookup_raises_in_strict2 + error = assert_raises(Liquid::SyntaxError) do + Liquid::Template.parse(ORIGINAL_SNIPPET, error_mode: :strict2) + end + assert_match( + /Bare bracket access is not allowed\. Use self\['\.\.\.'\] instead/, + error.message, + ) + end + + # Coverage extension: the bug is not a one-off of the empty-string-built + # variable name. Confirm `self[name]` still misses when `name` is sourced + # directly from the forloop index (no string concatenation), so a future + # rewriter fix cannot accidentally pass tests by special-casing + # constructed strings. + # + # `forloop.index` is a number; we cast to string via `| append: ''` to + # form `item_1_title` in a different way. Same expected failure: empty + # output today, should be `[Cookware Set]` once fixed. + def test_rewritten_self_lookup_with_forloop_constructed_key_loses_local_assign + snippet = <<~LIQUID + {%- liquid + assign item_1_title = 'Cookware Set' + -%} + {%- for i in (1..1) -%} + {%- assign suffix = forloop.index | append: '_title' -%} + {%- assign name = 'item_' | append: suffix -%} + {%- assign title = self[name] -%} + [{{ title }}] + {%- endfor -%} + LIQUID + + assert_template_result( + EXPECTED_OUTPUT, + "{% render 'snippet', self: my_obj %}", + { 'my_obj' => { 'unrelated_key' => 'foo' } }, + partials: { 'snippet' => snippet }, + error_mode: :strict2, + ) + end + + # If it fails: Inner snippet's SelfDrop saw outer bound self OR outer locals; + # isolation broken. + def test_nested_render_each_level_resolves_its_own_local_via_bound_self + snippet_a = <<~LIQUID + {%- assign label_a = 'A_local' -%} + {%- assign key_a = 'label_a' -%} + A=[{{ self[key_a] }}]{% render 'b', self: obj_b %} + LIQUID + snippet_b = <<~LIQUID + {%- assign label_b = 'B_local' -%} + {%- assign key_b = 'label_b' -%} + B=[{{ self[key_b] }}] + LIQUID + parent = "{% render 'a', self: obj_a %}" + assigns = { + 'obj_a' => { 'unrelated_a' => 'xa' }, + 'obj_b' => { 'unrelated_b' => 'xb' }, + } + assert_template_result( + "A=[A_local]B=[B_local]\n\n", + parent, + assigns, + partials: { 'a' => snippet_a, 'b' => snippet_b }, + error_mode: :strict2, + ) + end + + # If it fails: Bound self leaked across `new_isolated_subcontext` boundary; + # SelfDrop carries state across subcontexts. + def test_nested_render_inner_without_self_walks_only_inner_scope + snippet_a = <<~LIQUID + {%- assign label_a = 'A_local' -%} + A=[{{ self['label_a'] }}]{% render 'b' %} + LIQUID + snippet_b = <<~LIQUID + {%- assign label_b = 'B_local' -%} + {%- assign key_b = 'label_b' -%} + B=[{{ self[key_b] }}] + LIQUID + parent = "{% render 'a', self: obj_a %}" + assigns = { 'obj_a' => { 'label_b' => 'LEAK_FROM_OBJ_A' } } + assert_template_result( + "A=[A_local]B=[B_local]\n\n", + parent, + assigns, + partials: { 'a' => snippet_a, 'b' => snippet_b }, + error_mode: :strict2, + ) + end + + # If it fails: Specific segment in concatenated output names the broken layer + # (top-level, snippet_a local, snippet_a bound, snippet_b local, snippet_b + # bound). + def test_full_chain_top_level_plus_nested_renders_with_mixed_self_binding + snippet_a = <<~LIQUID + {%- assign a_local = 'A!' -%} + {%- assign a_key = 'a_local' -%} + [a:{{ self[a_key] }}|reg:{{ regular_var }}|bound:{{ self['shared'] }}]{% render 'b', self: obj_b %} + LIQUID + snippet_b = <<~LIQUID + {%- assign b_local = 'B!' -%} + {%- assign b_key = 'b_local' -%} + [b:{{ self[b_key] }}|bound:{{ self['only_in_b'] }}] + LIQUID + template = <<~LIQUID + {%- assign top_key = 'top_var' -%} + top:{{ self[top_key] }}|lit:LITERAL|{% render 'a', self: obj_a, regular_var: 'REG' %} + LIQUID + assigns = { + 'top_var' => 'TOP!', + 'obj_a' => { 'shared' => 'SHARED_A' }, + 'obj_b' => { 'only_in_b' => 'B_BOUND', 'shared' => 'SHARED_B_NOT_USED' }, + } + expected = "top:TOP!|lit:LITERAL|[a:A!|reg:REG|bound:SHARED_A][b:B!|bound:B_BOUND]\n\n\n" + assert_template_result( + expected, + template, + assigns, + partials: { 'a' => snippet_a, 'b' => snippet_b }, + error_mode: :strict2, + ) + end + + # If it fails: Lookup precedence flipped from bound-first to scope-first; + # section C invariant lost. + def test_bound_self_key_hit_returns_bound_value_not_scope_value + snippet = <<~LIQUID + {%- assign shared = 'SCOPE_VALUE' -%} + [{{ self['shared'] }}] + LIQUID + assert_template_result( + "[BOUND_VALUE]\n", + "{% render 'snippet', self: my_obj %}", + { 'my_obj' => { 'shared' => 'BOUND_VALUE' } }, + partials: { 'snippet' => snippet }, + error_mode: :strict2, + ) + end +end