Limit the fingerprint #15886

tsteur · 2020-04-29T21:41:04Z

The purpose of the fingerprint is to put all tracking requests within the same visit into the same visit in the DB. A fingerprint is basically valid for up to 30 minutes after the visit ends (unless different visit length is configured).

By adding a random hash to the fingerprint we make sure when a user disables cookies, a visitor cannot be identified across multiple days and such no cookie consent should be needed.

This change should not really change behaviour for anyone AFAIK. In Matomo 4 we could remove the check for create_new_visit_after_midnight potentially. We cannot limit the fingerprint in that case as when someone has create_new_visit_after_midnight=0 and has cookies disabled, then a new visit would be still created after midnight since the configId would change.

The limited fingerprint will also not apply when enabling [Tracker]enable_fingerprinting_across_websites = 1 since different sites can have different timezones and therefore we can't add any date to the fingerprint.

refs #13655

mattab · 2020-04-30T23:00:01Z

config/global.ini.php

@@ -761,6 +761,15 @@
 ; Note: setting this to 0 increases your users' privacy.
 enable_fingerprinting_across_websites = 0

+; When enabled (set to 1) and cookies are disabled in the tracker, then the fingerprint will be restricted even further
+; to prevent a visitor to be tracked over multiple days. This means typically that no cookie consent is needed when cookies
+; are disabled. You may disable this feature if you don't want to use cookies in the browser, but still be able to get more


You may disable this feature if you don't want to use cookies in the browser, but still be able to get more accurate unique visitor numbers.

Actually afaik, you would disable this feature only if you have set a window_look_back_for_visitor value higher than 12 or 24 hours. By default window_look_back_for_visitor = 0 which means we only look back 30min to match a visit. Returning visitors wouldn't even be matched from 2 hours ago. So it's only when cookies are disabled and window_look_back_for_visitor > say 43200 then it might be useful to deactivate this.

Are you certain fingerprint is only looked back 30 minutes by default? Then I'd remove the config setting, and also the extra tracking parameter cd when cookies are disabled and we simply do apply the shorter fingerprint always unless window_look_back_for_visitor > 24 hours (but even then it would be only needed probably when not a new visit should be created after midnight). So we could apply this more privacy friendly fingerprint pretty much always

looking at the code, yes it looks like the window_look_back_for_visitor is only used for fingerprint and defaults to 30min (defaults to the visit duration setting)

core/Tracker/Settings.php

mattab

Awesome to see this pr. this will improve privacy and limiting the fingerprint so it's clear that people can use without consent for cookie-less analytics.

PS: didn't review the code

mattab · 2020-04-30T23:09:55Z

core/Updates/3.13.6-b1.php

+    {
+        $config = Config::getInstance();
+        $tracker = $config->Tracker;
+        $tracker['enable_fingerprinting_limited_when_cookie_disabled'] = 0;


could we do this only when window_look_back_for_visitor > say 43200? (if window_look_back_for_visitor is not custom, then no impact to users afaik)

as mentioned we're creating new visits after midnight anyway... so this should probably not be important...
unless create_new_visit_after_midnight = 0. Right now it looks like we can safely always apply the shorter fingerprint.

tsteur · 2020-05-05T03:20:36Z

core/Tracker/FingerprintSalt.php

+
+    public function getDateString(Date $date, $timezone)
+    {
+        $dateString = Date::factory($date->getTimestampUTC(), $timezone)->toString();


not 100% sure if this is correct? Had initially Date::factory()->setTimezone($timezone) but then noticed in tests it might not return correct values unless I'm expecting the wrong values. In ArchiveProcessor we're using for example setTimezone: https://github.com/matomo-org/matomo/blob/3.13.5/core/ArchiveProcessor/Parameters.php#L166 so would have maybe expected setTimezone is right but then in CronArchive we're also using it as a parameter https://github.com/matomo-org/matomo/blob/3.13.5/core/CronArchive.php#L1504 ... looking at the test results the parameter seems to give expected result

Guess that's something we should probably check again with #13829

tsteur · 2020-05-05T03:26:32Z

tests/PHPUnit/System/expected/test_CustomEvents__Live.getLastVisitsDetails_day.xml

@@ -794,8 +794,8 @@
 				<eventCategory>Music</eventCategory>
 				<eventAction>play50%</eventAction>
 				<bandwidth />
-				<timeSpent>60</timeSpent>
-				<timeSpentPretty>1 min 0s</timeSpentPretty>
+				<timeSpent>0</timeSpent>


Couldn't figure out where these changes are from. The tests seem to pass in 3.x-dev.

Not sure how this would change in this PR since in the tests itself the fingerprintSalt is basically not set and nothing changes.

Looking at the tests it seems the changes to the test files can be reverted.

tsteur · 2020-05-05T03:27:06Z

Should be ready for a review. If I see things right, nothing changes for users in the end but hard to say.

sgiehl · 2020-05-05T14:59:37Z

Haven't looked deeper into the code yet. But wouldn't changing the config id will have the effect, that every visitor will be a new visitor after the update? 🤔

tsteur · 2020-05-05T20:28:07Z

@sgiehl only when cookies are disabled. Otherwise not. The fingerprint as @mattab mentioned was always only used to look 30 minutes back. So it would only affect it when cookies are disabled, and a visitor was active in the last 30 minutes. Then one new visit would be created which should be fine.

tsteur · 2020-05-18T21:37:37Z

@sgiehl did you maybe have another look?

sgiehl

Did some short local testing and it seems to work as expected I guess.

tests/PHPUnit/Integration/Tracker/FingerprintSaltTest.php

sgiehl · 2020-05-20T11:31:50Z

core/Config/IniFileChain.php

+            }
+            return $result;
+        } else {
+            return preg_replace('/[^a-zA-Z0-9_-]/', '', $value);


That actually causes problems doing something like this:
./console config:set --section="General" --key="login_whitelist_ip[]" --value="127.0.0.1"
as it will result in login_whitelist_ip = "127.0.0.1" in the config file, causing all previously set ips in the array to be removed...
Maybe it would be better to throw an exception if unexpected chars occur instead of silently removing them?

That wouldn't solve this problem, because [...] is valid in this case, but ]asldkjf, eg, is not. Throwing an exception isn't an issue for me though so if that is more desired than removing the chars then that's fine too.

sgiehl · 2020-05-20T12:18:47Z

tests/PHPUnit/System/expected/test_CustomEvents__Live.getLastVisitsDetails_day.xml

@@ -794,8 +794,8 @@
 				<eventCategory>Music</eventCategory>
 				<eventAction>play50%</eventAction>
 				<bandwidth />
-				<timeSpent>60</timeSpent>
-				<timeSpentPretty>1 min 0s</timeSpentPretty>
+				<timeSpent>0</timeSpent>


Looking at the tests it seems the changes to the test files can be reverted.

plugins/SitesManager/SitesManager.php

sgiehl · 2020-05-20T12:23:16Z

core/Tracker/FingerprintSalt.php

+
+    public function getDateString(Date $date, $timezone)
+    {
+        $dateString = Date::factory($date->getTimestampUTC(), $timezone)->toString();


Guess that's something we should probably check again with #13829

Co-authored-by: Stefan Giehl <stefan@matomo.org>

sgiehl · 2020-05-21T19:34:21Z

@tsteur I've reverted the test file changes and the tests seem to pass again. From my point a view the changes look fine now. Maybe you can also have a look at the changes from @diosmosis. Otherwise feel free to merge

When cookies are disabled, also make sure the fingerprint is limited

0c2fc74

tsteur added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 29, 2020

tsteur added this to the 3.13.6 milestone Apr 29, 2020

mattab reviewed Apr 30, 2020

View reviewed changes

core/Tracker/Settings.php Outdated Show resolved Hide resolved

mattab reviewed Apr 30, 2020

View reviewed changes

tsteur added 2 commits May 5, 2020 11:01

simplify

55e4519

add some tests

e1de8c7

tsteur changed the title ~~When cookies are disabled, also make sure the fingerprint is limited~~ Limit the fingerprint May 4, 2020

tsteur added 4 commits May 5, 2020 11:56

try to use same fingerprint hash in tests

8a6ef01

fix some tests

8a845e1

fix some failing tests

d209247

no fingerprint in tests so tests still succeed

078d354

tsteur commented May 5, 2020

View reviewed changes

sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 5, 2020

diosmosis force-pushed the cookiefingerprint branch from 32e0cb2 to d265bf4 Compare May 13, 2020 01:00

update tests

6968f2d

diosmosis force-pushed the cookiefingerprint branch from d265bf4 to 6968f2d Compare May 14, 2020 02:31

sgiehl requested changes May 20, 2020

View reviewed changes

tsteur and others added 4 commits May 21, 2020 08:47

Update tests/PHPUnit/Integration/Tracker/FingerprintSaltTest.php

a5ab2ff

Co-authored-by: Stefan Giehl <stefan@matomo.org>

Update plugins/SitesManager/SitesManager.php

801257f

Co-authored-by: Stefan Giehl <stefan@matomo.org>

apply pr fix

f338e0b

revert test file changes

f83000a

sgiehl approved these changes May 21, 2020

View reviewed changes

tsteur merged commit 3aa66a2 into 3.x-dev May 21, 2020

tsteur deleted the cookiefingerprint branch May 21, 2020 22:05

tsteur mentioned this pull request May 26, 2020

Limit visitor fingerprint by default for even better privacy #13655

Closed

mattab mentioned this pull request Sep 1, 2020

Explain how the new fingerprint is limited and why consent is not needed (update the fingerprint FAQ) #16361

Closed

jonasgrilleres pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 22, 2020

Limit the fingerprint (matomo-org#15886)

642197e

jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020

Limit the fingerprint (matomo-org#15886)

09539ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit the fingerprint #15886

Limit the fingerprint #15886

tsteur commented Apr 29, 2020 •

edited

mattab Apr 30, 2020 •

edited

tsteur May 1, 2020

mattab May 4, 2020

mattab left a comment •

edited

mattab Apr 30, 2020

tsteur May 4, 2020

tsteur May 5, 2020

sgiehl May 20, 2020

tsteur May 5, 2020

sgiehl May 20, 2020

tsteur commented May 5, 2020

sgiehl commented May 5, 2020

tsteur commented May 5, 2020

tsteur commented May 18, 2020

sgiehl left a comment

sgiehl May 20, 2020

diosmosis May 20, 2020

sgiehl May 20, 2020

sgiehl May 20, 2020

sgiehl commented May 21, 2020

Limit the fingerprint #15886

Limit the fingerprint #15886

Conversation

tsteur commented Apr 29, 2020 • edited

mattab Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented May 5, 2020

sgiehl commented May 5, 2020

tsteur commented May 5, 2020

tsteur commented May 18, 2020

sgiehl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgiehl commented May 21, 2020

tsteur commented Apr 29, 2020 •

edited

mattab Apr 30, 2020 •

edited

mattab left a comment •

edited