Page MenuHomePhabricator

Unknown HTTP status code 200 OK in /srv/mediawiki/php-1.26wmf9/includes/libs/HttpStatus.php on line 100
Closed, ResolvedPublic

Description

From Fatalmonitor: Unknown HTTP status code 200 OK in /srv/mediawiki/php-1.26wmf9/includes/libs/HttpStatus.php on line 100

Unknown, really?

Event Timeline

mmodell created this task.Jun 10 2015, 6:19 PM
mmodell raised the priority of this task from to Needs Triage.
mmodell updated the task description. (Show Details)
mmodell added a subscriber: mmodell.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2015, 6:19 PM
hashar added a subscriber: hashar.EditedJun 11 2015, 11:00 AM

Might be due to the recent http status changes:

Krinkle added a comment.EditedJun 11 2015, 2:47 PM

https://gerrit.wikimedia.org/r/#/c/215032/6/includes/libs/HttpStatus.php adds:

* @param int $code Status code
*/
public static function header( $code ) {
..
trigger_error( "Unknown HTTP status code $code", E_USER_WARNING );

This method takes an integer.

@mmodell wrote

From Fatalmonitor: Unknown HTTP status code 200 OK in /srv/mediawiki/php-1.26wmf9/includes/libs/HttpStatus.php on line 100

This suggests something is calling it with the string "200 OK".

The relevant commits added no new uses of http status codes. I've walked through it twice to confirm. I'm reasonably certain that this is an existing problem we were unable to detect before because there was no error handling, this kind of scenario was previously silently ignored.

Probably from includes/AjaxResponse.php:89

$this->mResponseCode = '200 OK';

Change 217524 had a related patch set uploaded (by BryanDavis):
Fix non-numeric HTTP status code

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

Change 217527 had a related patch set uploaded (by Krinkle):
AjaxResponse: Fix broken logic for extracting HTTP status codes

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

The only occurrence of 200 OK in MediaWiki core is:

class AjaxResponse {
..
		$this->mResponseCode = '200 OK';
..
		if ( $this->mResponseCode ) {
			$n = preg_replace( '/^ *(\d+)/', '\1', $this->mResponseCode );
			HttpStatus::header( $n );
..
}

Which turns out, is a bad regex. It matches any and all whitespace at the start and has a match group for the first sequence of digits. And replaces that with just the digits match. This is essentially ltrim(). It doesn't account for the content after the first match.

php -a
> echo preg_replace( '/^ *(\d+)/', '\1', '200 OK' );
200 OK
Krinkle claimed this task.Jun 11 2015, 3:31 PM
Krinkle triaged this task as Normal priority.
Krinkle set Security to None.
Krinkle removed a subscriber: gerritbot.

Change 217524 abandoned by BryanDavis:
Fix non-numeric HTTP status codes in AjaxResponse

Reason:
Krinkle has a better commit message and a more explicit conversion in Iafff9982bbbee893c13f891901dde88f998db7a6

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

Change 217527 merged by jenkins-bot:
AjaxResponse: Fix broken logic for extracting HTTP status codes

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

I'd like to cherry pick this one to wmf9, it's spamming the logs

Change 217583 had a related patch set uploaded (by 20after4):
AjaxResponse: Fix broken logic for extracting HTTP status codes

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

Change 217583 merged by jenkins-bot:
AjaxResponse: Fix broken logic for extracting HTTP status codes

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

Restricted Application added a subscriber: Mjbmr. · View Herald TranscriptJun 13 2015, 12:17 AM
mmodell closed this task as Resolved.Jun 14 2015, 2:55 AM