Hi, I installed MVCToolkit and played with tutorials. I tried to create some Product object (Linq To Sql) in an action method: But encoding for string values in Request.Form didn't happen and I successfully saved javascript code as product name :(

[ControllerAction]
public void CreateNew()
{
   Product prod = new Product();
   prod.UpdateFrom(Request.Form);
   northwind.AddProduct(prod);
   northwind.SubmitChanges();
   .....
}

If you need to apply custom logic to the data that's assigned, the current UpdateFrom() method might not be a good approach to you. There isn't a way to control programmatically any encoding or access restrictions (besides passing an array of property names).

If would be nice if, in future, there was an extended version of UpdateFrom() that would take a delegate to call back some custom logic to handle each member.  It could look like this:

// Defined by the framework
public class UpdateMemberData
{
	public object Value { get; set; }
	public bool AllowUpdate { get; set; }
}
public delegate void UpdateMemberDelegate(Object targetObject, MemberInfo targetMember, UpdateMemberData data);
public static void UpdateFrom(this T obj, NameValueCollection values, UpdateMemberDelegate callback) { ... }

// Called by the user
myObject.UpdateFrom(Request.Form, delegate(Object targetObject, MemberInfo targetMember, UpdateMemberData data) {
	switch (targetMember.Name)
	{
		case "SomeProperty":
		case "SomeOtherProperty":
			data.Value = HttpUtility.HtmlEncode((string)data.Value);
			break;
		case "UnprotectedProperty":
			break; // Allow unescaped data
		default:
			data.AllowUpdate = false; // Block access to any other members
			break;
	}
	data.AllowUpdate = false; 
});

Not really sure how advantageous this is anyway, as some type-safety is lost.

How the javascript getting into the form fields?

I found an acceptable solution (for me :) ):

In BindingHelpers.cs file of MVCToolkit solution I added some code to UpdateForm method (at line number 100)...

...
string strValue = value as string;
if (strValue != null)
   value = HttpUtility.HtmlEncode(strValue);
...

slynch:

How the javascript getting into the form fields?

I just entered <script>alert('possible xss');</script> into input field

Sorry, what you ment was pretty obvious now that I think of it.

You might want to consider adding something like this so it wont be replace if you update to the next drop of the MVCToolkit

namespace System.Web.Mvc.BindingHelpers{
public static class CustomBindingHelperExtentions{
  public static void UpdateFrom(this object obj, NameValueCollection values, Expression<Func<string, string>> encoder, params string[] keys)
  {
    NameValueCollection encodedValues = new NameValueCollection();
    Func<string, string> encodeFunc = encoder.Compile();
    foreach (string key in values.Keys)
      encodedValues.Add(key, encodeFunc(values[key]));
    if (keys.Length > 0)
      obj.UpdateFrom(encodedValues, keys);
    else
      obj.UpdateFrom(encodedValues);
    }
  }
}

And then call it like:

prod.UpdateFrom(Request.Form,c=>HttpUtility.HtmlEndoce(c));

Thanks for the feedback - we're going to make two changes to the helpers in the next drop:

1) Encoding will be ON by default

2) There will be an override to turn it off (if you use an RTE for instance)

The other alternative (as you've seen above) is to override what we're doing with your own method for now.

I should have done it this way in the first place :).

quick question, I'm a little confused, but why would want to by default HtmlEncode an input and stuff that into your DomainObject and ultimately into the database? shouldn't it be the other way around, you HtmlEncode it for displaying purpose, and store it as is?

It can be either way - but the safest bet is "protect inbound" since you never know where that info will turn up again. For instance if you're object is a ProductReview and you forget to encode the output. It happens.

Also - it's not a very expensive operation, but it is string manipulation and therefore has a cost to it. If you encode on every output it can add up and hurts scaling.

slynch:

Sorry, what you ment was pretty obvious now that I think of it.

You might want to consider adding something like this so it wont be replace if you update to the next drop of the MVCToolkit

namespace System.Web.Mvc.BindingHelpers{
public static class CustomBindingHelperExtentions{
  public static void UpdateFrom(this object obj, NameValueCollection values, Expression<Func<string, string>> encoder, params string[] keys)
  {
    NameValueCollection encodedValues = new NameValueCollection();
    Func<string, string> encodeFunc = encoder.Compile();
    foreach (string key in values.Keys)
      encodedValues.Add(key, encodeFunc(values[key]));
    if (keys.Length > 0)
      obj.UpdateFrom(encodedValues, keys);
    else
      obj.UpdateFrom(encodedValues);
    }
  }
}

And then call it like:

prod.UpdateFrom(Request.Form,c=>HttpUtility.HtmlEndoce(c));

Thank you, it's the best solution for now

Rob, please tell me you're not serious about HTML-encoding all input by default. I appreciate that you're trying to protect people from their own ignorance or forgetfulness, but as shinakuma indicated, this causes more problems than it solves.

[1] It's an inappropriate mix of technologies. HTML encoding is only a concern of displaying HTML - it has no place in a SQL database. It's ugly when querying, interferes with full-text indexing, and clashes with other applications that might be reading from the same DB. What about when I'm not outputting to HTML (e.g. when rendering to a plain-text email, WinForms app, or 3rd party system) - do I have to unescape it first? Yuck!

[2] It leads to more serious security vulnerabilities. You say it's safer to "protect inbound", but here the opposite is true. Remember that by saying "we'll protect inbound", you're also saying "you must not protect outbound" (otherwise you'll double-escape and & will render as &amp;). So now we are forced to trust that all systems writing to the DB remember to encode all strings exactly once, and any single vulnerability spreads to all parts of the system because nothing protects outbound. "Perimiter security" is not safer when it actually prevents you from adding real security.

[3] It's inconsistent with how decent ASP.NET controls work. Well-written controls take responsibility for HTML-encoding their own output, so pre-escaped strings would cause display problems.

[4] It's inconsistent with how other aspects of the MVC framework work. You're not pre-escaping all values passed to IHttpRequest.Form, so myObject.Value = form['key'] would behave differently to myObject.UpdateFrom(form).

If someone on my team encoded strings before putting them into the DB, then depended on that when rendering them later, I'd have to tell them off and ask them to change it. Please don't encourage this behaviour!

+1Steve

Um... maybe that was an excessive rant - I do really like the MVC framework so far! Good work

ScottGu and I talked a bit about this and HTMLEncoding a security concern so we have to take this into consideration when deciding how to deal with these things.

SteveSanderson1:

It's an inappropriate mix of technologies. HTML encoding is only a concern of displaying HTML - it has no place in a SQL database. It's ugly when querying, interferes with full-text indexing, and clashes with other applications that might be reading from the same DB.

In terms of it's "place" - I highly disagree. Anything that is being reported back to a web front end (usually a browser) should, at some level, be checked so that you don't inadvertantly sabotage your user. Database, flatfile, cache - the storage medium has nothing to do with this :). FullText indexing ignores anything in <>.

One thing you can do here is to Encode on the way out - but again that's a perf thing you'd need to think about. I hear you though about the other readability issues, and we can have an override to turn it off.

SteveSanderson1:

"Perimiter security" is not safer when it actually prevents you from adding real security.

Yep. Security comes at a cost at some level and I agree with you in principle here. However you're talking about a 1% issue at best (shared DB input) and many architects would completely cringe at sharing a DB with a web site for this very reason. However your point is valid and again - we'll provide an override so the decision to turn this off if you need.

SteveSanderson1:

It's inconsistent with how other aspects of the MVC framework work.

Hopefully not - but if we're not encoding properly we need to take a look at that.

Ultimately it boils down to line of responsibility - should Microsoft, by default, disable security or should you, as the developer, disable it for yourself? I'm all for trying to find a better answer here - would another route be acceptable - something like "UpdateFromEncode(Request.Form)"? The thing is - and I'm going to stick to this - the decision to turn encoding off needs to be the developer's.

--Iniviting ScottGu and DamienG to this thread - it's a good one and it's important.

We are talking about Encoding not Decoding.... The name alone dictates the direction.

What if I am not even outputting html but some other format.  Now I need to ensure that all my input is not encoded?

On by default is just confusing.  I would change the method names all together so it is explicit what is going on.