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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen created an issue. See original summary.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Active » Needs review
FileSize
2867749-2.patch1.36 KB
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 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies 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
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen 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 CreditAttribution: mErilainen at Wunder 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
CreditAttribution: pfrenssen 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 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies 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
CreditAttribution: dawehner at Chapter Three 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
CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented
Status: Reviewed & tested by the community » Needs review
FileSize
2867749-11-test_only.patch531 bytes
2867749-11.patch1.58 KB
interdiff.txt939 bytes

Sure! Added some documentation.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies 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 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer 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 CreditAttribution: cilefen as a volunteer 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.