Security advisory GHSA-4948-f92q-f432: @nocobase/database has SQL Injection via String Concatenation through Recursive Eager Loading
Package: @nocobase/database (npm), vulnerable version: 2.0.38, fixed in: 2.0.39
CWE: CWE-89
Files changed by the fix:
packages/core/database/src/eager-loading/eager-loading-tree.ts
packages/plugins/@nocobase/plugin-field-sort/src/server/sort-field.ts

Fix diff:
diff --git a/packages/core/database/src/__tests__/eager-loading/eager-loading-tree.test.ts b/packages/core/database/src/__tests__/eager-loading/eager-loading-tree.test.ts
index 49b1a5c50c697..902655a8db8a1 100644
--- a/packages/core/database/src/__tests__/eager-loading/eager-loading-tree.test.ts
+++ b/packages/core/database/src/__tests__/eager-loading/eager-loading-tree.test.ts
@@ -620,4 +620,63 @@ describe('Eager loading tree', () => {
     expect(u1.get('posts')[0].get('tags')[0].get('tagCategory')).toBeDefined();
     expect(u1.get('posts')[0].get('tags')[0].get('tagCategory').get('name')).toBe('c1');
   });
+
+  it('should use bind parameters when loading parent recursively with string primary keys', async () => {
+    const payload = `root') UNION ALL SELECT 'pwned', NULL WHERE ('1'='1`;
+    const Tree = db.collection({
+      name: 'categories',
+      tree: 'adjacency-list',
+      fields: [
+        { type: 'string', name: 'id', primaryKey: true },
+        { type: 'string', name: 'name' },
+        {
+          type: 'belongsTo',
+          name: 'parent',
+          treeParent: true,
+          target: 'categories',
+          foreignKey: 'parentId',
+          targetKey: 'id',
+        },
+        {
+          type: 'hasMany',
+          name: 'children',
+          treeChildren: true,
+          target: 'categories',
+          foreignKey: 'parentId',
+          sourceKey: 'id',
+        },
+      ],
+    });
+
+    await db.sync();
+
+    await Tree.repository.create({
+      values: [
+        { id: 'root', name: 'root' },
+        { id: payload, name: 'payload' },
+        { id: 'child', name: 'child', parentId: payload },
+      ],
+    });
+
+    const querySpy = vi.spyOn(db.sequelize, 'query');
+
+    const child = await Tree.repository.findOne({
+      where: { id: 'child' },
+      appends: ['parent(recursively=true)'],
+    });
+
+    expect(child.parent.id).toBe(payload);
+
+    const recursiveQueryCall = querySpy.mock.calls.find(
+      ([sql]) => typeof sql === 'string' && sql.includes('WITH RECURSIVE cte'),
+    );
+
+    expect(recursiveQueryCall).toBeDefined();
+    expect(recursiveQueryCall[0]).toContain('IN ($1)');
+    expect(recursiveQueryCall[0]).not.toContain(payload);
+    expect(recursiveQueryCall[1]).toMatchObject({
+      bind: [payload],
+      type: 'SELECT',
+    });
+  });
 });
diff --git a/packages/core/database/src/eager-loading/eager-loading-tree.ts b/packages/core/database/src/eager-loading/eager-loading-tree.ts
index ab497845faa90..36dbb0fcd63d0 100644
--- a/packages/core/database/src/eager-loading/eager-loading-tree.ts
+++ b/packages/core/database/src/eager-loading/eager-loading-tree.ts
@@ -71,16 +71,20 @@ const queryParentSQL = (options: {
 
   const queryInterface = db.sequelize.getQueryInterface();
   const q = queryInterface.quoteIdentifier.bind(queryInterface);
-  return `WITH RECURSIVE cte AS (
+  const placeholders = nodeIds.map((_, index) => `$${index + 1}`).join(', ');
+  return {
+    sql: `WITH RECURSIVE cte AS (
       SELECT ${q(targetKeyField)}, ${q(foreignKeyField)}
       FROM ${tableName}
-      WHERE ${q(targetKeyField)} IN ('${nodeIds.join("','")}')
+      WHERE ${q(targetKeyField)} IN (${placeholders})
       UNION ALL
       SELECT t.${q(targetKeyField)}, t.${q(foreignKeyField)}
       FROM ${tableName} AS t
       INNER JOIN cte ON t.${q(targetKeyField)} = cte.${q(foreignKeyField)}
       )
-      SELECT ${q(targetKeyField)} AS ${q(targetKey)}, ${q(foreignKeyField)} AS ${q(foreignKey)} FROM cte`;
+      SELECT ${q(targetKeyField)} AS ${q(targetKey)}, ${q(foreignKeyField)} AS ${q(foreignKey)} FROM cte`,
+    bind: nodeIds,
+  };
 };
 
 export class EagerLoadingTree {
@@ -381,7 +385,7 @@ export class EagerLoadingTree {
           // load parent instances recursively
           if (node.includeOption.recursively && instances.length > 0) {
             const targetKey = association.targetKey;
-            const sql = queryParentSQL({
+            const { sql, bind } = queryParentSQL({
               db: this.db,
               collection,
               foreignKey,
@@ -390,6 +394,7 @@ export class EagerLoadingTree {
             });
 
             const results = await this.db.sequelize.query(sql, {
+              bind,
               type: 'SELECT',
               transaction,
             });
diff --git a/packages/plugins/@nocobase/plugin-field-sort/src/server/__tests__/sort.test.ts b/packages/plugins/@nocobase/plugin-field-sort/src/server/__tests__/sort.test.ts
index 85ce42325a2f1..fc56ae0ddf6b3 100644
--- a/packages/plugins/@nocobase/plugin-field-sort/src/server/__tests__/sort.test.ts
+++ b/packages/plugins/@nocobase/plugin-field-sort/src/server/__tests__/sort.test.ts
@@ -218,6 +218,58 @@ describe('sort field', () => {
       expect(r5.get('sort')).toBe(1);
     });
 
+    it('should use bind parameters when initializing scoped sort values', async () => {
+      const payload = `group') OR 1=1 --`;
+      const Test = db.collection({
+        name: 'tests',
+        fields: [
+          {
+            type: 'string',
+            name: 'name',
+          },
+          {
+            type: 'string',
+            name: 'group',
+          },
+        ],
+      });
+
+      await db.sync();
+      await Test.repository.create({
+        values: [
+          {
+            group: payload,
+            name: 'r1',
+          },
+          {
+            group: payload,
+            name: 'r2',
+          },
+        ],
+      });
+
+      const querySpy = vi.spyOn(db.sequelize, 'query');
+
+      Test.setField('sort', { type: 'sort', scopeKey: 'group' });
+
+      await db.sync();
+
+      const sortInitQueryCall = querySpy.mock.calls.find(
+        ([sql, options]) =>
+          typeof sql === 'string' &&
+          sql.includes('ROW_NUMBER() OVER') &&
+          Array.isArray(options?.bind) &&
+          options.bind.includes(payload),
+      );
+
+      expect(sortInitQueryCall).toBeDefined();
+      expect(sortInitQueryCall[0]).toContain('IN ($1)');
+      expect(sortInitQueryCall[0]).not.toContain(payload);
+      expect(sortInitQueryCall[1]).toMatchObject({
+        bind: [payload],
+      });
+    });
+
     it('should init sorted value by createdAt when primaryKey not exists', async () => {
       const Test = db.collection({
         autoGenId: false,
diff --git a/packages/plugins/@nocobase/plugin-field-sort/src/server/sort-field.ts b/packages/plugins/@nocobase/plugin-field-sort/src/server/sort-field.ts
index af118393cf390..d931670e1666c 100644
--- a/packages/plugins/@nocobase/plugin-field-sort/src/server/sort-field.ts
+++ b/packages/plugins/@nocobase/plugin-field-sort/src/server/sort-field.ts
@@ -120,21 +120,29 @@ export class SortField extends Field {
       const sortColumnName = queryInterface.quoteIdentifier(this.collection.model.rawAttributes[this.name].field);
 
       let sql: string;
+      let bind: any[] | undefined;
 
       const whereClause =
         scopeKey && scopeValue
           ? (() => {
               const filteredScopeValue = scopeValue.filter((v) => v !== null);
-              if (filteredScopeValue.length === 0) {
-                return '';
+              const clauses: string[] = [];
+
+              if (filteredScopeValue.length > 0) {
+                bind = filteredScopeValue;
+                const placeholders = filteredScopeValue.map((_, index) => `$${index + 1}`).join(', ');
+                clauses.push(`${queryInterface.quoteIdentifier(scopeKey)} IN (${placeholders})`);
+              }
+
+              if (scopeValue.includes(null)) {
+                clauses.push(`${queryInterface.quoteIdentifier(scopeKey)} IS NULL`);
               }
-              const initialClause = `
-  WHERE ${queryInterface.quoteIdentifier(scopeKey)} IN (${filteredScopeValue.map((v) => `'${v}'`).join(', ')})`;
 
-              const nullCheck = scopeValue.includes(null)
-                ? ` OR ${queryInterface.quoteIdentifier(scopeKey)} IS NULL`
-                : '';
-              return initialClause + nullCheck;
+              if (clauses.length === 0) {
+                return '';
+              }
+              return `
+  WHERE ${clauses.join(' OR ')}`;
             })()
           : '';
 
@@ -180,6 +188,7 @@ export class SortField extends Field {
   `;
       }
       await this.collection.db.sequelize.query(sql, {
+        bind,
         transaction,
       });
     };