An ASP.Net Code Review : 10 Years of .Net Compressed into Weeks #2

This post is part of a blog series ASP.Net 10 Years On. Even though this is part of a series I have tried to make each post standalone.

In the first post from this series we looked at my first ever project. We want to bring this code up to date and in to line with the new requirements established in the post. Before we do anything, we need to review what we currently have, decide what's good, what's bad and what we can keep.

Login Page

Let's start with the login page of the project as it doesn't have much logic and will be a good starting point. Here is the login page from the project:

The code:

Good Points

#1 There is certainly no way one could accuse this code of being over complicated. It's in its most basic form (sometimes this is a good thing!).

#2 We can see that the password is not being stored in plain text and is being hashed. Note the line:

FormsAuthentication.HashPasswordForStoringInConfigFile(txtPassword.Text, "MD5")

#3 A consistent error message is displayed when the login is incorrect thus not allowing users to gleam any information e.g. having a different error when the username is correct but the password is wrong vs. an invalid username.

#4 Considering I had no idea what I was doing, I was pleased to see parameterised SQL:

SELECT [admin].* FROM [admin] WHERE (([admin].[username] = @username) AND ([admin].[password] = @password))

#5 The variable and user controls are reasonably well named.

Bad Points

Are you sitting comfortably? Let's begin...

#1 The first problem is that the code is mixed in the same file as the presentation markup. I partly blame the book I was learning from, as all the examples were in this format. The code should be in a code-behind file and even better any logic should be pulled out again into a business layer.

#2 Writing the specific data access code within each page such as this is a huge mistake. Not only is this same code going to be duplicated throughout the application, it's all building up to a huge task if I ever want to change database. In most cases once an application is based on SQL Server - that's very unlikely to change. However in this case we are using Access and we want to upgrade to SQL Server. In its current state that means we have to go through every page in the application and alter the code such as the below:

Dim conSMSQuiz As OleDbConnection
Dim strConString, strReader As String
Dim cmdSelect As OleDbCommand
Dim dtrResults As OleDbDataReader

Needs to be changed to*:

Dim conSMSQuiz As SqlConnection
Dim strConString, strReader As String
Dim cmdSelect As SqlCommand
Dim dtrResults As SqlDataReader

* There is more to be changed than this, but that's been excluded for brevity.

#3 We have a hashed password which is a good thing, but this login code doesn't do anything to make life difficult for a brute force hacker. There is nothing in place to prevent repeated brute force attempts to crack the password by repeatedly posting to the page.

#4 If a user authenticates the line conSMSQuiz.Close() is not closed and the database connection is left open. This is going to leak database connections.

#5 Table layout - 'nuff said. However, this was written in 2002!

#6 Inline CSS style, again 2002 syndrome.

Update

#7 Some of my eagle eyed readers also pointed out the use of SELECT * FROM rather then selecting the specific columns required.

In summary the biggest problem with this page is that it violates the Single Responsibility Principle. It's doing database connectivity, the business logic, the view logic and the presentation formatting all within one space. This problem is repeated throughout he entire application. 

Competition Details Page

This is the most complex page within the application. It displays the statistics for a given competition, selects the winner and allows the admin users to send an SMS to the winner confirming that they have won.

The code:

Good Points

#1 There has been some attempt on this page to break code out into methods. The method names even yield some clue as to what they do. E.g. countAll(), countCorrectAnswers().

#2 There are some comments within the code.

#3 The SQL connections are all closed when they are finished with.

Bad Points

It should be apparent that this page has the same problems previously covered in the login page code review with regard to data access code and duplication. There are further issues with this page and one or two WTF experiences.

#1 For a UI, there is far too much code. There is no sign of any abstract data types to encapsulate this logic. It's all dumped in the UI. Horrible.

#2 The following line of code that adds a client side confirm dialogue to the "Pick a Winner" button should not be set here. It should be set within the client side JavaScript:

btnWinner.Attributes.Add("onclick", "return confirm('Are you sure you wish to pick a winner and close this competition?')")

#3 Starting with the BindDataGrid() method, the first issue that jumps out is that cmdSelectCompetitions.ExecuteReader() is called twice within the same method. This means two hits to the database for the same data. Next we encounter our first WTF moment: use of session state? I'm assuming it is due to the fact that I clearly didn't understand the concept of variable scoping properly when this code was written. There are moments when we would all like a code review time machine so that we can revisit ourselves and slam the keyboard against our finger tips: this is one of those moments.

#4 The countAll() and countCorrectAnswers() are straight forward and easy to understand but again are performing repeated database calls, albeit with slightly different parameters for the same logical chunk of data: the chunk of data being all entries for the competition we are viewing.

#5 The methods countA(), countB(), countC() and countD() are also making the same mistake of excessing database calls. These methods are practically identical also thus these methods are duplicate code.

#6 The code for the click event of the button btnWinner is also a little distressing. The horrible nested IF statement makes the method difficult to read and bloated. Again horrible use of session state. The code to pick a winner at random is not guarded once a winner has been picked. If the user were to refresh the browser then a different winner would be picked: I'm pretty sure that would be illegal and is most definitely unethical!

#7 The code to logout is duplicated across every page in the application. The main HTML header/footer is also duplicated for every page. This was pre-master pages.

FormsAuthentication.SignOut()
Session.Abandon()
Response.Redirect("default.aspx")

I could pick out more issues relating to type casting and lack of finally around the database connections to ensure they always get closed, but I think we have identified enough big problems to keep us busy for a while.

Enter Competition API

The final segment of code we will highlight is the code that accepts incoming SMS messages for the competition entries. A 3rd party provides the SMS integration and sends the values as an HTTP Form Post to a page configured within our application. Here is the code:

Bad Points

#1 We won't repeat ourselves but the same problem with regard to data access code has been duplicated again.

#2 We are assuming that the values sent to us via the 3rd party will always be valid. We should never assume this and thus there should be some validation here that at a minimum would ensure that the require fields have values before being saved to the database.

#3 Considering that the owners of this application make their money from the SMS messages being sent to the application there is a serious security hole here. Can you spot it?

Let's use a HTTP tool such as Fiddler and create the following HTTP Post:

The post was executed and the page didn't error. If we take a look at the database we can see that my hack has been successful.

This means the software is vulnerable to users entering a competition without paying. This is a major problem. We should be validating that the sender of the request is who we expect it to be.

Conclusion

There is little we can keep of this application due to the poor quality and security vulnerabilities. It's a common idiom that re-writing your software is the biggest mistake you will ever make, however in this instance keeping this code would be a bigger mistake.

In the next post we will measure the performance of the application. See the RSS subscription links at the bottom of the page to follow this series.