From 0736812293361ff53936a82f58682c0298aae9e3 Mon Sep 17 00:00:00 2001
From: Sam Rose <hello@samwho.dev>
Date: Thu, 24 Oct 2024 11:39:57 +0100
Subject: [PATCH] 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 }
+          )
+        })
+      })
+    })
   })
 })