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

Bug: is_cli() returns true on Apache #5336

Closed
kenjis opened this issue Nov 16, 2021 · 7 comments · Fixed by #5393
Closed

Bug: is_cli() returns true on Apache #5336

kenjis opened this issue Nov 16, 2021 · 7 comments · Fixed by #5393
Assignees
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@kenjis
Copy link
Member

kenjis commented Nov 16, 2021

PHP Version

7.4

CodeIgniter4 Version

4.1.5

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

No response

What happened?

is_cli() returns true after upgrade to 4.1.5 from 4.0.

Steps to Reproduce

Navigate to the site.

Expected Output

is_cli() returns false.

Anything else?

From https://forum.codeigniter.com/thread-80530-post-391559.html

The environment is:

  • Apache 2.0 Handler
  • no $_SERVER['HTTP_USER_AGENT']
  • has $_SERVER['argv']
  • has $_SERVER['REMOTE_ADDR']
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 16, 2021
@vlakoff
Copy link
Contributor

vlakoff commented Nov 16, 2021

@vlakoff
Copy link
Contributor

vlakoff commented Nov 16, 2021

It's also what Symfony does, see in VarDumper.php:

\in_array(\PHP_SAPI, ['cli', 'phpdbg'], true)

I investigated a bit, and it seems it would be useless to keep the if (defined('STDIN')) in addition.

@kenjis
Copy link
Member Author

kenjis commented Nov 17, 2021

CI3:

return (PHP_SAPI === 'cli' OR defined('STDIN'));

https://github.com/bcit-ci/CodeIgniter/blob/develop/system/core/Common.php#L378-L382

@lonnieezell
Copy link
Member

PHPUnit also uses the phpdbg check, so that's probably a good addition.

@paulbalandan
Copy link
Member

I did a bit of pondering and I think the compound isset should be split to 2 isset calls:

-! isset($_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_USER_AGENT'])
+! isset($_SERVER['REMOTE_ADDR']) && ! isset($_SERVER['HTTP_USER_AGENT'])

In the original, the two server variables are evaluated as a single condition, that's why when one fails, the whole condition is true. They should be checked individually.

If multiple parameters are supplied then isset() will return true only if all of the parameters are considered set.

! isset($_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_USER_AGENT']);
// ! isset(true, false)
// ! false
// true # unintended

! isset($_SERVER['REMOTE_ADDR']) && ! isset($_SERVER['HTTP_USER_AGENT']);
// ! true && ! false
// false && true
// false # intended

@vlakoff
Copy link
Contributor

vlakoff commented Nov 19, 2021

It's absolutely possible for $_SERVER['HTTP_USER_AGENT'] to be missing because the user agent omitted it.

At the very most it could be considered a hint, but we definitely can't rely on it.

@kenjis
Copy link
Member Author

kenjis commented Nov 26, 2021

I sent a PR #5393.

@kenjis kenjis removed the good first issue Good for newcomers label Nov 26, 2021
kenjis added a commit to kenjis/CodeIgniter4 that referenced this issue Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants