[Process] Update PHPDoc to use proper placeholder syntax #45197
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| @@ -180,6 +180,9 @@ public function __construct($command, string $cwd = null, array $env = null, $in | |||
| * $process = Process::fromShellCommandline('my_command "$MY_VAR"'); | |||
| * $process->run(null, ['MY_VAR' => $theValue]); | |||
| * | |||
| * It is also recommended to use double quotes around placeholders. This will help ensure the value | |||
There was a problem hiding this comment.
this is already strongly recommended just above. Feel free to improve the above sentence if you want, but recommending twice looks duplicate to me.
but note that the comment is wrong: since #34848, ppl should use "${:MY_VAR}" and not "$MY_VAR".
There was a problem hiding this comment.
Ok, using "${:MY_VAR}" should address my concern. If the developer leaves off the double quotes, the substitution won't work, so they'll be less likely to create an argument injection vulnerability. I've updated the PR to use the "${:MY_VAR}" syntax in the phpdoc here.
|
Thank you @chrismcgehee. |
I'd like to add this PHPDoc comment to help make sure
Process::fromShellCommandlineis used securely. The other day, one of the developers at my company wrote some code that was roughly like:Since
$fileNameis user input, he thought he was doing the secure thing by using placeholders. The issue is that a malicious user could have utilized the-execoption offindto gain arbitrary code execution, for example$fileName = '. -exec echo Foo! ;'. This can be fixed by simply surrounding$FILENAMEwith double quotes because this passes the input as a single argument tofindinstead of passing it as multiple arguments. I believe there are enough programs out there that can be manipulated if an attacker is able to control multiple arguments that it's worth putting a warning here to help prevent the mistake of not surrounding placeholders with quotes.