morebits: fix handling of invalid token errors #682
Conversation
siddharthvp
force-pushed
the
morebits-token-errors
branch
from
3a35939
to
efab77d
Compare
|
I can confirm that this is working (at least for the |
|
|
||
| ctx.statusElement.info('Edit token is invalid, retrying'); | ||
| --Morebits.wiki.numberOfActionsLeft; // allow for normal completion if retry succeeds | ||
| if (fnCanUseMwUserToken('edit')) { | ||
| this.load(fnAutoSave, ctx.onSaveFailure); // try the append or prepend again | ||
| } else { |
There was a problem hiding this comment.
I'm not entirely clear on this bit. fnCanUseMwUserToken is weird and not very well named but wasn't the intent here to merely retry the pre/append again? I don't see how that would help with badtoken issues, so changing it makes sense, I'm just a little confused on the path where the suggestion fully replaces the (assumed) previous behavior.
There was a problem hiding this comment.
I don't understand your comment, or for that matter, the usage of fnCanUseMwUserToken, but I think this change here is clear.
The call this.load requests a new copy of the latest stored revision from the server. Its callback fnAutoSave just calls this.save which constructs the saveApi query and posts the api request. But all this was unnecessary. We already had a copy of the latest stored revision, we had already constructed the saveApi query object, before encountering this error. Now, since this is a token error, the only change that needs to be made to the saveApi.query is object to update the token, and just repost it. I don't see how it makes a difference whether we are prepending/appending/normal editing since the final step - creating saveApi.query and posting it - is the same across all of them.
So, this change is an optimisation that does away with the repeated execution of this.load, an unnecessary api call, and repeated execution of the this.save.
Do I make sense? Or is there a fallacious gap in my reasoning?
There was a problem hiding this comment.
No, that matches with what I can reason (doesn't mean we're not both wrong, I suppose). My concern with this.load -> saveApi would be handling any intervening edits/etc. to the page, but I can't seem to trigger that?
What I was trying to ask is basically what you yourself seem confused by, so at least we're on the same page. Treating prepend/append differently makes sense, but the whole point AFAICT is to avoid loading the page unnecessarily, so I likewise am unsure what exactly fnCanUseMwUserToken was really doing here. Something to do with redirect resolution, perhaps?
There was a problem hiding this comment.
this.load -> saveApi would be handling any intervening edits/etc.
Yes. But if there were intermediate edits to the page, they will be handled anyway via edit conflict resolution mechanism of the next attempt.
Something to do with redirect resolution, perhaps?
No, since if editmode is prepend/append, redirect resolution is handled by the api server itself. (according to the comment inside fnCanUseMwUserToken). In this.append & this.prepend, the only way if (fnCanUseMwUserToken('edit')) can be true is if we need to fetch protection status. But it would already have been fetched (and the sysop would have had went past the confirmation) before we encountered the error. In the present setup, it looks as if sysops need to confirm the edit to a protected page a second time if the original attempt failed due to a token error. I don't think that is desirable.
|
I'm sure this will be appreciated!🏅 These might be rare but they've got to be one of the more confusing errors for the average user. |
|
Oh, I should add that, for testing purposes, logging in and out should invalidate these tokens, which should be somewhat easier than just waiting. Going put my 2FA through the ringer I guess... |
If `fnGetToken` fails, then the resulting api call will probably give an errorCode `notoken`, hence that is added into the condition.
siddharthvp
force-pushed
the
morebits-token-errors
branch
from
efab77d
to
e13ea42
Compare
Amorymeltzer
dismissed
their stale review
Requested changes made, will test further before approval
The `badtoken` error ("Invalid CSRF token") is commonly encountered per
https://en.wikipedia.org/wiki/Wikipedia_talk:Twinkle/Archive_40#Twinkle_broken?
but was not being handled till now. Rather `notoken` (unset token parameter) was
being handled though it most probably never occurs as we do explicitly set the token
parameter.
Also simplified the way the error is handled - just retry the save rather than go
over the loading again.
siddharthvp
force-pushed
the
morebits-token-errors
branch
2 times, most recently
from
77e50c8
to
f5acd1b
Compare
|
The force push removes the |
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
There is no need to use "async: false" in both the places where this was being used till now. The latter was added by me in wikimedia-gadgets#682; don't know what I was smoking when I wrote that. "async: false" has been deprecated in jQuery since version 1.8. Its usage is not compatible with the use of $.Deferred() in $.ajax(), and thus do not work with the new promise-based Morebits.wiki.api class. Synchronous XMLHttpRequests are anyway bad on performance and should not be used anywhere.
Improve handling of token errors in
morebits.wiki.page'ssave,deletePageandundeletePage.I believe the 3rd commit fixes the probably decade-old bug (oh, again!) described here.
Though I am not sure whether these changes really work, as there's no easy way to test them.