From 11c00f980589b3fd7f28182bff0dde4c714d79e9 Mon Sep 17 00:00:00 2001 From: tuutti <tuutti@iki.fi> Date: Thu, 27 Jan 2022 11:29:59 +0200 Subject: [PATCH 1/4] Make sure image styles are run through PHP --- helfi_proxy.services.yml | 2 +- src/ProxyManager.php | 65 +++++++++++++++++++++++++++++++++++++-- src/ProxyTrait.php | 1 + src/Selector/Selector.php | 2 +- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/helfi_proxy.services.yml b/helfi_proxy.services.yml index baff543..0171c80 100644 --- a/helfi_proxy.services.yml +++ b/helfi_proxy.services.yml @@ -16,7 +16,7 @@ services: tags: # Must be run before PathProcessorFront (200 weight). - { name: path_processor_inbound, priority: 201 } - # Must to be run after LanguageProcessorLanguage (100 weight). + # Must be run after LanguageProcessorLanguage (100 weight). - { name: path_processor_outbound, priority: 99 } cache_context.site_prefix: diff --git a/src/ProxyManager.php b/src/ProxyManager.php index 8d68716..5829230 100644 --- a/src/ProxyManager.php +++ b/src/ProxyManager.php @@ -138,14 +138,16 @@ private function addAssetPath(string $value) : ? string { * The value. * @param string $separator * The separator. + * @param callable $callback + * The callback to run value through. * * @return string|null * The value. */ - private function handleMultiValue(string $value, string $separator) : ? string { + private function handleMultiValue(string $value, string $separator, callable $callback) : ? string { $parts = []; foreach (explode($separator, $value) as $item) { - $parts[] = $this->addAssetPath(trim($item)); + $parts[] = $callback(trim($item)); } return implode($separator, $parts); } @@ -176,6 +178,53 @@ private function handleSitePrefix(Request $request, string $value) : ? string { return sprintf('%s/%s', $prefix, ltrim($value, '/')); } + /** + * Checks if the given value is an image style. + * + * @param string $value + * The value. + * + * @return bool + * TRUE if image is image style. + */ + private function isImageStyle(string $value) : bool { + return str_contains($value, '/files/styles/'); + } + + /** + * Special handling for image styles. + * + * @param \Drupal\helfi_proxy\Selector\Selector $tag + * The tag. + * @param string $value + * The value with domain added to it. + * + * @return string|null + * The image style url. + */ + private function handleImageStyle(Selector $tag, string $value) : ? string { + if ($tag->multipleValues) { + return $this->handleMultiValue($value, $tag->multivalueSeparator, + function (string $value): string { + return $this->addDomain($value); + }); + } + return $this->addDomain($value); + } + + /** + * Adds domain to relative URL. + * + * @param string $value + * The value. + * + * @return string + * The value with domain added to it. + */ + private function addDomain(string $value) : string { + return sprintf('//%s/%s', $this->getHostname(), ltrim($value, '/')); + } + /** * {@inheritdoc} */ @@ -184,6 +233,12 @@ public function getAttributeValue(Request $request, Selector $tag, ?string $valu if (!$value || $this->isCdnAddress($value)) { return $value; } + + // Image styles need special handling because they need to be run through + // PHP before they are uploaded to CDN. + if ($this->isImageStyle($value)) { + return $this->handleImageStyle($tag, $value); + } // Certain elements are absolute URLs already (such as og:image:url) // so we need to convert them to relative first and then back to // absolute. @@ -202,7 +257,11 @@ public function getAttributeValue(Request $request, Selector $tag, ?string $valu } if ($tag->multipleValues) { - return $this->handleMultiValue($value, $tag->multivalueSeparator); + return $this->handleMultiValue($value, $tag->multivalueSeparator, + function (string $value) : string { + return $this->addAssetPath($value); + } + ); } return $this->addAssetPath($value); diff --git a/src/ProxyTrait.php b/src/ProxyTrait.php index a1a23bf..e8e8da0 100644 --- a/src/ProxyTrait.php +++ b/src/ProxyTrait.php @@ -27,6 +27,7 @@ public function getHostname() : string { } $variables = [ + 'DRUPAL_BACKEND_DOMAIN', 'DRUPAL_ROUTES', 'DRUPAL_REVERSE_PROXY_ADDRESS', ]; diff --git a/src/Selector/Selector.php b/src/Selector/Selector.php index 121d83c..aa7824e 100644 --- a/src/Selector/Selector.php +++ b/src/Selector/Selector.php @@ -15,7 +15,7 @@ class Selector { * @param string $xpath * The tag. * @param string $attribute - * The atribute. + * The attribute. * @param bool $alwaysAbsolute * Whether the given value needs to be converted to relative url. * @param bool $sitePrefix From 152ea5611f92e419ec1782133b26913f4285ce6e Mon Sep 17 00:00:00 2001 From: tuutti <tuutti@iki.fi> Date: Thu, 27 Jan 2022 11:32:56 +0200 Subject: [PATCH 2/4] Fixed tests --- tests/src/Kernel/ProxyManagerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/ProxyManagerTest.php b/tests/src/Kernel/ProxyManagerTest.php index cc9a2b8..29d16b2 100644 --- a/tests/src/Kernel/ProxyManagerTest.php +++ b/tests/src/Kernel/ProxyManagerTest.php @@ -217,7 +217,7 @@ public function testSourceSrcSet() : void { foreach ($values as $value) { $this->assertEquals( - '/test-assets' . $value, + '//' . $this->getHostname() . $value, $this->proxyManager()->getAttributeValue($request, Selectors::get('source'), $value) ); } From 112bf6f9e1e32a09f77426ed3cc2f95d175df8f8 Mon Sep 17 00:00:00 2001 From: tuutti <tuutti@iki.fi> Date: Fri, 28 Jan 2022 11:15:14 +0200 Subject: [PATCH 3/4] UHF-3883: Only convert locally hosted assets to absolute url --- src/ProxyManager.php | 67 ++++++++++++++------------- tests/src/Kernel/ProxyManagerTest.php | 4 +- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/src/ProxyManager.php b/src/ProxyManager.php index 5829230..083f99b 100644 --- a/src/ProxyManager.php +++ b/src/ProxyManager.php @@ -33,32 +33,6 @@ public function __construct(ConfigFactoryInterface $configFactory) { $this->config = $configFactory->get('helfi_proxy.settings'); } - /** - * Converts the given URL to relative. - * - * @param string|null $value - * The value. - * - * @return string|null - * the value. - */ - private function convertAbsoluteToRelative(?string $value) : ? string { - if (!$value) { - return $value; - } - $parts = parse_url($value); - - // Value is already relative. - if (empty($parts['host'])) { - return $value; - } - - if (isset($parts['path'])) { - return $parts['path'] . (isset($parts['query']) ? '?' . $parts['query'] : NULL); - } - return $value; - } - /** * Checks if given URL is hosted from a CDN. * @@ -95,6 +69,24 @@ private function isCdnAddress(?string $value) : bool { return FALSE; } + /** + * Checks whether the asset is hosted locally. + * + * @param string $value + * The path to given asset. + * + * @return bool + * TRUE if asset is local. + */ + private function isLocalAsset(string $value) : bool { + return match(TRUE) { + str_starts_with($value, '/sites'), + str_starts_with($value, '/core'), + str_starts_with($value, '/themes') => TRUE, + default => FALSE, + }; + } + /** * Handles tags with 'alwaysAbsolute' option. * @@ -107,12 +99,24 @@ private function isCdnAddress(?string $value) : bool { * The value. */ private function handleAlwaysAbsolute(Request $request, string $value) : ? string { - $value = $this->convertAbsoluteToRelative($value); + if (!$value) { + return $value; + } + $parts = parse_url($value); - // Skip non-relative values. - if (!str_starts_with($value, '/')) { + // Value is already relative. + if (empty($parts['host']) || empty($parts['path'])) { return $value; } + + // Skip non-local assets. + if (!$this->isLocalAsset($parts['path'])) { + return $value; + } + + if (isset($parts['path'])) { + $value = $parts['path'] . (isset($parts['query']) ? '?' . $parts['query'] : NULL); + } return sprintf('%s%s', $request->getSchemeAndHttpHost(), $this->addAssetPath($value)); } @@ -239,9 +243,8 @@ public function getAttributeValue(Request $request, Selector $tag, ?string $valu if ($this->isImageStyle($value)) { return $this->handleImageStyle($tag, $value); } - // Certain elements are absolute URLs already (such as og:image:url) - // so we need to convert them to relative first and then back to - // absolute. + // Certain elements might be absolute URLs already (such as og:image:url). + // Make sure locally hosted files are always served from correct domain. if ($tag->alwaysAbsolute) { return $this->handleAlwaysAbsolute($request, $value); } diff --git a/tests/src/Kernel/ProxyManagerTest.php b/tests/src/Kernel/ProxyManagerTest.php index 29d16b2..72417a4 100644 --- a/tests/src/Kernel/ProxyManagerTest.php +++ b/tests/src/Kernel/ProxyManagerTest.php @@ -174,9 +174,9 @@ public function testMetaTags() : void { $this->setAssetPath('test-assets'); foreach (['og:image', 'og:image:url'] as $tag) { - $this->assertEquals('http://' . $this->getHostname() . '/test-assets/path/to/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get($tag), 'https://www.hel.fi/path/to/og-image.png')); + $this->assertEquals('http://' . $this->getHostname() . '/test-assets/themes/hdbt/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get($tag), 'https://www.hel.fi/themes/hdbt/og-image.png')); } - $this->assertEquals('http://' . $this->getHostname() . '/test-assets/path/to/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get('twitter:image'), 'https://www.hel.fi/path/to/og-image.png')); + $this->assertEquals('http://' . $this->getHostname() . '/test-assets/themes/hdbt/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get('twitter:image'), 'https://www.hel.fi/themes/hdbt/og-image.png')); } /** From b0bf0a635d5cf5b22e44d1470a700d9cdec71529 Mon Sep 17 00:00:00 2001 From: tuutti <tuutti@iki.fi> Date: Fri, 28 Jan 2022 11:51:47 +0200 Subject: [PATCH 4/4] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 05c2cbd..e247629 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,7 @@ robots_paths: Enable modules with `drush en varnish_purger varnish_purge_tags purge_drush purge_processor_cron purge_queuer_coretags purge_tokens`. -Copy configuration from `helfi_proxy/config/optional` to your `conf/cmi` folder if you enabled this module before Varnish/Purge modules and import configuration changes. +Copy configuration from `helfi_proxy/config/optional` to your `conf/cmi` folder if you enabled this module before Varnish/Purge modules. Add these to your settings.php to use varnish cache: