Page MenuHomePhabricator

DiscussionTools reply tool needs double-posting protection
Closed, ResolvedPublicBUG REPORT

Description

Behavior

  1. Post a reply to some comment
  2. Break internet connection after the server received the request, but before the browser received the response...this results in a warning dialog, and the Reply Tool's comment form remains open
  3. Try to post the reply again (in my case I changed the wording slightly, but I doubt that matters), do not interfere with the connection this time

Expected

  1. ✅ You get an error saying you have already posted the comment (even better would be if your previous comment gets replaced, but that seems unnecessarily complex)

Actual

  1. ❗️In Chrome 91 on Ubuntu: the comment gets posted twice

Approaches

Ways in which the `Expected behavior could be achieved.

  • Some kind of nonce could be used to prevent this situation.

Open questions

  • 1. What – if any – existing edit functionality behaves similar to what is described in **Expected** above.

Done

  • All ===Open questions are answered
  • An approach within the ===Approaches section is decided upon and implement

Event Timeline

Next step

  • Editing Engineering to investigate and document here whether there is existing edit functionality that handles the scenario this ticket is describing.

The reply tool API is different from most editing processes in that it fetches the latest version of the page and appends content. This is the technique by which we have all but eliminated edit conflicts.

As other edit processes usually send the entire document I don't think there will be any existing code to guard against this as it would just be handled as an edit conflict.

The reply tool API is different from most editing processes in that it fetches the latest version of the page and appends content. This is the technique by which we have all but eliminated edit conflicts.

As other edit processes usually send the entire document I don't think there will be any existing code to guard against this as it would just be handled as an edit conflict.

Noted. @Esanders, considering [it sounds like] we'll need to write something from scratch to implement what this task is describing, how big/complex might something like this be to implement? Are we talking on the order of hours? Days? Week(s)?

Also: if there are open questions we need to address before being able to answer the above, can you please document them here?

Are we talking on the order of hours? Days? Week(s)?

It depends on what infrastructure is available - we'll need to do further investigation, but I doubt it would be as long as weeks.

I've verified the issue also exists in Flow, so there's no solution there for us (and possibly an indicator that this isn't a very common occurrence)

and possibly an indicator that this isn't a very common occurrence

Or an indicator of the not very common occurrence of Flow, especially in heated discussions. It’s enabled only on a few wikis, and even less have it as opt-out on talk pages. And the most heated discussions probably happen in the project namespace, which doesn’t use Flow by default anywhere. I’ve just got a conflict in Flow the other day, but the browser tab has been open for hours before I started drafting my reply, not for minutes, as it could happen in a heated village pump discussion on a major Wikipedia.

You need an unreliable internet connection for it to happen. I think that's uncommon in the parts of the world where most of the current userbase lives but more common elsewhere.

I think it is fairly easy to fix (not sure if it's worth the added complexity): generate a random number, send it when trying to post the message, stash it into object cache with an expiry of an hour or so, if it's already there do not save. (The OAuth extension does this as a defense against replay attacks; there is some discussion in T106066#5973566 about which object cache is appropriate, although the kind of problems it caused for OAuth would not be relevant here.)

@Tgr That's a fundamental contract between MW and Memcached that I don't think we will depart from anytime soon. Non-cache use cases do not belong in Memcached. The use case of T106066 is what session store, echo store, main stash, db-replicated, or dedicated tables are for.

It sounds like this would apply to this use case too ("Non-cache use cases do not belong in Memcached").

(I was also thinking that this is basically a special case of an edit conflict, which we started discussing in T250295. We could simply dsplay a warning when we find any comments by yourself that weren't there when you started writing the reply.)

Change 731848 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Generate form tokens in the client to prevent double posting

https://gerrit.wikimedia.org/r/731848

As discussed offline, we think it's better to keep this logic separate from the edit conflict logic, as they are not quite the same and may be connected to different user experiences.

Change 731848 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Generate form tokens in the client to prevent double posting

https://gerrit.wikimedia.org/r/731848

matmarex moved this task from Code Review to QA on the Editing-team (Kanban Board) board.

I'm not sure if this is practically testable, feel free to move it over if you can't reproduce the scenario. I have tested it locally (by changing the API code to throw an exception after saving changes but before returning the response).

I looked at this and it works fine on both Chrome:

Screenshot 2021-11-01 at 17.58.15.png (714×1 px, 113 KB)

and Firefox:
Screenshot 2021-11-01 at 18.02.06.png (926×1 px, 146 KB)

Screenshot 2021-11-01 at 18.03.44.png (564×1 px, 70 KB)

  NODES
INTERN 2
Note 4
Project 9