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

[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 this issue Nov 17, 2024 · 5 comments

Comments

@ssabatini
Copy link

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
Copy link
Member

richard67 commented Nov 17, 2024

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 joomla-framework/application#131 .

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

@richard67
Copy link
Member

Please test joomla-framework/application#132 and report back the result in a comment there. Thanks in advance.

@richard67
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

@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.

@Hackwar Hackwar added the bug label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants