5

I am hoping for some feedback about the method I intend to use for preventing duplicate records in an ASP.NET MVC 4 application, and the knock on effects I have not though of for the users experience.

The web form has six input fields and a save button (as well as a cancel button), Users can take up to 10 minutes filling in the form.

Once the fields are submitted via a post the page is redirected on success / failure to a different page, after the data is recorded in a database table using a new Guid as the primary key.

To stop the users pressing the save button many times, but allowing the browser to repost the request on a closed connection I intend to provide the Guid for the new records primary key as a hidden input field when the form is rendered.

If the repost happens, or the user presses save multiple times, the database server will reject the second post of the record because of a duplicate key, which I can check for and cope with server side.

But does this create bigger problems for me?

Old fart
  • 590
  • 1
  • 4
  • 12
  • What is the problem? – U r s u s Jan 13 '15 at 17:13
  • The only time there is a problem with putting the Primary Key Id in a hidden field is that users can go in that field and change the primary key and update every record in the databse. now since they are Guid's it will be nearly impossible to guess Guid's that have already ben used and go in an update over previous user's forms , if you were using an integer then I would so it is a bad idea – Scott Selby Jan 13 '15 at 17:17
  • HAve you used Bootstrap? Because it automatically disabvles the button once clicked and if not use jquery or some loader or hide the button once clicked untill u receive response for the post request' – Tushar Gupta Jan 13 '15 at 17:17
  • Currently, without this new records have been repeated up to seven times when customers networks are busy and the users expect a faster response. – Old fart Jan 13 '15 at 17:17
  • I started disabling the save button, but from time to time it left the user without any way forward. The form was stuck, the server hadn't got the post, and they have to retype everything. The problem is that the browser never knows if the server didn't get the POST or that the browser didn't get the response. – Old fart Jan 13 '15 at 17:22
  • Possible duplicate of [How do I prevent multiple form submission in .NET MVC without using Javascript?](https://stackoverflow.com/questions/4250604/how-do-i-prevent-multiple-form-submission-in-net-mvc-without-using-javascript) – KyleMit Sep 19 '17 at 12:34

6 Answers6

7

If you make use of a hidden anti-forgery token in your form (as you should), you can cache the anti-forgery token on first submit and remove the token from cache if required, or expire the cached entry after set amount of time.

You will then be able to check with each request against the cache whether the specific form has been submitted and reject it if it has.

You don't need to generate your own GUID as this is already being done when generating the anti-forgery token.

UPDATE

When designing your solution, please keep in mind that each request will be processed asynchronously in its own separate thread, or perhaps even on entirely different servers / app instances.

As such, it is entirely possible that multiple requests (threads) can be processed even before the first cache entry is made. To get around this, implement the cache as a queue. On each submit(post request), write the machine name / id and thread id to the cache, along with the anti-forgery token... delay for a couple of milliseconds, and then check whether the oldest entry in cache/queue for that anti-forgery token corresponds.

In addition, all running instances must be able to access the cache (shared cache).

CShark
  • 2,183
  • 1
  • 24
  • 42
3

You can prevent the double click from the client side using some jQuery, if you'd like.

In your HTML, have your submit button be something like this:

<a id="submit-button"> Submit Form </a> <span id="working-message"></span>

In JavaScript (jQuery):

$('#submit-button').click(function() {
  $(this).hide();
  $('#working-message').html('Working on that...');
  $.post('/someurl', formData, function(data) {
    //success 
    //redirect to all done page
  }).fail(function(xhr) {
    $('#submit-button').show();
    $('#working-message').html('Submit failed, try again?');
  });
}); // end on click

This will hide the button before it even tries to submit, so the user can't click twice. This also shows progress, and on failure, allows them to resubmit. You may want to think about adding a timeout to my above code.

Another alternative is to use jquery to grab the form $('#form-id').submit(), but you wouldn't be able to track the progress as easily like the ajax call I've done.

EDIT: I would still recommend looking at ways to prevent double submission from a server-side stand point, just for security reasons.

KyleMit
  • 30,350
  • 66
  • 462
  • 664
Porschiey
  • 1,981
  • 2
  • 17
  • 25
  • Interesting thank you, I hadn't looked at ajax because I am using the ASP.NET MVC 4 pattern and was hoping to keep the MVC 4, increasingly seeing that it is just better to walk away from MVC4. Still hoping for an MVC4 answer – Old fart Jan 13 '15 at 17:36
  • Hey no problem! :) I use MVC4 and MVC5 extensively in my day to day, but I will almost always use this pattern inside my MVC app for form submission. It allows for a lot more flexibility, and I can always use other MVC options to validate the data "server side." Take a look at `JsonResult`: you can create actions in your MVC application that are catered to these kinds of jQuery / ajax calls. – Porschiey Jan 13 '15 at 17:40
2

This is actually quite a pervasive issue in MVC (and probably other web frameworks), so I'm going to explain it a bit and then offer a solution.

The Problem

Suppose you're on a webpage with a form. You click submit. The server's taking a while to respond, so you click it again. And again. At this point you've fired off three separate requests, all of which the server is going to handle simultaneously. But only one response will be executed in the browser - the first one.

This situation can be represented by the following line chart.

          ┌────────────────────┐
Request 1 │                    │  Response 1: completes, browser executes response
          └────────────────────┘
            ┌────────────────┐
Request 2   │                │  Response 2: also completes!
            └────────────────┘
              ┌───────────────────┐
Request 3     │                   │  Response 3: also completes!
              └───────────────────┘

The horizontal axis represents time (not to scale). In other words, the three requests are fired off in order, but only the first response is returned to the browser; the others are discarded.

This is a problem. Not always, but often enough to be annoying, such requests have side-effects. These side-effects could vary from a counter incrementing, duplicate records being created, or even a credit card payment being processed multiple times.

The Solution

Now, in MVC, most POST requests (especially ones with side effects) should be using the built-in AntiForgeryToken logic to generate and validate a random token for each form. The following solution takes advantage of this.

The plan: we discard all duplicate requests. The logic here goes: cache the token from each request. If it's already in the cache, then return some dummy redirect response, perhaps with an error message.

In terms of our line chart, this looks like...

          ┌────────────────────┐
Request 1 │                    │  Response 1: completes, browser executes the response [*]
          └────────────────────┘
            ┌───┐
Request 2   │ x │  Response 2: rejected by overwriting the response with a redirect
            └───┘
              ┌───┐
Request 3     │ x │  Response 3: rejected by overwriting the response with a redirect
              └───┘

[*] the browser executes the wrong response, because it's already been replaced by requests 2 and 3.

Notice a few things here: because we're not handling duplicate requests at all, they execute their result quickly. Too quickly - they actually replace the response of the first request by getting in first.

Because we didn't actually process these duplicate requests, we don't know where to redirect the browser. If we use a dummy redirect (like /SameController/Index) then when the first response returns to the browser, it'll execute that redirect instead of whatever it was supposed to do. This leaves the user oblivious as to whether or not their request actually completed successfully, since the result of the first request is lost.

Clearly this is less than ideal.

So, our modified plan: cache not only the tokens for each request, but the response too. That way, instead of assigning an arbitrary redirect to the duplicate requests, we can assign the response that's actually supposed to be returned to the browser.

Here's how that looks in code, using a filter attribute.

/// <summary>
/// When applied to a controller or action method, this attribute checks if a POST request with a matching
/// AntiForgeryToken has already been submitted recently (in the last minute), and redirects the request if so.
/// If no AntiForgeryToken was included in the request, this filter does nothing.
/// </summary>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
public class PreventDuplicateRequestsAttribute : ActionFilterAttribute {
    /// <summary>
    /// The number of minutes that the results of POST requests will be kept in cache.
    /// </summary>
    private const int MinutesInCache = 1;

    /// <summary>
    /// Checks the cache for an existing __RequestVerificationToken, and updates the result object for duplicate requests.
    /// Executes for every request.
    /// </summary>
    public override void OnActionExecuting(ActionExecutingContext filterContext) {
        base.OnActionExecuting(filterContext);

        // Check if this request has already been performed recently
        string token = filterContext?.HttpContext?.Request?.Form["__RequestVerificationToken"];
        if (!string.IsNullOrEmpty(token)) {
            var cache = filterContext.HttpContext.Cache[token];
            if (cache != null) {
                // Optionally, assign an error message to discourage users from clicking submit multiple times (retrieve in the view using TempData["ErrorMessage"])
                filterContext.Controller.TempData["ErrorMessage"] =
                    "Duplicate request detected. Please don't mash the submit buttons, they're fragile.";

                if (cache is ActionResult actionResult) {
                    filterContext.Result = actionResult;
                } else {
                    // Provide a fallback in case the actual result is unavailable (redirects to controller index, assuming default routing behaviour)
                    string controller = filterContext.ActionDescriptor.ControllerDescriptor.ControllerName;
                    filterContext.Result = new RedirectResult("~/" + controller);
                } 
            } else {
                // Put the token in the cache, along with an arbitrary value (here, a timestamp)
                filterContext.HttpContext.Cache.Add(token, DateTime.UtcNow.ToString("s"),
                    null, Cache.NoAbsoluteExpiration, new TimeSpan(0, MinutesInCache, 0), CacheItemPriority.Default, null);
            }
        }
    }

    /// <summary>
    /// Adds the result of a completed request to the cache.
    /// Executes only for the first completed request.
    /// </summary>
    public override void OnActionExecuted(ActionExecutedContext filterContext) {
        base.OnActionExecuted(filterContext);

        string token = filterContext?.HttpContext?.Request?.Form["__RequestVerificationToken"];
        if (!string.IsNullOrEmpty(token)) {
            // Cache the result of this request - this is the one we want!
            filterContext.HttpContext.Cache.Insert(token, filterContext.Result,
                null, Cache.NoAbsoluteExpiration, new TimeSpan(0, MinutesInCache, 0), CacheItemPriority.Default, null);
        }
    }
}

To use this attribute, simply stick it on a method alongside [HttpPost] and [ValidateAntiForgeryToken]:

[HttpPost]
[ValidateAntiForgeryToken]
[PreventDuplicateRequests]
public ActionResult MySubmitMethod() {
    // Do stuff here
    return RedirectToAction("MySuccessPage");
}

...and spam those submit buttons all you like. I've been using this on several action methods and had no issues so far - and no more duplicate records, no matter how much I spam the submit button.

If anyone has any more accurate descriptions of how MVC actually processes requests (as this was written purely from observation and stack traces), be my guest, and I'll update this answer accordingly.

Finally, thanks to @CShark, whose suggestions I used as the basis for my solution.

Extragorey
  • 1,654
  • 16
  • 30
  • I like this as it returns the user to a sensible place. In our case we have many front end servers which operate queues, any request can arrive in any order because the user or load balancer has resubmitted a request and each front end server has a queue. So Caching values has holes in it, only the database (single point of write) can decide if the record has been submitted but the PK exception can be picked up and then used with this concept. – Old fart Dec 15 '20 at 09:57
  • This answer is great. I suggest changing the way `If`s are made so you can make your code simpler. You can also add new properties so you can choose where you want the user to be redirected. – EduLopez Apr 17 '22 at 15:33
  • 1
    @EduLopez True, when I wrote this solution I tended to use the pattern `if (whatWeWant) { [code] } else { [fallback] }` whereas now I favour the guard pattern to reduce indenting: `if (!whatWeWant) { [fallback] return } [code]`. Swift has changed my perspective. ;) – Extragorey Apr 19 '22 at 00:12
  • Nice one, dude. Why not create an open source library and a NuGet package for this? It solves a very common problem. – CShark Jul 06 '22 at 14:23
1

Sometimes, deal with it only on client side isn't enought. Try to gen a hash code of the form and save in cache (set an expiration date or something like that).

the algorithm is something like:

1- User made post

2- Generate hash of the post

3- Check the hash in cache

4- Post already on cache? Throw exception

5- Post isn't on cache? Save the new hash on cache and save post on database

A sample:

            //verify duplicate post
            var hash = Util.Security.GetMD5Hash(String.Format("{0}{1}", topicID, text));
            if (CachedData.VerifyDoublePost(hash, Context.Cache))
                throw new Util.Exceptions.ValidadeException("Alert! Double post detected.");

The cache function could be something like that:

    public static bool VerifyDoublePost(string Hash, System.Web.Caching.Cache cache)
    {
        string key = "Post_" + Hash;

        if (cache[key] == null)
        {
            cache.Insert(key, true, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
            return false;
        }
        else
        {
            return true;
        }
    }
Bruno Bastos
  • 786
  • 6
  • 6
0

As I understand, you are planning to keep the primary key in a hidden input when you are rendering the page initially. Obviously this is not a good idea. To start with, if you are using the Guid implementation in c#, it's string and having string as a primary key is not a good idea (See the answer for this SO question).

You can address this issue in two ways. First, Disable the button on the first click. Second, build validations in code behind without relying on the primary key.

Community
  • 1
  • 1
su8898
  • 1,703
  • 19
  • 23
  • you posted a link to a question that was about the performance of Guid vs int as a primary key clustered index - that has very very little to do with this questions , except the fact they both mention Guid – Scott Selby Jan 13 '15 at 17:27
  • Leaving aside the Guid PK performance issue which I am aware of and take on board. The security issue worried me too, but I have been unable to define a real issue with it, except for the person faking the entries do you have any links to help with this? – Old fart Jan 13 '15 at 17:31
  • Except for the performance issue and possibility of Guid tampering, there are no issues with this method. In fact you don't even need to keep the Guid as the PK. You can insert the Guid along with the record and check to see if a record with this Guid already exists just before inserting. – su8898 Jan 13 '15 at 17:43
-1

You can simply deal with it in the client side.

Create an overlay div, a css class with display none and a big z-index and a jquery script that shows that div when the user presses the submit button.

pdeane
  • 34
  • 5
  • From time to time it left the user without any way forward. The form was stuck, the server hadn't got the post, and they have to retype everything. The problem is that the browser never knows if the server didn't get the POST or that the browser didn't get the response. – Old fart Jan 13 '15 at 17:23
  • There's no need for a massive z-index if display is set to none. – Porschiey Jan 13 '15 at 17:42
  • @Porschiey Yes but eventually you will show the div and when you do, it should be on top – pdeane Jan 13 '15 at 17:48
  • @pdeane - sure, but you're talking about a scenario where you've got a lot of layers - which the OP hasn't indicated is the case, nor should be the standard. – Porschiey Jan 13 '15 at 20:42