bkiers / Liqp

An ANTLR based 'Liquid Template' parser and rendering engine.
MIT License
165 stars 94 forks source link

Refactor Template parser to TemplateParser; fix global scope issue #241

Closed kohlschuetter closed 1 year ago

kohlschuetter commented 1 year ago

Currently, there are several places in the API that may have unsafe side-effects on otherwise unrelated code that shares Liqp running in the same VM. This is due to global scope that allows modifications around Insertions and Filters, which are stored as static HashMaps shared across all callers.

In the worst case this could be seen as a security issue because unrelated code may interfere and override a certain insertion (tag or block), leading to information disclosure or data manipulation.

The bug may also just trigger a ConcurrentModificationException when these modifications occur at an inopportune time.

This patch introduces a clean solution while providing backwards compatibility for existing code. A new API class, TemplateParser, is introduced, combining all settings-related configurations in one immutable place.

Insertions and Filters are now maintained in immutable data structures, and adding them via the global methods will not affect an existing TemplateParser instance at all. In the previous cde, these were copied multiple times from Lists to HashMaps during Template construction, which is now also no longer necessary.

The Flavor enum is enhanced in such way that it can be used to quickly access TemplateParsers/ParserSettings with default settings.

The old Template API is marked with @Deprecated statments, encouraging users to adapt the new one.

msangel commented 1 year ago

This PR is huge

msangel commented 1 year ago

Honestly, I don't see any issues with having global scope...

msangel commented 1 year ago

Backward compatibility also will be totally broken as now users have to pass an exact instance of their parser if they want to make some modifications or simply reuse the same settings.

kohlschuetter commented 1 year ago

Thanks for looking into the review, @msangel.

Backwards compatibility should be preserved. All tests still pass with code unchanged.

Global scope is dangerous, for example, if you have two apps/classes sharing the same liqp library, and both have different Insertions under the same name. Depending on timing, you may get the one or the other result.

Let's say you have a simple parser app as a transitive dependency somewhere in your classpath that you don't know about. That app decides to overwrite the "include" tag with its own code. Your code may do the same, but in a different way, maybe checking for some other paths in the file system, or adding access control or whatever.

Since the other app modified the same global scope, your app is effectively broken.

That's what this patch fixes.

msangel commented 1 year ago

One things this PR will make better: split it into as small parts as possible. One functionality per PR. Right now it’s one and with many code improvements that can be split. Some of them are really good(like lazy init to break static init recursion), some are decorative(like getters instead direct final fields access) and some are quite discussable(marking static methods deprecated and run out of static context). If first two can be merger right now without any doubt, the “static context” question will require deeper testing and attention of @bkiers . Let’s simplify our life and make one step at a time! Thank you!

bkiers commented 1 year ago

... will require deeper testing and attention of @bkiers . Let’s simplify our life and make one step at a time! Thank you!

Thank you both! I will have a closer look at the changes and suggestions this week.

msangel commented 1 year ago

I am working on resolving conflicts. Future recommendation: no need to commit changes in imports if those are only about reordering and replacing wildcard ones with explicit ones.


upd: since the source PR in another repo, I simply added a new commit with changes. This cannot be rebased, but can be merged via "Create a merge commit"

kohlschuetter commented 1 year ago

@msangel Any chance we can get this PR merged this week? I'd like to move on since I already got a few other bug fixes lined up that depend on this one.

msangel commented 1 year ago

Done