Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Environment & server variables are available in all templates #54

Open
dsazup opened this issue Aug 23, 2018 · 6 comments
Open

Environment & server variables are available in all templates #54

dsazup opened this issue Aug 23, 2018 · 6 comments

Comments

@dsazup
Copy link

dsazup commented Aug 23, 2018

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
Copy link
Contributor

sanmai commented Jan 30, 2019

Care to provide an example for the issue? Thanks.

@dsazup
Copy link
Author

dsazup commented Feb 4, 2019

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
Copy link
Contributor

sanmai commented Feb 4, 2019

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
Copy link
Author

dsazup commented Feb 4, 2019

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
Copy link
Contributor

sanmai commented Feb 4, 2019

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
Copy link
Author

dsazup commented Feb 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants