This module makes all image paths use protocol relative URLs with double “//“, which is normally great. However, this includes various social share meta tags that include image paths. If the protocol relative URLs from these tags are used to construct social share URLs, the image paths will not work, as the URL has nothing to determine the relative protocol from when it is embedded in a query string. Thus it seems that we need a way to selectively disable the rewriting of image paths for paths that will be used in metatag values.

I went exploring to attempt to find a solution and found that the only contextual information that the rewriting logic has available to it is the file URI. I ended up "solving" the issue by overriding the the cdn.file_url_generator service class and put the following in my new generate() method.

/**
 * {@inheritdoc}
 */
public function generate($uri) {
  if (strpos($this->getRelativeUrl($uri), '/social_share/') !== FALSE) {
    return FALSE;
  }
  return parent::generate($uri);
}

This is obviously very brittle and not ideal, but it fixed the issue for us since all of our images used the same image style. I’m hoping that this issue can help provide a quick fix solution for others and also facilitate discussion of how we could fix this issue in a better way. Or perhaps this isn’t an issue for others due to the way they are handling social share images and if I took an alternate approach I could avoid this issue entirely.

Another potential approach is adding a rewriting disabled property to the FileUrlGenerator class that other modules can toggle on and off. Then the metatag module (or any other module) could disable rewriting while it renders its token values, and then re-enable the rewriting upon conclusion. I haven't tested this approach yet, as I wanted to get some general feedback before I started working on a more robust solution.

It should be noted that the Metatag module strips double “//“ from token values and replaced it with a single/, so even if the issue noted here doesn’t cause an issue with your metatag image paths, they will still be broken. I filed this issue to begin discussion in the metatag issue queue for this semi-related issue.

Comments

adamzimmermann created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Related issues: +#2840751: Protocol-relative URLs are broken

Marking that other issue you filed as a related one.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Title: Disable rewriting for meta tag tokens » Disable CDN's file URL altering for meta tag tokens (because file URL system + token system are not context-aware)
Category: Bug report » Task

Yep, the need for the context in which the file URL will appear is once again haunting us :( This was a problem in Drupal 7, and still is in Drupal 8, because the "file URL" API is identical.

Can you point me to where exactly the image URL is being generated though? There's zero calls to file_create_url() in the metatag module. It pretty much must be the url token that the file module provides. That does this:

        case 'url':
          // Ideally, this would use file_url_transform_relative(), but because
          // tokens are also often used in e-mails, it's better to keep absolute
          // file URLs. The 'url.site' cache context is associated to ensure the
          // correct absolute URL is used in case of a multisite setup.
          $replacements[$original] = file_create_url($file->getFileUri());
          $bubbleable_metadata->addCacheContexts(['url.site']);
          break;

The comment there describes a related problem, which in fact points to exactly the same problem that we're encountering here, with the exact same root cause: the lack of context-awareness. This time in the Token system, where it's arguably an even bigger/more fundamental flaw.

Conclusion: this problem is caused by design flaws in both the file URL system and the token system. Both need to be context-aware, but aren't. This is going to be tough to solve.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Related issues: +#1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send

The comment I quoted was added in #1494670-77: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

So I agree that the CDN module's file URL altering should be context-aware, but it can't be, because the file URL system is not context-aware. We're limited by the abstraction we use.

However, thinking a bit more about this exact use case…

However, this includes various social share meta tags that include image paths. If the protocol relative URLs from these tags are used to construct social share URLs, the image paths will not work, as the URL has nothing to determine the relative protocol from when it is embedded in a query string

I don't think this makes sense. I don't see a reason why it would not be okay to use protocol-relative URLs in meta tags. All protocol-relative URLs in a HTML document must be evaluated with the document's protocol inserted instead. All relative URLs in a HTML document must be evaluated with the document's <base> element's base URL inserted instead. And so on.

<meta> tags live in a HTML document. Hence URLs in <meta> tags must also be evaluated according to these rules.

So:

  1. while the CDN module should support context-dependent file URL altering (and thus use absolute URLs in case of an "e-mail" context), it can't
  2. protocol-relative URLs in <meta> tags are fine, the metatag module should not break them: #2840751: Protocol-relative URLs are broken must be fixed
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Title: Disable CDN's file URL altering for meta tag tokens (because file URL system + token system are not context-aware) » The CDN module should not alter in certain contexts (but can't because file URL system + token system are not context-aware)
Status: Active » Closed (works as designed)

So, to the best of my knowledge, the CDN module is working as correctly as it can, the Metatag has a bug to be fixed. (And turns out the D7 version of the Metatag module had the exact same bug fixed a few months ago: #2791963: Protocol Relative URLs not handled properly.)

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented

Thank you for digging into all of this so thoroughly and quickly.

Can you point me to where exactly the image URL is being generated though? There's zero calls to file_create_url() in the metatag module. It pretty much must be the url token that the file module provides. That does this:

I think this is what you are asking for Drupal\metatag\MetatagToken::replace(). Then this is the line that generates the URL.

$replaced = $this->token->replace($string, $data, $options);

So essentially, what you said about it being the URL token is correct. I was using an image field value token when I discovered this.

while the CDN module should support context-dependent file URL altering (and thus use absolute URLs in case of an "e-mail" context), it can't

I assume you ruled out my idea with a property that we could toggle that would allow other modules to temporarily enable/disable CDN rewriting? I'm curious why that wouldn't potentially work.

protocol-relative URLs in tags are fine, the metatag module should not break them: #2840751: Protocol relative paths are broken must be fixed

They are fine in the context of the page, but we ran into this issue since we use these values to populate image URLs that we put in social share query strings, such as Pinterest or Facebook. The shared "media" URL wouldn't work in the popup that our Pinterest button created.

https://www.pinterest.com/pin/create/button/?url=http://example.com&media=//example.cdndomain.com/cdn/farfuture/mZRxNljyhBA6tIH8ozCr-uQC1ybvCaBRrUW94h7O0gg/1482159252/sites/example.com/files/styles/social_share/public/product/image.png&description=Example description" data-pin-shape="round"></a>

Again, perhaps others aren't taking this approach, and that is why this hasn't been previously reported.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

I assume you ruled out my idea with a property that we could toggle that would allow other modules to temporarily enable/disable CDN rewriting? I'm curious why that wouldn't potentially work.

This would make the CDN module be dependent on global state. Which can cause all sorts of problems. It's a can oil tanker of worms that I'd rather not open.

They are fine in the context of the page, but we ran into this issue since we use these values to populate image URLs that we put in social share query strings

Then the logic over there needs to be updated; it should be able to handle non-absolute URLs, and convert them to absolute URLs when necessary.

Finally, I want to stress #2840751-5: Protocol-relative URLs are broken again — you can see that the same bug existed in the D7 version of Metatag!

adamzimmermann’s picture

adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented

Opening a can of worms is never good. Ya, it definitely seems to makes sense to leave it as given all of that.