-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Preventing file-system access via include/layouts #131
Comments
I think restrict file system access within Another thing to keep in mind is that, the browser version is implemented as AJAX. The new |
Interesting, I hadn't noticed the browser fs implementation at https://github.com/harttle/liquidjs/blob/bbebb5515c7efe03c43ae7eaa25ecd9b8a113467/src/fs/browser.ts. I see that the implementation is wired at build time here: Line 53 in bef2909
Would it be useful to also allow overriding the |
That rollup config is intended to generate out-of-the-box bundles for both node.js and browser. Another possible solution is to provide an |
🎉 This issue has been resolved in version 8.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
const engine = new Liquid({
fs: {}
}) Looks like a good enough solution Or if you want to get fancy you can override the resolve method
|
Context
We are building a Node.js back-end application that sends emails to end-users and we would like to use
liquidjs
to render email templates. The application provides default email templates out of the box, but we also allow tenants to customize the templates.In order to support this use case, we'd like to restrict the use of the
{% include ... %}
and{% layout ... %}
tags: we should not allow reading arbitrary files from the file system.For example, a tenant would be able to use includes in a custom template like this:
That would render the package.json file inside the email. Note that the
root
option is set to a sub-directory of the project likemyproject/templates
.Potential solutions
We found various approaches to address this concern but we would like to validate them with you.
Restrict
root
One way to address this concern would be to keep allowing file-system access, but set the
root
option to an empty directory. But at the same time,liquidjs
should not allow relative paths OUTSIDE of this root (../
should not be allowed).This would require a change to this library.
Override the
include
andlayout
tagsIn the related issue #20, it is suggested to override the
include
andlayout
tags by registering alternate implementations. We could provide implementations that return an empty string or throw an exception.One drawback of this approach is that tags are currently registered statically. See:
liquidjs/src/template/tag/tag.ts
Line 16 in bbebb55
That means that ALL instances of the
liquidjs
engine will apply the overridden tags. Unfortunately, this is problematic for us: we want to have instances of the engine that allow fs access and others that don't allow them (basically, default email templates allow them and custom email templates don't allow them).Patch the
Liquid.getTemplate()
methodBoth the
include
andlayout
tags' current implementation are calling theLiquid.getTemplate()
method to read from disk. As a matter of fact, all file-system access is confined to this method. It might be more secure to patch this method by a custom implementation (returning an empty string or throwing an exception). This might be more future-proof if this library starts adding more tags that access the file-system. On the other hand, patching this method breaks theLiquid.renderFile()
method, but we don't use this method.If this approach sounds like a good idea, could we consider providing abstractions to this library that allow extending this behavior in a more structured approach? For example, there's the
liquid-node
library that supports registering custom "File System" objects: https://github.com/liquid-lang/liquid-node/blob/7f5076526adcef13ad03b11dfe69b33e9011731b/lib/liquid/engine.js#L76-L82Adding a new option to
liquidjs
If our use case is shared by other users of this lib, we might want to add a new option in
liquidjs
to disable file system access (something likedisableFs
?).Any guidance with our use case is much appreciated. We'd be glad to contribute PRs if valuable.
Thanks for maintaining this library! ❤️
The text was updated successfully, but these errors were encountered: