[Console] Remaining regressions due to #33897 #36565
Comments
|
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 |
|
@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. |
To avoid running into symfony/symfony#36565 we add the code that was removed from Symfony to our application.
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. |
|
Interestingly sudo has an option to allow reading the password from STDIN |
|
I played with your reproducer and interesting thing is that loop happens because you use custom exception. If your class extended |
|
Check #36590 |
… session (ostrolucky) This PR was merged into the 4.4 branch. Discussion ---------- [Console] Default hidden question to 1 attempt for non-tty session | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36565 | License | MIT | Doc PR | ### Problem 1 `validateAttempts()` method repeats validation forever by default, until exception extending `RuntimeException` isn't thrown. This currently happens disregarding if user is in tty session where they can actually type input, or non-tty session. This presents a problem when user code throws custom exceptions for hidden questions -> loop doesn't stop. As far as I can tell this issue is in all Symfony versions, but it was uncovered only after we stopped marking interactive flag to false automatically ourselves. Actually, all 3 problems were already existing problems, just hidden until now. ### Problem 2 Infinite loop problem is related to hidden questions, but this one isn't. If validation fails, another attempt to read & validate happens. This means user will get two prompts: 2x same question with 2 different error messages. One error message coming from validator, second error message about inability to read input (because this loop repeats until this kind of error happens, so last output will always be this error). As an example, output in practice would look like following ``` What do you want to do: > [ERROR] Action must not be empty. What do you want to do: > Aborted. ``` So even if loop stops, output is more than expected. ### Problem 3 This is purely cosmetic issue, but currently user gets `stty: stdin isn't a terminal` printed additionally when question helper tries to ask a hidden question without having tty. I have fixed this in same fashion as was already done for [getShell() method](https://github.com/symfony/symfony/blob/ee7fc5544ef6bf9f410f91ea0aeb45546a0db740/src/Symfony/Component/Console/Helper/QuestionHelper.php#L500). ### More details Well root of the first problem is that `\Symfony\Component\Console\Helper\QuestionHelper::getHiddenResponse` is inconsistent. In some cases it does throw `MissingInputException` (which extends `RuntimeException`), in others doesn't. This is because in others, `shell_exec` is used, which won't return `false` even in non-tty sessions. Initially I attempted to fix this and make them consistent by checking for empty result + `isTty` call, but during my testing I found that at least last, `bash -c` method returns `\n` as output both when passing empty input and when passing newline as input. This means we cannot differentiate with this technique when input is really empty, or at least I can't currently tell how, maybe someone does. I had also idea to use proc_open and check if `STDERR` cotains message about stdin not being a terminal, but I realized these functions might not be available. In future we should modernize this method to use less hacky techniques. Other solutions, eg. Inquirer.js or [hoa/console](https://github.com/hoaproject/Console/blob/master/Source/Readline/Readline.php) have much more elegant solutions. Anyway, since I encountered this issue and additionally this doesn't solve Problem 2, I stopped trying to fix this on this level. ### Alternative solution Alternative solution to problem 1 and 3 would be to fallback to default in case of hidden questions when tty is missing. But this still doesn't solve problem 2 and I can't think about solution right now which would fix problem 2 separately. We also didn't really reach consensus if reading passwords via stdin is desired. I tried this in `Inquirer.js` and this library *does read password from stdin* Commits ------- ee7fc55 [Console] Default hidden question to 1 attempt for non-tty session
…tions (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Console] always use stty when possible to ask hidden questions | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36565, replaces #36590 | License | MIT | Doc PR | - The current code doesn't make much sense: we check `hasSttyAvailable()`, and if the answer is `false`, we still use `stty` directly. This PR relies on `stream_isatty` and equivalent fallback checks to decide if the password can be hidden or not. Best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/37469/files?w=1). Commits ------- 055b605 [Console] always use stty when possible to ask hidden questions
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:inputExpectation
Error message saying that two required arguments are missing
Output:
Actual
Infinite loop.
This definitely needs a fix in
AskHiddenhandling to avoid the infinite loop.Additionally it would be helpful to inform users that action argument is missing as well.
Test 2
Forcing detached terminal (no tty) and not providing any input via STDIN,
but providing password argument
echo '' | php console.php hidden:input 123456Expectation
Password argument is set to "123456" and error message that action argument is missing
Output:
Actual
Works in general, but would be helpful to inform users that action argument is missing
Test 3
Forcing detached terminal (no tty) and providing password argument via STDIN
echo '123456' | php console.php hidden:inputExpectation
Password argument is set to "123456" and error message that action argument is missing
Output:
Actual
Works in general, but would be helpful to inform users that action argument is missing.
How to reproduce
Here is an example application
to test out the issues I discovered.
Possible Solution
Probably checking whether the return value is an empty string and setting
$retto false in this case will fix it.Calling
interacteven 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.
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.
The text was updated successfully, but these errors were encountered: