Page MenuHomePhabricator

String warning appears on Special:Upload
Closed, DeclinedPublic

Description

At the top of the Special:Upload page, the following warning appears:

Warning: hash_equals(): Expected user_string to be a string, NULL given in /includes/GlobalFunctions.php

This problem was introduced in MediaWiki 1.23.12.

Event Timeline

Porplemontage raised the priority of this task from to Medium.
Porplemontage updated the task description. (Show Details)
Porplemontage added a subscriber: Porplemontage.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 27 2015, 8:54 PM
Reedy added a subscriber: Reedy.Dec 27 2015, 8:56 PM

Got a line number etc?

Also, what PHP version?

Oops, I missed copying the line number. It's 136.

Warning: hash_equals(): Expected user_string to be a string, NULL given in /includes/GlobalFunctions.php on line 136

PHP version 5.5.30

I found that the issue was caused by this change:

https://git.wikimedia.org/commitdiff/mediawiki%2Fcore.git/ad1fdc3b216daa3d3e88ecd47fcd7f604853da4b

Undo it, and the warning goes away.

Reedy set Security to None.Dec 27 2015, 9:41 PM
Reedy added a subscriber: Tgr.
Reedy added a comment.Dec 27 2015, 9:43 PM

Cheers for the debugging

So $val is null...

Reedy added a comment.Dec 27 2015, 9:45 PM

Wonder if other versions are affected too

Tgr added a comment.Dec 28 2015, 4:11 AM

@Porplemontage do you have a stack trace for that?

I do not. $wgShowExceptionDetails didn't add anything, I just made the change and it worked.

Probably needs some debugging hacking in; warnings won't show stack traces (though it can be done with a specific debug log, probably only in newer MW versions)

Check if the string is null, if it is, print a backtrace?

Tgr added a comment.Jan 1 2016, 5:43 AM

Yeah, non-exception stack traces were added in 1.25 (I8764cf5df87b226813c9b9cf99f9b4f3fa4b7c92). You'd have to get the stack trace manually.

Tkinkhorst added a subscriber: Tkinkhorst.EditedJan 2 2016, 5:19 PM

I can confirm seeing this on 1.23.12 and 1.23.13 although it's a different file and line number than reported above:

Warning: hash_equals(): Expected user_string to be a string, null given in [...]/includes/User.php on line 3810

It shows when browsing to the Special:Upload page (as a logged in user).

Let me know if I can provide more information.

Tgr added a comment.Jan 5 2016, 7:40 AM

@Tkinkhorst: could you make it throw an exception instead of the warning (add something like ıf ($val===null) throw new Exception('null!'); before the line reported in the warning) and copy the stack trace?

Unexpected non-MediaWiki exception encountered, of type "Exception"
[cacd8a6c] /wiki/Special:Upload Exception from line 3813 of [...]w/includes/User.php: null user val
Backtrace:
#0 [...]w/includes/specials/SpecialUpload.php(125): User->matchEditToken(NULL)
#1 [...]w/includes/specials/SpecialUpload.php(170): SpecialUpload->loadRequest()
#2 [...]w/includes/specialpage/SpecialPage.php(379): SpecialUpload->execute(NULL)
#3 [...]w/includes/specialpage/SpecialPageFactory.php(503): SpecialPage->run(NULL)
#4 [...]w/includes/Wiki.php(279): SpecialPageFactory::executePath(Title, RequestContext)
#5 [...]w/includes/Wiki.php(647): MediaWiki->performRequest()
#6 [...]w/includes/Wiki.php(506): MediaWiki->main()
#7 [...]w/index.php(46): MediaWiki->run()
#8 {main}

I believe the trigger is this line in includes/specials/SpecialUpload's loadRequest() function:

$this->mTokenOk = $this->getUser()->matchEditToken( $token );

This causes the user's posted $token to be verified on every pageload, even if the user hasn't posted a token (because they're just trying to view the page, not submit something). That causes null to be passed to User::matchEditToken(), which passes null to hash_equals(), which issues a PHP warning because of the bad type of the argument.

I suggest that explicit null checks should be added to these functions in includes/User:

		public function matchEditToken( $val, $salt = '', $request = null ) {
			if ($val === null) {
				return false;
			}
	
			$sessionToken = $this->getEditToken( $salt, $request );
			$equals = hash_equals( $sessionToken, $val );
			if ( !$equals ) {
				wfDebug( "User::matchEditToken: broken session data\n" );
			}
			return $equals;
		}
	
		public function matchEditTokenNoSuffix( $val, $salt = '', $request = null ) {
			if ($val === null) {
				return false;
			}
	
			$sessionToken = $this->getEditToken( $salt, $request );
			return hash_equals( substr( $sessionToken, 0, 32 ), substr( $val, 0, 32 ) );
		}

This solves it for me on 1.23.13

should this be fixed in: 1.23.14 ?

Reedy added a comment.May 24 2016, 6:59 PM

should this be fixed in: 1.23.14 ?

No

Krinkle closed this task as Declined.May 11 2017, 9:47 PM
Krinkle added a subscriber: Krinkle.

Declining per @Reedy's comment. This is a minor debug warning that causes no functional problems. The problem itself has been resolved in 1.25 and later (including the currently supported non-legacy release lines 1.27, 1.28 and 1.29).