C# asp.net build SQL Query dynamically from viewstate

Question

Hi all,

On my C# asp.net webform I have a search page that has roughly 20 elements that "could" be used as part of the search. There will be more added later.

What I have done is extend the textbox and dropdown listbox to include a few variables:

fieldname: Tablename.columnname dbtype: DbType.Int32 Joinparam: LEFT Join on otherTable ON x.y = a.b

These are all stored in the viewstate and loaded back in. The reason I do this is so that I can iterate through all the controls and pull out all the controls that are of my type. I can then validate them to make sure they have input and are of the right type. If so I can then pass them off to the database access layer and let the code dynamically generate the SQL statement.

I do not let anything but SELECT statements happen from this. The fields selected and returned can not be changed and I use the dbparameter to try and avoid sql injection.

My worry is that I put the table and field names that will be used in the search criteria and the JOINS required all in the viewstate. Is this a real bad idea?

I could obscurify this by just having some int index's into tables that hold this info in the DB but this would still need to be put into the viewstate and just mean they would have an extra layer to figure out.

The reason I went for this approach was that I did not want to have to put tons of IF statements in the DB layer to build the statement there. It would be ugly as hell and a pain to maintain.

Thanks for any and all advice about this.

Jon

EDIT

Thanks all for the advice. Thankfully this app is an internal thing only so the damage would be limited. But I will never use this technique again and will work on the search template idea instead.

Cheers :)

Answer 1

I think it's a real design mistake to encode parts of your data access layer in your view logic. Putting aside the security concerns, this is going to be really hard to maintain and understand for anyone coming after you. I think a factory class to produce your specific queries from the various selected inputs is probably going to be easier to work with in the long run. Or perhaps, you could populate a "search template" from the inputs and have the search template function as the factory for producing the query, much like the way the UserPrincipal interacts with a PrincipalSearcher in the System.DirectoryServices.AccountManagement namespace.

Answer 2

Viewstate is not encrypted, it is base64 encoded. There are utilities readily available that will allow you to decode a page's viewstate.

It is possible to encrypt viewstate for a page or all of your pages however: http://msdn.microsoft.com/en-us/library/aa479501.aspx

That said, I would not recommend this approach. The best approach for application design is to decouple your UI from your business and data access logic. In this case, you would be closely coupling them for no apparent benefit.

If you did build a more robust ad-hoc query feature into your data access layer, you might be adding value to the back end of your application. You could provide search capabilities via web services, windows forms apps etc. Even if this type of functionality is not something you envision happening in the near future, it can be a huge time (and $$$) saver when you take this approach.

The logic you have built into the UI could easily be built into some type of query engine. Instead of the IFs that you wanted to avoid, you can build a method to assemble a query by dynamically adding criteria to it.

Answer 3

Okay, so ViewState defaults to being MACed, and can optionally be encrypted. So, though you can (by default) read viewstate, tampering isn't trivial.

Building your query dynamically like this most definitely opens you up to Sql Injection - you're parameterizing the search terms, but if I can inject a ;DROP TABLE statement into your JOIN clause, you're hosed (unless you've taken good care to set DB level security, of course).

That being said, I don't see any reason that your JOIN clauses and such would ever need to be in ViewState. I'd think you'd set them as properties of your subclassed textbox in the ASPX markup - there's no reason for them to change. The only thing you may need ViewState for is the search condition itself - which is appropriately parameterized. This is similar to how the SqlDataSource protects it's SelectCommand property - don't store it in ViewState so as not to leak information (or risk tampering - though I don't think that's a serious vulnerability).

As to the larger question of whether this is maintainable - well, you'll have to make that call I suppose.