DefaultExceptionHtmlSubscriber should not clone the request for 401s

Re-running a couple of functional test jobs due to random test failures but otherwise MR should be green now.

Converted to a data provider.

Fixing issue title.

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

Makes sense, applied the suggestions.

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.

@bkosborne yes that's the expected behaviour with the MR.

There are other things we could do, like make a subrequest with a sanitized path or similar, but to me it feels both more complex and less robust.

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.

Tagging as beta target because it's exacerbated by the latest Symfony release.

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.

larowlan → closed merge request !10153

larowlan → reopened merge request !10153

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

Test needed to be updated for a different error message, just an expectation change. Moving back to RTBC since the fix is trivial.

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

@mfb PathBaseBreadcrumbBuilder looks like a different issue to me, I have seen this without breadcrumbs being involved at all (i.e. in form processing), could you create a new issue for that?

Restoring the RTBC here.

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 now that many, outside of tests

Well, the root of the issue here is that Request::create() never threw exceptions previously, but now it may.

No this predates the recent Symfony security release, for example InputBag and ParameterBag can also throw a BadRequestException and those have been in Symfony for years now - any code throwing a BadRequestException in the course of page building can trigger this.

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.

@mfb not with just core, but yes.

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 🐛 Something wrong with symfony/http-foundation ? Active
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 🐛 Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder Needs review which should resolve it (or if not, you have to look at the chained exception stacktraces to see what exception you're hitting)

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.

No that's the change here - if the exception is thrown, then it produces a minimal exception error without a themed page, because the themed page could also throw the same exception again, leading to an infinite loop.

🐛 Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder Needs review and possibly other issues will make things more fault tolerant so the exception doesn't get thrown at all if it can otherwise be safely handled - as @mfb points out above.

ok, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.