Remove the pre-execution validation in string templates to double performance and prevent JS binding issues when mutating context

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
+    // Our $ context function gets a value from context.
-    const sandboxContext = { $: path => getContextValue(path, 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)

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<object|array>} The structure input, as fully updated as possible.
  */
-module.exports.processObject = async (
+module.exports.processObject = async (object, context, opts) => {
-  object,
-  context,
-  opts = { noHelpers: false }
-) => {
   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<string>} The enriched string, all templates should have been replaced if they can be.
  */
-module.exports.processString = async (
+module.exports.processString = async (string, context, opts) => {
-  string,
-  context,
-  opts = { noHelpers: false }
-) => {
   // 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 = (
+module.exports.processObjectSync = (object, context, opts) => {
-  object,
-  context,
-  opts = { noHelpers: false }
-) => {
   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 = (
+module.exports.processStringSync = (string, context, opts) => {
-  string,
+  opts = { ...defaultOpts, ...opts }
-  context,
+
-  opts = { noHelpers: false }
+  // take a copy of input in case of error
-) => {
-  if (!exports.isValid(string)) {
-    return string
-  }
-  // take a copy of input incase 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) {

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
 }

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' }}", {
+    const output = await processString(
-      time: date.toUTCString(),
+      "{{ date time 'HH-mm-ss Z' 'America/New_York' }}",
-    })
+      {
-    const formatted = new dayjs(date).tz("America/New_York").format("HH-mm-ss Z")
+        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: [
+      items: [{ price: 20 }, { price: 30 }],
-        { 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] }}",
+      dataProvider: "{{ literal [c670254c9e74e40518ee5becff53aa5be] }}",
-      theme:"spectrum--lightest",
+      theme: "spectrum--lightest",
-      showAutoColumns:false,
+      showAutoColumns: false,
-      quiet:true,
+      quiet: true,
-      size:"spectrum--medium",
+      size: "spectrum--medium",
-      rowCount:8,
+      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 () => {

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")
   })
 })