BindAttribute is easy to get wrong, leading to a model with no bindable properties

[rynowak]

It's easy to do the wrong thing with [Bind(...)] and end up excluding all properties from ModelBinding. The MVC5 BindAttribute supported a comma-separated list of included properties:
https://msdn.microsoft.com/en-us/library/system.web.mvc.bindattribute.include(v=vs.118).aspx

whereas MVC6 BindAttribute uses params string[]. Anyone copy-pasting code is likely to make this mistake.

Compare:

[Bind("Title", "Year", "Price",  "Genre")] // correct in MVC 6
public class Book
{
        [Required]
        public string Title { get; set; }
}

[Bind(Include = "Title, Year, Price, Genre")] // correct in MVC 5
public class Book
{
        [Required]
        public string Title { get; set; }
}

[Bind("Title, Year, Price, Genre")] // incorrect in MVC 6
public class Book
{
        [Required]
        public string Title { get; set; }
}

The MutableObjectModelBinder will attempt to handle this model, as it has ModelMetadata.IsComplexType == true, but then won't actually bind anything, because the predicate excludes all of the properties.

The result is a success but with a default-initialized model object. If you have validators on the properties, they will of course run and generally find the model to be invalid.

[Muchiachio]

Little question, will you add "Exlude" option later on, or was it design decision to only leave "Include"?

[rynowak]

/cc @blowdart

It was a design decision to only have Include this time, it's too easy to get it wrong with Exclude when you add properties to the model.

If you look at the implementation, it's now a customizable extensibility point:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/BindAttribute.cs

https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/IPropertyBindingPredicateProvider.cs

You could implement your own policy that does something creative if Include by itself isn't sufficient. Here's an example that does Include using expression trees:

Mvc/test/WebSites/ModelBindingWebSite/Controllers/BindAttributeController.cs

Line 73 in 64e726d

private class ExcludeUserPropertiesAtParameter : DefaultPropertyBindingPredicateProvider<User>

We'd still recommend as the most secure option, only using view models that expose just what you want to bind. If you can't use viewmodels or are porting a legacy then whitelisting (Include) is the next best thing.

[Muchiachio]

I fully agree that including just the needed properties is the best approach. But I found it hard to exclude the "Id" property from the model. Here's a little use case to show you what I mean.

I have a base view model which looks like this, as you can see I am generating "Id" value on every property request if it's not set already, and it's being set back using entity framework and auto mapper queries for index, edit and other views. You could say that I am too lazy to set Ids in the controller or service layer 😄

This works fine until I need to create a record, a simple:

public ActionResult Create(AccountCreateView account)

can be abused by a "smart user", to change "Id" to something bad like "YoMammaSoFat...". So using:

[Bind(Exclude = "Id")]

on create actions doesn't hurt much, especially knowing that scaffolding can be used without knowing model properties, which is not so easy with the white listing approach. So I am just going for exclude like this on every action which creates records.

Nevertheless, I will try to extend it as you suggested and will report back if I have any useful feedback.

[blowdart]

Putting on my official security hat 🔒

Exclude becomes very problematic in multi-developer environments as people add properties to models, without updating the exclude list. After discussion we decided there was no way to guard against this, and we should drop support to improve security. White lists are simply safer than blacklists.

Really view models are the way to go, binding to, for example, your EF model classes is fraught with over binding danger and makes me sad.

[Eilon]

We should detect commas in each string and split them each on commas to make a big list. E.g. need to handle the case of "foo,bar", "qux", "quux, quuux".

[IDisposable]

👍 @Eilon

[ajaybhargavb]

f5cabf2