Ayende @ Rahien

It's a girl

Elegant Code, Raccoon Blog’s CSS Controller

The following piece of code is responsible for the CSS generation for this blog:

public class CssController : Controller
{
     public ActionResult Merge(string[] files)
     {
         var builder = new StringBuilder();
         foreach (var file in files)
         {
             var pathAllowed = Server.MapPath(Url.Content("~/Content/css"));
             var normalizeFile = Server.MapPath(Url.Content(Path.Combine("~/Content/css", file)));
             if (normalizeFile.StartsWith(pathAllowed) == false)
             {
                 return HttpNotFound("Path not allowed");
             }
             if (System.IO.File.Exists(normalizeFile))
             {
                 Response.AddFileDependency(normalizeFile);
                 builder.AppendLine(System.IO.File.ReadAllText(normalizeFile));
             }
         }

         Response.Cache.VaryByParams["files"] = true;
         Response.Cache.SetLastModifiedFromFileDependencies();
         Response.Cache.SetETagFromFileDependencies();
         Response.Cache.SetCacheability(HttpCacheability.Public);

         var css = dotless.Core.Less.Parse(builder.ToString(), new DotlessConfiguration());

         return Content(css, "text/css");
     }
}

There are a lot of things going on in a very short amount of code. The CSS for this blog is defined as:

<link rel="stylesheet" type="text/css" href="/blog/css?files=ResetCss.css&files=custom/ayende.settings.less.css&files=base.less.css&files=custom/ayende.less.css">

This means that while we have multiple CSS files that make maintaining things easier, we only make a single request to the server to get all of them.

Next, and important, we have a security check that ensures that only files from the appropriate path can be served. If you aren’t in the CSS directory, you won’t be returned.

Then we have a lot of code that is related to caching, which basically means that we will rely on the ASP.Net output cache to do everything for us. The really nice thing is that unless the files change, we will not be executing this code again, rather, it would be served directly from the cache, without any computation on our part.

All in all, I am more than happy with this code.

Comments

Dave
06/07/2011 09:49 AM by
Dave

I recommend that you also use a CSS minifier before returning the results. We have a similar solution and can indicate with (&minify=0) to not minify the CSS if I want to debug the dotless output.

We have based out minifier on this blogpost http://www.ko-sw.com/Blog/post/An-Ultra-Fast-CSS-Minify-Algorithm.aspx. Second we changed a few thing so we could also minify script (.js) files.

Matt
06/07/2011 10:06 AM by
Matt

You really should look into Combres too - it does all this work for you, except that it's completely configurable and functions differently in Debug vs. Release - in Debug mode you get all the separate files loaded separately; in Release mode they get minified, combined, and sent as one.

It does JavaScript too, and the minifier etc. is all configurable.

Felix
06/07/2011 10:22 AM by
Felix

I'm using very similar code and noticed that browsers occasionally will still re-ask for the files and IIS wouldn't serve them from the cache. So adding checks for If-ModifiedXXX headers will allow you to return 304 instead of sending the entire file stream.

tobi
06/07/2011 10:32 AM by
tobi

"If you aren’t in the CSS directory, you won’t be returned." Are you sure "..\" is stripped from the path? I don't like that path validation at all. I would restrict the input file names to letters, numbers and "-._". That way you white list and are safe.

Erik Juhlin
06/07/2011 11:53 AM by
Erik Juhlin

Yes, this code is nice. Didn't know about the cache/file dependencies. I've always done that coding myself. I've looked at dotless, but thought it lacked a good caching strategy.

The only problem I can see with this code is that files that are imported with the @import directive will not be added as dependencies. It would be nice if dotless could include this in the HttpHandler. I prefer using @import instead of putting the filenames in the querystring.

Erik Juhlin
06/07/2011 11:59 AM by
Erik Juhlin

@tobi, I think that validation is pretty much best practice. It doesn't stop you from writing "files=../css/resetcss.css", but it doesn't matter since you will still be in the correct directory. And it can be nice to be able to write "files=layout/foo.css".

Ryan Heath
06/07/2011 12:08 PM by
Ryan Heath

I don't like the files parameter. I would hardcode the css files you need in code. And even make another action method if you need to include different css files.

public ActionResult CssOne() { return Merge("base.css", "one.css"); } public ActionResult CssTwo() { return Merge("base.css", "two.css"); }

private ActionResult Merge(param string[] files) { // your implementation }

Dan Plaskon
06/07/2011 12:25 PM by
Dan Plaskon

Another bump for Combres; it does this and a WHOLE lot more.

Jesse Williamson
06/07/2011 01:00 PM by
Jesse Williamson

I like this very much, and I would disagree with Ryan about hardcoding your file names in your controller. I saw an interesting alternative from Mads Kristensen from a talk he gave at Mix (http://channel9.msdn.com/events/MIX/MIX11/FRM04). What I liked about his solution was the hashing of the files to create a unique filename. So the view calls a static function that creates the file name, and if the CSS or JS ever changes, the file name reflects that and the new version is sent to the browser as needed.

Ayende Rahien
06/07/2011 01:32 PM by
Ayende Rahien

Tobi, Did you see the call to MapPath, as part of its work, it will normalize the path properly.

Patrick Smacchia
06/07/2011 01:51 PM by
Patrick Smacchia

It would look safer to me with...

normalizeFile.ToLower().StartsWith(pathAllowed.ToLower())

... since path are case insensitive. I hate treating path as string!

http://codebetter.com/patricksmacchia/2008/12/28/what-is-microsoft-waiting-for-providing-a-descent-path-api/

JC
06/07/2011 02:01 PM by
JC

You can take this line from inside the loop.

var pathAllowed = Server.MapPath(Url.Content("~/Content/css"

Kiliman
06/07/2011 02:01 PM by
Kiliman

@Patrick, I'm not sure why it would be safer. The code disallows anything that is not exactly ~/Content/css. So using incorrect case would simply return 404. It's a pretty restrictive whitelist, which is all that is needed.

tobi
06/07/2011 03:59 PM by
tobi

I saw the call to MapPath but was unsure about its contract. And that is what bothers me: I do not feel sure about this.

PS: I would appreciate if the Name/Email input was remembered by the comment form.

Ryan Heath
06/07/2011 05:40 PM by
Ryan Heath

@Jesse I don't like the fact that the client can determine which files are going to be merged. It's only a security bug waiting to be exploited. Of course Ayende has checked the path, but another dev could easily remove the check (unintended) and all seems to be ok and working, but then the server is left wide open ...

I would not perse (hard)code it into the controller. Via a route or some other mapping (hashing,lookup) would be fine too, but certainly not on the client.

// Ryan

Bertrand Le Roy
06/07/2011 07:24 PM by
Bertrand Le Roy

If I'm not mistaken, there is a minor issue that you may flood the cache by querying many different combinations of existing files (and maybe also requiring the same file more than once). While the file dependency call might mitigate this somehow, adding a crypto validation hash of sorts might help if that becomes a problem (unlikely but eh). Never a fan of crypto stuff on the querystring but I thought I would mention it.

tobi
06/07/2011 08:34 PM by
tobi

You could even overload the server by causing it to process the css over and over.

Alistair
06/07/2011 11:30 PM by
Alistair

Yeah there are some ugly caching implications of this code. Specifically it violates:

http://code.google.com/speed/page-speed/docs/caching.html#LeverageProxyCaching

So some proxies won't cache this resource as there is a querystring in the url.

Additionally, how do you handle refreshing peoples cache? Presumably with something like &version=1?

Luke
06/07/2011 11:48 PM by
Luke

Interested in why you use == false instead of !. I assume that is to increase readability. Is that something you do throughout your code?

Ayende Rahien
06/08/2011 09:06 AM by
Ayende Rahien

Bertrand, That is just another way to say DDoS, and there are other ways to mitigate against that. You couldn't really flood the cache, since the only things that goes into the cache are the actual files on disk. And even if you flood the cache, nothing bad would happen, things would just not be cached.

Ayende Rahien
06/08/2011 09:06 AM by
Ayende Rahien

Tobi, How could you do that?

Dave Roberts
06/08/2011 11:29 AM by
Dave Roberts

Path validation is hard. The best approach I've seen is CAS (http://msdn.microsoft.com/en-us/library/930b76w0(v=VS.90).aspx):

var perm = new FileIOPermission(FileIOPermissionAccess.Read, rootPhysicalPath); perm.PermitOnly();

Lots of the discussions around CAS are for sandboxing plugins, but I've found it very useful in cases like this.

tobi
06/08/2011 12:52 PM by
tobi

Overload it like this:

from i in Enumerable.Range(0, 10000000) let files = new [] { "normal.css", i + ".css" } select "/css?files=" + files.Concat(",");

And request all of those URLs. Solution: Return 404 if any file does not exist and, if files is not sorted, redirect to the canonical URL.

Bertrand Le Roy
06/08/2011 05:42 PM by
Bertrand Le Roy

@Ayende: right, that's why I said it was probably a minor issue ;) This being said, unless I'm missing something, what goes into the cache is the combination of files (so yes it would take a lot of these to flood the cache) but my point was that it's keyed by the Files querystring parameter, and it's not handling multiple inclusions of the same file. So if you wanted to mitigate that maybe just taking distinct paths and/or rejecting requests with duplicates would help. If you consider the threat to be serious. Which we would in a framework but you probably would justifiably dismiss in this specific context.

tobi
06/08/2011 05:42 PM by
tobi

And I forgot that you have to normalize casing and filenames may not contain ".." etc. because that would also allow multiple URLs for the same content. Disallow duplicates.

Harry Steinhilber
06/08/2011 06:53 PM by
Harry Steinhilber

@tobi,

The code is exiting with an HttpNotFound on the first file that doesn't exist on disk. So in your scenario, it would exit with a 404 once it hit the 0.css file. It won't even attempt to process the other 9,999,999 files. So I don't believe this could overload it.

I have to agree that not handling the duplicate file bit could be an issue, though.

Harry Steinhilber
06/08/2011 06:56 PM by
Harry Steinhilber

Nevermind, the 404 will occur for outside of the correct path. So I suppose that could be an issue.

Jonathan Mahoney
06/09/2011 06:04 AM by
Jonathan Mahoney

I have the same concerns as Alistair - if you're using MVC you could create a route that took something like this:

/CSS/{files}.{version}.css

Make 'files' an underscore delimited list of the files, which you split and use in your controller code above. 'Version' doesn't even need to be used server side - it could just be a number to do cache busting.

However one thing I might like as an addition is a variable that allows you to serve the full (for development) or minified version.

Dmitry
06/09/2011 02:03 PM by
Dmitry

Many proxies do not cache URLs with ?querystring in them. It is better to make files a part of the actual URL like proposed above. The only negative is there is a shorter length limitation.

dario-g
06/09/2011 10:07 PM by
dario-g

Check out: http://mscd.codeplex.com

RichB
07/04/2011 09:07 AM by
RichB

Path.Combine("~/Content/css", file)

This combines with a backslash delimiter, when the other segments contain forward slashes. Better to use VirtualPathUtility.Combine():

http://msdn.microsoft.com/en-us/library/system.web.virtualpathutility.combine.aspx

Also if you put a rooted path into the files parameter, you will get a YSOD eg: http://localhost/css?files=c:\boot.ini

Comments have been closed on this topic.