Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prompts does not check for a TTY #64

Closed
Spice-King opened this issue Sep 11, 2023 · 1 comment · Fixed by #73
Closed

Prompts does not check for a TTY #64

Spice-King opened this issue Sep 11, 2023 · 1 comment · Fixed by #73
Assignees
Labels

Comments

@Spice-King
Copy link

Spice-King commented Sep 11, 2023

Laravel Prompts Version

0.1.6

Laravel Version

10.22.0

PHP Version

8.2.10

Operating System & Version

Linux 6.4.6

Terminal Application

N/A, this is about TTYless enviroments and thus no terminal

Description

Prompts does zero checking if standard input is a tty. Without an interactive tty, which can be caused running commands from cron or in Docker without allocating one, this causes stty here to fail with stty: standard input: Not a tty. Since there is zero checks to see if this was successful or checking that it's on a tty separately, the code always assumes it is on a tty. When this while loop is entered, there are no time out checks to break out of the loop, nor is there a there something to slow it down since fread(STDIN, 1024) here instantly returns zero length string with out a tty (assuming nothing has been piped in there, or has otherwise been drained).

This has the following consequences, assuming that scheduled commands in cron trigger this, 100% CPU thread utilization per instance started and slow memory exhaustion as concurrently running commands pile up.

There are a few ways I can see fixing this, from detecting the lack of a TTY & throwing an exception to end everything, to accepting default options. My assumption is detect and throw is the better one, but I'm happy to be proven wrong.

Steps To Reproduce

  1. Add a command that uses Illuminate\Console\ConfirmableTrait to the scheduler in Laravel, while forgetting to set --force for production
    • Alternatively, upgrade a package you already have a scheduled command for that migrated to using laravel/prompts, and not realize the consequences that change causes.
  2. Wait for schedule to execute the command without a TTY.
    • Alternatively, you can run a command in your shell this way to execute it without a TTY attached: (setsid php artisan activitylog:clean) </dev/null
    • Yes, the parentheses are needed, use of the subshell avoids setsid forking and going into the background, and the /dev/null redirect is to remove keyboard input, like it would be while cron runs.
  3. Profit!
@jessarcher
Copy link
Member

Thanks for reporting, @Spice-King.

I think #63 will indirectly help with this, and #57 (comment) will fill in the gaps.

I will finish implementing #57 and verify it will pass your reproduction steps. My thoughts are that if it can return a configured default value or an empty value that passes any validation, then it will do so. Otherwise, it will throw an exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants