Page MenuHomePhabricator

TypeError: Argument 1 passed to MediaWiki\CheckUser\ClientHints\ClientHintsData::newFromJsApi() must be of the type array, null given
Closed, ResolvedPublicPRODUCTION ERROR

Description

What is the problem?

I can get the client hints API endpoint (rest.php/checkuser/v0/useragent-clienthints/revision/) to return this error:

[exception] [30b80b4c154e277391656329] /bare/rest.php/checkuser/v0/useragent-clienthints/revision/6890   TypeError: Argument 1 passed to MediaWiki\CheckUser\ClientHints\ClientHintsData::newFromJsApi() must be of the type array, null given, called in /home/drw/wikimedia/srv/bare/extensions/CheckUser/src/Api/Rest/Handler/UserAgentClientHintsHandler.php on line 55
#0 /home/drw/wikimedia/srv/bare/extensions/CheckUser/src/Api/Rest/Handler/UserAgentClientHintsHandler.php(55): MediaWiki\CheckUser\ClientHints\ClientHintsData::newFromJsApi()
#1 /home/drw/wikimedia/srv/bare/includes/Rest/SimpleHandler.php(38): MediaWiki\CheckUser\Api\Rest\Handler\UserAgentClientHintsHandler->run()
#2 /home/drw/wikimedia/srv/bare/includes/Rest/Router.php(517): MediaWiki\Rest\SimpleHandler->execute()
#3 /home/drw/wikimedia/srv/bare/includes/Rest/Router.php(422): MediaWiki\Rest\Router->executeHandler()
#4 /home/drw/wikimedia/srv/bare/includes/Rest/EntryPoint.php(195): MediaWiki\Rest\Router->execute()
#5 /home/drw/wikimedia/srv/bare/includes/Rest/EntryPoint.php(135): MediaWiki\Rest\EntryPoint->execute()
#6 /home/drw/wikimedia/srv/bare/rest.php(31): MediaWiki\Rest\EntryPoint::main()
#7 {main}

I think it happens when you try to pass it something that isn't JSON.

Steps to reproduce problem
  1. Setup a wiki with CheckUser installed and client hints enabled (they are by default)
  2. Run this command (You may need to change the value of the server to point to your local wiki):
curl 'http://localhost:8080/w/rest.php/checkuser/v0/useragent-clienthints/revision/6890' -H 'Content-Type: application/x-www-form-urlencoded' --data-raw 'foobar'

I have tried other content types, but so far I have only been able to reproduce it with Content-Type: application/x-www-form-urlencoded.

Environment

Wiki(s): MediaWiki 1.41.0-alpha (e20dc2f) 14:13, 7 August 2023. CheckUser 2.5 (bfcb3ae) 04:46, 24 August 2023.

Event Timeline

This is because the UserAgentClientHintsHandler::getBodyValidator method isn't called if the content type is either application/x-www-form-urlencoded or mutlipart/form-data. This means that the UnsupportedContentTypeBodyValidator cannot be used as the method is never called and as such the "valid" data is used. When the data isn't valid (because it's a form data), the ::getValidatedBody method returns null. This was not accounted for.

The fix should just require checking that:

  • The validated body is an array. If not, then:
    • If the content type is a form, then return a 415 with the message rest-unsupported-content-type
    • Otherwise return some 4XX with an appropriate message (this should be accounted for by ::getBodyValidator, but it makes sense to explicitly check for it to prevent other issues that may not be seen at the moment)
Dreamy_Jazz changed the subtype of this task from "Bug Report" to "Production Error".

Re-producible on production wikis (Logstash single document). Impact is low as it would require a user sending a custom request to hit the server error.

Change 952187 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] clienthints: Handle non-array response from ::getValidatedBody

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

Thanks for spotting this. I have created a patch that fixes this by returning an appropriate 4XX error code instead of a 500.

It seems that many other uses of ::getValidatedBody also assume an array will be returned if the JSON validator is used. While outside the scope of this task, the other usages will need to be fixed as I've seen a few in codesearch that would also cause a 500 server error if also given "form data" via setting the content type to a form.

Code search link: https://codesearch.wmcloud.org/search/?q=getValidatedBody&files=&excludeFiles=&repos=

Considering that the ::getValidatedBody method will also return null in many other cases (more than just the Content-Type check in Validator.php line 174), the usages of this method should explicitly be checking for null and return some kind of 4XX error when the validated body is null.

Change 952187 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] clienthints: Handle non-array response from ::getValidatedBody

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

I can no longer reproduce the bug in the description.

Instead, the API returns:

{"errorKey":"rest-unsupported-content-type","messageTranslations":{"en":"Unsupported Content-Type: application/x-www-form-urlencoded"},"httpCode":415,"httpReason":"Unsupported Media Type"}

I attacked the client hints API using OWASP ZAP which, among other things, modifies the Content-Type of the request header. I did not see any 500 responses or exceptions in the logs.

Test environment: CheckUser 2.5 (9ed2b65) 19:12, 29 August 2023.