joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

[5.2] deprecated warning in error_log caused by detectEngine() if $userAgent contains "Chrome" but no version after a space character #44469

Open ssabatini opened 2 days ago

ssabatini commented 2 days ago

Steps to reproduce the issue

Access WebClient with a user agent containing "Chrome" but no version after a space character.

Expected result

No warning in error log

Actual result

2 warnings in error log:

[Tue Nov 12 23:04:25.666104 2024] [fcgid:warn] [pid 1015922] [client xxx] mod_fcgid: stderr: PHP Warning:  Undefined array key 1 in /home/httpd/vhosts/cuul.ch/j4x.cuul.ch/libraries/vendor/joomla/application/src/Web/WebClient.php on line 389
[Tue Nov 12 23:04:25.667085 2024] [fcgid:warn] [pid 1015922] [client xxx] mod_fcgid: stderr: PHP Deprecated:  explode(): Passing null to parameter #2 ($string) of type string is deprecated in /home/httpd/vhosts/cuul.ch/j4x.cuul.ch/libraries/vendor/joomla/application/src/Web/WebClient.php on line 389
richard67 commented 2 days ago

This issue has to be fixed in the application framework https://github.com/joomla-framework/application .

Unfortunately I think it will not be fixed by the already merged PR https://github.com/joomla-framework/application/pull/131 .

See also this issue there which seems to have the same root cause: https://github.com/joomla-framework/application/issues/112 . I have added a comment to that issue with reference to this issue here.

richard67 commented 2 days ago

Please test https://github.com/joomla-framework/application/pull/132 and report back the result in a comment there. Thanks in advance.

richard67 commented 2 days ago

@ssabatini Could you post the exact user agent string here so I can use that for a unit test in my PR mentioned in my previous comment? Thanks in advance.

ssabatini commented 23 hours ago

On Tue Nov 12 23:04:25, I find this 2 entries in the Apache access log:

x.x.x.x - - [12/Nov/2024:23:04:25 +0100] "GET /.well-known/traffic-advice HTTP/1.0" 200 1401 "-" "Chrome Privacy Preserving Prefetch Proxy"
x.x.x.x - - [12/Nov/2024:23:04:25 +0100] "GET /de/ HTTP/1.0" 200 15321 "https://www.google.com/" "Mozilla/5.0 (Linux; Android 10; K) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36"

Probably, it was the first one, as this user string does not contain a slash ('/') character. It mets the 1st condition (contains 'Chrome'), but explode will not find a slash and therefore result[1] is null (non-existing):

        } elseif (\stripos($userAgent, 'Chrome') !== false) {
            $result  = \explode('/', \stristr($userAgent, 'Chrome'));
            $version = \explode(' ', $result[1]);

            if ($version[0] >= 28) {
                $this->engine = self::BLINK;
            } else {
                $this->engine = self::WEBKIT;
            }

I see another problem here (and in other places): The code assumes that the value after the slash is a number. This will fail as well if the user agent is something like 'Chrome/abc', version[0] will then contain 'abc' and I wonder about the result of ('abc' >= 28) ...

richard67 commented 22 hours ago

@ssabatini Yes, problem is the first one. The user agent "Chrome Privacy Preserving Prefetch Proxy" is used by the https://developer.chrome.com/blog/private-prefetch-proxy and does not provide a version by purpose.

If you check the changes made by my pull request, which is currently ongoing work, you will see that the issue is handled with my PR: https://github.com/joomla-framework/application/pull/132/files

Because there is no version appended with a slash, the isset($result[1]) will be false, so $version will be false, and then I check with if ($version === false || $version[0] >= 28) {, and for the mentioned user agent without version we can assume a version > 28 because that feature was introduced in Chrome with version 103 for Android.

The reason why my PR is work in progress is because I am not sure yet if I shall handle any theoretical case of user agents without a version or only this well know one mentioned above.

Anyway: Instead of having a theoretical discussion here, please test if the changes from my PR solve your issue and report back the result in a comment there. Thanks in advance.