4
\$\begingroup\$

I've been practicing C# and ASP.NET, and have been working on a somewhat larger-scale project than I am currently used to. I am sure there are horrible things wrong with this code that I will find out later, and would rather get some input on it now.

One of the things I'm concerned with is the fact that RunSelectCommand can return false if the database fails to connect, or if the query returns nothing.

Currently, in order to check for that, I do if (!(data is bool)) and if it's not I assume there is data to work with, and I return an error from the web server if not.

If any more information is needed, please let me know! I am very excited for some real peer-reviewed code.

public object RunSelectCommand(MySqlCommand command, List<MySqlParameter> parameterCollection = null)
{
    using (MySqlConnection conn = new MySqlConnection(connectionString))
    using (var cmd = conn.CreateCommand())
    {
        try
        {
            conn.Open();
        }
        catch (MySqlException ex)
        {
            return ex;
        }
        cmd.CommandText = command.CommandText;
        if (parameterCollection != null) // if parameters for query are specified, add them here.
        {
            cmd.Parameters.AddRange(parameterCollection.ToArray());
        }
        using (var reader = cmd.ExecuteReader())
        {
            if (reader.Read())
            {
                Dictionary<string, string> data = new Dictionary<string, string>(); // This is designed for returning only 1 row at a time
                for (int i = 0; i < (reader.FieldCount - 1); i++) // TODO seperate SelectAll command
                {
                    data.Add(reader.GetName(i), reader[i].ToString()); // Iterates through reader and adds data to Dictionary for output to JSON
                }
                conn.Close();
                return data;
            }
            else
            {
                conn.Close();
                return false;
            }
        }
    }
}

Usage example:

[HttpGet("checkapi/{text}")] 
    public object CheckApi(string text) // returning object automatically converts to JSon
    {
        MySqlCommand C = new MySqlCommand("SELECT * FROM api WHERE apikey = @apikey");
        List<MySqlParameter> collection = new List<MySqlParameter>()
        {
            new MySqlParameter() { ParameterName = "@apikey", Value = text }
        };
        return mysqlWrapper.RunSelectCommand(C, collection);
    }
\$\endgroup\$
5
  • 1
    \$\begingroup\$ I think you might have posted the wrong code. In your example you use RunUpdateCommand but the first snippet is RunSelectCommand. \$\endgroup\$
    – t3chb0t
    Feb 10 '18 at 16:22
  • \$\begingroup\$ @t3chb0t Whoops! You're right. I updated the code, although it's essentially the same as the original code. \$\endgroup\$
    – Tom F
    Feb 10 '18 at 16:27
  • 1
    \$\begingroup\$ If you are not averse to using an external lib then I suggest StackExchange.Dapper \$\endgroup\$
    – Nkosi
    Feb 10 '18 at 18:00
  • 1
    \$\begingroup\$ Outside of that, returning object IMO is a poor design choice, even if yo are the one using the code. create a model to use as response that will have a bool property to indicate success and have a data property that will hold result. \$\endgroup\$
    – Nkosi
    Feb 10 '18 at 18:02
  • \$\begingroup\$ @Nkosi Will definitely check out that lib, and thank you for your comments. I agree that is a better implementation and will rework that. \$\endgroup\$
    – Tom F
    Feb 10 '18 at 18:11
1
\$\begingroup\$

As you suspected, this code is pretty bad. You are returning three different types of data here. Just, don't.

1)

try
{
    conn.Open();
}
catch (MySqlException ex)
{
    return ex;
}

Don't return the exception, re-throw it and catch it in the caller.

try
{
    conn.Open();
}
catch (MySqlException ex)
{
    throw;
}

In this block, you are catching the exception, then returning the exception to the user as a piece of data. A better way to do this would be to just not catch the exception and let the caller catch it:

try
{
    return mysqlWrapper.RunSelectCommand(C, collection);
}
catch (Exception ex)
{
    return ex;
}

However, now you are exposing your call stack to the user, which you don't want to do. This information could provide hints to someone trying to break into your system, and is not allowed under some security protocols (SOC 2, for example). Instead of returning the whole exception, build a new piece of data and return just the relevant bits (or if they just need one piece, then just return that piece).

2)

Don't return false. Either return an empty object (if the caller doesn't care if there were no results), or throw an exception. Alternately, you could use a ValueTuple and return (bool success, Dictionary<string, string> data) from the method. The first successful path would return (true, {data}) and the second would return (false, null).

3)

You might have a SQL Injection attack going on here. Check out what happens if the client sends a request with the text parameter "test' or 1 = 1 --". It looks like the resulting query might be SELECT * FROM api WHERE apikey = 'test' or 1 = 1 --', but I don't know this well enough to be sure.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your input! Can you expand a bit upon #1? Thank you for the suggestion on #2. I will definitely look into that. As for #3, to my knowledge parameterized queries handle query sanitation. \$\endgroup\$
    – Tom F
    Feb 13 '18 at 21:10
  • 1
    \$\begingroup\$ Just following up: Did you see my edit? Let me know if it makes more sense now. \$\endgroup\$
    – user34073
    Feb 26 '18 at 2:59
  • \$\begingroup\$ Yes, this makes a lot of sense. Thank you for the information! I was not aware that exposing an entire call stack to the user could be a security risk. That is very helpful information \$\endgroup\$
    – Tom F
    Feb 27 '18 at 2:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.