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 e69bfc2d7179f27c0c090cacc0dc73026db774f2 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 21 Oct 2024 17:17:05 +0100 Subject: [PATCH 02/26] Adding a fix for mysql, stopping coercion to dates in some cases, attempting to make this less all catching. Likely an area of concern, but there is currently no way to search for dates without this. --- .../routes/tests/queries/generic-sql.spec.ts | 109 +++++++++++------- packages/server/src/integrations/mysql.ts | 12 +- packages/server/src/utilities/index.ts | 12 ++ .../server/src/utilities/tests/utils.spec.ts | 30 +++++ 4 files changed, 115 insertions(+), 48 deletions(-) create mode 100644 packages/server/src/utilities/tests/utils.spec.ts diff --git a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts index 0979f8bed3..7501b43911 100644 --- a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts +++ b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts @@ -18,17 +18,18 @@ import { Knex } from "knex" describe.each( [ - DatabaseName.POSTGRES, + // DatabaseName.POSTGRES, DatabaseName.MYSQL, - DatabaseName.SQL_SERVER, - DatabaseName.MARIADB, - DatabaseName.ORACLE, + // DatabaseName.SQL_SERVER, + // DatabaseName.MARIADB, + // DatabaseName.ORACLE, ].map(name => [name, getDatasource(name)]) )("queries (%s)", (dbName, dsProvider) => { const config = setup.getConfig() const isOracle = dbName === DatabaseName.ORACLE const isMsSQL = dbName === DatabaseName.SQL_SERVER const isPostgres = dbName === DatabaseName.POSTGRES + const mainTableName = "test_table" let rawDatasource: Datasource let datasource: Datasource @@ -71,15 +72,15 @@ describe.each( client = await knexClient(rawDatasource) - await client.schema.dropTableIfExists("test_table") - await client.schema.createTable("test_table", table => { + await client.schema.dropTableIfExists(mainTableName) + await client.schema.createTable(mainTableName, table => { table.increments("id").primary() table.string("name") table.timestamp("birthday") table.integer("number") }) - await client("test_table").insert([ + await client(mainTableName).insert([ { name: "one" }, { name: "two" }, { name: "three" }, @@ -105,7 +106,7 @@ describe.each( const query = await createQuery({ name: "New Query", fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, }) @@ -114,7 +115,7 @@ describe.each( name: "New Query", parameters: [], fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, schema: {}, queryVerb: "read", @@ -133,7 +134,7 @@ describe.each( it("should be able to update a query", async () => { const query = await createQuery({ fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, }) @@ -143,7 +144,7 @@ describe.each( ...query, name: "Updated Query", fields: { - sql: client("test_table").where({ id: 1 }).toString(), + sql: client(mainTableName).where({ id: 1 }).toString(), }, }) @@ -152,7 +153,7 @@ describe.each( name: "Updated Query", parameters: [], fields: { - sql: client("test_table").where({ id: 1 }).toString(), + sql: client(mainTableName).where({ id: 1 }).toString(), }, schema: {}, queryVerb: "read", @@ -169,7 +170,7 @@ describe.each( it("should be able to delete a query", async () => { const query = await createQuery({ fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, }) @@ -188,7 +189,7 @@ describe.each( it("should be able to list queries", async () => { const query = await createQuery({ fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, }) @@ -199,7 +200,7 @@ describe.each( it("should strip sensitive fields for prod apps", async () => { const query = await createQuery({ fields: { - sql: client("test_table").select("*").toString(), + sql: client(mainTableName).select("*").toString(), }, }) @@ -217,7 +218,7 @@ describe.each( const jsonStatement = `COALESCE(json_build_object('name', name),'{"name":{}}'::json)` const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .select([ "*", client.raw( @@ -245,7 +246,7 @@ describe.each( datasourceId: datasource._id!, queryVerb: "read", fields: { - sql: client("test_table").where({ id: 1 }).toString(), + sql: client(mainTableName).where({ id: 1 }).toString(), }, parameters: [], transformer: "return data", @@ -391,7 +392,7 @@ describe.each( it("should work with dynamic variables", async () => { const basedOnQuery = await createQuery({ fields: { - sql: client("test_table").select("name").where({ id: 1 }).toString(), + sql: client(mainTableName).select("name").where({ id: 1 }).toString(), }, }) @@ -440,7 +441,7 @@ describe.each( it("should handle the dynamic base query being deleted", async () => { const basedOnQuery = await createQuery({ fields: { - sql: client("test_table").select("name").where({ id: 1 }).toString(), + sql: client(mainTableName).select("name").where({ id: 1 }).toString(), }, }) @@ -494,7 +495,7 @@ describe.each( it("should be able to insert with bindings", async () => { const query = await createQuery({ fields: { - sql: client("test_table").insert({ name: "{{ foo }}" }).toString(), + sql: client(mainTableName).insert({ name: "{{ foo }}" }).toString(), }, parameters: [ { @@ -517,7 +518,7 @@ describe.each( }, ]) - const rows = await client("test_table").where({ name: "baz" }).select() + const rows = await client(mainTableName).where({ name: "baz" }).select() expect(rows).toHaveLength(1) for (const row of rows) { expect(row).toMatchObject({ name: "baz" }) @@ -527,7 +528,7 @@ describe.each( it("should not allow handlebars as parameters", async () => { const query = await createQuery({ fields: { - sql: client("test_table").insert({ name: "{{ foo }}" }).toString(), + sql: client(mainTableName).insert({ name: "{{ foo }}" }).toString(), }, parameters: [ { @@ -563,7 +564,7 @@ describe.each( const date = new Date(datetimeStr) const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .insert({ name: "foo", birthday: client.raw("{{ birthday }}"), @@ -585,7 +586,7 @@ describe.each( expect(result.data).toEqual([{ created: true }]) - const rows = await client("test_table") + const rows = await client(mainTableName) .where({ birthday: datetimeStr }) .select() expect(rows).toHaveLength(1) @@ -601,7 +602,7 @@ describe.each( async notDateStr => { const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .insert({ name: client.raw("{{ name }}") }) .toString(), }, @@ -622,7 +623,7 @@ describe.each( expect(result.data).toEqual([{ created: true }]) - const rows = await client("test_table") + const rows = await client(mainTableName) .where({ name: notDateStr }) .select() expect(rows).toHaveLength(1) @@ -634,7 +635,7 @@ describe.each( it("should execute a query", async () => { const query = await createQuery({ fields: { - sql: client("test_table").select("*").orderBy("id").toString(), + sql: client(mainTableName).select("*").orderBy("id").toString(), }, }) @@ -677,7 +678,7 @@ describe.each( it("should be able to transform a query", async () => { const query = await createQuery({ fields: { - sql: client("test_table").where({ id: 1 }).select("*").toString(), + sql: client(mainTableName).where({ id: 1 }).select("*").toString(), }, transformer: ` data[0].id = data[0].id + 1; @@ -700,7 +701,7 @@ describe.each( it("should coerce numeric bindings", async () => { const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .where({ id: client.raw("{{ id }}") }) .select("*") .toString(), @@ -734,7 +735,7 @@ describe.each( it("should be able to update rows", async () => { const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .update({ name: client.raw("{{ name }}") }) .where({ id: client.raw("{{ id }}") }) .toString(), @@ -759,7 +760,7 @@ describe.each( }, }) - const rows = await client("test_table").where({ id: 1 }).select() + const rows = await client(mainTableName).where({ id: 1 }).select() expect(rows).toEqual([ { id: 1, name: "foo", birthday: null, number: null }, ]) @@ -768,7 +769,7 @@ describe.each( it("should be able to execute an update that updates no rows", async () => { const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .update({ name: "updated" }) .where({ id: 100 }) .toString(), @@ -778,7 +779,7 @@ describe.each( await config.api.query.execute(query._id!) - const rows = await client("test_table").select() + const rows = await client(mainTableName).select() for (const row of rows) { expect(row.name).not.toEqual("updated") } @@ -787,14 +788,14 @@ describe.each( it("should be able to execute a delete that deletes no rows", async () => { const query = await createQuery({ fields: { - sql: client("test_table").where({ id: 100 }).delete().toString(), + sql: client(mainTableName).where({ id: 100 }).delete().toString(), }, queryVerb: "delete", }) await config.api.query.execute(query._id!) - const rows = await client("test_table").select() + const rows = await client(mainTableName).select() expect(rows).toHaveLength(5) }) }) @@ -803,7 +804,7 @@ describe.each( it("should be able to delete rows", async () => { const query = await createQuery({ fields: { - sql: client("test_table") + sql: client(mainTableName) .where({ id: client.raw("{{ id }}") }) .delete() .toString(), @@ -823,7 +824,7 @@ describe.each( }, }) - const rows = await client("test_table").where({ id: 1 }).select() + const rows = await client(mainTableName).where({ id: 1 }).select() expect(rows).toHaveLength(0) }) }) @@ -831,7 +832,7 @@ describe.each( describe("query through datasource", () => { it("should be able to query the datasource", async () => { - const entityId = "test_table" + const entityId = mainTableName await config.api.datasource.update({ ...datasource, entities: { @@ -876,7 +877,7 @@ describe.each( beforeAll(async () => { queryParams = { fields: { - sql: client("test_table") + sql: client(mainTableName) .insert({ name: client.raw("{{ bindingName }}"), number: client.raw("{{ bindingNumber }}"), @@ -929,4 +930,34 @@ describe.each( }) }) }) + + describe("edge cases", () => { + it("should find rows with a binding containing a slash", async () => { + const slashValue = "1/10" + await client(mainTableName).insert([{ name: slashValue }]) + + const query = await createQuery({ + fields: { + sql: client(mainTableName) + .select("*") + .where("name", "=", client.raw("{{ bindingName }}")) + .toString(), + }, + parameters: [ + { + name: "bindingName", + default: "", + }, + ], + queryVerb: "read", + }) + const results = await config.api.query.execute(query._id!, { + parameters: { + bindingName: slashValue, + }, + }) + expect(results).toBeDefined() + expect(results.data.length).toEqual(1) + }) + }) }) diff --git a/packages/server/src/integrations/mysql.ts b/packages/server/src/integrations/mysql.ts index 8b1ada4184..f78915fb49 100644 --- a/packages/server/src/integrations/mysql.ts +++ b/packages/server/src/integrations/mysql.ts @@ -24,8 +24,7 @@ import { checkExternalTables, HOST_ADDRESS, } from "./utils" -import dayjs from "dayjs" -import { NUMBER_REGEX } from "../utilities" +import { isDate, NUMBER_REGEX } from "../utilities" import { MySQLColumn } from "./base/types" import { getReadableErrorMessage } from "./base/errorMapping" import { sql } from "@budibase/backend-core" @@ -129,11 +128,7 @@ export function bindingTypeCoerce(bindings: SqlQueryBinding) { } // if not a number, see if it is a date - important to do in this order as any // integer will be considered a valid date - else if ( - /^\d/.test(binding) && - dayjs(binding).isValid() && - !binding.includes(",") - ) { + else if (isDate(binding)) { let value: any value = new Date(binding) if (isNaN(value)) { @@ -439,8 +434,7 @@ class MySQLIntegration extends Sql implements DatasourcePlus { dumpContent.push(createTableStatement) } - const schema = dumpContent.join("\n") - return schema + return dumpContent.join("\n") } finally { this.disconnect() } diff --git a/packages/server/src/utilities/index.ts b/packages/server/src/utilities/index.ts index ce6f2345ca..353cf77281 100644 --- a/packages/server/src/utilities/index.ts +++ b/packages/server/src/utilities/index.ts @@ -3,6 +3,7 @@ import { context } from "@budibase/backend-core" import { generateMetadataID } from "../db/utils" import { Document } from "@budibase/types" import stream from "stream" +import dayjs from "dayjs" const Readable = stream.Readable @@ -14,6 +15,17 @@ export const isDev = env.isDev export const NUMBER_REGEX = /^[+-]?([0-9]*[.])?[0-9]+$/g +export function isDate(str: string) { + // checks for xx/xx/xx or ISO date timestamp formats + return ( + /^\d{2}\/\d{2}\/\d{2,4}$|^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2})?)?$/.test( + str + ) && + dayjs(str).isValid() && + !str.includes(",") + ) +} + export function removeFromArray(array: any[], element: any) { const index = array.indexOf(element) if (index !== -1) { diff --git a/packages/server/src/utilities/tests/utils.spec.ts b/packages/server/src/utilities/tests/utils.spec.ts new file mode 100644 index 0000000000..10301f5f4e --- /dev/null +++ b/packages/server/src/utilities/tests/utils.spec.ts @@ -0,0 +1,30 @@ +import { isDate } from "../" + +describe("isDate", () => { + it("should handle DD/MM/YYYY", () => { + expect(isDate("01/01/2001")).toEqual(true) + }) + + it("should handle DD/MM/YY", () => { + expect(isDate("01/01/01")).toEqual(true) + }) + + it("should handle ISO format YYYY-MM-DD", () => { + expect(isDate("2001-01-01")).toEqual(true) + }) + + it("should handle ISO format with time (YYYY-MM-DDTHH:MM)", () => { + expect(isDate("2001-01-01T12:30")).toEqual(true) + }) + + it("should handle ISO format with full timestamp (YYYY-MM-DDTHH:MM:SS)", () => { + expect(isDate("2001-01-01T12:30:45")).toEqual(true) + }) + + it("should return false for invalid formats", () => { + expect(isDate("")).toEqual(false) + expect(isDate("1/10")).toEqual(false) + expect(isDate("random string")).toEqual(false) + expect(isDate("123456")).toEqual(false) + }) +}) From 44bd00a0d7be259b0d4a54b8421f6537337d948c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 21 Oct 2024 18:20:52 +0100 Subject: [PATCH 03/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 04/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 bdac304551a381077673deff9df18bf21c2459d0 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 17:20:27 +0100 Subject: [PATCH 05/26] Adding back test cases. --- .../src/api/routes/tests/queries/generic-sql.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts index 7501b43911..4e9a1e5548 100644 --- a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts +++ b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts @@ -18,11 +18,11 @@ import { Knex } from "knex" describe.each( [ - // DatabaseName.POSTGRES, + DatabaseName.POSTGRES, DatabaseName.MYSQL, - // DatabaseName.SQL_SERVER, - // DatabaseName.MARIADB, - // DatabaseName.ORACLE, + DatabaseName.SQL_SERVER, + DatabaseName.MARIADB, + DatabaseName.ORACLE, ].map(name => [name, getDatasource(name)]) )("queries (%s)", (dbName, dsProvider) => { const config = setup.getConfig() From bd37698055080d231e3d5ec355a0fb5d961e82df Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 17:42:10 +0100 Subject: [PATCH 06/26] Switching away from regex to use custom formats. --- packages/server/src/utilities/index.ts | 27 ++++++++++++++----- .../server/src/utilities/tests/utils.spec.ts | 4 +++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/server/src/utilities/index.ts b/packages/server/src/utilities/index.ts index 353cf77281..5bd81b754c 100644 --- a/packages/server/src/utilities/index.ts +++ b/packages/server/src/utilities/index.ts @@ -4,7 +4,9 @@ import { generateMetadataID } from "../db/utils" import { Document } from "@budibase/types" import stream from "stream" import dayjs from "dayjs" +import customParseFormat from "dayjs/plugin/customParseFormat" +dayjs.extend(customParseFormat) const Readable = stream.Readable export function wait(ms: number) { @@ -14,16 +16,27 @@ export function wait(ms: number) { export const isDev = env.isDev export const NUMBER_REGEX = /^[+-]?([0-9]*[.])?[0-9]+$/g +const ACCEPTED_DATE_FORMATS = [ + "MM/DD/YYYY", + "MM/DD/YY", + "DD/MM/YYYY", + "DD/MM/YY", + "YYYY/MM/DD", + "YYYY-MM-DD", + "YYYY-MM-DDTHH:mm", + "YYYY-MM-DDTHH:mm:ss", + "YYYY-MM-DDTHH:mm:ss[Z]", + "YYYY-MM-DDTHH:mm:ss.SSS[Z]", +] export function isDate(str: string) { // checks for xx/xx/xx or ISO date timestamp formats - return ( - /^\d{2}\/\d{2}\/\d{2,4}$|^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2})?)?$/.test( - str - ) && - dayjs(str).isValid() && - !str.includes(",") - ) + for (const format of ACCEPTED_DATE_FORMATS) { + if (dayjs(str, format, true).isValid()) { + return true + } + } + return false } export function removeFromArray(array: any[], element: any) { diff --git a/packages/server/src/utilities/tests/utils.spec.ts b/packages/server/src/utilities/tests/utils.spec.ts index 10301f5f4e..bd94b5fdd9 100644 --- a/packages/server/src/utilities/tests/utils.spec.ts +++ b/packages/server/src/utilities/tests/utils.spec.ts @@ -21,6 +21,10 @@ describe("isDate", () => { expect(isDate("2001-01-01T12:30:45")).toEqual(true) }) + it("should handle complete ISO format", () => { + expect(isDate("2001-01-01T12:30:00.000Z")).toEqual(true) + }) + it("should return false for invalid formats", () => { expect(isDate("")).toEqual(false) expect(isDate("1/10")).toEqual(false) From 56a68db1d4e9ed2046c25b0708afc37e1f3522db Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 22 Oct 2024 18:33:44 +0100 Subject: [PATCH 07/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 309506adab54ca89cb0612650afbddedcf8761b1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 23 Oct 2024 15:05:41 +0100 Subject: [PATCH 08/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 09/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 10/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 11/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 12/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 13/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 14/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 15/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 16/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 17/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 18/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 19/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 20/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 21/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 22/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 23/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 24/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 25/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 26/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() {