Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm pretty new to asp and c# and I have a couple of concerns with the security of the code I have written (shown below). Basically all it does is allow you to enter a quantity before you add the item to your cart using a razor form.

Every example I see uses validation on input fields, like:

[Required]
[DataType(DataType.EmailAddress)]

Is that sort of validation (or any other) necessary in this case, or is my form secure enough because the controller requires the input of the product id and the quantity, both of which are limited to integers? Also, is there anything else that I should be concerned about with this form, like Javascript injection?

Thanks for your help!

The view has a razor input form:

@using ( Html.BeginForm( "AddToCart", "Cart" ) )
{
    @Html.HiddenFor(_ => item .ProductID)
    @Html.TextBoxFor(_=> item.Quantity)
    <input type="submit" value="+ Add to cart" />
}

The form is posted to the AddToCart action of the CartController:

[HttpPost]
public ActionResult AddToCart(int id, int quantity)
{
    // Retriebe the product from the database
     var addedProduct = storeDB.Products
         .Single(product => product.ProductId == id);

    // Add it to the shopping cart
    var cart = ShoppingCart.GetCart(this.HttpContext);

    cart.AddToCart(addedProduct, quantity);

    //Go back to the main store page for more shopping
    return RedirectToAction("Index");
}

And here is the AddToCart method:

public void AddToCart(Product product, int quantity)
{
    //Get the matching cart and album instances
    var cartItem = storeDB.Carts.SingleOrDefault(
        c => c.CartId == ShoppingCartId
            && c.ProductId == product.ProductId);

    if (cartItem == null)
    {
        // Create a new cart item if no cart exists
        cartItem = new Cart
        {
            ProductId = product.ProductId,
            CartId = ShoppingCartId,
            Count = quantity,
            DateCreated = DateTime.Now
        };
        storeDB.Carts.Add(cartItem);
    }
    else
    {
        // If the item does exist in the car, 
        // than add one to the quantity
        var newCount = cartItem.Count + quantity;
        cartItem.Count = newCount;
    }
    // Save changes
    storeDB.SaveChanges();
}

Thanks again for your help!

share|improve this question
1  
What exactly do you mean by 'secure'? –  Trevor Pilley Nov 6 '12 at 22:59
    
I'm not that familiar with ShoppingCarts but don't they normally have some sort of timeout? In that case if the ShoppingCart timedout by the time this request happens what would happen to the line ShoppingCart.GetCart() ?? –  dreza Nov 8 '12 at 18:38
add comment

2 Answers

You can secure against CSRF (pronounced C-surf) by using ValidateAntiForgeryTokenAttribute and AntiForgeryToken method.

How CSRF (Cross Site Request Forgery) works:

We have 2 actors, an evil person E and a good person G. E makes an evil html image tag on some forum, that links to your website and puts a product in your user's cart.

If G has a cookie for your site and the cookie is not expired, when G goes to E's forum, the browser tries to load the html img and it will send a Request to your site. Even though G didn't intend to do that...

Implementation:

View:

@using (Html.BeginForm("AddToCart", "Cart"))
{
   @HtmlHelper.AntiForgeryToken()
   // Code -->
}

Controller:

[HttpPost][ValidateAntiForgeryToken]
public ActionResult AddToCart(int id, int quantity)
{
   // Code -->
}

How it prevents CSRF:

  1. In the view a hidden input field with a unique string is created.
  2. A cookie is sendt to the user with the same string. (I believe it is a httpOnly cookie).
  3. The controller checks if the form contains the same string as is in the cookie.

Because person E, cannot form a valid form post the cross site request will fail. But note that if you have XSS (Cross site scripting) vulnerability this security check can be circumvented.

And another thing... If your 2nd method public void AddToCart(Product product, int quantity) is not an action consider using the [NonAction] attribute, because all public methods in a controller are considered by default Action methods. Meaning it is invokable as an Action method.

share|improve this answer
    
I would go as far at the controller level to say, if it's not an action it should in general be private. –  dreza Nov 12 '12 at 5:15
add comment

Security is an important aspect of any application, so you must evaluate what you want to do for security.If this is a public portal then you need to make sure all query string value are safe and no problem with the input value , MVC also does care these things automatically but t to make sure you can double check this params.

like in this case you can make sure quantity cannot be zero or product ID cannot be zero.

It depends upon your requirement and your security model.

share|improve this answer
    
Checking that the quantity or product Id is not zero is validation, not security. –  Trevor Pilley Nov 8 '12 at 8:33
2  
validation is also a form of security... input type should be secured by means of validation. –  paritosh Nov 8 '12 at 9:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.