[Console] Remaining regressions due to #33897

[helhum]

Symfony version(s) affected: 4.4.7 / 5.0.7

Description
#33897 caused a regression, which I would call a BC break.

In general I agree with the changes made with the intention to allow
providing input to the application using STDIN.
There are however some edge cases demonstrated here that need some attention.

Tests with current state (symfony/console 5.0.7/4.4.7)

Test 1

Forcing detached terminal (no tty) and not providing any input via STDIN
echo '' | php console.php hidden:input

Expectation

Error message saying that two required arguments are missing
Output:

Not enough arguments (missing: "password, action").

Actual

Infinite loop.
This definitely needs a fix in AskHidden handling to avoid the infinite loop.
Additionally it would be helpful to inform users that action argument is missing as well.

 Password:
 > stty: stdin isn't a terminal
stty: stdin isn't a terminal


                                                                                
 [ERROR] Password must not be empty.                                            
                                                                                

 Password:
 > stty: stdin isn't a terminal
stty: stdin isn't a terminal


                                                                                
 [ERROR] Password must not be empty.                                            
                                                                                
...

Test 2

Forcing detached terminal (no tty) and not providing any input via STDIN,
but providing password argument
echo '' | php console.php hidden:input 123456

Expectation

Password argument is set to "123456" and error message that action argument is missing
Output:

Not enough arguments (missing: "action").

Actual

Works in general, but would be helpful to inform users that action argument is missing

 What do you want to do:
 > 

                                                                                
 [ERROR] Action must not be empty.                                              
                                                                                

 What do you want to do:
 > 
            
  Aborted.  
            

hidden:input <password> <action>

Test 3

Forcing detached terminal (no tty) and providing password argument via STDIN
echo '123456' | php console.php hidden:input

Expectation

Password argument is set to "123456" and error message that action argument is missing
Output:

Not enough arguments (missing: "action").

Actual

Works in general, but would be helpful to inform users that action argument is missing.

 Password:
 > stty: stdin isn't a terminal
stty: stdin isn't a terminal


 What do you want to do:
 > 
            
  Aborted.  
            

hidden:input <password> <action>

How to reproduce
Here is an example application
to test out the issues I discovered.

Possible Solution

1. The infinite loop is triggered, because the hidden response is empty, but it is checked for false

Probably checking whether the return value is an empty string and setting $ret to false in this case will fix it.

2. Calling interact even when no tty is available.
   That is a tough one, as doing so is major part of the initial fix.
   It could be mitigated, by wrapping the call in a try/catch, catching MissingInputException, setting to non iteractive in the catch and letting the application continue. This would in my case at least output the error message about the missing argument in the end.

3. Cluttered output
   Since the output for the questions is already sent, the output will look much more cluttered than before. Depending on the type of the cli application, this might be acceptable, but as well might not.

This could be mitigated by an additional Application option/property whether to evaluate the existance of a tty and set interactive to false in this case or not.
If this would be accepted, it would be up for decision which the default behaviour should be.

[ostrolucky]

Yeah I must say I wasn't thinking about hidden inputs at all. Anybody knows what's expected behaviour regarding stdin for hidden inputs in unix tools? We might disable this behaviour for hidden inputs perhaps. @javiereguiluz you are good at these things. Do you know some other standard CLI tools that ask users for passwords, similarly like sudo? What happens when doing echo hereismypassword | some_program_asking_for_password?

[javiereguiluz]

@ostrolucky I'm sorry but this is tricky for me too. I'm not sure how this works in general. I did a quick test for example with PostgreSQL CLI and it didn't work:

$ echo "foo" | psql database_name --password
Password:

$ echo "foo\n" | psql database_name --password
Password:

I can't make it work in any way.

[helhum]

I did a quick test for example with PostgreSQL CLI and it didn't work

That is my knowledge as well, that hidden / password input can never be passed via STDIN for security reasons. I got side tracked, by some testing where I thought I could pass a password via STDIN to the mysql binary, but my tests today lead to the same result as @javiereguiluz showed.

With that knowledge, I'm not sure any more whether the introduced behaviour is generally useful, or if there are use cases where the previous behaviour is desired.

I decided to normalize the behaviour for different symfony/console versions in the application I manage, by re-introducing the removed code.

[helhum]

Interestingly sudo has an option to allow reading the password from STDIN

[ostrolucky]

I played with your reproducer and interesting thing is that loop happens because you use custom exception. If your class extended Symfony\Component\Console\Exception\RuntimeException, you wouldn't get a loop at least.

[ostrolucky]

Check #36590