Brendan Enrick

Daily Software Development

Hidden Inputs - Code Audits #4

ManBehindCurtainNo user could ever find your “hidden” inputs. You’re a master of stealth and the HTML hidden fields are totally safe and secure for whatever you want to put there. That’s why they’re called “hidden”. Obviously.

Hidden Inputs

In simple terms, these are HTML controls that sit on a page and have no display for users. They’re only “hidden” by not being in plain sight. It’s like hiding something behind a curtain. It makes it easily ignored, but anyone can decide they want to pay attention to that man behind the curtain.

When a user submits a form to your server, it will also post the values of the hidden fields. These are occasionally useful for developers, but are not a place for secure information. You might have an unimportant value you want to maintain.

Never put any information in a hidden input that shouldn’t be seen or modified by a user.

The Bad

One project that I had the pleasure of looking over for someone was a homebrew ecommerce application… Yes, this one is going to get scary.

When you buy something from a site, it will often show you some prices and a total for your order before you checkout. This application had a rather genius location to store the total price that you would be paying. It kept the total in a HiddenInput, so that when the user confirmed the order it would send that up to the server for processing.

I think this was a form of making sure that the same value shown to the user was what they paid. I can only assume that the developer was trying to avoid the values changing in the system and the customer paying a price different from what was shown on the preview.

The danger, however, is that if you want to pay $0.01 for anything on the site, you can just change the hidden input value to 0.01 and then submit the page. Sure it takes someone with some web knowledge to do that, but anymore that’s not a rare thing to know about.

The really cool part is that the system used that value to send to the credit card processor and actually recorded the full detailed list when it logged it. That means that the site logged everything correctly and when it processed the order with the card company it used the amount you specified in the hidden input. Yes, you could totally have ripped them off and their own sites records wouldn’t even show it!

If you’re thinking about trying this on that site, I am sorry to burst your bubble. I told them about that issue and they fixed it.

Less cool than the above example, I’ve seen AccountIds stored in hidden inputs as well as other values that shouldn’t be kept in those locations. I’ve yet to see passwords stored there, but I am sure it’s been done somewhere by someone.

The Better

Make sure to keep all information that requires any level of security or control on the server. Never ever put something like that on the page. Information that should be shown to a user in a read-only fashion should be treated as such. When sent to the client, it should be displayed only. Those values should be left with the client and only information from the server should be trusted.

If you need to store that information somewhere, put it in server memory in some kind of state or store it in the server’s permanent storage.

User sessions are great for storing user-specific information. In the case of the shopping cart, you could use some of that in-memory storage like session or you could store a temporary record in the database of that cart. I am not going to get in the specifics here, but make sure that you get it stored somewhere that users don’t have access to it.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Massive Classes - Code Audits #3

Today we’re going to take a look at something that I’m sure most of you have seen before.

We’re talking about classes that have grown so large that they’ve earned the size category “massive”. These classes can sometimes house an entire layer of an application. Who would create these monstrosities? Usually it’s a team of malicious, evil developers who are trying to ruin your day. The devised a clever way to make your development experience terrible; they create a massive class and make sure that you end up having to maintain/repair/modify the code.

The real question everyone should be asking is “how big?”

The Bad

I could write all day long about different instances of these that I’ve seen, but instead of boring you, I will only talk about one of the worst offenders I’ve ever seen.

One fantastic code base we inherited was built using a “great” technique for organizing the code. It’s simple approach state that any time you needed to do any data access you could just go to the class named, “MasterDAL”. What made this so great? Well, you always knew where the method you needed would be. What is wrong with that? Well, it was quite common to have 2 or 3 methods that did the same thing in that massive class.

Sometimes you had a few methods that all did the same thing, but each one did it slightly different. The really fun part is that some of them worked and some of them didn’t. It was your job to make sure that you used one that had been updated and had its bugs fixed.

How big was MasterDAL you ask? That file alone was over 50,000 lines long. It was not auto-generated. It was written by developers.

OK, so something I told you so far about this wasn’t true. The code wasn’t really this nice. It gets better. I lied about there being 1 place for everything. MasterDAL was the master data access layer. There were two other data access layers.

Luckily those ones had much more specific names and were not as general or as large as the MasterDal. MiscellaneousDAL and OtherDAL were each only about 20,000 lines long. Obviously the miscellaneous data access and all the other data access go in those. That’s how they’re different from “Master”.

Oh and did I mention that not only did it have repeated code internally, but it also had some methods repeated in the MasterDAL. I know you’re surprised now.

The Better

Please always make sure that you’re organizing in smaller classes. Go read about the Single Responsibility Principle. It will explain that we need to organize our code. We also need to not repeat ourselves quite so much.

There are so many great ways of dealing with data access. Go get yourself an ORM or just use Entity Framework. I don’t care how you do it, but use one of those. Go read about some designs for organizing your data access code. There are so many good options out there. There’s no reason to create these big messes.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Cookies - Code Audits #2

This is the second post in what I hope to make as a 30 post series of blogs about things I’ve seen in different code bases over my years as a software consultant. I will not be mentioning any client’s names or giving any easy way for people to identify the client whose code base has or did have any of these issues.

My team and I are often brought in on existing projects where the original development team failed to complete the code, created a big ball of mud that made bugs common and features slow to create, or simply where the previous team has completed the work and the customer just wants to know the quality of the code base.

Cookies

Cookies are a great way for developers to keep track of which client is communicating with them. They can store information in the browser. That sounds great doesn’t it?

The cookie gets an expiration date and is accessible from ASP.NET when the user visits your site. This gives you a chance to look at the cookie and get the information that has been stored in the cookie.

The cookie is able to expire when you want it to and will allow the user to close their browser, come back later, and still have the cookie being identified by your site.

These are most often used a means of identifying a user. When you log in to a site, you will get a cookie, which has a unique identifier allowing the site to identify you during that session.

What should be stored in a cookie?

  • Temporary GUIDs
  • Unimportant information
  • Tracking information that in no way identifies the user

Interesting things found in cookies

You can find all kinds of things getting stored in cookies. Now might be a good time to go looking through your own browser’s cookies to see what things sites have stored there.

Full Address Information

One site had an interesting way of making sure it always had access to your relevant information. It kept it all stored in a cookie. Yes, it had your name, address, phone number, email address, etc. stored in a cookie. I bet you’re really happy to know that the information is being stored in a cookie where pretty much any interested party can access it.

This means that anyone sniffing your traffic would see these values being passed when you navigate around the site. Also, it keeps everything nicely packaged up for consumption by spyware or other malware.

Username and Password Information

Ever wonder how a site lets you “Stay logged in for 30 days”? Well I hope the site you’re using doesn’t do it the same way as this one. One application used a cookie to keep your username and password… Where anyone can get it… and it wasn’t encrypted or anything… Clear text…

I think this one explains itself.

You account number and access rights

When you log in to a site, you get access to your stuff and not other people’s stuff. One site whose codebase I’ve seen kept track of this information by storing the account number in the cookie. (At least it wasn’t in the address bar.) Yes, that’s correct. If you just change the account in your cookie, you can see someone else’s account.

One might expect at least a GUID, so it would be hard to guess the other account number, but one would be wrong. These account numbers were integers starting at the number… 1. Each was incremented sequentially by one.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

URL Parameters - Code Audits #1

This is the first post in a series that I am writing that focuses on some interesting finds from different code bases I’ve seen over my years looking at lots of different software projects. Code Audits are one place where lots of interesting bits of code can be found, since you're combing through an entire software system. I will be explaining what I’ve seen, why it’s wrong, and usually the better approach if it’s not obvious.

URL Parameters

URLs let you control the navigation as your user navigates around your site. Users may not know what URLs mean or how they work, but most people browsing the web know that URLs exist. A good number of them know that modifying that URL will take them to different pages. Even my mother knows that much about URLs.

It’s not just a tech savvy computer programmer or hacker who can figure out that changing the URL might get them to a page they’re trying to see. This is sometimes useful on site’s whose navigation is somewhat lacking. It’s also often possible to guess the page you’re looking for by adding “/login” or some other common word to find a page whose link you’re not seeing.

The Bad

On web sites that have user accounts associated with multiple accounts, it’s quite common for the current account that the user is viewing to be specified in the address bar using a query string parameter. Normally, this means that there will be something along the lines of “?AccountId=123456” on the end of the URL. It’s also very common for those to be built in as “subfolders” like this “/Account/123456/”. Either way, it’s easily visible, modifiable information presented directly to the user.

As I am sure most of you can predict, I am going to point out that I’ve seen some code that didn’t do the proper protection to make sure that only an allowed user can access any given AccountId. The site will only display the links to the allowed accounts, but the site itself does not protect against a user changing that AccountId.

If you want to get to another user’s account, you can just type in another AccountId into the URL and the site will take you there…

The circumstances where these mostly come up is when 1 user account is allowed to have access to more than one account. It’s on these sites that the user will need to be able to change accounts, and sometimes users keep multiple accounts open at the same time, so the URL is needed to keep both open at the same time. It’s important, however, that we maintain at least some level of security here.

The Better

When we build sites, it’s extremely important that we validate inputs given by users to make sure that they’re valid. It’s also important to make sure that users have access to the information they’re requesting.

The first thing you do when you receive user input is to validate that input. Obviously if you consider a URL to be user input, you would validate that immediately as well. This means that either directly or through some layer of abstraction, your code should be verifying a user has access to any information being requested.

Citigroup

There was a big fiasco last year related to Citigroup. (http://www.nytimes.com/2011/06/14/technology/14security.html) The rumor is that they had a security breach based on this type of issue. I have no real, credible information about this, but it makes for a good example even if it’s only rumor.

From the New York Times article we get these:

Once inside, they leapfrogged between the accounts of different Citi customers by inserting vari-ous account numbers into a string of text located in the browser’s address bar.

That sure sounds to me like what I just described. If it is what I described, then this security expert stretched the truth quite a bit based on what is in that same article.

One security expert familiar with the investigation wondered how the hackers could have known to breach security by focusing on the vulnerability in the browser. “It would have been hard to prepare for this type of vulnerability,” he said. The security expert insisted on anonymity because the inquiry was at an early stage.

It’s not hard to prepare for that kind of vulnerability. I guess if you have it, it’s hard to track and log the issue. What’s easy is not having the issue to begin with. It’s a simple policy of making sure that all inputs are validated for permissions before being executed. Also, that is not a “vulnerability in the browser”.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Single Responsibility Principle

Now that we’re in the second half of June, I think it’s time that those of us with one, take a look our 2011 Software Craftsmanship Calendar. In June 2011, the topic was the Single Responsibility Principle, which states that your code should only have one reason to change.

The Single Responsibility Principle is the first in a set of Principles called SOLID,

SingleResponsibilityPrinciple

That looks dangerous…

For the rest of June, please try to follow Single Responsibility Principle and try to not be a Duct Tape Coder. Everyone who has to maintain your code in the future will thank you.

Duct Tape Coder

For anyone following along in the NimblePros Software Craftsmanship Anti-patterns Calendar, June is the month of the Duct Tape Coder. The calendar is a great reminder to try to avoid all of these bad practices. You obviously want to avoid all of the bad practices illustrated in the calendar, but for the month of June, you should be focusing on not being a Duct Tape Coder.

Duct Tape Coding

Similar to Cowboy Coding, Duct Tape Coding is all about getting things done. Not worrying about how you do it or how it will be maintained in the future. The goal of the duct tape coder is to get something working.

You might be saying to yourself, “I thought that all of these agile good practices said to start with the simplest thing that works!”

You’re correct. It says to start there. That means that you need to clean things up and use refactoring to create a maintainable, clean solution. Consider the initial duct tape to be scaffolding to allow you to build the final, maintainable solution. The duct tape, along with some automated tests allows you to build a system to be proud of.

DuctTapeCoder

Spend June making sure that you’re building your software the right way. Don’t settle for a “working” solution. Build a good solution.

The Clean Coder Review

I recently read The Clean Coder: A Code of Conduct for Professional Programmers (Robert C. Martin Series), and it is one of the best books on software development that I’ve ever read. The book is a quick read from Uncle Bob Martin and is comparable in size to his similarly named, Clean Code: A Handbook of Agile Software Craftsmanship.

The book goes through many, many examples from the author’s development experiences. It has sample discussions between parties to explain points and really goes over professionalism in software development.

The Title

The impression I have about why the book is called “The Clean Coder” is mostly a play on professional developers wanting to have clean code. This doesn’t just mean the code, the code is included of course, but it also means that the whole coding experience and the whole project should be clean.

There are a lot of mistakes that developers can make, and the book is covering them and discussing how to keep things clean.

The Material

Without jumping into too much of the meat of the book, I will say that there are many things in the book that I’ve done before and don’t do anymore. There were some things mentioned that I’ve done recently, and I am going to make sure I never do again. There are some things in there that I’ve thankfully never done, and I hope I don’t ever do them.

The book focuses on a lot of bad situations, why these situations are bad, and how we should really be handling things.

Consider this book to be a guidebook of suggestions for how to act as a professional. For example, in the book’s discussion of estimation, instead of focusing on how to be more accurate with estimation, it covers how you should be estimating. This means that it tells you that you should be very clear about things being estimates, not to imply a commitment, and not to make commitments if you cannot meet them.

Controversial Points

The Clean Coder makes some recommendations that I am sure that plenty of people will disagree with, and the way some points are worded cause me to sometimes disagree as well, but I don’t think it really takes away from the value of the book.

One example of this type of suggestion is that you work 40 hours a week and then another 20 each week on improving your career, leaving 56 hours for sleeping and 52 hours for everything else. It’s presented as doing this makes you a professional and not doing it is not professional. What I disagree with is the definite amount of time. I think it’s important for people to work on their careers, but 20 hours is arbitrary here. Someone may get the same value out of 10 hours that requires me 30 hours. There also is no definitive line between professionalism and unprofessionalism. There are some gray areas, and I believe this would be one of them.

Some things in the book were kept clear that it’s how he works, and I took these as suggestions to learn how I work and make sure that I am evaluating a possible issue.

In this case, I am referring to a point on music. Some people listen to music lightly while programming. This is even true when pair programming. I sometimes like having a very light bit of background buzz from music while pairing. In this case, I keep it so quiet that we can still communicate easily. The book seems to be against music, since it distracts and changes the author’s thinking. For me, I think I don’t have this issue because my pairing partner prevents a change in thinking. I also try to listen to music with foreign lyrics or no lyrics at all. It also helps to not distract from the task at hand. The value here for me is that I should be evaluating and thinking about how I am doing things to make sure that something as seemingly inconsequential as music is not detracting from my work.

Meetings

One great suggestion made by Uncle Bob is that it’s OK to decline a meeting invite. I think that’s very important to understand. Unless you’re really needed in the meeting, you should try not to attend. Meetings take up time, and if it’s less important than your other work, you shouldn’t be attending.

I had a meeting I scheduled yesterday. I invited 8 people to that meeting. I made sure it was clear that if you had something to bring up, you should attend the meeting, otherwise your attendance was not required. We had 4 people in the meeting, and I think 1 of the attendees need not have been there anyway.

Trying

You should read the section on trying. It’s important not to try, and this book makes it very clear why. If you’re going to try, you’re making a commitment to try.

Mentoring

The first paragraph on the chapter on mentoring illustrates one of the biggest shortfalls of the software development industry. I owe a lot of my successes as a developer to the mentoring that I’ve received through books, blogs, and any other recorded sources of information I can get my hands on. I was also lucky enough to have a mentor and a role model to learn from as I became a better developer.

Summary

If you go in to this book expecting it to be a recipe for success, you will be mistaken. This book is fantastic, but it’s important to think and consider why it’s valuable. It is one developer’s experience as a developer.

From his own stories, he is a developer who has hit some pretty rough patches and made some terrible mistakes. That’s a good thing! It means that he has learned a lot!

While reading the book, I never felt like it was forcing his way of doing things onto me. It is presented in a great way to inspire changes in the developers who read the book. You don’t have to do the same things that he is doing, but try to consider what the right answer should be.

The examples are simplified for the book, and business are more complex than what is in the examples. The reader will need to adjust and make decisions, but that’s what you should be doing anyway. Think about being professional and doing what you should be doing.

This book has been on my intended reading list for a while, and now I am going to recommend it to other people.

Well done, Uncle Bob. Thank you for sharing your experience with everyone.

Information Overload

After CodeMash, I was informed by many people that I had missed a great talk that Scott Hanselman presented. The talk was his “Information Overload” talk, which is about the onslaught of information that the world sends at us constantly. I’ve since discovered that he has been presenting this around the world. Last week, at StirTrek, I made a point of watching Scott’s “Information Overload” talk.

His talk is designed for developers, but it’s not developer-specific. It is all about managing the streams of information that come into our lives. As is usual, his talk is entertaining, well-presented, and contains enough impromptu comments and laughter to keep it memorable. (More than just the comedy is memorable even.)

He had a lot of suggestions about how to organize what is important, when we should deal with things, and how we should deal with things. Techniques, tools, recommendations, and guidelines are the name of the game.

If you get the chance to see the talk in person, I recommend doing so. It is one of the better talks I’ve seen presented at conferences.

Try Writing Try Methods

When I say "Try Methods", I am of course referring to the common prefix "Try" on a method, which implies that the method is going to attempt to do what you're asking by using an output parameter for the operation and using the return value to indicate whether the attempt succeeded.

The common ones that people see in the .NET Framework are the TryParse methods, which attempt to parse something and if it can't be parse, they assign the default value to the output parameter and return false. If the succeed, the parsed value will be placed in the output parameter and the return value will be true. If the code failed, the convention is to use the type’s default value to assign to the output parameter. This lets you write code like this:

int someNumber;
int.TryParse(userInput, out someNumber);
// use the number entered or the default 0
int result = 1 + someNumber;

 

This is also great when you're going to be checking whether the method succeeded. In fact, you can use the try method directly in the if block, which usually makes things nice and clean. Here is an example of using them like that:

int someNumber;
if (int.TryParse(userInput, out someNumber))
{
    // Do something with someNumber
}
else
{
    // Invalid input: handle this case accordingly
}

 

Yes, you know all of this and have seen it before, however, are you writing these types of methods yourself or is your only experience with them the standard TryParse ones?

What I am trying to emphasize here is that you need to write your own Try methods in your code. You will thank yourself later.

Everyone can see the obvious benefit of not having to check for the default value to see if it the method worked. You also don't have to have ugly Try-Catch logic in your code. You get an if statement instead.

The real benefit of a writing a Try method has nothing to do with what I've already said. The most important benefit, in my opinion, is that you have a return value from it that isn't the value you are using for your logic. By having this return value for its success, you have to decide on how to handle the case where it failed.

There are important cases that might be forgotten, but having a return value means that I need to handle the return value. When I have it, I need to decide what to do with it. There may be a message I need to display to a user. There may be logging I need to do. I might be able to ignore that case. What is important, however, is that I thought about how to handle it. I had the chance to make sure that I handled the case correctly.

Create your own “Try” methods. I recommend looking into creating your own “TryParse”, “TryGet”, TryDelete”, and anything else that has a chance of failure that you may want to handle.

Common Reuse Principle

Classes that are used together are packaged together.

This is the April topic from 2011 NimblePros Software Craftsmanship Calendar, which includes a nice quote.

Common Reuse Principle (CRP):
Classes should be packaged together when reusing one will mean reusing them all. Source: “Agile Principles, Patterns, and Practices in C#”
by Robert C Martin

This is a pretty simple principle, and one that should seem quite logical. Not everyone agrees with the principle, but it least has a good point. If two things are going to be reused together, it makes logical sense that you should package them together.

Imagine if we didn’t follow this for ASP.NET and instead had everything spread around in some random assortment of assemblies. If I were accessing my HttpRequest, which is normally in the System.Web assembly and doing something with the cookies from that, which is an HttpCookieCollection. That HttpCookieCollection is a collection of HttpCookie. All of these are in System.Web, since they’re commonly used together. There is one assembly that we need to use this full set of classes.

Now think of how much fun this development experience would be if we had broken these apart such that the HttpRequest was in the System.Web assembly, HttpCookieCollection was in System.Web.Collections assembly, and HttpCookie was in the System.Web.State assembly. That would mean having to access three different assemblies to get three closely related classes.

Don’t get me wrong, I like having smaller, broken up dependencies, so that I don’t have to depend on a lot of things I don’t need. In fact, I have to have a good number of assemblies when I use the onion architecture. (They’re not all compile time dependencies everywhere they’re used though.)

The painful part, however, is when there are obvious pieces that should be together and aren’t. Common reuse is basically saying that there is pretty much no time that I would use HttpCookieCollection without also needing HttpCookie, so they should be in the same package. (Just those two from my example. I think that HttpRequest could be somewhere else with a good reason for it. Although we all know that all of these classes are in the same assembly right now.)

What’s your take on it? There is a lot of gray area with Common Reuse Principle.