symfony/http-foundation commit 32310ff breaks PathValidator

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

@alfthecat the most obvious cause of this, looking at your view, would be if the product_id has a trailing space or control character. The view creates links with path: /product/{{ product_id }} and this path will be processed by Drupal\Core\Path\PathValidator, which calls Request::create('/' . $path); which now throws that exception.

Adding a remaining tasks item to analyze PathValidator class. On the one hand, it can be safer to throw exceptions for security reasons, as Request::create() now does in its most recent versions. On the other hand, PathValidator is a validator which returns FALSE if the path is not valid - thus it may be expected to catch this new exception and return FALSE.

@alfthecat can you check if MR !10307 resolves your issue? If not, then I guess a full stacktrace of the BadRequestHttpException would be helpful, I was really just guessing/assuming that this was the Request::create() call throwing the exception.

I looked at how some invalid paths are handled with MR !10307 applied, and it appears they are handled correctly, because the invalid path is actually "repaired" by being URL-encoded. Some examples:

Drupal\Core\Url::fromUri('internal:/1:1')->toString();
// "/1%3A1"

Drupal\Core\Url::fromUri('internal:/foo ')->toString();
// "/foo%20"

Adding an issue that may be a related symptom.

Ok great. I added a test which passes a new line character to PathValidator - I don't know specifically what caused the exception in your case, but I don't think it matters, we just want to ensure it catches a BadRequestException and returns FALSE.

We hit this on:

 [notice] Update started: block_content_post_update_revision_type
 [error]  Invalid URI: A URI must not start nor end with ASCII control characters or spaces. 
 [error]  Update failed: block_content_post_update_revision_type 
 [error]  Update aborted by: block_content_post_update_revision_type 

Quickly downgraded symfony/http-foundation to get the deploy working (it had worked on dev and stage, so of course we only hit it on prod), but we got dev into a broken state so we could confirm the fix later, and this appears to fix it.

In our monitoring tool we saw a lot of "Allowed memory size of ... bytes exhausted" errors. These errors were caused due to a infinite loop via DefaultExceptionHtmlSubscriber (and maybe in our case only because we are using menu_trail_by_path module).

The error still occurred after the MR, because the non-ASCII characters in the request path are urlencoded. I've updated the MR to handle both encoded and non-encoded non-ASCII characters.

The attached patch is a snapshot of the latest state for safe usage with composer patches.

Maybe related

@casey #14 definitely sounds like #3486972: DefaultExceptionHtmlSubscriber should not clone the request for 400/BadRequestException, reviews over there would be great.

Bumping this issue to critical, also reviewed the MR and it looks good to me, so RTBC.

And tagging beta target because this is a new regression.

Drupal\Tests\Core\Path\PathValidatorTest::testGetUrlIfValidWithoutAccessCheckWithInvalidPath
TypeError: MockObject_UrlMatcherInterface_0ba26206::match(): Argument #1 ($pathinfo) must be of type string, null given, called in core/lib/Drupal/Core/Path/PathValidator.php on line 167

@casey re: the infinite loop + out of memory, I also needed to catch the new Request::create() exceptions in a different place, see https://git.drupalcode.org/project/drupal/-/merge_requests/10306

Another example of the problem https://www.smartsheet.com/customers/stegh as you can see here Learn more online: <a href="https://www.stegh.on.ca/ "> the extra space causes no problems at all in a browser but running PathValidator over this blows up because Symfony is garbage^W broke semver in https://github.com/symfony/http-foundation/commit/32310ff3aa8126ede47168... as this is an API incompatible change which shouldn't have been committed to any released branches at all.

Please commit to 10.x too.

Committed to 11.x and backported to 10.5.x, 10.4.x and 11.1.x

Removing beta target tag

Setting as Patch to be ported for 11.0.x and 10.3.x

Updated the MR for 11.0.x so we can make sure its green before we cherry-pick to production branches

Is there anything I can help with for the 10.3 patch?

@pandaski review the current MR and mark the issue as RTBC if it looks ok, then we can rebase it for 10.3

Drupal\Tests\Core\Path\PathValidatorTest::testGetUrlIfValidWithoutAccessCheckWithInvalidPath
TypeError: MockObject_UrlMatcherInterface_017374f3::match(): Argument #1 ($pathinfo) must be of type string, null given, called in core/lib/Drupal/Core/Path/PathValidator.php on line 167

Tests fail on 11.0.x, so fixing those would help too @pandaski

The patch for 10.3 and 11.0 looks good and resolves the issue locally.

During local testing, I successfully applied the patch from this merge request to 10.3.x and 11.0.x.

https://git.drupalcode.org/project/drupal/-/merge_requests/10307.patch

The patch has resolved the issue in initial local tests.

The recent PHPUnit test error seems similar to issue #18. I’m running local tests to validate it.

My local PHPUnit tests ran successfully.

10.3.11-dev

PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Path\PathValidatorTest
Path Validator (Drupal\Tests\Core\Path\PathValidator)
 ✔ Is valid with frontpage
 ✔ Is valid with none
 ✔ Is valid with external url
 ✔ Is valid with invalid external url
 ✔ Is valid with link to any page account
 ✔ Is valid without link to any page account
 ✔ Is valid with path alias
 ✔ Is valid with access denied
 ✔ Is valid with resource not found
 ✔ Is valid with param not converted
 ✔ Is valid with method not allowed
 ✔ Is valid with failing parameter converting
 ✔ Is valid with not existing path
 ✔ Get url if valid with access
 ✔ Get url if valid with query
 ✔ Get url if valid without access
 ✔ Get url if valid with front page and query and fragments
 ✔ Get url if valid without access check
 ✔ Get url if valid without access check with invalid path

Time: 00:00.017, Memory: 10.00 MB

OK (19 tests, 85 assertions)

11.0.10-dev

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.14
Configuration: /opt/drupal/web/core/phpunit.xml.dist

...................                                               19 / 19 (100%)

Time: 00:00.021, Memory: 12.00 MB

Path Validator (Drupal\Tests\Core\Path\PathValidator)
 ✔ Is valid with frontpage
 ✔ Is valid with none
 ✔ Is valid with external url
 ✔ Is valid with invalid external url
 ✔ Is valid with link to any page account
 ✔ Is valid without link to any page account
 ✔ Is valid with path alias
 ✔ Is valid with access denied
 ✔ Is valid with resource not found
 ✔ Is valid with param not converted
 ✔ Is valid with method not allowed
 ✔ Is valid with failing parameter converting
 ✔ Is valid with not existing path
 ✔ Get url if valid with access
 ✔ Get url if valid with query
 ✔ Get url if valid without access
 ✔ Get url if valid with front page and query and fragments
 ✔ Get url if valid without access check
 ✔ Get url if valid without access check with invalid path

OK (19 tests, 85 assertions)

I think what's happening is the test expectations needed to be adjusted because this branch is using an older version of symfony/http-foundation that doesn't actually throw. In other words, this MR wouldn't actually need to be backported to this branch until symfony/http-foundation is updated?

I changed the test expectations for it to pass old with the older version of symfony/http-foundation, but on the other hand, if this branch is still supported, then actually symfony/http-foundation should be updated instead and we should revert the last commit?

One quick question, cant we sanitize the $path variable to remove space instead of handling it with the BadRequestException?

For example, $path = trim($path); resolves the first issue I guess.

@praveenpb for a few reasons.. PathValidator is already catching numerous exceptions and returning FALSE, so it clearly makes sense to do so in a new case. There is some fairly complex logic in Request::create() for deciding which strings are not valid URLs, which also includes the even more complex logic for strings that parse_url() considers "seriously malformed URLs," so you wouldn't want to try to recreate this logic. It's best to leave it to Request::create() (and parse_url()) to decide, and catch the exception that it can now throw.

(It's actually a little bizarre that Request::create() didn't previously throw an exception - I guess you could pass any arbitrary string to it and get a request object.)

@mfb Okay, makes sense. Reverting my changes.

@mfb One more question, for existing paths which has an extra leading or trailing spaces already, this will return FALSE and consider as an invalid url. What would be the impact? Will it break some feature in existing sites?

By the way, I was trying to grok parse_url()'s seemingly arbitrary logic for what returns FALSE, and noticed that the PHP docs say "This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using filter_var() with the FILTER_VALIDATE_URL filter."

So, it might not actually be a good idea for Request::create() to rely on parse_url() for parsing relative URLs, or for Drupal core to rely on Request::create() to parse relative URLs/paths, and some followup might be needed related to this? But so far that seems to be a concern only for weird edge cases (a path with a trailing space or a colon and numbers doesn't seem normal) and could take a while to sort out, so we presumably want this fix either way.

@praveenpb paths can contain spaces (they get URL-encoded when the path string is rendered to a URL string). But I think trailing spaces would be unlikely because a path seems to be trimmed? If there are some paths that are throwing this exception then they would already be broken - this MR wouldn't make them any more broken.

I ran into this issue on a menu provided by the domain_menus module. The patch from MR 10307 applied to 10.3 and fixes the issue for me.

Thanks for the work on this! I'd mark it RTBC, but there appears to still be some discussions about the coding.

I'm thinking that it seems like a good idea to update symfony/http-foundation, rather than changing the tests to pass on the older version, because after all, symfony's changes here were security-related. If folks don't feel comfortable with that, we could revert the last two commits in the MR.

Hi all! I am having a similar issue with Views module and symfony/http-foundation. After upgrading to Drupal 10.3.x
In my case, it is a taxonomy view with an image entity reference field field_image and a content entity reference field field_park_association

The field_image field is set to rewrite results with with the following configuration:

• - Output this field as a custom link with link path set to {{ field_park_association }}
• - The option "Replace spaces with dashes" is checked
• - The option "Remove whitespace" is checked

The error this configuration is throwing:
Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid URI: A URI must not start nor end with ASCII control characters or spaces. in Symfony\Component\HttpKernel\HttpKernel->handle() (line 83 of /code/vendor/symfony/http-kernel/HttpKernel.php).

Currently, Drupal 10.3.10 site
PHP version 8.2
symfony/http-foundation v6.4.16
views 10.3.10
views_ui 10.3.10

By the way, this merge request added a try/catch BadRequestException for a $router->match() call.

However, I don't believe the Router::match() method should throw a BadRequestException - according to its upstream phpdoc, it should only throw various routing exceptions.

I have a merge request resolving this over at #3491543: symfony/http-foundation Follow up issue for isAdminPath validator (theoretically we could remove catch statements that shouldn't be needed, but I didn't get to that yet).

Opened new merge request !10509 for the 10.3.x branch so folks can test/review it. This includes updating symfony/http-foundation, rather than modifying the test to pass on the older version. But if necessary, we could instead remove the update and modify the test.

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.

I cannot apply any MR to 10.4.0:

Installing drupal/core (10.4.0): Extracting archive
Applying patches for drupal/core
https://git.drupalcode.org/project/drupal/-/merge_requests/10509.diff (https://www.drupal.org/project/drupal/issues/3489329: symfony/http-foundation commit 32310ff breaks PathValidator)
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/10509.diff

Installing drupal/core (10.4.0): Extracting archive
- Applying patches for drupal/core
https://git.drupalcode.org/project/drupal/-/merge_requests/10307.diff (https://www.drupal.org/project/drupal/issues/3489329: symfony/http-foundation commit 32310ff breaks PathValidator)
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/10307.diff

I think we should modify the test on 10.3.x (or at least not do it here), or backport the fix without test coverage.

Sorry for the mess I've made trying to integrate the changes into 10.4.x.
Partly because I see that these changes are already in the 10.4.x branch, so nothing needs to be done.

Hi everyone,

I tried to update the core from 10.3.10 to D10.4.1 but doesn't work, I tried with both MR (10307, 10509) too.
I received the same message on browser:
"Invalid URI: A URI must not start nor end with ASCII control characters or spaces."
and checking with drush ws:
"Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid URI: A URI must not start nor end with ASCII control characters or spaces. in Symfony\Component\HttpKernel\HttpKerne.."
similar to https://www.drupal.org/project/drupal/issues/3486195

Updated the issue summary. Added steps to reproduce and other edits.

Minor change to the test comment in the MR.

Thanks for the work on fixing this Critical bug.

About this time last year this was set to 11.0.x for backport to 11.0.x and 10.3.x. Since both of those versions are no longer supported this will not be backported to those versions. And the last report of this causing a problem was in Jan of this year so hopefully that has been resolved over the year. It is time to close this as fixed.

I updated credit.