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

isTtySupported() does not take into account open_basedir() restrictions #54874

Closed
LukeTowers opened this issue May 10, 2024 · 2 comments
Closed

Comments

@LukeTowers
Copy link
Contributor

LukeTowers commented May 10, 2024

Symfony version(s) affected

>= 6.2

Description

In 007fa61 / #46971 the logic that checks if TTY is supported changed from checking /dev/tty for reading / writing capabilities directly to checking stream_isatty(\STDOUT).

This causes an issue because the new way fails to take into account the open_basedir restrictions.

Related: wintercms/storm#175

How to reproduce

open_basedir restriction not in effect, works fine:

php -r "require_once(__DIR__ . '/vendor/autoload.php'); (new \Symfony\Component\Process\Process(['ls', '-lsa']))->setTty(true)->run();"

open_basedir restriction in effect, causes hard crash with PHP Fatal error: Uncaught Symfony\Component\Process\Exception\RuntimeException: Unable to launch a new process. in vendor/symfony/process/Process.php:350:

php -d open_basedir="/path/to/cwd" -r "require_once(__DIR__ . '/vendor/autoload.php'); (new \Symfony\Component\Process\Process(['ls', '-lsa']))->setTty(true)->run();"

Possible Solution

Revert the linked commit or add an additional is_writable('/dev/tty') check.

Additional Context

Side note, can we change the self::isTtySupported() call to static::isTtySupported() instead so that extending classes can override just the relevant method if needed in the future instead of having to override the setTty() method entirely?

@LukeTowers LukeTowers added the Bug label May 10, 2024
@LukeTowers
Copy link
Contributor Author

Ping @Yurunsoft

@xabbuh xabbuh added the Process label May 10, 2024
@mjauvin
Copy link
Contributor

mjauvin commented May 10, 2024

Fixed by #54863

xabbuh added a commit that referenced this issue May 15, 2024
…ss to `/dev/tty` (mjauvin)

This PR was merged into the 6.4 branch.

Discussion
----------

[Process] Return `false` when `open_basedir` prevents access to `/dev/tty`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no

If open_basedir restrictions are in effect, checking if the file /dev/tty is writable will prevent setting tty mode on the process, and avoid failing to create a Process.

Fixes #54874

Commits
-------

825e38b Return false in isTtySupported() when open_basedir restrictions prevent access to /dev/tty.
@xabbuh xabbuh closed this as completed May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants