Problem/Motivation
The module homepage warns that you must configure the whitelist in order for it to process images, but images can show with zero configuration.
Steps to reproduce
* Install and enable module
* (Do no configuration, although saving the config page without making changes will not change this)
* Add this to page.html.twig
{% set test = {
'#theme': 'imagecache_external',
'#style_name': 'large', (or an image style ID that exists for you)
'#uri': 'https://www.google.com/images/branding/googlelogo/2x/googlelogo_light_color_272x92dp.png'
} %}
{{ test }}
Proposed resolution
A) Remove the warning OR B) update the code.
I'm not sure the direction for this one.
If one goes with B since there may be people that did not read the warning and they did not configure the module, now if the code is patched, their site may not work correctly anymore.
If one goes with A, the module info AND the admin interface should be clear that its default is 'Blacklist'
The reason this issue is happening is because for the function imagecache_external_validate_host, the default return value of $config->get('imagecache_external_hosts') out of the box is an array with a single empty string value (like array['']).
When that empty string is used in the conditional, it is the same as doing:
preg_match('/\.?/', 'google.com'))
Which is a successful match.
For the preg_match(), it's also a good idea to preg_quote any variables that are put into the pattern (first parameter), otherwise any special characters are treated as such. For example, if one used google.com as a whitelist domain, the '.' is not a period, it's 'any character'.
Remaining tasks
Either make is clear the default is blacklist or fix the bug that makes it work like blacklist if nothing is configured.
User interface changes
API changes
Data model changes
Issue fork imagecache_external-3425517
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
swentel commentedYou're right, it's confusing, especially since whitelist defaults to TRUE in config.
I'd go to actually block the urls if nothing is configured in the textarea. We have to add an upgrade path for existing sites where, in many cases, the checkbox will be toggled probably without additional hosts. In that case, the upgrade path needs to turn of the checkbox.
Comment #4
swentel commentedStarted coding on this. Will add tests to make sure we don't miss anything stupid here :)
Comment #5
swentel commentedComment #6
swentel commentedComment #8
swentel commented