in2code-de / in2publish_core

in2publish Community Version
https://www.in2code.de/produkte/content-publisher/
GNU General Public License v3.0
40 stars 23 forks source link

SshConnection also needs to be sanitized like RCE API #26

Closed AndreasA closed 7 years ago

AndreasA commented 7 years ago

The response of SshConnection also needs to be sanitized like in RCE API. Otherwise invalid / partial invalid responses that are handled with RCE API, are not handled during SshConnection::transferTemporaryFile

AndreasA commented 7 years ago

Probably SshConnection should use RCE API during executeRemoteCommand. That way files can be transferred correctly using SshConnection and for other stuff, the RCE API is always used.

AndreasA commented 7 years ago

@vertexvaar: Shouldn't https://github.com/in2code-de/in2publish_core/blob/1eece4eaf5f54fb6164b315960c3b4b32c91e989/Classes/Security/SshConnection.php#L320

and https://github.com/in2code-de/in2publish_core/blob/1eece4eaf5f54fb6164b315960c3b4b32c91e989/Classes/Security/SshConnection.php#L390

Also use RCE API?

vertexvaar commented 7 years ago

chmod-ing via CLI does not check if the operation was successful, so the output is completely ignored. It was added as a workaround for PHP 7 clients and will stay there until the file transfer stuff received its overhaul. Checking for an existing directory does also not rely on stdout but on the exit status of the actual command. If there are BOM characters they should be simply ignored. (I don't expect BOMs to appear in front of the echo $? CMD, rather in your /etc/motd file. So it's safe to ignore those places, as they will be replaced eventually. But still, thank you for your investigation and the time and effort you put into this, it's still very welcome and i'm looking further to receive even more 👍

AndreasA commented 7 years ago

Should be true but I meant in regards to consistency it would be good to always use RCE API and deprecate executeRemoteCommand method completely.

vertexvaar commented 7 years ago

You are absolutely right, but i don't want to do it twice 'cause the overhaul awaits ;)