From cc2605a9dd7b7a8eedb9889205aa060d1130f5b7 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 21 Oct 2024 10:04:03 +0100 Subject: [PATCH 01/26] WIP --- packages/backend-core/src/sql/sql.ts | 2 +- .../server/src/api/routes/tests/search.spec.ts | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 949d7edf1b..0a9d55cf59 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -236,7 +236,7 @@ class InternalBuilder { const alias = this.getTableName(endpoint.entityId) const schema = meta.table.schema if (!this.isFullSelectStatementRequired()) { - return [this.knex.raw(`${this.quote(alias)}.*`)] + return [this.knex.raw("??", [`${alias}.*`])] } // get just the fields for this table return resource.fields diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index e0143c5938..1c36a6577c 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -48,14 +48,14 @@ import { generateRowIdField } from "../../../integrations/utils" import { cloneDeep } from "lodash/fp" describe.each([ - ["in-memory", undefined], - ["lucene", undefined], - ["sqs", undefined], + // ["in-memory", undefined], + // ["lucene", undefined], + // ["sqs", undefined], [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" From 44bd00a0d7be259b0d4a54b8421f6537337d948c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 21 Oct 2024 18:20:52 +0100 Subject: [PATCH 02/26] Making progress on converting raw calls to use bindings. --- packages/backend-core/src/sql/sql.ts | 138 ++++++++---------- .../src/api/routes/tests/search.spec.ts | 52 +++---- .../src/api/routes/tests/viewV2.spec.ts | 12 +- 3 files changed, 90 insertions(+), 112 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 0a9d55cf59..ec22ac37f9 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -258,30 +258,38 @@ class InternalBuilder { const columnSchema = schema[column] if (this.SPECIAL_SELECT_CASES.POSTGRES_MONEY(columnSchema)) { - return this.knex.raw( - `${this.quotedIdentifier( - [table, column].join(".") - )}::money::numeric as ${this.quote(field)}` - ) + return this.knex.raw("??::money::numeric as ??", [ + [table, column].join("."), + field, + ]) } if (this.SPECIAL_SELECT_CASES.MSSQL_DATES(columnSchema)) { // Time gets returned as timestamp from mssql, not matching the expected // HH:mm format - return this.knex.raw(`CONVERT(varchar, ${field}, 108) as "${field}"`) + + // TODO: figure out how to express this safely without string + // interpolation. + return this.knex.raw(`CONVERT(varchar, ??, 108) as "${field}"`, [ + field, + ]) } - const quoted = table - ? `${this.quote(table)}.${this.quote(column)}` - : this.quote(field) - return this.knex.raw(quoted) + if (table) { + return this.knex.raw("??", [`${table}.${column}`]) + } else { + return this.knex.raw("??", [field]) + } }) } // OracleDB can't use character-large-objects (CLOBs) in WHERE clauses, // so when we use them we need to wrap them in to_char(). This function // converts a field name to the appropriate identifier. - private convertClobs(field: string, opts?: { forSelect?: boolean }): string { + private convertClobs( + field: string, + opts?: { forSelect?: boolean } + ): Knex.Raw { if (this.client !== SqlClient.ORACLE) { throw new Error( "you've called convertClobs on a DB that's not Oracle, this is a mistake" @@ -290,7 +298,7 @@ class InternalBuilder { const parts = this.splitIdentifier(field) const col = parts.pop()! const schema = this.table.schema[col] - let identifier = this.quotedIdentifier(field) + let identifier = this.knex.raw("??", [field]) if ( schema.type === FieldType.STRING || @@ -301,9 +309,9 @@ class InternalBuilder { schema.type === FieldType.BARCODEQR ) { if (opts?.forSelect) { - identifier = `to_char(${identifier}) as ${this.quotedIdentifier(col)}` + identifier = this.knex.raw("to_char(??) as ??", [identifier, col]) } else { - identifier = `to_char(${identifier})` + identifier = this.knex.raw("to_char(??)", [identifier]) } } return identifier @@ -485,9 +493,7 @@ class InternalBuilder { .where( `${throughAlias}.${manyToMany.from}`, "=", - mainKnex.raw( - this.quotedIdentifier(`${fromAlias}.${manyToMany.fromPrimary}`) - ) + mainKnex.raw(`??`, [`${fromAlias}.${manyToMany.fromPrimary}`]) ) // in SQS the same junction table is used for different many-to-many relationships between the // two same tables, this is needed to avoid rows ending up in all columns @@ -516,7 +522,7 @@ class InternalBuilder { subQuery = subQuery.where( toKey, "=", - mainKnex.raw(this.quotedIdentifier(foreignKey)) + mainKnex.raw("??", [foreignKey]) ) query = query.where(q => { @@ -736,39 +742,29 @@ class InternalBuilder { ArrayOperator.ONE_OF, (q, key: string, array) => { if (this.client === SqlClient.ORACLE) { + // @ts-ignore key = this.convertClobs(key) - array = Array.isArray(array) ? array : [array] - const binding = new Array(array.length).fill("?").join(",") - return q.whereRaw(`${key} IN (${binding})`, array) - } else { - return q[fnc](key, Array.isArray(array) ? array : [array]) } + return q[fnc](key, Array.isArray(array) ? array : [array]) }, (q, key: string[], array) => { if (this.client === SqlClient.ORACLE) { - const keyStr = `(${key.map(k => this.convertClobs(k)).join(",")})` - const binding = `(${array - .map((a: any) => `(${new Array(a.length).fill("?").join(",")})`) - .join(",")})` - return q.whereRaw(`${keyStr} IN ${binding}`, array.flat()) - } else { - return q[fnc](key, Array.isArray(array) ? array : [array]) + // @ts-ignore + key = key.map(k => this.convertClobs(k)) } + return q[fnc](key, Array.isArray(array) ? array : [array]) } ) } if (filters.string) { iterate(filters.string, BasicOperator.STRING, (q, key, value) => { - const fnc = allOr ? "orWhere" : "where" // postgres supports ilike, nothing else does if (this.client === SqlClient.POSTGRES) { + const fnc = allOr ? "orWhere" : "where" return q[fnc](key, "ilike", `${value}%`) } else { - const rawFnc = `${fnc}Raw` - // @ts-ignore - return q[rawFnc](`LOWER(${this.quotedIdentifier(key)}) LIKE ?`, [ - `${value.toLowerCase()}%`, - ]) + const fnc = allOr ? "orWhereRaw" : "whereRaw" + return q[fnc](`LOWER(??) LIKE ?`, [key, `${value.toLowerCase()}%`]) } }) } @@ -795,48 +791,33 @@ class InternalBuilder { const schema = this.getFieldSchema(key) + let rawKey: string | Knex.Raw = key + let high = value.high + let low = value.low + if (this.client === SqlClient.ORACLE) { - // @ts-ignore - key = this.knex.raw(this.convertClobs(key)) + rawKey = this.convertClobs(key) + } else if ( + this.client === SqlClient.SQL_LITE && + schema?.type === FieldType.BIGINT + ) { + rawKey = this.knex.raw("CAST(?? AS INTEGER)", [key]) + high = this.knex.raw("CAST(? AS INTEGER)", [value.high]) + low = this.knex.raw("CAST(? AS INTEGER)", [value.low]) } if (lowValid && highValid) { - if ( - schema?.type === FieldType.BIGINT && - this.client === SqlClient.SQL_LITE - ) { - return q.whereRaw( - `CAST(${key} AS INTEGER) BETWEEN CAST(? AS INTEGER) AND CAST(? AS INTEGER)`, - [value.low, value.high] - ) - } else { - const fnc = allOr ? "orWhereBetween" : "whereBetween" - return q[fnc](key, [value.low, value.high]) - } + const fnc = allOr ? "orWhereBetween" : "whereBetween" + // @ts-ignore + return q[fnc](rawKey, [low, high]) } else if (lowValid) { - if ( - schema?.type === FieldType.BIGINT && - this.client === SqlClient.SQL_LITE - ) { - return q.whereRaw(`CAST(${key} AS INTEGER) >= CAST(? AS INTEGER)`, [ - value.low, - ]) - } else { - const fnc = allOr ? "orWhere" : "where" - return q[fnc](key, ">=", value.low) - } + const fnc = allOr ? "orWhere" : "where" + // @ts-ignore + return q[fnc](rawKey, ">=", low) } else if (highValid) { - if ( - schema?.type === FieldType.BIGINT && - this.client === SqlClient.SQL_LITE - ) { - return q.whereRaw(`CAST(${key} AS INTEGER) <= CAST(? AS INTEGER)`, [ - value.high, - ]) - } else { - const fnc = allOr ? "orWhere" : "where" - return q[fnc](key, "<=", value.high) - } + const fnc = allOr ? "orWhere" : "where" + // @ts-ignore + return q[fnc](rawKey, "<=", high) } return q }) @@ -976,9 +957,7 @@ class InternalBuilder { const selectFields = qualifiedFields.map(field => this.convertClobs(field, { forSelect: true }) ) - query = query - .groupByRaw(groupByFields.join(", ")) - .select(this.knex.raw(selectFields.join(", "))) + query = query.groupBy(groupByFields).select(selectFields) } else { query = query.groupBy(qualifiedFields).select(qualifiedFields) } @@ -990,11 +969,10 @@ class InternalBuilder { if (this.client === SqlClient.ORACLE) { const field = this.convertClobs(`${tableName}.${aggregation.field}`) query = query.select( - this.knex.raw( - `COUNT(DISTINCT ${field}) as ${this.quotedIdentifier( - aggregation.name - )}` - ) + this.knex.raw(`COUNT(DISTINCT ??) as ??`, [ + field, + aggregation.name, + ]) ) } else { query = query.countDistinct( diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 1c36a6577c..718ad47a31 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -48,14 +48,14 @@ import { generateRowIdField } from "../../../integrations/utils" import { cloneDeep } from "lodash/fp" describe.each([ - // ["in-memory", undefined], - // ["lucene", undefined], - // ["sqs", undefined], + ["in-memory", undefined], + ["lucene", undefined], + ["sqs", undefined], [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], - // [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" const isLucene = name === "lucene" @@ -155,24 +155,24 @@ describe.each([ describe.each([ ["table", createTable], - [ - "view", - async (schema: TableSchema) => { - const tableId = await createTable(schema) - const viewId = await createView( - tableId, - Object.keys(schema).reduce((viewSchema, fieldName) => { - const field = schema[fieldName] - viewSchema[fieldName] = { - visible: field.visible ?? true, - readonly: false, - } - return viewSchema - }, {}) - ) - return viewId - }, - ], + // [ + // "view", + // async (schema: TableSchema) => { + // const tableId = await createTable(schema) + // const viewId = await createView( + // tableId, + // Object.keys(schema).reduce((viewSchema, fieldName) => { + // const field = schema[fieldName] + // viewSchema[fieldName] = { + // visible: field.visible ?? true, + // readonly: false, + // } + // return viewSchema + // }, {}) + // ) + // return viewId + // }, + // ], ])("from %s", (sourceType, createTableOrView) => { const isView = sourceType === "view" @@ -792,7 +792,7 @@ describe.each([ }) }) - describe.each([FieldType.STRING, FieldType.LONGFORM])("%s", () => { + describe.only.each([FieldType.STRING, FieldType.LONGFORM])("%s", () => { beforeAll(async () => { tableOrViewId = await createTableOrView({ name: { name: "name", type: FieldType.STRING }, diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index dfd4f50bd1..0c18307909 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -35,12 +35,12 @@ import { quotas } from "@budibase/pro" import { db, roles, features } from "@budibase/backend-core" describe.each([ - ["lucene", undefined], - ["sqs", undefined], - [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // ["lucene", undefined], + // ["sqs", undefined], + // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() From 4545493cd514a8c53a0f5f21f9a512454225f1ad Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 22 Oct 2024 11:48:38 +0100 Subject: [PATCH 03/26] Checkpoint, more raws converted. --- packages/backend-core/src/sql/sql.ts | 69 ++++++++++--------- .../src/api/routes/tests/search.spec.ts | 7 +- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index ec22ac37f9..a7a1de2673 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -258,7 +258,9 @@ class InternalBuilder { const columnSchema = schema[column] if (this.SPECIAL_SELECT_CASES.POSTGRES_MONEY(columnSchema)) { - return this.knex.raw("??::money::numeric as ??", [ + // TODO: figure out how to express this safely without string + // interpolation. + return this.knex.raw(`??::money::numeric as "${field}"`, [ [table, column].join("."), field, ]) @@ -632,18 +634,16 @@ class InternalBuilder { } const like = (q: Knex.QueryBuilder, key: string, value: any) => { - const fuzzyOr = filters?.fuzzyOr - const fnc = fuzzyOr || allOr ? "orWhere" : "where" - // postgres supports ilike, nothing else does - if (this.client === SqlClient.POSTGRES) { - return q[fnc](key, "ilike", `%${value}%`) - } else { - const rawFnc = `${fnc}Raw` - // @ts-ignore - return q[rawFnc](`LOWER(${this.quotedIdentifier(key)}) LIKE ?`, [ - `%${value.toLowerCase()}%`, - ]) + if (filters?.fuzzyOr || allOr) { + q = q.or } + if ( + this.client === SqlClient.ORACLE || + this.client === SqlClient.SQL_LITE + ) { + return q.whereRaw(`LOWER(??) LIKE ?`, [key, `%${value.toLowerCase()}%`]) + } + return q.whereILike(key, this.knex.raw("?", [`%${value}%`])) } const contains = (mode: AnySearchFilter, any: boolean = false) => { @@ -1069,17 +1069,20 @@ class InternalBuilder { private buildJsonField(field: string): string { const parts = field.split(".") - let tableField: string, unaliased: string + let unaliased: string + + let tableField: string if (parts.length > 1) { const alias = parts.shift()! unaliased = parts.join(".") - tableField = `${this.quote(alias)}.${this.quote(unaliased)}` + tableField = `${alias}.${unaliased}` } else { unaliased = parts.join(".") - tableField = this.quote(unaliased) + tableField = unaliased } + const separator = this.client === SqlClient.ORACLE ? " VALUE " : "," - return `'${unaliased}'${separator}${tableField}` + return this.knex.raw(`?${separator}??`, [unaliased, tableField]).toString() } maxFunctionParameters() { @@ -1175,13 +1178,13 @@ class InternalBuilder { subQuery = subQuery.where( correlatedTo, "=", - knex.raw(this.quotedIdentifier(correlatedFrom)) + knex.raw("??", [correlatedFrom]) ) - const standardWrap = (select: string): Knex.QueryBuilder => { + const standardWrap = (select: Knex.Raw): Knex.QueryBuilder => { subQuery = subQuery.select(`${toAlias}.*`).limit(getRelationshipLimit()) // @ts-ignore - the from alias syntax isn't in Knex typing - return knex.select(knex.raw(select)).from({ + return knex.select(select).from({ [toAlias]: subQuery, }) } @@ -1191,12 +1194,12 @@ class InternalBuilder { // need to check the junction table document is to the right column, this is just for SQS subQuery = this.addJoinFieldCheck(subQuery, relationship) wrapperQuery = standardWrap( - `json_group_array(json_object(${fieldList}))` + this.knex.raw(`json_group_array(json_object(${fieldList}))`) ) break case SqlClient.POSTGRES: wrapperQuery = standardWrap( - `json_agg(json_build_object(${fieldList}))` + this.knex.raw(`json_agg(json_build_object(${fieldList}))`) ) break case SqlClient.MARIADB: @@ -1210,21 +1213,25 @@ class InternalBuilder { case SqlClient.MY_SQL: case SqlClient.ORACLE: wrapperQuery = standardWrap( - `json_arrayagg(json_object(${fieldList}))` + this.knex.raw(`json_arrayagg(json_object(${fieldList}))`) ) break - case SqlClient.MS_SQL: + case SqlClient.MS_SQL: { + const comparatorQuery = knex + .select(`${fromAlias}.*`) + // @ts-ignore - from alias syntax not TS supported + .from({ + [fromAlias]: subQuery + .select(`${toAlias}.*`) + .limit(getRelationshipLimit()), + }) + wrapperQuery = knex.raw( - `(SELECT ${this.quote(toAlias)} = (${knex - .select(`${fromAlias}.*`) - // @ts-ignore - from alias syntax not TS supported - .from({ - [fromAlias]: subQuery - .select(`${toAlias}.*`) - .limit(getRelationshipLimit()), - })} FOR JSON PATH))` + `(SELECT ?? = (${comparatorQuery} FOR JSON PATH))`, + [toAlias] ) break + } default: throw new Error(`JSON relationships not implement for ${sqlClient}`) } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 718ad47a31..74bf0eaf53 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -792,10 +792,11 @@ describe.each([ }) }) - describe.only.each([FieldType.STRING, FieldType.LONGFORM])("%s", () => { + const stringTypes = [FieldType.STRING, FieldType.LONGFORM] as const + describe.only.each(stringTypes)("%s", type => { beforeAll(async () => { tableOrViewId = await createTableOrView({ - name: { name: "name", type: FieldType.STRING }, + name: { name: "name", type }, }) await createRows([{ name: "foo" }, { name: "bar" }]) }) @@ -1602,7 +1603,7 @@ describe.each([ }) }) - describe.each([FieldType.ARRAY, FieldType.OPTIONS])("%s", () => { + describe("arrays", () => { beforeAll(async () => { tableOrViewId = await createTableOrView({ numbers: { From 56a68db1d4e9ed2046c25b0708afc37e1f3522db Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 22 Oct 2024 18:33:44 +0100 Subject: [PATCH 04/26] Checkpoint EOD: fixed a bunch more raw cases, some test failures to fix tomorrow. --- packages/backend-core/src/sql/sql.ts | 184 +++++++++++------- .../src/api/routes/tests/search.spec.ts | 4 +- packages/types/src/sdk/search.ts | 2 +- 3 files changed, 114 insertions(+), 76 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index a7a1de2673..20ef5fab59 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -13,6 +13,7 @@ import SqlTableQueryBuilder from "./sqlTable" import { Aggregation, AnySearchFilter, + ArrayFilter, ArrayOperator, BasicOperator, BBReferenceFieldMetadata, @@ -152,30 +153,24 @@ class InternalBuilder { return this.query.meta.table } + get knexClient(): Knex.Client { + return this.knex.client as Knex.Client + } + getFieldSchema(key: string): FieldSchema | undefined { const { column } = this.splitter.run(key) return this.table.schema[column] } private quoteChars(): [string, string] { - switch (this.client) { - case SqlClient.ORACLE: - case SqlClient.POSTGRES: - return ['"', '"'] - case SqlClient.MS_SQL: - return ["[", "]"] - case SqlClient.MARIADB: - case SqlClient.MY_SQL: - case SqlClient.SQL_LITE: - return ["`", "`"] - } + const wrapped = this.knexClient.wrapIdentifier("foo", {}) + return [wrapped[0], wrapped[wrapped.length - 1]] } - // Takes a string like foo and returns a quoted string like [foo] for SQL Server - // and "foo" for Postgres. + // Takes a string like foo and returns a quoted string like [foo] for SQL + // Server and "foo" for Postgres. private quote(str: string): string { - const [start, end] = this.quoteChars() - return `${start}${str}${end}` + return this.knexClient.wrapIdentifier(str, {}) } private isQuoted(key: string): boolean { @@ -193,6 +188,21 @@ class InternalBuilder { return key.map(part => this.quote(part)).join(".") } + // Unfortuantely we cannot rely on knex's identifier escaping because it trims + // the identifier string before escaping it, which breaks cases for us where + // columns that start or end with a space aren't referenced correctly anymore. + // + // So whenever you're using an identifier binding in knex, e.g. knex.raw("?? + // as ?", ["foo", "bar"]), you need to make sure you call this: + // + // knex.raw("?? as ?", [this.quotedIdentifier("foo"), "bar"]) + // + // Issue we filed against knex about this: + // https://github.com/knex/knex/issues/6143 + private rawQuotedIdentifier(key: string): Knex.Raw { + return this.knex.raw(this.quotedIdentifier(key)) + } + // Turns an identifier like a.b.c or `a`.`b`.`c` into ["a", "b", "c"] private splitIdentifier(key: string): string[] { const [start, end] = this.quoteChars() @@ -261,7 +271,7 @@ class InternalBuilder { // TODO: figure out how to express this safely without string // interpolation. return this.knex.raw(`??::money::numeric as "${field}"`, [ - [table, column].join("."), + this.rawQuotedIdentifier([table, column].join(".")), field, ]) } @@ -273,14 +283,14 @@ class InternalBuilder { // TODO: figure out how to express this safely without string // interpolation. return this.knex.raw(`CONVERT(varchar, ??, 108) as "${field}"`, [ - field, + this.rawQuotedIdentifier(field), ]) } if (table) { - return this.knex.raw("??", [`${table}.${column}`]) + return this.rawQuotedIdentifier(`${table}.${column}`) } else { - return this.knex.raw("??", [field]) + return this.rawQuotedIdentifier(field) } }) } @@ -300,7 +310,7 @@ class InternalBuilder { const parts = this.splitIdentifier(field) const col = parts.pop()! const schema = this.table.schema[col] - let identifier = this.knex.raw("??", [field]) + let identifier = this.rawQuotedIdentifier(field) if ( schema.type === FieldType.STRING || @@ -311,7 +321,10 @@ class InternalBuilder { schema.type === FieldType.BARCODEQR ) { if (opts?.forSelect) { - identifier = this.knex.raw("to_char(??) as ??", [identifier, col]) + identifier = this.knex.raw("to_char(??) as ??", [ + identifier, + this.rawQuotedIdentifier(col), + ]) } else { identifier = this.knex.raw("to_char(??)", [identifier]) } @@ -437,7 +450,6 @@ class InternalBuilder { filterKey: string, whereCb: (filterKey: string, query: Knex.QueryBuilder) => Knex.QueryBuilder ): Knex.QueryBuilder { - const mainKnex = this.knex const { relationships, endpoint, tableAliases: aliases } = this.query const tableName = endpoint.entityId const fromAlias = aliases?.[tableName] || tableName @@ -459,8 +471,8 @@ class InternalBuilder { relationship.to && relationship.tableName ) { - const joinTable = mainKnex - .select(mainKnex.raw(1)) + const joinTable = this.knex + .select(this.knex.raw(1)) .from({ [toAlias]: relatedTableName }) let subQuery = joinTable.clone() const manyToMany = validateManyToMany(relationship) @@ -495,7 +507,7 @@ class InternalBuilder { .where( `${throughAlias}.${manyToMany.from}`, "=", - mainKnex.raw(`??`, [`${fromAlias}.${manyToMany.fromPrimary}`]) + this.rawQuotedIdentifier(`${fromAlias}.${manyToMany.fromPrimary}`) ) // in SQS the same junction table is used for different many-to-many relationships between the // two same tables, this is needed to avoid rows ending up in all columns @@ -524,7 +536,7 @@ class InternalBuilder { subQuery = subQuery.where( toKey, "=", - mainKnex.raw("??", [foreignKey]) + this.rawQuotedIdentifier(foreignKey) ) query = query.where(q => { @@ -641,15 +653,30 @@ class InternalBuilder { this.client === SqlClient.ORACLE || this.client === SqlClient.SQL_LITE ) { - return q.whereRaw(`LOWER(??) LIKE ?`, [key, `%${value.toLowerCase()}%`]) + return q.whereRaw(`LOWER(??) LIKE ?`, [ + this.rawQuotedIdentifier(key), + `%${value.toLowerCase()}%`, + ]) } - return q.whereILike(key, this.knex.raw("?", [`%${value}%`])) + return q.whereILike( + // @ts-expect-error knex types are wrong, raw is fine here + this.rawQuotedIdentifier(key), + this.knex.raw("?", [`%${value}%`]) + ) } - const contains = (mode: AnySearchFilter, any: boolean = false) => { - const rawFnc = allOr ? "orWhereRaw" : "whereRaw" - const not = mode === filters?.notContains ? "NOT " : "" - function stringifyArray(value: Array, quoteStyle = '"'): string { + const contains = (mode: ArrayFilter, any = false) => { + function addModifiers(q: Knex.QueryBuilder) { + if (allOr || mode === filters?.containsAny) { + q = q.or + } + if (mode === filters?.notContains) { + q = q.not + } + return q + } + + function stringifyArray(value: any[], quoteStyle = '"'): string { for (let i in value) { if (typeof value[i] === "string") { value[i] = `${quoteStyle}${value[i]}${quoteStyle}` @@ -657,60 +684,64 @@ class InternalBuilder { } return `[${value.join(",")}]` } + if (this.client === SqlClient.POSTGRES) { iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { const wrap = any ? "" : "'" const op = any ? "\\?| array" : "@>" - const fieldNames = key.split(/\./g) - const table = fieldNames[0] - const col = fieldNames[1] - return q[rawFnc]( - `${not}COALESCE("${table}"."${col}"::jsonb ${op} ${wrap}${stringifyArray( - value, - any ? "'" : '"' - )}${wrap}, FALSE)` + + const stringifiedArray = stringifyArray(value, any ? "'" : '"') + return addModifiers(q).whereRaw( + `COALESCE(??::jsonb ${op} ${wrap}${stringifiedArray}${wrap}, FALSE)`, + [this.rawQuotedIdentifier(key)] ) }) } else if ( this.client === SqlClient.MY_SQL || this.client === SqlClient.MARIADB ) { - const jsonFnc = any ? "JSON_OVERLAPS" : "JSON_CONTAINS" iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { - return q[rawFnc]( - `${not}COALESCE(${jsonFnc}(${key}, '${stringifyArray( - value - )}'), FALSE)` - ) + return addModifiers(q).whereRaw(`COALESCE(?(??, ?), FALSE)`, [ + this.knex.raw(any ? "JSON_OVERLAPS" : "JSON_CONTAINS"), + this.rawQuotedIdentifier(key), + stringifyArray(value), + ]) }) } else { - const andOr = mode === filters?.containsAny ? " OR " : " AND " iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { - let statement = "" - const identifier = this.quotedIdentifier(key) - for (let i in value) { - if (typeof value[i] === "string") { - value[i] = `%"${value[i].toLowerCase()}"%` - } else { - value[i] = `%${value[i]}%` - } - statement += `${ - statement ? andOr : "" - }COALESCE(LOWER(${identifier}), '') LIKE ?` - } - - if (statement === "") { + if (value.length === 0) { return q } - if (not) { - return q[rawFnc]( - `(NOT (${statement}) OR ${identifier} IS NULL)`, - value - ) - } else { - return q[rawFnc](statement, value) + q = addModifiers(q).where(subQuery => { + subQuery.where(subSubQuery => { + for (const elem of value) { + if (mode === filters?.containsAny) { + subSubQuery = subSubQuery.or + } else { + subSubQuery = subSubQuery.and + } + + const lower = + typeof elem === "string" ? `"${elem.toLowerCase()}"` : elem + + subSubQuery = subSubQuery.whereLike( + // @ts-expect-error knex types are wrong, raw is fine here + this.knex.raw(`COALESCE(LOWER(??), '')`, [ + this.rawQuotedIdentifier(key), + ]), + `%${lower}%` + ) + } + }) + }) + + if (mode === filters?.notContains) { + // @ts-expect-error knex types are wrong, raw is fine here + q.or.whereNull(this.rawQuotedIdentifier(key)) } + + return q }) } } @@ -764,7 +795,10 @@ class InternalBuilder { return q[fnc](key, "ilike", `${value}%`) } else { const fnc = allOr ? "orWhereRaw" : "whereRaw" - return q[fnc](`LOWER(??) LIKE ?`, [key, `${value.toLowerCase()}%`]) + return q[fnc](`LOWER(??) LIKE ?`, [ + this.rawQuotedIdentifier(key), + `${value.toLowerCase()}%`, + ]) } }) } @@ -801,7 +835,9 @@ class InternalBuilder { this.client === SqlClient.SQL_LITE && schema?.type === FieldType.BIGINT ) { - rawKey = this.knex.raw("CAST(?? AS INTEGER)", [key]) + rawKey = this.knex.raw("CAST(?? AS INTEGER)", [ + this.rawQuotedIdentifier(key), + ]) high = this.knex.raw("CAST(? AS INTEGER)", [value.high]) low = this.knex.raw("CAST(? AS INTEGER)", [value.low]) } @@ -1082,7 +1118,9 @@ class InternalBuilder { } const separator = this.client === SqlClient.ORACLE ? " VALUE " : "," - return this.knex.raw(`?${separator}??`, [unaliased, tableField]).toString() + return this.knex + .raw(`?${separator}??`, [unaliased, this.rawQuotedIdentifier(tableField)]) + .toString() } maxFunctionParameters() { @@ -1178,7 +1216,7 @@ class InternalBuilder { subQuery = subQuery.where( correlatedTo, "=", - knex.raw("??", [correlatedFrom]) + this.rawQuotedIdentifier(correlatedFrom) ) const standardWrap = (select: Knex.Raw): Knex.QueryBuilder => { @@ -1228,7 +1266,7 @@ class InternalBuilder { wrapperQuery = knex.raw( `(SELECT ?? = (${comparatorQuery} FOR JSON PATH))`, - [toAlias] + [this.rawQuotedIdentifier(toAlias)] ) break } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 74bf0eaf53..146791a268 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -691,7 +691,7 @@ describe.each([ ]) }) - it("should not match the session user id in a multi user field", async () => { + it.only("should not match the session user id in a multi user field", async () => { await expectQuery({ notContains: { multi_user: ["{{ [user]._id }}"] }, notEmpty: { multi_user: true }, @@ -793,7 +793,7 @@ describe.each([ }) const stringTypes = [FieldType.STRING, FieldType.LONGFORM] as const - describe.only.each(stringTypes)("%s", type => { + describe.each(stringTypes)("%s", type => { beforeAll(async () => { tableOrViewId = await createTableOrView({ name: { name: "name", type }, diff --git a/packages/types/src/sdk/search.ts b/packages/types/src/sdk/search.ts index d41bb0fb99..d700b1b41d 100644 --- a/packages/types/src/sdk/search.ts +++ b/packages/types/src/sdk/search.ts @@ -49,7 +49,7 @@ type BasicFilter = Record & { [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: never } -type ArrayFilter = Record & { +export type ArrayFilter = Record & { [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: { id: string[] values: string[] From 71671ad62ab7735bcb739554b9f90a76a1829d96 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 23 Oct 2024 14:24:24 +0200 Subject: [PATCH 05/26] Update pro submodule --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index 297fdc937e..6041196305 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 297fdc937e9c650b4964fc1a942b60022b195865 +Subproject commit 60411963053b98e0415a1e9285b721fc26c628c8 From 67d2e0cf61dfd09df3be98ecb13be6dbe3300d6e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 23 Oct 2024 14:34:39 +0200 Subject: [PATCH 06/26] Port changes from PR #14846 --- .../src/api/controllers/global/configs.ts | 56 +++++++++++-------- .../controllers/global/tests/configs.spec.ts | 4 -- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/packages/worker/src/api/controllers/global/configs.ts b/packages/worker/src/api/controllers/global/configs.ts index e6e80ff3a5..750b02088c 100644 --- a/packages/worker/src/api/controllers/global/configs.ts +++ b/packages/worker/src/api/controllers/global/configs.ts @@ -44,9 +44,7 @@ const getEventFns = async (config: Config, existing?: Config) => { fns.push(events.email.SMTPCreated) } else if (isAIConfig(config)) { fns.push(() => events.ai.AIConfigCreated) - fns.push(() => - pro.quotas.updateCustomAIConfigCount(Object.keys(config.config).length) - ) + fns.push(() => pro.quotas.addCustomAIConfig()) } else if (isGoogleConfig(config)) { fns.push(() => events.auth.SSOCreated(ConfigType.GOOGLE)) if (config.config.activated) { @@ -85,9 +83,6 @@ const getEventFns = async (config: Config, existing?: Config) => { fns.push(events.email.SMTPUpdated) } else if (isAIConfig(config)) { fns.push(() => events.ai.AIConfigUpdated) - fns.push(() => - pro.quotas.updateCustomAIConfigCount(Object.keys(config.config).length) - ) } else if (isGoogleConfig(config)) { fns.push(() => events.auth.SSOUpdated(ConfigType.GOOGLE)) if (!existing.config.activated && config.config.activated) { @@ -253,7 +248,7 @@ export async function save(ctx: UserCtx) { if (existingConfig) { await verifyAIConfig(config, existingConfig) } - await pro.quotas.updateCustomAIConfigCount(Object.keys(config).length) + await pro.quotas.addCustomAIConfig() break } } catch (err: any) { @@ -342,29 +337,43 @@ export async function find(ctx: UserCtx) { let scopedConfig = await configs.getConfig(type) if (scopedConfig) { - if (type === ConfigType.OIDC_LOGOS) { - enrichOIDCLogos(scopedConfig) - } - - if (type === ConfigType.AI) { - await pro.sdk.ai.enrichAIConfig(scopedConfig) - // Strip out the API Keys from the response so they don't show in the UI - for (const key in scopedConfig.config) { - if (scopedConfig.config[key].apiKey) { - scopedConfig.config[key].apiKey = PASSWORD_REPLACEMENT - } - } - } - ctx.body = scopedConfig + await handleConfigType(type, scopedConfig) + } else if (type === ConfigType.AI) { + scopedConfig = { config: {} } as AIConfig + await handleAIConfig(scopedConfig) } else { - // don't throw an error, there simply is nothing to return + // If no config found and not AI type, just return an empty body ctx.body = {} + return } + + ctx.body = scopedConfig } catch (err: any) { ctx.throw(err?.status || 400, err) } } +async function handleConfigType(type: ConfigType, config: Config) { + if (type === ConfigType.OIDC_LOGOS) { + enrichOIDCLogos(config) + } else if (type === ConfigType.AI) { + await handleAIConfig(config) + } +} + +async function handleAIConfig(config: AIConfig) { + await pro.sdk.ai.enrichAIConfig(config) + stripApiKeys(config) +} + +function stripApiKeys(config: AIConfig) { + for (const key in config?.config) { + if (config.config[key].apiKey) { + config.config[key].apiKey = PASSWORD_REPLACEMENT + } + } +} + export async function publicOidc(ctx: Ctx) { try { // Find the config with the most granular scope based on context @@ -508,6 +517,9 @@ export async function destroy(ctx: UserCtx) { try { await db.remove(id, rev) await cache.destroy(cache.CacheKey.CHECKLIST) + if (id === configs.generateConfigID(ConfigType.AI)) { + await pro.quotas.removeCustomAIConfig() + } ctx.body = { message: "Config deleted successfully" } } catch (err: any) { ctx.throw(err.status, err) diff --git a/packages/worker/src/api/controllers/global/tests/configs.spec.ts b/packages/worker/src/api/controllers/global/tests/configs.spec.ts index 9091f29247..857f499dcc 100644 --- a/packages/worker/src/api/controllers/global/tests/configs.spec.ts +++ b/packages/worker/src/api/controllers/global/tests/configs.spec.ts @@ -13,10 +13,6 @@ describe("Global configs controller", () => { await config.afterAll() }) - afterEach(() => { - jest.resetAllMocks() - }) - it("Should strip secrets when pulling AI config", async () => { const data = structures.configs.ai() await config.api.configs.saveConfig(data) From 309506adab54ca89cb0612650afbddedcf8761b1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 15:05:41 +0100 Subject: [PATCH 07/26] wip --- .../server/src/api/routes/tests/search.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 146791a268..9aa482fb6c 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -48,13 +48,13 @@ import { generateRowIdField } from "../../../integrations/utils" import { cloneDeep } from "lodash/fp" describe.each([ - ["in-memory", undefined], - ["lucene", undefined], - ["sqs", undefined], - [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + // ["in-memory", undefined], + // ["lucene", undefined], + // ["sqs", undefined], + // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" From aaf4022f25b4998bf69066d9e7f8749fa6f2cbef Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 16:22:07 +0100 Subject: [PATCH 08/26] Finally fix notContains tests. --- packages/backend-core/src/sql/sql.ts | 24 +++++++++++++------ .../src/api/routes/tests/search.spec.ts | 16 ++++++------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 20ef5fab59..69e5391eb2 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -713,7 +713,11 @@ class InternalBuilder { return q } - q = addModifiers(q).where(subQuery => { + q = q.where(subQuery => { + if (mode === filters?.notContains) { + subQuery = subQuery.not + } + subQuery.where(subSubQuery => { for (const elem of value) { if (mode === filters?.containsAny) { @@ -733,13 +737,19 @@ class InternalBuilder { `%${lower}%` ) } - }) - }) - if (mode === filters?.notContains) { - // @ts-expect-error knex types are wrong, raw is fine here - q.or.whereNull(this.rawQuotedIdentifier(key)) - } + return subSubQuery + }) + + if (mode === filters?.notContains) { + subQuery = subQuery.or.whereNull( + // @ts-expect-error knex types are wrong, raw is fine here + this.rawQuotedIdentifier(key) + ) + } + + return subQuery + }) return q }) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 9aa482fb6c..b319248ff7 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -48,13 +48,13 @@ import { generateRowIdField } from "../../../integrations/utils" import { cloneDeep } from "lodash/fp" describe.each([ - // ["in-memory", undefined], - // ["lucene", undefined], - // ["sqs", undefined], - // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + ["in-memory", undefined], + ["lucene", undefined], + ["sqs", undefined], + [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("search (%s)", (name, dsProvider) => { const isSqs = name === "sqs" @@ -691,7 +691,7 @@ describe.each([ ]) }) - it.only("should not match the session user id in a multi user field", async () => { + it("should not match the session user id in a multi user field", async () => { await expectQuery({ notContains: { multi_user: ["{{ [user]._id }}"] }, notEmpty: { multi_user: true }, From a120ce4e144d9ab859d8ef750f5a347b464c2ec0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 17:07:42 +0100 Subject: [PATCH 09/26] More refactoring. --- packages/backend-core/src/sql/sql.ts | 83 +++++++++++++++++----------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 69e5391eb2..b331a4da61 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -162,6 +162,12 @@ class InternalBuilder { return this.table.schema[column] } + private supportsILike(): boolean { + return !( + this.client === SqlClient.ORACLE || this.client === SqlClient.SQL_LITE + ) + } + private quoteChars(): [string, string] { const wrapped = this.knexClient.wrapIdentifier("foo", {}) return [wrapped[0], wrapped[wrapped.length - 1]] @@ -566,7 +572,7 @@ class InternalBuilder { filters = this.parseFilters({ ...filters }) const aliases = this.query.tableAliases // if all or specified in filters, then everything is an or - const allOr = filters.allOr + const shouldOr = filters.allOr const isSqlite = this.client === SqlClient.SQL_LITE const tableName = isSqlite ? this.table._id! : this.table.name @@ -630,7 +636,7 @@ class InternalBuilder { value ) } else if (shouldProcessRelationship) { - if (allOr) { + if (shouldOr) { query = query.or } query = builder.addRelationshipForFilter( @@ -646,7 +652,7 @@ class InternalBuilder { } const like = (q: Knex.QueryBuilder, key: string, value: any) => { - if (filters?.fuzzyOr || allOr) { + if (filters?.fuzzyOr || shouldOr) { q = q.or } if ( @@ -667,7 +673,7 @@ class InternalBuilder { const contains = (mode: ArrayFilter, any = false) => { function addModifiers(q: Knex.QueryBuilder) { - if (allOr || mode === filters?.containsAny) { + if (shouldOr || mode === filters?.containsAny) { q = q.or } if (mode === filters?.notContains) { @@ -777,38 +783,46 @@ class InternalBuilder { } if (filters.oneOf) { - const fnc = allOr ? "orWhereIn" : "whereIn" iterate( filters.oneOf, ArrayOperator.ONE_OF, (q, key: string, array) => { + if (shouldOr) { + q = q.or + } if (this.client === SqlClient.ORACLE) { // @ts-ignore key = this.convertClobs(key) } - return q[fnc](key, Array.isArray(array) ? array : [array]) + return q.whereIn(key, Array.isArray(array) ? array : [array]) }, (q, key: string[], array) => { + if (shouldOr) { + q = q.or + } if (this.client === SqlClient.ORACLE) { // @ts-ignore key = key.map(k => this.convertClobs(k)) } - return q[fnc](key, Array.isArray(array) ? array : [array]) + return q.whereIn(key, Array.isArray(array) ? array : [array]) } ) } if (filters.string) { iterate(filters.string, BasicOperator.STRING, (q, key, value) => { - // postgres supports ilike, nothing else does - if (this.client === SqlClient.POSTGRES) { - const fnc = allOr ? "orWhere" : "where" - return q[fnc](key, "ilike", `${value}%`) - } else { - const fnc = allOr ? "orWhereRaw" : "whereRaw" - return q[fnc](`LOWER(??) LIKE ?`, [ + if (shouldOr) { + q = q.or + } + if ( + this.client === SqlClient.ORACLE || + this.client === SqlClient.SQL_LITE + ) { + return q.whereRaw(`LOWER(??) LIKE ?`, [ this.rawQuotedIdentifier(key), `${value.toLowerCase()}%`, ]) + } else { + return q.whereILike(key, `${value}%`) } }) } @@ -852,37 +866,42 @@ class InternalBuilder { low = this.knex.raw("CAST(? AS INTEGER)", [value.low]) } + if (shouldOr) { + q = q.or + } + if (lowValid && highValid) { - const fnc = allOr ? "orWhereBetween" : "whereBetween" // @ts-ignore - return q[fnc](rawKey, [low, high]) + return q.whereBetween(rawKey, [low, high]) } else if (lowValid) { - const fnc = allOr ? "orWhere" : "where" // @ts-ignore - return q[fnc](rawKey, ">=", low) + return q.where(rawKey, ">=", low) } else if (highValid) { - const fnc = allOr ? "orWhere" : "where" // @ts-ignore - return q[fnc](rawKey, "<=", high) + return q.where(rawKey, "<=", high) } return q }) } if (filters.equal) { iterate(filters.equal, BasicOperator.EQUAL, (q, key, value) => { - const fnc = allOr ? "orWhereRaw" : "whereRaw" + if (shouldOr) { + q = q.or + } if (this.client === SqlClient.MS_SQL) { - return q[fnc]( - `CASE WHEN ${this.quotedIdentifier(key)} = ? THEN 1 ELSE 0 END = 1`, - [value] - ) - } else if (this.client === SqlClient.ORACLE) { - const identifier = this.convertClobs(key) - return q[fnc](`(${identifier} IS NOT NULL AND ${identifier} = ?)`, [ + return q.whereRaw(`CASE WHEN ?? = ? THEN 1 ELSE 0 END = 1`, [ + this.quotedIdentifier(key), value, ]) + } else if (this.client === SqlClient.ORACLE) { + const identifier = this.convertClobs(key) + return q.where(subq => + // @ts-expect-error knex types are wrong, raw is fine here + subq.whereNotNull(identifier).andWhere(identifier, value) + ) } else { - return q[fnc](`COALESCE(${this.quotedIdentifier(key)} = ?, FALSE)`, [ + return q.whereRaw(`COALESCE(?? = ?, FALSE)`, [ + this.rawQuotedIdentifier(key), value, ]) } @@ -890,7 +909,7 @@ class InternalBuilder { } if (filters.notEqual) { iterate(filters.notEqual, BasicOperator.NOT_EQUAL, (q, key, value) => { - const fnc = allOr ? "orWhereRaw" : "whereRaw" + const fnc = shouldOr ? "orWhereRaw" : "whereRaw" if (this.client === SqlClient.MS_SQL) { return q[fnc]( `CASE WHEN ${this.quotedIdentifier(key)} = ? THEN 1 ELSE 0 END = 0`, @@ -911,13 +930,13 @@ class InternalBuilder { } if (filters.empty) { iterate(filters.empty, BasicOperator.EMPTY, (q, key) => { - const fnc = allOr ? "orWhereNull" : "whereNull" + const fnc = shouldOr ? "orWhereNull" : "whereNull" return q[fnc](key) }) } if (filters.notEmpty) { iterate(filters.notEmpty, BasicOperator.NOT_EMPTY, (q, key) => { - const fnc = allOr ? "orWhereNotNull" : "whereNotNull" + const fnc = shouldOr ? "orWhereNotNull" : "whereNotNull" return q[fnc](key) }) } From ebcbadfd3ae3fd53e5ddabfc73fff18cdd26dcb3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 17:21:14 +0100 Subject: [PATCH 10/26] remove all of the `fnc` variables --- packages/backend-core/src/sql/sql.ts | 59 ++++++++++--------- .../src/api/routes/tests/search.spec.ts | 2 +- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b331a4da61..b579454f3a 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -719,12 +719,12 @@ class InternalBuilder { return q } - q = q.where(subQuery => { + return q.where(subQuery => { if (mode === filters?.notContains) { subQuery = subQuery.not } - subQuery.where(subSubQuery => { + return subQuery.where(subSubQuery => { for (const elem of value) { if (mode === filters?.containsAny) { subSubQuery = subSubQuery.or @@ -743,21 +743,8 @@ class InternalBuilder { `%${lower}%` ) } - - return subSubQuery }) - - if (mode === filters?.notContains) { - subQuery = subQuery.or.whereNull( - // @ts-expect-error knex types are wrong, raw is fine here - this.rawQuotedIdentifier(key) - ) - } - - return subQuery }) - - return q }) } } @@ -890,7 +877,7 @@ class InternalBuilder { } if (this.client === SqlClient.MS_SQL) { return q.whereRaw(`CASE WHEN ?? = ? THEN 1 ELSE 0 END = 1`, [ - this.quotedIdentifier(key), + this.rawQuotedIdentifier(key), value, ]) } else if (this.client === SqlClient.ORACLE) { @@ -909,20 +896,30 @@ class InternalBuilder { } if (filters.notEqual) { iterate(filters.notEqual, BasicOperator.NOT_EQUAL, (q, key, value) => { - const fnc = shouldOr ? "orWhereRaw" : "whereRaw" + if (shouldOr) { + q = q.or + } if (this.client === SqlClient.MS_SQL) { - return q[fnc]( - `CASE WHEN ${this.quotedIdentifier(key)} = ? THEN 1 ELSE 0 END = 0`, - [value] - ) + return q.whereRaw(`CASE WHEN ?? = ? THEN 1 ELSE 0 END = 0`, [ + this.rawQuotedIdentifier(key), + value, + ]) } else if (this.client === SqlClient.ORACLE) { const identifier = this.convertClobs(key) - return q[fnc]( - `(${identifier} IS NOT NULL AND ${identifier} != ?) OR ${identifier} IS NULL`, - [value] + return ( + q + .where(subq => + subq.not + // @ts-expect-error knex types are wrong, raw is fine here + .whereNull(identifier) + .and.where(identifier, "!=", value) + ) + // @ts-expect-error knex types are wrong, raw is fine here + .or.whereNull(identifier) ) } else { - return q[fnc](`COALESCE(${this.quotedIdentifier(key)} != ?, TRUE)`, [ + return q.whereRaw(`COALESCE(?? != ?, TRUE)`, [ + this.rawQuotedIdentifier(key), value, ]) } @@ -930,14 +927,18 @@ class InternalBuilder { } if (filters.empty) { iterate(filters.empty, BasicOperator.EMPTY, (q, key) => { - const fnc = shouldOr ? "orWhereNull" : "whereNull" - return q[fnc](key) + if (shouldOr) { + q = q.or + } + return q.whereNull(key) }) } if (filters.notEmpty) { iterate(filters.notEmpty, BasicOperator.NOT_EMPTY, (q, key) => { - const fnc = shouldOr ? "orWhereNotNull" : "whereNotNull" - return q[fnc](key) + if (shouldOr) { + q = q.or + } + return q.whereNotNull(key) }) } if (filters.contains) { diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index b319248ff7..74bf0eaf53 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -793,7 +793,7 @@ describe.each([ }) const stringTypes = [FieldType.STRING, FieldType.LONGFORM] as const - describe.each(stringTypes)("%s", type => { + describe.only.each(stringTypes)("%s", type => { beforeAll(async () => { tableOrViewId = await createTableOrView({ name: { name: "name", type }, From 0695888659f0637218c725e04dffdc0acadf29e7 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 11:01:35 +0100 Subject: [PATCH 11/26] wip --- packages/backend-core/src/sql/sql.ts | 57 +++++++++++++------ .../src/api/routes/tests/search.spec.ts | 4 +- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index b579454f3a..684f276145 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -99,6 +99,23 @@ function isSqs(table: Table): boolean { ) } +function escapeQuotes(value: string, quoteChar = '"'): string { + return value.replace(new RegExp(quoteChar, "g"), `${quoteChar}${quoteChar}`) +} + +function wrap(value: string, quoteChar = '"'): string { + return `${quoteChar}${escapeQuotes(value, quoteChar)}${quoteChar}` +} + +function stringifyArray(value: any[], quoteStyle = '"'): string { + for (let i in value) { + if (typeof value[i] === "string") { + value[i] = wrap(value[i], quoteStyle) + } + } + return `[${value.join(",")}]` +} + const allowEmptyRelationships: Record = { [BasicOperator.EQUAL]: false, [BasicOperator.NOT_EQUAL]: true, @@ -194,6 +211,15 @@ class InternalBuilder { return key.map(part => this.quote(part)).join(".") } + private quotedValue(value: string): string { + const formatter = this.knexClient.formatter(this.knexClient.queryBuilder()) + return formatter.wrap(value, false) + } + + private rawQuotedValue(value: string): Knex.Raw { + return this.knex.raw(this.quotedValue(value)) + } + // Unfortuantely we cannot rely on knex's identifier escaping because it trims // the identifier string before escaping it, which breaks cases for us where // columns that start or end with a space aren't referenced correctly anymore. @@ -682,25 +708,20 @@ class InternalBuilder { return q } - function stringifyArray(value: any[], quoteStyle = '"'): string { - for (let i in value) { - if (typeof value[i] === "string") { - value[i] = `${quoteStyle}${value[i]}${quoteStyle}` - } - } - return `[${value.join(",")}]` - } - if (this.client === SqlClient.POSTGRES) { iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => { - const wrap = any ? "" : "'" - const op = any ? "\\?| array" : "@>" - - const stringifiedArray = stringifyArray(value, any ? "'" : '"') - return addModifiers(q).whereRaw( - `COALESCE(??::jsonb ${op} ${wrap}${stringifiedArray}${wrap}, FALSE)`, - [this.rawQuotedIdentifier(key)] - ) + q = addModifiers(q) + if (any) { + return q.whereRaw(`COALESCE(??::jsonb \\?| array??, FALSE)`, [ + this.rawQuotedIdentifier(key), + this.knex.raw(stringifyArray(value, "'")), + ]) + } else { + return q.whereRaw(`COALESCE(??::jsonb @> '??', FALSE)`, [ + this.rawQuotedIdentifier(key), + this.knex.raw(stringifyArray(value)), + ]) + } }) } else if ( this.client === SqlClient.MY_SQL || @@ -710,7 +731,7 @@ class InternalBuilder { return addModifiers(q).whereRaw(`COALESCE(?(??, ?), FALSE)`, [ this.knex.raw(any ? "JSON_OVERLAPS" : "JSON_CONTAINS"), this.rawQuotedIdentifier(key), - stringifyArray(value), + this.knex.raw(wrap(stringifyArray(value))), ]) }) } else { diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 74bf0eaf53..b22b1545be 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -793,7 +793,7 @@ describe.each([ }) const stringTypes = [FieldType.STRING, FieldType.LONGFORM] as const - describe.only.each(stringTypes)("%s", type => { + describe.each(stringTypes)("%s", type => { beforeAll(async () => { tableOrViewId = await createTableOrView({ name: { name: "name", type }, @@ -2163,7 +2163,7 @@ describe.each([ }) }) - describe("multi user", () => { + describe.only("multi user", () => { let user1: User let user2: User From e14918c105185e751e05b72e88d1b1ccea14feb7 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 11:20:31 +0100 Subject: [PATCH 12/26] Fix notContains tests again. --- packages/backend-core/src/sql/sql.ts | 12 ++++++++++-- packages/server/src/api/routes/tests/search.spec.ts | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 684f276145..8922ce055b 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -740,12 +740,12 @@ class InternalBuilder { return q } - return q.where(subQuery => { + q = q.where(subQuery => { if (mode === filters?.notContains) { subQuery = subQuery.not } - return subQuery.where(subSubQuery => { + subQuery = subQuery.where(subSubQuery => { for (const elem of value) { if (mode === filters?.containsAny) { subSubQuery = subSubQuery.or @@ -765,7 +765,15 @@ class InternalBuilder { ) } }) + if (mode === filters?.notContains) { + subQuery = subQuery.or.whereNull( + // @ts-expect-error knex types are wrong, raw is fine here + this.rawQuotedIdentifier(key) + ) + } + return subQuery }) + return q }) } } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index b22b1545be..b319248ff7 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -2163,7 +2163,7 @@ describe.each([ }) }) - describe.only("multi user", () => { + describe("multi user", () => { let user1: User let user2: User From 0736812293361ff53936a82f58682c0298aae9e3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 11:39:57 +0100 Subject: [PATCH 13/26] Add SQL injection tests. --- packages/backend-core/src/sql/sql.ts | 14 +++--- .../src/api/routes/tests/search.spec.ts | 50 +++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 8922ce055b..883f7a4d5f 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -887,13 +887,13 @@ class InternalBuilder { } if (lowValid && highValid) { - // @ts-ignore + // @ts-expect-error knex types are wrong, raw is fine here return q.whereBetween(rawKey, [low, high]) } else if (lowValid) { - // @ts-ignore + // @ts-expect-error knex types are wrong, raw is fine here return q.where(rawKey, ">=", low) } else if (highValid) { - // @ts-ignore + // @ts-expect-error knex types are wrong, raw is fine here return q.where(rawKey, "<=", high) } return q @@ -1132,9 +1132,11 @@ class InternalBuilder { } else { let composite = `${aliased}.${key}` if (this.client === SqlClient.ORACLE) { - query = query.orderByRaw( - `${this.convertClobs(composite)} ${direction} nulls ${nulls}` - ) + query = query.orderByRaw(`?? ?? nulls ??`, [ + this.convertClobs(composite), + direction, + nulls, + ]) } else { query = query.orderBy(composite, direction, nulls) } diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index b319248ff7..dfee63fa4d 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -3471,5 +3471,55 @@ describe.each([ ]) }) }) + + describe("SQL injection", () => { + const badStrings = [ + "1; DROP TABLE test;", + "1; DELETE FROM test;", + "1; UPDATE test SET name = 'foo';", + "1; INSERT INTO test (name) VALUES ('foo');", + "' OR '1'='1' --", + "'; DROP TABLE users; --", + "' OR 1=1 --", + "' UNION SELECT null, null, null; --", + "' AND (SELECT COUNT(*) FROM users) > 0 --", + "\"; EXEC xp_cmdshell('dir'); --", + "\"' OR 'a'='a", + "OR 1=1;", + "'; SHUTDOWN --", + ] + + describe.only.each(badStrings)("bad string: %s", badString => { + it("should not allow SQL injection as a field name", async () => { + const tableOrViewId = await createTableOrView({ + [badString]: { + name: badString, + type: FieldType.STRING, + }, + }) + + await config.api.row.search( + tableOrViewId, + { query: {} }, + { status: 200 } + ) + }) + + it("should not allow SQL injection as a field value", async () => { + const tableOrViewId = await createTableOrView({ + foo: { + name: "foo", + type: FieldType.STRING, + }, + }) + + await config.api.row.search( + tableOrViewId, + { query: { equal: { foo: badString } } }, + { status: 200 } + ) + }) + }) + }) }) }) From 478160c412cf4c008e3b581d7ac7bbf78d4072f3 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 12:28:23 +0100 Subject: [PATCH 14/26] Fix all tests. --- packages/backend-core/src/sql/sql.ts | 4 +- .../src/api/routes/tests/search.spec.ts | 142 ++++++++++-------- 2 files changed, 81 insertions(+), 65 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 883f7a4d5f..d94b817ba4 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -1134,8 +1134,8 @@ class InternalBuilder { if (this.client === SqlClient.ORACLE) { query = query.orderByRaw(`?? ?? nulls ??`, [ this.convertClobs(composite), - direction, - nulls, + this.knex.raw(direction), + this.knex.raw(nulls as string), ]) } else { query = query.orderBy(composite, direction, nulls) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index dfee63fa4d..2f3dbe91f6 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -61,6 +61,7 @@ describe.each([ const isLucene = name === "lucene" const isInMemory = name === "in-memory" const isInternal = isSqs || isLucene || isInMemory + const isOracle = name === DatabaseName.ORACLE const isSql = !isInMemory && !isLucene const config = setup.getConfig() @@ -155,24 +156,24 @@ describe.each([ describe.each([ ["table", createTable], - // [ - // "view", - // async (schema: TableSchema) => { - // const tableId = await createTable(schema) - // const viewId = await createView( - // tableId, - // Object.keys(schema).reduce((viewSchema, fieldName) => { - // const field = schema[fieldName] - // viewSchema[fieldName] = { - // visible: field.visible ?? true, - // readonly: false, - // } - // return viewSchema - // }, {}) - // ) - // return viewId - // }, - // ], + [ + "view", + async (schema: TableSchema) => { + const tableId = await createTable(schema) + const viewId = await createView( + tableId, + Object.keys(schema).reduce((viewSchema, fieldName) => { + const field = schema[fieldName] + viewSchema[fieldName] = { + visible: field.visible ?? true, + readonly: false, + } + return viewSchema + }, {}) + ) + return viewId + }, + ], ])("from %s", (sourceType, createTableOrView) => { const isView = sourceType === "view" @@ -3472,54 +3473,69 @@ describe.each([ }) }) - describe("SQL injection", () => { - const badStrings = [ - "1; DROP TABLE test;", - "1; DELETE FROM test;", - "1; UPDATE test SET name = 'foo';", - "1; INSERT INTO test (name) VALUES ('foo');", - "' OR '1'='1' --", - "'; DROP TABLE users; --", - "' OR 1=1 --", - "' UNION SELECT null, null, null; --", - "' AND (SELECT COUNT(*) FROM users) > 0 --", - "\"; EXEC xp_cmdshell('dir'); --", - "\"' OR 'a'='a", - "OR 1=1;", - "'; SHUTDOWN --", - ] + isSql && + describe("SQL injection", () => { + const badStrings = [ + "1; DROP TABLE test;", + "1; DELETE FROM test;", + "1; UPDATE test SET name = 'foo';", + "1; INSERT INTO test (name) VALUES ('foo');", + "' OR '1'='1' --", + "'; DROP TABLE users; --", + "' OR 1=1 --", + "' UNION SELECT null, null, null; --", + "' AND (SELECT COUNT(*) FROM users) > 0 --", + "\"; EXEC xp_cmdshell('dir'); --", + "\"' OR 'a'='a", + "OR 1=1;", + "'; SHUTDOWN --", + ] - describe.only.each(badStrings)("bad string: %s", badString => { - it("should not allow SQL injection as a field name", async () => { - const tableOrViewId = await createTableOrView({ - [badString]: { - name: badString, - type: FieldType.STRING, - }, + describe.each(badStrings)("bad string: %s", badString => { + // The SQL that knex generates when you try to use a double quote in a + // field name is always invalid and never works, so we skip it for these + // tests. + const skipFieldNameCheck = isOracle && badString.includes('"') + + !skipFieldNameCheck && + it("should not allow SQL injection as a field name", async () => { + const tableOrViewId = await createTableOrView({ + [badString]: { + name: badString, + type: FieldType.STRING, + }, + }) + + await config.api.row.save(tableOrViewId, { [badString]: "foo" }) + + const { rows } = await config.api.row.search( + tableOrViewId, + { query: {} }, + { status: 200 } + ) + + expect(rows).toHaveLength(1) + }) + + it("should not allow SQL injection as a field value", async () => { + const tableOrViewId = await createTableOrView({ + foo: { + name: "foo", + type: FieldType.STRING, + }, + }) + + await config.api.row.save(tableOrViewId, { foo: "foo" }) + + const { rows } = await config.api.row.search( + tableOrViewId, + { query: { equal: { foo: badString } } }, + { status: 200 } + ) + + expect(rows).toBeEmpty() }) - - await config.api.row.search( - tableOrViewId, - { query: {} }, - { status: 200 } - ) - }) - - it("should not allow SQL injection as a field value", async () => { - const tableOrViewId = await createTableOrView({ - foo: { - name: "foo", - type: FieldType.STRING, - }, - }) - - await config.api.row.search( - tableOrViewId, - { query: { equal: { foo: badString } } }, - { status: 200 } - ) }) }) - }) }) }) From e54bb3fbdc76cbe4252dea99a86669d46ac26344 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 12:33:32 +0100 Subject: [PATCH 15/26] Uncomment view tests. --- packages/server/src/api/routes/tests/viewV2.spec.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 0c18307909..dfd4f50bd1 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -35,12 +35,12 @@ import { quotas } from "@budibase/pro" import { db, roles, features } from "@budibase/backend-core" describe.each([ - // ["lucene", undefined], - // ["sqs", undefined], - // [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], - // [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], - // [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], - // [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], + ["lucene", undefined], + ["sqs", undefined], + [DatabaseName.POSTGRES, getDatasource(DatabaseName.POSTGRES)], + [DatabaseName.MYSQL, getDatasource(DatabaseName.MYSQL)], + [DatabaseName.SQL_SERVER, getDatasource(DatabaseName.SQL_SERVER)], + [DatabaseName.MARIADB, getDatasource(DatabaseName.MARIADB)], [DatabaseName.ORACLE, getDatasource(DatabaseName.ORACLE)], ])("/v2/views (%s)", (name, dsProvider) => { const config = setup.getConfig() From 6e6e1368c1395e53e3ff1f0716afc9e9ce04fd91 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 15:32:08 +0100 Subject: [PATCH 16/26] Assert table is not deleted in SQL injection tests. --- .../src/api/routes/tests/search.spec.ts | 77 +++++++++++++------ 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 2f3dbe91f6..d69f3f2c38 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -7,6 +7,7 @@ import { import { context, db as dbCore, + docIds, features, MAX_VALID_DATE, MIN_VALID_DATE, @@ -130,14 +131,14 @@ describe.each([ } }) - async function createTable(schema: TableSchema) { + async function createTable(schema?: TableSchema) { const table = await config.api.table.save( tableForDatasource(datasource, { schema }) ) return table._id! } - async function createView(tableId: string, schema: ViewV2Schema) { + async function createView(tableId: string, schema?: ViewV2Schema) { const view = await config.api.viewV2.create({ tableId: tableId, name: generator.guid(), @@ -154,22 +155,34 @@ describe.each([ rows = await config.api.row.fetch(tableOrViewId) } + async function getTable(tableOrViewId: string): Promise { + if (docIds.isViewId(tableOrViewId)) { + const view = await config.api.viewV2.get(tableOrViewId) + return await config.api.table.get(view.tableId) + } else { + return await config.api.table.get(tableOrViewId) + } + } + describe.each([ ["table", createTable], [ "view", - async (schema: TableSchema) => { + async (schema?: TableSchema) => { const tableId = await createTable(schema) const viewId = await createView( tableId, - Object.keys(schema).reduce((viewSchema, fieldName) => { - const field = schema[fieldName] - viewSchema[fieldName] = { - visible: field.visible ?? true, - readonly: false, - } - return viewSchema - }, {}) + Object.keys(schema || {}).reduce( + (viewSchema, fieldName) => { + const field = schema![fieldName] + viewSchema[fieldName] = { + visible: field.visible ?? true, + readonly: false, + } + return viewSchema + }, + {} + ) ) return viewId }, @@ -3476,36 +3489,45 @@ describe.each([ isSql && describe("SQL injection", () => { const badStrings = [ - "1; DROP TABLE test;", - "1; DELETE FROM test;", - "1; UPDATE test SET name = 'foo';", - "1; INSERT INTO test (name) VALUES ('foo');", + "1; DROP TABLE %table_name%;", + "1; DELETE FROM %table_name%;", + "1; UPDATE %table_name% SET name = 'foo';", + "1; INSERT INTO %table_name% (name) VALUES ('foo');", "' OR '1'='1' --", - "'; DROP TABLE users; --", + "'; DROP TABLE %table_name%; --", "' OR 1=1 --", "' UNION SELECT null, null, null; --", - "' AND (SELECT COUNT(*) FROM users) > 0 --", + "' AND (SELECT COUNT(*) FROM %table_name%) > 0 --", "\"; EXEC xp_cmdshell('dir'); --", "\"' OR 'a'='a", "OR 1=1;", "'; SHUTDOWN --", ] - describe.each(badStrings)("bad string: %s", badString => { + describe.each(badStrings)("bad string: %s", badStringTemplate => { // The SQL that knex generates when you try to use a double quote in a // field name is always invalid and never works, so we skip it for these // tests. - const skipFieldNameCheck = isOracle && badString.includes('"') + const skipFieldNameCheck = isOracle && badStringTemplate.includes('"') !skipFieldNameCheck && it("should not allow SQL injection as a field name", async () => { - const tableOrViewId = await createTableOrView({ - [badString]: { - name: badString, - type: FieldType.STRING, + const tableOrViewId = await createTableOrView() + const table = await getTable(tableOrViewId) + const badString = badStringTemplate.replace( + /%table_name%/g, + table.name + ) + + await config.api.table.save({ + ...table, + schema: { + [badString]: { name: badString, type: FieldType.STRING }, }, }) + expect(await client!.schema.hasTable(table.name)).toBeTrue() + await config.api.row.save(tableOrViewId, { [badString]: "foo" }) const { rows } = await config.api.row.search( @@ -3515,6 +3537,7 @@ describe.each([ ) expect(rows).toHaveLength(1) + expect(await client!.schema.hasTable(table.name)).toBeTrue() }) it("should not allow SQL injection as a field value", async () => { @@ -3524,6 +3547,13 @@ describe.each([ type: FieldType.STRING, }, }) + const table = await getTable(tableOrViewId) + const badString = badStringTemplate.replace( + /%table_name%/g, + table.name + ) + + expect(await client!.schema.hasTable(table.name)).toBeTrue() await config.api.row.save(tableOrViewId, { foo: "foo" }) @@ -3534,6 +3564,7 @@ describe.each([ ) expect(rows).toBeEmpty() + expect(await client!.schema.hasTable(table.name)).toBeTrue() }) }) }) From 977826a0ca344188b75b26bc9c5efd073592f23c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 15:37:53 +0100 Subject: [PATCH 17/26] Clean up table assertions in SQL injection tests. --- .../src/api/routes/tests/search.spec.ts | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index d69f3f2c38..1021139ed1 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -164,6 +164,14 @@ describe.each([ } } + async function assertTableExists(name: string) { + expect(await client!.schema.hasTable(name)).toBeTrue() + } + + async function assertTableNumRows(name: string, numRows: number) { + expect(await client!.from(name).count()).toEqual([{ count: `${numRows}` }]) + } + describe.each([ ["table", createTable], [ @@ -3526,10 +3534,11 @@ describe.each([ }, }) - expect(await client!.schema.hasTable(table.name)).toBeTrue() - await config.api.row.save(tableOrViewId, { [badString]: "foo" }) + await assertTableExists(table.name) + await assertTableNumRows(table.name, 1) + const { rows } = await config.api.row.search( tableOrViewId, { query: {} }, @@ -3537,7 +3546,9 @@ describe.each([ ) expect(rows).toHaveLength(1) - expect(await client!.schema.hasTable(table.name)).toBeTrue() + + await assertTableExists(table.name) + await assertTableNumRows(table.name, 1) }) it("should not allow SQL injection as a field value", async () => { @@ -3553,7 +3564,8 @@ describe.each([ table.name ) - expect(await client!.schema.hasTable(table.name)).toBeTrue() + await assertTableExists(table.name) + await assertTableNumRows(table.name, 1) await config.api.row.save(tableOrViewId, { foo: "foo" }) @@ -3564,7 +3576,8 @@ describe.each([ ) expect(rows).toBeEmpty() - expect(await client!.schema.hasTable(table.name)).toBeTrue() + await assertTableExists(table.name) + await assertTableNumRows(table.name, 1) }) }) }) From 5a46e16b8d3f75b9d9de74e43a9865b1cbf86375 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 24 Oct 2024 16:54:08 +0100 Subject: [PATCH 18/26] Adding some tests around the openAPI public APIs to make sure the security works the way we expect, do not redirect API requests. --- packages/backend-core/src/context/identity.ts | 2 +- .../src/middleware/authenticated.ts | 50 +++++--- .../src/api/routes/public/tests/Request.ts | 110 ++++++++++++++++++ .../{metrics.spec.js => metrics.spec.ts} | 2 +- .../api/routes/public/tests/security.spec.ts | 74 ++++++++++++ .../src/api/routes/public/tests/utils.ts | 27 +++-- packages/server/src/middleware/currentapp.ts | 4 +- packages/server/src/middleware/utils.ts | 13 ++- packages/types/package.json | 3 +- packages/types/src/sdk/koa.ts | 8 ++ 10 files changed, 258 insertions(+), 35 deletions(-) create mode 100644 packages/server/src/api/routes/public/tests/Request.ts rename packages/server/src/api/routes/public/tests/{metrics.spec.js => metrics.spec.ts} (94%) create mode 100644 packages/server/src/api/routes/public/tests/security.spec.ts diff --git a/packages/backend-core/src/context/identity.ts b/packages/backend-core/src/context/identity.ts index 84de3b68c9..41f4b1eb66 100644 --- a/packages/backend-core/src/context/identity.ts +++ b/packages/backend-core/src/context/identity.ts @@ -27,7 +27,7 @@ export function doInUserContext(user: User, ctx: Ctx, task: any) { hostInfo: { ipAddress: ctx.request.ip, // filled in by koa-useragent package - userAgent: ctx.userAgent._agent.source, + userAgent: ctx.userAgent.source, }, } return doInIdentityContext(userContext, task) diff --git a/packages/backend-core/src/middleware/authenticated.ts b/packages/backend-core/src/middleware/authenticated.ts index 85aa2293ef..b713f509e0 100644 --- a/packages/backend-core/src/middleware/authenticated.ts +++ b/packages/backend-core/src/middleware/authenticated.ts @@ -1,20 +1,26 @@ import { Cookie, Header } from "../constants" import { - getCookie, clearCookie, - openJwt, + getCookie, isValidInternalAPIKey, + openJwt, } from "../utils" import { getUser } from "../cache/user" import { getSession, updateSessionTTL } from "../security/sessions" import { buildMatcherRegex, matches } from "./matchers" -import { SEPARATOR, queryGlobalView, ViewName } from "../db" -import { getGlobalDB, doInTenant } from "../context" +import { queryGlobalView, SEPARATOR, ViewName } from "../db" +import { doInTenant, getGlobalDB } from "../context" import { decrypt } from "../security/encryption" import * as identity from "../context/identity" import env from "../environment" -import { Ctx, EndpointMatcher, SessionCookie, User } from "@budibase/types" -import { InvalidAPIKeyError, ErrorCode } from "../errors" +import { + Ctx, + EndpointMatcher, + LoginMethod, + SessionCookie, + User, +} from "@budibase/types" +import { ErrorCode, InvalidAPIKeyError } from "../errors" import tracer from "dd-trace" const ONE_MINUTE = env.SESSION_UPDATE_PERIOD @@ -26,16 +32,18 @@ interface FinaliseOpts { internal?: boolean publicEndpoint?: boolean version?: string - user?: any + user?: User | { tenantId: string } + loginMethod?: LoginMethod } function timeMinusOneMinute() { return new Date(Date.now() - ONE_MINUTE).toISOString() } -function finalise(ctx: any, opts: FinaliseOpts = {}) { +function finalise(ctx: Ctx, opts: FinaliseOpts = {}) { ctx.publicEndpoint = opts.publicEndpoint || false ctx.isAuthenticated = opts.authenticated || false + ctx.loginMethod = opts.loginMethod ctx.user = opts.user ctx.internal = opts.internal || false ctx.version = opts.version @@ -120,9 +128,10 @@ export default function ( } const tenantId = ctx.request.headers[Header.TENANT_ID] - let authenticated = false, - user = null, - internal = false + let authenticated: boolean = false, + user: User | { tenantId: string } | undefined = undefined, + internal: boolean = false, + loginMethod: LoginMethod | undefined = undefined if (authCookie && !apiKey) { const sessionId = authCookie.sessionId const userId = authCookie.userId @@ -146,6 +155,7 @@ export default function ( } // @ts-ignore user.csrfToken = session.csrfToken + loginMethod = LoginMethod.COOKIE if (session?.lastAccessedAt < timeMinusOneMinute()) { // make sure we denote that the session is still in use @@ -170,17 +180,16 @@ export default function ( apiKey, populateUser ) - if (valid && foundUser) { + if (valid) { authenticated = true + loginMethod = LoginMethod.API_KEY user = foundUser - } else if (valid) { - authenticated = true - internal = true + internal = !foundUser } } if (!user && tenantId) { user = { tenantId } - } else if (user) { + } else if (user && "password" in user) { delete user.password } // be explicit @@ -204,7 +213,14 @@ export default function ( } // isAuthenticated is a function, so use a variable to be able to check authed state - finalise(ctx, { authenticated, user, internal, version, publicEndpoint }) + finalise(ctx, { + authenticated, + user, + internal, + version, + publicEndpoint, + loginMethod, + }) if (isUser(user)) { return identity.doInUserContext(user, ctx, next) diff --git a/packages/server/src/api/routes/public/tests/Request.ts b/packages/server/src/api/routes/public/tests/Request.ts new file mode 100644 index 0000000000..70e83d711d --- /dev/null +++ b/packages/server/src/api/routes/public/tests/Request.ts @@ -0,0 +1,110 @@ +import { User, Table, SearchFilters, Row } from "@budibase/types" +import { HttpMethod, MakeRequestResponse, generateMakeRequest } from "./utils" +import TestConfiguration from "../../../../tests/utilities/TestConfiguration" +import { Expectations } from "../../../../tests/utilities/api/base" + +type RequestOpts = { internal?: boolean; appId?: string } + +type PublicAPIExpectations = Omit + +export class PublicAPIRequest { + private makeRequest: MakeRequestResponse | undefined + private appId: string | undefined + private _tables: PublicTableAPI | undefined + private _rows: PublicRowAPI | undefined + private _apiKey: string | undefined + + async init(config: TestConfiguration, user: User, opts?: RequestOpts) { + this._apiKey = await config.generateApiKey(user._id) + this.makeRequest = generateMakeRequest(this.apiKey, opts) + this.appId = opts?.appId + this._tables = new PublicTableAPI(this) + this._rows = new PublicRowAPI(this) + return this + } + + opts(opts: RequestOpts) { + if (opts.appId) { + this.appId = opts.appId + } + this.makeRequest = generateMakeRequest(this.apiKey, opts) + } + + async send( + method: HttpMethod, + endpoint: string, + body?: any, + expectations?: PublicAPIExpectations + ) { + if (!this.makeRequest) { + throw new Error("Init has not been called") + } + const res = await this.makeRequest(method, endpoint, body, this.appId) + if (expectations?.status) { + expect(res.status).toEqual(expectations.status) + } + if (expectations?.body) { + expect(res.body).toEqual(expectations?.body) + } + return res.body + } + + get apiKey(): string { + if (!this._apiKey) { + throw new Error("Init has not been called") + } + return this._apiKey + } + + get tables(): PublicTableAPI { + if (!this._tables) { + throw new Error("Init has not been called") + } + return this._tables + } + + get rows(): PublicRowAPI { + if (!this._rows) { + throw new Error("Init has not been called") + } + return this._rows + } +} + +export class PublicTableAPI { + request: PublicAPIRequest + + constructor(request: PublicAPIRequest) { + this.request = request + } + + async create( + table: Table, + expectations?: PublicAPIExpectations + ): Promise<{ data: Table }> { + return this.request.send("post", "/tables", table, expectations) + } +} + +export class PublicRowAPI { + request: PublicAPIRequest + + constructor(request: PublicAPIRequest) { + this.request = request + } + + async search( + tableId: string, + query: SearchFilters, + expectations?: PublicAPIExpectations + ): Promise<{ data: Row[] }> { + return this.request.send( + "post", + `/tables/${tableId}/rows/search`, + { + query, + }, + expectations + ) + } +} diff --git a/packages/server/src/api/routes/public/tests/metrics.spec.js b/packages/server/src/api/routes/public/tests/metrics.spec.ts similarity index 94% rename from packages/server/src/api/routes/public/tests/metrics.spec.js rename to packages/server/src/api/routes/public/tests/metrics.spec.ts index 2fb5e91000..84d47dfa1d 100644 --- a/packages/server/src/api/routes/public/tests/metrics.spec.js +++ b/packages/server/src/api/routes/public/tests/metrics.spec.ts @@ -1,4 +1,4 @@ -const setup = require("../../tests/utilities") +import * as setup from "../../tests/utilities" describe("/metrics", () => { let request = setup.getRequest() diff --git a/packages/server/src/api/routes/public/tests/security.spec.ts b/packages/server/src/api/routes/public/tests/security.spec.ts new file mode 100644 index 0000000000..971cb086eb --- /dev/null +++ b/packages/server/src/api/routes/public/tests/security.spec.ts @@ -0,0 +1,74 @@ +import * as setup from "../../tests/utilities" +import { roles } from "@budibase/backend-core" +import { basicTable } from "../../../../tests/utilities/structures" +import { Table, User } from "@budibase/types" +import { PublicAPIRequest } from "./Request" + +const BROWSER_USER_AGENT = + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36" + +describe("check public API security", () => { + const config = setup.getConfig() + let builderRequest: PublicAPIRequest, + appUserRequest: PublicAPIRequest, + table: Table, + appUser: User + + beforeAll(async () => { + await config.init() + const builderUser = await config.globalUser() + appUser = await config.globalUser({ + builder: { global: false }, + roles: { + [config.getProdAppId()]: roles.BUILTIN_ROLE_IDS.BASIC, + }, + }) + builderRequest = await new PublicAPIRequest().init(config, builderUser) + appUserRequest = await new PublicAPIRequest().init(config, appUser) + table = (await builderRequest.tables.create(basicTable())).data + }) + + it("should allow with builder API key", async () => { + const res = await builderRequest.rows.search( + table._id!, + {}, + { + status: 200, + } + ) + expect(res.data.length).toEqual(0) + }) + + it("should 403 when from browser, but API key", async () => { + await appUserRequest.rows.search( + table._id!, + {}, + { + status: 403, + } + ) + }) + + it("should re-direct when using cookie", async () => { + const headers = await config.login({ + userId: appUser._id!, + builder: false, + prodApp: false, + }) + await config.withHeaders( + { + ...headers, + "User-Agent": BROWSER_USER_AGENT, + }, + async () => { + await config.api.row.search( + table._id!, + { query: {} }, + { + status: 302, + } + ) + } + ) + }) +}) diff --git a/packages/server/src/api/routes/public/tests/utils.ts b/packages/server/src/api/routes/public/tests/utils.ts index 1b57682af9..4fb048a540 100644 --- a/packages/server/src/api/routes/public/tests/utils.ts +++ b/packages/server/src/api/routes/public/tests/utils.ts @@ -21,17 +21,19 @@ export type MakeRequestWithFormDataResponse = ( function base( apiKey: string, endpoint: string, - intAppId: string | null, - isInternal: boolean + opts?: { + intAppId?: string + internal?: boolean + } ) { const extraHeaders: any = { "x-budibase-api-key": apiKey, } - if (intAppId) { - extraHeaders["x-budibase-app-id"] = intAppId + if (opts?.intAppId) { + extraHeaders["x-budibase-app-id"] = opts.intAppId } - const url = isInternal + const url = opts?.internal ? endpoint : checkSlashesInUrl(`/api/public/v1/${endpoint}`) return { headers: extraHeaders, url } @@ -39,7 +41,7 @@ function base( export function generateMakeRequest( apiKey: string, - isInternal = false + opts?: { internal?: boolean } ): MakeRequestResponse { const request = setup.getRequest()! const config = setup.getConfig()! @@ -47,9 +49,12 @@ export function generateMakeRequest( method: HttpMethod, endpoint: string, body?: any, - intAppId: string | null = config.getAppId() + intAppId: string | undefined = config.getAppId() ) => { - const { headers, url } = base(apiKey, endpoint, intAppId, isInternal) + const { headers, url } = base(apiKey, endpoint, { ...opts, intAppId }) + if (body && typeof body !== "string") { + headers["Content-Type"] = "application/json" + } const req = request[method](url).set(config.defaultHeaders(headers)) if (body) { req.send(body) @@ -62,7 +67,7 @@ export function generateMakeRequest( export function generateMakeRequestWithFormData( apiKey: string, - isInternal = false + opts?: { internal?: boolean; browser?: boolean } ): MakeRequestWithFormDataResponse { const request = setup.getRequest()! const config = setup.getConfig()! @@ -70,9 +75,9 @@ export function generateMakeRequestWithFormData( method: HttpMethod, endpoint: string, fields: Record, - intAppId: string | null = config.getAppId() + intAppId: string | undefined = config.getAppId() ) => { - const { headers, url } = base(apiKey, endpoint, intAppId, isInternal) + const { headers, url } = base(apiKey, endpoint, { ...opts, intAppId }) const req = request[method](url).set(config.defaultHeaders(headers)) for (let [field, value] of Object.entries(fields)) { if (typeof value === "string") { diff --git a/packages/server/src/middleware/currentapp.ts b/packages/server/src/middleware/currentapp.ts index d616377298..d5840712a0 100644 --- a/packages/server/src/middleware/currentapp.ts +++ b/packages/server/src/middleware/currentapp.ts @@ -10,7 +10,7 @@ import { import { generateUserMetadataID, isDevAppID } from "../db/utils" import { getCachedSelf } from "../utilities/global" import env from "../environment" -import { isWebhookEndpoint } from "./utils" +import { isWebhookEndpoint, isBrowser, isApiKey } from "./utils" import { UserCtx, ContextUser } from "@budibase/types" import tracer from "dd-trace" @@ -27,7 +27,7 @@ export default async (ctx: UserCtx, next: any) => { } // deny access to application preview - if (!env.isTest()) { + if (isBrowser(ctx) && !isApiKey(ctx)) { if ( isDevAppID(requestAppId) && !isWebhookEndpoint(ctx) && diff --git a/packages/server/src/middleware/utils.ts b/packages/server/src/middleware/utils.ts index 714df12f38..650260b136 100644 --- a/packages/server/src/middleware/utils.ts +++ b/packages/server/src/middleware/utils.ts @@ -1,9 +1,18 @@ -import { BBContext } from "@budibase/types" +import { LoginMethod, UserCtx } from "@budibase/types" const WEBHOOK_ENDPOINTS = new RegExp( ["webhooks/trigger", "webhooks/schema"].join("|") ) -export function isWebhookEndpoint(ctx: BBContext) { +export function isWebhookEndpoint(ctx: UserCtx) { return WEBHOOK_ENDPOINTS.test(ctx.request.url) } + +export function isBrowser(ctx: UserCtx) { + const browser = ctx.userAgent.browser + return browser !== "unknown" +} + +export function isApiKey(ctx: UserCtx) { + return ctx.loginMethod === LoginMethod.API_KEY +} diff --git a/packages/types/package.json b/packages/types/package.json index bce25c7086..d132383081 100644 --- a/packages/types/package.json +++ b/packages/types/package.json @@ -19,7 +19,8 @@ "@types/koa": "2.13.4", "@types/redlock": "4.0.7", "rimraf": "3.0.2", - "typescript": "5.5.2" + "typescript": "5.5.2", + "koa-useragent": "^4.1.0" }, "dependencies": { "scim-patch": "^0.8.1" diff --git a/packages/types/src/sdk/koa.ts b/packages/types/src/sdk/koa.ts index a7df701171..38622b974a 100644 --- a/packages/types/src/sdk/koa.ts +++ b/packages/types/src/sdk/koa.ts @@ -2,6 +2,12 @@ import { Context, Request } from "koa" import { User, Role, UserRoles, Account, ConfigType } from "../documents" import { FeatureFlag, License } from "../sdk" import { Files } from "formidable" +import { UserAgentContext } from "koa-useragent" + +export enum LoginMethod { + API_KEY = "api_key", + COOKIE = "cookie", +} export interface ContextUser extends Omit { globalId?: string @@ -31,6 +37,7 @@ export interface BBRequest extends Request { export interface Ctx extends Context { request: BBRequest body: ResponseBody + userAgent: UserAgentContext["userAgent"] } /** @@ -40,6 +47,7 @@ export interface UserCtx extends Ctx { user: ContextUser roleId?: string + loginMethod?: LoginMethod } /** From 226c8d4f8ec9cb9a3f5f17bb5f088810234b23a5 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 17:33:16 +0100 Subject: [PATCH 19/26] Fix SQL tests. --- packages/server/src/integrations/tests/sql.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/server/src/integrations/tests/sql.spec.ts b/packages/server/src/integrations/tests/sql.spec.ts index 5a505fbb40..2b3649161c 100644 --- a/packages/server/src/integrations/tests/sql.spec.ts +++ b/packages/server/src/integrations/tests/sql.spec.ts @@ -212,7 +212,7 @@ describe("SQL query builder", () => { const filterSet = [`%20%`, `%25%`, `%"john"%`, `%"mary"%`] expect(query).toEqual({ bindings: [...filterSet, limit], - sql: `select * from (select * from "test" where COALESCE(LOWER("test"."age"), '') LIKE :1 AND COALESCE(LOWER("test"."age"), '') LIKE :2 and COALESCE(LOWER("test"."name"), '') LIKE :3 AND COALESCE(LOWER("test"."name"), '') LIKE :4 order by "test"."id" asc) where rownum <= :5`, + sql: `select * from (select * from "test" where ((COALESCE(LOWER("test"."age"), '') like :1 and COALESCE(LOWER("test"."age"), '') like :2)) and ((COALESCE(LOWER("test"."name"), '') like :3 and COALESCE(LOWER("test"."name"), '') like :4)) order by "test"."id" asc) where rownum <= :5`, }) query = new Sql(SqlClient.ORACLE, limit)._query( @@ -244,7 +244,7 @@ describe("SQL query builder", () => { expect(query).toEqual({ bindings: ["John", limit], - sql: `select * from (select * from "test" where (to_char("test"."name") IS NOT NULL AND to_char("test"."name") = :1) order by "test"."id" asc) where rownum <= :2`, + sql: `select * from (select * from "test" where (to_char("test"."name") is not null and to_char("test"."name") = :1) order by "test"."id" asc) where rownum <= :2`, }) }) @@ -262,7 +262,7 @@ describe("SQL query builder", () => { expect(query).toEqual({ bindings: ["John", limit], - sql: `select * from (select * from "test" where (to_char("test"."name") IS NOT NULL AND to_char("test"."name") != :1) OR to_char("test"."name") IS NULL order by "test"."id" asc) where rownum <= :2`, + sql: `select * from (select * from "test" where (to_char("test"."name") is not null and to_char("test"."name") != :1) or to_char("test"."name") is null order by "test"."id" asc) where rownum <= :2`, }) }) }) From 0863a1167cba60bfd962840e0f0da22eaa1d3173 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 24 Oct 2024 17:41:34 +0100 Subject: [PATCH 20/26] Updating OpenAPI definition to contain all required variables. --- packages/server/specs/generate.ts | 18 +++++-------- packages/server/specs/openapi.json | 30 ++++++++++++---------- packages/server/specs/openapi.yaml | 22 +++++++++------- packages/server/src/definitions/openapi.ts | 21 ++++++++++----- 4 files changed, 51 insertions(+), 40 deletions(-) diff --git a/packages/server/specs/generate.ts b/packages/server/specs/generate.ts index eea00f83aa..8f6376195f 100644 --- a/packages/server/specs/generate.ts +++ b/packages/server/specs/generate.ts @@ -20,19 +20,15 @@ const options = { { url: "https://budibase.app/api/public/v1", description: "Budibase Cloud API", - }, - { - url: "{protocol}://{hostname}/api/public/v1", - description: "Budibase self hosted API", variables: { - protocol: { - default: "http", - description: - "Whether HTTP or HTTPS should be used to communicate with your Budibase instance.", + apiKey: { + default: "", + description: "The API key of the user to assume for API call.", }, - hostname: { - default: "localhost:10000", - description: "The URL of your Budibase instance.", + appId: { + default: "", + description: + "The ID of the app the calls will be executed within the context of, this should start with app_ (production) or app_dev (development).", }, }, }, diff --git a/packages/server/specs/openapi.json b/packages/server/specs/openapi.json index b21554505b..c96296a8bb 100644 --- a/packages/server/specs/openapi.json +++ b/packages/server/specs/openapi.json @@ -8,19 +8,15 @@ "servers": [ { "url": "https://budibase.app/api/public/v1", - "description": "Budibase Cloud API" - }, - { - "url": "{protocol}://{hostname}/api/public/v1", - "description": "Budibase self hosted API", + "description": "Budibase Cloud API", "variables": { - "protocol": { - "default": "http", - "description": "Whether HTTP or HTTPS should be used to communicate with your Budibase instance." + "apiKey": { + "default": "", + "description": "The API key of the user to assume for API call." }, - "hostname": { - "default": "localhost:10000", - "description": "The URL of your Budibase instance." + "appId": { + "default": "", + "description": "The ID of the app the calls will be executed within the context of, this should start with app_ (production) or app_dev (development)." } } } @@ -833,7 +829,8 @@ "type": "string", "enum": [ "static", - "dynamic" + "dynamic", + "ai" ], "description": "Defines whether this is a static or dynamic formula." } @@ -857,6 +854,7 @@ "link", "formula", "auto", + "ai", "json", "internal", "barcodeqr", @@ -1042,7 +1040,8 @@ "type": "string", "enum": [ "static", - "dynamic" + "dynamic", + "ai" ], "description": "Defines whether this is a static or dynamic formula." } @@ -1066,6 +1065,7 @@ "link", "formula", "auto", + "ai", "json", "internal", "barcodeqr", @@ -1262,7 +1262,8 @@ "type": "string", "enum": [ "static", - "dynamic" + "dynamic", + "ai" ], "description": "Defines whether this is a static or dynamic formula." } @@ -1286,6 +1287,7 @@ "link", "formula", "auto", + "ai", "json", "internal", "barcodeqr", diff --git a/packages/server/specs/openapi.yaml b/packages/server/specs/openapi.yaml index 6a2ae89c61..9ac7b7533a 100644 --- a/packages/server/specs/openapi.yaml +++ b/packages/server/specs/openapi.yaml @@ -6,16 +6,14 @@ info: servers: - url: https://budibase.app/api/public/v1 description: Budibase Cloud API - - url: "{protocol}://{hostname}/api/public/v1" - description: Budibase self hosted API variables: - protocol: - default: http - description: Whether HTTP or HTTPS should be used to communicate with your - Budibase instance. - hostname: - default: localhost:10000 - description: The URL of your Budibase instance. + apiKey: + default: + description: The API key of the user to assume for API call. + appId: + default: + description: The ID of the app the calls will be executed within the context of, + this should start with app_ (production) or app_dev (development). components: parameters: tableId: @@ -761,6 +759,7 @@ components: enum: - static - dynamic + - ai description: Defines whether this is a static or dynamic formula. - type: object properties: @@ -779,6 +778,7 @@ components: - link - formula - auto + - ai - json - internal - barcodeqr @@ -929,6 +929,7 @@ components: enum: - static - dynamic + - ai description: Defines whether this is a static or dynamic formula. - type: object properties: @@ -947,6 +948,7 @@ components: - link - formula - auto + - ai - json - internal - barcodeqr @@ -1104,6 +1106,7 @@ components: enum: - static - dynamic + - ai description: Defines whether this is a static or dynamic formula. - type: object properties: @@ -1122,6 +1125,7 @@ components: - link - formula - auto + - ai - json - internal - barcodeqr diff --git a/packages/server/src/definitions/openapi.ts b/packages/server/src/definitions/openapi.ts index cc3c8a7d4d..3abf7e6fa7 100644 --- a/packages/server/src/definitions/openapi.ts +++ b/packages/server/src/definitions/openapi.ts @@ -257,7 +257,7 @@ export interface components { * @description Defines whether this is a static or dynamic formula. * @enum {string} */ - formulaType?: "static" | "dynamic"; + formulaType?: "static" | "dynamic" | "ai"; } | { /** @@ -277,11 +277,14 @@ export interface components { | "link" | "formula" | "auto" + | "ai" | "json" | "internal" | "barcodeqr" + | "signature_single" | "bigint" - | "bb_reference"; + | "bb_reference" + | "bb_reference_single"; /** @description A constraint can be applied to the column which will be validated against when a row is saved. */ constraints?: { /** @enum {string} */ @@ -366,7 +369,7 @@ export interface components { * @description Defines whether this is a static or dynamic formula. * @enum {string} */ - formulaType?: "static" | "dynamic"; + formulaType?: "static" | "dynamic" | "ai"; } | { /** @@ -386,11 +389,14 @@ export interface components { | "link" | "formula" | "auto" + | "ai" | "json" | "internal" | "barcodeqr" + | "signature_single" | "bigint" - | "bb_reference"; + | "bb_reference" + | "bb_reference_single"; /** @description A constraint can be applied to the column which will be validated against when a row is saved. */ constraints?: { /** @enum {string} */ @@ -477,7 +483,7 @@ export interface components { * @description Defines whether this is a static or dynamic formula. * @enum {string} */ - formulaType?: "static" | "dynamic"; + formulaType?: "static" | "dynamic" | "ai"; } | { /** @@ -497,11 +503,14 @@ export interface components { | "link" | "formula" | "auto" + | "ai" | "json" | "internal" | "barcodeqr" + | "signature_single" | "bigint" - | "bb_reference"; + | "bb_reference" + | "bb_reference_single"; /** @description A constraint can be applied to the column which will be validated against when a row is saved. */ constraints?: { /** @enum {string} */ From 68354cc50f80576b6ae94a8dc8fb72336d791816 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 24 Oct 2024 17:48:25 +0100 Subject: [PATCH 21/26] Defaulting app ID to variable. --- packages/server/specs/openapi.json | 2 ++ packages/server/specs/openapi.yaml | 2 ++ packages/server/specs/parameters.ts | 2 ++ 3 files changed, 6 insertions(+) diff --git a/packages/server/specs/openapi.json b/packages/server/specs/openapi.json index c96296a8bb..f3091a1fc7 100644 --- a/packages/server/specs/openapi.json +++ b/packages/server/specs/openapi.json @@ -47,6 +47,7 @@ "required": true, "description": "The ID of the app which this request is targeting.", "schema": { + "default": "{{ appId }}", "type": "string" } }, @@ -56,6 +57,7 @@ "required": true, "description": "The ID of the app which this request is targeting.", "schema": { + "default": "{{ appId }}", "type": "string" } }, diff --git a/packages/server/specs/openapi.yaml b/packages/server/specs/openapi.yaml index 9ac7b7533a..1e9b9921cf 100644 --- a/packages/server/specs/openapi.yaml +++ b/packages/server/specs/openapi.yaml @@ -36,6 +36,7 @@ components: required: true description: The ID of the app which this request is targeting. schema: + default: "{{ appId }}" type: string appIdUrl: in: path @@ -43,6 +44,7 @@ components: required: true description: The ID of the app which this request is targeting. schema: + default: "{{ appId }}" type: string queryId: in: path diff --git a/packages/server/specs/parameters.ts b/packages/server/specs/parameters.ts index a928026bb1..a4f2b27ae4 100644 --- a/packages/server/specs/parameters.ts +++ b/packages/server/specs/parameters.ts @@ -24,6 +24,7 @@ export const appId = { required: true, description: "The ID of the app which this request is targeting.", schema: { + default: "{{ appId }}", type: "string", }, } @@ -34,6 +35,7 @@ export const appIdUrl = { required: true, description: "The ID of the app which this request is targeting.", schema: { + default: "{{ appId }}", type: "string", }, } From dd6a0853a488b386a8f1777b88382cc576c01e94 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 18:05:33 +0100 Subject: [PATCH 22/26] Fix tests (again) --- .../src/api/routes/tests/search.spec.ts | 47 ++++++++++++++----- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 1021139ed1..9cc1432180 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -164,12 +164,21 @@ describe.each([ } } - async function assertTableExists(name: string) { + async function assertTableExists(nameOrTable: string | Table) { + const name = + typeof nameOrTable === "string" ? nameOrTable : nameOrTable.name expect(await client!.schema.hasTable(name)).toBeTrue() } - async function assertTableNumRows(name: string, numRows: number) { - expect(await client!.from(name).count()).toEqual([{ count: `${numRows}` }]) + async function assertTableNumRows( + nameOrTable: string | Table, + numRows: number + ) { + const name = + typeof nameOrTable === "string" ? nameOrTable : nameOrTable.name + const row = await client!.from(name).count() + const count = parseInt(Object.values(row[0])[0] as string) + expect(count).toEqual(numRows) } describe.each([ @@ -3495,7 +3504,8 @@ describe.each([ }) isSql && - describe("SQL injection", () => { + !isSqs && + describe.only("SQL injection", () => { const badStrings = [ "1; DROP TABLE %table_name%;", "1; DELETE FROM %table_name%;", @@ -3530,14 +3540,25 @@ describe.each([ await config.api.table.save({ ...table, schema: { + ...table.schema, [badString]: { name: badString, type: FieldType.STRING }, }, }) + if (docIds.isViewId(tableOrViewId)) { + const view = await config.api.viewV2.get(tableOrViewId) + await config.api.viewV2.update({ + ...view, + schema: { + [badString]: { visible: true }, + }, + }) + } + await config.api.row.save(tableOrViewId, { [badString]: "foo" }) - await assertTableExists(table.name) - await assertTableNumRows(table.name, 1) + await assertTableExists(table) + await assertTableNumRows(table, 1) const { rows } = await config.api.row.search( tableOrViewId, @@ -3547,8 +3568,8 @@ describe.each([ expect(rows).toHaveLength(1) - await assertTableExists(table.name) - await assertTableNumRows(table.name, 1) + await assertTableExists(table) + await assertTableNumRows(table, 1) }) it("should not allow SQL injection as a field value", async () => { @@ -3564,11 +3585,11 @@ describe.each([ table.name ) - await assertTableExists(table.name) - await assertTableNumRows(table.name, 1) - await config.api.row.save(tableOrViewId, { foo: "foo" }) + await assertTableExists(table) + await assertTableNumRows(table, 1) + const { rows } = await config.api.row.search( tableOrViewId, { query: { equal: { foo: badString } } }, @@ -3576,8 +3597,8 @@ describe.each([ ) expect(rows).toBeEmpty() - await assertTableExists(table.name) - await assertTableNumRows(table.name, 1) + await assertTableExists(table) + await assertTableNumRows(table, 1) }) }) }) From c33f3319041a6662364c51bae297ba9d907f474c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 24 Oct 2024 18:08:49 +0100 Subject: [PATCH 23/26] Test fix. --- packages/server/src/middleware/utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/middleware/utils.ts b/packages/server/src/middleware/utils.ts index 650260b136..8e7c623e2a 100644 --- a/packages/server/src/middleware/utils.ts +++ b/packages/server/src/middleware/utils.ts @@ -9,8 +9,8 @@ export function isWebhookEndpoint(ctx: UserCtx) { } export function isBrowser(ctx: UserCtx) { - const browser = ctx.userAgent.browser - return browser !== "unknown" + const browser = ctx.userAgent?.browser + return browser && browser !== "unknown" } export function isApiKey(ctx: UserCtx) { From 2b1bf4d711e7cb97a02e5284ba2f7413926c8986 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 25 Oct 2024 10:39:42 +0100 Subject: [PATCH 24/26] Fix lint. --- packages/server/src/api/routes/tests/search.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 9cc1432180..d7a7e09e90 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -3505,7 +3505,7 @@ describe.each([ isSql && !isSqs && - describe.only("SQL injection", () => { + describe("SQL injection", () => { const badStrings = [ "1; DROP TABLE %table_name%;", "1; DELETE FROM %table_name%;", From f1fa0a3a6f2963accc49c28442895c8a004711e1 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 25 Oct 2024 10:41:20 +0100 Subject: [PATCH 25/26] Fixing tests, updating to typescript. --- .../api/routes/public/tests/security.spec.ts | 5 +- .../{routing.spec.js => routing.spec.ts} | 61 ++++++++++++------- ...{currentapp.spec.js => currentapp.spec.ts} | 16 +++-- .../src/tests/utilities/TestConfiguration.ts | 5 ++ 4 files changed, 55 insertions(+), 32 deletions(-) rename packages/server/src/api/routes/tests/{routing.spec.js => routing.spec.ts} (72%) rename packages/server/src/middleware/tests/{currentapp.spec.js => currentapp.spec.ts} (94%) diff --git a/packages/server/src/api/routes/public/tests/security.spec.ts b/packages/server/src/api/routes/public/tests/security.spec.ts index 971cb086eb..a285bc7736 100644 --- a/packages/server/src/api/routes/public/tests/security.spec.ts +++ b/packages/server/src/api/routes/public/tests/security.spec.ts @@ -4,9 +4,6 @@ import { basicTable } from "../../../../tests/utilities/structures" import { Table, User } from "@budibase/types" import { PublicAPIRequest } from "./Request" -const BROWSER_USER_AGENT = - "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36" - describe("check public API security", () => { const config = setup.getConfig() let builderRequest: PublicAPIRequest, @@ -58,7 +55,7 @@ describe("check public API security", () => { await config.withHeaders( { ...headers, - "User-Agent": BROWSER_USER_AGENT, + "User-Agent": config.browserUserAgent(), }, async () => { await config.api.row.search( diff --git a/packages/server/src/api/routes/tests/routing.spec.js b/packages/server/src/api/routes/tests/routing.spec.ts similarity index 72% rename from packages/server/src/api/routes/tests/routing.spec.js rename to packages/server/src/api/routes/tests/routing.spec.ts index 5bdd4ee852..0d4ffc7c3e 100644 --- a/packages/server/src/api/routes/tests/routing.spec.js +++ b/packages/server/src/api/routes/tests/routing.spec.ts @@ -1,9 +1,10 @@ -const setup = require("./utilities") -const { basicScreen, powerScreen } = setup.structures -const { checkBuilderEndpoint, runInProd } = require("./utilities/TestFunctions") -const { roles } = require("@budibase/backend-core") -const { BUILTIN_ROLE_IDS } = roles +import * as setup from "./utilities" +import { checkBuilderEndpoint, runInProd } from "./utilities/TestFunctions" +import { roles } from "@budibase/backend-core" +import { Screen } from "@budibase/types" +const { BUILTIN_ROLE_IDS } = roles +const { basicScreen, powerScreen } = setup.structures const route = "/test" // there are checks which are disabled in test env, @@ -12,7 +13,7 @@ const route = "/test" describe("/routing", () => { let request = setup.getRequest() let config = setup.getConfig() - let basic, power + let basic: Screen, power: Screen afterAll(setup.afterAll) @@ -25,26 +26,40 @@ describe("/routing", () => { describe("fetch", () => { it("prevents a public user from accessing development app", async () => { - await runInProd(() => { - return request - .get(`/api/routing/client`) - .set(config.publicHeaders({ prodApp: false })) - .expect(302) - }) + await config.withHeaders( + { + "User-Agent": config.browserUserAgent(), + }, + async () => { + await runInProd(() => { + return request + .get(`/api/routing/client`) + .set(config.publicHeaders({ prodApp: false })) + .expect(302) + }) + } + ) }) it("prevents a non builder from accessing development app", async () => { - await runInProd(async () => { - return request - .get(`/api/routing/client`) - .set( - await config.roleHeaders({ - roleId: BUILTIN_ROLE_IDS.BASIC, - prodApp: false, - }) - ) - .expect(302) - }) + await config.withHeaders( + { + "User-Agent": config.browserUserAgent(), + }, + async () => { + await runInProd(async () => { + return request + .get(`/api/routing/client`) + .set( + await config.roleHeaders({ + roleId: BUILTIN_ROLE_IDS.BASIC, + prodApp: false, + }) + ) + .expect(302) + }) + } + ) }) it("returns the correct routing for basic user", async () => { const res = await request diff --git a/packages/server/src/middleware/tests/currentapp.spec.js b/packages/server/src/middleware/tests/currentapp.spec.ts similarity index 94% rename from packages/server/src/middleware/tests/currentapp.spec.js rename to packages/server/src/middleware/tests/currentapp.spec.ts index 22e47b0a6e..202d9f96d5 100644 --- a/packages/server/src/middleware/tests/currentapp.spec.js +++ b/packages/server/src/middleware/tests/currentapp.spec.ts @@ -1,4 +1,6 @@ -require("../../db").init() +import * as db from "../../db" + +db.init() mockAuthWithNoCookie() mockWorker() mockUserGroups() @@ -45,7 +47,7 @@ function mockAuthWithNoCookie() { }, cache: { user: { - getUser: async id => { + getUser: async () => { return { _id: "us_uuid1", } @@ -82,7 +84,7 @@ function mockAuthWithCookie() { }, cache: { user: { - getUser: async id => { + getUser: async () => { return { _id: "us_uuid1", } @@ -94,6 +96,10 @@ function mockAuthWithCookie() { } class TestConfiguration { + next: jest.MockedFunction + throw: jest.MockedFunction + ctx: any + constructor() { this.next = jest.fn() this.throw = jest.fn() @@ -130,7 +136,7 @@ class TestConfiguration { } describe("Current app middleware", () => { - let config + let config: TestConfiguration beforeEach(() => { config = new TestConfiguration() @@ -192,7 +198,7 @@ describe("Current app middleware", () => { }, cache: { user: { - getUser: async id => { + getUser: async () => { return { _id: "us_uuid1", } diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index 713f8b31de..5ed60a59b6 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -423,6 +423,7 @@ export default class TestConfiguration { Accept: "application/json", Cookie: [`${constants.Cookie.Auth}=${authToken}`], [constants.Header.APP_ID]: appId, + ...this.temporaryHeaders, } }) } @@ -527,6 +528,10 @@ export default class TestConfiguration { return this.login({ userId: email, roleId, builder, prodApp }) } + browserUserAgent() { + return "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36" + } + // TENANCY tenantHost() { From 8f17d7f1bf48df724b21034a88782342b615c9c5 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 25 Oct 2024 13:39:01 +0100 Subject: [PATCH 26/26] Updating pro submodule again. --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index 6041196305..f6aebba944 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 60411963053b98e0415a1e9285b721fc26c628c8 +Subproject commit f6aebba94451ce47bba551926e5ad72bd75f71c6