DefaultExceptionHtmlSubscriber should not clone the request for 400/BadRequestException

Looks good, but we can lean on the data provider even more.

One minor issue I noticed is excessive logging - three messages are logged for a bad request: "page not found" warning, "client error" warning, "php" error

After updating symfony/http-foundation to 6.4.14, I started experiencing out of memory errors for requests for paths like this:

/+/testing

With symfony/http-foundation 6.4.13, a request like that on my site would show the normal 404 page. But 6.4.14 has this in Request.php:

        if ('' !== $uri && (\ord($uri[0]) <= 32 || \ord($uri[-1]) <= 32)) {
            throw new BadRequestException('Invalid URI: A URI must not start nor end with ASCII control characters or spaces.');
        }

I guess the + is interpreted as a space so that exception is thrown. Then I get an infinite loop as described here and eventually an out of memory error.

However, with this patch, applied, while I no longer get an infinite redirect loop, I also don't get the normal 404 page anymore. Instead, I get a page that just has message No route found for "GET https://mysite.com/+/test"

Is that expected behavior with this patch? that the 404 page wouldn't be shown anymore for bad requests? I guess that makes sense.

I think this is fine. This does mean that some pages that were themed 404s before the Symfony security update will now become a different error, but the behaviour change is in Symfony; this just fixes a related problem that already existed but is perhaps more visible now - but as @catch says this shouldn't happen to end users in normal operation.

Okay, thanks for explaining. Sounds good. I agree that requests seeing these errors will be bots.

Committed to 11.x and backported to 11.1.x, 11.0.x, 10.5.x, 10.4.x and 10.3.x as this is a critical bug and there's minimal risk of disruption.

Reverted this one, the failure in MediaSourceOEmbedVideoTest is from this change

This issue ended up in the release notes for Drupal 11.0.9 but presumably that's a typo, since it was reverted

@mfb the revert is also in the release notes which is confusing, will remove both.

oh somehow missed that? nevermind :p

Huh I couldn't reproduce the test failure on this MR? 🙃🤷

I'm working on an alternate fix for this: Catch the exception when calling Request::create() in core/modules/system/src/PathBasedBreadcrumbBuilder.php

Well, the root of the issue here is that Request::create() never threw exceptions previously, but now it may. So, dealing with these exceptions did seem like the correct fix to me - and resolves my infinite loop.

Of course there is more than one Request::create() call in core, but there actually are not that many, outside of tests

Are you saying you were seeing an infinite loop prior to the recent symfony update? For me that's not the case, this was a new bug report.

Interesting! In that case I'd want to look at those problems, but I don't know specifically where you were seeing that.

I can only talk about the two brand new issues I've looked at related to the new Request::create() exception, where it totally makes sense (imho) to catch it where it's thrown because we can logically return FALSE or NULL - https://git.drupalcode.org/project/drupal/-/merge_requests/10306 and https://git.drupalcode.org/project/drupal/-/merge_requests/10307

If you don't accept https://git.drupalcode.org/project/drupal/-/merge_requests/10306 for this issue then I can open a child issue (and I realize it needs a test, but I haven't gotten there yet :)

@mfb can you open another issue for your work on the path breadcrumb builder. Thanks!

Committed and pushed cec8630b61c to 11.x and 1641c8904af to 11.1.x and 25d18f8735f to 11.0.x and 22176d99bd1 to 10.5.x and c8034c72fdd to 10.4.x and 9414efaca36 to 10.3.x. Thanks!

This seems to solve issue I had with encdoded backslash https://www.drupal.org/project/drupal/issues/3492437
But did I miss some commit from here because I'm now getting white page with plain text "Invalid URI: A URI cannot contain a backslash." It used to be normal page not found before.

See also the child issue #3490710: Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder which should resolve it (or if not, you have to look at the chained exception stacktraces to see what exception you're hitting)

ok, thanks!

What is the appropriate way now to throw a BadRequestHttpException that returns a themed (user-friendly) error page? We encountered this recently in the course of one of our custom modules throwing this exception only to find that we now get a blank page, whereas before we could present a themed page (i.e., for the system.4xx route).