TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

pounard · 2017-03-24T15:30:06Z

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no
Symfony version	>=3.2

In Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\ExtensionPass, you may see the following method:

    private function getComposerRootDir($rootDir)
    {
        $dir = $rootDir;
        while (!file_exists($dir.'/composer.json')) {
            if ($dir === dirname($dir)) {
                return $rootDir;
            }

            $dir = dirname($dir);
        }

        return $dir;
    }

Which looks up for the composer.json, but in our environments, the project main folder is under the open_basedir restriction, file_exists() call will generate PHP warnings; because the extension cannot find the composer.json due to open_basedir restriction. Because it does not find the file, the algorithm continues down until it reaches the filesystem root. In another scenarios, we could deploy applications without the composer.json (why not).

Is there any valid reason for doing this lookup?

In our use case, due to this buggy lookup behaviour, it will register a Twig_Loader_Filesystem instance which is plugged on the filesystem root (it seems very, very wrong to me).

In most Symfony applications, this would suffice:

$composerRootDir = dirname($container->getParameter('kernel.root_dir'));

I don't understand why this was introduced, it seems to give a more flexible way of registering templates for highly customized projects, but it's also a very dangerous thing to do.

Another way of fixing this might by introducing an optional parameter we could set ourselves to the project root, such as:

if ($container->hasParameter('kernel.composer_root_dir')) {
    $composerRootDir = $container->getParameter('kernel.composer_root_dir');
} else {
    $composerRootDir = $this->getComposerRootDir($container->getParameter('kernel.root_dir'));
}

The text was updated successfully, but these errors were encountered:

pounard · 2017-03-24T15:49:14Z

Using git bisect found the culprit to be #20428

Were I found that there is many people raising warnings about the method being wrong, and guess what now that it triggers bugs on my own deployments, I do agree with them it's wrong :)

Allow me to quote @fabpot :

@nicolas-grekas I prefer my strategy. Storing the composer.json (which should be in your repository) elsewhere than the root directory is I think not something we need to support (and a nightmare for anyone doing this). But the vendor directory can be anywhere (there is even a configuration for that in composer.json: vendor-dir). As the vendor should not be in your repo, and because this is just a read-only artifact, I can easily see people storing that somewhere else.

And my opinion is that you may not support people not using composing.json or not storing it in the project root, but you should support people that do aggressive security tuning on their projects.

xabbuh · 2017-03-24T15:52:35Z

You could just build the container and remove the composer.json file afterwards instead of doing the opposite.

pounard · 2017-03-24T15:53:21Z

@xabbuh We are not removing the composer.json that's not the point, but an example, the real problem is that with security restrictions, this just does not work.

I'm going to boldify open_basedir in my initial issue description.

pounard · 2017-03-25T17:00:28Z

Another side effect is that during normal runtime (by normal I mean when caches are not rebuilt, production mode) it seems that the loaders attempt a realpath() on the found project path, which causes PHP warnings and failures.

nicolas-grekas · 2017-04-17T19:58:43Z

Can't this be fixed with the new Kernel::getProjectDir() method? Anyone who'd would like to give it a try?

mvrhov · 2017-04-18T07:28:47Z

Have not figured this one out but we are removing composer.json when generating deploy.tar.gz file. And the container gets rebuild on the server.

pounard · 2017-04-19T10:01:29Z

@nicolas-grekas I guess it depends on the getProjectDir() implementation, I hope it's not traversing the filesystem until it finds the composer.json file else the fail would be the exact same.

pounard · 2017-04-19T10:03:54Z

According to https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L317 the problem will probably still exist using the getProjectDir() method. Nevertheless, for consistency reasons, the TwigBundle should use it from the 3.3 version.

The good thing with getProjectDir() is we actually can override it in our custom kernel, and that would be a perfectly fine solution for me.

nicolas-grekas · 2017-04-19T10:06:27Z

I meant: just provide the kernel.project_dir param to the constructor.

pounard · 2017-04-19T10:41:52Z

Could be it, still needs to be fixed, this is a real bug, can it be tagged as such ?

I will do a PR within the week.

pounard · 2017-04-28T10:34:09Z

@nicolas-grekas I opened the #22570 PR, and I will open new ones for fixing >=3.0 to <=3.2 versions, because our productions sites are misbehaving, and people should be allowed to fix this on their live applications.

This PR was squashed before being merged into the 3.3-dev branch (closes #22570). Discussion ---------- Fixes twig bundle project root dir discovery | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22149 | License | MIT | Doc PR | none  TwigBundle ExtensionPass uses a directory lookup algorithm to find the composer.json file. When working under open_basedir restrictions, if the project root folder is not allowed PHP runtime to read, the composer.json will never be found, throwing PHP notices along the way and ending up using the filesystem root instead. Since that 3.3 introduced the kernel getProjectRoot() method and kernel.project_dir variable, the TwigBundle can now leverage it, in order to get more consistent, and allow site builders to override the getProjectRoot() kernel method, hence bypass the open_basedir restrictions in reliable and safe way. Commits ------- 3cb2e5b Fixes twig bundle project root dir discovery

pounard mentioned this issue Mar 24, 2017

[TwigBundle] fixed template root path #20428

Merged

javiereguiluz added the TwigBundle label Mar 29, 2017

pounard changed the title ~~TwigBundle's ExtensionPass does extensive filesystem lookup, might hit open_basedir restrictions~~ TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions Apr 19, 2017

pounard mentioned this issue Apr 28, 2017

Fixes twig bundle project root dir discovery #22570

Closed

fabpot closed this as completed Apr 28, 2017

pounard mentioned this issue Jul 25, 2017

Symfony\Component\VarDumper\Caster\LinkStub::getComposerRoot() will raise errors when open_basedir restrictions is set on the project root dir #23661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

pounard commented Mar 24, 2017 •

edited

Loading

pounard commented Mar 24, 2017 •

edited

Loading

xabbuh commented Mar 24, 2017

pounard commented Mar 24, 2017 •

edited

Loading

pounard commented Mar 25, 2017

nicolas-grekas commented Apr 17, 2017 •

edited

Loading

mvrhov commented Apr 18, 2017

pounard commented Apr 19, 2017

pounard commented Apr 19, 2017 •

edited

Loading

nicolas-grekas commented Apr 19, 2017

pounard commented Apr 19, 2017 •

edited

Loading

pounard commented Apr 28, 2017

TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

TwigBundle's ExtensionPass does extensive filesystem lookup, and fail when project root dir is under open_basedir restrictions #22149

Comments

pounard commented Mar 24, 2017 • edited Loading

pounard commented Mar 24, 2017 • edited Loading

xabbuh commented Mar 24, 2017

pounard commented Mar 24, 2017 • edited Loading

pounard commented Mar 25, 2017

nicolas-grekas commented Apr 17, 2017 • edited Loading

mvrhov commented Apr 18, 2017

pounard commented Apr 19, 2017

pounard commented Apr 19, 2017 • edited Loading

nicolas-grekas commented Apr 19, 2017

pounard commented Apr 19, 2017 • edited Loading

pounard commented Apr 28, 2017

pounard commented Mar 24, 2017 •

edited

Loading

pounard commented Mar 24, 2017 •

edited

Loading

pounard commented Mar 24, 2017 •

edited

Loading

nicolas-grekas commented Apr 17, 2017 •

edited

Loading

pounard commented Apr 19, 2017 •

edited

Loading

pounard commented Apr 19, 2017 •

edited

Loading