Problem/Motivation

Due to https://github.com/advisories/GHSA-mrqx-rp3w-jpjp symfony/http-foundation 7.1.7 and 6.4.14 patched Request::create() to throw a BadRequestException exception on certain invalid URLs. When processing such an invalid path, PathBasedBreadcrumbBuilder now throws this new exception.

Steps to reproduce

Visit certain invalid paths such as /:123/foo - an exception will be thrown by PathBasedBreadcrumbBuilder (as can be seen by inspecting the stacktrace of one of the chained exceptions)

Proposed resolution

PathBasedBreadcrumbBuilder should instead catch the new exception and return NULL, which indicates "the path couldn't be matched" and skips this breadcrumb, as it did before the symfony/http-foundation update.

Remaining tasks

Backport/reapply to the 10.5.x and 10.4.x branches.

User interface changes

Sites will once again respond with the 404 error page when such invalid URLs are requested, as they did before the symfony/http-foundation update.

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3490710

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mfb created an issue. See original summary.

mfb opened merge request !10398

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

The wording seems like maybe a regression was introduced? Think we should have test coverage for the scenario described

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Test coverage looks good https://git.drupalcode.org/issue/drupal-3490710/-/jobs/3540197

  • catch committed c44c9a2e on 11.1.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...

  • catch committed d034f558 on 11.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...

  • catch committed 899ed6f1 on 10.3.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

  • catch committed bfda2356 on 10.4.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...

  • catch committed 19e00dae on 10.5.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...

  • catch committed 98c0e0f0 on 11.0.x 
    Issue #3490710 by mfb: Catch potential exception when calling Request::...

catch closed merge request !10398

spokje’s picture

spokje
Primary language Dutch
commented
Status: Fixed » Needs work

This broken HEAD on 10.5.x and 10.4.x with a cheery:

$ vendor/bin/phpunit -c core core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\system\Unit\Breadcrumbs\PathBasedBreadcrumbBuilderTest
...........                                                       11 / 11 (100%)

Time: 00:00.257, Memory: 12.00 MB

OK (11 tests, 56 assertions)

Remaining direct deprecation notices (2)

  2x: Since symfony/http-foundation 6.3: Calling "Symfony\Component\HttpFoundation\Request::create()" with an invalid URI is deprecated.
    2x in PathBasedBreadcrumbBuilderTest::testBuildWithInvalidPath from Drupal\Tests\system\Unit\Breadcrumbs

Remaining indirect deprecation notices (1)

  1x: Automatic conversion of false to array is deprecated
    1x in PathBasedBreadcrumbBuilderTest::testBuildWithInvalidPath from Drupal\Tests\system\Unit\Breadcrumbs

Unsure why it works on 11.x with Symfony 7.2, where I would expect the deprecation throwing code would be removed, but it does.

As an added weirdness-bonus, the test fails, making the unit-test job fail on GitLab, but the specific error doesn't show up in the report. I can only find it in the raw output of the unit-test job.

But that's probably because it's a deprecation error, not an "actual" error?

spokje’s picture

spokje
Primary language Dutch
commented
Version: 10.3.x-dev » 10.5.x-dev

  • catch committed 677b2dc3 on 10.4.x 
    Revert "Issue #3490710 by mfb: Catch potential exception when calling...

  • catch committed d3892f34 on 10.5.x 
    Revert "Issue #3490710 by mfb: Catch potential exception when calling...
catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Major

Reverted from 10.5.x and 10.4.x

spokje’s picture

spokje
Primary language Dutch
commented

_tips hat_

mfb’s picture

mfb
they or he
commented

So it looks like this is specifically for the case where parse_url() returns FALSE.

I was discussing this over in https://www.drupal.org/project/drupal/issues/3489329#comment-15880688

The PHP docs for parse_url() 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, technically speaking, maybe Request::create() should not rely on parse_url() for parsing relative URLs, or Drupal core should not rely on Request::create() to parse relative URLs/paths.

However, so far I've only noticed parse_url() returning FALSE for "weird" relative URLs (in my case, requests made by a bot), not something that I would have expected to work. So a quick fix could be to test the relative URL with parse_url() and return NULL, before calling Request::create()

mfb’s picture

mfb
they or he
commented

I think this can be fixed on all branches by simply updating the symfony/http-foundation requirement to a more recent version that throws the exception, rather than triggering the deprecation? I.e. bump from 6.4.15 to at least 6.4.16?

My workaround in #20 would only be needed if there is a requirement to support older versions of symfony/http-foundation

mfb opened merge request !10485

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
mfb’s picture

mfb
they or he
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

We need to do that anyway, makes sense to me.

  • longwave committed 3116614f on 10.4.x 
    Issue #3490710 by mfb, catch, spokje: Catch potential exception when...

  • longwave committed c6143563 on 10.5.x 
    Issue #3490710 by mfb, catch, spokje: Catch potential exception when...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 10.5.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

symfony/http-foundation was updated in #3490183: Update Composer dependencies for 10.4.0 so from here I have just committed the fix and test.

Committed and pushed c6143563c31 to 10.5.x and 3116614f706 to 10.4.x. Thanks!

longwave closed merge request !10485

eric_a’s picture

eric_a commented

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

eric_a’s picture

eric_a commented

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

My bad. The loop with the out-of-memory was mentioned in #3486972: DefaultExceptionHtmlSubscriber should not clone the request for 400/BadRequestException.

The point more or less remains though. Without this fix in 10.3.x it's a choice between secure dependencies or a pretty broken site.
Granted that out of memory seems to be more of a problem then "just" an exception.

catch’s picture

catch
he/him
Primary language English
commented
Version: 10.4.x-dev » 10.3.x-dev
Status: Fixed » Patch (to be ported)

I think we can probably backport this to 10.3.x too.

eric_a’s picture

eric_a commented
Related issues: +#3486195: An update to symfony/http-foundation plus a trailing space took down the views UI, +#3489329: symfony/http-foundation commit 32310ff breaks PathValidator
spokje’s picture

spokje
Primary language Dutch
commented
Version: 10.3.x-dev » 11.0.x-dev
Status: Patch (to be ported) » Needs work

Seems to have broken HEAD of 11.0.x: https://git.drupalcode.org/project/drupal/-/pipelines/382966/test_report...

  • catch committed a2071c0f on 11.0.x 
    Revert "Issue #3490710 by mfb: Catch potential exception when calling...
catch’s picture

catch
he/him
Primary language English
commented

Reverted from 11.0.x too.

mfb’s picture

mfb
they or he
commented

What's the policy for security updates of dependencies on security-only branches? Assuming it's ok and/or required to do so, the composer update could be applied on that branch

catch’s picture

catch
he/him
Primary language English
commented

@mfb we can update, but this was quite a minor security fix (at least for us) whereas the function regressions have been quite serious, and there are multiple contexts where this error can be thrown, so I would personally prefer us to keep fixing these bugs and then only update the constraints if we really need to - individual sites can update with composer update whenever they want.

  • alexpott committed 4d9f4243 on 10.3.x 
    Issue #3490710 follow-up by alexpott: Catch potential exception when...
quietone’s picture

quietone commented
Version: 11.0.x-dev » 10.3.x-dev
Status: Needs work » Fixed

This was applied to 10.3.x, 10.4.x, 10.5.x, 11.1.x, and 11.x. It was applied to 11.0.x, which is no longer supported, but then reverted.
Therefore restoring the fixed status.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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