Merge pull request #14861 from Budibase/sql-security

Add tests for SQL injection attacks on table/view creation and search.

packages/backend-core/src/sql/sql.ts

@@ -13,6 +13,7 @@ import SqlTableQueryBuilder from "./sqlTable"
 import {
   Aggregation,
   AnySearchFilter,
+  ArrayFilter,
   ArrayOperator,
   BasicOperator,
   BBReferenceFieldMetadata,
@@ -98,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<SearchFilterKey, boolean> = {
   [BasicOperator.EQUAL]: false,
   [BasicOperator.NOT_EQUAL]: true,
@@ -152,30 +170,30 @@ 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] {
+  private supportsILike(): boolean {
-    switch (this.client) {
+    return !(
-      case SqlClient.ORACLE:
+      this.client === SqlClient.ORACLE || this.client === SqlClient.SQL_LITE
-      case SqlClient.POSTGRES:
+    )
-        return ['"', '"']
-      case SqlClient.MS_SQL:
-        return ["[", "]"]
-      case SqlClient.MARIADB:
-      case SqlClient.MY_SQL:
-      case SqlClient.SQL_LITE:
-        return ["`", "`"]
-    }
   }
 
-  // Takes a string like foo and returns a quoted string like [foo] for SQL Server
+  private quoteChars(): [string, string] {
-  // and "foo" for Postgres.
+    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.
   private quote(str: string): string {
-    const [start, end] = this.quoteChars()
+    return this.knexClient.wrapIdentifier(str, {})
-    return `${start}${str}${end}`
   }
 
   private isQuoted(key: string): boolean {
@@ -193,6 +211,30 @@ 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.
+  //
+  // 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()
@@ -236,7 +278,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
@@ -258,30 +300,40 @@ class InternalBuilder {
         const columnSchema = schema[column]
 
         if (this.SPECIAL_SELECT_CASES.POSTGRES_MONEY(columnSchema)) {
-          return this.knex.raw(
+          // TODO: figure out how to express this safely without string
-            `${this.quotedIdentifier(
+          // interpolation.
-              [table, column].join(".")
+          return this.knex.raw(`??::money::numeric as "${field}"`, [
-            )}::money::numeric as ${this.quote(field)}`
+            this.rawQuotedIdentifier([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}"`, [
+            this.rawQuotedIdentifier(field),
+          ])
         }
 
-        const quoted = table
+        if (table) {
-          ? `${this.quote(table)}.${this.quote(column)}`
+          return this.rawQuotedIdentifier(`${table}.${column}`)
-          : this.quote(field)
+        } else {
-        return this.knex.raw(quoted)
+          return this.rawQuotedIdentifier(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 +342,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.rawQuotedIdentifier(field)
 
     if (
       schema.type === FieldType.STRING ||
@@ -301,9 +353,12 @@ 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,
+          this.rawQuotedIdentifier(col),
+        ])
       } else {
-        identifier = `to_char(${identifier})`
+        identifier = this.knex.raw("to_char(??)", [identifier])
       }
     }
     return identifier
@@ -427,7 +482,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
@@ -449,8 +503,8 @@ class InternalBuilder {
         relationship.to &&
         relationship.tableName
       ) {
-        const joinTable = mainKnex
+        const joinTable = this.knex
-          .select(mainKnex.raw(1))
+          .select(this.knex.raw(1))
           .from({ [toAlias]: relatedTableName })
         let subQuery = joinTable.clone()
         const manyToMany = validateManyToMany(relationship)
@@ -485,9 +539,7 @@ class InternalBuilder {
             .where(
               `${throughAlias}.${manyToMany.from}`,
               "=",
-              mainKnex.raw(
+              this.rawQuotedIdentifier(`${fromAlias}.${manyToMany.fromPrimary}`)
-                this.quotedIdentifier(`${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 +568,7 @@ class InternalBuilder {
           subQuery = subQuery.where(
             toKey,
             "=",
-            mainKnex.raw(this.quotedIdentifier(foreignKey))
+            this.rawQuotedIdentifier(foreignKey)
           )
 
           query = query.where(q => {
@@ -546,7 +598,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
 
@@ -610,7 +662,7 @@ class InternalBuilder {
             value
           )
         } else if (shouldProcessRelationship) {
-          if (allOr) {
+          if (shouldOr) {
             query = query.or
           }
           query = builder.addRelationshipForFilter(
@@ -626,85 +678,102 @@ class InternalBuilder {
     }
 
     const like = (q: Knex.QueryBuilder, key: string, value: any) => {
-      const fuzzyOr = filters?.fuzzyOr
+      if (filters?.fuzzyOr || shouldOr) {
-      const fnc = fuzzyOr || allOr ? "orWhere" : "where"
+        q = q.or
-      // postgres supports ilike, nothing else does
+      }
-      if (this.client === SqlClient.POSTGRES) {
+      if (
-        return q[fnc](key, "ilike", `%${value}%`)
+        this.client === SqlClient.ORACLE ||
-      } else {
+        this.client === SqlClient.SQL_LITE
-        const rawFnc = `${fnc}Raw`
+      ) {
-        // @ts-ignore
+        return q.whereRaw(`LOWER(??) LIKE ?`, [
-        return q[rawFnc](`LOWER(${this.quotedIdentifier(key)}) LIKE ?`, [
+          this.rawQuotedIdentifier(key),
           `%${value.toLowerCase()}%`,
         ])
       }
+      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 contains = (mode: ArrayFilter, any = false) => {
-      const rawFnc = allOr ? "orWhereRaw" : "whereRaw"
+      function addModifiers<T extends {}, Q>(q: Knex.QueryBuilder<T, Q>) {
-      const not = mode === filters?.notContains ? "NOT " : ""
+        if (shouldOr || mode === filters?.containsAny) {
-      function stringifyArray(value: Array<any>, quoteStyle = '"'): string {
+          q = q.or
-        for (let i in value) {
-          if (typeof value[i] === "string") {
-            value[i] = `${quoteStyle}${value[i]}${quoteStyle}`
-          }
         }
-        return `[${value.join(",")}]`
+        if (mode === filters?.notContains) {
+          q = q.not
+        }
+        return q
       }
+
       if (this.client === SqlClient.POSTGRES) {
         iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => {
-          const wrap = any ? "" : "'"
+          q = addModifiers(q)
-          const op = any ? "\\?| array" : "@>"
+          if (any) {
-          const fieldNames = key.split(/\./g)
+            return q.whereRaw(`COALESCE(??::jsonb \\?| array??, FALSE)`, [
-          const table = fieldNames[0]
+              this.rawQuotedIdentifier(key),
-          const col = fieldNames[1]
+              this.knex.raw(stringifyArray(value, "'")),
-          return q[rawFnc](
+            ])
-            `${not}COALESCE("${table}"."${col}"::jsonb ${op} ${wrap}${stringifyArray(
+          } else {
-              value,
+            return q.whereRaw(`COALESCE(??::jsonb @> '??', FALSE)`, [
-              any ? "'" : '"'
+              this.rawQuotedIdentifier(key),
-            )}${wrap}, FALSE)`
+              this.knex.raw(stringifyArray(value)),
-          )
+            ])
+          }
         })
       } 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](
+          return addModifiers(q).whereRaw(`COALESCE(?(??, ?), FALSE)`, [
-            `${not}COALESCE(${jsonFnc}(${key}, '${stringifyArray(
+            this.knex.raw(any ? "JSON_OVERLAPS" : "JSON_CONTAINS"),
-              value
+            this.rawQuotedIdentifier(key),
-            )}'), FALSE)`
+            this.knex.raw(wrap(stringifyArray(value))),
-          )
+          ])
         })
       } else {
-        const andOr = mode === filters?.containsAny ? " OR " : " AND "
         iterate(mode, ArrayOperator.CONTAINS, (q, key, value) => {
-          let statement = ""
+          if (value.length === 0) {
-          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 === "") {
             return q
           }
 
-          if (not) {
+          q = q.where(subQuery => {
-            return q[rawFnc](
+            if (mode === filters?.notContains) {
-              `(NOT (${statement}) OR ${identifier} IS NULL)`,
+              subQuery = subQuery.not
-              value
+            }
-            )
+
-          } else {
+            subQuery = subQuery.where(subSubQuery => {
-            return q[rawFnc](statement, value)
+              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) {
+              subQuery = subQuery.or.whereNull(
+                // @ts-expect-error knex types are wrong, raw is fine here
+                this.rawQuotedIdentifier(key)
+              )
+            }
+            return subQuery
+          })
+          return q
         })
       }
     }
@@ -730,45 +799,46 @@ class InternalBuilder {
     }
 
     if (filters.oneOf) {
-      const fnc = allOr ? "orWhereIn" : "whereIn"
       iterate(
         filters.oneOf,
         ArrayOperator.ONE_OF,
         (q, key: string, array) => {
-          if (this.client === SqlClient.ORACLE) {
+          if (shouldOr) {
-            key = this.convertClobs(key)
+            q = q.or
-            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])
           }
+          if (this.client === SqlClient.ORACLE) {
+            // @ts-ignore
+            key = this.convertClobs(key)
+          }
+          return q.whereIn(key, Array.isArray(array) ? array : [array])
         },
         (q, key: string[], array) => {
-          if (this.client === SqlClient.ORACLE) {
+          if (shouldOr) {
-            const keyStr = `(${key.map(k => this.convertClobs(k)).join(",")})`
+            q = q.or
-            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])
           }
+          if (this.client === SqlClient.ORACLE) {
+            // @ts-ignore
+            key = key.map(k => this.convertClobs(k))
+          }
+          return q.whereIn(key, Array.isArray(array) ? array : [array])
         }
       )
     }
     if (filters.string) {
       iterate(filters.string, BasicOperator.STRING, (q, key, value) => {
-        const fnc = allOr ? "orWhere" : "where"
+        if (shouldOr) {
-        // postgres supports ilike, nothing else does
+          q = q.or
-        if (this.client === SqlClient.POSTGRES) {
+        }
-          return q[fnc](key, "ilike", `${value}%`)
+        if (
-        } else {
+          this.client === SqlClient.ORACLE ||
-          const rawFnc = `${fnc}Raw`
+          this.client === SqlClient.SQL_LITE
-          // @ts-ignore
+        ) {
-          return q[rawFnc](`LOWER(${this.quotedIdentifier(key)}) LIKE ?`, [
+          return q.whereRaw(`LOWER(??) LIKE ?`, [
+            this.rawQuotedIdentifier(key),
             `${value.toLowerCase()}%`,
           ])
+        } else {
+          return q.whereILike(key, `${value}%`)
         }
       })
     }
@@ -795,67 +865,59 @@ 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
+          rawKey = this.convertClobs(key)
-          key = this.knex.raw(this.convertClobs(key))
+        } else if (
+          this.client === SqlClient.SQL_LITE &&
+          schema?.type === FieldType.BIGINT
+        ) {
+          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])
+        }
+
+        if (shouldOr) {
+          q = q.or
         }
 
         if (lowValid && highValid) {
-          if (
+          // @ts-expect-error knex types are wrong, raw is fine here
-            schema?.type === FieldType.BIGINT &&
+          return q.whereBetween(rawKey, [low, high])
-            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])
-          }
         } else if (lowValid) {
-          if (
+          // @ts-expect-error knex types are wrong, raw is fine here
-            schema?.type === FieldType.BIGINT &&
+          return q.where(rawKey, ">=", low)
-            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)
-          }
         } else if (highValid) {
-          if (
+          // @ts-expect-error knex types are wrong, raw is fine here
-            schema?.type === FieldType.BIGINT &&
+          return q.where(rawKey, "<=", high)
-            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)
-          }
         }
         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](
+          return q.whereRaw(`CASE WHEN ?? = ? THEN 1 ELSE 0 END = 1`, [
-            `CASE WHEN ${this.quotedIdentifier(key)} = ? THEN 1 ELSE 0 END = 1`,
+            this.rawQuotedIdentifier(key),
-            [value]
-          )
-        } else if (this.client === SqlClient.ORACLE) {
-          const identifier = this.convertClobs(key)
-          return q[fnc](`(${identifier} IS NOT NULL AND ${identifier} = ?)`, [
             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,
           ])
         }
@@ -863,20 +925,30 @@ class InternalBuilder {
     }
     if (filters.notEqual) {
       iterate(filters.notEqual, BasicOperator.NOT_EQUAL, (q, key, value) => {
-        const fnc = allOr ? "orWhereRaw" : "whereRaw"
+        if (shouldOr) {
+          q = q.or
+        }
         if (this.client === SqlClient.MS_SQL) {
-          return q[fnc](
+          return q.whereRaw(`CASE WHEN ?? = ? THEN 1 ELSE 0 END = 0`, [
-            `CASE WHEN ${this.quotedIdentifier(key)} = ? THEN 1 ELSE 0 END = 0`,
+            this.rawQuotedIdentifier(key),
-            [value]
+            value,
-          )
+          ])
         } else if (this.client === SqlClient.ORACLE) {
           const identifier = this.convertClobs(key)
-          return q[fnc](
+          return (
-            `(${identifier} IS NOT NULL AND ${identifier} != ?) OR ${identifier} IS NULL`,
+            q
-            [value]
+              .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,
           ])
         }
@@ -884,14 +956,18 @@ class InternalBuilder {
     }
     if (filters.empty) {
       iterate(filters.empty, BasicOperator.EMPTY, (q, key) => {
-        const fnc = allOr ? "orWhereNull" : "whereNull"
+        if (shouldOr) {
-        return q[fnc](key)
+          q = q.or
+        }
+        return q.whereNull(key)
       })
     }
     if (filters.notEmpty) {
       iterate(filters.notEmpty, BasicOperator.NOT_EMPTY, (q, key) => {
-        const fnc = allOr ? "orWhereNotNull" : "whereNotNull"
+        if (shouldOr) {
-        return q[fnc](key)
+          q = q.or
+        }
+        return q.whereNotNull(key)
       })
     }
     if (filters.contains) {
@@ -976,9 +1052,7 @@ class InternalBuilder {
         const selectFields = qualifiedFields.map(field =>
           this.convertClobs(field, { forSelect: true })
         )
-        query = query
+        query = query.groupBy(groupByFields).select(selectFields)
-          .groupByRaw(groupByFields.join(", "))
-          .select(this.knex.raw(selectFields.join(", ")))
       } else {
         query = query.groupBy(qualifiedFields).select(qualifiedFields)
       }
@@ -990,11 +1064,10 @@ class InternalBuilder {
           if (this.client === SqlClient.ORACLE) {
             const field = this.convertClobs(`${tableName}.${aggregation.field}`)
             query = query.select(
-              this.knex.raw(
+              this.knex.raw(`COUNT(DISTINCT ??) as ??`, [
-                `COUNT(DISTINCT ${field}) as ${this.quotedIdentifier(
+                field,
-                  aggregation.name
+                aggregation.name,
-                )}`
+              ])
-              )
             )
           } else {
             query = query.countDistinct(
@@ -1059,9 +1132,11 @@ class InternalBuilder {
         } else {
           let composite = `${aliased}.${key}`
           if (this.client === SqlClient.ORACLE) {
-            query = query.orderByRaw(
+            query = query.orderByRaw(`?? ?? nulls ??`, [
-              `${this.convertClobs(composite)} ${direction} nulls ${nulls}`
+              this.convertClobs(composite),
-            )
+              this.knex.raw(direction),
+              this.knex.raw(nulls as string),
+            ])
           } else {
             query = query.orderBy(composite, direction, nulls)
           }
@@ -1091,17 +1166,22 @@ 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, this.rawQuotedIdentifier(tableField)])
+      .toString()
   }
 
   maxFunctionParameters() {
@@ -1197,13 +1277,13 @@ class InternalBuilder {
       subQuery = subQuery.where(
         correlatedTo,
         "=",
-        knex.raw(this.quotedIdentifier(correlatedFrom))
+        this.rawQuotedIdentifier(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,
         })
       }
@@ -1213,12 +1293,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:
@@ -1232,21 +1312,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 ?? = (${comparatorQuery} FOR JSON PATH))`,
-              .select(`${fromAlias}.*`)
+            [this.rawQuotedIdentifier(toAlias)]
-              // @ts-ignore - from alias syntax not TS supported
-              .from({
-                [fromAlias]: subQuery
-                  .select(`${toAlias}.*`)
-                  .limit(getRelationshipLimit()),
-              })} FOR JSON PATH))`
           )
           break
+        }
         default:
           throw new Error(`JSON relationships not implement for ${sqlClient}`)
       }

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,
@@ -61,6 +62,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()
 
@@ -129,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(),
@@ -153,22 +155,51 @@ describe.each([
     rows = await config.api.row.fetch(tableOrViewId)
   }
 
+  async function getTable(tableOrViewId: string): Promise<Table> {
+    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)
+    }
+  }
+
+  async function assertTableExists(nameOrTable: string | Table) {
+    const name =
+      typeof nameOrTable === "string" ? nameOrTable : nameOrTable.name
+    expect(await client!.schema.hasTable(name)).toBeTrue()
+  }
+
+  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([
     ["table", createTable],
     [
       "view",
-      async (schema: TableSchema) => {
+      async (schema?: TableSchema) => {
         const tableId = await createTable(schema)
         const viewId = await createView(
           tableId,
-          Object.keys(schema).reduce<ViewV2Schema>((viewSchema, fieldName) => {
+          Object.keys(schema || {}).reduce<ViewV2Schema>(
-            const field = schema[fieldName]
+            (viewSchema, fieldName) => {
-            viewSchema[fieldName] = {
+              const field = schema![fieldName]
-              visible: field.visible ?? true,
+              viewSchema[fieldName] = {
-              readonly: false,
+                visible: field.visible ?? true,
-            }
+                readonly: false,
-            return viewSchema
+              }
-          }, {})
+              return viewSchema
+            },
+            {}
+          )
         )
         return viewId
       },
@@ -792,10 +823,11 @@ describe.each([
         })
       })
 
-    describe.each([FieldType.STRING, FieldType.LONGFORM])("%s", () => {
+    const stringTypes = [FieldType.STRING, FieldType.LONGFORM] as const
+    describe.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 +1634,7 @@ describe.each([
         })
       })
 
-    describe.each([FieldType.ARRAY, FieldType.OPTIONS])("%s", () => {
+    describe("arrays", () => {
       beforeAll(async () => {
         tableOrViewId = await createTableOrView({
           numbers: {
@@ -3470,5 +3502,105 @@ describe.each([
           ])
         })
       })
+
+    isSql &&
+      !isSqs &&
+      describe("SQL injection", () => {
+        const badStrings = [
+          "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 %table_name%; --",
+          "' OR 1=1 --",
+          "' UNION SELECT null, null, null; --",
+          "' AND (SELECT COUNT(*) FROM %table_name%) > 0 --",
+          "\"; EXEC xp_cmdshell('dir'); --",
+          "\"' OR 'a'='a",
+          "OR 1=1;",
+          "'; SHUTDOWN --",
+        ]
+
+        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 && badStringTemplate.includes('"')
+
+          !skipFieldNameCheck &&
+            it("should not allow SQL injection as a field name", async () => {
+              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: {
+                  ...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)
+              await assertTableNumRows(table, 1)
+
+              const { rows } = await config.api.row.search(
+                tableOrViewId,
+                { query: {} },
+                { status: 200 }
+              )
+
+              expect(rows).toHaveLength(1)
+
+              await assertTableExists(table)
+              await assertTableNumRows(table, 1)
+            })
+
+          it("should not allow SQL injection as a field value", async () => {
+            const tableOrViewId = await createTableOrView({
+              foo: {
+                name: "foo",
+                type: FieldType.STRING,
+              },
+            })
+            const table = await getTable(tableOrViewId)
+            const badString = badStringTemplate.replace(
+              /%table_name%/g,
+              table.name
+            )
+
+            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 } } },
+              { status: 200 }
+            )
+
+            expect(rows).toBeEmpty()
+            await assertTableExists(table)
+            await assertTableNumRows(table, 1)
+          })
+        })
+      })
   })
 })

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`,
     })
   })
 })

packages/types/src/sdk/search.ts

@@ -49,7 +49,7 @@ type BasicFilter<T = any> = Record<string, T> & {
   [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: never
 }
 
-type ArrayFilter = Record<string, any[]> & {
+export type ArrayFilter = Record<string, any[]> & {
   [InternalSearchFilterOperator.COMPLEX_ID_OPERATOR]?: {
     id: string[]
     values: string[]