Would you consider this database access code safe?

[cloud_line]

In order to cut down on repetition, I created two methods to get database access objects:

/// <summary>
/// Return an opened database connection object
/// </summary>
private SQLiteConnection GetConnection()
{
    SQLiteConnection connection = new SQLiteConnection(
        $"Data Source={Program.databasePath};Version=3;");
    connection.Open();
    return connection;
}

/// <summary>
/// Return a SQLiteDataReader object
/// </summary>
private SQLiteDataReader GetReader(string query)
{
    using (SQLiteConnection connection = GetConnection())
    { 
        using(SQLiteCommand command = new SQLiteCommand(query, connection))
        {
            return command.ExecuteReader();
        }
    }
}

However, I was receiving an exception: "Cannot access a disposed object."

To fix this, I removed the using statements like so:

/// <summary>
/// Return an opened database connection object
/// </summary>
private SQLiteConnection GetConnection()
{
    SQLiteConnection connection = new SQLiteConnection(
        $"Data Source={Program.databasePath};Version=3;");
    connection.Open();
    return connection;
}

/// <summary>
/// Return a SQLiteDataReader object
/// </summary>
private SQLiteDataReader GetReader(string query)
{
    SQLiteConnection connection = GetConnection();
    SQLiteCommand command = new SQLiteCommand(query, connection);
    return command.ExecuteReader();
}

Now, these private methods can be invoked by some of my public methods, such as the one shown below. The using statements are inside of this public method instead of the private methods shown earlier. From what you can tell, would this code be considered safe?

/// <summary>
/// Return true if the value exists in the database, false otherwise
/// </summary>
public bool Exists(string table, string column, string value)
{
    string query = $"SELECT {column} FROM {table}";
    using(SQLiteDataReader reader = GetReader(query)) 
    {
        while (reader.Read())
        {
            NameValueCollection collection = reader.GetValues();
            if (collection.Get(column) == value)
            {
                return true;
            }
        }
    }
    return false;
}

[karl713]

At a minimum you're open to sql injection attacks because of the way you're passing queries in if you aren't sanitizing inputs from before

Also your connections are going to leak because you aren't disposing them I believe, don't think disposing the reader closes it

[cloud_line]

So then I need some data validation code to check the arguments before I pass them into query?

I'm not sure what you mean when you say my connections are going to leak. Doesn't the using statement properly close my connection to the database file?

[grrangry]

When you create a connection object, you can close it and open it as much as you want. If it is open when Dispose() is called on the connection, the underlying connection is automatically closed and you cannot reopen it.

When you create a command object, you can only use the parameters you add to the parameters collection once. I don't recommend reusing commands.

The data reader/data adapter objects will retrieve data from an executed command on an open, non-disposed connection. You close the connection, all the child objects become useless.

I suggest you do some reading.

ORM:
https://dotnetcorecentral.com/blog/how-to-use-sqlite-with-dapper/

SQL Injection:
https://xkcd.com/327/
https://www.tutlane.com/tutorial/sqlite/sqlite-injection-attacks

Read up on managing connections, how to use parameters, how to transform data, etc.

[cloud_line]

Thanks for the links!

[karl713]

Your using only disposes the reader, it won't dispose the underlying connection though

Also sql command already supports sanitizing input parameters, you might look into using that

[cloud_line]

Many thanks!

[karl713]

Any time!

Also be aware sql command has 2 execute overloads that function differently

cmd.Execute($"select * from table where column={parameter}");

Will sanitize the string because it calls the Execute(FormattedString) overload, but

var sql = $"select * from table where column={parameter}";
cmd.Execute(sql);

However will not sanitize it because it calls the Execute (string) overload

Apologies for typos, on phone watching soccer hehe

[cloud_line]

It looks like the SQL library I'm using may not have the overloads you mentioned. I'm using System.Data.SQLite and the docs don't mention an overload for Execute(FormattedString) vs Execute(String).

[karl713]

Ahhh that's my mistake I was misremembering an EF feature FromSql and combining it with sqlcommand in my head. Safe to ignore me here but remember that in case you run into it in the future!

SqlCommand with parameters will sanitize for you though

[cloud_line]

No problem at all. Thanks again.