Unhandled exception when looking up a configuration objects by name which contains non-ASCII characters

jannakha → changed the visibility of the branch 3475540-unhandled-exception-ascii to hidden.

before I start writing tests - please review/comment/provide suggestions to the solution!
Thanks!

I don't believe this is a bug and I think it is the caller's responsibility to filter input. By that I mean that each use-case needs fixing separately.

What does this have to do with security?

Looks like a bug to me: https://dri.es/node/add/%D1%8B%D0%B2%D0%B0%D1%8B%D0%B2%D1%8B 🤖

Indeed but I would say that particular one is a node system bug. Any code that loads configuration objects should understand the rules: that their keys are ASCII only.

@cilefen
Re: "caller's responsibility to filter input" - it's a lookup into config database which doesn't validate non-ascii input, there are levels of abstractions between config object and node/webform/view/or other caller which will make it quite hard to validate it on per-caller basis.

* there's a case-by-case fix which doesn't fix much: https://www.drupal.org/project/drupal/issues/3457963 🐛 Validate view_name in ajaxView() Needs work

It was discovered during a pen test and flagged as a potential security issue since:
- This leads to unnecessary hits of database which can lead to server outage.
- The error message is not handled and white screen of death is displayed.

examples:
https://events.drupal.org/node/add/тест
https://www.cyber.gov.au/node/add/тест

Stack trace of node/add/{node_type} and each other config has its own trace:

#0 /web/core/lib/Drupal/Core/Database/Connection.php(883): Drupal\mysql\Driver\Database\mysql\ExceptionHandler->handleExecutionException(Object(PDOException), Object(Drupal\Core\Database\StatementWrapperIterator), Array, Array)
#1 /web/core/lib/Drupal/Core/Config/DatabaseStorage.php(119): Drupal\Core\Database\Connection->query('SELECT [name], ...', Array, Array)
#2 /web/core/lib/Drupal/Core/Config/CachedStorage.php(95): Drupal\Core\Config\DatabaseStorage->readMultiple(Array)
#3 /web/core/lib/Drupal/Core/Config/ConfigFactory.php(165): Drupal\Core\Config\CachedStorage->readMultiple(Array)
#4 /web/core/lib/Drupal/Core/Config/ConfigFactory.php(136): Drupal\Core\Config\ConfigFactory->doLoadMultiple(Array)
#5 /web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php(181): Drupal\Core\Config\ConfigFactory->loadMultiple(Array)
#6 /web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(312): Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple(Array)
#7 /web/core/lib/Drupal/Core/Entity/EntityRepository.php(183): Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array)
#8 /web/core/lib/Drupal/Core/Entity/EntityRepository.php(175): Drupal\Core\Entity\EntityRepository->getCanonicalMultiple('node_type', Array, Array)
#9 /web/core/lib/Drupal/Core/ParamConverter/EntityConverter.php(134): Drupal\Core\Entity\EntityRepository->getCanonical('node_type', '\xD0\xB4\xD1\x8B\xD0\xBB\xD0\xB2\xD0\xBE\xD0\xB0\xD0\xB4\xD1...', Array)
#10 /web/core/lib/Drupal/Core/ParamConverter/ParamConverterManager.php(100): Drupal\Core\ParamConverter\EntityConverter->convert('\xD0\xB4\xD1\x8B\xD0\xBB\xD0\xB2\xD0\xBE\xD0\xB0\xD0\xB4\xD1...', Array, 'node_type', Array)
#11 /web/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php(45): Drupal\Core\ParamConverter\ParamConverterManager->convert(Array)
#12 /web/core/lib/Drupal/Core/Routing/Router.php(270): Drupal\Core\Routing\Enhancer\ParamConversionEnhancer->enhance(Array, Object(Symfony\Component\HttpFoundation\Request))
#13 /web/core/lib/Drupal/Core/Routing/Router.php(150): Drupal\Core\Routing\Router->applyRouteEnhancers(Array, Object(Symfony\Component\HttpFoundation\Request))
#14 /web/core/lib/Drupal/Core/Routing/AccessAwareRouter.php(90): Drupal\Core\Routing\Router->matchRequest(Object(Symfony\Component\HttpFoundation\Request))
#15 /vendor/symfony/http-kernel/EventListener/RouterListener.php(105): Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object(Symfony\Component\HttpFoundation\Request))
#16 [internal function]: Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object(Symfony\Component\HttpKernel\Event\RequestEvent), 'kernel.request', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))

Maybe throw a specific exception for this?

Even an InvalidArgument exception explaining that only ASCII is allowed would be clear to callers.

+1 for throwing an exception mentioned in #13

so I've added the exception at Drupal\Core\Config\DatabaseStorage->readMultiple() level, but it's not being caught anywhere in the stack and each stack trace is different for different config types (see below).

Please advise/recommend where/how exception should be thrown/caught.

IMHO handling it on Drupal\Core\Config\DatabaseStorage->readMultiple() without additional exceptions is sufficient as it's such an edge case which is used for exploitation/attack only (there's already check for if (empty($names)) which doesn't throw any exceptions).

Here's a stack trace for: /node/add/тест

The website encountered an unexpected error. Try again later.

InvalidArgumentException: The array contains invalid values. in Drupal\Core\Config\DatabaseStorage->readMultiple() (line 116 of core/lib/Drupal/Core/Config/DatabaseStorage.php).
Drupal\Core\Config\CachedStorage->readMultiple(Array) (Line: 165)
Drupal\Core\Config\ConfigFactory->doLoadMultiple(Array) (Line: 136)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array) (Line: 181)
Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple(Array) (Line: 312)
Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) (Line: 183)
Drupal\Core\Entity\EntityRepository->getCanonicalMultiple('node_type', Array, Array) (Line: 175)
Drupal\Core\Entity\EntityRepository->getCanonical('node_type', 'тест', Array) (Line: 134)
Drupal\Core\ParamConverter\EntityConverter->convert('тест', Array, 'node_type', Array) (Line: 100)
Drupal\Core\ParamConverter\ParamConverterManager->convert(Array) (Line: 45)
Drupal\Core\Routing\Enhancer\ParamConversionEnhancer->enhance(Array, Object) (Line: 270)
Drupal\Core\Routing\Router->applyRouteEnhancers(Array, Object) (Line: 150)
Drupal\Core\Routing\Router->matchRequest(Object) (Line: 90)
Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object) (Line: 105)
Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object, 'kernel.request', Object)
call_user_func(Array, Object, 'kernel.request', Object) (Line: 111)

here's a stack for: /admin/structure/views/view/тест

The website encountered an unexpected error. Try again later.

InvalidArgumentException: The array contains invalid values. in Drupal\Core\Config\DatabaseStorage->readMultiple() (line 116 of core/lib/Drupal/Core/Config/DatabaseStorage.php).
Drupal\Core\Config\CachedStorage->readMultiple(Array) (Line: 165)
Drupal\Core\Config\ConfigFactory->doLoadMultiple(Array) (Line: 136)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array) (Line: 181)
Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple(Array) (Line: 312)
Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) (Line: 487)
Drupal\Core\Config\Entity\ConfigEntityStorage->loadMultipleOverrideFree(Array) (Line: 478)
Drupal\Core\Config\Entity\ConfigEntityStorage->loadOverrideFree('тест') (Line: 80)
Drupal\Core\ParamConverter\AdminPathConfigEntityConverter->convert('тест', Array, 'view', Array) (Line: 64)
Drupal\views_ui\ParamConverter\ViewUIConverter->convert('тест', Array, 'view', Array) (Line: 100)
Drupal\Core\ParamConverter\ParamConverterManager->convert(Array) (Line: 45)
Drupal\Core\Routing\Enhancer\ParamConversionEnhancer->enhance(Array, Object) (Line: 270)
Drupal\Core\Routing\Router->applyRouteEnhancers(Array, Object) (Line: 150)
Drupal\Core\Routing\Router->matchRequest(Object) (Line: 90)
Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object) (Line: 105)
Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object, 'kernel.request', Object)
call_user_func(Array, Object, 'kernel.request', Object) (Line: 111)

Here's stack trace for a contrib module: /webform/тест:

The website encountered an unexpected error. Try again later.

InvalidArgumentException: The array contains invalid values. in Drupal\Core\Config\DatabaseStorage->readMultiple() (line 116 of core/lib/Drupal/Core/Config/DatabaseStorage.php).
Drupal\Core\Config\CachedStorage->readMultiple(Array) (Line: 165)
Drupal\Core\Config\ConfigFactory->doLoadMultiple(Array) (Line: 136)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array) (Line: 181)
Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple(Array) (Line: 312)
Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) (Line: 183)
Drupal\Core\Entity\EntityRepository->getCanonicalMultiple('webform', Array, Array) (Line: 175)
Drupal\Core\Entity\EntityRepository->getCanonical('webform', 'тест', Array) (Line: 134)
Drupal\Core\ParamConverter\EntityConverter->convert('тест', Array, 'webform', Array) (Line: 100)
Drupal\Core\ParamConverter\ParamConverterManager->convert(Array) (Line: 45)
Drupal\Core\Routing\Enhancer\ParamConversionEnhancer->enhance(Array, Object) (Line: 270)
Drupal\Core\Routing\Router->applyRouteEnhancers(Array, Object) (Line: 150)
Drupal\Core\Routing\Router->matchRequest(Object) (Line: 90)
Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object) (Line: 105)
Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object, 'kernel.request', Object)
call_user_func(Array, Object, 'kernel.request', Object) (Line: 111)

Indeed, what I am suggesting is a developer experience improvement that would highlight the precise problem of invalid input to the function. It is up to the callers to provide valid input in the first place or to handle the exception.

Will also need test coverage.

I will add a dissenting opinion, just to be helpful. Rather than be limited to us-ascii, Drupal should handle UTF8 throughout. That way, users and developers would be able to properly use Drupal in their local language.

@cafuego That is a separate matter. The reasons for ASCII are from #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters → and I suggest creating a new issue if you want to challenge those decisions in a way that works across all supported database engines.

patch of MR9542 @ 265e30ba9c9e65396211044895ec0c1e50c52449 for temporary fix

Added test for the Special character and additional check added into the Configuration exist method.

Interesting, I found some incoming requests to our production going to /filter/tips/｡｡｡, which is accessible as anonymous.
This throws such errors as well.

📌 Add access control to /filter/tips Needs work

This needs work based on the plan for this issue, which is to throw an understandable exception.

Such request are done by spambots, which triggers a false-positive in your app-monitoring software. So I am happy, with a notfound exception, but not with any other which triggers a false-positive problem.

See comment #16. I believe this component should throw a specific exception when there is invalid input and the callers must be fixed to send only valid input.

@cilefen

The caller are spambots. Spambots does not do what we want. ;-)

I mean do we have issue like:

Why does this not work?

$config = \Drupal::configFactory()->getEditable('Über.settings');

or

drush cget 'Über.settings'

The patch does what would happen, when you only use ASCII or postgres (?). Page 404, so I am happy. :D

The exception DatabaseExceptionWrapper in the new test are not thrown or should not. So try/catch can be removed.

I would replace it with just a request to /node/add/Über which should return a 404.
With postgres this should already throw a not found exception.

And because we do not use native typehints to force config name is string. the $value should not be null in mb_check_encoding (deprecated).

Configuration object names must be ASCII only. Other input is invalid. In my opinion the better developer experience is to throw an exception when there is invalid input than to silently do nothing. Calling functions should handle the exception.

The only way to have a consistent experience across database engines is to throw an exception on invalid input.

If exception is to be thrown, consider:
where the exception should be thrown:
- at DB level
- at config level
- at entity level
- at routing level

where exception should be handled:
- at config level
- at entity level
- at routing level
- at request processing level

In the end, exception would be visible to the client as 404 (as route is not found)

Not all situations would be 404s.

One approach I have used is to create an event listener. The event is triggered by a 404. But you set additional condition(s) that must be met, so not just any 404 but one which is caused by x, or y. X or y can include a particular exception occurring.

If anyone believes that this issue should be fixed by presenting a 404 or other 4xx status code to the user with custom message and possibly also a log message, please create a child issue.

I would suggest the following steps:

1. Create a unique Exception e.g. one that extends the InvalidArgumentException class
2. Create an event listener that listens for the unique Exception to be thrown
3. Redirect to a 4xx status code page and display a message
4. Possibly add a log entry also

I have edited the kernel test so it now passes and created instead Functional test coverage. I have reverted DatabaseStorage.php to pre-issue version and found that the Functional test fails. Then checking out the latest version of that file (the latest MR) the tests all pass. So I am flagging 'Needs Review'

Appears to still have open threads to address.

@smustgrave I see several things in the threads. I note this issue → . However if you search through core for usages of InvalidArgumentException you will find that by far the majority are not extending from the base class. Issue 2654936 concerns the Rules module. In that issue it is stated that not extending from base classes like InvalidArgumentException is 'bad OOP'. However if true then they could have created a child issue: to remove all the unextended uses of InvalidArgumentException in core. I am unaware of any initiatives to do that.

After throwing InvalidArgumentException for the DatabaseStorage::exists and DatabaseStorage::read methods, I found that the former breaks an existing kernel assertion/ test. So I have reverted to the existing DatabaseExceptionWrapper class to fix the test. The tests are back to green. Setting back to 'Needs review'.

Patch of MR9542 @ 4090fac2899a7df61a6db7ebe553ad39ecdd3a8c for temporary fix

Small update around the test, rest looks good!