From def3b0260e394ceac61718154e7c39600e44810d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 20 Jun 2024 18:48:22 +0100 Subject: [PATCH 1/4] Disallowing prohibited columns consistently, no matter the case, and backend validation for this as well. --- packages/backend-core/src/db/constants.ts | 19 +++++----------- .../DataTable/modals/CreateEditColumn.svelte | 17 ++++++++++---- .../src/sdk/app/tables/internal/index.ts | 12 +++++++++- packages/shared-core/src/constants/index.ts | 1 + packages/shared-core/src/constants/rows.ts | 14 ++++++++++++ packages/shared-core/src/table.ts | 22 ++++++++++++++++++- 6 files changed, 65 insertions(+), 20 deletions(-) create mode 100644 packages/shared-core/src/constants/rows.ts diff --git a/packages/backend-core/src/db/constants.ts b/packages/backend-core/src/db/constants.ts index bfa7595d62..69c98fe569 100644 --- a/packages/backend-core/src/db/constants.ts +++ b/packages/backend-core/src/db/constants.ts @@ -1,14 +1,5 @@ -export const CONSTANT_INTERNAL_ROW_COLS = [ - "_id", - "_rev", - "type", - "createdAt", - "updatedAt", - "tableId", -] as const - -export const CONSTANT_EXTERNAL_ROW_COLS = ["_id", "_rev", "tableId"] as const - -export function isInternalColumnName(name: string): boolean { - return (CONSTANT_INTERNAL_ROW_COLS as readonly string[]).includes(name) -} +export { + CONSTANT_INTERNAL_ROW_COLS, + CONSTANT_EXTERNAL_ROW_COLS, + isInternalColumnName, +} from "@budibase/shared-core" diff --git a/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte b/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte index 17ecd8f844..29d418c1f6 100644 --- a/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte +++ b/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte @@ -17,6 +17,8 @@ SWITCHABLE_TYPES, ValidColumnNameRegex, helpers, + CONSTANT_INTERNAL_ROW_COLS, + CONSTANT_EXTERNAL_ROW_COLS, } from "@budibase/shared-core" import { createEventDispatcher, getContext, onMount } from "svelte" import { cloneDeep } from "lodash/fp" @@ -487,20 +489,27 @@ }) } const newError = {} + const prohibited = externalTable + ? CONSTANT_EXTERNAL_ROW_COLS + : CONSTANT_INTERNAL_ROW_COLS if (!externalTable && fieldInfo.name?.startsWith("_")) { newError.name = `Column name cannot start with an underscore.` } else if (fieldInfo.name && !fieldInfo.name.match(ValidColumnNameRegex)) { newError.name = `Illegal character; must be alpha-numeric.` - } else if (PROHIBITED_COLUMN_NAMES.some(name => fieldInfo.name === name)) { - newError.name = `${PROHIBITED_COLUMN_NAMES.join( + } else if ( + prohibited.some( + name => fieldInfo?.name?.toLowerCase() === name.toLowerCase() + ) + ) { + newError.name = `${prohibited.join( ", " - )} are not allowed as column names` + )} are not allowed as column names - case insensitive.` } else if (inUse($tables.selected, fieldInfo.name, originalName)) { newError.name = `Column name already in use.` } if (fieldInfo.type === FieldType.AUTO && !fieldInfo.subtype) { - newError.subtype = `Auto Column requires a type` + newError.subtype = `Auto Column requires a type.` } if (fieldInfo.fieldName && fieldInfo.tableId) { diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index ea40d2bfe9..9178b2cea3 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -16,7 +16,8 @@ import { EventType, updateLinks } from "../../../../db/linkedRows" import { cloneDeep } from "lodash/fp" import isEqual from "lodash/isEqual" import { runStaticFormulaChecks } from "../../../../api/controllers/table/bulkFormula" -import { context } from "@budibase/backend-core" +import { context, db as dbCore } from "@budibase/backend-core" +import { findDuplicateInternalColumns } from "@budibase/shared-core" import { getTable } from "../getters" import { checkAutoColumns } from "./utils" import * as viewsSdk from "../../views" @@ -44,6 +45,15 @@ export async function save( if (hasTypeChanged(table, oldTable)) { throw new Error("A column type has changed.") } + + // check for case sensitivity - we don't want to allow duplicated columns + const duplicateColumn = findDuplicateInternalColumns(table) + if (duplicateColumn) { + throw new Error( + `Column "${duplicateColumn}" is duplicated - make sure there are no duplicate columns names, this is case insensitive.` + ) + } + // check that subtypes have been maintained table = checkAutoColumns(table, oldTable) diff --git a/packages/shared-core/src/constants/index.ts b/packages/shared-core/src/constants/index.ts index afb7e659e1..084bc8fe9e 100644 --- a/packages/shared-core/src/constants/index.ts +++ b/packages/shared-core/src/constants/index.ts @@ -1,5 +1,6 @@ export * from "./api" export * from "./fields" +export * from "./rows" export const OperatorOptions = { Equals: { diff --git a/packages/shared-core/src/constants/rows.ts b/packages/shared-core/src/constants/rows.ts new file mode 100644 index 0000000000..bfa7595d62 --- /dev/null +++ b/packages/shared-core/src/constants/rows.ts @@ -0,0 +1,14 @@ +export const CONSTANT_INTERNAL_ROW_COLS = [ + "_id", + "_rev", + "type", + "createdAt", + "updatedAt", + "tableId", +] as const + +export const CONSTANT_EXTERNAL_ROW_COLS = ["_id", "_rev", "tableId"] as const + +export function isInternalColumnName(name: string): boolean { + return (CONSTANT_INTERNAL_ROW_COLS as readonly string[]).includes(name) +} diff --git a/packages/shared-core/src/table.ts b/packages/shared-core/src/table.ts index 7706b78037..1e40b38c05 100644 --- a/packages/shared-core/src/table.ts +++ b/packages/shared-core/src/table.ts @@ -1,4 +1,5 @@ -import { FieldType } from "@budibase/types" +import { FieldType, Table } from "@budibase/types" +import { CONSTANT_INTERNAL_ROW_COLS } from "./constants" const allowDisplayColumnByType: Record = { [FieldType.STRING]: true, @@ -51,3 +52,22 @@ export function canBeDisplayColumn(type: FieldType): boolean { export function canBeSortColumn(type: FieldType): boolean { return !!allowSortColumnByType[type] } + +export function findDuplicateInternalColumns(table: Table): string | undefined { + // get the column names + const columnNames = Object.keys(table.schema) + .concat(CONSTANT_INTERNAL_ROW_COLS) + .map(colName => colName.toLowerCase()) + // there are duplicates + const set = new Set(columnNames) + let foundDuplicate: string | undefined + if (set.size !== columnNames.length) { + for (let key of set.keys()) { + const count = columnNames.filter(name => name === key).length + if (count > 1) { + foundDuplicate = key + } + } + } + return foundDuplicate +} From ae68c561f4750bd0901f349837f7695b871c1436 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 20 Jun 2024 18:51:04 +0100 Subject: [PATCH 2/4] Test case. --- .../server/src/api/routes/tests/table.spec.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index f23e0de6db..4c3d8db1af 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -276,6 +276,31 @@ describe.each([ }) }) + it("shouldn't allow duplicate column names", async () => { + const saveTableRequest: SaveTableRequest = { + ...basicTable(), + } + saveTableRequest.schema["Type"] = { type: FieldType.STRING, name: "Type" } + await config.api.table.save(saveTableRequest, { + status: 400, + body: { + message: + 'Column "type" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + }, + }) + saveTableRequest.schema = { + foo: { type: FieldType.STRING, name: "foo" }, + FOO: { type: FieldType.STRING, name: "FOO" }, + } + await config.api.table.save(saveTableRequest, { + status: 400, + body: { + message: + 'Column "foo" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + }, + }) + }) + it("should add a new column for an internal DB table", async () => { const saveTableRequest: SaveTableRequest = { ...basicTable(), From fead1f436a5f30c4c28974d2b282866bbf0be70f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 20 Jun 2024 18:53:01 +0100 Subject: [PATCH 3/4] test case is only for internal. --- .../server/src/api/routes/tests/table.spec.ts | 50 ++++++++++--------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 4c3d8db1af..a84fd923bb 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -276,30 +276,34 @@ describe.each([ }) }) - it("shouldn't allow duplicate column names", async () => { - const saveTableRequest: SaveTableRequest = { - ...basicTable(), - } - saveTableRequest.schema["Type"] = { type: FieldType.STRING, name: "Type" } - await config.api.table.save(saveTableRequest, { - status: 400, - body: { - message: - 'Column "type" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', - }, + isInternal && + it("shouldn't allow duplicate column names", async () => { + const saveTableRequest: SaveTableRequest = { + ...basicTable(), + } + saveTableRequest.schema["Type"] = { + type: FieldType.STRING, + name: "Type", + } + await config.api.table.save(saveTableRequest, { + status: 400, + body: { + message: + 'Column "type" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + }, + }) + saveTableRequest.schema = { + foo: { type: FieldType.STRING, name: "foo" }, + FOO: { type: FieldType.STRING, name: "FOO" }, + } + await config.api.table.save(saveTableRequest, { + status: 400, + body: { + message: + 'Column "foo" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + }, + }) }) - saveTableRequest.schema = { - foo: { type: FieldType.STRING, name: "foo" }, - FOO: { type: FieldType.STRING, name: "FOO" }, - } - await config.api.table.save(saveTableRequest, { - status: 400, - body: { - message: - 'Column "foo" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', - }, - }) - }) it("should add a new column for an internal DB table", async () => { const saveTableRequest: SaveTableRequest = { From b4910043c67fe3bcd9c162684a46bee5bcaaa707 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 21 Jun 2024 11:27:47 +0100 Subject: [PATCH 4/4] Addressing PR comments. --- .../backend/DataTable/modals/CreateEditColumn.svelte | 1 - packages/server/src/api/routes/tests/table.spec.ts | 11 +++++------ packages/server/src/sdk/app/tables/internal/index.ts | 8 +++++--- packages/shared-core/src/table.ts | 8 ++++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte b/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte index 29d418c1f6..d79eedd194 100644 --- a/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte +++ b/packages/builder/src/components/backend/DataTable/modals/CreateEditColumn.svelte @@ -54,7 +54,6 @@ const DATE_TYPE = FieldType.DATETIME const dispatch = createEventDispatcher() - const PROHIBITED_COLUMN_NAMES = ["type", "_id", "_rev", "tableId"] const { dispatch: gridDispatch, rows } = getContext("grid") export let field diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index a84fd923bb..e75e5e23e7 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -289,18 +289,17 @@ describe.each([ status: 400, body: { message: - 'Column "type" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + 'Column(s) "type" are duplicated - check for other columns with these name (case in-sensitive)', }, }) - saveTableRequest.schema = { - foo: { type: FieldType.STRING, name: "foo" }, - FOO: { type: FieldType.STRING, name: "FOO" }, - } + saveTableRequest.schema.foo = { type: FieldType.STRING, name: "foo" } + saveTableRequest.schema.FOO = { type: FieldType.STRING, name: "FOO" } + await config.api.table.save(saveTableRequest, { status: 400, body: { message: - 'Column "foo" is duplicated - make sure there are no duplicate columns names, this is case insensitive.', + 'Column(s) "type, foo" are duplicated - check for other columns with these name (case in-sensitive)', }, }) }) diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index 9178b2cea3..fc32708708 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -16,7 +16,7 @@ import { EventType, updateLinks } from "../../../../db/linkedRows" import { cloneDeep } from "lodash/fp" import isEqual from "lodash/isEqual" import { runStaticFormulaChecks } from "../../../../api/controllers/table/bulkFormula" -import { context, db as dbCore } from "@budibase/backend-core" +import { context } from "@budibase/backend-core" import { findDuplicateInternalColumns } from "@budibase/shared-core" import { getTable } from "../getters" import { checkAutoColumns } from "./utils" @@ -48,9 +48,11 @@ export async function save( // check for case sensitivity - we don't want to allow duplicated columns const duplicateColumn = findDuplicateInternalColumns(table) - if (duplicateColumn) { + if (duplicateColumn.length) { throw new Error( - `Column "${duplicateColumn}" is duplicated - make sure there are no duplicate columns names, this is case insensitive.` + `Column(s) "${duplicateColumn.join( + ", " + )}" are duplicated - check for other columns with these name (case in-sensitive)` ) } diff --git a/packages/shared-core/src/table.ts b/packages/shared-core/src/table.ts index 1e40b38c05..4b578a2aef 100644 --- a/packages/shared-core/src/table.ts +++ b/packages/shared-core/src/table.ts @@ -53,21 +53,21 @@ export function canBeSortColumn(type: FieldType): boolean { return !!allowSortColumnByType[type] } -export function findDuplicateInternalColumns(table: Table): string | undefined { +export function findDuplicateInternalColumns(table: Table): string[] { // get the column names const columnNames = Object.keys(table.schema) .concat(CONSTANT_INTERNAL_ROW_COLS) .map(colName => colName.toLowerCase()) // there are duplicates const set = new Set(columnNames) - let foundDuplicate: string | undefined + let duplicates: string[] = [] if (set.size !== columnNames.length) { for (let key of set.keys()) { const count = columnNames.filter(name => name === key).length if (count > 1) { - foundDuplicate = key + duplicates.push(key) } } } - return foundDuplicate + return duplicates }