matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!
https://matomo.org/
GNU General Public License v3.0
19.7k stars 2.62k forks source link

[Bug]: User ID tracks all visitors as the same visit/visitor when incorrectly set #21407

Open Starker3 opened 11 months ago

Starker3 commented 11 months ago

What happened?

When the User ID feature is incorrectly configured in the tracking code, users may accidentally start tracking users that are not logged in or when no User ID is found. This usually is the case when using the JS tracker and the variable used for the User ID tracking is undefined or null

In that case Matomo would track all visitors that are not logged in as the same visitor and a lot of visitor information would be effectively lost/useless.

What should happen?

Preferably Matomo should detect when an invalid User ID is set and ignore just that parameter and track the visit like normal, or perhaps send a tracking failure type email to the Super Users about the incorrectly set User ID

How can this be reproduced?

Set the User ID as undefined or null and send a tracking request to Matomo

Matomo Version

Matomo 4

Matomo Patch or Minor Version

4.15.1

PHP Version

8.1

Server Operating System

Debian

What browsers are you seeing the problem on?

Other

Computer Operating System

N/A

Relevant log output

No response

Validations

bx80 commented 11 months ago

Thanks for reporting this @Starker3, I'll assign it to the product team for prioritization :+1:

Stan-vw commented 11 months ago

Can you help me understand in which circumstances the User ID feature becomes incorrectly configured? For example, is the setup very complicated and therefore the risk of incorrectly configuring it is very high? Is the default configuration incorrect? etc.

Also, can you help me understand why you titled it "[Bug]" whereas it sounds that the feature is working, unless it's incorrectly configured? That sounds like it's not a bug

bx80 commented 11 months ago

@Stan-vw It's a bug because the API shouldn't be accepting null as a valid User ID no matter what the source.

The User ID is often supplied to the tracking code from another application such as a CMS or Shop package which for various reasons outside of the site admin's control could break in a way that provides invalid values, in this case Matomo should default to a fail safe approach of ignoring the invalid User IDs. It's better to log individual visits lacking User IDs than to start combining every visit into a single visit from the same 'null' user.

Starker3 commented 11 months ago

As @bx80 mentioned - but I'd like to add that in Cloud I believe the current behaviour would be that a tracking request that contains a null User ID would cause Matomo to ignore that tracking request completely (I.e. nothing is logged) - this was determined by testing so may be an incorrect statement.

So it would be much better to at least log an individual visit without a User ID instead of combining them all into one invalid User ID or discarding the tracking request.

tsteur commented 8 months ago

FYI In cloud we're having a fix applied to this in a RequestProcessor using a code like below:

public function manipulateRequest(Request $request)
{
    $uid = $request->getParam('uid');
    if (!empty($uid)) {
        $uid = trim($uid);
        $uidLower = Common::mb_strtolower($uid);
        if (empty($uid)
            || in_array($uidLower, ['null', 'undefined', 'nil', 'none'], true)
            || in_array($uid, ['NaN'], true)) {
            // avoid programming errors where people end up having uid=undefined and then we end up having
            // hundred thousands of actions in one visit 
            $request->setParam('uid', '');
        }
    }
}

If we do this change in core, it will be a breaking change and some users may want this behaviour. We would definitely want to document this clearly in our developer guides and FAQs.