asp.net mvc - Checking image mime, size etc in MVC

Question

I have found here a pretty good way of checking whether the file uploaded by the user is a picture or not, how ever I ran into problems when I tried implementing it.

Here is the class I have found to check files

public static class HttpPostedFileBaseExtensions
{
    public const int ImageMinimumBytes = 512;

    public static bool IsImage(this HttpPostedFileBase postedFile)
    {            
        //-------------------------------------------
        //  Check the image mime types
        //-------------------------------------------
        if (postedFile.ContentType.ToLower() != "image/jpg" &&
                    postedFile.ContentType.ToLower() != "image/jpeg" &&
                    postedFile.ContentType.ToLower() != "image/pjpeg" &&
                    postedFile.ContentType.ToLower() != "image/gif" &&
                    postedFile.ContentType.ToLower() != "image/x-png" &&
                    postedFile.ContentType.ToLower() != "image/png")
        {
            return false;
        }

        //-------------------------------------------
        //  Check the image extension
        //-------------------------------------------
        if (Path.GetExtension(postedFile.FileName).ToLower() != ".jpg"
            && Path.GetExtension(postedFile.FileName).ToLower() != ".png"
            && Path.GetExtension(postedFile.FileName).ToLower() != ".gif"
            && Path.GetExtension(postedFile.FileName).ToLower() != ".jpeg")
        {
            return false;
        }

        //-------------------------------------------
        //  Attempt to read the file and check the first bytes
        //-------------------------------------------
        try
        {
            if (!postedFile.InputStream.CanRead)
            {
                return false;
            }

            if (postedFile.ContentLength < ImageMinimumBytes)
            {
                return false;
            }

            byte[] buffer = new byte[512];
            postedFile.InputStream.Read(buffer, 0, 512);
            string content = System.Text.Encoding.UTF8.GetString(buffer);
            if (Regex.IsMatch(content, @"<script|<html|<head|<title|<body|<pre|<table|<as+href|<img|<plaintext|<cross-domain-policy",
                RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.Multiline))
            {
                return false;
            }
        }
        catch (Exception)
        {
            return false;
        }

        //-------------------------------------------
        //  Try to instantiate new Bitmap, if .NET will throw exception
        //  we can assume that it's not a valid image
        //-------------------------------------------

        try
        {
            using (var bitmap = new System.Drawing.Bitmap(postedFile.InputStream))
            {
            }
        }
        catch (Exception)
        {
            return false;
        }

        return true;
    }
}    

My profile class

public class Profile
{
    public int ProfileID { get; set; }
    [Required(ErrorMessage = "Please enter a profile name")]
    public string Name { get; set; }
    [Required(ErrorMessage = "Please enter a intro")]
    public string Intro { get; set; }
    [Required(ErrorMessage = "Please enter a description")]
    public string Description { get; set; }
    public decimal Rate { get; set; }
    public byte[] ImageData { get; set; }
    public string ImageMimeType { get; set; }
}    

My ProfileController after I have changed it. I added HttpPostedFileBase as an argument and also used this line, if (HttpPostedFileBaseExtensions.IsImage(file) == true) , which I thought would sort it out but did not do any difference.

[HttpPost]
    public ActionResult Edit(Profile profile, HttpPostedFileBase file)
    {
        if (HttpPostedFileBaseExtensions.IsImage(file) == true)
            {
                if (ModelState.IsValid)
                    {               

                        repository.SaveProfile(profile);
                        TempData["message"] = string.Format("{0} has been saved", profile.Name);
                        return RedirectToAction("List");
                    }
                 else
                    {
                        // there is something wrong with the data values
                        return View(profile);
                    }
            }
        else
        {
            return View(ViewBag);
        }            
    }

And finally the SaveProfile method from the repository.

public void SaveProfile(Profile profile)
    {
        Profile dbEntry = context.Profiles.Find(profile.ProfileID);                        
        if (profile.ProfileID == 0)
        {
            context.Profiles.Add(profile);
        }
        else
        {
            if (dbEntry != null)
            {                    
                    dbEntry.Name = profile.Name;
                    dbEntry.Rate = profile.Rate;
                    dbEntry.Intro = profile.Intro;
                    dbEntry.Description = profile.Description;
                if (profile.ImageData != null)
                {

                    dbEntry.ImageData = profile.ImageData;
                    dbEntry.ImageMimeType = profile.ImageMimeType;
                }                                                               
            }
        }
        context.SaveChanges();
    }   

I also tried to edit the SaveProfile method, but could not implement all functions as in the class and would rather just have that separate and use it as it is. Any ideas where did I go wrong?

Answer 1

You've got a number of issues, some major, some minor.

First and foremost, you're using the extension method wrong. The whole point of adding an extension is that it becomes a method off an instance of that type. The this keyword param, is implicit, and is filled by the back-referencing the object the method is called from, not passed explicitly. In other words, what you should have for your conditional is:

if (file.IsImage())
{
    ...

Notice also that there's no comparison with true. While there's nothing wrong with that, it's completely unnecessary, you already have a boolean.

Second, while placing this conditional around the rest of the code should be effective to prevent the object being saved, it provides no guidance to the user. Instead, you should do something like:

if (!file.IsImage())
{
    ModelState.AddModelError("file", "Upload must be an image");
}

if (ModelState.IsValid)
{
    ...

By adding an error to ModelState, not only do you cause IsValid to be false, but now an actual error message will be presented to the user when the form is returned again.

Third, by attempting to select an existing profile instance from the database, you'll either get that instance or null. Therefore, you don't need to check whether ProfileId is 0, which is a pretty fragile check anyways (a user could merely change the hidden field's value to something else to modify an existing item). Instead, just do:

    var dbEntry = context.Profiles.Find(profile.ProfileID);                        
    if (dbEntry == null)
    {
        // add profile
    }
    else
    {
        // update profile
    }

Fifth, you're never doing anything with file. At some point, you should be doing something like:

var binaryReader = new BinaryReader(file.InputStream);
dbEntry.ImageData = binaryReader.ReadBytes(file.InputStream.Length);
dbEntry.ImageMimeType = file.ContentType;

Finally, and this is more stylistic than anything, but excessive use of else blocks that are unnecessary makes your code harder to read. You can simply let the error case fall through. For example:

if (!file.IsImage())
{
    ModelState.AddModelError("file", "Upload must be an image");
}

if (ModelState.IsValid)
{
    // save profile and redirect
}

return View(profile);

The first conditional will either add an error or not to ModelState. Then, in the second conditional, the code will only run if there's no errors, then it returns, so you never hit the final return View(profile). However, if there's any validation errors, you just fall through to that final return. No else is necessary, and the code is much more concise and readable.