Use a route requirement to prevent non-ASCII characters causing an exception when looking up a config entity

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.

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

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.

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.

#3063877: Add access control to /filter/tips

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.

Not all situations would be 404s.

I think that a simple InvalidArgumentException that returns a short, simple explanatory message to the screen is all that is needed.

@jannakha I would appreciate it if you can take a look at the kernel test you created and see if it can be made to fail then pass when the MR is applied.

Screenshot of the InvalidArgumentException

Unlike the DatabaseStorageWrapper exception no database query is involved. Instead, the InvalidArgumentException is thrown if validation of the 'name' argument fails. The validation does not rely on or require a database query.

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 have added Exceptions to the read and exists methods and checked for not NULL.

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!

#47 works well

We do not need to use use function... for global functions.
Was that suggested by AI?

Appears to have unresolved threads.

I didn't find any problems while testing the patch (MR). Everything works.

Regarding this:

just curious,
- why https://www.cyber.gov.au/node/add/testXYZ returns 404, but
- why https://www.cyber.gov.au/node/add/тест would return unhandled exception?
should it just return 404?

My opinion is that in this case, it is justified, since we cannot check that this configuration does not exist in a database table to return a 404. But we know for sure that according to Drupal, the configuration name must contain only [^a-z0-9_]+ (at least from the administrative panel we cannot create it with a different name). Therefore, I believe that there should be validation of configuration objects, namely their names, and the patch only fixes the result of the lack of this check.

Since the main responsibility of DatabaseStorage is to perform queries rather than validate the encoding of parameter, especially considering that if the database uses the utf8mb4_unicode_ci collation, there will be no issues with the query.

This one needs a rebase.

Rebased.

I ran into this issue on my personal blog, so I took a closer look and helped review and test it. Disclaimer: I used Claude Code, but reviewed everything manually.

I just committed a change because testExceptionIsThrownWithNonAsciiCharacters contains dead code. See diff.

PHPUnit’s expectException() and expectExceptionMessage() are setters, so each call overwrites the previous one. In the original test, they are called twice, which means only the second pair is actually asserted. I verified this manually by replacing the first expectExceptionMessage() with a random string. The test still passes.

$this->expectException(\InvalidArgumentException::class);          // line 157 — set
$this->expectExceptionMessage('The config name part of the URI...'); // line 158 — set

// We cannot test multiple exceptions at once.
$this->expectException(\InvalidArgumentException::class);          // line 161 — overwrites 157
$this->expectExceptionMessage('The name config.testóú...');        // line 163 — overwrites 158

As a result, read() with non-ASCII input is never actually tested, only readMultiple().

My fix splits the test into two separate methods: one for read() and one for readMultiple(), so each exception is properly asserted.

I just committed another change to the tests: see diff.

The MR adds ASCII validation to read() and readMultiple(), but exists() also queries the config table by name and will hit the same collation error with non-ASCII input. In other words, the solution is incomplete.

The existing tests tried to show this by calling exists('config.testáéóú') and catching a DatabaseExceptionWrapper. However, this check was part of testExceptionIsThrownIfQueryFails, which drops the config table and recreates it with a broken schema. In that setup, exists() fails because the table is broken, not because of non-ASCII input, so the test was not actually verifying what it claimed.

So I moved the test into a dedicated test method that runs against a normal/correct config table, where the collation mismatch is the actual cause of the failure. After my commit, you can see that the tests still pass, meaning that we throw a DatabaseExceptionWrapper (i.e. exists() causes a collation error). We clearly isolated the problem, which I think is an improvement.

I am happy to work on a fix, but I have a question: should exists() return FALSE or throw an InvalidArgumentException?

Returning FALSE feels natural to me, but it would be inconsistent with read() and readMultiple(), which throw an exception. Most (all?) PHP methods that are of type exists() for lack of a better term (e.g. file_exists(), class_exists(), array_key_exists(), etc) return FALSE.

That said, it also feels weird to return FALSE when we don't actually go to the database ... So throwing InvalidArgumentException might actually the right call?

One more commit: I added the same mb_check_encoding guard to exists() that read() and readMultiple() use. It now throws InvalidArgumentException, so I updated the isolated test accordingly.

@olly pointed out in #43 that this breaks existing tests, but that has been over a year, and there were other changes since. Let's see what happens ...

Looks like the change in exists() works now ... 👍

This was discussed already (e.g. #28, #37, #54, #55) but the statusCodeEquals(500) feels a bit iffy because the correct response is almost certainly a 404, not a 500.

The route parameter goes through the parameter converter, hits config entity storage, throws the InvalidArgumentException, and returns a 500. Hence the statusCodeEquals(500) assert in one of the tests.

This could be handled at a higher level, i.e. do better parameter validation at the routing level. Per the previous comments that could belong in its own issue, or it could now be a relatively easy addition to this one.

I did some more digging to think through the 404 vs 500 problem, and the actual bug I'm seeing on my site. Here are my current learnings and thinking.

Most config entity IDs use the machine_name data type (/^[a-z0-9_]+$/, defined in core.data_types.schema.yml), though some entity types use custom patterns (shortcut sets allow dashes, blocks allow dots, language entities use a langcode pattern with uppercase). Either way, all variants are ASCII-only.

These constraints are validated on write (the config entity validation enforces the schema constraints, and UI forms use them too). However, they are not validated during entity loading.

To fix the original issue, we could add a check in ConfigEntityStorage::doLoadMultiple() that skips non-ASCII IDs before they become config names. It's a 3-line change:

foreach ($ids as $id) {
  // Config entity IDs use ASCII-only patterns (machine_name, langcode,
  // etc). Non-ASCII IDs can never be valid and would cause a database
  // collation error if queried.
  if (!mb_check_encoding($id, 'ASCII')) {
    continue;
  }
  $names[] = $prefix . $id;
}

I tested it on my localhost and it results in a 404 on all 3 paths given in the issue summary (i.e. /node/add/öüä, /media/add/öüä, /admin/structure/views/view/öüä).

Since non-ASCII IDs can never be valid, they are skipped by the check, and result in an "entity not found". It's maybe a bit strange to silently skip over them, but it should be fine because of the validation that happens on write.

So, with this 3-line check, the entity repository returns NULL, the param converter throws ParamNotConvertedException, and the routing enhancer turns that into a 404. Now the behavior for /node/add/invalid-id and /node/add/öüä both result in a 404, rather than a 404 and 500 respectively. And because the check sits inside the storage layer, every caller (routing, REST, Drush, contrib) hits it automatically ...

I considered using the strict machine name pattern /^[a-z0-9_]+$/ instead because mb_check_encoding($id, 'ASCII') always felt a bit odd, but that would reject valid IDs from entity types with custom patterns. As explained in the second paragraph, entity IDs are not fully standardized. I also didn't see a way to validate IDs using a config entity specific validator. I contemplated loading the typed config manager etc, but that would add a lot of overhead in a hot code path. In absence, I concluded that the mb_check_encoding is probably the right common-denominator. It's fast, and in a way, the entity ID standard is ASCII-only.

This 3-line fix could be complementary to the DatabaseStorage validation already in this MR. The InvalidArgumentException in read(), readMultiple(), and exists() remains a safety net for code that calls config storage directly.

But it could also be instead of the DatabaseStorage validation changes we made. With the 3-line change, the safety net doesn't really trigger anymore. I'll leave that up to the entity system maintainers to decide as I'm not an expert on it.

I didn't commit these test changes, but I've added a patch to the comment if that helps any of you evaluate this. I'd be happy to commit it in case we want to include it and run the tests. I'd personally include it as it's only a 3-line change and it's directly related to the bug report. If not, I'm happy to create a separate issue for this too. Hope this helps!

It could be included, but it would be good to get some extra eyes on it. Should it live in doLoadMultiple, or does it belong at a level higher? If so, we'd need to add the check in multiple places. Or should doLoadMultiple catch exceptions instead, and skip over those?

@dries your comments lead me to ponder if maybe we could instead put your improvement in the routing system. What would be really neat is if \Drupal\Core\ParamConverter\ParamConverterManager::setRouteParameterConverters() could allow a param converter to set a regex requirement if one is not set and the param converter supported it. We'd need to add a new interface to support getting a regex but I think this should be possible. The param converter could then get the regex from config schema for config entities and from the field system for content entities.

I also think making the suggested change in #65 is worth is as a defence in depth but ideally the routing system would catch this for us.

Here's some sub-optimal code to set the route regex for a config entity type on a route.

            // The schema key for config entities is prefix + '.*'
            $schema_definition = \Drupal::service('config.typed')->getDefinition($config_entity_type->getConfigPrefix() . '.*');
            $id_key = $config_entity_type->getKey('id');
            $schema_definition = \Drupal::service('config.typed')->getDefinition($schema_definition['mapping'][$id_key]['type']);
            if (isset($schema_definition['constraints']['Regex']['pattern'])) {
              $route->setRequirement($bundle_entity_type_id, trim($schema_definition['constraints']['Regex']['pattern'], '/'));
            }

Just pasting it here in case someone has a go at implementing #68

I've thrown up an MR with #68 mostly implemented. I've manually tested and run through the route building the debugger and this works quite nicely apart from for the multi-part IDs eg core.entity_form_display.*.*.* and where the ID is based on another config entity eg. editor.editor.* - I think we will be able to support both cases with a little work. See https://git.drupalcode.org/project/drupal/-/merge_requests/15283

@alexpott: thanks for the quick response and code. I decided to write some test to help me learn about your proposed changes. I just pushed them to your branch. It includes a kernel test for getRouteRequirement() and a functional test that hits /node/add/öüä and asserts a 404. They pass on my localhost.

While writing the tests, I noticed one thing myself. Config entities with multi-wildcard schemas (like entity_view_mode with core.entity_view_mode.*.*) don't match the simple prefix.* lookup, so getRouteRequirement() returned NULL and their routes are unprotected. Since all config entity IDs are ASCII-only (this is what this entire issue is about), one option is to fall back to a generic ASCII pattern like [a-zA-Z0-9_.\-]+ when the specific schema pattern can't be determined. I included this in my commit but happy to revert if that is wrong, or a better solution emerges.

Also, the pattern extraction uses trim($pattern, $delimiter) then trim($pattern, '^$'). I wondered if we have regex with flags and it appears we do. In core.data_types.schema.yml we have pattern: '/^[a-z][a-z0-9+\-\.]*$/i', for example. trim only removes the leading / (the trailing character is i, not /). I replaced it with a preg_match that properly separates the delimiter, pattern body, and flags.

I can't say I fully understand your code yet, but I feel my additions are directionally correct.

Just to summarize, we now have 3 layers going on (sorry for the scope creep!):

1. "Route requirements" that reject non-matching URLs at the router level. The most architecturally correct layer, in my opinion. I believe this catches 95%+ of the invalid IDs at the right level.

2. ConfigEntityStorage::doLoadMultiple() skips non-ASCII IDs before they reach config storage. Useful because not everything goes through routing (e.g. Drush, programmatic entity loads, etc). Plus, we might not always get route requirements? I must admit that I don't fully understand the multi-wildcard schemas, potential contrib gaps, etc in 1. This layer might be optional, but it probably doesn't hurt as it's just 3 lines of code that are pretty fast.

3. DatabaseStorage throws InvalidArgumentException for non-ASCII names in read(), readMultiple(), exists(). This is a good safety net for any code that calls config storage directly.

Some great progress, Alex.

I've been digging into this more this morning, and I have some more thoughts based on what I've learned so far. I'm not an expert on the Entity system, so please take this with a grain of salt.

The config schema defines what a valid entity ID looks like (e.g. machine_name with /^[a-z0-9_]+$/). And the typed data system can already validate against it: calling $entity->getTypedData()->validate() on a config entity with an invalid ID produces the right violations. This is tested in ConfigEntityValidationTestBase::testInvalidMachineNameCharacters(). So far so good! :)

But this validation is only used during config import. Strangely, it does not seem to be called during save(), load(), or routing. Also, forms use the MachineName element, which is separate from the schema. If someone or something changes or overwrites the schema pattern, the form element doesn't know. It seems fragile that the MachineName validation is not necessarily the same as the entity ID validation. Lastly, save() only checks for empty IDs and length.

So, this all leads to the question: should the schema not be the single source of truth, and should it not be used everywhere?

For save: ConfigEntityStorage::save() could call $entity->getTypedData()->validate() before writing. The infrastructure already exists, it just needs to be wired up.

For forms: if ConfigEntityStorage::save() validated against the schema via $entity->getTypedData()->validate(), the MachineName element wouldn't need to do validation anymore (in this case)?

For load and routing: Alex's MR already reads the regex pattern from the config schema via TypedConfigManagerInterface::getDefinition(). That is the same schema that $entity->getTypedData()->validate() reads from. That is a great step in the right direction, as both paths start to use the same source of truth.

The one question is where the schema-to-regex lookup lives. Alex's MR puts it in the param converter. I think it might actually belong on the entity type, similar to how hasIntegerId() already describes the ID format and is used in DefaultHtmlRouteProvider to set \d+ route requirements. A new getIdConstraint() method would generalize this for both content and config entities.

(I also think it may lead to a path where hasIntegerId() could be deprecated, but I've not verified that. At a minimum, something like getIdConstraint() could replace hasIntegerId() in DefaultHtmlRouteProvider. It would be an improvement.)

This is obviously bigger than the current issue but I think it points to the right long-term direction: one schema, used for import, save, load, and routing, rather than solving the same problem differently at each layer.

So, maybe we create a new issue for this, and get 3475540-unhandled-exception-ascii-2 committed here? I think 3475540-unhandled-exception-ascii-2 could be RTBC.

One more thought, separate from my previous comment, as it's more radical. :)

We should question how much validation infrastructure we actually need. We've basically operated without ID validation on load or save for the entire lifetime of Drupal 8 through 11. Sure, we validate on config import, but that is only so useful if everything else bypasses it.

I'm not saying we shouldn't do any of this. But it's always worth questioning new requirements before adding complexity.

Sometimes less is more. The validation gap is real, and the existing validation inconsistent. In many ways, the system hasn't been perfect (this issue proves that), but it's also been largely fine.

We could just validate everything as 'needs to be ASCII' and not allow entity types to deviate from that. That could be a very simple but effective improvement to a system that has worked largely fine?

Food for thought?

@dries I think #76 points us in a good direction. I think this issue should:

• Add the ASCII check to loadMultiple()
• Add a simple ascii regex on config entity routes where there is no requirement already. It think this regex perhaps should be more permissive… i.e. just be printable ascii.. [\x20-\x7E]  … we could increase the complexity and not permit the characters listed in \Drupal\Core\Config\ConfigBase::validateName()  but not sure that’s worth it.

We should also open a follow-up to discuss whether we can improve the route requirement based on the regex constraint and/or whether we can ties the param conversion and entity validation using constraints together in a better way.

Actually @tim.plunkett indicated they are ambivalent about the low level changes to config and the more I think about it the more i agree. We're just just one exception for another to no real benefit. The improvement here is we're stopping the routing system from being able to cuase these exceptions and that's delivered by https://git.drupalcode.org/project/drupal/-/merge_requests/15283

I like the "simple regex" approach in 253487f0. It's much simpler, fixes the bug, and defers complexity for now. In general, it makes sense to move your branch forward. Thank you for all the work on this, Alex!

Direct calls to ConfigFactory, Drush, and programmatic entity loads would still throw a DatabaseExceptionWrapper on MySQL. The DatabaseStorage validation addresses that, and ensures consistent behavior across database backends. It feels like a minor improvement, and is well-tested now, but it might be optional. I'll let others decide.

I created #3582417: Use config schema as single source of truth for entity ID validation to explore using config schema as the single source of truth for entity ID validation across save, load, and routing.

I'm glad we explored different options and reined it back in.

I think this simple regex looks great, I was wondering if this would break any current sites - but because we're not adding to an existing interface but creating a new one this seems like it will not be an issue.

I've fixed up the issue summary and title to align with the solution we've ended up with.

Also this feels like a bug fix. 500s are becoming 404s

I did a review. Overall this is clean and much simpler than the earlier iterations.

I added two comments that could be safely ignored.

Everything seems to work on my localhost.

We still need a final verdict on the DatabaseStorage validation, I believe.

Thank you, @alexpott.

I've renamed the interface and improved the comment to be more precise.

I agree this is RTBC.

Once it is in, we can reopen the discussion to decide on the DatabaseStorage validation (hidden branch), if needed.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased on main and fixed the conflict.

One question on the MR, it's about the presence or absence of a single word in a comment so leaving RTBC.

Responded in the MR. I think the comment is fine.

Committed/pushed to main, thanks!

Will need a backport MR for 11.x

Relatively easy backport, just 1 test needed to manually apply.

Committed 677b3b1 and pushed to 11.x. Thanks!

Consulting other RMs on whether this should go to 11.4.x, leaving at Patch (to be ported) for now.