Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Images not generated when source path is out of root directory #780

Closed
nicokaag opened this issue Sep 1, 2016 · 12 comments
Closed

Images not generated when source path is out of root directory #780

nicokaag opened this issue Sep 1, 2016 · 12 comments

Comments

@nicokaag
Copy link

nicokaag commented Sep 1, 2016

In pull request #775 a check is added so that the source file cannot be outside of the root path.
Our source files are loaded through a symlink, which is outside of the root path, and therefor I get the error message:
Source image invalid \"/home/data/uploads/article/someimage.png\" as it is outside of the defined root path

I can understand that it might improve security, but I think it is a pretty normal use-case to keep your uploaded images outside of the project root path (for back-up and autoscaling purposes)

Therefor I would suggest that there should be a configuration option to enable/disable this check, and then have it enabled by default.

@robfrawley
Copy link
Collaborator

robfrawley commented Sep 1, 2016

@nicokaag: There has always been a check requiring you are within the root directory, only prior it didn't resolve paths to their real path. Why don't you just set a proper root path value for your specific setup via the configuration YML:

liip_imagine:
  loaders:
    default:
      filesystem:
        data_root: "/your/data/root"

Alternatively, if you are on Linux or FreeBSD, you can always bind one directory path (the folder outside your web root) to another (a folder inside your web root) like so:

# For Linux
mount -o bind /directory/foo /directory/bar

# For FreeBSD
mount -t nullfs /directory/foo /directory/bar

@slaubi
Copy link

slaubi commented Sep 1, 2016

a simple reason to not set the data_root in the configuration is, that there is difference per environment (staging/production). only way around that, is again a setting over the parameters.yml.
For my taste this it's a bit too much "configuration" for a total valid usage of a symlink.

@robfrawley
Copy link
Collaborator

robfrawley commented Sep 1, 2016

@slaubi I disagree that blindly following symbolic links outside the defined data root should be acceptable behavior. I absolutely do not want any piece of code doing that without my explicit consent, as doing so negates the value of setting a data root entirely and opens the door to a host of security concerns. In the context of this bundle, "explicit consent" simply means setting the data_root configuration parameter appropriately.

As for requiring different configuration depending on environment, I don't find it overly ambitious to ask users to utilize Symfony's build-in environment-specific configuration loading (via config_[prod|dev|test|staging|etc].yml) to set different values.

That being said, I am not a "bundle maintainer" so it is possible a PR relaxing this behavior would be accepted if you submitted one.

As it stands, you need to do one of the following to allow for your images to be stored in an arbitrary location:

  1. If you are hard-set on a symbolic link, set an appropriate value for data_root.
  2. Alternatively, utilize an available file-system-level configuration option to mount the directory within the default data root (such as a bind mount).

@nicokaag
Copy link
Author

nicokaag commented Sep 2, 2016

I now explicitly set the directory over the configuration. So I close this ticket because that does resolve my issue.

@ryanotella
Copy link

ryanotella commented Jun 23, 2017

This is quite messy to resolve if you're deploying to a large number of servers. And it's made trickier if you are using symlinks to achieve atomic-ish deployments, because your uploads folder is symlinked to a permanent shared directory. The real path of your scripts will not match the real path of your uploaded assets.

config.yml

imports:
  - { resource: parameters.php }

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

parameters.php

$container->setParameter(
    'liip_imagine.data_root', 
    dirname(realpath(__DIR__ . '/../../web/uploads'))
);

I can't help but feel that there is a better solution than this, but I'm not convinced hard-coding the home directory into the parameters file is it.

@robfrawley
Copy link
Collaborator

I'm not sure what your exact issue with configuration is, can you elaborate further? The data_root option exists as we need to define a root path that constricts where you can load image files for two important reasons:

  1. So we know where to look for images, and
  2. So images aren't loaded from anywhere on the disk without authorization

The configuration of the liip_imagine.loaders.default.filesystem.data_root parameter has been greatly expanded in the past six months, and I believe it can accommodate most setups. Here are the available options at this point:

Default setup enables loading from the web/ directory of your application:

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

Support for multiple paths enables loading from any number of directories:

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

Support for named paths enables loading from any number of directories and explicity selecting a root path when calling our API:

liip_imagine:
  loaders:
    default:
      filesystem:
        data_root:
          foo: "%kernel.root_dir%/../web/foo"
          bar: "%kernel.root_dir%/../web/bar"
          baz: "%kernel.root_dir%/../web/baz"
// load from the first root path where the image is found
<img src="<?php $this['imagine']->filter('path/to/file.ext', 'my_filter_set') ?>" />

// load from the "baz" path explicitly
<img src="<?php $this['imagine']->filter('@baz:path/to/file.ext', 'my_filter_set') ?>" />

Support for bundle-autoregistration allows you to automatically register the Resources/public folder of all bundles. You can optionally set the access_control_type to whitelist or blacklist and either supply an inclusive or exclusive list to access_control_list of bundles (bundles are automatically registered as "named" paths):

liip_imagine:
  loaders:
    default:
      filesystem:
          bundle_resources:
            enabled: true
            access_control_type: blacklist
            access_control_list:
              - FooBundle
// load from the first bundle path where the image is found
<img src="<?php $this['imagine']->filter('path/to/file.ext', 'my_filter_set') ?>" />

// load from a "MyCoolBundle" public bundle resource path explicitly
<img src="<?php $this['imagine']->filter('@MyCoolBundle:path/to/file.ext', 'my_filter_set') ?>" />

Support for symlink-friendly locator allows using symlink-heavy applications; using this locator disables symlink resolution on file paths and allows for symlink-deployed apps to function properly, at the expense of some security:

liip_imagine:
  loaders:
    default:
      filesystem:
          locator: filesystem_insecure

@ryancastle Is there something in our configuration missing for your environment? Otherwise, we'd be happy to review a PR adding useful functionality, or if your data loading requirements are extremely specific we support custom data loaders. Does this not meet the vast majority of requirements? :-)

@ryanotella
Copy link

@robfrawley The insecure locator might be what we need. I wasn't aware of that. It just broke during a composer update, and I couldn't see any other good options. In our instance, we are dealing with fairly isolated environments where we largely trust the symlinks. It turns out the real difficulty was in calculating the home directory correctly provide consistent access to a shared directory. We probably just needed to update our deployments to provide that path as an environment variable. The resolved path of our assets is outside the release path root, which is why there is an issue in the first place.

@robfrawley
Copy link
Collaborator

Providing the shared data path as an environment variable is a great idea. As for the locator changes, we became aware of security issues with our previous algorithm, so as of 1.7 the locator is much more strict, whereas the filesystem_insecure locator reverts to 1.6 behavior.

Also, be sure to reference the UPGRADE.md file in the repository root if you experience any future upgrade difficulties; for the entirety of the 1.7.x and 1.8.x release cycles I've made sure to include verbose information about changes in the UPGRADE.md file, including all the information from my above post.

If you have any other questions be sure to reach out! We'll do our best to provide guidance.

@jeroendesloovere
Copy link

jeroendesloovere commented Sep 22, 2017

Hi y'all.

The same problem over here

FORK CMS is using LiipImagineBundle in the MediaLibrary module.

When deploying a Fork CMS website using Capistrano we have the same problem: source is outside of the defined root path(s).
schermafbeelding 2017-09-21 om 13 25 46

A little more information about our modern setup

In the server root we have /apps/production/current which is a symlink to the current used revision /apps/production/releases/20170918140829.

We have shared files which should stay outside a revision.
They are located at /apps/production/shared and symlinks are used to show these files...

We have a symlink in every revision, f.e.: /apps/production/releases/20170918140829/src/Frontend/Files/MediaLibrary to /apps/production/shared/src/Frontend/Files/MediaLibrary.

schermafbeelding 2017-09-21 om 13 39 59

Conclusion

Symlinks are being used, since that is a modern setup.
=> Checking for the data_root of a "source" seems like something LiipImagineBundle isn't responsible for. So we suggest removing that check or either fix working with symlinks.

@jeroendesloovere
Copy link

jeroendesloovere commented Sep 22, 2017

This is the Fork CMS setup.

liip_imagine:
    resolvers:
       default:
          web_path:
            web_root: "%kernel.project_dir%/"
            cache_prefix: src/Frontend/Files/Cache
    loaders:
        default:
            filesystem:
                data_root: "%kernel.project_dir%/"
    cache: default
    data_loader: default
    filter_sets:
        # ...

Working solution

As described from @robfrawley
Was to add locator: filesystem_insecure which results in:

    loaders:
        # ...
        default:
            filesystem:
                data_root: "%kernel.project_dir%/"
                locator: filesystem_insecure

And now allows symlinks.

What didn't work

Changing data_root from "%kernel.project_dir%/" to "%kernel.project_dir%/../" -- trying to include shared.

@robfrawley
Copy link
Collaborator

@jeroendesloovere The filesystem_insecure option you used to resolve your issue absolutely works, but is not the recommended approach. The best option (if workable in your environment) is to simply define all the resolved root paths under your data roots. Many don't know that we've added the ability to define any number of data roots, and each will be searched for the requested file during the filter resolution request. For example, take the following folder structure:

/my/app/web/
/my/app/web/foo -> /data/foo (symlink)
/my/app/web/bar -> /data/bar (symlink)

As you've encountered, the following will not work, as your root contains symbolic links and these resolve outside the configured data root path:

loaders:
    # ...
    default:
        filesystem:
            data_root: "/my/app/web/"

What you can do (instead of changing the locator algorythm to filesystem_insecure) is to define all the possible (resolved symlink) data roots:

loaders:
    # ...
    default:
        filesystem:
            data_root:
                - "/my/app/web/" # this contains "foo" and "bar" symlinks
                - "/data/foo"    # the "foo" resolved symlink path
                - "/data/bar"    # the "bar" resolved symlink path

The above multiple-data-roots example will accomplish the same thing as enabling the insecure filesystem locator, without degrading the security of the data loader operation:

loaders:
    # ...
    default:
        filesystem:
            data_root: "/my/app/web/"
            locator: filesystem_insecure # not recommended unless unavoidable

Bonus Information: You can also utilize "named" data roots to enable selecting the correct root path when the same file exists in multiple roots. Let's assume the same directory structure (with the symbolic links) as detailed above. Now, assume you have a file baz.jpg in both the "foo" and "bar" data root paths. We can amend our configuration to specify named paths:

loaders:
    # ...
    default:
        filesystem:
            data_root:
                default: "/my/app/web/" # this contains "foo" and "bar" symlinks
                foo: "/data/foo"        # the "foo" resolved symlink path
                bar: "/data/bar"        # the "bar" resolved symlink path

Once we've done this, we request our file (via the Twig extension or another API entry point) as you normally would, but instead use the notation @root-name:path/to/file.ext. Given our example of having a file named both /data/foo/baz.jpg and /data/bar/baz.jpg, you would request the image as either @foo:baz.jpg or @bar:baz.jpg to request the file from the specific root path. If you were to simply request baz.jpg, it would be loaded from /data/foo, as this is the first configured path where the file exists.


Hope the above info help! We've been adding new features regularly, so the documentation for these new features (while it does exist) isn't verbose, clear, or readily advertised. If you have any questions, feel free to ask. And again, while filesystem_insecure locator isn't recommended, sometimes it is a required evil, and there is nothing wrong with using it, I'd simply like to discourage it as much as possible.

@robfrawley
Copy link
Collaborator

robfrawley commented Sep 22, 2017

@jeroendesloovere

What didn't work: Changing data_root from "%kernel.project_dir%/" to "%kernel.project_dir%/../" -- trying to include shared.

I have one last comment for you. :-) I just noticed the above note you added to the end of your last post.

While switching one for the other won't resolve your issue (as your data root changes and therefore so does all the paths you call your images from), instead you can add both and this will resolve your issue, without enabling filesystem_insecure too:

loaders:
    # ...
    default:
        filesystem:
            data_root:
                - "%kernel.project_dir%/"
                - "%kernel.project_dir%/../"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants