Handle alternate domains in filter_html_image_secure

1. +++ b/core/modules/filter/config/schema/filter.schema.yml
   @@ -69,6 +69,13 @@ filter_settings.filter_html:
   +    filter_html_image_secure_domains:
   +      type: string
   

   The key should just be "domains" and the type should be an array (which is type 'sequence', unless I'm mistaken)

2. +++ b/core/modules/filter/filter.module
   @@ -1065,13 +1065,28 @@ function _filter_html_image_secure_process($text) {
   +      . implode('|', array_map('preg_quote',
   

   preg_quote() always needs the second parameter specified; i.e., the PCRE delimiter being used by the pattern.

3. +++ b/core/modules/filter/filter.module
   @@ -1065,13 +1065,28 @@ function _filter_html_image_secure_process($text) {
   +          array_map('trim', preg_split('/(\r\n?|\n)/', $domains))
   

   This trim is obsolete after changing the configuration to an array.

4. +++ b/core/modules/filter/filter.module
   @@ -1065,13 +1065,28 @@ function _filter_html_image_secure_process($text) {
        $src = $image->getAttribute('src');
        // Transform absolute image URLs to relative image URLs: prevent problems on
        // multisite set-ups and prevent mixed content errors.
   -    $image->setAttribute('src', file_url_transform_relative($src));
   +    if (isset($domains_regexp)) {
   +      $image->setAttribute('src', preg_replace($domains_regexp, '', $src));
   +    }
   +    else {
   +      $image->setAttribute('src', file_url_transform_relative($src));
   +    }
   

   With this change, I wonder whether we shouldn't switch this code entirely to use an approach that is based on parse_url(), so that we can simply check the parsed 'host' against the allowed domains?

This looks much better now. Mostly minor issues below, but a major task is to add (unit) test coverage for all possible permutations of $domains vs. $src. (I think we can skip test coverage for the UI configuration, so just focus on the filter processing.)

1. +++ b/core/modules/filter/config/schema/filter.schema.yml
   @@ -69,6 +69,16 @@ filter_settings.filter_html:
   +  label: 'Filter HTML image secure'
   

   Hm, that's the filter's internal name only, which doesn't appear anywhere else as label.

   The plugin uses "Restrict images to this site"

   I don't know what the policy for such config schema plugin cases is...

2. +++ b/core/modules/filter/config/schema/filter.schema.yml
   @@ -69,6 +69,16 @@ filter_settings.filter_html:
   +          label: 'Value'
   

   'Domain name', no?

3. +++ b/core/modules/filter/filter.module
   @@ -1057,7 +1057,7 @@ function _filter_html_escape($text) {
   +function _filter_html_image_secure_process($text, $domains) {
   

   Let's type-hint $domains as an array.

4. +++ b/core/modules/filter/filter.module
   @@ -1070,8 +1070,20 @@ function _filter_html_image_secure_process($text) {
   +      $url = parse_url($src);
   

   FYI: The backport to D7 will need error-suppression here; the PHP warning on malformed input was removed in PHP 5.3.3.

5. +++ b/core/modules/filter/filter.module
   @@ -1070,8 +1070,20 @@ function _filter_html_image_secure_process($text) {
   +      // If the URL has a host, check if it is in the list of allowed domains.
   +      if (isset($url['host']) && isset($url['path']) && in_array($url['host'], $domains)) {
   

   Hm. Why is there no else condition?

   I also think we want to use a strict comparison for in_array() (i.e., 3rd param: TRUE).

6. +++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterHtmlImageSecure.php
   @@ -25,8 +29,37 @@ class FilterHtmlImageSecure extends FilterBase {
   +      '#title' => $this->t('Alternate domains'),
   

   Shouldn't this be labeled "Allowed domains"?

7. +++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterHtmlImageSecure.php
   @@ -25,8 +29,37 @@ class FilterHtmlImageSecure extends FilterBase {
   +      '#description' => $this->t('If your site reachable from muliple domains, such as example.com and www.example.com, list them one per line.'),
   

   As a user, it's not clear to me what happens if I don't fill in any domains.

   "If left empty, only image URLs that are equal to the current domain are allowed."

   (my addition reads a bit long and complex; let's find a way to shorten + simplify it)

8. +++ b/core/modules/filter/lib/Drupal/filter/Plugin/Filter/FilterHtmlImageSecure.php
   @@ -25,8 +29,37 @@ class FilterHtmlImageSecure extends FilterBase {
   +      if ($configuration['settings']['domains'] === '') {
   

   This comparison should use a trim() on its own already, so that pure white-space is not interpreted as a value.

I ran into this recently as well. It's often not feasible to back port a gigantic filesystem of changing content between environments. It looks like I'm really more after #2825598: file_url_transform_relative() only works with current request's host, but found this issue first. I'm not sure if these issues should be combined or if that one should soley focus on file_url_transform_relative and this one on adding a whitelist of domains. I think for drupal.org's use case, fixing file_url_transform_relative would be fine?

We would need an update hook and update tests to test the configuration schema change.

I re-rolled @drumm's patch in #16 based on 8.7.x. There were not any differences detected by interdiff as a 3-way merge worked (curl https://www.drupal.org/files/issues/2257291_3.diff | git apply -3 --index).

This should be flipped back to Needs work after the test run if it doesn't fail.

Patch in #28 fails in Drupal 8.8, both on existing and on a fresh install:

ResponseText: Doctrine\Common\Annotations\AnnotationException: [Semantical Error] Couldn't find constant domains, class Drupal\filter\Plugin\Filter\FilterHtmlImageSecure. in Doctrine\Common\Annotations\AnnotationException::semanticalError() (line 54 of /var/www/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php).

Edit: To clarify I got this during install.