Merge branch 'master' into allow-calculation-fields-to-hide-required-…

…fields

charts/budibase/templates/app-service-deployment.yaml

@@ -184,6 +184,10 @@ spec:
         - name: NODE_DEBUG
           value: {{ .Values.services.apps.nodeDebug | quote }}
         {{ end }}
+        {{ if .Values.services.apps.xssSafeMode }}
+        - name: XSS_SAFE_MODE
+          value: {{ .Values.services.apps.xssSafeMode | quote }}
+        {{ end }}
         {{ if .Values.globals.datadogApmEnabled }}
         - name: DD_LOGS_INJECTION
           value: {{ .Values.globals.datadogApmEnabled | quote }}

lerna.json

@@ -1,6 +1,6 @@
 {
   "$schema": "node_modules/lerna/schemas/lerna-schema.json",
-  "version": "2.32.11",
+  "version": "2.32.12",
   "npmClient": "yarn",
   "packages": [
     "packages/*",

packages/server/src/api/routes/tests/viewV2.spec.ts

@@ -571,6 +571,53 @@ describe.each([
         expect(sum.calculationType).toEqual(CalculationType.SUM)
         expect(sum.field).toEqual("Price")
       })
+
+      it("cannot create a calculation view with more than 5 aggregations", async () => {
+        await config.api.viewV2.create(
+          {
+            tableId: table._id!,
+            name: generator.guid(),
+            schema: {
+              sum: {
+                visible: true,
+                calculationType: CalculationType.SUM,
+                field: "Price",
+              },
+              count: {
+                visible: true,
+                calculationType: CalculationType.COUNT,
+                field: "Price",
+              },
+              min: {
+                visible: true,
+                calculationType: CalculationType.MIN,
+                field: "Price",
+              },
+              max: {
+                visible: true,
+                calculationType: CalculationType.MAX,
+                field: "Price",
+              },
+              avg: {
+                visible: true,
+                calculationType: CalculationType.AVG,
+                field: "Price",
+              },
+              sum2: {
+                visible: true,
+                calculationType: CalculationType.SUM,
+                field: "Price",
+              },
+            },
+          },
+          {
+            status: 400,
+            body: {
+              message: "Calculation views can only have a maximum of 5 fields",
+            },
+          }
+        )
+      })
     })
 
     describe("update", () => {

packages/server/src/environment.ts

@@ -83,6 +83,7 @@ const environment = {
   PLUGINS_DIR: process.env.PLUGINS_DIR || DEFAULTS.PLUGINS_DIR,
   MAX_IMPORT_SIZE_MB: process.env.MAX_IMPORT_SIZE_MB,
   SESSION_EXPIRY_SECONDS: process.env.SESSION_EXPIRY_SECONDS,
+  XSS_SAFE_MODE: process.env.XSS_SAFE_MODE,
   // SQL
   SQL_MAX_ROWS: process.env.SQL_MAX_ROWS,
   SQL_LOGGING_ENABLE: process.env.SQL_LOGGING_ENABLE,

packages/server/src/jsRunner/tests/jsRunner.spec.ts

@@ -8,7 +8,6 @@ import { init } from ".."
 import TestConfiguration from "../../tests/utilities/TestConfiguration"
 
 const DATE = "2021-01-21T12:00:00"
-
 tk.freeze(DATE)
 
 describe("jsRunner (using isolated-vm)", () => {
@@ -48,6 +47,33 @@ describe("jsRunner (using isolated-vm)", () => {
     ).toEqual("ReferenceError: process is not defined")
   })
 
+  it("should not allow the context to be mutated", async () => {
+    const context = { array: [1] }
+    const result = await processJS(
+      `
+        const array = $("array");
+        array.push(2);
+        return array[1]
+      `,
+      context
+    )
+    expect(result).toEqual(2)
+    expect(context.array).toEqual([1])
+  })
+
+  it("should copy values whenever returning them from $", async () => {
+    const context = { array: [1] }
+    const result = await processJS(
+      `
+        $("array").push(2);
+        return $("array")[1];
+      `,
+      context
+    )
+    expect(result).toEqual(undefined)
+    expect(context.array).toEqual([1])
+  })
+
   describe("helpers", () => {
     runJsHelpersTests({
       funcWrap: (func: any) => config.doInContext(config.getAppId(), func),

packages/server/src/sdk/app/rows/tests/utils.spec.ts

@@ -8,6 +8,7 @@ import {
 import { generateTableID } from "../../../../db/utils"
 import { validate } from "../utils"
 import { generator } from "@budibase/backend-core/tests"
+import { withEnv } from "../../../../environment"
 
 describe("validate", () => {
   const hour = () => generator.hour().toString().padStart(2, "0")
@@ -332,4 +333,46 @@ describe("validate", () => {
       })
     })
   })
+
+  describe("XSS Safe mode", () => {
+    const getTable = (): Table => ({
+      type: "table",
+      _id: generateTableID(),
+      name: "table",
+      sourceId: INTERNAL_TABLE_SOURCE_ID,
+      sourceType: TableSourceType.INTERNAL,
+      schema: {
+        text: {
+          name: "sometext",
+          type: FieldType.STRING,
+        },
+      },
+    })
+    it.each([
+      "SELECT * FROM users WHERE username = 'admin' --",
+      "SELECT * FROM users WHERE id = 1; DROP TABLE users;",
+      "1' OR '1' = '1",
+      "' OR 'a' = 'a",
+      "<script>alert('XSS');</script>",
+      '"><img src=x onerror=alert(1)>',
+      "</script><script>alert('test')</script>",
+      "<div onmouseover=\"alert('XSS')\">Hover over me!</div>",
+      "'; EXEC sp_msforeachtable 'DROP TABLE ?'; --",
+      "{alert('Injected')}",
+      "UNION SELECT * FROM users",
+      "INSERT INTO users (username, password) VALUES ('admin', 'password')",
+      "/* This is a comment */ SELECT * FROM users",
+      '<iframe src="http://malicious-site.com"></iframe>',
+    ])("test potentially unsafe input: %s", async input => {
+      await withEnv({ XSS_SAFE_MODE: "1" }, async () => {
+        const table = getTable()
+        const row = { text: input }
+        const output = await validate({ source: table, row })
+        expect(output.valid).toBe(false)
+        expect(output.errors).toStrictEqual({
+          text: ["Input not sanitised - potentially vulnerable to XSS"],
+        })
+      })
+    })
+  })
 })

packages/server/src/sdk/app/rows/utils.ts

@@ -22,6 +22,7 @@ import { extractViewInfoFromID, isRelationshipColumn } from "../../../db/utils"
 import { isSQL } from "../../../integrations/utils"
 import { docIds, sql } from "@budibase/backend-core"
 import { getTableFromSource } from "../../../api/controllers/row/utils"
+import env from "../../../environment"
 
 const SQL_CLIENT_SOURCE_MAP: Record<SourceName, SqlClient | undefined> = {
   [SourceName.POSTGRES]: SqlClient.POSTGRES,
@@ -43,6 +44,9 @@ const SQL_CLIENT_SOURCE_MAP: Record<SourceName, SqlClient | undefined> = {
   [SourceName.BUDIBASE]: undefined,
 }
 
+const XSS_INPUT_REGEX =
+  /[<>;"'(){}]|--|\/\*|\*\/|union|select|insert|drop|delete|update|exec|script/i
+
 export function getSQLClient(datasource: Datasource): SqlClient {
   if (!isSQL(datasource)) {
     throw new Error("Cannot get SQL Client for non-SQL datasource")
@@ -222,6 +226,15 @@ export async function validate({
     } else {
       res = validateJs.single(row[fieldName], constraints)
     }
+
+    if (env.XSS_SAFE_MODE && typeof row[fieldName] === "string") {
+      if (XSS_INPUT_REGEX.test(row[fieldName])) {
+        errors[fieldName] = [
+          "Input not sanitised - potentially vulnerable to XSS",
+        ]
+      }
+    }
+
     if (res) errors[fieldName] = res
   }
   return { valid: Object.keys(errors).length === 0, errors }

packages/server/src/sdk/app/views/index.ts

@@ -64,6 +64,14 @@ async function guardCalculationViewSchema(
   view: Omit<ViewV2, "id" | "version">
 ) {
   const calculationFields = helpers.views.calculationFields(view)
+
+  if (Object.keys(calculationFields).length > 5) {
+    throw new HTTPError(
+      "Calculation views can only have a maximum of 5 fields",
+      400
+    )
+  }
+
   for (const calculationFieldName of Object.keys(calculationFields)) {
     const schema = calculationFields[calculationFieldName]
     const isCount = schema.calculationType === CalculationType.COUNT

packages/string-templates/src/helpers/javascript.ts

@@ -1,13 +1,14 @@
-import { atob, isJSAllowed } from "../utilities"
-import cloneDeep from "lodash/fp/cloneDeep"
+import { atob, isBackendService, isJSAllowed } from "../utilities"
 import { LITERAL_MARKER } from "../helpers/constants"
 import { getJsHelperList } from "./list"
 import { iifeWrapper } from "../iife"
 import { JsTimeoutError, UserScriptError } from "../errors"
+import { cloneDeep } from "lodash/fp"
 
 // The method of executing JS scripts depends on the bundle being built.
 // This setter is used in the entrypoint (either index.js or index.mjs).
-let runJS: ((js: string, context: any) => any) | undefined = undefined
+let runJS: ((js: string, context: Record<string, any>) => any) | undefined =
+  undefined
 export const setJSRunner = (runner: typeof runJS) => (runJS = runner)
 
 export const removeJSRunner = () => {
@@ -31,9 +32,19 @@ const removeSquareBrackets = (value: string) => {
   return value
 }
 
+const isReservedKey = (key: string) =>
+  key === "snippets" ||
+  key === "helpers" ||
+  key.startsWith("snippets.") ||
+  key.startsWith("helpers.")
+
 // Our context getter function provided to JS code as $.
 // Extracts a value from context.
 const getContextValue = (path: string, context: any) => {
+  // We populate `snippets` ourselves, don't allow access to it.
+  if (isReservedKey(path)) {
+    return undefined
+  }
   const literalStringRegex = /^(["'`]).*\1$/
   let data = context
   // check if it's a literal string - just return path if its quoted
@@ -46,6 +57,7 @@ const getContextValue = (path: string, context: any) => {
     }
     data = data[removeSquareBrackets(key)]
   })
+
   return data
 }
 
@@ -67,10 +79,23 @@ export function processJS(handlebars: string, context: any) {
       snippetMap[snippet.name] = snippet.code
     }
 
-    // Our $ context function gets a value from context.
-    // We clone the context to avoid mutation in the binding affecting real
-    // app context.
-    const clonedContext = cloneDeep({ ...context, snippets: null })
+    let clonedContext: Record<string, any>
+    if (isBackendService()) {
+      // On the backned, values are copied across the isolated-vm boundary and
+      // so we don't need to do any cloning here. This does create a fundamental
+      // difference in how JS executes on the frontend vs the backend, e.g.
+      // consider this snippet:
+      //
+      //   $("array").push(2)
+      //   return $("array")[1]
+      //
+      // With the context of `{ array: [1] }`, the backend will return
+      // `undefined` whereas the frontend will return `2`. We should fix this.
+      clonedContext = context
+    } else {
+      clonedContext = cloneDeep(context)
+    }
+
     const sandboxContext = {
       $: (path: string) => getContextValue(path, clonedContext),
       helpers: getJsHelperList(),

packages/string-templates/src/index.ts

@@ -1,4 +1,4 @@
-import { Context, createContext, runInNewContext } from "vm"
+import { createContext, runInNewContext } from "vm"
 import { create, TemplateDelegate } from "handlebars"
 import { registerAll, registerMinimum } from "./helpers/index"
 import { postprocess, preprocess } from "./processors"
@@ -455,21 +455,14 @@ export function convertToJS(hbs: string) {
 
 export { JsTimeoutError, UserScriptError } from "./errors"
 
-export function defaultJSSetup() {
-  if (!isBackendService()) {
-    /**
-     * Use polyfilled vm to run JS scripts in a browser Env
-     */
-    setJSRunner((js: string, context: Context) => {
-      context = {
-        ...context,
-        alert: undefined,
-        setInterval: undefined,
-        setTimeout: undefined,
-      }
-      createContext(context)
+export function browserJSSetup() {
+  /**
+   * Use polyfilled vm to run JS scripts in a browser Env
+   */
+  setJSRunner((js: string, context: Record<string, any>) => {
+    createContext(context)
 
-      const wrappedJs = `
+    const wrappedJs = `
         result = {
           result: null,
           error: null,
@@ -484,12 +477,17 @@ export function defaultJSSetup() {
         result;
       `
 
-      const result = runInNewContext(wrappedJs, context, { timeout: 1000 })
-      if (result.error) {
-        throw new UserScriptError(result.error)
-      }
-      return result.result
-    })
+    const result = runInNewContext(wrappedJs, context, { timeout: 1000 })
+    if (result.error) {
+      throw new UserScriptError(result.error)
+    }
+    return result.result
+  })
+}
+
+export function defaultJSSetup() {
+  if (!isBackendService()) {
+    browserJSSetup()
   } else {
     removeJSRunner()
   }

packages/string-templates/src/utilities.ts

@@ -4,7 +4,14 @@ export const FIND_HBS_REGEX = /{{([^{].*?)}}/g
 export const FIND_ANY_HBS_REGEX = /{?{{([^{].*?)}}}?/g
 export const FIND_TRIPLE_HBS_REGEX = /{{{([^{].*?)}}}/g
 
+const isJest = () => typeof jest !== "undefined"
+
 export const isBackendService = () => {
+  // We consider the tests for string-templates to be frontend, so that they
+  // test the frontend JS functionality.
+  if (isJest()) {
+    return false
+  }
   return typeof window === "undefined"
 }