Templates that use `assign` / `capture` tags are thread-unsafe

[UlyssesZh]

require 'liquid'
Liquid::Template.register_filter Module.new { def s(input) = sleep(input/10.0) && input }
template = Liquid::Template.parse '{% assign u = v %}{{ u | s }}{{ u }}'
3.times.map { |i| Thread.new { print template.render! 'v' => i } }.each &:join

Expected output: 001122; Actual output: 021222.

Why I found this: https://github.com/jekyll/jekyll/issues/9485#issuecomment-1797873978

[rouralberto]

This actually would be super interesting to solve! +100000

[ianks]

I think this happens because certain state for a Liquid::Context will be re-used when a hash passed in to Liquid::Template#render. If you want to avoid that behavior, it is best to explicitly create a new Liquid::Context for each render invocation, i.e.:

template.render!(Liquid::Context.new("v" => i))

[ioquatix]

Liquid::Template does not look thread-safe, or even shareable on the same thread between requests. Re-using Liquid::Template instances can leak state between calls to render, e.g. template.freeze.render! fails.

As per the above discussion, when a hash is supplied...

      when Hash
        Context.new([args.shift, assigns], instance_assigns, registers, @rethrow_errors, @resource_limits)

...the context is created using self.assigns, which is a lazy-initialized (not thread safe) per-template cache of assignments. So naturally, this state leaks. An even more simple example:

template = Liquid::Template.parse '{% increment u %}'

> template.render!
=> "0"
> template.render!
=> "1"
> template.render!

As we can see, subsequent calls to render! reuse the assigned variables. The suggestion to use a context does appear to work:

context = Liquid::Context.new

> template.render! context
=> "0"
> template.render!
=> "5"
> template.render! context
=> "1"

However, I think this kind of default mutable behaviour is extremely risky, because without understanding the details of the implementation, it's easy to get wrong and the impact of "getting it wrong" is likely to be a security issue (e.g. leaking customer data between template renders). I would highly recommend that the render call does not mutate the template instance. In other words, this should work correctly:

template.freeze.render!(context)

The example in the readme is likely to be a security issue for any non-trivial template and probably gives a false impression about the safety of Liquid::Template. Specifically because it uses @template which implies some kind of instance variable which could have a life-time spanning several requests. Reusing state like this can and does lead to security issues, even on single threaded applications.

I would strongly advise to remove this mutable behaviour from Liquid::Template, and the solution of using Liquid::Context is a reasonable one, all things considered.

If there are issues adopting a more secure approach, I suggest you have users explicitly opt into the legacy behaviour, e.g. Liquid::Template.new(mutable: true). Also, lazy initialization of @registers, @assigns and so on is also not thread safe. Therefore, there is no way to share an instance of Liquid::Template safely between threads. If that is intentional, I'd strongly consider enforcing this, e.g. raise ... unless @thread == Thread.current in render.

Given what I've seen in the implementation, I wouldn't be surprised if it was possible to leverage this mutable design to leak data between requests in applications that use Liquid::Template if there is any kind of reuse of template instances. Maybe using a context is sufficient, but at the code level, the mutable by default design is likely to invite these kinds of issues, i.e. it's not what I'd consider "defensive programming".

Suggestions (in no particular order, and not all are required):

• Require the user to always provide a context.
• Allow assigns and other variables to be set by default, but don't allow them to be mutated by render.
• Prevent usage across threads.
• Ensure that frozen templates can render.
• Freeze templates by default after loading them.
• Don't use instance variables to control behaviour and capture results of render! and render.
• Clearly document all assumptions, and make the "easy way" the "correct way" (including the examples in the readme).

[ur5us]

At the moment, this is only 1 anecdote as I can’t provide a demo app (yet) to replicate the issue so take it with a grain of salt: in combination with https://github.com/bensheldon/good_job, a multi-threaded ActiveJob back-end, we’ve had emails with another person’s contents/context going out to the wrong person.