Skip to content

[Messenger][Process] add fromShellCommandline to RunProcessMessage #59768

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

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

Staormin
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

The RunProcess::fromShellCommandline() method can be useful when using pipes or redirection.
This feature allow the use of this method when using a RunProcessMessage.

@Staormin Staormin force-pushed the runprocessmessage-fromshellcommandline branch from 243baf1 to 5746aab Compare February 13, 2025 14:37
@ro0NL
Copy link
Contributor

ro0NL commented Feb 13, 2025

What about array|string $command instead?

@stof
Copy link
Member

stof commented Feb 13, 2025

the Process component has explicitly rejected the usage of array|string (we deprecated passing a string to the constructor when introducing the distinction) because the security considerations are totally different between the array of arguments (where escaping is handled internally) and the command line (where the code building the command line is responsible for all required escaping).
So I would vote against an API using array|string for the RunProcessMessage.

@ro0NL
Copy link
Contributor

ro0NL commented Feb 13, 2025

i mean if it's a string use Process static constructor, otherwise its regular constructor

having both $command and $commandLine in a regular constructor here, bugs me more :)

@Staormin
Copy link
Contributor Author

What about array|string $command instead?

the Process component has explicitly rejected the usage of array|string (we deprecated passing a string to the constructor when introducing the distinction) because the security considerations are totally different between the array of arguments (where escaping is handled internally) and the command line (where the code building the command line is responsible for all required escaping). So I would vote against an API using array|string for the RunProcessMessage.

Also I find the dedicated method approach more explicit for developper, parameters with multiple type aren't as clear for me.

@Staormin
Copy link
Contributor Author

i mean if it's a string use Process static constructor, otherwise its regular constructor

having both $command and $commandLine in a regular constructor here, bugs me more :)

Yeah that's a good point, maybe a whole other message (RunProcessMessageFromCommandLine ?) would be cleaner ?
Then we could make an abstract that'll be used by both RunProcessMessage and this new message.

@ro0NL
Copy link
Contributor

ro0NL commented Feb 13, 2025

personally new RunProcessMessage('ls -a') vs new RunProcessMessage(['ls', '-a']) conveys fine to me.

Otheriwse i'd prefer the current approach yes over a new class.

@stof
Copy link
Member

stof commented Feb 13, 2025

@ro0NL The case of static command is not the one having security issues.

new RunProcessMessage(['ls', $folder]); // Secure

new RunProcessMessage(\sprintf('ls %s', $folder)); // Insecure

new RunProcessMessage(\sprintf('ls %s', escapeshellarg($folder))); // Secure on Unix system (and partially secure on Windows)

@ro0NL
Copy link
Contributor

ro0NL commented Feb 13, 2025

new RunProcessMessage(\sprintf('ls %s', $folder)); // Insecure

It would be passed to Process::fromShellCommandline

@Staormin
Copy link
Contributor Author

Staormin commented Feb 13, 2025

new RunProcessMessage(\sprintf('ls %s', $folder)); // Insecure

It would be passed to Process::fromShellCommandline

Yes but the point here is that the user won't know that the call is inherently unsecure, having a dedicated method that is documented as such is better imo

@Staormin Staormin force-pushed the runprocessmessage-fromshellcommandline branch from 5746aab to 4afeaee Compare February 13, 2025 17:13
Allows using the Process::fromShellCommandline when using a RunProcessMessage
@Staormin Staormin force-pushed the runprocessmessage-fromshellcommandline branch from 4afeaee to e448596 Compare February 14, 2025 16:23
@Staormin
Copy link
Contributor Author

BTW @nicolas-grekas I bisected the php-src to find out why the unit tests fails with php 8.5 and i narrowed it down to those change to reflection: php/php-src#17755
Should I open an issue or should we wait for 8.5 to stabilise before adressing broken components?

@nicolas-grekas
Copy link
Member

Ah nice! Please report to php-src. The earlier the better.

@fabpot
Copy link
Member

fabpot commented Feb 15, 2025

Thank you @Staormin.

@fabpot fabpot merged commit 6fe4cb5 into symfony:7.3 Feb 15, 2025
6 of 10 checks passed
@Staormin Staormin deleted the runprocessmessage-fromshellcommandline branch February 17, 2025 09:25
@fabpot fabpot mentioned this pull request May 2, 2025
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.

6 participants