Merge pull request #6770 from nocodb/fix/6342-incorrect-filter

fix: Validate filter before bulk delete/update operations

packages/nocodb/src/db/BaseModelSqlv2.ts

@@ -138,9 +138,11 @@ class BaseModelSqlv2 {
     {
       ignoreView = false,
       getHiddenColumn = false,
+      throwErrorIfInvalidParams = false,
     }: {
       ignoreView?: boolean;
       getHiddenColumn?: boolean;
+      throwErrorIfInvalidParams?: boolean;
     } = {},
   ): Promise<any> {
     const qb = this.dbDriver(this.tnPath);
@@ -150,6 +152,7 @@ class BaseModelSqlv2 {
       model: this.model,
       view: ignoreView ? null : this.viewId && (await View.get(this.viewId)),
       getHiddenColumn,
+      throwErrorIfInvalidParams,
     });
 
     await this.selectObject({
@@ -265,6 +268,7 @@ class BaseModelSqlv2 {
     } = {},
     ignoreViewFilterAndSort = false,
     validateFormula = false,
+    throwErrorIfInvalidParams = false,
   ): Promise<any> {
     const { where, fields, ...rest } = this._getListArgs(args as any);
 
@@ -281,8 +285,16 @@ class BaseModelSqlv2 {
     }
 
     const aliasColObjMap = await this.model.getAliasColObjMap();
-    let sorts = extractSortsObject(rest?.sort, aliasColObjMap);
+    let sorts = extractSortsObject(
-    const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
+      rest?.sort,
+      aliasColObjMap,
+      throwErrorIfInvalidParams,
+    );
+    const filterObj = extractFilterFromXwhere(
+      where,
+      aliasColObjMap,
+      throwErrorIfInvalidParams,
+    );
     // todo: replace with view id
     if (!ignoreViewFilterAndSort && this.viewId) {
       await conditionV2(
@@ -305,6 +317,8 @@ class BaseModelSqlv2 {
           }),
         ],
         qb,
+        undefined,
+        throwErrorIfInvalidParams,
       );
 
       if (!sorts)
@@ -312,7 +326,7 @@ class BaseModelSqlv2 {
           ? args.sortArr
           : await Sort.list({ viewId: this.viewId });
 
-      await sortV2(this, sorts, qb);
+      await sortV2(this, sorts, qb, undefined, throwErrorIfInvalidParams);
     } else {
       await conditionV2(
         this,
@@ -329,11 +343,13 @@ class BaseModelSqlv2 {
           }),
         ],
         qb,
+        undefined,
+        throwErrorIfInvalidParams,
       );
 
       if (!sorts) sorts = args.sortArr;
 
-      await sortV2(this, sorts, qb);
+      await sortV2(this, sorts, qb, undefined, throwErrorIfInvalidParams);
     }
 
     // sort by primary key if not autogenerated string
@@ -366,6 +382,7 @@ class BaseModelSqlv2 {
   public async count(
     args: { where?: string; limit?; filterArr?: Filter[] } = {},
     ignoreViewFilterAndSort = false,
+    throwErrorIfInvalidParams = false,
   ): Promise<any> {
     await this.model.getColumns();
     const { where } = this._getListArgs(args);
@@ -374,7 +391,11 @@ class BaseModelSqlv2 {
 
     // qb.xwhere(where, await this.model.getAliasColMapping());
     const aliasColObjMap = await this.model.getAliasColObjMap();
-    const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
+    const filterObj = extractFilterFromXwhere(
+      where,
+      aliasColObjMap,
+      throwErrorIfInvalidParams,
+    );
 
     if (!ignoreViewFilterAndSort && this.viewId) {
       await conditionV2(
@@ -398,6 +419,8 @@ class BaseModelSqlv2 {
           ...(args.filterArr || []),
         ],
         qb,
+        undefined,
+        throwErrorIfInvalidParams,
       );
     } else {
       await conditionV2(
@@ -416,6 +439,8 @@ class BaseModelSqlv2 {
           ...(args.filterArr || []),
         ],
         qb,
+        undefined,
+        throwErrorIfInvalidParams,
       );
     }
 
@@ -2991,7 +3016,7 @@ class BaseModelSqlv2 {
         const { where } = this._getListArgs(args);
         const qb = this.dbDriver(this.tnPath);
         const aliasColObjMap = await this.model.getAliasColObjMap();
-        const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
+        const filterObj = extractFilterFromXwhere(where, aliasColObjMap, true);
 
         const conditionObj = [
           new Filter({
@@ -3177,7 +3202,7 @@ class BaseModelSqlv2 {
       const { where } = this._getListArgs(args);
       const qb = this.dbDriver(this.tnPath);
       const aliasColObjMap = await this.model.getAliasColObjMap();
-      const filterObj = extractFilterFromXwhere(where, aliasColObjMap);
+      const filterObj = extractFilterFromXwhere(where, aliasColObjMap, true);
 
       await conditionV2(
         this,
@@ -3194,6 +3219,8 @@ class BaseModelSqlv2 {
           }),
         ],
         qb,
+        undefined,
+        true,
       );
       const execQueries: ((trx: Knex.Transaction, qb: any) => Promise<any>)[] =
         [];
@@ -4846,6 +4873,7 @@ class BaseModelSqlv2 {
 export function extractSortsObject(
   _sorts: string | string[],
   aliasColObjMap: { [columnAlias: string]: Column },
+  throwErrorIfInvalid = false,
 ): Sort[] {
   if (!_sorts?.length) return;
 
@@ -4862,6 +4890,11 @@ export function extractSortsObject(
     // replace + at the beginning if present
     else sort.fk_column_id = aliasColObjMap[s.replace(/^\+/, '')]?.id;
 
+    if (throwErrorIfInvalid && !sort.fk_column_id)
+      NcError.unprocessableEntity(
+        `Invalid column '${s.replace(/^[+-]/, '')}' in sort`,
+      );
+
     return new Sort(sort);
   });
 }
@@ -4869,6 +4902,7 @@ export function extractSortsObject(
 export function extractFilterFromXwhere(
   str,
   aliasColObjMap: { [columnAlias: string]: Column },
+  throwErrorIfInvalid = false,
 ) {
   if (!str) {
     return [];
@@ -4890,7 +4924,11 @@ export function extractFilterFromXwhere(
       nestedArrayConditions = str.split(
         /(?=~(?:or(?:not)?|and(?:not)?|not)\()/,
       );
-    return extractCondition(nestedArrayConditions || [], aliasColObjMap);
+    return extractCondition(
+      nestedArrayConditions || [],
+      aliasColObjMap,
+      throwErrorIfInvalid,
+    );
   }
 
   // iterate until finding right closing
@@ -4918,7 +4956,11 @@ export function extractFilterFromXwhere(
   const lhsOfNestedQuery = str.substring(0, openIndex);
 
   nestedArrayConditions.push(
-    ...extractFilterFromXwhere(lhsOfNestedQuery, aliasColObjMap),
+    ...extractFilterFromXwhere(
+      lhsOfNestedQuery,
+      aliasColObjMap,
+      throwErrorIfInvalid,
+    ),
     // calling recursively for nested query
     new Filter({
       is_group: true,
@@ -4929,7 +4971,11 @@ export function extractFilterFromXwhere(
       ),
     }),
     // RHS of nested query(recursion)
-    ...extractFilterFromXwhere(str.substring(closingIndex + 2), aliasColObjMap),
+    ...extractFilterFromXwhere(
+      str.substring(closingIndex + 2),
+      aliasColObjMap,
+      throwErrorIfInvalid,
+    ),
   );
   return nestedArrayConditions;
 }
@@ -4956,7 +5002,11 @@ function validateFilterComparison(uidt: UITypes, op: any, sub_op?: any) {
   }
 }
 
-export function extractCondition(nestedArrayConditions, aliasColObjMap) {
+export function extractCondition(
+  nestedArrayConditions,
+  aliasColObjMap,
+  throwErrorIfInvalid,
+) {
   return nestedArrayConditions?.map((str) => {
     let [logicOp, alias, op, value] =
       str.match(/(?:~(and|or|not))?\((.*?),(\w+),(.*)\)/)?.slice(1) || [];
@@ -4983,6 +5033,8 @@ export function extractCondition(nestedArrayConditions, aliasColObjMap) {
       }
 
       validateFilterComparison(aliasColObjMap[alias].uidt, op, sub_op);
+    } else if (throwErrorIfInvalid) {
+      NcError.unprocessableEntity(`Column '${alias}' not found.`);
     }
 
     return new Filter({

packages/nocodb/src/db/conditionV2.ts

@@ -8,6 +8,7 @@ import type Column from '~/models/Column';
 import type LookupColumn from '~/models/LookupColumn';
 import type RollupColumn from '~/models/RollupColumn';
 import type FormulaColumn from '~/models/FormulaColumn';
+import { NcError } from '~/helpers/catchError';
 import formulaQueryBuilderv2 from '~/db/formulav2/formulaQueryBuilderv2';
 import genRollupSelectv2 from '~/db/genRollupSelectv2';
 import { sanitize } from '~/helpers/sqlSanitize';
@@ -21,13 +22,21 @@ export default async function conditionV2(
   conditionObj: Filter | Filter[],
   qb: Knex.QueryBuilder,
   alias?: string,
+  throwErrorIfInvalid = false,
 ) {
   if (!conditionObj || typeof conditionObj !== 'object') {
     return;
   }
-  (await parseConditionV2(baseModelSqlv2, conditionObj, { count: 0 }, alias))(
+  (
-    qb,
+    await parseConditionV2(
-  );
+      baseModelSqlv2,
+      conditionObj,
+      { count: 0 },
+      alias,
+      undefined,
+      throwErrorIfInvalid,
+    )
+  )(qb);
 }
 
 function getLogicalOpMethod(filter: Filter) {
@@ -49,6 +58,7 @@ const parseConditionV2 = async (
   aliasCount = { count: 0 },
   alias?,
   customWhereClause?,
+  throwErrorIfInvalid = false,
 ) => {
   const knex = baseModelSqlv2.dbDriver;
 
@@ -60,7 +70,14 @@ const parseConditionV2 = async (
   if (Array.isArray(_filter)) {
     const qbs = await Promise.all(
       _filter.map((child) =>
-        parseConditionV2(baseModelSqlv2, child, aliasCount, alias),
+        parseConditionV2(
+          baseModelSqlv2,
+          child,
+          aliasCount,
+          alias,
+          undefined,
+          throwErrorIfInvalid,
+        ),
       ),
     );
 
@@ -76,7 +93,14 @@ const parseConditionV2 = async (
 
     const qbs = await Promise.all(
       (children || []).map((child) =>
-        parseConditionV2(baseModelSqlv2, child, aliasCount),
+        parseConditionV2(
+          baseModelSqlv2,
+          child,
+          aliasCount,
+          undefined,
+          undefined,
+          throwErrorIfInvalid,
+        ),
       ),
     );
 
@@ -89,7 +113,14 @@ const parseConditionV2 = async (
     };
   } else {
     const column = await filter.getColumn();
-    if (!column) return () => {};
+    if (!column) {
+      if (throwErrorIfInvalid) {
+        NcError.unprocessableEntity(
+          `Invalid column id '${filter.fk_column_id}' in filter`,
+        );
+      }
+      return;
+    }
     if (column.uidt === UITypes.LinkToAnotherRecord) {
       const colOptions =
         (await column.getColOptions()) as LinkToAnotherRecordColumn;
@@ -153,6 +184,9 @@ const parseConditionV2 = async (
               fk_column_id: childModel?.displayValue?.id,
             }),
             aliasCount,
+            undefined,
+            undefined,
+            throwErrorIfInvalid,
           )
         )(selectQb);
 
@@ -216,6 +250,9 @@ const parseConditionV2 = async (
               fk_column_id: parentModel?.displayValue?.id,
             }),
             aliasCount,
+            undefined,
+            undefined,
+            throwErrorIfInvalid,
           )
         )(selectQb);
 
@@ -292,6 +329,9 @@ const parseConditionV2 = async (
               fk_column_id: parentModel?.displayValue?.id,
             }),
             aliasCount,
+            undefined,
+            undefined,
+            throwErrorIfInvalid,
           )
         )(selectQb);
 
@@ -310,6 +350,7 @@ const parseConditionV2 = async (
         filter,
         knex,
         aliasCount,
+        throwErrorIfInvalid,
       );
     } else if (
       [UITypes.Rollup, UITypes.Links].includes(column.uidt) &&
@@ -843,6 +884,7 @@ async function generateLookupCondition(
   filter: Filter,
   knex,
   aliasCount = { count: 0 },
+  throwErrorIfInvalid = false,
 ): Promise<any> {
   const colOptions = await col.getColOptions<LookupColumn>();
   const relationColumn = await colOptions.getRelationColumn();
@@ -880,6 +922,7 @@ async function generateLookupCondition(
         knex,
         alias,
         aliasCount,
+        throwErrorIfInvalid,
       );
 
       return (qbP: Knex.QueryBuilder) => {
@@ -906,6 +949,7 @@ async function generateLookupCondition(
         knex,
         alias,
         aliasCount,
+        throwErrorIfInvalid,
       );
 
       return (qbP: Knex.QueryBuilder) => {
@@ -943,6 +987,7 @@ async function generateLookupCondition(
         knex,
         childAlias,
         aliasCount,
+        throwErrorIfInvalid,
       );
 
       return (qbP: Knex.QueryBuilder) => {
@@ -962,6 +1007,7 @@ async function nestedConditionJoin(
   knex,
   alias: string,
   aliasCount: { count: number },
+  throwErrorIfInvalid = false,
 ) {
   if (
     lookupColumn.uidt === UITypes.Lookup ||
@@ -1044,6 +1090,7 @@ async function nestedConditionJoin(
         knex,
         relAlias,
         aliasCount,
+        throwErrorIfInvalid,
       );
     } else {
       switch (relationColOptions.type) {
@@ -1059,6 +1106,8 @@ async function nestedConditionJoin(
                 }),
                 aliasCount,
                 relAlias,
+                undefined,
+                throwErrorIfInvalid,
               )
             )(qb);
           }
@@ -1075,6 +1124,8 @@ async function nestedConditionJoin(
                 }),
                 aliasCount,
                 relAlias,
+                undefined,
+                throwErrorIfInvalid,
               )
             )(qb);
           }
@@ -1091,6 +1142,8 @@ async function nestedConditionJoin(
                 }),
                 aliasCount,
                 relAlias,
+                undefined,
+                throwErrorIfInvalid,
               )
             )(qb);
           }
@@ -1108,6 +1161,8 @@ async function nestedConditionJoin(
         }),
         aliasCount,
         alias,
+        undefined,
+        throwErrorIfInvalid,
       )
     )(qb);
   }

packages/nocodb/src/db/sortV2.ts

@@ -7,6 +7,7 @@ import type {
   LookupColumn,
   RollupColumn,
 } from '~/models';
+import { NcError } from '~/helpers/catchError';
 import formulaQueryBuilderv2 from '~/db/formulav2/formulaQueryBuilderv2';
 import genRollupSelectv2 from '~/db/genRollupSelectv2';
 import { sanitize } from '~/helpers/sqlSanitize';
@@ -17,6 +18,7 @@ export default async function sortV2(
   sortList: Sort[],
   qb: Knex.QueryBuilder,
   alias?: string,
+  throwErrorIfInvalid = false,
 ) {
   const knex = baseModelSqlv2.dbDriver;
 
@@ -32,7 +34,14 @@ export default async function sortV2(
       sort = new Sort(_sort);
     }
     const column = await sort.getColumn();
-    if (!column) continue;
+    if (!column) {
+      if (throwErrorIfInvalid) {
+        NcError.unprocessableEntity(
+          `Invalid column id '${sort.fk_column_id}' in sort`,
+        );
+      }
+      continue;
+    }
     const model = await column.getModel();
 
     const nulls = sort.direction === 'desc' ? 'LAST' : 'FIRST';

packages/nocodb/src/helpers/catchError.ts

@@ -436,23 +436,29 @@ export default function (
   };
 }
 
-export class BadRequest extends Error {}
+export class NcBaseError extends Error {
+  constructor(message: string) {
+    super(message);
+  }
+}
+
+export class BadRequest extends NcBaseError {}
 
-export class NotAllowed extends Error {}
+export class NotAllowed extends NcBaseError {}
 
-export class Unauthorized extends Error {}
+export class Unauthorized extends NcBaseError {}
 
-export class Forbidden extends Error {}
+export class Forbidden extends NcBaseError {}
 
-export class NotFound extends Error {}
+export class NotFound extends NcBaseError {}
 
-export class InternalServerError extends Error {}
+export class InternalServerError extends NcBaseError {}
 
-export class NotImplemented extends Error {}
+export class NotImplemented extends NcBaseError {}
 
-export class UnprocessableEntity extends Error {}
+export class UnprocessableEntity extends NcBaseError {}
 
-export class AjvError extends Error {
+export class AjvError extends NcBaseError {
   constructor(param: { message: string; errors: ErrorObject[] }) {
     super(param.message);
     this.errors = param.errors;

packages/nocodb/src/helpers/getAst.ts

@@ -5,6 +5,7 @@ import type {
   LookupColumn,
   Model,
 } from '~/models';
+import { NcError } from '~/helpers/catchError';
 import { GalleryView, KanbanView, View } from '~/models';
 
 const getAst = async ({
@@ -19,6 +20,7 @@ const getAst = async ({
     fieldsSet: new Set(),
   },
   getHiddenColumn = query?.['getHiddenColumn'],
+  throwErrorIfInvalidParams = false,
 }: {
   query?: RequestQuery;
   extractOnlyPrimaries?: boolean;
@@ -27,6 +29,7 @@ const getAst = async ({
   view?: View;
   dependencyFields?: DependantFields;
   getHiddenColumn?: boolean;
+  throwErrorIfInvalidParams?: boolean;
 }) => {
   // set default values of dependencyFields and nested
   dependencyFields.nested = dependencyFields.nested || {};
@@ -63,6 +66,15 @@ const getAst = async ({
   let fields = query?.fields || query?.f;
   if (fields && fields !== '*') {
     fields = Array.isArray(fields) ? fields : fields.split(',');
+    if (throwErrorIfInvalidParams) {
+      const colAliasMap = await model.getColAliasMapping();
+      const invalidFields = fields.filter((f) => !colAliasMap[f]);
+      if (invalidFields.length) {
+        NcError.unprocessableEntity(
+          `Following fields are invalid: ${invalidFields.join(', ')}`,
+        );
+      }
+    }
   } else {
     fields = null;
   }
@@ -99,6 +111,7 @@ const getAst = async ({
               nested: {},
               fieldsSet: new Set(),
             }),
+          throwErrorIfInvalidParams,
         });
 
         value = ast;
@@ -126,6 +139,7 @@ const getAst = async ({
               nested: {},
               fieldsSet: new Set(),
             }),
+          throwErrorIfInvalidParams,
         })
       ).ast;
     }

packages/nocodb/src/services/data-table.service.ts

@@ -25,6 +25,7 @@ export class DataTableService {
       model,
       view,
       query: param.query,
+      throwErrorIfInvalidParams: true,
     });
   }
 
@@ -45,7 +46,9 @@ export class DataTableService {
       dbDriver: await NcConnectionMgrv2.get(source),
     });
 
-    const row = await baseModel.readByPk(param.rowId, false, param.query);
+    const row = await baseModel.readByPk(param.rowId, false, param.query, {
+      throwErrorIfInvalidParams: true,
+    });
 
     if (!row) {
       NcError.notFound('Row not found');
@@ -167,7 +170,7 @@ export class DataTableService {
       countArgs.filterArr = JSON.parse(countArgs.filterArrJson);
     } catch (e) {}
 
-    const count: number = await baseModel.count(countArgs);
+    const count: number = await baseModel.count(countArgs, false, true);
 
     return { count };
   }

packages/nocodb/src/services/datas.service.ts

@@ -7,7 +7,7 @@ import type { BaseModelSqlv2 } from '~/db/BaseModelSqlv2';
 import type { PathParams } from '~/modules/datas/helpers';
 import { getDbRows, getViewAndModelByAliasOrId } from '~/modules/datas/helpers';
 import { Base, Column, Model, Source, View } from '~/models';
-import { NcError } from '~/helpers/catchError';
+import { NcBaseError, NcError } from '~/helpers/catchError';
 import getAst from '~/helpers/getAst';
 import { PagedResponseImpl } from '~/helpers/PagedResponse';
 import NcConnectionMgrv2 from '~/utils/common/NcConnectionMgrv2';
@@ -133,6 +133,7 @@ export class DatasService {
     view?: View;
     query: any;
     baseModel?: BaseModelSqlv2;
+    throwErrorIfInvalidParams?: boolean;
   }) {
     const { model, view, query = {} } = param;
 
@@ -146,7 +147,12 @@ export class DatasService {
         dbDriver: await NcConnectionMgrv2.get(source),
       }));
 
-    const { ast, dependencyFields } = await getAst({ model, query, view });
+    const { ast, dependencyFields } = await getAst({
+      model,
+      query,
+      view,
+      throwErrorIfInvalidParams: param.throwErrorIfInvalidParams,
+    });
 
     const listArgs: any = dependencyFields;
     try {
@@ -163,11 +169,17 @@ export class DatasService {
         try {
           data = await nocoExecute(
             ast,
-            await baseModel.list(listArgs),
+            await baseModel.list(
+              listArgs,
+              false,
+              false,
+              param.throwErrorIfInvalidParams,
+            ),
             {},
             listArgs,
           );
         } catch (e) {
+          if (e instanceof NcBaseError) throw e;
           this.logger.log(e);
           NcError.internalServerError(
             'Please check server log for more details',

packages/nocodb/tests/unit/rest/tests/newDataApis.test.ts

@@ -865,16 +865,19 @@ function textBased() {
       query: {
         sort: 'abc',
       },
+      status: 422,
     });
     await ncAxiosGet({
       query: {
         where: 'abc',
       },
+      status: 422,
     });
     await ncAxiosGet({
       query: {
         fields: 'abc',
       },
+      status: 422,
     });
   });
 
@@ -2109,7 +2112,23 @@ function linkBased() {
         linkId: getColumnId(columnsActor, 'Films'),
         rowId: 1,
       },
-      body: [{Id:1}, {Id:3}, {Id:5}, {Id:7}, {Id:9}, {Id:11}, {Id:13}, {Id:15}, {Id:17}, {Id:19}, {Id:21}, {Id:23}, {Id:25}, {Id:27}, {Id:29}],
+      body: [
+        { Id: 1 },
+        { Id: 3 },
+        { Id: 5 },
+        { Id: 7 },
+        { Id: 9 },
+        { Id: 11 },
+        { Id: 13 },
+        { Id: 15 },
+        { Id: 17 },
+        { Id: 19 },
+        { Id: 21 },
+        { Id: 23 },
+        { Id: 25 },
+        { Id: 27 },
+        { Id: 29 },
+      ],
     });
 
     // verify in Actor table
@@ -2159,7 +2178,7 @@ function linkBased() {
         linkId: getColumnId(columnsCountry, 'Cities'),
         rowId: 1,
       },
-      body: [{Id:1}, {Id:2}, {Id:3}],
+      body: [{ Id: 1 }, { Id: 2 }, { Id: 3 }],
     });
 
     // update the link
@@ -2169,7 +2188,7 @@ function linkBased() {
         linkId: getColumnId(columnsCountry, 'Cities'),
         rowId: 2,
       },
-      body: [{Id:2}, {Id:3}],
+      body: [{ Id: 2 }, { Id: 3 }],
     });
 
     // verify record 1