What's Wrong With This Code? (#29)

Monday, April 2, 2012

Here is some code similar to other code I've seen that has a severe bug. The Entity Framework model configuration is setup to allow optimistic concurrency checks via a movie's Version property.

public class Movie
{
    public virtual int ID { get; set; }
    public virtual string Title { get; set; }
    public byte[] Version { get; set; }
}

public class MovieDB : DbContext
{
    public DbSet<Movie> Movies { get; set; }

    protected override void OnModelCreating(
        DbModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Movie>()
                    .Property(p => p.Version)
                    .IsRowVersion();
        base.OnModelCreating(modelBuilder);
    }
}

During editing, the view correctly holds the row version in a hidden input.

@Html.HiddenFor(model => model.Version)

But unfortunately, the controller action accepting the edit request has a flaw. It's trying to retrieve the existing movie from the database, overwrite the entity with new values (and the previously retrieved version value), then save the entity. The idea is that if the underlying record changed, we'll see an optimistic concurrency exception because we gave the entity an old row version value.

[HttpPost]
public ActionResult Edit(int id, Movie editedMovie)
{
    if (ModelState.IsValid)
    {
        var movie = db.Movies.Find(id);
        movie.Title = editedMovie.Title;
        movie.Version = editedMovie.Version;       
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(editedMovie);
}

The update will almost never fail with an OptimisticConcurrencyException. The update will almost always overwrite the current record in the database*.

What's wrong?

Hint: the Entity Framework doesn't behave like the implementation expects.

Answer

A property marked as a RowVersion property implicitly holds a  "store-generated" value. The Entity Framework never expects an application to modify the value. When you retrieve an entity and set a computed property to a new value, you'll still see the original value sent to the database when you call SaveChanges. The Entity Framework will always use the original value loaded from the database. The correct way to save the new values is to "attach" the movie instead of using a query to retrieve values from the database.

[HttpPost]
public ActionResult Edit(Movie editedMovie)
{
    if (ModelState.IsValid)
    {
        db.Entry(editedMovie).State = EntityState.Modified;
        db.SaveChanges();
        return RedirectToAction("Index");
    }
    return View(editedMovie);
}

* I say "almost" because you could have a concurrency exception if the data changes between the query and the call to SaveChanges (where time is measured in milliseconds). It doesn't fail if the data changes between the Edit GET request and the POST request (where time might be measured in minutes).


Comments
gravatar TweeZz Monday, April 2, 2012
Hi..

Sry, I'm not really understanding this one. Shouldn't the editedMovie instance be 'attached' to the Movies collection before calling SaveChanges? Or is it somehow the db.Entry(editedMovie) call that does that?

Manu.
gravatar scott Monday, April 2, 2012
@Manu - yes. With DbContext, the Entry method will do an attach and tell the context "here is an object you need to track but is already persisted - you just need to manage changes and updates".
tobi Monday, April 2, 2012
You certainly shouldn't attach an object which is entirely controlled by unfiltered user input! Some other part of your app might get this object back from a query and rely on its values.
gravatar scott Monday, April 2, 2012
@tobi: Agreed - it's not the focus of the post, but asp.net input validation should be sufficient for this simple scenario.
gravatar tobi Monday, April 2, 2012
No! Users can just set any IsAdmin property to true. This is so dangerous. What validation on earth is going to protect against that? true is a valid value!
gravatar scott Monday, April 2, 2012
@tobi - There is no IsAdmin flag.
gravatar Jonathan Allen Monday, April 2, 2012
Are you seriously suggesting that we take a raw entity from the client and blindly save it in the database?

Did you not see the countless news articles about how GitHub was compromised because they did exactly that?

It is this type of sloppy design that makes me really dislike ORMs.
gravatar scott Monday, April 2, 2012
@Johnathan: If you want to prevent mass assignment, see this post: odetocode.com/...

The flaw in the code would exist regardless of how many validations the 8 byte Version passes through.
gravatar Kristof Claes Tuesday, April 3, 2012
Would this work differently when passing ViewModels between your Views and Controllers?
gravatar scott Tuesday, April 3, 2012
@Kristof: No, it all boils down to "attaching" an existing entity versus querying and applying changes to an entity. The Version property for optimistic concurrency would have to come through the view model, and if it was used to attach (with .Entry) everything would be fine, if you query first and copy over properties - same problem.

Does that make sense?
gravatar tobi Tuesday, April 3, 2012
Copying over properties explicitly is safe because you decide for each and every property.

Copying over properties implicitly (using reflection or model binding or automapper) you _will_ get a security bug sooner or later. Blacklisting does not help here because this is about human error. It is not about technology.

There might not be an IsAdmin property but what if I post the key-value pair

editedMovie.AuthorUser.IsAdmin=true

Did you think about that? No? That was human error at work.

What if someone else adds an IsAdmin flag later? Then: #fail

This pattern just cannot be fixed. It is f*cked to no end.
gravatar Scott Allen Wednesday, April 4, 2012
@tobi: If you copy over properties one by one, you can still get bit by the bug in the code.

I think you are looking for the post from last month, too: odetocode.com/...
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!