Problem/Motivation

Fuzzing reveals that UrlHelper::parse() throws a warning when called with some malformed strings:

   WARNING  Undefined array key 1 in core/lib/Drupal/Component/Utility/UrlHelper.php on line 200.

Steps to reproduce

\Drupal\Component\Utility\UrlHelper::parse('#/://#')

Proposed resolution

One solution could be to check if $parts[0] is empty before trying to use it in explode().

Issue fork drupal-3442833

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

prudloff created an issue. See original summary.

Vivek Panicker made their first commit to this issue’s fork.

Vivek Panicker opened merge request !7685

vivek panicker’s picture

vivek panicker commented
Status: Active » Needs review
vivek panicker’s picture

vivek panicker commented

Tests are failing because of media_library test https://git.drupalcode.org/issue/drupal-3442833/-/jobs/1413833#L79.
Not sure how to resolve it.

smustgrave’s picture

smustgrave commented
Version: 10.2.x-dev » 11.x-dev
Status: Needs review » Needs work

Current development branch is 11.x

Can't just add this though it will have to have backward compatibility coverage for the typehint being added. With a deprecation.

prudloff’s picture

prudloff commented

The type hint would not fix the issue.
You can see in the steps to reproduce that I am passing a string.

Binoli Lalani made their first commit to this issue’s fork.

Binoli Lalani opened merge request !7721

binoli lalani’s picture

binoli lalani commented
Status: Needs work » Needs review

Hello,

I can reproduce the error in drupal 11.x and raised MR against 11.x branch. Please review.

Thank you!

smustgrave’s picture

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

Since this is a component class believe we will need test coverage

binoli lalani’s picture

binoli lalani commented
Status: Needs work » Needs review

Hello @smustgrave,

Thank you for reviewing the code. I have added test coverage for the warning. Please review.

Thank you!

smustgrave’s picture

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

Not 100% the use case but the test does show the failure

1) Drupal\Tests\Component\Utility\UrlHelperTest::testParse with data set #5 ('#/://#', array(null, array(), '/://#'))
Undefined array key 1
/builds/issue/drupal-3442833/core/lib/Drupal/Component/Utility/UrlHelper.php:213
/builds/issue/drupal-3442833/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php:272
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/TextUI/Command.php:146
/builds/issue/drupal-3442833/vendor/phpunit/phpunit/src/TextUI/Command.php:99
ERRORS!
Tests: 161, Assertions: 165, Errors: 1.

And checking if empty doesn't seem to hurt. LGTM.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Left a suggestion on the MR to preserve the logic of which bit of logic processes external urls and which processes internal.

pradhumanjain2311 made their first commit to this issue’s fork.

prudloff changed the visibility of the branch 3442833-undefined-array-key to hidden.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I believe @alexpott's comment have been addressed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed

longwave made their first commit to this issue’s fork.

  • longwave committed ea023626 on 10.4.x 
    Issue #3442833 by binoli lalani, vivek panicker, pradhumanjain2311,...

  • longwave committed 4bf739cf on 10.5.x 
    Issue #3442833 by binoli lalani, vivek panicker, pradhumanjain2311,...

  • longwave committed 56da8641 on 11.1.x 
    Issue #3442833 by binoli lalani, vivek panicker, pradhumanjain2311,...

  • longwave committed 9307769b on 11.x 
    Issue #3442833 by binoli lalani, vivek panicker, pradhumanjain2311,...
longwave’s picture

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

Backported down to 10.4.x as an eligible bug fix.

Committed and pushed 9307769b8b1 to 11.x and 56da864154b to 11.1.x and 4bf739cf9e9 to 10.5.x and ea023626eaa to 10.4.x. Thanks!

longwave closed merge request !7721

Status: Fixed » Closed (fixed)

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