Insufficient link validation for external URLs in link widget

Followed the steps mentioned in the issue description to reproduce the issue. However the reported issue seems to be working fine in Drupal 8.x.0. Attached the screenshots for reference.

Related to #2573635: Url::fromUri() should accept protocol-relative URLs - although not entirely the same.

I added some code for it that also validates Punycode.
While working on it I realize this is a deeper problem in the class UrlHelper, but we better solve it here first because that class will need some deeper changes in order to be a really good validation. For example, maybe taking help from Symfony UrlValidator and other improvements.

I opened a new issue, as this validation maybe needs a bigger rewrite: #2691099: Improve external URL validation in many ways, but we can still apply this patch to fix this bug until the other issue is discussed.

Magnet links do not work with parse_url($uri, PHP_URL_HOST);, they need to be parsed by parse_url($uri, PHP_URL_QUERY);

Let's ensure to expand the test coverage.

@dawehner I added tests for Punycode (in UTF-8) and for some malformed URLs.
I could not add a malformed URL like irc:// because in this case there is an exception thrown by the file Url.php at line 303:

    $uri_parts = parse_url($uri);
    if ($uri_parts === FALSE) {
      throw new \InvalidArgumentException("The URI '$uri' is malformed.");
    }

I could not manage to make the test understand it is expected this is a malformed URL. So it still needs a test for irc://.

in a chat with @dawehner at IRC, if I understand correctly, handling irc:// in these tests at the patch #14 would require changes in that class and this is out of scope of this issue.

Then, marking patch #14 as 'needs review'.

removed the test that we are not going to implement.

Looks really good and very nice with a solution for better link validation. I have run the patch against 8.3.x which is the current Drupal 8 Core development and run phpunit test also. Everything is looking really good and therefore I have changed status. Lets get this patch ported right?

$ ../vendor/bin/phpunit --group link -v
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

Runtime:	PHP 7.0.11-1+deb.sury.org~trusty+1 with Xdebug 2.5.0-dev
Configuration:	/www/drupal8/web/core/phpunit.xml

.

Time: 2.8 minutes, Memory: 132.00Mb

OK (1 test, 33 assertions)

1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -39,9 +39,44 @@ public function validate($value, Constraint $constraint) {
   +      // External URLs validation.
   

   The comment seems superfluous because from the following line this is obvious. But maybe some people would like it.

2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -39,9 +39,44 @@ public function validate($value, Constraint $constraint) {
   +          // Converts Punycode to ASCII.
   

   This comment is superfluous.

3. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -39,9 +39,44 @@ public function validate($value, Constraint $constraint) {
   +          // Validates the URL.
   

   This comment is pointless. There are other comments that serve no purpose.

4. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
   @@ -39,9 +39,44 @@ public function validate($value, Constraint $constraint) {
   +        // If cannot do Punycode validation, then just checks the presence of
   +        // a hostname;
   

   This sentence doesn't make much sense.

5. +++ b/core/modules/system/system.install
   @@ -820,6 +820,23 @@ function system_requirements($phase) {
   +    $installation_url = '//secure.php.net/manual/intl.setup.php';
   +    $documentation_url = 'https://secure.php.net/manual/en/book.intl.php';
   

   Why use a relative protocol and https on consecutive lines?

I have read the comments but I am missing the point as to why these changes do not belong in UrlHelper itself. This issue summary needs a rewrite to explain what is actually going on with this patch. And I am wondering if the test coverage covers all paths.

This patch contains :

• A reroll to pass on 8.4.x-dev branch
• Addresses points 1,2,3,4,5
• Does not address the point about moving that to the UrlHelper class.

Added 2 tests for common protocol typos like htp:// and http:/

Good work so far, however I am not sure what is trying to be achieved. Yes the patch will return invalid for irc: but you can still get http://invalid accepted. Is that meant to be the case? Feels like the task needs to be a bit clearer.

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
@@ -24,9 +24,39 @@ public function validate($value, Constraint $constraint) {
+        $is_invalid_protocol =  !in_array($protocol, UrlHelper::getAllowedProtocols());

There is a small issue with double spacing after equals sign.

Also would it not make sense to return the violation if $is_invalid_protocol return true and save doing the additional checks thereafter?

Here is an updated patch and interdiff which implemented as per #26

Error:

Deprecated function: idn_to_ascii(): INTL_IDNA_VARIANT_2003 is deprecated in Drupal\link\Plugin\Validation\Constraint\LinkExternalProtocolsConstraintValidator->validate() (line 50 of core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php).

@see http://php.net/manual/en/function.idn-to-ascii.php

7.2.0 INTL_IDNA_VARIANT_2003 has been deprecated; use INTL_IDNA_VARIANT_UTS46 instead.

Patch fixed.

Perhaps using the Symfony Url validator could be useful? I just added it to our codebase , commit attached, it should be quite easy to adjust it for core. Things I had trouble with:

1. figuring out %value (UrlValidator::validate sets {{ value }} which DrupalTranslator::processParameters translates to %value)
2. LinkUriValidConstraint needs to extend Url otherwise we get a type error, it's not enough to extend Constraint.
3. Minor problem but the try-catch is necessary to avoid the errors caught by other validators throwing an exception.

Found this issue while working on #3151047: Expand LinkWidget test coverage for the Bug Smash Initiative.

The next steps here is to decide on which of the proposed resolutions to implement. Setting to NR for that discussion.

Let's start from #34.

Drupal\Tests\link\Functional\LinkFieldTest::testURLValidation is failing on the test of these internal links

'?example=llama' => '?example=llama',
      '#example' => '#example',,

And I suspect the other tests are failing on $url3 = '#net'; So, it is not playing nicely with internals that don't start with a '/'.

Oh dear, if you are starting from there, you should use https://gist.github.com/chx/e6cf5783b76e3e9637e9d6b756f99cf6 it's much better.

Thanks!

Here is a patch using that constraint.

Edit: No idea what's going on here, just quickly added a + ['scheme' => ''] to make that error go away. I am not going to work on this, I am afraid.

Working on fixing the tests.

Adding A patch for 9.2.x. Made few changes to the tests.
Please review.

Fixed Cspell issues in #47 and adding inter diff.

Patch in #48 wasn't applying and had two PHPCS failures. Resolved and re-rolled.

Previous patch was bogus, please ignore. Corrected patch attached.

Thanks for your work on this. When $url becomes empty you get a notice, so I'd recommend wrapping if (!$url = $urlObject->setAbsolute()->toString()) { return; } like this. As I said before, Smartsheet runs this in production, so we already hit the bugs :)

Fixed the fails of #51 and wrapped $urlObject->setAbsolute()->toString() the line.

Started a review and immediately made a missing interdiff. Looking at it there are out of scope changes as well as changes to a test, which should not be happening. So, I made a new patch.

What I am confused about is that now these two URLs are considered invalid

      ["http://foo.com/unicode_(✪)_in_parens", [$violation_2]],
      ["http://-.~_!$&'()*+,;=:%40:80%2f::::::@example.com", [$violation_2]],

But the page they came from states they are valid.

Perhaps there is something unique about my test setup, but even with the latest patch installed I continue to get an exception rather than a nicely formatted error message when processing an internal URI that is missing the leading slash, such as node/1. I suggest adding a test for this.

Reroll patch for 9.4.x

Currently multiple error messages are getting displayed when `link text` is enabled.
More information is described in issue https://www.drupal.org/project/drupal/issues/3077149

I have rerolled patch #60 by adding a fix for error message display. Apply this patch along with patch from above issue.

Reroll patch for #62

Reroll patch for #62

@gaurav moving to needs review on your behalf, also triggering tests

Did a quick code review and the following issues were observed

Maybe we should wrap this statement as its 167 chars

          $this->context->setNode($this->context->getValue(), $this->context->getObject(), $this->context->getMetadata(), $this->context->getPropertyPath() . '.uri');

The comment is not descriptive and also we could just log the exception message

catch (\Exception $e) {
        // This is not our job, it can be invalid internal path and more.
      }

Try to address comment #66.
Please review.

adding working patch for drupal 10.3.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

I closed #2935307: Link field doesn't sufficiently validate input as a duplicate. I updated the issue summary with info uncovered by that issue.

In the previous patch the URL constraints failed because the child constraint object was passed directly to the parent validator.

Updates in this patch:

1. Created a Symfony Url constraint object and passed it to the parent validate().
2. Added allowed protocols and used the child constraint message.
3. Applied the constraint only for external URLs.
4. Added test coverage for invalid characters, multiple comma-separated URLs, and other edge cases.

This ensures external links are properly validated and the behavior is covered by tests.

@riyas_nr I haven't done an in-depth review of the MR yet, but I notice we're adding a condition for mailto: URLs. There's no test coverage for it though. Is it possible to add a couple of test cases to LinkItemUrlValidationTest::getTestLinks() for them?

Added test coverage for mailto scheme. Moving to NR.

Appears to be some open feedback on the MR

Yes, I'm still working on the review.

Removing the Needs Tests tag because I think that's probably covered now.

I have a couple of problems with the new constraint and validator. So I went back through the history of this issue and discovered that they stem from this code being imported from a custom project.

First, the constraint class name is LinkUriValidConstraint, but its message says "The path %value contains invalid characters." Checking for invalid characters is certainly part of validation, but is that all of it? Or does the Symfony constraint also check URL format? The Symfony docs say "Validates that a value is a valid URL string." It seems like the constraint's label and message need to be updated.

Second, the validator class is doing two separate things:

• Using Symfony's UrlValidator
• Checking mailto URLs

Yet both cases end up with the same constraint message about invalid characters, even though the mailto validation may fail for other reasons. We could add a second message to the constraint, but I think I'd prefer to move this code to separate constraint and validator classes.

I just realized that the issue summary doesn't mention validating email: hrefs at all. So everything related to that is probably out-of-scope.