App_Code SQL Inject/Select

I need some guidance on the following if possible please

Explanation I have a main project.cs file in the App_Code which contains main functions. One of these functions is a SQL_Inject which inserts data into the database.

I then have multiple pages that utilize this function from multiple client machines at the same time.

Question The answer i am after is, is this a safe method of choice? Or should i be creating a new connection separately on each .cs page.

Reason/Problem Reason this is becoming a concern, we are currently a small company but growing. It has happened that a page crashes due to the SQL Connection is still open. I am worried its due to two connections trying to be made at the same time. I am not sure if this is the issue or if it comes from something else.

//GLOBAL DECLARATIONS

//DB CONNECTIONS - retrieve from config file

public static string ConProjectms = System.Configuration.ConfigurationManager.ConnectionStrings["conProject"].ConnectionString;

//DB CONNECT TO SQL
public static SqlConnection SqlConn = new SqlConnection();
public static SqlCommand SqlCmd = new SqlCommand();
public static SqlDataReader SqLdr;
public static string SqlStr;
public static string ConnString;


public static void SqlInject(string query, string dataBase)
{

    SqlConn.ConnectionString = ConProjectms;
    //Set the Connection String
    SqlConn.Open();
    //Open the connection 
    SqlCmd.Connection = SqlConn;
    //Sets the Connection to use with the SQL Command
    SqlCmd.CommandText = query;
    //Sets the SQL String
    SqlCmd.ExecuteNonQuery();
    //put Data 
    SqlClose();

}


public static void SqlClose()
{
    if (SqlConn.State != ConnectionState.Open) return;
    SqlConn.Close();
    SqlCmd.Parameters.Clear();
}

Solution

SQL can handle multiple connections at the same time. However, you're code is very likely to be be run by two clients at the same time, and they'll be using the same connection not two separate connections. That's bad thing #1.

SQL Server does a fantastic job of connection pooling - and I assume other DBs have similar capabilities. In such a world, you shouldn't try to keep and reuse any of your data-related objects around - but create them as you need them and when SQL sees that you're using a connection it's created before and since freed up, it'll use that. You don't have to do anything weird to get this functionality.

With that in mind, your static objects should mostly go away, and your SQLInject method might look something like this:

public static void SqlInject(string query, string dataBase)
{
   var connectionString = 
     System 
     .Configuration 
     .ConfigurationManager 
     .ConnectionStrings["conProject"] 
     .ConnectionString;

  using ( var connection = new SqlConnection( connectionString ) )
  {
    connection.Open( );
    using ( var command = connection.CreateCommand( ) )
    {
      command.CommandText = query;
      command.CommandType = CommandType.Text;
      command.ExecuteNonQuery( );
    }
  }
}

Notice that you don't have to worry about closing the connection per se; the using blocks handle the disposition of your open, active objects. This is largely how folks are doing direct SQL from c#. By the way, neither your code nor mine uses the dataBase argument. Maybe you're supposed to edit the base connection string with it??

But wait - there's more!

Having said all that, and since you raised a concern about security, you should know that this isn't safe code at all - yours or mine. SqlInject is probably a good name, because it allows pretty much anything in the query argument (which, BTW, if you're doing ExecuteNonQuery, then maybe query isn't a good name).

You're far far better allowing arguments to a library of known statements (maybe stored procedures), validating those arguments, and using SQL Injection attack mitigation to parameterize your known statements (look up that phrase and you'll find an abundance of examples and advice).

Just for yuks, here's a scaffold of what you might consider:

public static void SqlInject(string commandName, params[] object commandArgs )
{
   //--> no point in going on if we got no command...
   if ( string.IsNullOrEmpty( commandName ) )
     throw new ArgumentNullException( nameof( commandName ) );

   var connectionString = 
     System 
     .Configuration 
     .ConfigurationManager 
     .ConnectionStrings["conProject"] 
     .ConnectionString;

  using ( var connection = new SqlConnection( connectionString ) )
  {
    connection.Open( );
    using ( var command = connection.CreateCommand( ) )
    {
      command.CommandType = CommandType.Text;
      command.CommandText = "select commandText from dbo.StatementRepository where commandName = @commandName";
      command.Parameters.AddWithValue( "@commandName", commandName );
      var results = command.ExecuteScalar( );
      if ( results != null && results != DbNull.Value )
      {            
        //--> calling a separate method to validate args, that returns
        //--> an IDictionary<string,object> of parameter names
        //--> and possibly modified arguments.
        //--> Let this validation method throw exceptions.
        var validatedArgs = ValidateArgs( commandName, commandArgs );

        command.Parameters.Clear( );
        command.CommandText = query;
        foreach( var kvp in validatedArgs )
        {
          command.Parameters.AddWithValue( kvp.Key, kvp.Value );
        }
        command.ExecuteNonQuery( );
      }
      else
      {
        throw new InvalidOperationException( "Invalid command" );
      }          
    }
  }
}

I didn't attempt to write an actual argument validating method, because that's all wrapped up in your application logic...but I wanted to give you an idea of how you might get to a safer state.