completions/magento: Fixes module aggregation for module related commands #10446
Conversation
…mands Previousely when attempting completion for commands `module:enable`, `mmodule:disable` and `module:uninstall` and error would be disaplyed, stating that "magento" was not found. Upon inspection of the issue in the related completion script it became clear that: 1. The shell command `magento` does not exist as the CLI script of Magentoresides under `bin/magento`. 2. The module aggregation would not work after referncing the appropriate CLI command as an undeclared variable was being introspected. 3. Using Magento's CLI command took too long to respond as it has to bootstrap the whole Magento stack in order to deliver modules. Thus the whole aggregation was rewritten to a form that actually works and reduces the aggregation to reading the appropriate information directly from the configuration file, provided that the file exists and PHP is installed.
share/completions/magento.fish
Outdated
| command -q php; or return | ||
|
|
||
| php -r ' | ||
| $config = include "app/etc/config.php"; |
There was a problem hiding this comment.
I'm going to be honest: I am uncomfortable with completions running php files in the current working directory just by name, when the name is this generic.
Is there another way?
There was a problem hiding this comment.
I'm not sure i understand what you mean. Can you elaborate?
There was a problem hiding this comment.
include is php's equivalent of source in shell. It runs the given file and keeps the definitions around.
That means, if your current directory includes a file in "app/etc/config.php", completing magento would run that entire file, no matter what code it contained. It could do anything.
And the name doesn't mention "magento" anywhere, it's conceivable that a file at that place would exist and not be meant for magento at all.
There was a problem hiding this comment.
Ah, I get your point. Yes that that is a security threat indeed. In Magento projects the file app/etc/config.php is used to define multiple things that are necessary for Magento to run, amongst others the list of active and inactive modules. Usually the file contains a huge array that is being returned, e.g.:
return [
'modules' => [
'Module_Name1' => 1,
'InactiveMModule_Name1' => 0,
...
],
'config' => [
...
]
];Magento itself includes the files in the same manner as I did in the code of my PR. Yet there is always a risk, that someone has added malicious code to the file, as you have elaborated. This would result in an infected Magento installation as the same file is being used in production, which would bring the thing to a completely different level.
Yet, you are right in feeling bad about this as it could be an attack targeted at fish only. Would it be an option to execute the code in a subshell? If not, I'd revert the code to execute bin/magento and accept that it is less performant.
There was a problem hiding this comment.
Would it be an option to execute the code in a subshell?
The code is unable to change anything inside fish. It runs in a php process.
But that php process can do arbitrary things, including change any file or run any other command.
The shell isn't the problem, so a subshell wouldn't help.
I'd revert the code to execute bin/magento and accept that it is less performant
That would be preferable, but still not ideal. Really all it would change is that we could blame magento for using a script file (instead of e.g. a json file) as its config.
So I'm not sure if we want to include this at all and shouldn't just accept that we can't complete magento without running arbitrary code.
There was a problem hiding this comment.
That would be preferable, but still not ideal. Really all it would change is that we could blame magento for using a script file (instead of e.g. a json file) as its config.
True yet that is the state it was in before I fixed the error. Of course this whole part of the completion could be dumped but considering that usually a lot of modules are being used by Magento, this part of the completion is quite comfortable to have. What would you suggest?
There was a problem hiding this comment.
Having thought about this some more, I believe this part of the completion needs to be removed. Anyone who wants it can get it elsewhere, but fish completions executing random scripts in $PWD is surprising and a potential problem, especially considering how we changed the git prompt to avoid such things.
Sorry.
If you want, you can change it here, or I'll close this and do it myself.
There was a problem hiding this comment.
I get your point and it definitely is a valid one, even though this part of the completion is a real gain to every day work with Magento. If that is Ok with you, I'd like to take a couple of days to think about an alternative approach.
There was a problem hiding this comment.
I reimplemented said completion using a sed approach. Better?
…commmands to not use PHP script Executing random scripts from fish completion poses a threat to the system. While this would indicate that the Magento installation has been corrupted, it still is better to not run `app/etc/config.php` to get hold of the modules. Thus the module aggregation was rewritten to make use od `sed` instead, which has the additional benefit of being faster than using PHP.
…d `module:status`
Description
Previousely when attempting completion for commands
module:enable,mmodule:disableandmodule:uninstalland error would be disaplyed, stating that "magento" was not found.Upon inspection of the issue in the related completion script it became clear that:
magentodoes not exist as the CLI script of Magento resides underbin/magento.Thus the whole aggregation was rewritten to a form that actually works and reduces the aggregation to reading the appropriate information directly from the configuration file, provided that the file exists and PHP is installed.
Note that no issue has been filed for this behaviour, yet the fix also removes
test -nfrom the completion script thus contributing to #6520TODOs: