2

While looking at this answer to the question Why do we use ViewModels?, I came across this section:

"A view should not contain any non-presentational logic" and "You should not trust the View" (because a View could be user-provided). By providing a Model object (potentially still connected to an active DatabaseContext) a view can make malicious changes to your database.

What exactly does this refer to? If I have UserId and Password in my Model and my ViewModel, where does the security come in? Some kind of check in the controller? What do we check?

How do we determine we can trust the data from the view? Is this handled by the antiforgery token?

Sinjai
  • 1,085
  • 1
  • 15
  • 34
  • When you use a view model you can validate the data before you push to the DB if you use a model that is still connected to an active databasecontext a view could still pass in some cases bad data into the DB which would not be possible with a true view model. – Derek Hackett Jul 25 '17 at 15:45
  • @Derek Doesn't the problem lie more with improperly managing connections than not using a ViewModel, then? – Sinjai Jul 25 '17 at 15:47
  • The problem is that the model binder will blindly update values in the model. If the model is connected to something like an entity in entity framework, it may end up updating fields that you don't want it to. A proper viewmodel will be totally disconnected from anything like that. – Bradley Uffner Jul 25 '17 at 15:49
  • @Bradley isn't that only possible if we don't use the `Bind` attribute in the controller? As I understand it, even if a model with 100 properties is passed back to the controller, all but the, say, 10 I select in `Bind` are discarded. – Sinjai Jul 25 '17 at 15:58
  • Don't use `Bind`. Seriously. Just stop. Either use a view model, or simply don't directly save the entity instance created by the modelbinder, as detailed in my answer below. `Bind` is shight. – Chris Pratt Jul 25 '17 at 16:27
  • @Chris I'm self-taught, I work with the tools I'm given. What's wrong with `Bind`? – Sinjai Jul 25 '17 at 16:30
  • 1
    @Sinjai: https://cpratt.co/bind-is-evil/ – Chris Pratt Jul 25 '17 at 16:31

1 Answers1

3

I believe the answer is referring to the over-post problem. When you utilize an entity class directly with your view, and particularly if you save that posted entity directly to your database, a malicious user could modify the form to post fields they should not be able to modify.

For example, let's say you had a form that allows a user to edit widgets. Let's also say that you have row-level permissions, such that a user can only edit widgets that belong to them. So, Joe, our fictitious malicious user, edits a widget he's allowed to edit with id 123. But, he decides he wants to mess with Jane's widget, so he adds a field to the form named Id and gives it the value of Jane's widget id. When Joe then posts the widget form, Jane's widget is updated instead.

A view model is not solely for solving this problem, but it does basically negate the issue because, inherently, you cannot directly save the view model to the database. Instead, you must map the view model's values onto the entity, before saving the entity to the database. As a result, you then explicitly control what does and does not get mapped, so in the same example above, Joe changing the id ends up having no effect because you're not mapping that onto the entity.

In truth, the real problem here is in directly saving anything posted by a user directly to the database. You could actually still feed your entity class to the view as the "model", but then not save the posted instance. Instead, you then create a new instance of the entity or pull an instance from the database fresh, and simply map the values from the posted instance over to that. Again, you wouldn't map a property like Id, so again Joe is foiled. In other words, it's not the view model that solves the problem, it's the never trusting a user enough to directly save anything created via a POST that solves the issue.

Microsoft gives another alternative solution in the form of the Bind attribute, which essentially allows you to include/exclude certain properties on an entity class from the modelbinding process (ignoring any posted values, in other words). So, for example, you could potentially solve the issue above by decorating the param on your action with [Bind(Exclude = "Id")], which would then discard any posted value for Id. However, Bind is horrible for a number of reasons, and you should not actually use it. Always use a view model instead, or simply don't ever directly save the entity instance created by the modelbinder.

Chris Pratt
  • 232,153
  • 36
  • 385
  • 444
  • Very helpful article Not exactly related, but definitely doesn't warrant its own question: How do you change what gets posted to the controller? My generated edit (update) method just takes in an `int?`. I'm not even sure where that comes from, other than an `Html.HiddenFor(model => model.Id)` in the view. There's such a thing as too much abstraction... – Sinjai Jul 25 '17 at 16:45
  • It *should* be coming from the route (i.e. the URL). Edit routes will typically be something like `/foo/edit/{id}`, where `{id}` would be the primary key of the "foo" you want to edit. You *could* post it as a hidden, but again, you're opening yourself up to potential security issues. The URL uniquely identifies a particular resource (hence the name), so change the id there literally means you're request an entirely different resource. You can then handle any attempted URL manipulation with standard row-level permissioning. – Chris Pratt Jul 25 '17 at 16:57
  • The `HiddenFor` was generated. Frankly, I'm afraid to remove it as my understanding of the whole architecture is fragile at best. I'm still trying to figure out where this first parameter came from in `Update(FooViewModel model, int id)`. Do views always pass their model to the view, or what? You were great in Guardians, by the way... – Sinjai Jul 25 '17 at 17:05
  • Your example sets the new value in the controller. Would having a `ViewModel.ToModel` method that unflattens the viewmodel back to an entity object be some sort of sin? – Sinjai Jul 25 '17 at 17:09
  • No. That would technically be ok, but I'd personally go with a separate factory. That way you can keep the entity logic out of your UI entirely if you wanted. – Chris Pratt Jul 25 '17 at 17:14
  • Going to have to forego that as it's beyond my current knowledge... But could you tell me what is passing the `FooViewModel model` argument to your `Update` method? – Sinjai Jul 25 '17 at 17:18
  • The modelbinder. Under the hood it takes the post data, instantiates an instance of the class, and attempts to "bind" that post data on to the instances properties, based on naming conventions. – Chris Pratt Jul 25 '17 at 17:21
  • Totally cool if you ignore this, you've gone above and beyond already, but... How do you handle data that you need in the ViewModel but want to ensure doesn't get changed? For instance, I have a time stamp that needs to be given to the ViewModel, but then also needs to persist. [This](https://hastebin.com/ucidayezap.cs) is my current thought. – Sinjai Jul 25 '17 at 19:54
  • Your problem is here: `entry.CurrentValues.SetValues(newModel);`. That's really just for handling optimistic concurrency scenarios. You should map the values you want to save explicitly onto your entity, i.e. `entity.Foo = model.Foo;`. – Chris Pratt Jul 26 '17 at 13:43
  • I was really hung up on creating a new instance for some reason. [Here's what I have, if you're curious.](https://hastebin.com/noqanazuqe.cs) It works, if nothing else. *(You're awesome, by the way. People like you are the reason I'm able to keep doing what I love. Keep it up.)* – Sinjai Jul 26 '17 at 16:26