From 27508d934dc7df489638c686317a894877b1a407 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 10:45:03 +0100 Subject: [PATCH 01/24] Validate that you cannot create a calculation view with more than 5 calculation fields. --- .../src/api/routes/tests/viewV2.spec.ts | 47 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 8 ++++ 2 files changed, 55 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index a1a4475ee5..7ad1be4c1f 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -568,6 +568,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", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 73785edd98..be7259b057 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -64,6 +64,14 @@ async function guardCalculationViewSchema( view: Omit ) { 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 From fb7133e64fe4e6985351f31ea5485ca30f66bf37 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 11:09:29 +0100 Subject: [PATCH 02/24] Validate that there are no duplicate calculations in calculation views. --- .../src/api/routes/tests/viewV2.spec.ts | 103 ++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 20 +++- 2 files changed, 119 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 7ad1be4c1f..97e82f071b 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -615,6 +615,109 @@ describe.each([ } ) }) + + it("cannot create a calculation view with duplicate calculations", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + sum: { + visible: true, + calculationType: CalculationType.SUM, + field: "Price", + }, + sum2: { + visible: true, + calculationType: CalculationType.SUM, + field: "Price", + }, + }, + }, + { + status: 400, + body: { + message: + 'Duplicate calculation on field "Price", calculation type "sum"', + }, + } + ) + }) + + it("finds duplicate counts", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + count: { + visible: true, + calculationType: CalculationType.COUNT, + }, + count2: { + visible: true, + calculationType: CalculationType.COUNT, + }, + }, + }, + { + status: 400, + body: { + message: + 'Duplicate calculation on field "*", calculation type "count"', + }, + } + ) + }) + + it("finds duplicate count distincts", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + count: { + visible: true, + calculationType: CalculationType.COUNT, + distinct: true, + field: "Price", + }, + count2: { + visible: true, + calculationType: CalculationType.COUNT, + distinct: true, + field: "Price", + }, + }, + }, + { + status: 400, + body: { + message: + 'Duplicate calculation on field "Price", calculation type "count"', + }, + } + ) + }) + + it("does not confuse counts and count distincts in the duplicate check", async () => { + await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + count: { + visible: true, + calculationType: CalculationType.COUNT, + }, + count2: { + visible: true, + calculationType: CalculationType.COUNT, + distinct: true, + field: "Price", + }, + }, + }) + }) }) describe("update", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index be7259b057..dcd2c8cc14 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -72,11 +72,23 @@ async function guardCalculationViewSchema( ) } - for (const calculationFieldName of Object.keys(calculationFields)) { - const schema = calculationFields[calculationFieldName] + const seen: Record> = {} + + for (const name of Object.keys(calculationFields)) { + const schema = calculationFields[name] const isCount = schema.calculationType === CalculationType.COUNT const isDistinct = isCount && "distinct" in schema && schema.distinct + const field = isCount && !isDistinct ? "*" : schema.field + if (seen[field]?.[schema.calculationType]) { + throw new HTTPError( + `Duplicate calculation on field "${field}", calculation type "${schema.calculationType}"`, + 400 + ) + } + seen[field] ??= {} as Record + seen[field][schema.calculationType] = true + // Count fields that aren't distinct don't need to reference another field, // so we don't validate it. if (isCount && !isDistinct) { @@ -86,14 +98,14 @@ async function guardCalculationViewSchema( const targetSchema = table.schema[schema.field] if (!targetSchema) { throw new HTTPError( - `Calculation field "${calculationFieldName}" references field "${schema.field}" which does not exist in the table schema`, + `Calculation field "${name}" references field "${schema.field}" which does not exist in the table schema`, 400 ) } if (!isCount && !helpers.schema.isNumeric(targetSchema)) { throw new HTTPError( - `Calculation field "${calculationFieldName}" references field "${schema.field}" which is not a numeric field`, + `Calculation field "${name}" references field "${schema.field}" which is not a numeric field`, 400 ) } From 27578db4b7e0298cd11aa937f7b795252bf8e918 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 09:48:33 +0100 Subject: [PATCH 03/24] Fix SQS error handling. --- packages/backend-core/src/db/couch/DatabaseImpl.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/backend-core/src/db/couch/DatabaseImpl.ts b/packages/backend-core/src/db/couch/DatabaseImpl.ts index 274c1b9e93..2ca52a2dce 100644 --- a/packages/backend-core/src/db/couch/DatabaseImpl.ts +++ b/packages/backend-core/src/db/couch/DatabaseImpl.ts @@ -371,11 +371,15 @@ export class DatabaseImpl implements Database { return this.performCall(() => { return async () => { const response = await directCouchUrlCall(args) - const json = await response.json() - if (response.status > 300) { - throw json + if (response.status >= 300) { + const text = await response.text() + console.error(`SQS error: ${text}`) + throw new CouchDBError( + "error while running SQS query, please try again later", + { name: "sqs_error", status: response.status } + ) } - return json as T + return (await response.json()) as T } }) } From db255119487fff16e1213f1761b8216c7c50730e Mon Sep 17 00:00:00 2001 From: melohagan <101575380+melohagan@users.noreply.github.com> Date: Mon, 7 Oct 2024 11:26:55 +0100 Subject: [PATCH 04/24] Changes relating to adding accountName to Account entity (#14712) * Remove unused code * Typing compromise because Account extends CreateAccount * Update account-portal * Update account-portal --- packages/account-portal | 2 +- .../core/utilities/structures/accounts.ts | 63 +------------------ .../types/src/documents/account/account.ts | 9 +-- 3 files changed, 4 insertions(+), 70 deletions(-) diff --git a/packages/account-portal b/packages/account-portal index 3e24f6293f..8cd052ce82 160000 --- a/packages/account-portal +++ b/packages/account-portal @@ -1 +1 @@ -Subproject commit 3e24f6293ff5ee5f9b42822e001504e3bbf19cc0 +Subproject commit 8cd052ce8288f343812a514d06c5a9459b3ba1a8 diff --git a/packages/backend-core/tests/core/utilities/structures/accounts.ts b/packages/backend-core/tests/core/utilities/structures/accounts.ts index 29453ad60a..daf4965c81 100644 --- a/packages/backend-core/tests/core/utilities/structures/accounts.ts +++ b/packages/backend-core/tests/core/utilities/structures/accounts.ts @@ -6,9 +6,6 @@ import { AccountSSOProviderType, AuthType, CloudAccount, - CreateAccount, - CreatePassswordAccount, - CreateVerifiableSSOAccount, Hosting, SSOAccount, } from "@budibase/types" @@ -19,6 +16,7 @@ export const account = (partial: Partial = {}): Account => { accountId: uuid(), tenantId: generator.word(), email: generator.email({ domain: "example.com" }), + accountName: generator.word(), tenantName: generator.word(), hosting: Hosting.SELF, createdAt: Date.now(), @@ -84,62 +82,3 @@ export function verifiableSsoAccount( thirdPartyProfile: { id: "abc123" }, } } - -export const cloudCreateAccount: CreatePassswordAccount = { - email: "cloud@budibase.com", - tenantId: "cloud", - hosting: Hosting.CLOUD, - authType: AuthType.PASSWORD, - password: "Password123!", - tenantName: "cloud", - name: "Budi Armstrong", - size: "10+", - profession: "Software Engineer", -} - -export const cloudSSOCreateAccount: CreateAccount = { - email: "cloud-sso@budibase.com", - tenantId: "cloud-sso", - hosting: Hosting.CLOUD, - authType: AuthType.SSO, - tenantName: "cloudsso", - name: "Budi Armstrong", - size: "10+", - profession: "Software Engineer", -} - -export const cloudVerifiableSSOCreateAccount: CreateVerifiableSSOAccount = { - email: "cloud-sso@budibase.com", - tenantId: "cloud-sso", - hosting: Hosting.CLOUD, - authType: AuthType.SSO, - tenantName: "cloudsso", - name: "Budi Armstrong", - size: "10+", - profession: "Software Engineer", - provider: AccountSSOProvider.MICROSOFT, - thirdPartyProfile: { id: "abc123" }, -} - -export const selfCreateAccount: CreatePassswordAccount = { - email: "self@budibase.com", - tenantId: "self", - hosting: Hosting.SELF, - authType: AuthType.PASSWORD, - password: "Password123!", - tenantName: "self", - name: "Budi Armstrong", - size: "10+", - profession: "Software Engineer", -} - -export const selfSSOCreateAccount: CreateAccount = { - email: "self-sso@budibase.com", - tenantId: "self-sso", - hosting: Hosting.SELF, - authType: AuthType.SSO, - tenantName: "selfsso", - name: "Budi Armstrong", - size: "10+", - profession: "Software Engineer", -} diff --git a/packages/types/src/documents/account/account.ts b/packages/types/src/documents/account/account.ts index 61792a7f47..c219229889 100644 --- a/packages/types/src/documents/account/account.ts +++ b/packages/types/src/documents/account/account.ts @@ -7,10 +7,9 @@ export interface CreateAccount { tenantId: string hosting: Hosting authType: AuthType + accountName: string // optional fields - for sso based sign ups registrationStep?: string - // profile - tenantName?: string name?: string size?: string profession?: string @@ -20,11 +19,6 @@ export interface CreatePassswordAccount extends CreateAccount { password: string } -export interface CreateVerifiableSSOAccount extends CreateAccount { - provider?: AccountSSOProvider - thirdPartyProfile?: any -} - export const isCreatePasswordAccount = ( account: CreateAccount ): account is CreatePassswordAccount => account.authType === AuthType.PASSWORD @@ -56,6 +50,7 @@ export interface Account extends CreateAccount { providerType?: AccountSSOProviderType quotaUsage?: QuotaUsage offlineLicenseToken?: string + tenantName?: string } export interface PasswordAccount extends Account { From 2c451d494b0f18c5b0faf1049351d82784ad8828 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 12:04:45 +0100 Subject: [PATCH 05/24] Backend/frontend JS parity, deep clone optimisation. --- .../src/jsRunner/tests/jsRunner.spec.ts | 28 +- .../src/helpers/javascript.ts | 30 +- packages/string-templates/src/index.ts | 40 +- packages/string-templates/src/utilities.ts | 7 + .../string-templates/test/javascript.spec.ts | 363 +++++++++++++++++- 5 files changed, 427 insertions(+), 41 deletions(-) diff --git a/packages/server/src/jsRunner/tests/jsRunner.spec.ts b/packages/server/src/jsRunner/tests/jsRunner.spec.ts index c7ab86f014..006df19fa6 100644 --- a/packages/server/src/jsRunner/tests/jsRunner.spec.ts +++ b/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), diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 2f850b0533..4d53fe335d 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/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) => 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,7 +57,15 @@ const getContextValue = (path: string, context: any) => { } data = data[removeSquareBrackets(key)] }) - return data + + // When data is returned from this function in the backend, isolated-vm copies + // it across the vm boundary. We need to replicate that behaviour on the + // frontend to ensure that JS runs the same in both environments. + if (isBackendService()) { + return data + } else { + return cloneDeep(data) + } } // Evaluates JS code against a certain context @@ -70,9 +89,8 @@ export function processJS(handlebars: string, context: any) { // 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 }) const sandboxContext = { - $: (path: string) => getContextValue(path, clonedContext), + $: (path: string) => getContextValue(path, context), helpers: getJsHelperList(), // Proxy to evaluate snippets when running in the browser diff --git a/packages/string-templates/src/index.ts b/packages/string-templates/src/index.ts index c2be63978d..7246bc4942 100644 --- a/packages/string-templates/src/index.ts +++ b/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) => { + 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() } diff --git a/packages/string-templates/src/utilities.ts b/packages/string-templates/src/utilities.ts index 18e0926b96..779bef3735 100644 --- a/packages/string-templates/src/utilities.ts +++ b/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" } diff --git a/packages/string-templates/test/javascript.spec.ts b/packages/string-templates/test/javascript.spec.ts index 99e6ee122a..9f2d493e23 100644 --- a/packages/string-templates/test/javascript.spec.ts +++ b/packages/string-templates/test/javascript.spec.ts @@ -1,7 +1,13 @@ -import vm from "vm" - -import { processStringSync, encodeJSBinding, setJSRunner } from "../src/index" +import { + processStringSync, + encodeJSBinding, + defaultJSSetup, +} from "../src/index" import { UUID_REGEX } from "./constants" +import tk from "timekeeper" + +const DATE = "2021-01-21T12:00:00" +tk.freeze(DATE) const processJS = (js: string, context?: object): any => { return processStringSync(encodeJSBinding(js), context) @@ -9,9 +15,7 @@ const processJS = (js: string, context?: object): any => { describe("Javascript", () => { beforeAll(() => { - setJSRunner((js, context) => { - return vm.runInNewContext(js, context, { timeout: 1000 }) - }) + defaultJSSetup() }) describe("Test the JavaScript helper", () => { @@ -118,8 +122,7 @@ describe("Javascript", () => { }) it("should handle errors", () => { - const output = processJS(`throw "Error"`) - expect(output).toBe("Error while executing JS") + expect(processJS(`throw "Error"`)).toEqual("Error") }) it("should timeout after one second", () => { @@ -127,16 +130,18 @@ describe("Javascript", () => { expect(output).toBe("Timed out while executing JS") }) - it("should prevent access to the process global", () => { - const output = processJS(`return process`) - expect(output).toBe("Error while executing JS") + it("should prevent access to the process global", async () => { + expect(processJS(`return process`)).toEqual( + "ReferenceError: process is not defined" + ) }) }) describe("check JS helpers", () => { it("should error if using the format helper. not helpers.", () => { - const output = processJS(`return helper.toInt(4.3)`) - expect(output).toBe("Error while executing JS") + expect(processJS(`return helper.toInt(4.3)`)).toEqual( + "ReferenceError: helper is not defined" + ) }) it("should be able to use toInt", () => { @@ -156,4 +161,336 @@ describe("Javascript", () => { expect(output).toBe("Custom") }) }) + + describe("mutability", () => { + 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("malice", () => { + it("should not be able to call JS functions", () => { + expect(processJS(`return alert("hello")`)).toEqual( + "ReferenceError: alert is not defined" + ) + + expect(processJS(`return prompt("hello")`)).toEqual( + "ReferenceError: prompt is not defined" + ) + + expect(processJS(`return confirm("hello")`)).toEqual( + "ReferenceError: confirm is not defined" + ) + + expect(processJS(`return setTimeout(() => {}, 1000)`)).toEqual( + "ReferenceError: setTimeout is not defined" + ) + + expect(processJS(`return setInterval(() => {}, 1000)`)).toEqual( + "ReferenceError: setInterval is not defined" + ) + }) + }) + + // the test cases here were extracted from templates/real world examples of JS in Budibase + describe("real test cases from Budicloud", () => { + const context = { + "Unit Value": 2, + Quantity: 1, + } + it("handle test case 1", async () => { + const result = await processJS( + ` + var Gross = $("[Unit Value]") * $("[Quantity]") + return Gross.toFixed(2)`, + context + ) + expect(result).toBeDefined() + expect(result).toBe("2.00") + }) + + it("handle test case 2", async () => { + const todayDate = new Date() + // add a year and a month + todayDate.setMonth(new Date().getMonth() + 1) + todayDate.setFullYear(todayDate.getFullYear() + 1) + const context = { + "Purchase Date": DATE, + today: todayDate.toISOString(), + } + const result = await processJS( + ` + var purchase = new Date($("[Purchase Date]")); + let purchaseyear = purchase.getFullYear(); + let purchasemonth = purchase.getMonth(); + + var today = new Date($("today")); + let todayyear = today.getFullYear(); + let todaymonth = today.getMonth(); + + var age = todayyear - purchaseyear + + if (((todaymonth - purchasemonth) < 6) == true){ + return age + } + `, + context + ) + expect(result).toBeDefined() + expect(result).toBe(1) + }) + + it("should handle test case 3", async () => { + const context = { + Escalate: true, + "Budget ($)": 1100, + } + const result = await processJS( + ` + if ($("[Escalate]") == true) { + if ($("Budget ($)") <= 1000) + {return 2;} + if ($("Budget ($)") > 1000) + {return 3;} + } + else { + if ($("Budget ($)") <= 1000) + {return 1;} + if ($("Budget ($)") > 1000) + if ($("Budget ($)") < 10000) + {return 2;} + else + {return 3} + } + `, + context + ) + expect(result).toBeDefined() + expect(result).toBe(3) + }) + + it("should handle test case 4", async () => { + const context = { + "Time Sheets": ["a", "b"], + } + const result = await processJS( + ` + let hours = 0 + if (($("[Time Sheets]") != null) == true){ + for (i = 0; i < $("[Time Sheets]").length; i++){ + let hoursLogged = "Time Sheets." + i + ".Hours" + hours += $(hoursLogged) + } + return hours + } + if (($("[Time Sheets]") != null) == false){ + return hours + } + `, + context + ) + expect(result).toBeDefined() + expect(result).toBe("0ab") + }) + + it("should handle test case 5", async () => { + const context = { + change: JSON.stringify({ a: 1, primaryDisplay: "a" }), + previous: JSON.stringify({ a: 2, primaryDisplay: "b" }), + } + const result = await processJS( + ` + let change = $("[change]") ? JSON.parse($("[change]")) : {} + let previous = $("[previous]") ? JSON.parse($("[previous]")) : {} + + function simplifyLink(originalKey, value, parent) { + if (Array.isArray(value)) { + if (value.filter(item => Object.keys(item || {}).includes("primaryDisplay")).length > 0) { + parent[originalKey] = value.map(link => link.primaryDisplay) + } + } + } + + for (let entry of Object.entries(change)) { + simplifyLink(entry[0], entry[1], change) + } + for (let entry of Object.entries(previous)) { + simplifyLink(entry[0], entry[1], previous) + } + + let diff = Object.fromEntries(Object.entries(change).filter(([k, v]) => previous[k]?.toString() !== v?.toString())) + + delete diff.audit_change + delete diff.audit_previous + delete diff._id + delete diff._rev + delete diff.tableId + delete diff.audit + + for (let entry of Object.entries(diff)) { + simplifyLink(entry[0], entry[1], diff) + } + + return JSON.stringify(change)?.replaceAll(",\\"", ",\\n\\t\\"").replaceAll("{\\"", "{\\n\\t\\"").replaceAll("}", "\\n}") + `, + context + ) + expect(result).toBe(`{\n\t"a":1,\n\t"primaryDisplay":"a"\n}`) + }) + + it("should handle test case 6", async () => { + const context = { + "Join Date": DATE, + } + const result = await processJS( + ` + var rate = 5; + var today = new Date(); + + // comment + function monthDiff(dateFrom, dateTo) { + return dateTo.getMonth() - dateFrom.getMonth() + + (12 * (dateTo.getFullYear() - dateFrom.getFullYear())) + } + var serviceMonths = monthDiff( new Date($("[Join Date]")), today); + var serviceYears = serviceMonths / 12; + + if (serviceYears >= 1 && serviceYears < 5){ + rate = 10; + } + if (serviceYears >= 5 && serviceYears < 10){ + rate = 15; + } + if (serviceYears >= 10){ + rate = 15; + rate += 0.5 * (Number(serviceYears.toFixed(0)) - 10); + } + return rate; + `, + context + ) + expect(result).toBe(10) + }) + + it("should handle test case 7", async () => { + const context = { + "P I": "Pass", + "PA I": "Pass", + "F I": "Fail", + "V I": "Pass", + } + const result = await processJS( + `if (($("[P I]") == "Pass") == true) + if (($("[ P I]") == "Pass") == true) + if (($("[F I]") == "Pass") == true) + if (($("[V I]") == "Pass") == true) + {return "Pass"} + + if (($("[PA I]") == "Fail") == true) + {return "Fail"} + if (($("[ P I]") == "Fail") == true) + {return "Fail"} + if (($("[F I]") == "Fail") == true) + {return "Fail"} + if (($("[V I]") == "Fail") == true) + {return "Fail"} + + else + {return ""}`, + context + ) + expect(result).toBe("Fail") + }) + + it("should handle test case 8", async () => { + const context = { + "T L": [{ Hours: 10 }], + "B H": 50, + } + const result = await processJS( + `var totalHours = 0; + if (($("[T L]") != null) == true){ + for (let i = 0; i < ($("[T L]").length); i++){ + var individualHours = "T L." + i + ".Hours"; + var hoursNum = Number($(individualHours)); + totalHours += hoursNum; + } + return totalHours.toFixed(2); + } + if (($("[T L]") != null) == false) { + return totalHours.toFixed(2); + } + `, + context + ) + expect(result).toBe("10.00") + }) + + it("should handle test case 9", async () => { + const context = { + "T L": [{ Hours: 10 }], + "B H": 50, + } + const result = await processJS( + `var totalHours = 0; + if (($("[T L]") != null) == true){ + for (let i = 0; i < ($("[T L]").length); i++){ + var individualHours = "T L." + i + ".Hours"; + var hoursNum = Number($(individualHours)); + totalHours += hoursNum; + } + return ($("[B H]") - totalHours).toFixed(2); + } + if (($("[T L]") != null) == false) { + return ($("[B H]") - totalHours).toFixed(2); + }`, + context + ) + expect(result).toBe("40.00") + }) + + it("should handle test case 10", async () => { + const context = { + "F F": [{ "F S": 10 }], + } + const result = await processJS( + `var rating = 0; + + if ($("[F F]") != null){ + for (i = 0; i < $("[F F]").length; i++){ + var individualRating = $("F F." + i + ".F S"); + rating += individualRating; + } + rating = (rating / $("[F F]").length); + } + return rating; + `, + context + ) + expect(result).toBe(10) + }) + }) }) From 55699cfaffd49de3edd5138ff66263549682face Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 12:23:22 +0100 Subject: [PATCH 06/24] Don't clone non-object/non-arrays, use default JS in manifest.spec.ts. --- packages/string-templates/src/helpers/javascript.ts | 6 +++++- packages/string-templates/test/manifest.spec.ts | 8 ++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 4d53fe335d..61fe872103 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -64,7 +64,11 @@ const getContextValue = (path: string, context: any) => { if (isBackendService()) { return data } else { - return cloneDeep(data) + if (typeof data === "object") { + return cloneDeep(data) + } else { + return data + } } } diff --git a/packages/string-templates/test/manifest.spec.ts b/packages/string-templates/test/manifest.spec.ts index de0e3355fd..2d09bc7871 100644 --- a/packages/string-templates/test/manifest.spec.ts +++ b/packages/string-templates/test/manifest.spec.ts @@ -1,5 +1,3 @@ -import vm from "vm" - jest.mock("@budibase/handlebars-helpers/lib/math", () => { const actual = jest.requireActual("@budibase/handlebars-helpers/lib/math") @@ -17,7 +15,7 @@ jest.mock("@budibase/handlebars-helpers/lib/uuid", () => { } }) -import { processString, setJSRunner } from "../src/index" +import { defaultJSSetup, processString } from "../src/index" import tk from "timekeeper" import { getParsedManifest, runJsHelpersTests } from "./utils" @@ -32,9 +30,7 @@ describe("manifest", () => { const manifest = getParsedManifest() beforeAll(() => { - setJSRunner((js, context) => { - return vm.runInNewContext(js, context, { timeout: 1000 }) - }) + defaultJSSetup() }) describe("examples are valid", () => { From c21d8a8131bd3b3a97f411e35ccc1c8bc4ff5d6d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 12:27:17 +0100 Subject: [PATCH 07/24] Remove inaccurate comment. --- packages/string-templates/src/helpers/javascript.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 61fe872103..48db4b9809 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -90,9 +90,6 @@ 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 sandboxContext = { $: (path: string) => getContextValue(path, context), helpers: getJsHelperList(), From deee5e4e05fcdb6f74a2d3812345ae5e051b87fa Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 12:35:05 +0100 Subject: [PATCH 08/24] Put the frontend back to how it was. --- .../src/helpers/javascript.ts | 32 +++++++++++-------- .../string-templates/test/javascript.spec.ts | 13 -------- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/packages/string-templates/src/helpers/javascript.ts b/packages/string-templates/src/helpers/javascript.ts index 48db4b9809..4fb1329196 100644 --- a/packages/string-templates/src/helpers/javascript.ts +++ b/packages/string-templates/src/helpers/javascript.ts @@ -58,18 +58,7 @@ const getContextValue = (path: string, context: any) => { data = data[removeSquareBrackets(key)] }) - // When data is returned from this function in the backend, isolated-vm copies - // it across the vm boundary. We need to replicate that behaviour on the - // frontend to ensure that JS runs the same in both environments. - if (isBackendService()) { - return data - } else { - if (typeof data === "object") { - return cloneDeep(data) - } else { - return data - } - } + return data } // Evaluates JS code against a certain context @@ -90,8 +79,25 @@ export function processJS(handlebars: string, context: any) { snippetMap[snippet.name] = snippet.code } + let clonedContext: Record + 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, context), + $: (path: string) => getContextValue(path, clonedContext), helpers: getJsHelperList(), // Proxy to evaluate snippets when running in the browser diff --git a/packages/string-templates/test/javascript.spec.ts b/packages/string-templates/test/javascript.spec.ts index 9f2d493e23..9134005acb 100644 --- a/packages/string-templates/test/javascript.spec.ts +++ b/packages/string-templates/test/javascript.spec.ts @@ -176,19 +176,6 @@ describe("Javascript", () => { 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("malice", () => { From e6e25fdf943d69e975f025bec9e51dc26227dec1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 14:59:29 +0100 Subject: [PATCH 09/24] Allow calculation views to hide required fields. --- .../src/api/routes/tests/viewV2.spec.ts | 20 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 5238b9b752..22c83419a4 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1024,6 +1024,26 @@ describe.each([ ) }) + it("can add a new group by field that is invisible, even if required on the table", async () => { + view.schema!.name = { visible: false } + await config.api.viewV2.update(view) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(2) + expect(rows).toEqual( + expect.arrayContaining([ + { + country: "USA", + age: 65, + }, + { + country: "UK", + age: 61, + }, + ]) + ) + }) + it("can add a new calculation field", async () => { view.schema!.count = { visible: true, diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 73785edd98..32fd696f20 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -178,7 +178,7 @@ function checkRequiredFields( continue } - if (!viewSchemaField?.visible) { + if (!helpers.views.isCalculationView(view) && !viewSchemaField?.visible) { throw new HTTPError( `You can't hide "${field.name}" because it is a required field.`, 400 @@ -186,6 +186,7 @@ function checkRequiredFields( } if ( + viewSchemaField && helpers.views.isBasicViewField(viewSchemaField) && viewSchemaField.readonly ) { From 672469526e95181280543d646fa302c86fc79fee Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:33:14 +0100 Subject: [PATCH 10/24] Mark calculation views explicitly instead of figuring it out implicitly. --- .../src/api/controllers/view/viewsV2.ts | 2 + .../src/api/routes/tests/viewV2.spec.ts | 64 ++++++++++++++++++- packages/server/src/sdk/app/views/external.ts | 3 + packages/server/src/sdk/app/views/index.ts | 7 ++ packages/server/src/sdk/app/views/internal.ts | 4 ++ packages/shared-core/src/helpers/views.ts | 5 ++ packages/types/src/documents/app/view.ts | 5 ++ 7 files changed, 88 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index c98940ed88..c60ab497cd 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -127,6 +127,7 @@ export async function create(ctx: Ctx) { const parsedView: Omit, "id" | "version"> = { name: view.name, + type: view.type, tableId: view.tableId, query: view.query, queryUI: view.queryUI, @@ -163,6 +164,7 @@ export async function update(ctx: Ctx) { const parsedView: RequiredKeys = { id: view.id, name: view.name, + type: view.type, version: view.version, tableId: view.tableId, query: view.query, diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 5238b9b752..0a8dd2ac70 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -26,6 +26,7 @@ import { BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, + ViewV2Type, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -155,7 +156,7 @@ describe.each([ }) it("can persist views with all fields", async () => { - const newView: Required> = { + const newView: Required> = { name: generator.name(), tableId: table._id!, primaryDisplay: "id", @@ -549,6 +550,7 @@ describe.each([ let view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, @@ -571,6 +573,29 @@ describe.each([ expect(sum.calculationType).toEqual(CalculationType.SUM) expect(sum.field).toEqual("Price") }) + + it("cannot create a view with calculation fields unless it has the right type", async () => { + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + schema: { + sum: { + visible: true, + calculationType: CalculationType.SUM, + field: "Price", + }, + }, + }, + { + status: 400, + body: { + message: + "Calculation fields are not allowed in non-calculation views", + }, + } + ) + }) }) describe("update", () => { @@ -615,7 +640,9 @@ describe.each([ it("can update all fields", async () => { const tableId = table._id! - const updatedData: Required> = { + const updatedData: Required< + Omit + > = { version: view.version, id: view.id, tableId, @@ -830,6 +857,32 @@ describe.each([ ) }) + it("cannot update view type after creation", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + schema: { + id: { visible: true }, + Price: { + visible: true, + }, + }, + }) + + await config.api.viewV2.update( + { + ...view, + type: ViewV2Type.CALCULATION, + }, + { + status: 400, + body: { + message: "Cannot update view type after creation", + }, + } + ) + }) + isInternal && it("updating schema will only validate modified field", async () => { let view = await config.api.viewV2.create({ @@ -902,6 +955,7 @@ describe.each([ view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { country: { visible: true, @@ -2639,6 +2693,7 @@ describe.each([ it("should be able to search by calculations", async () => { const view = await config.api.viewV2.create({ tableId: table._id!, + type: ViewV2Type.CALCULATION, name: generator.guid(), schema: { "Quantity Sum": { @@ -2673,6 +2728,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { quantity: { visible: true, @@ -2711,6 +2767,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { aggregate: { visible: true, @@ -2771,6 +2828,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -2805,6 +2863,7 @@ describe.each([ { tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { count: { visible: true, @@ -2853,6 +2912,7 @@ describe.each([ const view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), + type: ViewV2Type.CALCULATION, schema: { sum: { visible: true, diff --git a/packages/server/src/sdk/app/views/external.ts b/packages/server/src/sdk/app/views/external.ts index 3afd7e9bf9..b69ac0b9eb 100644 --- a/packages/server/src/sdk/app/views/external.ts +++ b/packages/server/src/sdk/app/views/external.ts @@ -70,6 +70,9 @@ export async function update(tableId: string, view: ViewV2): Promise { if (!existingView || !existingView.name) { throw new HTTPError(`View ${view.id} not found in table ${tableId}`, 404) } + if (isV2(existingView) && existingView.type !== view.type) { + throw new HTTPError(`Cannot update view type after creation`, 400) + } delete views[existingView.name] views[view.name] = view diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 73785edd98..e217f16400 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -111,6 +111,13 @@ async function guardViewSchema( if (helpers.views.isCalculationView(view)) { await guardCalculationViewSchema(table, view) + } else { + if (helpers.views.hasCalculationFields(view)) { + throw new HTTPError( + "Calculation fields are not allowed in non-calculation views", + 400 + ) + } } await checkReadonlyFields(table, view) diff --git a/packages/server/src/sdk/app/views/internal.ts b/packages/server/src/sdk/app/views/internal.ts index 19a9f6ab14..96b41bffe8 100644 --- a/packages/server/src/sdk/app/views/internal.ts +++ b/packages/server/src/sdk/app/views/internal.ts @@ -59,6 +59,10 @@ export async function update(tableId: string, view: ViewV2): Promise { throw new HTTPError(`View ${view.id} not found in table ${tableId}`, 404) } + if (isV2(existingView) && existingView.type !== view.type) { + throw new HTTPError(`Cannot update view type after creation`, 400) + } + delete table.views[existingView.name] table.views[view.name] = view await db.put(table) diff --git a/packages/shared-core/src/helpers/views.ts b/packages/shared-core/src/helpers/views.ts index f41c66adc8..0113375adf 100644 --- a/packages/shared-core/src/helpers/views.ts +++ b/packages/shared-core/src/helpers/views.ts @@ -3,6 +3,7 @@ import { ViewCalculationFieldMetadata, ViewFieldMetadata, ViewV2, + ViewV2Type, } from "@budibase/types" import { pickBy } from "lodash" @@ -21,6 +22,10 @@ export function isBasicViewField( type UnsavedViewV2 = Omit export function isCalculationView(view: UnsavedViewV2) { + return view.type === ViewV2Type.CALCULATION +} + +export function hasCalculationFields(view: UnsavedViewV2) { return Object.values(view.schema || {}).some(isCalculationField) } diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index c58852ecea..02c9b23e38 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -79,10 +79,15 @@ export enum CalculationType { MAX = "max", } +export enum ViewV2Type { + CALCULATION = "calculation", +} + export interface ViewV2 { version: 2 id: string name: string + type?: ViewV2Type primaryDisplay?: string tableId: string query?: LegacyFilter[] | SearchFilters From f4b430e27c86b68da9dd541c35471d86318bc258 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:38:18 +0100 Subject: [PATCH 11/24] Remove uiMetadata from ViewV2, it's not needed now we have the type field. --- packages/server/src/api/controllers/view/viewsV2.ts | 2 -- packages/server/src/api/routes/tests/viewV2.spec.ts | 6 ------ packages/types/src/documents/app/view.ts | 1 - 3 files changed, 9 deletions(-) diff --git a/packages/server/src/api/controllers/view/viewsV2.ts b/packages/server/src/api/controllers/view/viewsV2.ts index c60ab497cd..f864df9e9e 100644 --- a/packages/server/src/api/controllers/view/viewsV2.ts +++ b/packages/server/src/api/controllers/view/viewsV2.ts @@ -134,7 +134,6 @@ export async function create(ctx: Ctx) { sort: view.sort, schema, primaryDisplay: view.primaryDisplay, - uiMetadata: view.uiMetadata, } const result = await sdk.views.create(tableId, parsedView) ctx.status = 201 @@ -172,7 +171,6 @@ export async function update(ctx: Ctx) { sort: view.sort, schema, primaryDisplay: view.primaryDisplay, - uiMetadata: view.uiMetadata, } const result = await sdk.views.update(tableId, parsedView) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 0a8dd2ac70..b2b78853ef 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -178,9 +178,6 @@ describe.each([ visible: true, }, }, - uiMetadata: { - foo: "bar", - }, } const res = await config.api.viewV2.create(newView) @@ -670,9 +667,6 @@ describe.each([ readonly: true, }, }, - uiMetadata: { - foo: "bar", - }, } await config.api.viewV2.update(updatedData) diff --git a/packages/types/src/documents/app/view.ts b/packages/types/src/documents/app/view.ts index 02c9b23e38..1b2372da85 100644 --- a/packages/types/src/documents/app/view.ts +++ b/packages/types/src/documents/app/view.ts @@ -99,7 +99,6 @@ export interface ViewV2 { type?: SortType } schema?: ViewV2Schema - uiMetadata?: Record } export type ViewV2Schema = Record From f2e78ec4d500ae03b3bd1e2530c9e0893fdefe91 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:39:44 +0100 Subject: [PATCH 12/24] Don't check required fields at all for calculation views. --- packages/server/src/sdk/app/views/index.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 32fd696f20..fe2360f9d0 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -114,7 +114,11 @@ async function guardViewSchema( } await checkReadonlyFields(table, view) - checkRequiredFields(table, view) + + if (!helpers.views.isCalculationView(view)) { + checkRequiredFields(table, view) + } + checkDisplayField(view) } @@ -178,7 +182,7 @@ function checkRequiredFields( continue } - if (!helpers.views.isCalculationView(view) && !viewSchemaField?.visible) { + if (!viewSchemaField?.visible) { throw new HTTPError( `You can't hide "${field.name}" because it is a required field.`, 400 @@ -186,7 +190,6 @@ function checkRequiredFields( } if ( - viewSchemaField && helpers.views.isBasicViewField(viewSchemaField) && viewSchemaField.readonly ) { From af2071c60cee190afd0dd2e1198ccdb9b098c347 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Mon, 7 Oct 2024 16:44:28 +0100 Subject: [PATCH 13/24] fixing vulns for ent client --- hosting/proxy/nginx.prod.conf | 2 +- .../controllers/static/templates/preview.hbs | 2 +- packages/server/src/environment.ts | 1 + .../src/sdk/app/rows/tests/utils.spec.ts | 41 +++++++++++++++++++ packages/server/src/sdk/app/rows/utils.ts | 10 +++++ 5 files changed, 54 insertions(+), 2 deletions(-) diff --git a/hosting/proxy/nginx.prod.conf b/hosting/proxy/nginx.prod.conf index 59722dac5c..6d5db51bce 100644 --- a/hosting/proxy/nginx.prod.conf +++ b/hosting/proxy/nginx.prod.conf @@ -51,7 +51,7 @@ http { proxy_buffering off; set $csp_default "default-src 'self'"; - set $csp_script "script-src 'self' 'unsafe-inline' 'unsafe-eval' https://*.budibase.net https://cdn.budi.live https://js.intercomcdn.com https://widget.intercom.io https://d2l5prqdbvm3op.cloudfront.net https://us-assets.i.posthog.com"; + set $csp_script "script-src 'self' 'nonce-builderPreview' unsafe-eval' https://*.budibase.net https://cdn.budi.live https://js.intercomcdn.com https://widget.intercom.io https://d2l5prqdbvm3op.cloudfront.net https://us-assets.i.posthog.com"; set $csp_style "style-src 'self' 'unsafe-inline' https://cdn.jsdelivr.net https://fonts.googleapis.com https://rsms.me https://maxcdn.bootstrapcdn.com"; set $csp_object "object-src 'none'"; set $csp_base_uri "base-uri 'self'"; diff --git a/packages/server/src/api/controllers/static/templates/preview.hbs b/packages/server/src/api/controllers/static/templates/preview.hbs index 54b5b1a4e4..be38825ec9 100644 --- a/packages/server/src/api/controllers/static/templates/preview.hbs +++ b/packages/server/src/api/controllers/static/templates/preview.hbs @@ -31,7 +31,7 @@ } - ", + "\">", + "", + "
Hover over me!
", + "'; 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", + "" + ])('test potentially unsafe input: %s', async input => { + environment.XSS_SAFE_MODE = true + const table = getTable() + const row = { text: input } + const output = await validate({ source: table, row }) + expect(output.valid).toBe(false) + expect(output.errors).toBe(["Input not sanitised - potentially vulnerable to XSS"]) + environment.XSS_SAFE_MODE = false + }) + }) }) diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index 6ef4dcbc8e..4c02889f8f 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/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.POSTGRES]: SqlClient.POSTGRES, @@ -43,6 +44,8 @@ const SQL_CLIENT_SOURCE_MAP: Record = { [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 +225,13 @@ 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 } From ce61af1331ebaf6034b05af69afffba9f121c08e Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Mon, 7 Oct 2024 16:47:49 +0100 Subject: [PATCH 14/24] XSS safe mode to prevent unsanitised input --- packages/server/src/sdk/app/rows/tests/utils.spec.ts | 10 ++++++---- packages/server/src/sdk/app/rows/utils.ts | 7 +++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/server/src/sdk/app/rows/tests/utils.spec.ts b/packages/server/src/sdk/app/rows/tests/utils.spec.ts index cd25880a85..9b7711993e 100644 --- a/packages/server/src/sdk/app/rows/tests/utils.spec.ts +++ b/packages/server/src/sdk/app/rows/tests/utils.spec.ts @@ -354,7 +354,7 @@ describe("validate", () => { "1' OR '1' = '1", "' OR 'a' = 'a", "", - "\">", + '">', "", "
Hover over me!
", "'; EXEC sp_msforeachtable 'DROP TABLE ?'; --", @@ -362,14 +362,16 @@ describe("validate", () => { "UNION SELECT * FROM users", "INSERT INTO users (username, password) VALUES ('admin', 'password')", "/* This is a comment */ SELECT * FROM users", - "" - ])('test potentially unsafe input: %s', async input => { + '', + ])("test potentially unsafe input: %s", async input => { environment.XSS_SAFE_MODE = true const table = getTable() const row = { text: input } const output = await validate({ source: table, row }) expect(output.valid).toBe(false) - expect(output.errors).toBe(["Input not sanitised - potentially vulnerable to XSS"]) + expect(output.errors).toBe([ + "Input not sanitised - potentially vulnerable to XSS", + ]) environment.XSS_SAFE_MODE = false }) }) diff --git a/packages/server/src/sdk/app/rows/utils.ts b/packages/server/src/sdk/app/rows/utils.ts index 4c02889f8f..bded6a7a18 100644 --- a/packages/server/src/sdk/app/rows/utils.ts +++ b/packages/server/src/sdk/app/rows/utils.ts @@ -44,7 +44,8 @@ const SQL_CLIENT_SOURCE_MAP: Record = { [SourceName.BUDIBASE]: undefined, } -const XSS_INPUT_REGEX = /[<>;"'(){}]|--|\/\*|\*\/|union|select|insert|drop|delete|update|exec|script/i +const XSS_INPUT_REGEX = + /[<>;"'(){}]|--|\/\*|\*\/|union|select|insert|drop|delete|update|exec|script/i export function getSQLClient(datasource: Datasource): SqlClient { if (!isSQL(datasource)) { @@ -228,7 +229,9 @@ export async function validate({ 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'] + errors[fieldName] = [ + "Input not sanitised - potentially vulnerable to XSS", + ] } } From 12fdb930aa005a30e0f03999e718a27e2e3d80de Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Mon, 7 Oct 2024 17:31:45 +0100 Subject: [PATCH 15/24] remove nonce --- .../server/src/api/controllers/static/templates/preview.hbs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/api/controllers/static/templates/preview.hbs b/packages/server/src/api/controllers/static/templates/preview.hbs index be38825ec9..54b5b1a4e4 100644 --- a/packages/server/src/api/controllers/static/templates/preview.hbs +++ b/packages/server/src/api/controllers/static/templates/preview.hbs @@ -31,7 +31,7 @@ } -