Bind Is Evil

There's a handful of things that boil my blood, as a developer. One of those is the seemingly innocuous BindAttribute. Unfortunately, Microsoft seems to have a knack for placing landmines in their products: things that seem like they belong, things which Microsoft will even recommend that you use, but things which will blow up in your face as soon as you do. BindAttribute is one of those.

Why does it exist?

Well, frankly, I think it exists merely simplify code samples. Look at any ASP.NET MVC 101 blog/tutorial/video/etc. and you'll see something like the following:

[HttpPost]
public ActionResult Update(Foo foo)  
{
    if (ModelState.IsValid)
    {
        db.Entry(foo).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index")
    }

    return View(foo);
}

This is all pretty straight-forward. A user has posted some new data for a Foo instance, so assuming it's all valid, we save it to the database. The only problem is that we're saving all the properties on Foo. Anything the user posts, as long as it can be bound to an instance of Foo will then go directly into our database, and guess what one of those properties you use can post is? If you guessed the primary key, you've got a good head on your shoulders. What if we're editing a Foo with Id set to 1, and the malicious little brat of a user decides they're going to use their browser's developer tools to ensure that a field with name Id is posted with a value of 2? With the code above, the Foo record in the database with 2 as its key will be updated. Whoops. That's not good. Enter BindAttribute:

[HttpPost]
public ActionResult Update([Bind(Exclude = "Id")]Foo foo)  

Now, the model binder will simply ignore any posted value for the Id property. Simple enough, right? Well, not quite. First, there's issue of string data. What if in later development we change that key property to FooId? What happens then? In a word, nothing: no error, no exception, no yellow screen of death. In fact, everything will "just work", except for one little thing: malicious users can now manipulate our form again. Because we're setting the properties to exclude as a string, changes to the actual properties don't signal that a change needs to be made to these Binds as well.

Then, let's assume that you're an extremely diligent developer who never ever forgets to check for things like this (no offense, but yeah freaking right). What if there's multiple places we've done something like this? Because Bind only applies to this one particular action, if we use Foo in this way in some other action, we also need to apply the Bind there as well. Not only are we increasing the entropy of our code, but we're also adding additional maintenance concerns. You now need to track down all the instances where Foo has been used in this way, and ensure you fix all the Binds accordingly. Don't know about you, but to me, that sounds completely unappealing.

But, wait. We're not done. The potentially worse issue is that this code won't actually even work at this point. Can you guess why? I can't blame you if you can't, because it's not obvious at all. The worst bugs are the ones that aren't obvious. Those are the ones that waste hours of your day and keep you up at night. The issue here is that because we excluded Id, when the modelbinder created our foo instance, the Id property just gets its default. In the case of something like an int that would be 0. Now, you're attempting to update a record that doesn't even exist, and the landmine goes off: yellow screen of death. A typical workaround is to pass the id as part of the URL, which is actually good practice regardless:

[HttpPost]
public ActionResult Update(Foo foo, int id)  
{
    if (ModelState.IsValid)
    {
        foo.Id = id;
        db.Entry(foo).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index")
    }

    return View(foo);
}

This is better, as now we're not letting the user silently manipulate the id. It's coming from the URL. If the user changes the URL, they truly are updating a different resource, and things like object-level permissions can be employed at that point to prevent malicious changing of records they should have access to.

A Better Way

You know what's better than all of this, though? Take a look at the following code:

[HttpPost]
public ActionResult Update(FooViewModel model, int id)  
{
    var foo = db.Foos.Find(id);
    if (foo == null)
    {
        return new HttpNotFoundResult();
    }

    if (ModelState.IsValid)
    {
        foo.Bar = model.Bar;

        db.Entry(foo).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index")
    }

    return View(model);
}

You might be thinking that that's a bit more code. Yes, it is. As is the case with most things in life, the right way isn't usually the easy way. However, this code buys you a number of things:

  1. You know that record actually exists. If it doesn't, the user gets a 404 instead of a server error when it blows up later.
  2. You have complete control over what the user posts. Nothing is modified on your entity without your explicit permission. If you want to update a property, then you explicitly set the property on the entity to the posted value, otherwise, you don't. It's explicit and glanceable, i.e. there's no guess-work. What you see is what you get, as it were.
  3. Everything is strongly-typed. If I change the name of the Bar property, the build will fail until I update it here as well. And, although I haven't done it here, this mapping is abstractable. In other words, you can factor out the logic for how Foo is mapped to and from FooViewModel and reuse that logic.

Death to Bind

In the end, what we have here is something which encourages bad development practices, which isn't reusable, which the functionality of can be broken unwittingly by changes elsewhere in the codebase, and which can introduce non-obvious and sometimes difficult to diagnose bugs. Or, you can use a view model and few more lines of code and have a solution which is explicit, abstractable, and obvious. Hmm, don't know about you, but I'll take door number two. Now, if only I could get Microsoft to take down all the sample code that leads new developers astray.

comments powered by Disqus