r71721 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71720‎ | r71721 | r71722 >
Date:18:28, 26 August 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
When the CSRF token on is not available in the session, show the
'you have cookies disabled' message instead of "Session problem,
canceled to avoid session hijacking"
Modified paths:
  • /trunk/phase3/includes/specials/SpecialUserlogin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialUserlogin.php
——@@ -272,7 +272,7 @@
273273 # Request forgery checks.
274274 if ( !self::getCreateaccountToken() ) {
275275 self::setCreateaccountToken();
276 - $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 276+ $this->mainLoginForm( wfMsgExt( 'nocookiesnew', array( 'parseinline' ) ) );;
277277 return false;
278278 }
279279
——@@ -657,6 +657,8 @@
658658 break;
659659
660660 case self::NEED_TOKEN:
 661+ $this->mainLoginForm( wfMsgExt( 'nocookieslogin', array( 'parseinline' ) ) );
 662+ break;
661663 case self::WRONG_TOKEN:
662664 $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
663665 break;

Follow-up revisions

RevisionCommit summaryAuthorDate
r78613Aryeh was right in r71721....platonides22:40, 19 December 2010

Comments

#Comment by Simetrical (talk | contribs)   20:09, 27 August 2010

Why doesn't this have its own message? Message reuse is bad.

#Comment by Platonides (talk | contribs)   20:13, 27 August 2010

Those login messages mean "You have cookies disabled", but that check was being performed after login. The CSRF fix mean that we need cookies earlier and their code was never reached (yes, we should probably strip it).

It's just moving the position from which the message is called. I don't think a user would even notice that the step on which it is shown has changed.

#Comment by Simetrical (talk | contribs)   20:16, 27 August 2010

The error messages warn the user that they have cookies disabled, but is this the only reason you'd get no CSRF token? Wouldn't it be better to have a separate message saying that the CSRF check failed, and suggesting that the user might try enabling cookies in case that's the problem?

#Comment by Platonides (talk | contribs)   20:22, 27 August 2010

The message appears in the UI when there's no token stored at the session, which at that point should have been set.

They could have manually deleted their cookies, but that's also a cookie problem. Talking to the end users about CSRF at this point is gobleddyhook. All problems there should be cookie related. Note that if they have the token in session and it doesn't match then they get the different and more technical sessionfailure.

#Comment by Simetrical (talk | contribs)   21:09, 27 August 2010

They could also be the victims of CSRF.  :) Or there could be a bug in the token generation -- I once caused a bug that caused rollbacks to fail because I refactored the code that generated rollback URLs and mistakenly didn't include the token. Having distinct error messages for distinct errors is good practice, because it helps you track down errors more easily. Googling the messages will produce different advice on fixes, and it helps developers track things down more easily.

You don't have to add much gobbledegook, just enough to make it distinct.

#Comment by Simetrical (talk | contribs)   19:44, 31 December 2010

Fixed in r78613, marking ok.

Status & tagging log