Merge pull request #14294 from Budibase/fix/dont-allow-protected-colu…

…mn-names-on-import

Dont allow protected column names on import

packages/builder/src/components/backend/TableNavigator/TableDataImport.svelte

@@ -1,9 +1,9 @@
 <script>
   import { Select, Icon } from "@budibase/bbui"
   import { FIELDS } from "constants/backend"
+  import { canBeDisplayColumn, utils } from "@budibase/shared-core"
   import { API } from "api"
   import { parseFile } from "./utils"
-  import { canBeDisplayColumn } from "@budibase/shared-core"
 
   export let rows = []
   export let schema = {}
@@ -97,6 +97,8 @@
   let errors = {}
   let selectedColumnTypes = {}
 
+  let rawRows = []
+
   $: displayColumnOptions = Object.keys(schema || {}).filter(column => {
     return validation[column] && canBeDisplayColumn(schema[column].type)
   })
@@ -106,6 +108,8 @@
   }
 
   $: {
+    rows = rawRows.map(row => utils.trimOtherProps(row, Object.keys(schema)))
+
     // binding in consumer is causing double renders here
     const newValidateHash = JSON.stringify(rows) + JSON.stringify(schema)
     if (newValidateHash !== validateHash) {
@@ -122,7 +126,7 @@
 
     try {
       const response = await parseFile(e)
-      rows = response.rows
+      rawRows = response.rows
       schema = response.schema
       fileName = response.fileName
       selectedColumnTypes = Object.entries(response.schema).reduce(
@@ -188,7 +192,7 @@
     type="file"
     on:change={handleFile}
   />
-  <label for="file-upload" class:uploaded={rows.length > 0}>
+  <label for="file-upload" class:uploaded={rawRows.length > 0}>
     {#if error}
       Error: {error}
     {:else if fileName}
@@ -198,7 +202,7 @@
     {/if}
   </label>
 </div>
-{#if rows.length > 0 && !error}
+{#if rawRows.length > 0 && !error}
   <div class="schema-fields">
     {#each Object.entries(schema) as [name, column]}
       <div class="field">

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()
     }

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
   }

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

@@ -1,4 +1,8 @@
 import { context, docIds, events } from "@budibase/backend-core"
+import {
+  PROTECTED_EXTERNAL_COLUMNS,
+  PROTECTED_INTERNAL_COLUMNS,
+} from "@budibase/shared-core"
 import {
   AutoFieldSubType,
   BBReferenceFieldSubType,
@@ -119,6 +123,64 @@ 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()
+    })
+
+    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", () => {
@@ -1053,6 +1115,70 @@ describe.each([
           },
         })
       })
+
+      it.each(
+        isInternal ? PROTECTED_INTERNAL_COLUMNS : PROTECTED_EXTERNAL_COLUMNS
+      )("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", () => {

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(

packages/server/src/utilities/schema.ts

@@ -41,14 +41,20 @@ 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,
     invalidColumns: [],
     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 column 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)
@@ -109,6 +121,13 @@ export function validate(rows: Rows, schema: TableSchema): ValidationResults {
     })
   })
 
+  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)

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

packages/shared-core/src/utils.ts

@@ -67,3 +67,13 @@ export function hasSchema(test: any) {
     Object.keys(test).length > 0
   )
 }
+
+export function trimOtherProps(object: any, allowedProps: string[]) {
+  const result = Object.keys(object)
+    .filter(key => allowedProps.includes(key))
+    .reduce<Record<string, any>>(
+      (acc, key) => ({ ...acc, [key]: object[key] }),
+      {}
+    )
+  return result
+}