From bb40962e4eb5299515ac96c269c8ede5509ca9db Mon Sep 17 00:00:00 2001 From: Josh Faigan Date: Thu, 30 Apr 2026 15:22:49 -0400 Subject: [PATCH] fix(render): preserve scope chain when self: bound on render tag The render tag wrote `self:` attributes directly into @scopes[0] under key 'self', which shadowed the SelfDrop. `self[var]` lookups inside the snippet then did strict key access on the bound object instead of walking the scope chain, resolving to nil for any key not in the object. SelfDrop now holds an optional `bound_self`; `[]` and `key?` consult it first via duck typing (matches lib/liquid/variable_lookup.rb), then fall through to the scope-chain walk on miss. Render#render_tag routes attribute key Expression::SELF to inner_context.self_drop.bound_self= instead of the generic scope write. Lookup order is bound-first; existing self: users' hits stay intact, previously-nil misses now resolve via fallthrough. PR #2060 introduced the SelfDrop and bare-bracket prohibition but had no coverage for the {% render 'snippet', self: obj %} interaction. Adds 8 tests including two-deep nested renders with leak detection and a composite chain combining renders, top-level self[var], and regular variables. Discovered while investigating SFR strict-parser migration parity diffs. --- lib/liquid/context.rb | 6 +- lib/liquid/self_drop.rb | 23 ++- lib/liquid/tags/render.rb | 7 +- test/integration/self_drop_render_test.rb | 234 ++++++++++++++++++++++ 4 files changed, 266 insertions(+), 4 deletions(-) create mode 100644 test/integration/self_drop_render_test.rb 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