From 27508d934dc7df489638c686317a894877b1a407 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 10:45:03 +0100 Subject: [PATCH 01/13] 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 27578db4b7e0298cd11aa937f7b795252bf8e918 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 09:48:33 +0100 Subject: [PATCH 02/13] 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 e6e25fdf943d69e975f025bec9e51dc26227dec1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 14:59:29 +0100 Subject: [PATCH 03/13] 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 f2e78ec4d500ae03b3bd1e2530c9e0893fdefe91 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 16:39:44 +0100 Subject: [PATCH 04/13] 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 05/13] 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 06/13] 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 07/13] 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 @@ } -