When UrlHelper::parse() is parsing an internal URI which contains a URL in the query arguments then it throws the following notice:

Notice: Undefined offset: 1 in Drupal\Component\Utility\UrlHelper::parse() (line 151 of core/lib/Drupal/Component/Utility/UrlHelper.php).

This is caused by the method checking if the URI contains the string :// without verifying if this is part of the scheme, or in the query arguments. This causes the URI to be mistakenly considered an external URL and the wrong parse code path is entered.

CommentFileSizeAuthor
#11 interdiff.txt939 bytespfrenssen
#11 2867749-11.patch1.58 KBpfrenssen
#11 2867749-11-test_only.patch531 bytespfrenssen
#2 2867749-2-test_only.patch531 bytespfrenssen
#2 2867749-2.patch1.36 KBpfrenssen

Comments

pfrenssen created an issue. See original summary.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Active » Needs review
StatusFileSize
new 2867749-2.patch1.36 KB
new 2867749-2-test_only.patch531 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2867749-2-test_only.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented 
+++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
@@ -142,7 +142,9 @@ public static function parse($url) {
-    if (strpos($url, '://') !== FALSE) {
+    $scheme_delimiter_position = strpos($url, '://');
+    $query_delimiter_position = strpos($url, '?');
+    if ($scheme_delimiter_position !== FALSE && ($query_delimiter_position === FALSE || $scheme_delimiter_position < $query_delimiter_position)) {

Nice catch and nice that has also test coverage.

I wonder if we cannot write the change in a more cleaner way?

  list($url_to_check,) = explode('?', $url, 2);
  if (strpos($url_to_check, '://') !== FALSE) {
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs work » Needs review
pfrenssen’s picture

pfrenssen
Location Sofia
commented

I think my version is more readable actually. I had an earlier version that was more compact but it wasn't so clear why the check was being done so I made this more verbose version.

merilainen’s picture

merilainen commented

Steps to reproduce?
Also I was reading on the backport policy document https://www.drupal.org/core/backport-policy which says that bug reports should be against the latest dev-branch which is now 8.4.x-dev, right? Not experienced with the 8.x minor update cycle, so bear with me...

pfrenssen’s picture

pfrenssen
Location Sofia
commented

To reproduce:

$ drush php
Psy Shell v0.7.2 (PHP 7.1.3 — cli) by Justin Hileman
>>> \Drupal\Component\Utility\UrlHelper::parse('?referrer=http://localhost');
PHP error:  Undefined offset: 1 in /home/pieter/v/drupal/core/lib/Drupal/Component/Utility/UrlHelper.php on line 150

Or, through the UI, create a link field that accepts internal links, and enter ?url=http://www.drupal.org.

Since this is a bugfix this is still OK for 8.3.x, feature requests go to 8.4.x.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

I think my version is more readable actually

Probably my proposed alternative is more a matter of perception, subjectivity and taste :)

The provided fix is correct and clear enough. Also we see the test proving the bug and we're covered. RTBC.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
@@ -142,7 +142,9 @@ public static function parse($url) {
+    $scheme_delimiter_position = strpos($url, '://');
+    $query_delimiter_position = strpos($url, '?');
+    if ($scheme_delimiter_position !== FALSE && ($query_delimiter_position === FALSE || $scheme_delimiter_position < $query_delimiter_position)) {

Is it just me or could this need some additional documentation?

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2867749-11-test_only.patch531 bytes
new 2867749-11.patch1.58 KB
new interdiff.txt939 bytes

Sure! Added some documentation.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC.

The last submitted patch, 11: 2867749-11-test_only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
cilefen’s picture

cilefen commented
Issue summary: View changes

  • cilefen committed 3c72c97 on 8.4.x 
    Issue #2867749 by pfrenssen, claudiu.cristea: Parsing an URL with...

  • cilefen committed bc7a744 on 8.3.x 
    Issue #2867749 by pfrenssen, claudiu.cristea: Parsing an URL with...
cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Fixed

Committed 3c72c97 and pushed to 8.4.x, backported as bc7a744 and pushed to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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