open_basedir restriction in effect

[srebb]

Problem/Motivation

During a second composer install, I got this error:

is_dir(): open_basedir restriction in effect. 
File(../../doctrine/coding-standard/lib) is not within the allowed path(s): 
(/var/www/html:/var/www/.composer:/tmp:/dev/urandom:/proc/self/fd:/usr/local/bin)

This happens, cause during composer install the root directory is not the phpcs directory, therefore the ../../ points out of the projectDir. In my case, the code is limited via open_basedir to the projectDir.

Expected behaviour

No error

Actual behaviour

Code throws error during the second composer install.

Steps to reproduce

Create a project.

composer require --dev \
    dealerdirect/phpcodesniffer-composer-installer \
    doctrine/coding-standard \
    doctrine/doctrine-fixtures-bundle \
    squizlabs/php_codesniffer

add an open_basedir restriction in php to the root of the project (e.g. /var/www/html/projectName) where you installed the project.
Do another composer install.
Every composer install after the first one will get an error for open_basedir restriction.

Proposed changes

Change the order of is_readable checks in Plugin.php::cleanInstalledPaths()

See pull request #272

Environment

Question	Answer
OS	Linux)
PHP version	8.4.12
Composer version	2.8.3
PHP_CodeSniffer version	4.0.1
Install type	Composer project local

Tested against main branch?

• I have verified the issue still exists in the main branch.

[Potherca]

Thank you for taking the time to report this. And for proposing a solution!

I'm not sure if there are scenario's where looking at the absolute path before looking at the relative path has downsides.

I'd like to check that first before merging the linked merge request.

[Potherca]

For some reason, I feel hesitant to change the order of the checks. There were some issues early on with relative/absolute paths, and if possible, I'd rather not change things if not needed.

However, the only alternative to changing the order would be catching the Warning, which seem a bit like overkill...

function throwErrorException($errno, $errstr, $errfile, $errline) {
    throw new ErrorException($errstr, 0, $errno, $errfile, $errline);
}
set_error_handler('throwErrorException', E_WARNING);
try {
    $isDir = (is_dir($path) === false || is_readable($path) === false)
} catch (ErrorException $error) {
    $isDir = false;
}
restore_error_handler();

if ($isDir && (
    $alternativePath === false ||
    is_dir($alternativePath) === false ||
    is_readable($alternativePath) === false
) {
    // ...
}

This can be seen working at https://3v4l.org/g2fsO

@jrfnl Do you have any insights regarding this?

[rodrigoprimo]

@jrfnl, mentioned this issue in a chat we had a few weeks ago. I looked into it and dug into the commit history to try to understand why cleanInstalledPaths() checks both the relative and absolute paths, and in that specific order.

Historical context

The method was originally introduced in c2be705 (Oct 2016) with a single check on the path as-is. At that point, all stored paths were absolute:

if (file_exists($path) === false || is_dir($path) === false || is_readable($path) === false) {
    unset($this->installedPaths[$key]);
}

Then in 19fa187 (May 2017), local installations switched to storing relative paths. The alternativePath check was added as a second condition to handle those relative paths, while the original check on $path was kept as-is:

$alternativePath = realpath($this->getPHPCodeSnifferInstallPath() . DIRECTORY_SEPARATOR . $path);

if ((is_dir($path) === false || is_readable($path) === false) &&
    (is_dir($alternativePath) === false || is_readable($alternativePath) === false)
) {

Later, in 50b2f26 (Dec 2020), the $alternativePath === false check was added to handle realpath() returning false, but the order remained unchanged.

As far as I can see from the git history, the current order (relative path first) was not a deliberate design choice or a response to issues with relative vs. absolute paths. It is just how the code evolved. That being said, I don't have deep knowledge of this project, so I might be missing something.

Alternative approach

Instead of swapping the order of the checks, another option would be to simplify the method to a single check. The idea is to resolve the path to absolute before validating it, but only when the path is relative:

$changes = false;
foreach ($this->installedPaths as $key => $path) {
    if ($this->filesystem->isAbsolutePath($path) === false) {
        $path = realpath($this->getPHPCodeSnifferInstallPath() . \DIRECTORY_SEPARATOR . $path);
    }

    if ($path === false || is_dir($path) === false || is_readable($path) === false) {
        unset($this->installedPaths[$key]);
        $changes = true;
    }
}
return $changes;

This avoids the open_basedir warning because is_dir() is never called with a raw relative path, and simplifies the logic from two checks to one.

Test coverage

The existing testPreexistingInvalidInstalledPathsConfigIsRemoved() test covers valid absolute paths being kept and invalid relative paths being removed. However, it doesn't cover invalid absolute paths or valid relative paths going through cleanInstalledPaths(). It might be worth extending it to cover all four combinations (valid/invalid x absolute/relative).

Directly testing the open_basedir scenario with the current test setup would be less trivial.

On the set_error_handler approach

Again, as someone with very little experience with this project, I might be missing something, but I think both the swap proposed in #272 and the approach above would be preferable to catching the warning via set_error_handler. The error handler adds complexity to suppress a warning that can be avoided entirely by not calling is_dir() with a raw relative path in the first place.

I hope the above helps. Let me know if there is anything else I can help with.

[jrfnl]

However, the only alternative to changing the order would be catching the Warning, which seem a bit like overkill...

Or silencing the warning...

            if (
                (@is_dir($path) === false || @is_readable($path) === false) &&
                (
                    $alternativePath === false ||
                    @is_dir($alternativePath) === false ||
                    @is_readable($alternativePath) === false
                )
            ) {

While I'm not a fan of using the silence operator, this does seem like a potentially valid use-case.
Also, keep in mind that as of PHP 8.0, @ can no longer suppress critical errors.

Having said that, I kind of like the approach @rodrigoprimo proposes, including adding the additional tests as mentioned above.

Directly testing the open_basedir scenario with the current test setup would be less trivial.

@rodrigoprimo Agreed.

In case anyone is wondering why: even though open_basedir is INI_ALL, the test setup as it currently is, will run a composer command in a separate PHP process and we would want to change the open_basedir for that process, not for the PHP process running our test suite.

@srebb Would you be willing to update your pull request to incorporate the proposed solution and add the mentioned extra tests ?

If not, @rodrigoprimo might be willing to create an alternative PR to sort this out ?