How Not to Evolve an API

I have a rather uneasy relationship with DotNetNuke, upon which my client’s membership-only web site is built. I just don’t care for DNN, but there is no one reason why. Just lots of little ones. As an example, last week I had a hair-pulling experience with DNN that has lessons for would-be framework developers.

Quick background for those not familiar with it: DNN is a content management system (CMS) for portal-type sites that has evolved over the last several years from the old IBuySpy sample application put together by Microsoft. That was one of the first ASP.NET proof-of-concept applications.

DNN has a concept of “skins” and “skin containers” for customizing look and feel. Normally you assign a skin to a DNN portal and that is the skin that’s used on all pages. However, there are times when you want to override this assigned skin for certain pages. This can be done in a number of ways, such as assigning a skin to a particular tab … er, page (pointlessly off-base terminology is another thing about DNN that I don’t care for).

For whatever reason the original developers of the system I work on chose to override skins programmatically by passing a SkinSrc querystring argument when calling up certain pages / controls / modules. You do this by appending a querystring argument to a URL similar to the following:

?SkinSrc=[G]Skins|_default|No Skin

This actually is URLEncoded and then you do your Response.Redirect() call or whatever, and the called URL will have the specified skin — in the above case a blank skin we used for a small modal dialog.

I won’t go into the wisdom of this little mini-language they cooked up for specifying skins … suffice it to say that the “[G]” is a token for which a path for global skins will be substituted by the system, and the pipes will end up being replaced with slashes to create a path, as you’ll see in a bit.

We were upgrading from DNN 3.0.13 to 3.3.7 … as a stepping-stone to ultimately getting back in sync with the current 4.x release and moving to ASP.NET 2.0. In testing the new version, I discovered that whenever we used one of these querystring skin overrides, the called page would fail, ending up at a URL that looked like this:

http://localhost/Default.aspx?tabid=591&error=illegal+characters+in+path

So I was obliged to dive into the open source of the core DNN code and figure out what was going on.

DNN routes all requests through default.aspx, and it turns out that the code-behind for that page handles the SkinSrc argument. QueryString(“SkinSrc”) gets passed to a core routine called QueryStringDecode() to get cleaned up. The old version of QueryStringDecode did pretty much what you’d expect: it URLDecodes the string and applies a couple of String.Replace() operations and returns the result. The net effect is that this:

%5bG%5dSkins%7c_default%7cNo+Skin

… becomes this:

[G]Skins|_default|No Skin

… and finally, this:

/Some/Global/Path/Skins/_default/No Skin

But in the new version of QueryStringDecode(), after doing the URLDecode(), a call is made to MapPath(), followed by some checks for cross-site scripting or attempts to access the parent directory. In other words, QueryStringDecode() has started acting like it expects paths rather than QueryString arguments. And the String.Replace() calls are simply gone.

It turns out that QueryStringDecode() is only called from a half-dozen places in the core, and is always passed a QueryString argument … but in all but two cases (including the one that was giving me fits) the QueryString argument was a path.

In essence, QueryStringDecode() was completely re-purposed and would now better be named PathVerify() or something along those lines. The URLDecode() should probably be removed and become the caller’s responsibility. In short, QueryStringDecode():

1. Has become confused as to its responsibilities — it should not assume it’s being passed a QueryString that needs URLDecoding if it’s going to deal with paths. The fact that all paths that are sent to it currently come out of QueryStrings is irrelevant.

2. Has departed so far from its original purpose that its name no longer applies

3. Has broken a legacy feature that, so far as I can find, has not been dropped or even deprecated. If it were dropped then the attempt to handle the SkinSrc argument should have been replaced by throwing a NotSupportedException.

Two one-line fixes solved the problem; instead of calling QueryStringDecode() for SkinSrc and a related ContainerSrc argument processed elsewhere, I just call HttpUtility.UrlDecode(whatever).Replace(“|”,”/”) and kaboom … it works fine. But, I still have to remember to change that in the core code with each DNN code update going forward. I’ll somehow find the time to check the 4.x source to see if it’s been fixed already and if not submit it for a future release … that’s the beauty of open source I guess. Hopefully it’ll be rolled into the core so I don’t have to make sure it stays in place on my end.

The moral of this story is that when you’re writing framework code you need to spend a lot of time thinking about your abstractions. Every bad decision you make will be enshrined in the system forever so it’s worth taking the extra effort to keep this stuff out up front as much as you can. That funky, arcane, overloaded argument convention for SkinSrc that I illustrated above was probably a rushed expediency that someone intended to re-visit before release, and never did. Worse, it apparently didn’t become part of the regression test suite and fell through the cracks entirely.

Now we are faced with a funky, arcane convention that’s broken, and documentation that calls QueryStringDecode() a security method through which all paths should be sent. Huh?! They almost might as well have assigned the method a random name.

Way to go, DNN core team!!