harrydeluxe / php-liquid

A PHP port of Ruby's Liquid Templates
http://www.delacap.com/artikel/Liquid-Templates/
MIT License
239 stars 119 forks source link

Environment & server variables are available in all templates #54

Open dsazup opened 6 years ago

dsazup commented 6 years ago

Because decrement tag uses $_SERVER to store values, it is possible to get any variable from it. We store some environment variables in $_SERVER and user could do {{ DB_PASSWORD }} or any other server variable and he would be able to see that value. Is this not considered a security issue ?Should decrement really touch $_SERVER? could it not store data in registers or assigns?

sanmai commented 5 years ago

Care to provide an example for the issue? Thanks.

dsazup commented 5 years ago

Sure,

lets say we have this code in a file named test.php

<?php

require __DIR__ . '/vendor/autoload.php';

$liquid = new \Liquid\Template();
$liquid->parse('{{ MY_VARIABLE }}');

echo $liquid->render();

Now run this in your terminal. This is how we expose environment variables in our system, for example this could be DB_PASSWORD or other sensitive data such as GITHUB_API_KEY etc.

export MY_VARIABLE="this is an environment variable"

run php test.php and your should see this is an environment variable text. Notice we have not passed any data into render method, however this still works and shows the variable we exported in the terminal session.

sanmai commented 5 years ago

This looks very similar to this issue. I'll look into it. It is hard to tell why there's a need to pass $_SERVER in the context, but I guess we can safely pass a some subset of it.

dsazup commented 5 years ago

Yes looks very similar. The last time I looked into this was quite a while ago, but as far as I can remember this is because increment and decrement tags are using $_SERVER for storing data. And I guess this is because template class is not a singleton so it wants to retain the data in every instance?

We do not use these tags so I have just removed them from our fork (and the environments property), but it would be nice to fix this in the upstream.

sanmai commented 5 years ago

I believe they're using only the first element of that array, but later I'll see if anything breaks if I remove $_SERVER from there.

dsazup commented 5 years ago

Ah yes you are right, the exposed variables must come from here then https://github.com/harrydeluxe/php-liquid/blob/master/src/Liquid/Context.php#L210