refactor: Remove liquid templates #353

abovedave · 2024-10-01T12:15:15Z

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.

giamir · 2024-10-01T12:47:13Z

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

abovedave · 2024-10-01T12:53:52Z

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

giamir · 2024-10-01T13:02:54Z

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 🙏

netlify · 2024-10-01T13:03:02Z

✅ Deploy Preview for stacks-editor ready!

Name	Link
🔨 Latest commit	`68c162c`
🔍 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... Use your smartphone camera to open QR code link.

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

b-kelly · 2024-10-03T15:27:43Z

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

Other than the use of new Function, this change looks good to me. See my previous comment on OK'ing dropping liquid syntax entirely from this repo.

I'll pull this PR down today and find a good alternative to the new Function call.

b-kelly · 2024-10-03T15:40:32Z

config/webpack.dev.js

@@ -3,6 +3,8 @@ const common = require("./webpack.common.js");
 const HtmlWebpackPlugin = require("html-webpack-plugin");
 const fs = require("fs");

+const htmlLayout = fs.readFileSync("./site/layout.html");


optional: Move this into the exported function (after line 8) so it only runs when invoked.

In this circumstance it changes basically nothing, but its generally good form to avoid side effects on import when possible.

b-kelly · 2024-10-03T15:47:03Z

package.json

@@ -67,7 +67,6 @@
        "husky": "^9.0.11",
        "jest": "^29.7.0",
        "jest-environment-jsdom": "^29.7.0",
-        "liquidjs-loader": "^1.0.1",


b-kelly · 2024-10-03T15:48:05Z

site/layout.html

@@ -127,11 +127,7 @@
            <label id="a11y-editor-label" class="v-visible-sr">
                Stacks-Editor demo implementation
            </label>
-            {% block content %}


I'll miss having this default example for reference, but... not so much that I'd block this change from going in.

lgtm

b-kelly · 2024-10-03T15:48:54Z

config/webpack.dev.js

+                    );
+
+                    return new HtmlWebpackPlugin({
+                        templateContent: new Function(


The use of new Function here is a large code smell to me. I'll pull down the PR here shortly and see if I can't whip up an alternative approach.

b-kelly

I added a few comments with suggested changes. I'd push directly to the branch, but doing so to a fork is a pain in the rear. The change comments are prefixed with _loader-update_. The gist of the changes are as follows:

readd html-loader, since it handles recompiling on file change for us (which templateContent does not, along with a host of other potentially useful utilities (such as preprocessing, see next point)
add a preprocessor entry to wrap the files with our layout (inspired by the Templating section in the loader docs)

b-kelly · 2024-10-03T16:59:00Z

site/layout.html

-            <textarea id="content" name="content" class="d-none"></textarea>
-            <div id="example-1"></div>
-            {% endblock %}
+            ${content}


loader-update: I changed this to use lodash templating syntax, which is what html-webpack-template uses by default. The syntax doesn't actually matter, but it'll at least be internally consistent if we decide to use any of the other supported templating features later.

Suggested change

${content}

<%=content%>

b-kelly · 2024-10-03T16:59:53Z

config/webpack.dev.js

+                ...fs.readdirSync("./site/views").map((f) => {
+                    const htmlView = fs.readFileSync(
+                        `./site/views/${f}`,
+                        "utf8"
+                    );
+
+                    return new HtmlWebpackPlugin({
+                        templateContent: new Function(
+                            ["content"],
+                            `return \`${htmlLayout}\`;`
+                        )(htmlView),
+                        filename: f,
+                    });
+                }),


loader-update: Revert this change

Suggested change

...fs.readdirSync("./site/views").map((f) => {

const htmlView = fs.readFileSync(

`./site/views/${f}`,

"utf8"

);

return new HtmlWebpackPlugin({

templateContent: new Function(

["content"],

`return \`${htmlLayout}\`;`

)(htmlView),

filename: f,

});

}),

...fs.readdirSync("./site/views").map(

(f) =>

new HtmlWebpackPlugin({

template: "./site/views/" + f,

filename: f,

})

),

b-kelly · 2024-10-03T17:01:17Z

config/webpack.dev.js

@@ -17,22 +19,6 @@ module.exports = (env, argv) => {
            },
            mode: emulateProdServer ? "production" : "development",
            devtool: emulateProdServer ? false : "inline-source-map",
-            module: {


loader-update: Partially revert this change, modifying html-loader to do the work for us (GitHub doesn't allow suggestions on deleted lines, so you'll have to fix this manually)

module: { rules: [ { test: /\.html$/, use: [ { loader: "html-loader", options: { // wrap the content of the html file in the layout.html file preprocessor: (content) => htmlLayout.replace( "<%=content%>", content ), }, }, ], }, ], },

b-kelly · 2024-10-03T17:09:10Z

config/webpack.dev.js

@@ -3,6 +3,8 @@ const common = require("./webpack.common.js");
 const HtmlWebpackPlugin = require("html-webpack-plugin");
 const fs = require("fs");

+const htmlLayout = fs.readFileSync("./site/layout.html");


loader-update: Set the "utf-8" flag so the file is read as a string (instead of Buffer)

Suggested change

const htmlLayout = fs.readFileSync("./site/layout.html");

const htmlLayout = fs.readFileSync("./site/layout.html", "utf-8");

giamir · 2024-10-04T07:10:08Z

@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.

b-kelly · 2024-10-04T15:06:56Z

@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.

abovedave added 2 commits October 1, 2024 13:05

remove liquidjs-loader

370b48d

implement simple html templates

abe4726

abovedave requested review from dancormier and b-kelly October 1, 2024 12:16

remove String()

68c162c

b-kelly requested changes Oct 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Remove liquid templates #353

refactor: Remove liquid templates #353

abovedave commented Oct 1, 2024

giamir commented Oct 1, 2024

abovedave commented Oct 1, 2024

giamir commented Oct 1, 2024

netlify bot commented Oct 1, 2024

b-kelly commented Oct 3, 2024

b-kelly left a comment

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

b-kelly left a comment

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

b-kelly Oct 3, 2024

giamir commented Oct 4, 2024

b-kelly commented Oct 4, 2024

	const htmlLayout = fs.readFileSync("./site/layout.html");
	const htmlLayout = fs.readFileSync("./site/layout.html", "utf-8");

refactor: Remove liquid templates #353

Are you sure you want to change the base?

refactor: Remove liquid templates #353

Conversation

abovedave commented Oct 1, 2024

giamir commented Oct 1, 2024

abovedave commented Oct 1, 2024

giamir commented Oct 1, 2024

netlify bot commented Oct 1, 2024

✅ Deploy Preview for stacks-editor ready!

b-kelly commented Oct 3, 2024

b-kelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-kelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giamir commented Oct 4, 2024

b-kelly commented Oct 4, 2024