Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

By adding more and more features to my websites, I am now up to 7 different hard coded queries.

I have read a lot about SQL injections and possible flaws and security issues, and wanted to make sure that it was secure enough.

This is one of my regular UPDATE queries :

Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped Source""")
    Using thisCommand As OleDbCommand = thisConnection.CreateCommand
        ' Open connection object
        thisConnection.Open()

        ' Initialize SQL UPDATE command to update the desired data
        With thisCommand
            .CommandText = "UPDATE Login SET ClientName = @ClientName, Fonction = @Fonction, CompanyName = @CompanyName, Address = @Address, Country = @Country, " & _
                                "[Phone Number] = @Phone, [Fax Number] = @Fax, [I prefer to be contacted] = @Contacted " & _
                                "WHERE Username = @Username"
            With .Parameters
                .AddWithValue("@ClientName", VB_Name)
                .AddWithValue("@Fonction", VB_Fonction)
                .AddWithValue("@CompanyName", VB_Company)
                .AddWithValue("@Address", VB_Address)
                .AddWithValue("@Country", VB_Country)
                .AddWithValue("@Phone", VB_Phone)
                .AddWithValue("@Fax", VB_Fax)
                .AddWithValue("@Contacted", VB_Preference)
                .AddWithValue("@Username", LBL_Welcome.Text)
            End With
        End With

        thisCommand.ExecuteNonQuery()

        thisConnection.Close()
    End Using
End Using

And this is one of my non-regular double SELECT queries (Lengthy) :

Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped source""")
    Using thisCommand As OleDbCommand = thisConnection.CreateCommand

        thisConnection.Open()

        'This query is to get the information
        thisCommand.CommandText = "SELECT [Minimum Length] " & _
                                    "FROM Stats " & _
                                    "WHERE  ([N° Cylinder] = @cylinder) AND ([N° Section] = 0)"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder

        Dim thisReader As OleDbDataReader = thisCommand.ExecuteReader()
        If (thisReader.Read()) Then
            cylinderLengthMin = thisReader.GetValue(0)
        End If
        thisReader.Close()

        'This query is to get the number of stages of the cylinder
        thisCommand.CommandText = "SELECT Stroke " & _
                                    "FROM (Stats) " & _
                                    "WHERE ([N° Cylinder] = @cylinder) " & _
                                    "ORDER BY [N° Section]"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder)

        thisReader = thisCommand.ExecuteReader()
        count = -1
        Dim condition As Boolean = True
        Dim temp As String
        'This loop gets the strokes of the stages
        While (thisReader.Read()) And condition
            temp = thisReader.GetValue(0).ToString()
            If thisReader.GetValue(0).ToString() = "" Then
                condition = False
            Else
                count += 1
            End If
        End While
        countMax = count 'This is used later for array formatting
        thisReader.Close()

        If countMax = -1 Then
            Throw New SQLException("There has been an error with the database query.")
        End If

        'This query is to gather the data. It is stored in Imperial in the DataBase.
        ReDim cylinderOutsideDiameter(countMax)
        ReDim cylinderInsideDiameter(countMax)
        ReDim cylinderSectionStroke(countMax)
        ReDim cylinderSectionRetracted(countMax)
        ReDim cylinderIDStopRing(countMax)

        thisCommand.CommandText = "SELECT Stroke, [Retracted Length], [Inside Diameter], [Outside Diameter],  [Non-contact Diameter]" & _
                                    "FROM (Stats) " & _
                                    "WHERE ([N° Cylinder] = @cylinder) " & _
                                    "ORDER BY [N° Section]"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder)

        thisReader = thisCommand.ExecuteReader()

        count = 0
        While (thisReader.Read()) AndAlso count <= countMax 'Scans each row and adds it to respective string/double array
            For i = 0 To 4
                If thisReader.GetValue(i).ToString() = "" Then
                    Throw New SQLException("There has been an error with the database query.")
                End If
            Next
            cylinderSectionStroke(count) = thisReader.GetValue(0)
            cylinderSectionRetracted(count) = thisReader.GetValue(1)
            cylinderInsideDiameter(count) = thisReader.GetValue(2)
            cylinderOutsideDiameter(count) = thisReader.GetValue(3)
            cylinderIDStopRing(count) = thisReader.GetValue(4)
            count += 1
        End While

        thisReader.Close()
        thisConnection.Close()
    End Using
End Using

Questions

  1. Are my SQL queries secure?

  2. Should I make a function for the queries (as I am up to 7 queries now and I should never repeat the same code multiple times). If yes, how? (I'm not asking for a code dump, just a descriptive explanation on how I could do it would be awesome)

  3. If there is anything I could improve in the code, don't hesitate. I would love to have feedback on how to improve it.

share|improve this question

3 Answers 3

up vote 8 down vote accepted

Make your code DRY (don't repeat yourself). Extract out your whole code of data retrieving to helper class

CommandBehavior.CloseConnection will avoid closing the connection so you need to explicitly close the connection. so use using keyword for VB. it will close the connection automatically

you are using parameterized query so it will make your SQL safer in case of SQL Injection.

Public Shared Function ExecuteReader(ByVal commandtext As String, ByVal parameters As Dictionary(Of String, Object)) As IDataReader
    Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped source""")
        Using thisCommand As OleDbCommand = thisConnection.CreateCommand
            thisConnection.Open()
            thisCommand.CommandText = commandtext
            For Each parameter In parameters
                thisCommand.Parameters.AddWithValue(parameter.Key, parameter.Value)
            Next
            'CommandBehavior.CloseConnection will avoid closing the connection so you need to explicity close the connection
            Return thisCommand.ExecuteReader(CommandBehavior.CloseConnection)
        End Using
    End Using
End Function

Public Function HowToUseExecuteReader()
    Dim parameters As New Dictionary(Of String, Integer)
    parameters.Add("@cylinder", cylinder)

    Dim query As String = "SELECT [Minimum Length] FROM Stats WHERE  ([N° Cylinder] = @cylinder) AND ([N° Section] = 0)"
    Dim dataReader As IDataReader = ExecuteReader(query, New Dictionary(Of String, Object))
    Using dataReader
         While dataReader.Read
             Dim cylinderLengthMin = dataReader.GetValue(0)
         End While
    End Using
End Function
share|improve this answer
    
Impressive! This will enable me to use a shared function. This will clarify my code and simplify it in the same time! Thanks a lot for that (I will use this type of functions and use Malachi's suggestion (use stored procedures)) –  Sifu Aug 14 at 15:12
    
Just a note, I don't think you want to have a function named the same way as a command (ExecuteReader). It could create confusion, in my opinion. I used what you suggested and modified for my personnal use, thanks a lot. –  Sifu Aug 14 at 19:22
    
Even though I accepted this answer, I wanted to mention that if I could, I would of marked all three answers as accepted. All the three answers provided different parts to fix or improve. Thanks to all of you. –  Sifu Aug 14 at 19:23

Is this secure? Well, nothing is ever really secure, but yes. Generally speaking it is. You're using parameterized queries, which is good. Malachi is right though, stored procedures would be even better. What you can do to make this a little more secure is specify the datatype and size of each parameter.

I'm not sure where this code resides, but you should make sure that it is separated from application logic in its own Data Access Layer. You open and close a lot of connections to your back end. That's a performance hit. Implementing a proper DAL should help mitigate that.

What does "3" mean in this piece of code? Nix the magic number and create a constant with a meaningful name.

        For i = 0 To 3
            If thisReader.GetValue(i).ToString() = "" Then
                Throw New SQLException("There has been an error with the database query.")
            End If
        Next

The elephant in the room...

Access is not a great back end for a website. It has notorious performance issues. Considering you're concerned about security as well, I would recommend moving to a SQL Server Express backend. You'll find the move relatively painless and have a faster, more secure database in the long run.

share|improve this answer
    
Thanks for the info, I will specify the datatype and size of each parameter as you suggested. || I'm not sure I understand what you mean by "where this code resides", these pieces of codes are triggered from the website (asp.net) where a client enters some information (does that help?). || The "3" was actually a typo, thanks for noticing it! It was supposed to be "4" (as in 4 columns). It really exists just to spot a mistake in the database. || (Follows in next comment, not enough characters) –  Sifu Aug 14 at 15:30
    
I understand your thoughts on performance issue, though I have never mentioned speed issues for a reason, the database is extremely small (3 tables of 10*40 cells). And Access was picked for a reason of ease of modifications (we can simply open the Access file, go into the desired table and change whichever data we want. All this without any SQL knowledge, which most of my collegues don't have) –  Sifu Aug 14 at 15:32
    
Sounds like (knowingly or not) you have implemented a DAL then. Good. I wasn't sure if this was client side code or not. –  RubberDuck Aug 14 at 15:33
1  
Oh! Yes, this is server side code. –  Sifu Aug 14 at 15:34
1  
Thanks a lot for your advices, they are well appreciated. I added these comments for two reasons : for any future readers to understand everything (if they want to read through it) and for you to see that your answer was useful. –  Sifu Aug 14 at 15:49

You should not hard code queries into your VB code if you can help it.

if you were using SQL Server I would say that you use Stored Procedures on the database instead of hard coded queries.

by passing parameters to a Stored Procedure from your code and having the database send you information back you are allowing the database to take care of a lot of the security of making sure that no SQL injection occurs, I am not saying that you shouldn't make sure that your input is not sanitized before you send it to your stored procedure.

separate the database from the presentation. let the database handle all the updating and retrieval of information, let your VB.NET application handle all the presentation of that data, and handle the input.


Access does have Stored Procedures.

Check out this Answer to: How do I make a stored procedure in MS Access?

share|improve this answer
2  
@Sifu, you don't want to risk it. you are exposing your database to the world, someone out there knows how to exploit the vulnerabilities of your code, so make sure that you don't have many for them to work with. Get into the habit of writing nearly impenetrable code –  Malachi Aug 14 at 14:08
1  
@Sifu: what if my user name was: Phrancis; DELETE FROM Login; ? –  Phrancis Aug 14 at 14:31
2  
it's not good structure, you want to keep your database code on your database, you will still parameterize the input when you use stored procedures as well. you want the database to handle the data, the VB to handle gathering input and displaying output. the database is the box, VB is the Hand that reaches into the box or puts things in the box. –  Malachi Aug 14 at 14:43
1  
@IanRingrose you should have a separation of concerns, Data and Presentation. that is accepted by the majority of programmers. one way or the other you shouldn't have SQL in your VB. what is the VB going to do with it? VB is going to send the SQL to the DB, why send more information than you need to. version control is something that SQL databases have figured out by back ups. not sure how that works on Access though. –  Malachi Aug 14 at 16:02
1  
Comments are not the place for tutorials, discussions and general chatter. Please take this to a chat room, you are welcome to join The 2nd Monitor at any time –  rolfl Aug 14 at 16:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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