"Cache directory creation problem" and "source image could not be found" with v1.7

[davidromani]

Hello.

After upgrade this bundle from 1.6 to 1.7 my project doesn't works well.

In my local environment everything works well. My "web/media/cache" dir exists with all permissions. But when I deploy my app to a production server with Capifony, it seems that this bundle can't create "cache" directory.

Is there some change related with directory creation in this update? In my deployment server I only have "web/media" dir with all permissions, the "web/media/cache" is not deployed...

[alexwilson]

I don't believe that there's been any changes to the creation (or lack thereof) of the web/media/cache directory. I'll take a look to double-check...

(For reference - 1.6.0...1.7.0)

[adaniloff]

Hello there,

+1 with davidromani.

We got a problem with an update in the Binary/Loader/FileSystemLoader.php file.
I got the "Source image invalid "%s" as it is outside of the defined root path" error message since 1.7 upgrade.

Tried to downgrade to 1.6.* and it worked fine again.

It seems to be an environment problem (maybe because of symlinks), we're actually trying to figure it out; because it works perfectly on our local env but does not on production.

[raphaChoquet]

Hello,

The error is due to symbolic links created by 'assets:install --symlink'.
I made a pull request to fix it :
#848

[picks44]

Thanks @raphaChoquet! Hope there'll be a new release with this fix soon, I had to downgrade to 1.6.

[alexwilson]

Unfortunately this PR has broken the testing suite and so I'm not comfortable with merging and releasing it just yet. However I'll prioritise it, so as soon as we can be confident it won't break then we will create a patch level release.

[raphaChoquet]

Yes, tests has broken because program doesn't replace paths containing '../' in $path.
I think we need to replace realpath function by other logic to keep symbolic link.

[robfrawley]

@raphaChoquet We've covered this in multiple issues prior: you need to set a proper data_root value in your bundle configuration to allow for the real path once the symbolic link is resolved. The PR breaks prior bug fixes and introduces security concerns. Alternatively, (to setting a data_root), on Linux, you can use bind mounts.

[alexwilson]

Thanks for the clarification @robfrawley, I remember that being a particular pain-point in earlier issues. I wasn't actually aware of the need to configure data_root either, so I wonder if it's worth us documenting that to avoid more issues of this nature?

[robfrawley]

@antoligy It is alreADY documented: https://github.com/liip/LiipImagineBundle#images-outside-data-root

We could possibly highlight it more and add some clarity. If I remember correctly, that existed almost exactly as it currently is before I submitted the documentation re-write; we could expand upon that and provide some additional clarity, specifically point to the common error message(s) people get when they do not have a proper data_root value set.

[robfrawley]

Also, we don't mention the difficulties people encounter when they utilize symbolic links, so perhaps that should be explicitly spelled out in the documentation as well. I won't have any time to work on it immediately, but if no one else gets around to it before, I'll happily expand on that sometime this coming weekend.

[robfrawley]

@davidromani @adaniloff @raphaChoquet @picks44 The issue you are encountering is the following: prior releases didn't consistently chroot the file within the resolved root path, which was the intended behavior, it was simply written in an easily (and regularly) bypassed manned. Moving forward, the code requires images reside in the root configured. This means that if an image lives in /path/a and you have the following symbolic link /path/symlink pointing to /path/a, the data_root bundle configuration value must be set to /path/a. Can anyone confirm that this is actually the issue they are encountering, and that setting a proper data_root resolves your issue?

See the following issues for additional reference: issue-780, issue-826, and issue-783.

[adaniloff]

@robfrawley Reading the doc, it seems that we can only configure one data_root. It could be a pain in the *ss. For example, I got src/Foo1Bundle and src/Foo2Bundle having their own resources, how should I do?

Configure all the SRC/ as data_root path? Because it seems bad to do so to me.

Also, since it's an update from 1.6 to 1.7 only, I think the presence of this parameter should be checked and forced. Otherwise, a lot of people will one day composer update their production, and have this trouble without understanding why.

I may be wrong so please explain me the how and why if so.

Cheers

[robfrawley]

@adaniloff Reference my latest comment from the PR. Would allowing an array of data roots solve your problem while ensuring the security of the system?

[robfrawley]

@davidromani @adaniloff @raphaChoquet @picks44

Please try patching your Imagine Bundle vendor files with #851. This PR allows for configuring more than one data_root value and may serve as a solution for your issue.

To apply the patch, ensure you've upgraded liip/imagine-bundle to version 1.7 and then pipe the PR diff contents to the patch executable while within the respective vendor directory.

composer require "liip/imagine-bundle:~1.7"
cd vendor/liip/imagine-bundle
curl -L https://github.com/liip/LiipImagineBundle/pull/851.diff | patch -p1
cd ../../..

After patching this bundle you will be able to define multiple data roots for FileSystemLoader using an array of paths, as shown in the example configuration below.

# app/config/config.yml

liip_imagine:
  loaders:
    default:
      filesystem:
        data_root: 
          - "%kernel.root_dir%/../src/Bundles/A/Resources"
          - "%kernel.root_dir%/../src/Bundles/B/Resources"
          - "%kernel.root_dir%/../src/Bundles/C/Resources"
          // ...etc...

These paths should be the resolved path of the corresponding symbolic links residing within your web directory. For example, given the following symbolic links

"%kernel.root_dir%/../web/bundles/a" -> "%kernel.root_dir%/../src/Bundles/A/Resources/public"
"%kernel.root_dir%/../web/bundles/b" -> "%kernel.root_dir%/../src/Bundles/B/Resources/public"
"%kernel.root_dir%/../web/bundles/c" -> "%kernel.root_dir%/../src/Bundles/C/Resources/public"

You should add the following paths to your YML configuration under the data_root key

"%kernel.root_dir%/../src/Bundles/A/Resources/public"
"%kernel.root_dir%/../src/Bundles/B/Resources/public"
"%kernel.root_dir%/../src/Bundles/C/Resources/public"

Hopefully, this will allow for sane security while providing a means to support symbolic links that point to multiple locations outside a single root path. Let us know if this resolves your issue. Thanks!

[adaniloff]

@robfrawley Thanks for your replies and explanations. I can't try it right now as I won't have my computer with me until Monday.

Gonna keep you informed about #851.

I'd like to know if it is also possible to add a user notice (or warning) when upgrading from 1.6.x to 1.7+ as following :

• the user do a composer update that change the version of this bundle
• in the DependencyInjection/Factory/Loader/FileSystemLoaderFactory.php, you check the presence of an optionnal parameter (DependencyInjection/Configuration.php should also be change according to accept a this parameter as an optionnal boolean) like that :

<?php
/** Inside ... */
class FileSystemLoaderFactory implements LoaderFactoryInterface
{
    /**
     * {@inheritdoc}
     */
    public function create(ContainerBuilder $container, $loaderName, array $config)
    {
        $loaderDefinition = new DefinitionDecorator('liip_imagine.binary.loader.prototype.filesystem');
       /** Just that part ... */ 
       if (false == isset($config['data_root']) || empty($config['data_root'])
            && (false == isset($config['disable_notice']) || true !== (bool)$config['disable_notice'])) {
            trigger_error('You must configure at least one data_root path or disable notice from 1.7', E_USER_NOTICE);
        }
        $loaderDefinition->replaceArgument(2, $config['data_root']);
        $loaderDefinition->addTag('liip_imagine.binary.loader', array(
            'loader' => $loaderName,
        ));
        $loaderId = 'liip_imagine.binary.loader.'.$loaderName;
        $container->setDefinition($loaderId, $loaderDefinition);
        return $loaderId;
    }

I made this up quickly because I won't be able to do it properly before, at least Monday as I said. This example is just to show you what I'd want, if possible :).

The idea, is to explicitly show the developer the need to check the doc about the data_root arguments, without impacting the production environments. Also, the optionnal parameter is here to let a developer bypass the configuration, without triggering error.

My concern is, as the images are cached, when a dev. will composer update the dev env., he won't always see the problem occurs because Liip won't resolve already cached images. So he could update the production env. and then spend to much time trying to understanding what's up (if a new image is resolved like 1 week after having deployed, it could really be surprising to a developer I think).

I hope I made myself clear, it's a bit late over there and english is not my native language.

Regards.

[robfrawley]

@adaniloff I absolutely understand what you are trying to convey in terms of providing notice to the user, but I'm not sure your solution is indicative of the broader usage of this bundle: most people don't set data_root and never need to do so. As such, IMHO, it seems counter-intuitive to require they set a value for this option (when they normally would not) or set a configuration option whose sole purpose is to silence a notice (the disable_notice key in your example).

Perhaps adding a composer post-update hook for this upgrade cycle that provides a large, explicit notice to the user in the CLI output? Or something else that replies on Composer and doesn't affect the normal configuration handling of this bundle? Any composer buffs out there that may have an alternate solution?

[adaniloff]

@robfrawley
I was convinced that this would be the case of most of the people (I mean to use symlinks for images etc), but reading your previous message it seems not to be.

A composer hook could do the trick I guess. Outputing a big warning / danger message :)

Regards

[robfrawley]

@adaniloff Here is a notice that is pulled from the UPGRADE.md file (it matches the version header to the version being installed/upgraded and pulls the corresponding text). Note, this hook is only on my local machine, a PR for it is incoming, though. Thoughts? @antoligy

[alexwilson]

I really like this approach and honestly now wish that other bundles were doing it too! (It'd save me a lot of time 😂)

[adaniloff]

@robfrawley I already love it. Thanks :)

[robfrawley]

Looks like the response is positive so far, so I'll work on cleaning this up and getting it ready for submission. Note that it is "interactive-aware" and doesn't prompt the user to confirm the notice when Composer is run in non-interactive mode (for automated scripts or other non-interactive environments). I'll get a PR submitted later tonight.

[adaniloff]

@robfrawley Hi!
So you decided to abort the composer update hook part right ?
Could you be more explicit on the "why" ? I'm not quite sure that I have understood it.
Also, do you think of any other way to do something similar?

Sorry for bothering you that much :)

Regards

[robfrawley]

@adaniloff The composer hook was abandoned because the scripts key of a composer configuration file is ignored unless it is part of the root composer.json file. See their documentation on the subject for additional confirmation. What this means is that it would only be executed for developers of the bundle who are running composer explicitly for the package, and it would not do anything for people who require the package in their own projects, which is where it would actually be beneficial.

As for an alternative, I have yet to think of a solid way to handle this, but it is still on my mind, and I am open to ideas. That said, I am still against pursuing your original idea (re: #846 (comment)). Lastly, no bother whatsoever: I signed on to maintain this package, so ask as many question as you'd like, @adaniloff :-).

[robfrawley]

Looks like merging #851 automatically closed this too: I'll re-open this issue it until we get feedback from those involved in the discussion that their issue is resolved after upgrading to 1.7.1 and configuring multiple data roots, as described in the upgrade file.

[robfrawley]

@davidromani Is your issue resolved?

[davidromani]

@robfrawley sorry, but I don't undestand why allowing multiple data roots can fix my problem... I have a legacy project deployed with Capifony and every release creates a new directory.

Check chapter "4.Setup server" here http://capifony.org to see my directory structure. I usually share user uploads directory between deployments, and my web/media/cache dir is not shared.

So I only have a unique data root config like this:

liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: 
                    - "%kernel.root_dir%/../web/"

With this solution, shall I add a new data_root dir for each deploy? It's quite cumbersome...

[alexwilson]

You should be able to specify a data root pointing to the current directory, does that work?

…

On 28 Jan 2017 10:43 am, "David Romaní" ***@***.***> wrote: @robfrawley <https://github.com/robfrawley> sorry, but I don't undestand why allowing multiple data roots can fix my problem... I have a legacy project deployed with Capifony and every release creates a new directory. Check chapter "4.Setup server" here http://capifony.org to see my directory structure. I usually share user uploads directory between deployments, and my web/media/cache dir is *not* shared. So I only have a unique data root config like this: liip_imagine: loaders: default: filesystem: data_root: - "%kernel.root_dir%/../web/" With this solution I must add a new data_root dir for each deploy? It's quite cumbersome... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa29CjPLNS_An5sAwRFaSIwfAqiUejaks5rWxvWgaJpZM4LfNRM> .

[davidromani]

@antoligy I've tried to config this setting and it doesn't work.

liip_imagine:
    loaders:
        default:
            filesystem:
                data_root:
                    - "/path/to/my-web-project/current/web/"

And my directory structure is:

$ pwd
/path/to/my-web-project
$ ls -lh
total 8.0K
lrwxrwxrwx 1 flux flux   54 Jan 30 09:39 current -> /path/to/my-web-project/releases/20170130083913
drwxrwxr-x 5 flux flux 4.0K Jan 30 09:39 releases
drwxrwxr-x 5 flux flux 4.0K Nov 26  2015 shared

[robfrawley]

@davidromani That won't work. You must add the symlink resolved paths to your data_root (in your case: /path/to/my-web-project/releases/20170130083913. Alternatively, you can use bind mounts or other means.

[davidromani]

@robfrawley so it's not a solution for me because every release the path is changing...

I know that is not a problem related with this bundle, so you can close this issue if you want. Thanks.

[robfrawley]

Again, check out bind mounts, hard links, or subvolumes, all of which would work for your situation.

[alexwilson]

Actually - I am not sure that those are so easy to set up when working with Capistrano, especially where some webservers (at least, Apache) relies on the FollowSymlinks directive in order to do rolling deploys.
Wondering if it's worth some kind of Composer script that can automatically propagate data root configurations at build/deploy time? (also not ideal as this kills the immutability of a deployment, but I guess there's a point at which we have to draw a line)

[robfrawley]

There are absolutely functional workarounds. @davidromani Take, for example, bind-mounts. You could set up the following directory structure:

current/ -> ./releases/20170130083913 (normal synlink, handled by Capistrano)
releases/
application/ -> ./current (bind-mount)

And use application/ as the data root for Apache, Imagine Bundle, etc.

liip_imagine:
    loaders:
        default:
            filesystem:
                data_root:
                    - "/path/to/web/application/web/"

Given the above, because application/ is a bind-mount, it won't be resolved beyond itself, as to the filesystem it appears as a normal directory, despite the fact that it points to a symbolic link that points to the real folder.

The above requires a compatible filesystem, though.

This isn't to say that we couldn't implement some hooks that could be used, per your suggestion @antoligy, just that there are a bunch of easy and functional ways to work around this immediately, as well.

[robfrawley]

@antoligy Do we want to offer a toggle to disable security checks? And simply allow whatever paths are passed? (Disabled by default, of course).

[robfrawley]

@davidromani Also of note: you could easily write your own loader that does or does not perform whatever security checks you want for your specific situation. The interface isn't difficult to implement; it would only take a few minutes to do so.

<?php

namespace Liip\ImagineBundle\Binary\Loader;

interface LoaderInterface
{
    /**
     * Retrieve the Image represented by the given path.
     *
     * The path may be a file path on a filesystem, or any unique identifier among the storage engine implemented by this Loader.
     *
     * @param mixed $path
     *
     * @return \Liip\ImagineBundle\Binary\BinaryInterface|string An image binary content
     */
    public function find($path);
}

Here is an example of all you have to do:

<?php

namespace AppBundle\Imagine\Binary\Loader;

use Liip\ImagineBundle\Model\FileBinary;
use Liip\ImagineBundle\Binary\Loader\LoaderInterface;
use Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesserInterface;
use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;

class MyCustomDataLoader implements LoaderInterface
{
    /**
     * @var MimeTypeGuesserInterface
     */
    protected $mimeTypeGuesser;

    /**
     * @var ExtensionGuesserInterface
     */
    protected $extensionGuesser;

    /**
     * @var string
     */
    protected $dataRoot;

    /**
     * @param MimeTypeGuesserInterface  $mimeGuesser
     * @param ExtensionGuesserInterface $extensionGuesser
     * @param string                    $dataRoots
     */
    public function __construct(MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoot)
    {
        $this->mimeTypeGuesser = $mimeGuesser;
        $this->extensionGuesser = $extensionGuesser;
        $this->dataRoot = $dataRoot;
    }

    /**
     * {@inheritdoc}
     */
    public function find($path)
    {
        $path = $this->dataRoot.DIRECTORY_SEPARATOR.$path;   
        /* Perform any security checks you'd like on $path */
        $mime = $this->mimeTypeGuesser->guess($path);

        return new FileBinary($path, $mime, $this->extensionGuesser->guess($mime));
    }
}

Add it to the container:

# app/config/services.yml

services:
    imagine.data.loader.my_custom:
        class: AppBundle\Imagine\Binary\Loader\MyCustomDataLoader
        arguments:
            - "@liip_imagine.mime_type_guesser"
            - "@liip_imagine.extension_guesser"
            - "%your_root_path_parameter%"
        tags:
            - { name: "liip_imagine.binary.loader", loader: my_custom_data_loader }

And use it:

# app/config/config.yml

parameters:
    your_root_path_parameter: /path/to/root

liip_imagine:
    data_loader: my_custom_data_loader

[robfrawley]

@davidromani What is the exact, complete, exception you get when using the current/ directory as the data root? I just need to confirm the cause of this, in order to (possibly) provide an out-of-the-box solution.

[robfrawley]

Resolved via 1.7.2 release, specifically #866!

[akrz]

For anyone looking the less secure option:

liip_imagine :
  loaders:
    default:
      filesystem:
        locator: filesystem_insecure