Thursday, February 18, 2010

Antisample - Custom implementation of TextBoxWithLabelFor

Another, this time "almost dangerous" code from ISBN 978-1-933988-62-7 already mentioned


// helper code
public static string TextBoxWithLabelFor<TModel, TProperty>(
this HtmlHelper<TModel> htmlHelper,
Expression<Func<TModel, TProperty>> expression,
string label)
where TModel : class
{
string labelHtml =
string textboxHtml = htmlHelper.TextBoxFor(expression);
return labelHtml + "&nbsp;" + textboxHtml;
string.Format("<label for=\"{0}\">{1}:</label>",
ExpressionHelper.GetInputName(expression),
label); // !!!!!! ENCODING ???????
string textboxHtml = htmlHelper.TextBoxFor(expression);
return labelHtml + "&nbsp;" + textboxHtml;
}
// usage
<%= Html.TextBoxWithLabelFor (c => c.MaxAttendees, "Max Attendees")


Raw string, outputed to HTML Plane without encoding.
Today "the label" is constant typed in the view,
tomorrow it can be metadata obtained from other source,
some next day a user input....

Writing API should be responsible for encoding.....no assumtions about clients....
CWE-116: Improper Encoding or Escaping of Output

System.Web.Mvc.TagBuilder, dangerous, undocumented, suboptimal, useless ?

http://msdn.microsoft.com/en-us/library/system.web.mvc.tagbuilder.aspx

System.Web.Mvc.TagBuilder

Do we (they) really need this class ?

Useless,"non
validating" Constructor

Accepts any characters and builds invalid tag name
for HTML,(X)HTML and XML
<<&>Is this valid 'tag name' ?</<&>

MergeAttribute producing invalid attr. names


Accepts any characters and builds invalid
or even injected attributes for HTML,(X)HTML and XML

<a Small="Small"
is:this:correct:name="" small="small" xss="..">...<a
xss="">Is this valid attribute name ?</a>

Of course it encodes attribute value, but...... (to be shown later) BORDER CONDITIONS: null
value is converted to "" duplicit call for same attribute name is ignored silently attr names are case sensitive regarding duplicity and sorting

What the tag builder
really is ?


return String.Format( CultureInfo.InvariantCulture, "<{0}{1}>{2}",
// not validated, not encoded
TagName,
// HttpUtility.HtmlAttributeEncode
// encoded values,
// not encoded names !!!
GetAttributesString(),
// raw InnerHtml supplied or
// HttpUtility.HtmlEncode(innerText)
// null translated to ""
InnerHtml);
Do we really need 140 lines of code
to achieve this ?

Comming soon: HttpUtility.HtmlAttributeEncode and HttpUtility.HtmlEncode
challanged

"Quality" of MSDN "API Documentation"

Comming back from java world to .net world
I have found the major problem with quality of "official documentation".
For .net I still consider MSDN as the main API docs (give me better if you have).
So here is sample, when I'm interested in TagBuilder.MergeAttribute method.


TagBuilder Class

http://msdn.microsoft.com/en-us/library/system.web.mvc.tagbuilder.aspx


NO PURPOSE
NO SHORT DESCRIPTION
NO METHOD LIST AVAILABLE (on the same page)

!!! EXTRA CLICK NEEDED !!!

http://msdn.microsoft.com/en-us/library/system.web.mvc.tagbuilder_members.aspx

!!! EXTRA CLICK NEEDED !!!
http://msdn.microsoft.com/en-us/library/dd505114.aspx


MergeAttribute(String, String)
Adds an attribute to the tag by using the specified key/value pair.
MergeAttribute(String, String, Boolean)
Adds an attribute to the tag by using the specified key/value pair.


SIGNATURES WITHOUT PARAMETER NAMES are quite useless (HARD TO GUES),
specially if you give the SAME DESCRIPTION to both.

!!! EXTRA CLICK NEEDED !!! (I always go for the longest signature first, second link)

http://msdn.microsoft.com/en-us/library/dd470785.aspx


replaceExisting
Type: System..::.Boolean
true to replace the existing attributes.

ok now after 3 click I know the difference between methods.

Lets see the first signature (String,String)

!!! EXTRA CLICK NEEDED (BACK) !!!
!!! EXTRA CLICK NEEDED (FIRST SIGNATURE) !!!

http://msdn.microsoft.com/en-us/library/dd460515.aspx


WHAT ABOUT BORDER CONDITIONS ?
ArgumentException Throws if the key parameter is null reference (Nothing in Visual Basic) or empty.

Ok but:

WHAT IF VALUE IS NULL ?
(exception or attr="" or not added at all or depends on attribute's (X)HTML semantics ?)
WHAT IF ATRIBUTE ALTREADY EXISTS ?
(exception or duplicated attribute or do nothing and silent exit ?)

This is only demo, this can be applied to 90% of APIs I;m browsing, not only this "fresh MVC".
In Sun's case, the newer the API, the better docs.

In MS case ?

Usually there is NONE or FEW lines on MANY MANY pages separated by MANY MANY CLICKS about:

WHY the API exist
WHAT the API does
HOW it does what is does
WHEN it fails and
WHAT it produces in BORDER CONDITIONS
DEFAULTS in inputs
THREAD SAFETY,INSTANCES and INSTANTIATION PATTERNS used by "container" ....

These are basics, so please HELP ME:
Where do you read this info ?

Anti-Sample of the day, bad sample variable, wasting time .....

This a sample from one of the MVC books I have been reading:


using System.Web.Mvc;
namespace ViewSamples.Controllers
{
public class ViewDataController : Controller
{
public ActionResult Index()
{
ViewData.Add("one", "onevalue");
ViewData.Add("two", "twovalue");
ViewData.Add("three","threevalue");
return View(3);
}
}
}

WTF the 3 stands for ? IS it somehow magically related to "three" or the collection size ?

Try matching signatures in msdn:

View(Object) Creates a ViewResult object using the model that renders a view to the response.

Then continue reading "the book":
Notice the values that are added in the controller in listing 4.6. We have three key/value
pairs and an object set to the Model property (by virtue of passing in “3” to the View
method).

Virtue ? "3" ?

Passing some reasonable OBJECT as Model insted of magical autoboxing small int with coincidental value 3, would be bit easier to understand....