Bug: Sort Columns are no longer being verified

[Grauenwolf]

Bug Description

RepoDB 1.12.9 with RepoDb.SqlServer 1.1.4 will throw MissingFieldsException if a sort column doesn't exist.

RepoDB 1.12.10 with RepoDb.SqlServer 1.1.5 will pass the invalid sort column to the database.

If the sort column is user-supplied, this may result in a SQL injection vulnerability.

Expected Exception Message:

RepoDb.Exceptions.MissingFieldsException
  HResult=0x80131500
  Message=The order fields 'ProductName' are not present at the given fields 'CellPhone, EmployeeClassificationKey, EmployeeKey, FirstName, LastName, MiddleName, OfficePhone, Title'.
  Source=RepoDb
  StackTrace:
   at RepoDb.StatementBuilders.BaseStatementBuilder.CreateQuery(QueryBuilder queryBuilder, String tableName, IEnumerable`1 fields, QueryGroup where, IEnumerable`1 orderBy, Nullable`1 top, String hints)
   at RepoDb.CommandTextCache.GetQueryTextInternal(QueryRequest request, IEnumerable`1 fields)
   at RepoDb.CommandTextCache.GetQueryText(QueryRequest request)
   at RepoDb.DbConnectionExtension.QueryInternalBase[TEntity](IDbConnection connection, String tableName, QueryGroup where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, Nullable`1 cacheItemExpiration, Nullable`1 commandTimeout, IDbTransaction transaction, ICache cache, ITrace trace, IStatementBuilder statementBuilder)
   at RepoDb.DbConnectionExtension.QueryInternal[TEntity](IDbConnection connection, String tableName, QueryGroup where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, Nullable`1 cacheItemExpiration, Nullable`1 commandTimeout, IDbTransaction transaction, ICache cache, ITrace trace, IStatementBuilder statementBuilder)
   at RepoDb.DbConnectionExtension.Query[TEntity](IDbConnection connection, Expression`1 where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, Nullable`1 cacheItemExpiration, Nullable`1 commandTimeout, IDbTransaction transaction, ICache cache, ITrace trace, IStatementBuilder statementBuilder)
   at RepoDb.DbRepository`1.Query[TEntity](Expression`1 where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, IDbTransaction transaction)
   at RepoDb.BaseRepository`2.Query(Expression`1 where, IEnumerable`1 fields, IEnumerable`1 orderBy, Nullable`1 top, String hints, String cacheKey, IDbTransaction transaction)
   at Recipes.RepoDB.DynamicSorting.DynamicSortingScenario.SortBy(String lastName, String sortByColumn, Boolean isDescending) in C:\Source\TortugaResearch\DotNet-ORM-Cookbook\ORM Cookbook\Recipes.RepoDb\DynamicSorting\DynamicSortingScenario.cs:line 30

Actual Exception Message:

Microsoft.Data.SqlClient.SqlException (0x80131904): Invalid column name 'AND 1 = 1'.

Depending on the exact sort column, no exception may be returned.

Schema and Model:

Please share to us the schema of the table (not actual) that could help us replicate the issue if necessary.

And also the model that corresponds the schema.

    public class DynamicSortingScenario : BaseRepository<EmployeeSimple, SqlConnection>,
        IDynamicSortingScenario<EmployeeSimple>
    {
        public DynamicSortingScenario(string connectionString)
            : base(connectionString, ConnectionPersistency.Instance)
        { }

        public void InsertBatch(IList<EmployeeSimple> employees)
        {
            if (employees == null || employees.Count == 0)
                throw new ArgumentException($"{nameof(employees)} is null or empty.", nameof(employees));

            InsertAll(employees);
        }

        public IList<EmployeeSimple> SortBy(string lastName, string sortByColumn, bool isDescending)
        {
            var sort = new[] { new OrderField(sortByColumn, isDescending ? Order.Descending : Order.Ascending) };
            return Query(x => x.LastName == lastName, orderBy: sort).AsList();
        }

        public IList<EmployeeSimple> SortBy(string lastName, string sortByColumnA, bool isDescendingA, string sortByColumnB, bool isDescendingB)
        {
            var sort = new[] {
                new OrderField(sortByColumnA, isDescendingA ? Order.Descending : Order.Ascending),
                new OrderField(sortByColumnB, isDescendingB ? Order.Descending : Order.Ascending)
            };
            return Query(x => x.LastName == lastName, orderBy: sort).AsList();
        }
    }

Library Version:

RepoDB 1.12.10 with RepoDb.SqlServer 1.1.5

[mikependon]

We have made a fix for an almost identical scenario via this request (#963) and that affects this use-case. The main reason of change was because of RepoDB's poor column projection in the older versions.

Let us say you have the table schema below.

CREATE TABLE [Person]
(
  Id INT PRIMARY IDENTITY(1,1)
  , Name NVARCHAR(64)
  , DateOfBirth DATETIME2(5)
  , Created DATETIME2(5)
)
ON [Primary];

And you do the query below.

var result = connection.Query("Person", fields: Field.From("Id", "Name"), orderBy: new OrderField("Created", Order.Ascending));

In the previous version of the library, it will throw the MissingFieldsException exception above, but the query above seems to valid in general as the Created column is a part of the underlying table.

This is a bug as we limit the available orderable columns based from the target selected columns and not from the actual table columns during the projection. To rectify this, you have to include the Created column as part of the selection (like below).

var result = connection.Query("Person", fields: Field.From("Id", "Name", "Created"), orderBy: new OrderField("Created", Order.Ascending));

We can (of-course) re-apply the same level of validation in the library by simply checking the orderable columns from the list of columns from the actual table, but we have decided not to proceed with it as we prefer SQL Server to throw this raw exception back to the caller.

Is there something we can adjust on the test cases on the cookbook itself?

[Grauenwolf]

but we have decided not to proceed with it as we prefer SQL Server to throw this raw exception back to the caller.

The danger in that is it's a SQL injection vulnerability if the end user can choose the order by column.

To avoid this risk, either the ORM or the application needs to check the column list.

To rectify this, you have to include the Created column as part of the selection (like below).

If that will move the error outside of SQL Server, then it will pass the test.

Otherwise we need to check it manually like we do in the ADO.NET implementation.

[mikependon]

If that will move the error outside of SQL Server, then it will pass the test.

We can do revert this behavior again with more strict validation against the actual table columns. Thanks for filing this incident