What will be the best practices in my code to prevent sql injection?

Question

What will be the best practices to prevent sql injection? My client asked me to prevent sql injection. I used this structure for data inserting or updating

public bool Add(GreenItem aGreenItem, Employee emp)
        {
            aGreenItem.GreenItemCode = new CommonBLL().GetMaxId("[GreenItemCode]", "[Processing].[GreenItem]", "GTM");
            using (SqlConnection objConnection = Connection.GetConnection())
            {
                SqlTransaction transaction = objConnection.BeginTransaction("SampleTransaction");
                try
                {
                    string query = aGreenItem.GreenItemId == 0 ? "GreenItem_Create" : "GreenItem_Update";
                    SqlCommand sqCmd = new SqlCommand(query, objConnection, transaction);
                    sqCmd.CommandType = CommandType.StoredProcedure;
                    if (aGreenItem.GreenItemId > 0)
                    {
                        sqCmd.Parameters.AddWithValue("@GreenItemId", aGreenItem.GreenItemId);
                    }
                    else
                    {
                        sqCmd.Parameters.AddWithValue("@GreenItemCode", aGreenItem.GreenItemCode);
                    }

                    sqCmd.Parameters.AddWithValue("@GreenItemName", aGreenItem.GreenItemName);
                    sqCmd.Parameters.AddWithValue("@MeasurementUnitId", aGreenItem.MeasurementUnitId);                    
                    sqCmd.Parameters.AddWithValue("@Description", aGreenItem.Description);
                    sqCmd.Parameters.AddWithValue("@IsActive", aGreenItem.IsActive);
                    sqCmd.Parameters.AddWithValue("@GLTId", emp.GLTId);
                    sqCmd.Parameters.AddWithValue("@CreatorId", emp.EmployeeId);
                    sqCmd.ExecuteNonQuery();
                    transaction.Commit();
                    return true;
                }
                catch
                {
                    transaction.Rollback();
                    return false;
                }
            }
        }

I used this function to get the Max ID Which is called from the function above

public string GetMaxId(string coloumName, string tableName, string prefix)
        {
            string maxId = ""; string selectQuery = "SELECT '" + prefix + "'+RIGHT('0000000000'+ CONVERT(VARCHAR,ISNULL(MAX(RIGHT(" + coloumName + ",10)), 0)+1,10),10) maxID FROM " + tableName + " ";
            using (SqlConnection objConnection = Connection.GetConnection())
            {
                SqlCommand sqCmd = new SqlCommand(selectQuery, objConnection); sqCmd.CommandType = CommandType.Text;
                using (IDataReader dataReader = sqCmd.ExecuteReader())
                {
                    while (dataReader.Read())
                    {
                        maxId = dataReader["maxID"].ToString();
                    }
                }
                objConnection.Close();
            }

            return maxId;
        }

What needs to be added to for best output?

Answer 1

With SQL Server, avoiding SQL injection comes down to one simple thing

• use parameters, rather than concatenation, for all inputs

You're already doing that in the code that we can see, so: great job so far.

People often mistakenly say "use stored procedures" to avoid SQL injection, but "stored procedures" and "SQL injection" are actually entirely orthogonal - you can avoid SQL injection without stored procedures, and you can cause SQL injection inside stored procedures. We can't see what GreenItem_Create / GreenItem_Update do internally - they're probably fine if they are simple INSERT / UPDATE operations. As long as they don't do EXEC (@somethingYouConcatenated) internally, you should be fine. If you do ever need to build T-SQL inside T-SQL, make sure to use sp_ExecuteSQL to correctly parameterize that dynamic SQL.