Problem/Motivation
The link widget seems to rely purely on native browser side validation for checking the validity of external URLs. When an invalid URL such as "http:" (on Firefox) or "irc:" (on Chromium and Firefox) is used then these malformed URLs are accepted.
Steps to replicate:
- Add a link field on the "Article" node type with the option "Allowed link type" set to "External links only".
- Create an article, enter "http:" or "irc:" for the URL, and submit the form.
- Result: the invalid URL is accepted.
This was originally reported by idimopoulos.
Proposed resolution
There are two proposals
1) Add validation for punycode and magnet links in /core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator
or
2) Use the Symfony Url Validator, #34. This was proposed 6 years ago in #295021: filter_var() with FILTER_VALIDATE_URL accepts malformed URLs and rejects not all valid URLs and 4 years ago in #2691099: Improve external URL validation in many ways
Remaining tasks
Choose a proposed resolution and if the 1) then decide if these changes should be in UrlHelper See #21
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#67 | interdiff_64-67.txt | 1.83 KB | pradhumanjain2311 |
#67 | 2652236-67.patch | 10.45 KB | pradhumanjain2311 |
#64 | 2652236-64.patch | 10.27 KB | gaurav_manerkar |
#55 | 2652236-55.patch | 10.04 KB | quietone |
#55 | interdiff-54-55.txt | 4.47 KB | quietone |
Comments
Comment #2
pfrenssenComment #3
pfrenssenComment #4
pfrenssenComment #5
kavithad CreditAttribution: kavithad as a volunteer commentedComment #6
kavithad CreditAttribution: kavithad as a volunteer commentedFollowed 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.
Comment #7
pfrenssen@kavithad, thanks for testing!
However you seem to have tested with valid URLs like "irc://irc.server.org/channel", not the invalid URLs such as "irc:" that are mentioned in the issue summary.
Comment #8
swentel CreditAttribution: swentel commentedRelated to #2573635: Url::fromUri() should accept protocol-relative URLs - although not entirely the same.
Comment #9
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI 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 SymfonyUrlValidator
and other improvements.Comment #10
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI 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.
Comment #12
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedMagnet links do not work with
parse_url($uri, PHP_URL_HOST);
, they need to be parsed byparse_url($uri, PHP_URL_QUERY);
Comment #13
dawehnerLet's ensure to expand the test coverage.
Comment #14
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@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 fileUrl.php
at line 303: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://
.Comment #15
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedin 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'.
Comment #18
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedremoved the test that we are not going to implement.
Comment #20
pmchristensen CreditAttribution: pmchristensen commentedLooks 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?
Comment #21
cilefen CreditAttribution: cilefen as a volunteer commentedThe comment seems superfluous because from the following line this is obvious. But maybe some people would like it.
This comment is superfluous.
This comment is pointless. There are other comments that serve no purpose.
This sentence doesn't make much sense.
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.
Comment #23
pguillard CreditAttribution: pguillard commentedThis patch contains :
Comment #24
pguillard CreditAttribution: pguillard commentedAdded 2 tests for common protocol typos like htp:// and http:/
Comment #26
chenderson CreditAttribution: chenderson at Headscape commentedGood 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.
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?
Comment #27
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is an updated patch and interdiff which implemented as per #26
Comment #30
super_romeo CreditAttribution: super_romeo commentedError:
@see http://php.net/manual/en/function.idn-to-ascii.php
Patch fixed.
Comment #34
Ghost of Drupal PastPerhaps 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:
%value
(UrlValidator::validate
sets{{ value }}
whichDrupalTranslator::processParameters
translates to%value
)LinkUriValidConstraint
needs to extendUrl
otherwise we get a type error, it's not enough to extendConstraint
.try-catch
is necessary to avoid the errors caught by other validators throwing an exception.Comment #36
quietone CreditAttribution: quietone as a volunteer commentedFound 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.
Comment #37
jibranLet's start from #34.
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedDrupal\Tests\link\Functional\LinkFieldTest::testURLValidation is failing on the test of these internal links
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 '/'.Comment #40
Ghost of Drupal PastOh dear, if you are starting from there, you should use https://gist.github.com/chx/e6cf5783b76e3e9637e9d6b756f99cf6 it's much better.
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedThanks!
Here is a patch using that constraint.
Comment #43
Ghost of Drupal PastEdit: 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.Comment #44
quietone CreditAttribution: quietone as a volunteer commentedWorking on fixing the tests.
Comment #47
vakulrai CreditAttribution: vakulrai as a volunteer and at QED42 for QED42 commentedAdding A patch for 9.2.x. Made few changes to the tests.
Please review.
Comment #48
vakulrai CreditAttribution: vakulrai as a volunteer and at QED42 for QED42 commentedFixed Cspell issues in #47 and adding inter diff.
Comment #50
logickal CreditAttribution: logickal at Third and Grove commentedPatch in #48 wasn't applying and had two PHPCS failures. Resolved and re-rolled.
Comment #51
logickal CreditAttribution: logickal at Third and Grove commentedPrevious patch was bogus, please ignore. Corrected patch attached.
Comment #53
Ghost of Drupal PastThanks 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 :)Comment #54
Meenakshi_j CreditAttribution: Meenakshi_j at Srijan | A Material+ Company for Drupal India Association commentedFixed the fails of #51 and wrapped $urlObject->setAbsolute()->toString() the line.
Comment #55
quietone CreditAttribution: quietone as a volunteer commentedStarted 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
But the page they came from states they are valid.
Comment #57
Gribnif CreditAttribution: Gribnif commentedPerhaps 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.Comment #60
yivanov CreditAttribution: yivanov as a volunteer commentedReroll patch for 9.4.x
Comment #62
gaurav_manerkar CreditAttribution: gaurav_manerkar commentedCurrently 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.
Comment #63
gaurav_manerkar CreditAttribution: gaurav_manerkar commentedReroll patch for #62
Comment #64
gaurav_manerkar CreditAttribution: gaurav_manerkar commentedReroll patch for #62
Comment #65
ameymudras CreditAttribution: ameymudras at Salsa Digital commented@gaurav moving to needs review on your behalf, also triggering tests
Comment #66
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedDid a quick code review and the following issues were observed
Maybe we should wrap this statement as its 167 chars
The comment is not descriptive and also we could just log the exception message
Comment #67
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedTry to address comment #66.
Please review.