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

Command icon 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

mattsqd created an issue. See original summary.

swentel’s picture

Status: Needs work » Active

You'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.

swentel’s picture

Started coding on this. Will add tests to make sure we don't miss anything stupid here :)

swentel’s picture

Status: Active » Needs review
swentel’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

swentel’s picture

Status: Fixed » Closed (fixed)