640 triaging issue

[alessandro-fazzi]

This PR is just a triage for @castilma 's issue #640 . The CI will run only dedicated test.

This is not meant to be merged.

[castilma]

Where is the code that creates this commandline normally? The escaping may happen when the commandline is constructed.

[castilma]

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

found it:here
(the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets.
So the regex used for substituting secrets should search for shellescaped secrects!

Another potential issue:
Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input.
I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings.
That way you don't need to worry about string escaping! You just pass an array of the used arguments.

[alessandro-fazzi]

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

No problem; you'd be welcome anyway :)

found it:here (the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets. So the regex used for substituting secrets should search for shellescaped secrects!

This seems a good catch to me! But it seems more an endemic problem the fact we're logging escaped commands more than thinking about searching for escaped secrets. I have to tinker a bit on this.

Another potential issue: Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Why do you say system will call «whatever the user's current shell is»? In my knowledge system calls, in any form, should use the standard shell; which means, in *nix systems, /bin/sh. Would you mind to point me where you found that information? TIA

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input. I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings. That way you don't need to worry about string escaping! You just pass an array of the used arguments.

If your point is that it's difficult to use system due to the occurring shell expansion, keep in mind that the method has the same overloaded signature as exec. If that was not your point then I'm not catching it, sorry.

Instead I'm not prone to introduce threaded patterns (fork) because I've not enough knowledge to control them and sincerly I do not take what benefit it should bring on the table here. But I'm not interested in preventing a discussion about that if I am missing something you consider important :)

---

Thanks for the effort; I'll be on this as soon as I'll have a chunk of spare time

[castilma]

The system() documentation refers to exec(), which uses /bin/sh by default, but only after looking the environment variable RUBYSHELL.
(So I was wrong/unclear, but am right now :))

I did not notice that one could use system() without a subshell. So, one could just change to use system() with each argument separately passed.

[alessandro-fazzi]

Thanks for the patience and feedback.

I've never used that signature of #system, but my feeling is that it could be more functional and more straight to read/write.

I'll deepen on it a bit and will try to bring out something useful. For sure I won't target the 5.x version BTW.

[castilma]

If you want to do this in sometime, don't just forget it, because the stale bot closed the issue.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.