String interpolation in 2.0 is a breaking change when injecting parameters in invalid places

[jelledruyts]

In EF Core 1.x the following worked as expected:

var databaseName = "Customer";
this.Database.ExecuteSqlCommand($"ALTER DATABASE [{databaseName}] SET CHANGE_TRACKING = ON (CHANGE_RETENTION = 2 DAYS, AUTO_CLEANUP = ON)");

In EF Core 2.0 the new string interpolation feature extracts the "databaseName" and injects it as a parameter, but SQL Server doesn't allow the database name to be parameterized so it throws an exception:

Exception message: User does not have permission to alter database '@p0', the database does not exist, or the database is not in a state that allows access checks.
ALTER DATABASE statement failed.
Stack trace:
System.Data.SqlClient.SqlException occurred
  HResult=0x80131904
  Message=User does not have permission to alter database '@p0', the database does not exist, or the database is not in a state that allows access checks.
ALTER DATABASE statement failed.
  Source=Core .Net SqlClient Data Provider
  StackTrace:
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString)
   at System.Data.SqlClient.SqlCommand.RunExecuteReaderTds(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, Boolean async, Int32 timeout, Task& task, Boolean asyncWrite, SqlDataReader ds)
   at System.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean asyncWrite, String method)
   at System.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean asyncWrite, String methodName)
   at System.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Execute(IRelationalConnection connection, DbCommandMethod executeMethod, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.ExecuteSqlCommand(DatabaseFacade databaseFacade, RawSqlString sql, IEnumerable`1 parameters)
   at Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.ExecuteSqlCommand(DatabaseFacade databaseFacade, FormattableString sql)
   at Contoso.Web.Services.SqlDatabaseEventRepository.CustomerDataContext.Initialize() in C:\Code\Contoso.Web\Services\SqlDatabaseCustomerRepository\CustomerDataContext.cs:line 33
   at Contoso.Web.Services.SqlDatabaseEventRepository.SqlDatabaseCustomerRepository.Initialize() in C:\Code\Contoso.Web\Services\SqlDatabaseCustomerRepository\SqlDatabaseCustomerRepository.cs:line 25
   at Contoso.Web.ApplicationInitializer..ctor() in C:\Code\Contoso.Web\ApplicationInitializer.cs:line 13

Further technical details

EF Core version: Microsoft.EntityFrameworkCore.SqlServer version 2.0.0
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 version 1703
IDE: Visual Studio 2017.3

[jelledruyts]

As a workaround, you can explicitly cast the interpolation to "string" (so the FormattableString overload isn't used and the parameters aren't extracted):

this.Database.ExecuteSqlCommand((string)$"ALTER DATABASE [{databaseName}] SET CHANGE_TRACKING = ON (CHANGE_RETENTION = 2 DAYS, AUTO_CLEANUP = ON)");

[ajcvickers]

@jelledruyts When we updated EF Core to use string interpolation to do parameterization we recognized this as a breaking change for code that was already using it to do things other than parameterization. The guidance going forward is to not let EF do it's parameterization in this case, possibly by casting to string like you show. However, be careful that this does not open your code to SQL injection attacks by making sure that all data is correctly escaped, especially when coming from user input.

[NickCraver]

@ajcvickers Is there any chance we can get this feature removed in a future version? It's such a dangerous feature that makes the user think they're protected when in many cases they are not. For example, if they have var sql = $"...."; before the call, escaping will not occur.

[smitpatel]

@NickCraver - It worked like that even before we added overload in 2.0.
If user is doing var sql = $"..." generate SQL before passing it to EF, isn't that put responsibility of escaping on user?

[NickCraver]

@smitpatel It's not all all obvious to a user that this escapes:

this.Database.ExecuteSqlCommand($"....{..}...");

but this does not:

var sql = $"....{..}...";
this.Database.ExecuteSqlCommand(sql);

There's a lot of information telling people that this is safe now, but the actual overload and resolution rules make it very brittle and there are even a lot of refactorings (e.g. do this replacement once) that unwittingly break it, even introducing SQL injection rather silently. It's much better, IMO, to tell users to explicitly parameterize.

We've very explicitly stayed away from doing this with Dapper because the messaging is so dangerous, and we see a lot more raw SQL than any other library. I posted a repo (that you can run) showing just some of the injection cases and some of the many ways users can easily screw this up: https://github.com/NickCraver/EFCoreInjectionSample/blob/master/Program.cs

[jnm2]

If you provide only a FormattableString method and not a string overload, problem solved, right?

The method that takes a raw string could have the suffix Raw or Unsafe.