Shopify / liquid

Liquid markup language. Safe, customer facing template language for flexible web apps.
https://shopify.github.io/liquid/
MIT License
11.03k stars 1.38k forks source link

Can’t use symbols as hash keys for local variables in Liquid::Template#render #82

Closed codyrobbins closed 12 years ago

codyrobbins commented 12 years ago

Here’s a minimal test case:

Liquid::Template.parse('Hi, {{name}}!').render(:name => 'Cody')  #=> 'Hi, !'
Liquid::Template.parse('Hi, {{name}}!').render('name' => 'Cody') #=> 'Hi, Cody!'

This is especially annoying because as a result you can't use Ruby 1.9's new hash syntax:

Liquid::Template.parse('Hi, {{name}}!').render(name: 'Cody')  #=> 'Hi, !'

This problem occurs for me using both 2.3.0 and 2.2.2.

titanous commented 12 years ago

This isn't really a bug, using symbols was never supported. I'd be happy to accept a patch as long as it doesn't have a negative performance impact.

codyrobbins commented 12 years ago

Oh, my bad. I was looking at the wiki page explaining drops and it uses symbols and it wasn’t working for me. Looking at the other examples I guess that’s just a typo—I just updated it.

tobi commented 12 years ago

That's by design. We didn't want to take the performance hit that comes from that.

-- Tobi CEO Shopify

Written with thumbs.

On Nov 17, 2011, at 11:18 AM, Cody Robbinsreply@reply.github.com wrote:

Here’s a minimal test case:

Liquid::Template.parse('Hi, {{name}}!').render(:name => 'Cody') #=> 'Hi, !' Liquid::Template.parse('Hi, {{name}}!').render('name' => 'Cody') #=> 'Hi, Cody!'

This problem occurs for me using both 2.3.0 and 2.2.2.


Reply to this email directly or view it on GitHub: https://github.com/Shopify/liquid/issues/82

arg commented 10 years ago

Maybe it's time to reconsider this decision? We use liquid in our app and these hash.stringify_keys and key.to_s don't look good in code. And yes - they hit the performance anyways :)

sheerun commented 10 years ago

The option to opt-in would be awesome.

envygeeks commented 9 years ago

I just wanted to chime in and say that that keeping symbols out of the picture is a good idea because it allows us to pass private data into Liquid and not have it show up inside of the liquid layout. For example we use symbols when we are passing the controller into the context for the tags to use view_context to do work without having to add more objects to the stack and over-complicate the stack but we don't want that data to show up in the render. It's a cheap way to have private variables in Liquid contexts.

ddevault commented 8 years ago

Bump. I just ran into this and it's pretty lame.

parkr commented 8 years ago

@tobi, would it be absurd to enable some sort of "warning mode" that tells the user "any non-string keys will be inaccessible in your templates"?

ddevault commented 8 years ago

fwiw:

            <a href="#object-{{ endpoint[1].resource }}">
                <!--
                    liquid is stupid sometimes
                    this code SHOULD be:
                        { site.data.objects[endpoint[1].resource].name }
                    but liquid does not let you use hashes with variables
                -->
                {% for o in site.data.objects %}
                {% if o[0] == endpoint[1].resource %}
                    {{ o[1].name }}
                {% endif %}
                {% endfor %}
                objects
                <!-- end liquid is stupid sometimes -->
            </a>
tobi commented 8 years ago

It's a good idea to include this. I'd hate to run a check on this during normal operation though because its unnecessary in production On Mon, Nov 16, 2015 at 2:37 PM Drew DeVault notifications@github.com wrote:

fwiw:

        <a href="#object-{{ endpoint[1].resource }}">
            <!--
                liquid is stupid sometimes
                this code SHOULD be:
                    { site.data.objects[endpoint[1].resource].name }
                but liquid does not let you use hashes with variables
            -->
            {% for o in site.data.objects %}
            {% if o[0] == endpoint[1].resource %}
                {{ o[1].name }}
            {% endif %}
            {% endfor %}
            objects
            <!-- end liquid is stupid sometimes -->
        </a>

— Reply to this email directly or view it on GitHub https://github.com/Shopify/liquid/issues/82#issuecomment-157146153.

  • tobi CEO Shopify
pushrax commented 8 years ago

@SirCmpwn I think that should work in Liquid 3+ with the strict syntax mode enabled. Issue #473

ddevault commented 8 years ago

Thanks @pushrax. I'll come back to this issue once GitHub Pages updates Liquid (we'll probably all be retired by then, though).

Bilge commented 7 years ago

This is such utter nonsense. I spent hours banging my head against the desk just to realize a hash's symbol properties can never be output by this crippled software, but far be it for the software to give any indication of that fact.

brentdodell commented 6 years ago

I just ran into this as well, and it was pretty difficult to troubleshoot. We use symbols everywhere, so this breaks our style.

That's by design. We didn't want to take the performance hit that comes from that.

Is that still true? https://blog.gorbikoff.com/ruby-hashkey-showdown-symbol-vs-string/

It looks like rubocop prefers symbols these days as well. https://github.com/rubocop-hq/ruby-style-guide#symbols-as-keys

dylanahsmith commented 6 years ago

Liquid has traditionally used strings for variables names because they could be user defined and before Ruby 2.2 symbols weren't garbage collected. So in the past we would have had at best a memory leak and at worst a DoS attack vector from using symbols internally to represent variable names.

If liquid uses strings internally but accepted symbols from the application code, then this library would need to convert the symbols to strings using Symbol#to_s. In other words, we wouldn't be benefiting from using symbols as Hash keys and would be allocating new string objects for every call to Symbol#to_s.

Ruby 2.1 isn't maintained upstream anymore, so we should consider dropping support for it and switching to using symbols internally in liquid to represent variables. For backwards compatibility, we could convert String variables names passed in from the application code, but String#to_sym doesn't have the performance concerns that Symbol#to_s has.

So not only can we make this library more pleasant to use by supporting the Ruby 1.9 hash syntax, but we should also be able to improve performance by using Symbol hash keys.

Bilge commented 6 years ago

Liquid has traditionally used strings for variables names because they could be user defined before Ruby 2.2 symbols weren't garbage collected. So in the past we would have had at best a memory leak and at worst a DoS attack vector from using symbols internally to represent variable names.

You're implying every Ruby program using symbols since forever has been vulnerable to DoS attacks because of an integral language feature. That sounds like complete nonsense. Either your Ruby program is short-lived, in which case this is a non-issue, or it's a long-term process, in which case the symbols are immutable and shared, so it's still a non-issue. I don't know where you get the idea that you can DoS any Ruby program because it defines symbols, unless this is a bad joke.

fw42 commented 6 years ago

You're implying every Ruby program using symbols since forever has been vulnerable to DoS attacks because of an integral language feature. That sounds like complete nonsense

That's not what he's saying. The problem is (was) not using symbols per se but converting arbitrary user-supplied input strings to symbols.

https://bugs.ruby-lang.org/issues/7791 http://brakemanscanner.org/docs/warning_types/denial_of_service/index.html

fw42 commented 6 years ago

I think maybe the piece that's missing in your understanding is that (in Shopify's use-case) we allow people (i.e. attackers) to send us Liquid source code (i.e. attacker-controlled input) which we will then run for them (on our servers, using our memory). That's a very different scenario than, say, a Jekyll site running locally (or whatever your use-case might be).

Nobody said symbols are always evil. But in a scenario like ours (people send us strings which we will then convert to symbols in our server's memory), they are (used to be) a real problem.

envygeeks commented 6 years ago

Feels odd that a company would refer to their customers as "attackers", yes they should be referred to in the threat model, but outright calling them an attacker seems wrong, as well, your assessment is outdated.

  1. Ruby has frozen string literals to help with memory now
  2. Ruby has a symbol GC now to help with the old symbol attack that could happen
    • This has been the case since 2.2.
    • 2.2 is 4 years old.

If you aren't worried about strings, being worried about symbols is odd... So unless you happen to be creating methods for every symbol or string they send in that list of variables... which is highly unoptimized anyways, and also create a ton of symbols (that can't be collected,) and at that point would make this entire debate about symbols negligible... the argument is moot at this point because they will be garbage collected like regular strings will, and if you aren't worried about strings on modern Ruby, you shouldn't be worried about symbols, because when they are no longer used, or useful, they are garbage collected. Unless they are

  1. Representing a method
  2. Representing a true constant like a class, module, or CONSTANT
  3. Used on a regular basis
dylanahsmith commented 6 years ago

Feels odd that a company would refer to their customers as "attackers"

I think he just mixed up "e.g." with "i.e."

as well, your assessment is outdated.

You seem to have not read previous comments carefully, because they were talking about the historic reason why we didn't use symbols. My last comment (https://github.com/Shopify/liquid/issues/82#issuecomment-395163211) already came to the conclusion that these historic reasons aren't applicable anymore.

envygeeks commented 6 years ago

I didn't really read all the comments, just came across the last one and decided to reply, so sorry if I replied out of context and exacerbated a debate negatively, that was not my intention. On a side note, I still don't necessarily support symbols because I like the undocumented pretty much private transfer of variables by not supporting symbols.