Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Try to add a menu item with a scheme-relative URL (for example, "//drupal.org/").
Actual result: "The path '//drupal.org/' is either invalid or you do not have access to it."
Expected result: the menu item should be added.
Comment | File | Size | Author |
---|---|---|---|
#51 | scheme_relative_url-1783278-51.patch | 6.17 KB | sashi.kiran |
#40 | interdiff.txt | 2.67 KB | Mac_Weber |
#40 | scheme_relative_url-1783278-40.patch | 7.85 KB | Mac_Weber |
#38 | system-schema_relative-1783278-38.patch | 7.51 KB | Wim Leers |
#34 | system-schema_relative-1783278-34.patch | 7.07 KB | yobottehg |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedYou can add a relative URL (relative to DocumentRoot) or an absolute URL with full scheme. How do you resolve the relative scheme to a full scheme in a menu link? You can't so this seems to work as designed to me.
Comment #2
Hawk777 CreditAttribution: Hawk777 commentedThe proper way to render this URI:
//drupal.org/
is with this HTML:
<a href="//drupal.org/">link text</a>
This is a scheme-relative (aka protocol-relative, aka network-path-reference) URI. It is not an absolute URI, and it is not a document-root-relative URI. Section 5 of RFC 2396 states that "A relative reference beginning with two slash characters is termed a network-path reference, as defined by in Section 3. Such references are rarely used.". The BNF in that same RFC defines absoluteURI as scheme, colon, (hier_part|opaque_part). Hier_part is defined as (net_path|abs_path)[?query]. Net_path is defined as //authority[abs_path]. Working backwards along the BNF, this means net_path is literally just the URI (excluding query string, since that's part of the relativeURI nonterminal) minus the scheme and colon.
The proper interpretation of a net_path relative URI is that the scheme is taken from the base URI and everything else—both authority (hostname), path, query string, and so on—are taken from the relative URI.
This URI format is supported by all modern browsers and is exceedingly useful to construct links between different hostnames while neither gaining nor losing HTTPS (that is, an HTTPS page will link to an HTTPS target while an HTTP page will link to an HTTP target) and has been recommended by a number of people as a useful way to construct exactly this type of link. It can also be used in JavaScript or CSS cross-domain references, for example to load something like Google Analytics code from Google's HTTP server (for higher performance) when the containing page was loaded over HTTP, but from Google's HTTPS server (to avoid mixed content) when the containing page was loaded over HTTPS.
Alternatively, I could just ask for the menu system to support a format that allows inserting the "s" in an absolute URI at render time, but that seems like an utter waste of effort given that network-path reference relative URIs exist and do the job just fine—if only I were allowed to type one in!
Thanks for the consideration.
Comment #3
rbayliss CreditAttribution: rbayliss commentedYes, please. A side effect of this is letting l() and url() use scheme-relative links too. This would need to be applied to Drupal 8 first, then backported.
This will need a security review.
Comment #4
rbayliss CreditAttribution: rbayliss commentedComment #5
attiks CreditAttribution: attiks commentednice work, can you add tests?
Comment #6
rbayliss CreditAttribution: rbayliss commentedSure. In doing so, I realized that support also needs to be added to valid_url(), so I made that change as well.
Comment #7
tim.plunkettHere is a reroll and a backport.
Comment #8
Jody LynnThis is an issue beyond menu items. I ran into it using a scheme-relative path for a CDN. It's an issue in D6 as well.
Comment #9
Jody LynnPatch for D6 (Since the botched D7 upgrade I can't add a patch?)
Comment #10
tim.plunkettYes, this tests it beyond menu specific code.
(For uploading patches, edit the issue node)
Comment #11
Jody LynnThanks Tim. I am running both the D6 patch and the D7 patch in production. It allows me to use relative schemas with a CDN.
Comment #12
Dave ReidComment #13
tim.plunkettIt seems that #2195983: Support scheme-relative URLs is a more invasive change for D8 to do the same thing. If that indeed goes in, this can just be marked back to D7.
Comment #14
jhedstromEither this or the issue linked in ##1 need a reroll for 8.x.
Comment #17
redndahead CreditAttribution: redndahead commentedRe-roll for d7. Part of this patch went into core with the security release. Not sure if tests are needed anymore since some tests went in with that patch. I left them in just in case.
If it wasn't for the tests this would be a one line patch. So it's greatly simplified.
Comment #18
Jody LynnThe comment doesn't make sense: "# Look for ftp, http, http, feed or scheme relative schemes"
Comment #19
kerby70 CreditAttribution: kerby70 at Blink Reaction (now part of FFW) commentedReroll for D8 attached.
Comment #21
jhedstromNote that after #2455083: Open redirect fixes from SA-CORE-2015-001 need to be ported to Drupal 8 went in,
UrlHelper::isExternal()
now always treats urls starting with//
as external, so this patch needs to take that into account.I'm going to see if I can get this working.
Comment #22
jhedstromComment #24
jhedstromThis should fix the failing tests.
Comment #25
mgiffordPatch no longer applies.
Comment #26
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedThis will also help in other issues, such as: #2573635: Url::fromUri() should accept protocol-relative URLs
Rerolled and also added a test for a schema relative URL at
UnroutedUrlTest.php
Comment #27
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI think this is a better component, as it affects other modules, too.
Comment #28
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedMaybe we need a bigger rewrite for this validation: #2691099: Improve external URL validation in many ways
Comment #30
Wim LeersIndeed. The CDN module for Drupal 8 will only do protocol-relative URLs, to avoid having to render everything twice: once for HTTP and once for HTTPS. And e.g. the image dialog in the text editor uses
#type => managed_file
, which uses#theme => file_link
, which usesUrl::fromUri()
… and since that does not support protocol-relative URIs, it breaks.Two spaces after &&.
Eh… that doesn't seem quite right.
s/scheme/protocol/
… because that is the accepted term.
Missing dash.
s/Schemeless path/Relative URL/
Comment #31
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@Wim Leers, really cool to know we are also helping the CDN module and other contribs here
Comment #32
Wim LeersFirst bug report against the CDN module because of this core bug: #2711529: File upload widget broken when using CDN module, fixed in Drupal 8.1.4: require that version.
Comment #33
yobottehg CreditAttribution: yobottehg commentedComposer patches just told me the patch does not longer apply on 8.1.2 so tagging and setting to needs work
Comment #34
yobottehg CreditAttribution: yobottehg commentedrebased on 8.1.x
Comment #37
Wim LeersThis is a problem that did not exist in #26.
Nor this.
Plus the patch is smaller now, which probably means some hunks were lost, which probably explains the failing tests.
This looks like a bad reroll. I'll do it over.
Comment #38
Wim LeersStraight reroll of #26, with only one tiny trivial conflict.
Tests pass locally.
Comment #39
Wim LeersComment #40
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commented@Wim Leers I see you did not apply your proposed changes at #30 and I was going to apply them myself, but I realize that at
UrlHelperTest
in many places it calls methods and variables 'scheme'. Changing everything there would be a big change and no reason for such, then I did not replace all strings 'scheme' by 'protocol as pointed on item 3, only the ones that makes more sense.1- fixed.
2- I think it is right, why not?
3- not going to fix all entries, as commented above.
4- fixed.
5- These lines are not being patched and these comments are regarding invalid URLs, which makes sense to keep the comment as is.
Comment #41
dawehnerOne thing I'm wondering, should absolute URLs generated in the HTML use a scheme relative URL by default?
Ideally we would adapt the unit test instead of this old web test
Comment #42
Wim LeersThat's a very good question. Technically (well, pedantically to be pedantically precise) they're not an absolute URL anymore, but in practice that's how they behave. So I think this makes sense.
(Note that this would only affect local URLs that we render in an absolute manner. This would not affect external URLs. If it would, that'd result in problems, because we cannot know whether external URLs in fact support HTTP or HTTPS.)
Comment #43
Hawk777 CreditAttribution: Hawk777 as a volunteer commentedI think absolute URLs generated in HTML specifically can probably all be made scheme-relative, but not necessarily all absolute URLs; specifically, I think scheme-relative URLs (like any other relative URLs) are probably not permitted in HTTP Location headers.
Comment #45
dawehnerNeeds work due to the test in #1783278-41: Scheme-relative URL rejected by validation
Comment #50
DamienMcKennaNeeds a reroll.
Comment #51
sashi.kiran CreditAttribution: sashi.kiran at TA Digital commentedReroll that applies to commit 2db400d0bc4fefb.
Below are the conflicts addressed:
drupal\core\lib\Drupal\Core\Url.php
<<<<<<< HEAD
// We support protocol-relative URLs.
if (strpos($uri, '//') === 0) {
$uri_parts['scheme'] = '';
}
elseif (empty($uri_parts['scheme'])) {
=======
// Support for scheme-relative URLs.
if (empty($uri_parts['scheme']) && strpos($uri, '//') === 0) {
$uri_parts['scheme'] = 'relative';
}
if (empty($uri_parts['scheme'])) {
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb
drupal\core\tests\Drupal\Tests\Component\Utility\UrlHelperTest.php
<<<<<<< HEAD
$url_schemes = ['http', 'https', 'ftp'];
$data = [];
=======
$url_schemes = array('http://', 'https://', 'ftp://', '//');
$data = array();
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb
drupal\core\tests\Drupal\Tests\Core\UnroutedUrlTest.php
<<<<<<< HEAD
$this->expectException(\InvalidArgumentException::class);
$url = Url::fromUri($uri);
=======
Url::fromUri($uri);
>>>>>>> Applying patch from issue 1448e883d1a3 comment 400d0bc4fefb
Reroll is done w.r.t 8.9.x
Comment #52
sashi.kiran CreditAttribution: sashi.kiran at TA Digital commentedComment #53
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #56
quietone CreditAttribution: quietone as a volunteer commentedThis is a duplicate of #2195983: Support scheme-relative URLs. In a comment-14 over there from Feb 2014 sun stated that that issue was the preferred solution "because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there."
I'll work on combining the patches and post it over in the other issue and move credit etc.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedClosing this as a duplicate of #219598: Query trying to use invalid field in node_access table despite the fact that this is the older issue. In that being the older issue in #14 sun states that the other solution is the preferred. "because it also removes that apparently hard-coded list of schemes in the preg_match() of isValid(), which has absolutely no business in there. :)"
The work here has been added to that patch over there and credit moved.