Stacks-Editor icon indicating copy to clipboard operation
Stacks-Editor copied to clipboard

refactor: Remove liquid templates

Open abovedave opened this issue 1 year ago • 7 comments

I noticed when working on #352 that the Liquid decency is outdated and flagged in a security issue. This PR refactors the preview site templates to use a simpler approach as we're not using any of the features Liquid provides.

abovedave avatar Oct 01 '24 12:10 abovedave

Thanks @abovedave for the PR. I am all up for simplifying but consider that we have recently introduced liquid templates for mocking Core's layout in new Free Refills services. I don't see our usage of liquid going away any time soon.

Perhaps there is another webpack loader we can use instead of removing the liquid templates all together? Where can I read more about the vulnerabilities of that loader?

Thank you

giamir avatar Oct 01 '24 12:10 giamir

Ah I see! I could just swap out the new Function() with the actual Liquid package and skip the unmaintained loader if we really want it? It just seemed uncessary to use liquid just for a simple layout-view setup.

This was the error:

# npm audit report

liquidjs  <10.0.0
Severity: moderate
liquidjs may leak properties of a prototype - https://github.com/advisories/GHSA-45rm-2893-5f49
No fix available
node_modules/liquidjs
  liquidjs-loader  *
  Depends on vulnerable versions of liquidjs
  node_modules/liquidjs-loader

abovedave avatar Oct 01 '24 12:10 abovedave

It just seemed uncessary to use liquid just for a simple layout-view set

@abovedave I would like to talk to Dan before removing Liquid all together from this repo. If the new Function() approach can buy us some time and fix the vulnerability I would prefer it. Thanks 🙏

giamir avatar Oct 01 '24 13:10 giamir

Deploy Preview for stacks-editor ready!

Name Link
Latest commit 68c162cb17613ea77f96dbfeff1475d531bd1538
Latest deploy log https://app.netlify.com/sites/stacks-editor/deploys/66fbe8db3a4930000889de51
Deploy Preview https://deploy-preview-353--stacks-editor.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Oct 01 '24 13:10 netlify[bot]

I haven't reviewed this PR yet (expect a review later today), but I'm 100% ok with dropping liquid in this repo. The reason I introduced it was because I wanted a simple shared layout for the docs and liquid syntax was already used in Stacks's docs. The use of liquid in this repo is probably overkill, but it worked ok and stayed out of the way. If it is causing issues now, then let's ditch it.

b-kelly avatar Oct 03 '24 15:10 b-kelly

@b-kelly If we are going to keep a templating language of sort, I would prefer to stay consistent with what we use in Stacks docs. We have just started to use liquid also for some free refills mock layout stuff (see WIP). Perhaps it is just a matter of swapping the existing loader with one that does not have vulnerabilities. I haven't looked in details but maybe this one could be an option.

giamir avatar Oct 04 '24 07:10 giamir

@giamir I'm ok with adding back in a special loader or templating language, but I think it's overkill for our use case. 100% of the code that does the "templating" in this case is just a single line:

htmlLayout.replace("<%=content%>", content)

So... we really don't need a full blown templating solution for this. Imo, we can revisit in the future if we ever need a more full-featured solution.

b-kelly avatar Oct 04 '24 15:10 b-kelly