Protocol-relative URLs are broken

.

Note the D7 version of the Metatag module had pretty much exactly the same bug: #2791963: Protocol Relative URLs not handled properly. It was only discovered and fixed in August 2016.

Regexp solution from adamzimmermann works well and fixes my problem with CDN.

@ElgguaDP: Thanks for the patch, I appreciate it! Lets have the testbot take a look.

I've tried testing this today for the https://www.drupal.org/node/2867742 Issue which is directly related.

Applying the patch doesn't work for tokens which return img tags as the new regex is only testing for initial first character:

E.g

<img src="//cdn.fullbundle.com/sites/fullbundle/files/..." typeof="foaf:Image" />

This can be worked around by using this patch and changing the token to point directly to the path, we should be adding support for the base token.

[node:field_image:entity:url] rather than [node:field_image]

What tokens are you using?

Using [node:field_image] (Image Field) causes the wrong url.

https://www.fullbundle.com/cdn.fullbundle.com/sites/...

However this can be fixed for the time being by using [node:field_image:entity:url]

This is in the custom logic for image field handling.

The relevant code is in MetaNameBase::parseImageURL().

Well the Issue isn't limited to just image field handling its relevant to all Protocol Relative URLS. - I'll change the title back.

The patch #9 currently attempts to fix Simple URLs (and i believe this works) but doesn't address image field logic.

This sounds like it's a problem elsewhere then, maybe in CDN itself.

I've added a new patch which solves the issue, but it changes the new regex added in patch #9 to not limit '//' to just the beginning.

I'm not sure how to test the functionality of "Ensure that there are no double-slash sequences due to empty token values." to see if this has been affected by these adjustments.

The new additions added to the attached patch follow a similar fix to that of the patch committed in D7.

There are tests in #2628472: Write tests for image handling that we can build upon, so those need to be fixed first.

@ocastle This solution is not valid for Twitter. Twitter required a full url, including http(s).

• https://developer.twitter.com/en/docs/tweets/optimize-with-cards/guides/...
• • Twitter Cards and Open Graph
  • Full url required
• https://developer.twitter.com/en/docs/tweets/optimize-with-cards/overvie...

<meta property="og:image" content="http://example.com/ogp.jpg"/>

Twitter documentation example:

<meta name="twitter:card" content="summary_large_image">
<meta name="twitter:site" content="@nytimes">
<meta name="twitter:creator" content="@SarahMaslinNir">
<meta name="twitter:title" content="Parade of Fans for Houston’s Funeral">
<meta name="twitter:description" content="NEWARK - The guest list and parade of limousines with celebrities emerging from them seemed more suited to a red carpet event in Hollywood or New York than than a gritty stretch of Sussex Avenue near the former site of the James M. Baxter Terrace public housing project here.">
<meta name="twitter:image" content="http://graphics8.nytimes.com/images/2012/02/19/us/19whitney-span/19whitney-span-articleLarge.jpg">

Here more: #2867742: Interaction with CDN module

@keopx I think the patch addresses the issue when the url is protocol relative.

The business logic of twitter requiring the full non-relative protocol urls should be handled within the metatag_twitter_cards sub module not here.

The existing code is only handling the token value its given not altering urls.

@keopx I'm assuming Open Graph is also going to required full urls with their scheme.

#20: those URLs/docs don't say "full URLs" are required. But sadly, other developers report this same bug in Twitter's API:

• https://twittercommunity.com/t/domain-whitelisted-for-summary-large-imag... — with explicit acknowledgement from Twitter staff that this is a known bug
• https://twittercommunity.com/t/protocol-relative-urls-dont-work/31630
• https://www.questarter.com/q/twitter-image-not-displaying-with-protocol-...
• https://meta.discourse.org/t/twitter-card-image-meta-uses-relative-url/5...

So, created #2925819: Ability to force using a scheme instead of scheme-relative URLs as a potential solution. Looking forward to your feedback. Although I agree very much with @ocastle:

The business logic of twitter requiring the full non-relative protocol urls should be handled within the metatag_twitter_cards sub module not here.

In fact, he posted that after I created #2925819: Ability to force using a scheme instead of scheme-relative URLs. Tempted to close #2925819 already. Anyway, still looking forward to your feedback.

We do need a separate issue for the twitter/OG urls.

The question is where does the responsibility lie?

IMO as the CDN module is outputting the protocol relative urls only because there doesn't seem like a full proof way of determining a 'https/http' scenario its should be handled there.

The other option would be that it should be considered an extension inside the metatag module in favour of adding compatibility to the CDN module 'as its just not possible to determine the scheme' there.

Thoughts anyone?

Thanks @winleers for added links (I read the same).

I think the same as @ocastle, CDN module is "removing" protocol.

IMO as the CDN module is outputting the protocol relative urls only because there doesn't seem like a full proof way of determining a 'https/http' scenario its should be handled there.

That misses the point, I'm afraid. The point is that the CDN module is completely confirming to the HTTP spec, it works in all browsers. It follows the robustness principle: https://en.wikipedia.org/wiki/Robustness_principle.

The actual root cause is that the Twitter Card API is broken (or rather, Twitter's server-side code that processes the metadata that websites provide for it).

Since the Metatag module aims to provide integration with Twitter Card (among others), it should be up to the Metatag module to make the output meet the expectations of the APIs processing it, including working around bugs in those APIs. That's why I agreed with

The business logic of twitter requiring the full non-relative protocol urls should be handled within the metatag_twitter_cards sub module not here.

I proposed #2925819: Ability to force using a scheme instead of scheme-relative URLs not to solve this issue in particular alone, but to make it possible for sites to be explicitly HTTPS-only. If your site were explicitly HTTPS-only, and you'd be using Metatag and you'd be generating Twitter Card markup, then #2925819 would solve the problem reported here.

After a second thought I have to agree with @wim-leers. The CDN module does what it needs to and works, it doesn't have the requirement of having to have the url scheme.

As its Twitter / OG that requires http/https on the URL it should be these modules that validate the inputted value and adjust accordingly by prepending the relevant scheme. I've created an additional task as its not related to this one. - https://www.drupal.org/project/metatag/issues/2925974

It would be good to add support for this issue/bug here.

Thanks @ocastle and @winleers, excuse the mess

@ocastle this patch #2840751-18: Protocol-relative URLs are broken works fine!

I continue on #2925974: Twitter Cards require absolute URLs

@ocastle patch works fine for me #18

Wait, doesn't this conflict with #2925974: Twitter Cards require absolute URLs?

@WinLeers I applied both patches without any conflict :)

Both patches are necessary to works fine (with twitter).

But this patch does its job well

But don't both fix protocol-relative URLs?

They don't do the same no.
The patch here on comment #18 allows for protocol relative URLs
The other patch on https://www.drupal.org/project/metatag/issues/2925974 Forces the urls to NOT be protocol relative.

Patch #18 applied and fixed the issue for me.

I really appreciate the effort resolving this problem. Would someone mind adding a test for this? Thanks.

Lets move the test coverage to #2956996: Add test coverage for relative URL handling and deal with it later.

Committed, thank you all!