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] No user-agent has been set #946

Closed
trendoman opened this issue Oct 27, 2023 · 18 comments
Closed

[bug] No user-agent has been set #946

trendoman opened this issue Oct 27, 2023 · 18 comments
Assignees

Comments

@trendoman
Copy link

Following pops up in PHP error log:

PHP Fatal error: Uncaught Detection\Exception\MobileDetectException: No user-agent has been set. in .../Mobile-Detect-4.8.03/src/MobileDetect.php:1369

@trendoman
Copy link
Author

Now that I have checked that line 1369 I can see that it's not a bug but a feature 🥇 but it's spamming the log too much, so please give instruction to have that off.

if (!$this->hasUserAgent()) {
throw new MobileDetectException('No user-agent has been set.');
}

@serbanghita
Copy link
Owner

serbanghita commented Oct 27, 2023

@trendoman in what environment are you using this? In CLI, with Apache?

I just tested this with MobileDetect 4.8.03

$detect = new MobileDetect();
// $detect->setUserAgent($_SERVER['HTTP_USER_AGENT']);
$isMobile = $detect->isMobile();
$isTablet =- $detect->isTablet();
$deviceType = ($isMobile ? ($isTablet ? 'tablet' : 'phone') : 'computer');
$scriptVersion = $detect->getVersion();

var_dump($deviceType);
var_dump($scriptVersion);

and it works by automatically looking at $_SERVER

You could also explicitly setUserAgent like in the example https://github.com/serbanghita/Mobile-Detect/blob/4.8.x/scripts/example.php

@trendoman
Copy link
Author

trendoman commented Oct 27, 2023

I have some PHP script code running at this dev environment [link edited out] and so far very few people come there to upload and test other scripts. It makes me worry bc on production I expect many more visitors. The line which produces exceptions is very basic, pretty much boils down to this —

$MB = new \Detection\MobileDetect();
var_dump( $MB->isMobile() );

I had a v.3 previously without issues, but recently tried the fresh MD 4.8.03 and now stuck with spam in error log. Any advice appreciated. Should I validate a non-empty user-agent for each visitor before creating MD object? That's possible, however I'd rather rely on MD to have that sorted out without throwing fatal errors, if that's ok with you.

Edit: remove link to private server

@serbanghita
Copy link
Owner

@trendoman
From the phpini it seems that $_SERVER exists:

$_SERVER['HTTP_USER_AGENT'] | Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/118.0.0.0 Safari/537.36

I believe the $_SERVER variable is being deleted by the CMS and MobileDetect cannot auto-detect the HTTP_USER_AGENT header. I see a dump on your website to __ROOT__ array/object with the key of k_device_useragent.

In my case:

k_device_useragent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/118.0

try to set: $detect->setUserAgent(...);

@serbanghita
Copy link
Owner

You should never auto-update scripts that change major versions. In this case from 3.x to 4.x. The reason is that the packages might contain breaking changes. See semver

@serbanghita
Copy link
Owner

@trendoman you can go back to using 3.x, I'm still updating that version as well.

@trendoman
Copy link
Author

Thanks, this advice to get back to 3.x confirms my understanding of the issue.

Just a few last notes FYI (and others that will search for this issue):

  • no, the $_SERVER variable is not being deleted by the CMS, because the CMS is not used (yet) at the point where I test MB
  • yes, you see a dump on the website to __ROOT__ array/object with the key of k_device_useragent — I added that dump output in case you stumble upon it (if visited Index out of curiosity) — as I set that key for every visitor and make it available to later CMS vars. The original problem is about random empty user-agent strings (perhaps from internet bots) which spam error log with Fatal error and Uncaught exception. Hence I can not set the UA unless adding extra code. Your choice of throwing an exception for every random bot without UA is thus questioned and is labelled as bug.
  • there is no any autoupdate of scripts in my usecase, but I agree that this advice is universally good so thanks

I'm probably going to resolve this (to keep using 4.x) by adding an extra try-catch around MB to avoid constant error log errors (from random bots/people without UA). I prefer error log for important stuff (much more for fatal error).

Thanks for your amazing work and attending to issues, nevertheless.

@serbanghita
Copy link
Owner

The original problem is about random empty user-agent strings (perhaps from internet bots) which spam error log with Fatal error and Uncaught exception.

Okay, now I fully understand the issue.
When I changed the code behavior's in 4.x, I haven't thought about empty userAgent string scenario. I converted empty userAgent to null, thus throwing later an Exception. I think this is bad for developer experience.

I have released 4.0.04 which hopefully fixes your problem https://github.com/serbanghita/Mobile-Detect/releases/tag/4.8.04

Cheers 🙇

@trendoman
Copy link
Author

Cheers 💯

This makes redundant an unecessary complication with try-catch and is - by far - more friendly to developers.

Now I can get back and throw away the ugly —

$MB = new \Detection\MobileDetect();

try {
    $isMobile = $MB->isMobile();
    $isTablet = $MB->isTablet();
} catch ( Exception $e ) {
    //https://github.com/serbanghita/Mobile-Detect/issues/946
} finally {
    $isMobile = ( $isMobile == 1 ) ? 1 : 0;
    $isTablet = ( $isTablet == 1 ) ? 1 : 0;
}

Have a great weekend!

@penafelipe
Copy link

penafelipe commented Oct 28, 2023 via email

@bopoda
Copy link

bopoda commented Jan 10, 2024

Hi guys.
I would like to rise a question: does it make sense to throw an exception if User-Agent is empty?
We can wrap with try/catch in our code when calling methods of the library, but that's not convenient. I did not catch, how can we throw it away?

Regarding web: on the one hand, that's logical that user-agent should exist when request comes. On the other hand, if User-Agent does not exist (is empty) then the backend should still return OK response in most of the cases, I guess. That's why I consider throwing exception as redundant thing. Can we just initialize with empty string?

Regarding this part of code:
https://github.com/serbanghita/Mobile-Detect/blob/4.8.04/src/MobileDetect.php#L1088-L1108

// Setting new HTTP headers automatically resets the User-Agent.
// Set current User-Agent from known User-Agent-like HTTP header(s).
$userAgent = "";
foreach ($this->getUaHttpHeaders() as $altHeader) {
    if (!empty($this->httpHeaders[$altHeader])) {
        $userAgent .= $this->httpHeaders[$altHeader] . " ";
    }
}

if (!empty($userAgent)) {
    $this->setUserAgent($userAgent);
}

Empty string is not set if we pass empty string User-Agent in header, for example:
curl -v http://127.0.0.1:8000/ -H "User-Agent: "
So when handling http-request, if I call methods of library, I will receive an MobileDetectException.

Does it make sense to change:

if (!empty($userAgent)) {
    $this->setUserAgent($userAgent);
}

to:

$this->setUserAgent($userAgent);

And then remove this condition from all the methods?

if (!$this->hasUserAgent()) {
            throw new MobileDetectException('No user-agent has been set.');
}

The aim: avoid exceptions if the request was sent with empty User-Agent.
Please correct me If I'm wrong.

@trendoman
Copy link
Author

Please correct me If I'm wrong.

Not wrong. I personally found that I do not need exceptions if no user agent exists. If it does not exist I make it empty myself. Here is how I decided to do this in my app —

$MB = new \Detection\MobileDetect();
if( false === $MB->hasUserAgent() ){ $MB->setUserAgent(''); }

// then goes app's logic unrelevant to the topic but possibly a helpful context

if( !defined('K_USERAGENT_MOBILE') ) define( 'K_USERAGENT_MOBILE', $MB->isMobile() );
if( !defined('K_USERAGENT_TABLET') ) define( 'K_USERAGENT_TABLET', $MB->isTablet() );
unset($MB);

if( K_USERAGENT_MOBILE || K_USERAGENT_TABLET ) $k_cache_dir = K_COUCH_DIR . 'cache/mobile/';

As long as there is logic in your app about how to treat an empty ua, you are fine. The problem was with non-existent ua in the first place, which has been fixed by the @serbanghita

@serbanghita
Copy link
Owner

@bopoda Clearly this seems to be an issue for PHP users that I didn't expect to be

My current logic is:

  1. new MobileDetect() auto-initiates discovery of HTTP headers and implicitly the User-Agent header.
  2. If $_SERVER['HTTP_USER_AGENT'] + friends is found, then set $this->userAgent to a string value.
  3. At this step $md->isMobile() or any other detection method throws Exception if $this->userAgent is not a string.

Programmatically speaking this is okay. From a UX pov it appears to be a bad decision.

I thought that the user wants a mechanism to be informed that User-Agent HTTP header is missing.
I guess I can get rid of the exception since it might caught developers off-guard.

I believe the solution here is that when userAgent is not set on auto-initiation phase, I will set an empty string

@serbanghita
Copy link
Owner

if( false === $MB->hasUserAgent() ){ $MB->setUserAgent(''); }

@trendoman while this is fine, my aim was for users to use try/catch in PHP, I don't want you to be bothered to write extra code.

I think the point here is that the library exists for a long time and it has been used in a certain way and throwing exceptions is not really something that developers need in this case

@serbanghita
Copy link
Owner

I'll issue a patch to fix this behavior for when User-Agent is not present

@trendoman
Copy link
Author

I think the point here is that the library exists for a long time and it has been used in a certain way and throwing exceptions is not really something that developers need in this case

I get it.

Quite anecdotally, I recently was able to find a solution to irrelevant issue in an app thanks to UA being not set e.g. caught a cURL request with a missing UA to a 404 page. This was nice to not have the UA empty in that particular case, because I was able to find the problem faster.

@bopoda
Copy link

bopoda commented Jan 10, 2024

$this->mobileDetect->setUserAgent('');

yeah, this call before "auto-discovery" helps to avoid Exception.
In my opinion, invalid/missing header is not so big problem (but it depends).

I thought that the user wants a mechanism to be informed that User-Agent HTTP header is missing.
I guess I can get rid of the exception since it might caught developers off-guard.
I believe the solution here is that when userAgent is not set on auto-initiation phase, I will set an empty string

Yeah, got it, makes sense to inform the user somehow.

The case I would like to mention: If I have service and if someone sends http-requests with empty User-Agent -- FE 500 will be triggered if I don't handle it. It could trigger monitoring, spoil logs, etc. So I need to avoid 500 as this is the problem not from the server side.

  • wrap with try/catch (also, possible)
  • initialize mobileDetector with an empty string (I would prefer this one). So In general I can continue using it with no changes as we have this solution. It was a bit unexpected to see new Exceptions after the library update (yeah, but it's mentioned in new version of library)

Thank you!

serbanghita added a commit that referenced this issue Jan 10, 2024
…ot set. By default "autoInitOfHttpHeaders" is "true", and if UserAgent is not found, it will be set to empty string ""

#946 (comment)
@serbanghita
Copy link
Owner

serbanghita commented Jan 10, 2024

New patch https://github.com/serbanghita/Mobile-Detect/releases/tag/4.8.05

By default it will no longer throw an Exception, it just sets the userAgent to empty string, see tests

/**
* @throws MobileDetectException
*/
public function testNoUserAgentSet()
{
$detect = new MobileDetect();
$this->assertFalse($detect->isMobile());
}
/**
* @throws MobileDetectException
*/
public function testEmptyStringAsAUserAgent()
{
$detect = new MobileDetect();
$detect->setUserAgent('');
$this->assertFalse($detect->isMobile());
}

It will throw an exception only when starting MobileDetect with 'autoInitOfHttpHeaders' set to false

/**
* @throws MobileDetectException
*/
public function testNoUserAgentSetAndAutoInitOfHttpHeadersIsFalse()
{
$this->expectException(\Exception::class);
$this->expectExceptionMessage('No valid user-agent has been set.');
$detect = new MobileDetect(null, ['autoInitOfHttpHeaders' => false]);
$detect->isMobile();
}

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