Problem/Motivation
filter_html_image_secure
currently uses file_url_transform_relative()
, which only recognizes the host the request came in on. We are using the backport of this on Drupal.org, and it got us into some trouble, #1737022: Host Whitelist.
If the request comes in via Drush, there isn't a hostname. This may or may not apply in D8, since the method for getting the Request's hostname is completely different. The big problem is that the incorrect result is cached.
A more common problem is that sites can live at multiple hostnames. Drupal.org will be moving to www.drupal.org, a requirement for #2238131: Drupal.org CDN deployment notes. We should allow image URIs from either domain. Making https://drupal.org/…
suddenly invalid for images would not be a nice thing to do.
Proposed resolution
Support a whitelist of domains in the filter plugin.
Remaining tasks
- Question: Should this use trusted host names by default?
- Needs tests including an update test as filter config will change.
- Needs an update
User interface changes
Filter will have new configuration options and translated strings.
API changes
Internal function changes.
Data model changes
Filter plugin will have schema change.
Release notes snippet
TBD.
Original report by drumm
Comment | File | Size | Author |
---|---|---|---|
#28 | drupal-2257291-allow-whitelist-secure-images-28.patch | 3.31 KB | mradcliffe |
#16 | 2257291.diff | 3.3 KB | drumm |
Comments
Comment #1
drummAttached is an initial patch. I'm interested in knowing if this is a good approach. If so, we can proceed with any necessary changes and tests.
Getting a firm plan for this issue will let us proceed with the backport #1737022: Host Whitelist and hopefully get Drupal.org served via a CDN before DrupalCon Austin.
Comment #2
drummI should point out that "incorrect" values for allowed domains are not a problem. The URIs are rewritten to be relative, and checked against the filesystem, so they will serve correctly in the end.
Comment #3
sunThe key should just be "domains" and the type should be an array (which is type 'sequence', unless I'm mistaken)
preg_quote()
always needs the second parameter specified; i.e., the PCRE delimiter being used by the pattern.This trim is obsolete after changing the configuration to an array.
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?Comment #4
drummAgreed this can switch to
parse_url()
. That should make the code more readable.Re #1: is there a good example elsewhere in core for the UI to go with an array/sequence on strings in configuration? Does the splitting on newlines happen elsewhere, or is there a matching form widget for multiple text fields?
Comment #5
drummAttached is an updated patch. Still needs feedback - is this a good approach?
Comment #6
drummComment #7
sunThis 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.)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...
'Domain name', no?
Let's type-hint
$domains
as an array.FYI: The backport to D7 will need error-suppression here; the PHP warning on malformed input was removed in PHP 5.3.3.
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).Shouldn't this be labeled "Allowed domains"?
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)
This comparison should use a
trim()
on its own already, so that pure white-space is not interpreted as a value.Comment #8
drummAttached is a patch with the corrections from #7.
Still needs new tests.
Comment #9
drummComment #10
drummThis follows the behavior of
file_url_transform_relative()
, a matching domain is converted to a relative URI. Either way, the next section of code checks that the image is in the filesystem. Non-relative URIs will fail that check.Comment #11
m1r1k CreditAttribution: m1r1k commentedComment #12
drummThis could use a reroll.
Comment #15
drummI missed applying a chunk of this patch. The configuration UI is still missing.
(I'm using this issue to test #2518056: Need the ability to cancel a test job, so I'm setting this to Needs review so tests can at least start.)
Comment #16
drummComment #20
mgiffordComment #28
mradcliffeI 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.
Comment #33
R_H-L CreditAttribution: R_H-L commentedPatch 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.