3

First consider the following code snippet generated by ASP.NET Core MVC scaffolding.

// GET: Students/Delete/5
public async Task<IActionResult> Delete(int? id)
{
    if (id == null)
    {
        return NotFound();
    }

    var student = await _context.Students
        .SingleOrDefaultAsync(m => m.ID == id);

    if (student == null)
    {
        return NotFound();
    }

    return View(student);
}

// POST: Students/Delete/5
[HttpPost, ActionName("Delete")]
[ValidateAntiForgeryToken]
public async Task<IActionResult> DeleteConfirmed(int id)
{
    var student = await _context.Students.SingleOrDefaultAsync(m => m.ID == id);
    _context.Students.Remove(student);
    await _context.SaveChangesAsync();
    return RedirectToAction("Index");
}

There are some differences in HttpGet and HttpPost action methods as follows:

  • id is nullable in Get but not nullable in Post.
  • The preliminary check as follows is only in Get.

Code:

    if (id == null)
    {
        return NotFound();
    }

    var student = await _context.Students
                .SingleOrDefaultAsync(m => m.ID == id);

    if (student == null)
    {
       return NotFound();
    }

Questions:

For example, the visitor request id=5 to be deleted in GET but later he tampers with the id in POST by setting it to a number such as id=6 or setting it to an invalid value such as id=xodsfsdofsdfosdfsd. As there is no preliminary checks in HttpPost, how to prevent this?

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Second Person Shooter
  • 14,188
  • 21
  • 90
  • 165
  • Actually I also want to know the reason why the scaffolding does not provide the preliminary check in the POST by default. Do you know? – Second Person Shooter Apr 22 '17 at 20:52
  • store the values in cookies before its loaded into the form elements and validate it with the posted value. if both are same then proceed ahead else throw an error. – Rudresha Parameshappa Apr 22 '17 at 20:55
  • 3
    The user deleting student _may or may not_ have authorization to delete the student. That's up to your application to decide. You probably want to **add a check into your POST action to validate the user** because **you cannot prevent tampering with the value**. That's not something a scaffolding tool can decide for you. – Jasen Apr 22 '17 at 21:00
  • @Jasen: It makes sense. – Second Person Shooter Apr 22 '17 at 21:06
  • 2
    [What should every programmer know about security](http://stackoverflow.com/questions/2794016/what-should-every-programmer-know-about-security) – Jasen Apr 22 '17 at 21:07
  • you will need some kind of authorization method, like the simple membership to verify if the user is allowed to delete that is, for the invalid vales part, the string should cause a exception – subkonstrukt Apr 22 '17 at 21:10

3 Answers3

3

You probably want to add a check into your POST action to validate the user because you cannot prevent tampering with the value.

The user deleting student may or may not have authorization to delete the student. That's up to your application to decide. That's not something a scaffolding tool can decide for you.

[HttpPost, ActionName("Delete")]
[ValidateAntiForgeryToken]
public async Task<IActionResult> DeleteConfirmed(int id)
{
    var authorized = ValidateStudentDeletion(User, studentId: id);

    if (authorized)
    {
        // delete student
        ...
        return RedirectToAction("Index");
    }
}
Community
  • 1
  • 1
Jasen
  • 14,030
  • 3
  • 51
  • 68
2

I have been dealing with this issue for a long time and after reviewing a number of articles, here is my conclusion:

Let's assume that the sent id is taken from a checkbox or a dropdown list. Now if the user changes the value, what would happen:

If you want to modify the database, the first thing you need to check it is whether the data is associated with the user or not. If not, here comes other scenarios:

  1. The user intentionally had tampered the id. Well, if that's the case, the user account must be blocked but how? Check No. 2:

  2. The user has logged out and logged in again but with a different account in a different browser tab. Now, the page that sends the request has the previous-logged-in account information.

Basically, account information is retrieved from authentication cookies and if a user opens a tab and logs out and logins in with a different account, you need to find a way to figure out.

Here are some suggestions for you:

  • Use SignalR or other means to refresh all browser tabs if the user logs out. That's what most well-known companies practice suck as Google or Facebook.

  • If you want to make it easier, hash the username or whatever data you save in authentication cookie and save it in a hidden field in your HTML code. Then compare it with the same hashed value of the cookie that is retrieved from the request. If they do conflict, block the user from taking any action and refresh the page.

What I do normally is saving these conflicts in the database and if it exceeds a limits, I realize the user is junk!

Amir H. Bagheri
  • 1,416
  • 1
  • 9
  • 17
1

Well, here is my recommendations.

1- Data Access Authorization (this what you need) Before deleting the user with EF context, you need to validate that the logged in user really owns the student record. For instance, you need to validate that the student object returned has a user id (or other name) identical to logged in user id before choosing to remove the record. This will forbid existing logged-in applications users from tampering data of other existing users. Usually, each record must have its own owner foreign key db field.

2- Use of GUID: Never share your primary key student id this way. Instead, create a StudentGUID db field with uniqueidentifier type. This is what you need to share with the browser. This will forbid bad users from running an automated client code against your server with auto increment integers.

3- Session Authorization: If you are using asp.net Forms authentication, then its easy to add the [Authorize] attribute to mvc actions you need to secure. This will make sure only logged in users can execute your mvc action code.

Houssam Hamdan
  • 888
  • 6
  • 15