From 38da9012ea658fa6b36031983bd1a305ed7c7502 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 16:56:14 +0200 Subject: [PATCH 1/9] Display error --- .../backend/TableNavigator/modals/CreateTableModal.svelte | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte b/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte index 098369d3b4..b62c8af03d 100644 --- a/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte +++ b/packages/builder/src/components/backend/TableNavigator/modals/CreateTableModal.svelte @@ -78,7 +78,7 @@ await datasources.fetch() await afterSave(table) } catch (e) { - notifications.error(e) + notifications.error(e.message || e) // reload in case the table was created await tables.fetch() } From 785ab122378278ead729293911c9bfd0c1eb582d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:07:56 +0200 Subject: [PATCH 2/9] Add protected name validation test --- .../server/src/api/routes/tests/table.spec.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index dd40a2420e..b96c08ea53 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,5 @@ import { context, docIds, events } from "@budibase/backend-core" +import { PROTECTED_EXTERNAL_COLUMNS } from "@budibase/shared-core" import { AutoFieldSubType, BBReferenceFieldSubType, @@ -1053,6 +1054,42 @@ describe.each([ }, }) }) + + isInternal && + it.each(PROTECTED_EXTERNAL_COLUMNS)( + "don't allow protected names (%s)", + async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + [columnName]: generator.word(), + }, + ], + { + ...basicSchema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + } + ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + } + ) }) describe("validateExistingTableImport", () => { From 73eefa1046e9c24978572ff39014cf8f5b1a2007 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:14:02 +0200 Subject: [PATCH 3/9] Check protected names on validation --- packages/server/src/api/controllers/table/index.ts | 14 +++++++++++--- packages/server/src/utilities/schema.ts | 14 +++++++++++++- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index a02ecc665e..ee6947cec8 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -34,7 +34,11 @@ import sdk from "../../../sdk" import { jsonFromCsvString } from "../../../utilities/csv" import { builderSocket } from "../../../websockets" import { cloneDeep, isEqual } from "lodash" -import { helpers } from "@budibase/shared-core" +import { + helpers, + PROTECTED_EXTERNAL_COLUMNS, + PROTECTED_INTERNAL_COLUMNS, +} from "@budibase/shared-core" function pickApi({ tableId, table }: { tableId?: string; table?: Table }) { if (table && isExternalTable(table)) { @@ -167,7 +171,7 @@ export async function validateNewTableImport( if (isRows(rows) && isSchema(schema)) { ctx.status = 200 - ctx.body = validateSchema(rows, schema) + ctx.body = validateSchema(rows, schema, PROTECTED_INTERNAL_COLUMNS) } else { ctx.status = 422 } @@ -180,6 +184,7 @@ export async function validateExistingTableImport( let schema = null + let protectedColumnNames if (tableId) { const table = await sdk.tables.getTable(tableId) schema = table.schema @@ -189,6 +194,9 @@ export async function validateExistingTableImport( name: "_id", type: FieldType.STRING, } + protectedColumnNames = PROTECTED_INTERNAL_COLUMNS.filter(x => x !== "_id") + } else { + protectedColumnNames = PROTECTED_EXTERNAL_COLUMNS } } else { ctx.status = 422 @@ -197,7 +205,7 @@ export async function validateExistingTableImport( if (tableId && isRows(rows) && isSchema(schema)) { ctx.status = 200 - ctx.body = validateSchema(rows, schema) + ctx.body = validateSchema(rows, schema, protectedColumnNames) } else { ctx.status = 422 } diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index 4bd4e8f583..c7a4427dd7 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -41,7 +41,11 @@ export function isRows(rows: any): rows is Rows { return Array.isArray(rows) && rows.every(row => typeof row === "object") } -export function validate(rows: Rows, schema: TableSchema): ValidationResults { +export function validate( + rows: Rows, + schema: TableSchema, + protectedColumnNames: readonly string[] +): ValidationResults { const results: ValidationResults = { schemaValidation: {}, allValid: false, @@ -49,6 +53,8 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { errors: {}, } + protectedColumnNames = protectedColumnNames.map(x => x.toLowerCase()) + rows.forEach(row => { Object.entries(row).forEach(([columnName, columnData]) => { const { @@ -63,6 +69,12 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults { return } + if (protectedColumnNames.includes(columnName.toLowerCase())) { + results.schemaValidation[columnName] = false + results.errors[columnName] = `${columnName} is a protected name` + return + } + // If the columnType is not a string, then it's not present in the schema, and should be added to the invalid columns array if (typeof columnType !== "string") { results.invalidColumns.push(columnName) From c015f8d192416695e6b9200eb13749597ac475ad Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:18:03 +0200 Subject: [PATCH 4/9] Run for both internal and external --- packages/server/src/api/routes/tests/table.spec.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index b96c08ea53..0d15919db4 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,5 +1,8 @@ import { context, docIds, events } from "@budibase/backend-core" -import { PROTECTED_EXTERNAL_COLUMNS } from "@budibase/shared-core" +import { + PROTECTED_EXTERNAL_COLUMNS, + PROTECTED_INTERNAL_COLUMNS, +} from "@budibase/shared-core" import { AutoFieldSubType, BBReferenceFieldSubType, @@ -1055,10 +1058,9 @@ describe.each([ }) }) - isInternal && - it.each(PROTECTED_EXTERNAL_COLUMNS)( - "don't allow protected names (%s)", - async columnName => { + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )("don't allow protected names (%s)", async columnName => { const result = await config.api.table.validateNewTableImport( [ { From 788a16cf48cbddf82c041f4439a83df13246d4e6 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:26:42 +0200 Subject: [PATCH 5/9] Add safety tests --- .../server/src/api/routes/tests/table.spec.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 0d15919db4..16f7f68550 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -123,6 +123,29 @@ describe.each([ body: basicTable(), }) }) + + it("does not persist the row fields that are not on the table schema", async () => { + const table: SaveTableRequest = basicTable() + table.rows = [ + { + name: "test-name", + description: "test-desc", + nonValid: "test-non-valid", + }, + ] + + const res = await config.api.table.save(table) + + const persistedRows = await config.api.row.search(res._id!) + + expect(persistedRows.rows).toEqual([ + expect.objectContaining({ + name: "test-name", + description: "test-desc", + }), + ]) + expect(persistedRows.rows[0].nonValid).toBeUndefined() + }) }) describe("update", () => { From 3f4484af00dbbef7b4de8e0e3c8e64e2362e7e7c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:32:19 +0200 Subject: [PATCH 6/9] Add extra tests --- .../server/src/api/routes/tests/table.spec.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 16f7f68550..594f95f9fa 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -146,6 +146,41 @@ describe.each([ ]) expect(persistedRows.rows[0].nonValid).toBeUndefined() }) + + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )( + "cannot use protected column names (%s) while importing a table", + async columnName => { + const table: SaveTableRequest = basicTable() + table.rows = [ + { + name: "test-name", + description: "test-desc", + }, + ] + + await config.api.table.save( + { + ...table, + schema: { + ...table.schema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + }, + }, + { + status: 400, + body: { + message: `Column(s) "${columnName}" are duplicated - check for other columns with these name (case in-sensitive)`, + status: 400, + }, + } + ) + } + ) }) describe("update", () => { From ad74eca709b96d4b14027110e1c835b7a59d6af1 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:32:36 +0200 Subject: [PATCH 7/9] Fix --- .../server/src/sdk/app/tables/internal/index.ts | 4 +--- packages/shared-core/src/table.ts | 14 +++++--------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index 58f172e016..c0beed0db8 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -48,9 +48,7 @@ export async function save( } // check for case sensitivity - we don't want to allow duplicated columns - const duplicateColumn = findDuplicateInternalColumns(table, { - ignoreProtectedColumnNames: !oldTable && !!opts?.isImport, - }) + const duplicateColumn = findDuplicateInternalColumns(table) if (duplicateColumn.length) { throw new Error( `Column(s) "${duplicateColumn.join( diff --git a/packages/shared-core/src/table.ts b/packages/shared-core/src/table.ts index d5fd1dec00..9e7626cb1c 100644 --- a/packages/shared-core/src/table.ts +++ b/packages/shared-core/src/table.ts @@ -53,10 +53,7 @@ export function canBeSortColumn(type: FieldType): boolean { return !!allowSortColumnByType[type] } -export function findDuplicateInternalColumns( - table: Table, - opts?: { ignoreProtectedColumnNames: boolean } -): string[] { +export function findDuplicateInternalColumns(table: Table): string[] { // maintains the case of keys const casedKeys = Object.keys(table.schema) // get the column names @@ -72,11 +69,10 @@ export function findDuplicateInternalColumns( } } } - if (!opts?.ignoreProtectedColumnNames) { - for (let internalColumn of PROTECTED_INTERNAL_COLUMNS) { - if (casedKeys.find(key => key === internalColumn)) { - duplicates.push(internalColumn) - } + + for (let internalColumn of PROTECTED_INTERNAL_COLUMNS) { + if (casedKeys.find(key => key === internalColumn)) { + duplicates.push(internalColumn) } } return duplicates From 8f741ffe6ac1fa561311e6ecc35ea8f9e9b3cdef Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 31 Jul 2024 17:40:30 +0200 Subject: [PATCH 8/9] More validations --- .../server/src/api/routes/tests/table.spec.ts | 89 ++++++++++++------- packages/server/src/utilities/schema.ts | 9 +- 2 files changed, 67 insertions(+), 31 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 594f95f9fa..f383fed927 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1118,38 +1118,67 @@ describe.each([ it.each( isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS - )("don't allow protected names (%s)", async columnName => { - const result = await config.api.table.validateNewTableImport( - [ - { - id: generator.natural(), - name: generator.first(), - [columnName]: generator.word(), - }, - ], - { - ...basicSchema, - [columnName]: { - name: columnName, - type: FieldType.STRING, - }, - } - ) - - expect(result).toEqual({ - allValid: false, - errors: { - [columnName]: `${columnName} is a protected name`, - }, - invalidColumns: [], - schemaValidation: { - id: true, - name: true, - [columnName]: false, - }, - }) + )("don't allow protected names in schema (%s)", async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + [columnName]: generator.word(), + }, + ], + { + ...basicSchema, } ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected column name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + }) + + isInternal && + it.each( + isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS + )("don't allow protected names in the rows (%s)", async columnName => { + const result = await config.api.table.validateNewTableImport( + [ + { + id: generator.natural(), + name: generator.first(), + }, + ], + { + ...basicSchema, + [columnName]: { + name: columnName, + type: FieldType.STRING, + }, + } + ) + + expect(result).toEqual({ + allValid: false, + errors: { + [columnName]: `${columnName} is a protected column name`, + }, + invalidColumns: [], + schemaValidation: { + id: true, + name: true, + [columnName]: false, + }, + }) + }) }) describe("validateExistingTableImport", () => { diff --git a/packages/server/src/utilities/schema.ts b/packages/server/src/utilities/schema.ts index c7a4427dd7..c6b26b55c8 100644 --- a/packages/server/src/utilities/schema.ts +++ b/packages/server/src/utilities/schema.ts @@ -71,7 +71,7 @@ export function validate( if (protectedColumnNames.includes(columnName.toLowerCase())) { results.schemaValidation[columnName] = false - results.errors[columnName] = `${columnName} is a protected name` + results.errors[columnName] = `${columnName} is a protected column name` return } @@ -121,6 +121,13 @@ export function validate( }) }) + for (const schemaField of Object.keys(schema)) { + if (protectedColumnNames.includes(schemaField.toLowerCase())) { + results.schemaValidation[schemaField] = false + results.errors[schemaField] = `${schemaField} is a protected column name` + } + } + results.allValid = Object.values(results.schemaValidation).length > 0 && Object.values(results.schemaValidation).every(column => column) From f4bd3035721b0f778d6ff73d9b4653500d7baa37 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 1 Aug 2024 11:02:21 +0200 Subject: [PATCH 9/9] Handle frontend --- .../backend/TableNavigator/TableDataImport.svelte | 12 ++++++++---- packages/shared-core/src/utils.ts | 10 ++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte b/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte index 5bc3b9e728..4c9f4dd10f 100644 --- a/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte +++ b/packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte @@ -1,9 +1,9 @@