Url::fromUri($uri)->isExternal() and UrlHelper::isExternal($uri) do not always match

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Review so far has been addressed and this bug is still relevant. Attaching a re-roll for 10.1.x-dev and moving back to NR.

commit check failures

So the commit check failure here is —

/var/www/html/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php:461:41 - Unknown word (Fdvcmxk)

Caused by this added test case —

['data:text/plain;base64,SGVsbG8sIFdvcmxkIQ%3D%3D', TRUE]

I’m not quite sure what I can do about this. Any recommendations? I suppose I could change SGVsbG8sIFdvcmxkIQ to some word that pleases cspell as it doesn’t necessary have to be a real data URL but that seems like a silly workaround.

Attaching an updated patch using base64_encode with a string that doesn't make CSpell unhappy.

That's a great solution! +1

I wonder how to answer #11

I think I answered #11 in #13. I don't have a strong use case beyond wanting to use the features of the link function in Twig with data URLs.

Having said that... I guess this fix is too narrow. The real issue here is that \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols only allows http and https and the docblock on that function makes it all pretty clear why.

I think the bad DX here is just this oddness:

> use \Drupal\Component\Utility\UrlHelper;
> use \Drupal\Core\Url;
> $url = "data:text/plain;base64,SGVsbG8sIFdvcmxkIQ%3D%3D";
= "data:text/plain;base64,SGVsbG8sIFdvcmxkIQ%3D%3D"

> UrlHelper::isExternal($url);
= false

> Url::fromUri($url)->isExternal();
= true

It's inconvenient not to be able to use link with data URLs but now I see this more as a feature request (to support data as an external URL scheme) than a bug. And that perhaps leads to a much more complicated consideration.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

We may want to have a follow up to do more research.

But for this ticket and this request I think it covers this one case, even if it's updated later.

May not want to bother with a 9.5 backport but will need a different patch as str_starts_with is php8 only.

Will see if the committers have any other thought.

We have a polyfill for str_starts_with

I'm still no certain about this one, will ask for some other opinions

larowlan → credited Gábor Hojtsy → .

Discussed this with other core committers who agreed that we don't think isExternal should return TRUE for data urls. One suggestion was to instead add an isData method but then that opened up a whole question of what the security implications are here.

@alexpott also noted that in theory you could add data to filter_protocols in sites/default/services.yml and this might work, but also that there were security concerns around that.

I think we're leaning towards closed works as designed here folks, given you can just output a raw a tag. We don't really want to open a security can of worms around letting content-editors enter data: urls.

I'm going to mark it as such, but if anyone feels strongly the other way, please re-open with counter points and we can discuss further.

Crediting @alexpott and @Gábor Hojtsy who I discussed this with.

larowlan → credited alexpott → .

@larowlan, how about the DX example in #28?

Discussed this with other core committers who agreed that we don't think isExternal should return TRUE for data urls.

So Url::fromUri($url)->isExternal(); should return FALSE instead of TRUE (current behavior), right?

Yeah good point, we might need to repurpose things to address that?

Agreed. Attempting to (coherently) re-work the ticket a bit here and attaching a potential patch for approach.

In this patch I have changed final else dealing with the URI to use the value of UrlHelper::isExternal($uri) instead of just TRUE.

Interested to see how this tests and curious for feedback on if this makes sense and/or how it could be tested? I took a quick look at \Drupal\Tests\Core\UrlTest but couldn't think of a good way to fit anything that isn't just a rehash of \Drupal\Tests\Component\Utility\UrlHelperTest::testIsExternal.

The last submitted patch, 36: 3040688-36.patch, failed testing. View results →

Alright so the first two test fails relate to the test URL invalid://not-a-valid-protocol.

Here's how that URL is treated before #36 patch:

> use \Drupal\Core\Url;
> use \Drupal\Component\Utility\UrlHelper;
> $url = "invalid://not-a-valid-protocol"
= "invalid://not-a-valid-protocol"

> UrlHelper::isExternal($url);
= false

> Url::fromUri($url)->isExternal();
= true

And after #36 patch:

> use \Drupal\Core\Url;
> use \Drupal\Component\Utility\UrlHelper;
> $url = "invalid://not-a-valid-protocol"
= "invalid://not-a-valid-protocol"

> UrlHelper::isExternal($url);
= false

> Url::fromUri($url)->isExternal();
= false

This fails because \Drupal\link\Plugin\Validation\Constraint\LinkExternalProtocolsConstraintValidator::validate prevents external URLs using untrusted protocols only.

So it seems this change goes to far? If so I don't really think there is a logical place to draw a line here. This issue bleeds from a seemingly simple internal vs. external to to a much more complex valid vs. invalid scenario.

Maybe there is some other solution here? Or maybe this really does work as designed? The design is just a bit tough to parse here.