From cc2605a9dd7b7a8eedb9889205aa060d1130f5b7 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 21 Oct 2024 10:04:03 +0100 Subject: [PATCH 01/18] 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/18] 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/18] 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/18] 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 309506adab54ca89cb0612650afbddedcf8761b1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 15:05:41 +0100 Subject: [PATCH 05/18] 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 06/18] 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 07/18] 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 08/18] 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 09/18] 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 10/18] 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 11/18] 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 12/18] 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 13/18] 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 14/18] 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 15/18] 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 226c8d4f8ec9cb9a3f5f17bb5f088810234b23a5 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 17:33:16 +0100 Subject: [PATCH 16/18] 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 dd6a0853a488b386a8f1777b88382cc576c01e94 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Thu, 24 Oct 2024 18:05:33 +0100 Subject: [PATCH 17/18] 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 2b1bf4d711e7cb97a02e5284ba2f7413926c8986 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 25 Oct 2024 10:39:42 +0100 Subject: [PATCH 18/18] 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%;",