From 28557a3f96ed1471ffcd9005fa6041f01dfbe5c9 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Mon, 6 Dec 2021 17:58:43 +0000 Subject: [PATCH] Remove the pre-execution validation in string templates to double performance and prevent JS binding issues when mutating context --- .../src/helpers/javascript.js | 9 +++- packages/string-templates/src/index.js | 46 ++++++------------ packages/string-templates/src/utilities.js | 7 ++- .../string-templates/test/helpers.spec.js | 47 +++++++++++-------- .../worker/src/api/routes/tests/email.spec.js | 6 +-- 5 files changed, 58 insertions(+), 57 deletions(-) diff --git a/packages/string-templates/src/helpers/javascript.js b/packages/string-templates/src/helpers/javascript.js index 42e8a1508a..9231283e89 100644 --- a/packages/string-templates/src/helpers/javascript.js +++ b/packages/string-templates/src/helpers/javascript.js @@ -1,4 +1,5 @@ const { atob } = require("../utilities") +const { cloneDeep } = require("lodash/fp") // The method of executing JS scripts depends on the bundle being built. // This setter is used in the entrypoint (either index.cjs or index.mjs). @@ -38,8 +39,12 @@ module.exports.processJS = (handlebars, context) => { // This is required to allow the final `return` statement to be valid. const js = `function run(){${atob(handlebars)}};run();` - // Our $ context function gets a value from context - const sandboxContext = { $: path => getContextValue(path, context) } + // Our $ context function gets a value from context. + // We clone the context to avoid mutation in the binding affecting real + // app context. + const sandboxContext = { + $: path => getContextValue(path, cloneDeep(context)), + } // Create a sandbox with out context and run the JS return runJS(js, sandboxContext) diff --git a/packages/string-templates/src/index.js b/packages/string-templates/src/index.js index 2523b763ba..820b8da290 100644 --- a/packages/string-templates/src/index.js +++ b/packages/string-templates/src/index.js @@ -1,12 +1,13 @@ const handlebars = require("handlebars") const { registerAll } = require("./helpers/index") const processors = require("./processors") -const { removeHandlebarsStatements, atob, btoa } = require("./utilities") +const { atob, btoa } = require("./utilities") const manifest = require("../manifest.json") const hbsInstance = handlebars.create() registerAll(hbsInstance) const hbsInstanceNoHelpers = handlebars.create() +const defaultOpts = { noHelpers: false } /** * utility function to check if the object is valid @@ -28,11 +29,7 @@ function testObject(object) { * @param {object} opts optional - specify some options for processing. * @returns {Promise} The structure input, as fully updated as possible. */ -module.exports.processObject = async ( - object, - context, - opts = { noHelpers: false } -) => { +module.exports.processObject = async (object, context, opts) => { testObject(object) for (let key of Object.keys(object || {})) { if (object[key] != null) { @@ -63,11 +60,7 @@ module.exports.processObject = async ( * @param {object} opts optional - specify some options for processing. * @returns {Promise} The enriched string, all templates should have been replaced if they can be. */ -module.exports.processString = async ( - string, - context, - opts = { noHelpers: false } -) => { +module.exports.processString = async (string, context, opts) => { // TODO: carry out any async calls before carrying out async call return module.exports.processStringSync(string, context, opts) } @@ -81,11 +74,7 @@ module.exports.processString = async ( * @param {object} opts optional - specify some options for processing. * @returns {object|array} The structure input, as fully updated as possible. */ -module.exports.processObjectSync = ( - object, - context, - opts = { noHelpers: false } -) => { +module.exports.processObjectSync = (object, context, opts) => { testObject(object) for (let key of Object.keys(object || {})) { let val = object[key] @@ -106,26 +95,20 @@ module.exports.processObjectSync = ( * @param {object} opts optional - specify some options for processing. * @returns {string} The enriched string, all templates should have been replaced if they can be. */ -module.exports.processStringSync = ( - string, - context, - opts = { noHelpers: false } -) => { - if (!exports.isValid(string)) { - return string - } - // take a copy of input incase error +module.exports.processStringSync = (string, context, opts) => { + opts = { ...defaultOpts, ...opts } + + // take a copy of input in case of error const input = string if (typeof string !== "string") { throw "Cannot process non-string types." } try { - const noHelpers = opts && opts.noHelpers // finalising adds a helper, can't do this with no helpers - const shouldFinalise = !noHelpers + const shouldFinalise = !opts.noHelpers string = processors.preprocess(string, shouldFinalise) // this does not throw an error when template can't be fulfilled, have to try correct beforehand - const instance = noHelpers ? hbsInstanceNoHelpers : hbsInstance + const instance = opts.noHelpers ? hbsInstanceNoHelpers : hbsInstance const template = instance.compile(string, { strict: false, }) @@ -136,7 +119,7 @@ module.exports.processStringSync = ( }) ) } catch (err) { - return removeHandlebarsStatements(input) + return input } } @@ -155,7 +138,8 @@ module.exports.makePropSafe = property => { * @param opts optional - specify some options for processing. * @returns {boolean} Whether or not the input string is valid. */ -module.exports.isValid = (string, opts = { noHelpers: false }) => { +module.exports.isValid = (string, opts) => { + opts = { ...defaultOpts, ...opts } const validCases = [ "string", "number", @@ -169,7 +153,7 @@ module.exports.isValid = (string, opts = { noHelpers: false }) => { // don't really need a real context to check if its valid const context = {} try { - const instance = opts && opts.noHelpers ? hbsInstanceNoHelpers : hbsInstance + const instance = opts.noHelpers ? hbsInstanceNoHelpers : hbsInstance instance.compile(processors.preprocess(string, false))(context) return true } catch (err) { diff --git a/packages/string-templates/src/utilities.js b/packages/string-templates/src/utilities.js index 50d770c6ea..645aca78ba 100644 --- a/packages/string-templates/src/utilities.js +++ b/packages/string-templates/src/utilities.js @@ -10,7 +10,10 @@ module.exports.swapStrings = (string, start, length, swap) => { return string.slice(0, start) + swap + string.slice(start + length) } -module.exports.removeHandlebarsStatements = string => { +module.exports.removeHandlebarsStatements = ( + string, + replacement = "Invalid binding" +) => { let regexp = new RegExp(exports.FIND_HBS_REGEX) let matches = string.match(regexp) if (matches == null) { @@ -18,7 +21,7 @@ module.exports.removeHandlebarsStatements = string => { } for (let match of matches) { const idx = string.indexOf(match) - string = exports.swapStrings(string, idx, match.length, "Invalid Binding") + string = exports.swapStrings(string, idx, match.length, replacement) } return string } diff --git a/packages/string-templates/test/helpers.spec.js b/packages/string-templates/test/helpers.spec.js index 2cf9310638..b4179475fb 100644 --- a/packages/string-templates/test/helpers.spec.js +++ b/packages/string-templates/test/helpers.spec.js @@ -13,10 +13,14 @@ describe("test the custom helpers we have applied", () => { describe("test that it can run without helpers", () => { it("should be able to run without helpers", async () => { - const output = await processString("{{ avg 1 1 1 }}", {}, { noHelpers: true }) + const output = await processString( + "{{ avg 1 1 1 }}", + {}, + { noHelpers: true } + ) const valid = await processString("{{ avg 1 1 1 }}", {}) expect(valid).toBe("1") - expect(output).toBe("Invalid Binding") + expect(output).toBe("{{ avg 1 1 1 }}") }) }) @@ -185,17 +189,22 @@ describe("test the date helpers", () => { it("should test the timezone capabilities", async () => { const date = new Date(1611577535000) - const output = await processString("{{ date time 'HH-mm-ss Z' 'America/New_York' }}", { - time: date.toUTCString(), - }) - const formatted = new dayjs(date).tz("America/New_York").format("HH-mm-ss Z") + const output = await processString( + "{{ date time 'HH-mm-ss Z' 'America/New_York' }}", + { + time: date.toUTCString(), + } + ) + const formatted = new dayjs(date) + .tz("America/New_York") + .format("HH-mm-ss Z") expect(output).toBe(formatted) }) it("should guess the users timezone when not specified", async () => { const date = new Date() const output = await processString("{{ date time 'Z' }}", { - time: date.toUTCString() + time: date.toUTCString(), }) const timezone = dayjs.tz.guess() const offset = new dayjs(date).tz(timezone).format("Z") @@ -307,12 +316,12 @@ describe("test the comparison helpers", () => { describe("Test the object/array helper", () => { it("should allow plucking from an array of objects", async () => { const context = { - items: [ - { price: 20 }, - { price: 30 }, - ] + items: [{ price: 20 }, { price: 30 }], } - const output = await processString("{{ literal ( sum ( pluck items 'price' ) ) }}", context) + const output = await processString( + "{{ literal ( sum ( pluck items 'price' ) ) }}", + context + ) expect(output).toBe(50) }) @@ -442,15 +451,15 @@ describe("Cover a few complex use cases", () => { it("should only invalidate a single string in an object", async () => { const input = { - dataProvider:"{{ literal [c670254c9e74e40518ee5becff53aa5be] }}", - theme:"spectrum--lightest", - showAutoColumns:false, - quiet:true, - size:"spectrum--medium", - rowCount:8, + dataProvider: "{{ literal [c670254c9e74e40518ee5becff53aa5be] }}", + theme: "spectrum--lightest", + showAutoColumns: false, + quiet: true, + size: "spectrum--medium", + rowCount: 8, } const output = await processObject(input, tableJson) - expect(output.dataProvider).not.toBe("Invalid Binding") + expect(output.dataProvider).not.toBe("Invalid binding") }) it("should be able to handle external ids", async () => { diff --git a/packages/worker/src/api/routes/tests/email.spec.js b/packages/worker/src/api/routes/tests/email.spec.js index c8c93658f7..a66857249a 100644 --- a/packages/worker/src/api/routes/tests/email.spec.js +++ b/packages/worker/src/api/routes/tests/email.spec.js @@ -8,7 +8,7 @@ jest.mock("nodemailer") const nodemailer = require("nodemailer") nodemailer.createTransport.mockReturnValue({ sendMail: sendMailMock, - verify: jest.fn() + verify: jest.fn(), }) describe("/api/global/email", () => { @@ -39,6 +39,6 @@ describe("/api/global/email", () => { expect(sendMailMock).toHaveBeenCalled() const emailCall = sendMailMock.mock.calls[0][0] expect(emailCall.subject).toBe("Hello!") - expect(emailCall.html).not.toContain("Invalid Binding") + expect(emailCall.html).not.toContain("Invalid binding") }) -}) \ No newline at end of file +})