-
-
Notifications
You must be signed in to change notification settings - Fork 240
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Only allow access to view contexts own properties #254
Comments
Since liquidjs is expected to behave the same way as Shopify/liquid, it's by design not to escape values (i.e. It means the author of templates is responsible for escaping values wherever it's untrusted. On the otherway, it's possible to provide a LiquidJS option to enable default escaping. I'll keep this issue open for more discussions. |
Thanks for keeping the ticket open. The issue is more around sandboxing rather than escaping values. For example, escaping a value using the Another library that implements this style of sandboxing is https://github.com/NightlyCommit/twing |
I think you are misunderstanding each other. What @samuelthompson189 is talking about is RCE (remote code execution), not XSS (unless you are using liquid in the browser). You can access the Function constructor via I'm not sure what @samuelthompson189 means with sandboxing. What I think liquidjs needs to do is only allow access to own properties https://github.com/harttle/liquidjs/search?q=hasOwnProperty&type=code vs https://github.com/twigjs/twig.js/search?q=hasOwnProperty&type=code Edit: but I'm not sure if that's actually how twig does it twigjs/twig.js#32 twigjs/twig.js#204 |
@Prinzhorn Yes, thank you. You are absolutely correct. My terminology could have been better. Sorry about that! |
@samuelthompson189 I think your title is still a little sensational. So far there is no RCE. All we can do is access properties that arguably shouldn't be accessible. I also think GitHub issues are not the place to report security vulnerabilities. In the future you might want to check if there is a security policy. If not, try to contact the maintainers through other channels. You can also report issues with modules via https://hackerone.com/nodejs-ecosystem?type=team (your report in the current form would probably be rejected because your example doesn't show any vulnerability) @harttle maybe you can add a security policy to this repo so people know who and how to contact |
@Prinzhorn Thanks, I will take your advice next time and use the channel's linked above. 🙌 |
Thank you @Prinzhorn, I've added a security policy for subsequent reports. So it seems the question is: "is reading inherited properties safe?" As mentioned in twigjs/twig.js#32, it's useful to allow inheritance and twig did make it happen via twigjs/twig.js@2ca6349. The properties of scope objects is intended to be read arbitrarily. And it's not executed, just read. If I understand correctly, it'll be a vulnerability only when something like |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Templates are not sandboxed and should not be trusted when compiling templates from users, for example, in a CMS. This is very problematic and exposes XXS attacks into a node environment.
I have created an example here and compared it with twig which does sandbox the template context properly. https://codesandbox.io/s/bold-hofstadter-1p03t?file=/src/index.js
The text was updated successfully, but these errors were encountered: