Liipimaginebundle: Problem with loaders data_root

[peter-gribanov]

| Q | A
| --- | ---
| Bug Report? | yes
| Feature Request? | yes
| BC Break Report? | no
| RFC? | no
| Imagine Bundle Version | 1.9.1

My config:

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

is broken on production with error:

Source image invalid "/www/example.com/shared/web/media/tmp/1512029356077672_s_kotom.jpg" as it's outside of the defined root path(s) "/www/example.com/releases/20171130110615/web"

On production server we use file structure like this:

├─ current → /var/www/example.com/releases/20170914151406
├─ releases
│  ├─ 20170914122420
│  ├─ 20170914150626
│  └─ 20170914151406
│    └─ web
│       └─ media → /var/www/example.com/shared/web/media
└─ shared
  └─ web
     └─ media

I found simular issue #849, #911.
Okey. I change configuration for add access to shared folder:

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

Now it's work on production server, but...

But now, is broken on local machine :man_facepalming:

Root image path not resolvable "C:\projects\example.com\app/../../../web/"

Okey. I separate configuration by env configs:

# config_dev.yml
liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: '%kernel.root_dir%/../web/'

and

# config_prod.yml
liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: '%kernel.root_dir%/../../../shared/web/'

It seems that everything works, but...

If on the local machine we try to open the production mode for testing, then everything will broken.
If we change the file structure on the production server, then everything will broken.
:sob:

We can configure data_root by environment or parameters.yml

#parameters.yml
parameters:
    liip_imagine.data_root: '%kernel.root_dir%/../../../shared/web/'

# config.yml
liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: '%liip_imagine.data_root%'

But is not good in this way.

%kernel.root_dir%/../web/ - is in the project.
%kernel.root_dir%/../../../shared/web/ - is outside the project (although in reality it's available at %kernel.root_dir%/../web/).

I understand that it's difficult to fix this problem, but you can't leave it that way either.

[KevinSleegers]

@peter-gribanov I'm experiencing the exact same issues right now. Have you been able to solve it in a better way? If you did, could you share it with me and other people who are facing this issue.

[peter-gribanov]

@KevinSleegers i configure data_root by parameters.yml as written above.

# config.yml
liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: '%liip_imagine.data_root%'

As default use:

# parameters.yml.dist
parameters:
    liip_imagine.data_root: '%kernel.root_dir%/../web/'

On production server i change this value:

# parameters.yml
parameters:
    liip_imagine.data_root:
        - '%kernel.root_dir%/../../../shared/web/'
        - '%kernel.root_dir%/../web/'

[peter-gribanov]

As alternative way to configure with the environment variables.

# liip_imagine.yml
parameters:
    env(APP_WEB_PATH): 'web/'

    liip_imagine.data_root:
        - '%kernel.project_dir%/web/'
        - '%kernel.project_dir%/%env(APP_WEB_PATH)%'

liip_imagine:
    loaders:
        default:
            filesystem:
                data_root: '%liip_imagine.data_root%'

On production server set the environment variable:

APP_WEB_PATH="../../shared/web/"

[robfrawley]

@peter-gribanov @KevinSleegers: Is #1094 helpful?

[KevinSleegers]

@robfrawley I'm not sure, I've managed to make it work using the code @peter-gribanov provided.

I have the following code in my parameters.yml file:

liip_imagine_filesystem_data_root:
- "%kernel.root_dir%/../web"
- "%kernel.root_dir%/../../../shared/web"

[tristanbes]

Hi, I updated the bundle to 1.9.1 and i've came accross the exact same issue.

1. Why this bundle does not respect SemVer ? Already 2 BC breaks happened within major version and even a BC break happened in patch version... (1.9.0 && 1.9.1) 😱🚨🔥🤯

2. The bundle stopped working
   Source image invalid \"/home/johndoe/zilean/media/original/21752199-1488628521226091-7067398196013888458-n-175508.jpg\" as it is outside of the defined root path(s) \"/var/www/johndoe/releases/20180518134412/web\

This image is really on the path reported in the error, but it never raised an error before :/

• /home/johndoe/zilean/ is where the NFS is mounted on the server
• /var/www/.../current/web/ is where the symfony web root dir is and medias is a symlink tobelow:
• /var/www/..../shared/web/medias is where the symlink is

[robfrawley]

Why [does] this bundle not respect semver?

1. We do our absolute best to respect semver, though I do align my understanding of the concept much closer to Symfony's handling than other interpretations.

   Already 2 BC breaks happened within [a] major version [release]

   Major version releases are exactly where BC breaks are appropriate, so I don't understand what you take issue with (though it would be helpful to provide some context by specifically referencing what you are referring to).

   A BC break happened in [a] patch version (1.9.0...1.9.1)

   The BC break in 1.9.1 relates to an important bug introduced in 1.9.0 that rendered this project completely incompatible with supported dependencies (specifically, a command argument was added that conflicted with a core-Symfony argument from an older release we were still targetting, causing a failure when attempting to run the console command).

   Perhaps it should have been a minor release instead of a patch release, but as it only changed one command line argument that was only introduced eight days prior in 1.9.0, it didn't seem like a major concern.

The bundle stopped working: Source image invalid "[...].jpg" as it is outside of the defined root path(s) "[...]/releases/20180518134412/web"

1. The error you are getting is error likely the result of a bug fix introduced in the minor version that corrected the logic that determine if the requested file is located within a defined data root. The handling is extremely important and presents security concerns, hence the change.

   Prior, symbolic links could be placed within a data root (either accidently or by an attacker) and doing so would allow access to any location on the file system. That flies in the face of the original intent of the data root checks and is obvioudly a bug.

   Also, despite the similar error message, I do not believe your issue is related to this issue thread, so I'm not sure the discussion here or related PR #1094 will be helpful to you. If your issue is related to the change I'm describing above, there are two routes to fix your setup:

   • Explicitly configure your roots appropriately, including real paths for any symbolic links:
     yml liip_imagine: loaders: default: filesystem: data_root: - "%kernel.root_dir%/../web" - "/path/to/resolved/symbolic/link/foo" - "/path/to/resolved/symbolic/link/bar" // ...etc
   • Revert to prior logic if you understand the security ramifications and are comfortable with them:
     ```yml
     liip_imagine :
     loaders:
     default:
     filesystem:
     locator: filesystem_insecure
     ````

[tristanbes]

I wrote Major, I meant minor sorry.

1 BC break happened in a minor (1.9.0)
1 BC Break happened in a patch version (1.9.1)

Also, thanks for the detailled answer on what happened with the data root configuration.

[peter-gribanov]

@robfrawley i agree with @KevinSleegers
Your change makes the code not secure. It is better to always check the path and just to document this feature.

Perhaps it's better not to throw exception here. Just ignore the unresolved root paths.

[robfrawley]

Your change makes the code not secure.

Huh? I'm not sure I follow. The change that caused his issue was literally a security bug fix: it made the code more secure. It went into effect in 1.7.2, almost a year ago, and it will very likely remain the new, more secure handling unless someone comes along with a very persuasive argument otherwise. If you'd prefer to not use it, then follow the directions I mentioned above to revert to the prior behavior:

liip_imagine :
    loaders:
        default:
            filesystem:
                locator: filesystem_insecure

This "feature" is not documented here because it is a hack, and not recommended. It is mentioned in our UPGRADE.md specifically for those upgrading from pre-1.7.2 releases to know about it.

Perhaps it's better not to throw an exception here. Just ignore the unresolved root paths.

I previously mentioned #1094, which does just this. Please take some time to take a look and test it to ensure it resolved your issues and offers a productive fix; doing so would greatly help in getting it merged quicker.

[peter-gribanov]

@robfrawley i don't like the idea with the addition of option.

If one of the root paths is not resolved, then this is not error.
You can throw a warning, but do not throw exception.
The error will be if none of the root paths are resolved.

You throw exception only if you did not find the file in any of the root paths.
Not on every root path, namely in all paths.
Therefore, if one of the root paths does not resolve, then this will not be a error.
This is the essence of several root paths.

[robfrawley]

It's always thrown an exception on an invalid root path for the entire lifecycle of 1.x and will now continue to do so with the newly released 2.x. Since this is the expected behavior, and users may very well be relying on it, the only way to change this behavior is with a new configuration option that can optionally be toggled, as introduced in #1094. We could theoretically alter this behavior for 3.x (which is a long way off) but such a change would need a good reason behind it and I'm not convinced such behavior is beneficial for the majority of users.

This is the essence of several root paths.

The intention of the multiple data roots functionality is to support loading from multiple paths and to support loading from paths symlinked from the primary root. It was not to allow users to add a list of paths that may or may not exist. I would know; I wrote and introduced the feature. :-)