Skip to content

[Process] Support using Process::findExecutable() independently of open_basedir #47422

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

Conversation

BlackbitDevs
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #41006
License MIT

With this PR Console::findExecutable() will find executables also if the their path is not allowed in open_basedir config similar to

$command = '\\' === \DIRECTORY_SEPARATOR ? 'where' : 'command -v';
if ($php = strtok(exec($command.' '.escapeshellarg($php)), \PHP_EOL)) {
if (!is_executable($php)) {
return false;
}
} else {

Console::findExecutable()'s responsibility is to find an executable which can be called with a Symfony Process or by PHP's functions like exec, system etc.

The goal of PHP's open_basedir config is to restrict reading / writing files within PHP processes. Imho this is completely independent of finding an executable.

If PHP's intention was to restrict executing applications which are not present in open_basedir's paths, it would have been implemented there.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There's a related failure on Windows, see appveyor.
Please add a test case also.

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Sep 13, 2022

@nicolas-grekas

There's a related failure on Windows, see appveyor.
Please add a test case also.

Can you point me where the error occurs? Have bit of a problem interprerting https://ci.appveyor.com/project/fabpot/symfony/builds/44759158 - I only see Command exited with code 1 at the end but not where there is a failing test.

@stof
Copy link
Member

stof commented Sep 13, 2022

@BlackbitNeueMedien This is because this URL was showing only the end of the logs, as explained in the banner on top.

Looking at the full logs, I see an issue: https://ci.appveyor.com/project/fabpot/symfony/builds/44759158?fullLog=true#L1706 shows that your new code leaks to stderr

However, I'm still not sure what changes the exit code to a failure one.

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas changed the title [Process] Support finding executables independent of open_basedir config [Process] Support finding executables independently of open_basedir config Aug 1, 2023
@nicolas-grekas nicolas-grekas changed the title [Process] Support finding executables independently of open_basedir config [Process] Support using Process::findExecutable() independently of open_basedir Aug 1, 2023
@nicolas-grekas nicolas-grekas force-pushed the bugfix/find-executables-outside-open_basedir branch from 1e0428c to 5af7d53 Compare August 1, 2023 14:58
@fabpot
Copy link
Member

fabpot commented Aug 1, 2023

Thank you @BlackbitDevs.

@fabpot fabpot merged commit e66a68c into symfony:6.4 Aug 1, 2023
nicolas-grekas added a commit that referenced this pull request Aug 7, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

[Process] fix tests

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

- `testFindProcessInOpenBasedir` is a duplicate of `testFindWithOpenBaseDir`
- `testFindWithOpenBaseDir` currently expects that we search open_basedir instead of PATH when the setting is set, but this doesn't really make sense, and #47422 removed this behavior
- `PhpSubprocessTest::testSubprocess` expects a php that defaults to memory_limit=-1, which is not the case currently for the sigchild-enabled binary

Commits
-------

4ca4417 [Process] fix tests
This was referenced Oct 21, 2023
Luc45 added a commit to Luc45/symfony that referenced this pull request Mar 4, 2024
nicolas-grekas added a commit that referenced this pull request Sep 17, 2024
…sedir (BlackbitDevs)

This PR was merged into the 5.4 branch.

Discussion
----------

[Process] Fix finding executables independently of open_basedir

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

This backports #47422 to 5.4, which is a bugfix really.

Instead of #58008 and #57954 /cc `@xabbuh` `@fritzmg`

Commits
-------

4424763 [Process] Fix finding executables independently of open_basedir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExecutableFinder attempts access outside of open_basedir directive, producing unnecessary errors/logs
5 participants