+972 52-548-6969
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
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.
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.
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.
"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.
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.
@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".
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 }
Another bump for Combres; it does this and a WHOLE lot more.
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.
Tobi, Did you see the call to MapPath, as part of its work, it will normalize the path properly.
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/
You can take this line from inside the loop.
var pathAllowed = Server.MapPath(Url.Content("~/Content/css"
@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.
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.
@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
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.
You could even overload the server by causing it to process the css over and over.
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?
Interested in why you use == false instead of !. I assume that is to increase readability. Is that something you do throughout your code?
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.
Tobi, How could you do that?
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.
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.
@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.
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.
@tobi,
The code is exiting with an
HttpNotFoundon the first file that doesn't exist on disk. So in your scenario, it would exit with a 404 once it hit the0.cssfile. 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.
Nevermind, the 404 will occur for outside of the correct path. So I suppose that could be an issue.
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.
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.
Check out: http://mscd.codeplex.com
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
Comment preview