#Security improvements

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

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

larowlan → reopened merge request !10153

larowlan → closed merge request !10153

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.

Re #6 I think this is no longer an issue in 10.x/11.x due to:

https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/HttpFo... e.g. a BadRequestException is already thrown by Symfony if the value retrieved in ::get() isn't a string.

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

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

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.

@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.

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.

Should this target 11.x, instead of 7.x, because of this?

This is present in Drupal 8 too in \Drupal\user\Form\UserPasswordForm::buildForm().

I've added an update that changes from creating a separate read-only element to disabling the input fields. Existing test for the site-wide form was updated, and a test for the personal contact form was added.

catch → credited alexpott → .

catch → credited donquixote → .

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