This post originated from an RSS feed registered with .NET Buzz by Eric Gunnerson. Original Post: C#: What's wrong with this code #5 - Discussion Feed Title: Eric Gunnerson's C# Compendium Feed URL: /msdnerror.htm?aspxerrorpath=/ericgu/Rss.aspx Feed Description: Eric comments on C#, programming and dotnet in general, and the aerodynamic characteristics of the red-nosed flying squirrel of the Lesser Antilles Latest .NET Buzz Posts Latest .NET Buzz Posts by Eric Gunnerson Latest Posts From Eric Gunnerson's C# Compendium

Thanks for all the comments.

As a refresher, here's the code that we were looking at:

enum ResponseType { ReturnValues, InvalidInput }

string CreateWebText(string userInput, ResponseType operation) {
   switch (operation) {
      case ResponseType.ReturnValues:
         userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput);
         break;
      case ResponseType.InvalidInput:
         userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput);
         break;
   }
   return userInput;
}

First, there were a few comments about not generating HTML yourself, not echoing input back to the user, and not writing something that is already present in ASP.NET.

Agreed, agreed, agreed. There are lots of attacks that depend on this, and you shouldn't even do it for debugging.

There was considerable discussion about whether the variable "userInput" should be reused. In most cases, I think that it's best to come up with a new name, but there are cases where it makes more sense to reuse the name. I don't feel strongly about this case.

Finally, on to the big thing - or what I define as the big thing, at least - the handling of the enum.

Enums in C# are double-duty - they serve both as sets and as bit fields. That means that an enum can have any value that is valid on the underlying type. In this case, there's no type defined, so it's an int. In other words, you can write:

ResponseType responseType = (ResponseType) 157;

and things will work just fine.

So, we need a way to validate the value (assuming, of course, that we're going to keep this routine. We wouldn't in practice, but humor me...)

Here's a modification that does that:

string CreateWebText(string userInput, ResponseType operation) {
   if (!Enum.IsDefined(typeof(ResponseType), operation)
      throw new InvalidArgumentException(...);

   switch (operation) {
      case ResponseType.ReturnValues:
         userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput);
         break;
      case ResponseType.InvalidInput:
         userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput);
         break;
   }
   return userInput;
}

And that works just fine.

Or does it?