From 69f14905988aa06147452dbfe6d020d328063521 Mon Sep 17 00:00:00 2001 From: sgiehl Date: Thu, 2 Jul 2026 13:42:05 +0200 Subject: [PATCH 1/3] Add optional cookie allowlist for forwarded tracking requests Restricts the Cookie header sent to Matomo to configured names/prefixes instead of forwarding all site cookies. Opt-in via $COOKIE_ALLOWLIST, unset preserves current behavior. Fixes #99. --- .github/config.php | 9 +++ README.md | 17 +++++ config.php.example | 23 ++++++ proxy.php | 53 +++++++++++++- tests/ProxyTest.php | 159 ++++++++++++++++++++++++++++++++++++++++ tests/server/matomo.php | 2 +- 6 files changed, 261 insertions(+), 2 deletions(-) diff --git a/.github/config.php b/.github/config.php index f337cc3..1490510 100644 --- a/.github/config.php +++ b/.github/config.php @@ -13,3 +13,12 @@ if (strpos($MATOMO_URL, '/tests/server/') !== false && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { $http_ip_forward_header = $_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER']; } + +// Test-only: lets the suite exercise cookie-allowlist filtering via the X-Test-Cookie-Allowlist +// request header (comma-separated allowlist entries; empty value means an explicit empty +// allowlist). Gated on the local test-server URL so a stray copy to production is inert. +if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { + $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'] === '' + ? array() + : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); +} diff --git a/README.md b/README.md index f4f3b5b..c185ac3 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,14 @@ Because the proxy sits between your visitors and Matomo, it has to tell Matomo t > ⚠️ **Breaking change:** previously `$http_ip_forward_header` was sent *in addition* to `cip`+`token_auth`; the proxy now treats it as the *sole* IP mechanism and injects nothing else. If you already set it, make sure Matomo's trusted-proxy configuration above is in place — otherwise leave it empty to keep using `cip`. +### Cookie forwarding + +By default, the proxy forwards the visitor's entire `Cookie` header to Matomo unchanged. If your site also sets other cookies (session, consent-management, A/B testing, etc.) alongside Matomo's, those are forwarded too. + +To restrict this, set `$COOKIE_ALLOWLIST` in `config.php` to an array of cookie names. Each entry is matched against the incoming cookie names either as an **exact name** (e.g. `mtm_consent` matches only a cookie named exactly `mtm_consent`) or, if the entry ends with `*`, as a **prefix** (e.g. `_pk_id*` matches both `_pk_id` and `_pk_id.1.1fff`). Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, since the JavaScript tracker appends a per-site/per-domain suffix to their base name. Cookies whose name matches no entry are stripped from the `Cookie` header before the request reaches Matomo. Matching is case-sensitive. + +> Note: leaving `$COOKIE_ALLOWLIST` unset keeps today's behavior of forwarding all cookies unchanged. Setting it — even to an empty array — switches the proxy into allowlist mode; an empty array forwards no cookies at all. + ### Auth-protected tracking parameters Some tracking parameters (`cip`, `cdt`, `cdo`, `country`, `region`, `city`, `lat`, `long`) are only honored by Matomo for an authenticated request. The proxy never lends its `$TOKEN_AUTH` to a request — or to an individual entry of a bulk request — that carries one of these override parameters or its own `token_auth`: @@ -158,6 +166,15 @@ $timeout = 5; if (strpos($MATOMO_URL, '/tests/server/') !== false && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { $http_ip_forward_header = $_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER']; } + +// Test-only: lets the suite exercise cookie-allowlist filtering via the X-Test-Cookie-Allowlist +// request header (comma-separated allowlist entries; empty value means an explicit empty +// allowlist). Gated on the local test-server URL so a stray copy to production is inert. +if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { + $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'] === '' + ? array() + : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); +} ``` The tests need a webserver to be pointed to the root of this repository. The simplest way is to just use Vagrant: diff --git a/config.php.example b/config.php.example index dab8fda..9472a3e 100644 --- a/config.php.example +++ b/config.php.example @@ -49,3 +49,26 @@ $user_agent = ''; // for nginx see https://www.nginx.com/resources/wiki/start/topics/examples/forwarded/ // $http_ip_forward_header = ''; + +// By default, the proxy forwards the visitor's entire Cookie header to Matomo unchanged. If your +// site sets other cookies (session cookies, consent-management cookies, A/B testing tools, etc.) +// alongside Matomo's, those get forwarded too, which is broader than necessary from a privacy +// standpoint. +// +// Set $COOKIE_ALLOWLIST to restrict what's forwarded to only the cookies Matomo needs. Each entry +// is matched against the incoming cookie names either as: +// - an exact name, e.g. 'mtm_consent' matches only a cookie named exactly 'mtm_consent' +// - a prefix, by ending the entry with '*', e.g. '_pk_id*' matches '_pk_id' AND '_pk_id.1.1fff' +// Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, +// since the JavaScript tracker appends a per-site/per-domain suffix to their base name. +// Matching is case-sensitive. +// +// Typical Matomo cookie names/prefixes: '_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', +// 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'. Check your Matomo's tracker configuration +// (cookie name prefix, consent settings) to confirm which of these apply to your setup. +// +// IMPORTANT: leaving this unset (the default) forwards all cookies unchanged, for backward +// compatibility. Setting it to an empty array ($COOKIE_ALLOWLIST = array();) forwards NO cookies +// at all. +// +// $COOKIE_ALLOWLIST = array('_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'); diff --git a/proxy.php b/proxy.php index 9ffbe15..b84ee58 100644 --- a/proxy.php +++ b/proxy.php @@ -278,12 +278,57 @@ function handleHeaderLine($curl, $headerLine) return $originalByteCount; } +function cookieNameIsAllowed($name, array $allowlist) +{ + foreach ($allowlist as $entry) { + if ($entry === '' || $entry === '*') { + // Skip no-op / accidental match-everything entries so a typo can't silently allow + // every cookie through. + continue; + } + + if (substr($entry, -1) === '*') { + $prefix = substr($entry, 0, -1); + if (strncmp($name, $prefix, strlen($prefix)) === 0) { + return true; + } + } elseif ($name === $entry) { + return true; + } + } + + return false; +} + +function filterCookieHeader($cookieHeaderValue, array $allowlist) +{ + $keptPairs = array(); + + foreach (explode(';', $cookieHeaderValue) as $segment) { + $segment = trim($segment); + if ($segment === '') { + continue; + } + + $parts = explode('=', $segment, 2); + $name = $parts[0]; + $value = isset($parts[1]) ? $parts[1] : ''; + + if (cookieNameIsAllowed($name, $allowlist)) { + $keptPairs[] = $name . '=' . $value; + } + } + + return implode('; ', $keptPairs); +} + function getHttpContentAndStatus($url, $timeout, $user_agent, $postBody = '') { global $httpResponseHeaders; global $DEBUG_PROXY; global $NO_VERIFY_SSL; global $http_ip_forward_header; + global $COOKIE_ALLOWLIST; $useFopen = @ini_get('allow_url_fopen') == '1'; @@ -300,7 +345,13 @@ function getHttpContentAndStatus($url, $timeout, $user_agent, $postBody = '') } if (isset($_SERVER['HTTP_COOKIE'])) { - $header[] = "Cookie: " . $_SERVER['HTTP_COOKIE']; + $cookieHeaderValue = $_SERVER['HTTP_COOKIE']; + if (isset($COOKIE_ALLOWLIST) && is_array($COOKIE_ALLOWLIST)) { + $cookieHeaderValue = filterCookieHeader($cookieHeaderValue, $COOKIE_ALLOWLIST); + } + if ($cookieHeaderValue !== '') { + $header[] = "Cookie: " . $cookieHeaderValue; + } } $stream_options = array( diff --git a/tests/ProxyTest.php b/tests/ProxyTest.php index 88b7713..199ccf7 100644 --- a/tests/ProxyTest.php +++ b/tests/ProxyTest.php @@ -893,6 +893,165 @@ public function test_proxy_works_with_ipv6_in_header() $this->assertEquals($expected, $responseBody); } + public function test_cookie_header_forwarded_unchanged_by_default() + { + $response = $this->send('foo=bar', null, null, ['Cookie' => 'a=1; _pk_id.1.abc=qux']); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => 'a=1; _pk_id.1.abc=qux', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_allowlist_filters_to_prefix_match() + { + $headers = [ + 'Cookie' => 'a=1; _pk_id.1.abc=qux; other=2', + 'X-Test-Cookie-Allowlist' => '_pk_id*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => '_pk_id.1.abc=qux', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_allowlist_exact_entry_does_not_match_longer_name() + { + $headers = [ + 'Cookie' => 'mtm_consent=1; mtm_consent_removed=1', + 'X-Test-Cookie-Allowlist' => 'mtm_consent', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => 'mtm_consent=1', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_empty_cookie_allowlist_drops_all_cookies() + { + $headers = [ + 'Cookie' => 'a=1; _pk_id.1.abc=qux', + 'X-Test-Cookie-Allowlist' => '', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_allowlist_is_case_sensitive() + { + $headers = [ + 'Cookie' => '_PK_ID.1.abc=xyz', + 'X-Test-Cookie-Allowlist' => '_pk_id*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_value_with_equals_sign_is_preserved() + { + $headers = [ + 'Cookie' => '_pk_id.1.abc=YWJjZA==', + 'X-Test-Cookie-Allowlist' => '_pk_id*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => '_pk_id.1.abc=YWJjZA==', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_no_cookie_header_sent_with_allowlist_configured() + { + $headers = ['X-Test-Cookie-Allowlist' => '_pk_id*']; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + private function send($query = null, DateTime $modifiedSince = null, $matomoUrl = null, $addHeaders = null, $path = null, $method = 'GET', $body = null, $forceIpV6 = false) { diff --git a/tests/server/matomo.php b/tests/server/matomo.php index 18918b1..853d186 100644 --- a/tests/server/matomo.php +++ b/tests/server/matomo.php @@ -23,7 +23,7 @@ } $headers = array(); -foreach (array('DNT', 'X_DO_NOT_TRACK', 'X_FORWARDED_FOR') as $headerName) { +foreach (array('DNT', 'X_DO_NOT_TRACK', 'X_FORWARDED_FOR', 'COOKIE') as $headerName) { if (isset($_SERVER['HTTP_' . $headerName])) { $headers[$headerName] = $_SERVER['HTTP_' . $headerName]; } From 66c0fb67e597b03a9ec564802fd52da8f69ac367 Mon Sep 17 00:00:00 2001 From: sgiehl Date: Thu, 2 Jul 2026 14:12:44 +0200 Subject: [PATCH 2/3] Address review feedback on cookie allowlist Fail closed instead of open when $COOKIE_ALLOWLIST is misconfigured (non-array), document and test the '*'/'' no-op guard, trim cookie name/value around '=', and expand docs on cookie_name_prefix, matomo_ignore, and data-quality risks. --- .github/config.php | 7 ++++ README.md | 17 +++++++- config.php.example | 16 +++++-- proxy.php | 24 ++++++++--- tests/ProxyTest.php | 100 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 153 insertions(+), 11 deletions(-) diff --git a/.github/config.php b/.github/config.php index 1490510..bd7907d 100644 --- a/.github/config.php +++ b/.github/config.php @@ -22,3 +22,10 @@ ? array() : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); } + +// Test-only: lets the suite exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST) +// via the X-Test-Cookie-Allowlist-Invalid request header. Gated on the local test-server URL so a +// stray copy to production is inert. +if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { + $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID']; +} diff --git a/README.md b/README.md index c185ac3..1b0c651 100644 --- a/README.md +++ b/README.md @@ -127,9 +127,15 @@ Because the proxy sits between your visitors and Matomo, it has to tell Matomo t By default, the proxy forwards the visitor's entire `Cookie` header to Matomo unchanged. If your site also sets other cookies (session, consent-management, A/B testing, etc.) alongside Matomo's, those are forwarded too. -To restrict this, set `$COOKIE_ALLOWLIST` in `config.php` to an array of cookie names. Each entry is matched against the incoming cookie names either as an **exact name** (e.g. `mtm_consent` matches only a cookie named exactly `mtm_consent`) or, if the entry ends with `*`, as a **prefix** (e.g. `_pk_id*` matches both `_pk_id` and `_pk_id.1.1fff`). Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, since the JavaScript tracker appends a per-site/per-domain suffix to their base name. Cookies whose name matches no entry are stripped from the `Cookie` header before the request reaches Matomo. Matching is case-sensitive. +To restrict this, set `$COOKIE_ALLOWLIST` in `config.php` to an array of cookie names. Each entry is matched against the incoming cookie names either as an **exact name** (e.g. `mtm_consent` matches only a cookie named exactly `mtm_consent`) or, if the entry ends with `*`, as a **prefix** (e.g. `_pk_id*` matches both `_pk_id` and `_pk_id.1.1fff`). Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, since the JavaScript tracker appends a per-site/per-domain suffix to their base name. Cookies whose name matches no entry are stripped from the `Cookie` header before the request reaches Matomo. Matching is case-sensitive, and a prefix match is a plain string prefix (not delimiter-aware) — so `_pk_id*` would also match an unrelated cookie that happens to start with those characters, though this is very unlikely given Matomo's actual naming. -> Note: leaving `$COOKIE_ALLOWLIST` unset keeps today's behavior of forwarding all cookies unchanged. Setting it — even to an empty array — switches the proxy into allowlist mode; an empty array forwards no cookies at all. +> Note: leaving `$COOKIE_ALLOWLIST` unset keeps today's behavior of forwarding all cookies unchanged. Setting it — even to an empty array — switches the proxy into allowlist mode; an empty array forwards no cookies at all. A bare `*` or empty-string entry inside the array is treated as a no-op (matches nothing), not "allow everything" — this is deliberate, so a stray `array('*')` can't silently defeat the allowlist. + +> ⚠️ **Check your Matomo tracker configuration before copying the example list.** If your site calls `setCookieNamePrefix()` in the JavaScript tracker, every one of your cookies uses that custom prefix instead of `_pk_*`, and the default entries won't match any of them. Also check which plugins you have enabled (e.g. HeatmapSessionRecording sets its own `_pk_hsr*`-style cookie) — third-party or custom tracking plugins may set additional cookies not covered by the example list below. +> +> Always keep `matomo_ignore` (or whatever cookie your Matomo's opt-out/consent setup relies on) in the allowlist. Dropping it silently re-enables tracking for visitors who opted out, with no visible error. +> +> A dropped tracking cookie doesn't cause an error — Matomo just stops recognizing returning visitors, which silently inflates visit/visitor counts. After enabling `$COOKIE_ALLOWLIST`, load a tracked page twice from the same browser and confirm Matomo's visitor log shows one visit, not two, before relying on this in production. ### Auth-protected tracking parameters @@ -175,6 +181,13 @@ if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TE ? array() : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); } + +// Test-only: lets the suite exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST) +// via the X-Test-Cookie-Allowlist-Invalid request header. Gated on the local test-server URL so a +// stray copy to production is inert. +if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { + $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID']; +} ``` The tests need a webserver to be pointed to the root of this repository. The simplest way is to just use Vagrant: diff --git a/config.php.example b/config.php.example index 9472a3e..641dd02 100644 --- a/config.php.example +++ b/config.php.example @@ -65,10 +65,20 @@ $http_ip_forward_header = ''; // // Typical Matomo cookie names/prefixes: '_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', // 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'. Check your Matomo's tracker configuration -// (cookie name prefix, consent settings) to confirm which of these apply to your setup. +// (custom cookie name prefix via setCookieNamePrefix(), enabled plugins, consent settings) to +// confirm which of these actually apply to your setup — a custom cookie prefix means NONE of the +// '_pk_*' entries below will match, and enabled tracking plugins may set additional cookies of +// their own that aren't in this list. // // IMPORTANT: leaving this unset (the default) forwards all cookies unchanged, for backward -// compatibility. Setting it to an empty array ($COOKIE_ALLOWLIST = array();) forwards NO cookies -// at all. +// compatibility. Setting it to an empty array ($COOKIE_ALLOWLIST = array();), or to a non-array +// value by mistake, forwards NO cookies at all (fails closed, not open). A bare '*' or empty-string +// entry inside the array is a no-op (matches nothing) rather than "allow everything", so a typo +// can't silently defeat the allowlist. +// +// Always keep 'matomo_ignore' (or your consent/opt-out cookie) in this list - dropping it silently +// re-enables tracking for visitors who opted out. A dropped tracking cookie in general doesn't +// error, it just stops Matomo from recognizing returning visitors, silently inflating visit counts. +// After configuring this, load a tracked page twice and confirm Matomo logs one visit, not two. // // $COOKIE_ALLOWLIST = array('_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'); diff --git a/proxy.php b/proxy.php index b84ee58..95df056 100644 --- a/proxy.php +++ b/proxy.php @@ -278,15 +278,17 @@ function handleHeaderLine($curl, $headerLine) return $originalByteCount; } -function cookieNameIsAllowed($name, array $allowlist) +function cookieNameIsAllowed($name, $allowlist) { foreach ($allowlist as $entry) { if ($entry === '' || $entry === '*') { // Skip no-op / accidental match-everything entries so a typo can't silently allow - // every cookie through. + // every cookie through. Any future match-mode added here must keep this guard. continue; } + // A trailing '*' means prefix match (wildcard stripped before comparing); anything else + // is matched as an exact name only. if (substr($entry, -1) === '*') { $prefix = substr($entry, 0, -1); if (strncmp($name, $prefix, strlen($prefix)) === 0) { @@ -300,7 +302,7 @@ function cookieNameIsAllowed($name, array $allowlist) return false; } -function filterCookieHeader($cookieHeaderValue, array $allowlist) +function filterCookieHeader($cookieHeaderValue, $allowlist) { $keptPairs = array(); @@ -311,8 +313,8 @@ function filterCookieHeader($cookieHeaderValue, array $allowlist) } $parts = explode('=', $segment, 2); - $name = $parts[0]; - $value = isset($parts[1]) ? $parts[1] : ''; + $name = trim($parts[0]); + $value = isset($parts[1]) ? trim($parts[1]) : ''; if (cookieNameIsAllowed($name, $allowlist)) { $keptPairs[] = $name . '=' . $value; @@ -346,7 +348,17 @@ function getHttpContentAndStatus($url, $timeout, $user_agent, $postBody = '') if (isset($_SERVER['HTTP_COOKIE'])) { $cookieHeaderValue = $_SERVER['HTTP_COOKIE']; - if (isset($COOKIE_ALLOWLIST) && is_array($COOKIE_ALLOWLIST)) { + if (isset($COOKIE_ALLOWLIST)) { + if (!is_array($COOKIE_ALLOWLIST)) { + // Fail closed, not open: a misconfigured (non-array) allowlist must not silently + // revert to forwarding every cookie unfiltered, which would defeat the whole point + // of configuring one. + trigger_error( + '$COOKIE_ALLOWLIST must be an array; treating it as empty (no cookies forwarded) until fixed.', + E_USER_WARNING + ); + $COOKIE_ALLOWLIST = array(); + } $cookieHeaderValue = filterCookieHeader($cookieHeaderValue, $COOKIE_ALLOWLIST); } if ($cookieHeaderValue !== '') { diff --git a/tests/ProxyTest.php b/tests/ProxyTest.php index 199ccf7..a86245f 100644 --- a/tests/ProxyTest.php +++ b/tests/ProxyTest.php @@ -1052,6 +1052,106 @@ public function test_no_cookie_header_sent_with_allowlist_configured() $this->assertEquals($expected, $responseBody); } + public function test_cookie_allowlist_wildcard_entry_is_treated_as_noop_not_match_all() + { + $headers = [ + 'Cookie' => 'a=1; _pk_id.1.abc=qux', + 'X-Test-Cookie-Allowlist' => '*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + // A bare '*' entry must not be treated as "allow everything" - it's a no-op, so every + // cookie is dropped, same as an explicit empty allowlist. + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_allowlist_with_multiple_mixed_entries() + { + $headers = [ + 'Cookie' => '_pk_id.1.abc=qux; mtm_consent=1; mtm_consent_removed=1; other=2', + 'X-Test-Cookie-Allowlist' => '_pk_id*,mtm_consent', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + // '_pk_id*' (prefix) and 'mtm_consent' (exact) are both configured: the prefix-matching + // cookie and the exact-matching cookie are kept, 'mtm_consent_removed' and 'other' are not. + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => '_pk_id.1.abc=qux; mtm_consent=1', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_cookie_allowlist_with_whitespace_around_equals_still_matches() + { + $headers = [ + 'Cookie' => '_pk_id.1.abc = qux', + 'X-Test-Cookie-Allowlist' => '_pk_id*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => '_pk_id.1.abc=qux', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + + public function test_non_array_cookie_allowlist_fails_closed_and_drops_all_cookies() + { + $headers = [ + 'Cookie' => 'a=1; _pk_id.1.abc=qux', + 'X-Test-Cookie-Allowlist-Invalid' => '_pk_id*', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + // A misconfigured (non-array) allowlist must fail closed, not open: no cookies are + // forwarded rather than silently falling back to forwarding everything. + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + private function send($query = null, DateTime $modifiedSince = null, $matomoUrl = null, $addHeaders = null, $path = null, $method = 'GET', $body = null, $forceIpV6 = false) { From 1a63fec7cf8ade4e416c3f1ea806df3d214fff46 Mon Sep 17 00:00:00 2001 From: sgiehl Date: Thu, 2 Jul 2026 14:43:44 +0200 Subject: [PATCH 3/3] Fix round-2 review findings and tighten comments Use error_log() instead of trigger_error() so a misconfigured allowlist warning can never leak into the response body when display_errors is on. Trim allowlist entries for parity with the already-trimmed cookie name/value parsing, add a test for a stray empty entry among real ones, give matomo_ignore its own warning in the README, and de-duplicate/shorten the test-only config hooks and several overly long comments. --- .github/config.php | 21 ++++++++++----------- README.md | 33 ++++++++++++++++----------------- config.php.example | 39 ++++++++++++++------------------------- proxy.php | 18 +++++++----------- tests/ProxyTest.php | 36 ++++++++++++++++++++++++++++++------ 5 files changed, 77 insertions(+), 70 deletions(-) diff --git a/.github/config.php b/.github/config.php index bd7907d..44af390 100644 --- a/.github/config.php +++ b/.github/config.php @@ -8,24 +8,23 @@ $timeout = 5; -// Test-only: lets the suite exercise IP-forward-header handling via the X-Test-Ip-Forward-Header -// request header. Gated on the local test-server URL so a stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { +// Test-only request headers (below) let the suite exercise config variations per-request. +// Gated on the local test-server URL so a stray copy to production is inert. +$isTestServer = strpos($MATOMO_URL, '/tests/server/') !== false; + +// Exercise IP-forward-header handling. +if ($isTestServer && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { $http_ip_forward_header = $_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER']; } -// Test-only: lets the suite exercise cookie-allowlist filtering via the X-Test-Cookie-Allowlist -// request header (comma-separated allowlist entries; empty value means an explicit empty -// allowlist). Gated on the local test-server URL so a stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { +// Exercise cookie-allowlist filtering (comma-separated entries; empty value = explicit empty allowlist). +if ($isTestServer && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'] === '' ? array() : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); } -// Test-only: lets the suite exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST) -// via the X-Test-Cookie-Allowlist-Invalid request header. Gated on the local test-server URL so a -// stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { +// Exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST). +if ($isTestServer && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID']; } diff --git a/README.md b/README.md index 1b0c651..11969f1 100644 --- a/README.md +++ b/README.md @@ -127,15 +127,15 @@ Because the proxy sits between your visitors and Matomo, it has to tell Matomo t By default, the proxy forwards the visitor's entire `Cookie` header to Matomo unchanged. If your site also sets other cookies (session, consent-management, A/B testing, etc.) alongside Matomo's, those are forwarded too. -To restrict this, set `$COOKIE_ALLOWLIST` in `config.php` to an array of cookie names. Each entry is matched against the incoming cookie names either as an **exact name** (e.g. `mtm_consent` matches only a cookie named exactly `mtm_consent`) or, if the entry ends with `*`, as a **prefix** (e.g. `_pk_id*` matches both `_pk_id` and `_pk_id.1.1fff`). Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, since the JavaScript tracker appends a per-site/per-domain suffix to their base name. Cookies whose name matches no entry are stripped from the `Cookie` header before the request reaches Matomo. Matching is case-sensitive, and a prefix match is a plain string prefix (not delimiter-aware) — so `_pk_id*` would also match an unrelated cookie that happens to start with those characters, though this is very unlikely given Matomo's actual naming. +To restrict this, set `$COOKIE_ALLOWLIST` in `config.php` to an array of cookie names. Each entry is matched either as an **exact name** (e.g. `mtm_consent`) or, if it ends with `*`, as a **prefix** (e.g. `_pk_id*` matches both `_pk_id` and `_pk_id.1.1fff`). Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, since the JavaScript tracker appends a per-site/per-domain suffix to their base name. Cookies matching no entry are stripped before the request reaches Matomo. Matching is case-sensitive; a prefix match is a plain string prefix, not delimiter-aware (so `_pk_id*` could in theory also match an unrelated cookie sharing that prefix, though unlikely in practice). -> Note: leaving `$COOKIE_ALLOWLIST` unset keeps today's behavior of forwarding all cookies unchanged. Setting it — even to an empty array — switches the proxy into allowlist mode; an empty array forwards no cookies at all. A bare `*` or empty-string entry inside the array is treated as a no-op (matches nothing), not "allow everything" — this is deliberate, so a stray `array('*')` can't silently defeat the allowlist. +> Note: leaving `$COOKIE_ALLOWLIST` unset keeps today's behavior of forwarding all cookies unchanged. Setting it — even to an empty array — switches the proxy into allowlist mode; an empty array forwards no cookies at all. A bare `*` or empty-string entry is a no-op (matches nothing), not "allow everything", so a stray `array('*')` can't defeat the allowlist. -> ⚠️ **Check your Matomo tracker configuration before copying the example list.** If your site calls `setCookieNamePrefix()` in the JavaScript tracker, every one of your cookies uses that custom prefix instead of `_pk_*`, and the default entries won't match any of them. Also check which plugins you have enabled (e.g. HeatmapSessionRecording sets its own `_pk_hsr*`-style cookie) — third-party or custom tracking plugins may set additional cookies not covered by the example list below. -> -> Always keep `matomo_ignore` (or whatever cookie your Matomo's opt-out/consent setup relies on) in the allowlist. Dropping it silently re-enables tracking for visitors who opted out, with no visible error. +> ⚠️ **Always keep `matomo_ignore`** (or whatever cookie your Matomo's opt-out/consent setup relies on) in the allowlist. Dropping it silently re-enables tracking for visitors who opted out, with no visible error. + +> ⚠️ **Check your Matomo tracker config before copying the example list below.** A custom `setCookieNamePrefix()` replaces `_pk_*` with your own prefix, and enabled plugins (e.g. HeatmapSessionRecording) or third-party tracking code may set additional cookies not covered by the example. > -> A dropped tracking cookie doesn't cause an error — Matomo just stops recognizing returning visitors, which silently inflates visit/visitor counts. After enabling `$COOKIE_ALLOWLIST`, load a tracked page twice from the same browser and confirm Matomo's visitor log shows one visit, not two, before relying on this in production. +> A dropped tracking cookie doesn't error — Matomo just stops recognizing returning visitors, silently inflating visit counts. After configuring this, load a tracked page twice and confirm Matomo logs one visit, not two. ### Auth-protected tracking parameters @@ -167,25 +167,24 @@ $PROXY_URL = 'http://localhost/'; $TOKEN_AUTH = 'xyz'; $timeout = 5; -// Test-only: lets the suite exercise IP-forward-header handling via the X-Test-Ip-Forward-Header -// request header. Gated on the local test-server URL so a stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { +// Test-only request headers (below) let the suite exercise config variations per-request. +// Gated on the local test-server URL so a stray copy to production is inert. +$isTestServer = strpos($MATOMO_URL, '/tests/server/') !== false; + +// Exercise IP-forward-header handling. +if ($isTestServer && !empty($_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER'])) { $http_ip_forward_header = $_SERVER['HTTP_X_TEST_IP_FORWARD_HEADER']; } -// Test-only: lets the suite exercise cookie-allowlist filtering via the X-Test-Cookie-Allowlist -// request header (comma-separated allowlist entries; empty value means an explicit empty -// allowlist). Gated on the local test-server URL so a stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { +// Exercise cookie-allowlist filtering (comma-separated entries; empty value = explicit empty allowlist). +if ($isTestServer && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'])) { $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST'] === '' ? array() : explode(',', $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST']); } -// Test-only: lets the suite exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST) -// via the X-Test-Cookie-Allowlist-Invalid request header. Gated on the local test-server URL so a -// stray copy to production is inert. -if (strpos($MATOMO_URL, '/tests/server/') !== false && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { +// Exercise misconfiguration handling (a non-array $COOKIE_ALLOWLIST). +if ($isTestServer && isset($_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID'])) { $COOKIE_ALLOWLIST = $_SERVER['HTTP_X_TEST_COOKIE_ALLOWLIST_INVALID']; } ``` diff --git a/config.php.example b/config.php.example index 641dd02..2e21430 100644 --- a/config.php.example +++ b/config.php.example @@ -50,35 +50,24 @@ $user_agent = ''; // $http_ip_forward_header = ''; -// By default, the proxy forwards the visitor's entire Cookie header to Matomo unchanged. If your -// site sets other cookies (session cookies, consent-management cookies, A/B testing tools, etc.) -// alongside Matomo's, those get forwarded too, which is broader than necessary from a privacy -// standpoint. +// By default, the proxy forwards the visitor's entire Cookie header to Matomo unchanged, which +// also forwards unrelated site cookies (session, consent tools, A/B testing, etc.). // -// Set $COOKIE_ALLOWLIST to restrict what's forwarded to only the cookies Matomo needs. Each entry -// is matched against the incoming cookie names either as: -// - an exact name, e.g. 'mtm_consent' matches only a cookie named exactly 'mtm_consent' -// - a prefix, by ending the entry with '*', e.g. '_pk_id*' matches '_pk_id' AND '_pk_id.1.1fff' -// Prefix entries are needed for Matomo's default id/session/referrer/custom-variable cookies, -// since the JavaScript tracker appends a per-site/per-domain suffix to their base name. -// Matching is case-sensitive. +// Set $COOKIE_ALLOWLIST to restrict what's forwarded. Each entry matches either an exact name +// (e.g. 'mtm_consent') or, if it ends with '*', a prefix (e.g. '_pk_id*' matches '_pk_id.1.1fff'). +// Prefix entries are needed for Matomo's id/session/referrer/custom-variable cookies, since the +// JS tracker appends a per-site/per-domain suffix to their base name. Matching is case-sensitive. // // Typical Matomo cookie names/prefixes: '_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', -// 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'. Check your Matomo's tracker configuration -// (custom cookie name prefix via setCookieNamePrefix(), enabled plugins, consent settings) to -// confirm which of these actually apply to your setup — a custom cookie prefix means NONE of the -// '_pk_*' entries below will match, and enabled tracking plugins may set additional cookies of -// their own that aren't in this list. +// 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'. Check your tracker config first: a custom +// setCookieNamePrefix() replaces '_pk_*' entirely, and enabled plugins may set cookies of their own. // -// IMPORTANT: leaving this unset (the default) forwards all cookies unchanged, for backward -// compatibility. Setting it to an empty array ($COOKIE_ALLOWLIST = array();), or to a non-array -// value by mistake, forwards NO cookies at all (fails closed, not open). A bare '*' or empty-string -// entry inside the array is a no-op (matches nothing) rather than "allow everything", so a typo -// can't silently defeat the allowlist. +// Leaving this unset (the default) forwards all cookies, for backward compatibility. Setting it to +// an empty array, or to a non-array value by mistake, forwards NO cookies (fails closed). A bare +// '*' or empty-string entry is a no-op, not "allow everything", so a typo can't defeat the allowlist. // -// Always keep 'matomo_ignore' (or your consent/opt-out cookie) in this list - dropping it silently -// re-enables tracking for visitors who opted out. A dropped tracking cookie in general doesn't -// error, it just stops Matomo from recognizing returning visitors, silently inflating visit counts. -// After configuring this, load a tracked page twice and confirm Matomo logs one visit, not two. +// Always keep 'matomo_ignore' (or your opt-out cookie) in this list - dropping it silently +// re-enables tracking for opted-out visitors. A dropped tracking cookie otherwise just stops +// Matomo recognizing returning visitors, inflating visit counts; verify by loading a page twice. // // $COOKIE_ALLOWLIST = array('_pk_id*', '_pk_ses*', '_pk_ref*', '_pk_cvar*', '_pk_hsr*', 'mtm_consent', 'mtm_consent_removed', 'matomo_ignore'); diff --git a/proxy.php b/proxy.php index 95df056..ceba137 100644 --- a/proxy.php +++ b/proxy.php @@ -281,14 +281,13 @@ function handleHeaderLine($curl, $headerLine) function cookieNameIsAllowed($name, $allowlist) { foreach ($allowlist as $entry) { + $entry = trim($entry); if ($entry === '' || $entry === '*') { - // Skip no-op / accidental match-everything entries so a typo can't silently allow - // every cookie through. Any future match-mode added here must keep this guard. + // No-op: guards against a typo silently allowing every cookie through. continue; } - // A trailing '*' means prefix match (wildcard stripped before comparing); anything else - // is matched as an exact name only. + // Trailing '*' = prefix match; otherwise exact match only. if (substr($entry, -1) === '*') { $prefix = substr($entry, 0, -1); if (strncmp($name, $prefix, strlen($prefix)) === 0) { @@ -350,13 +349,10 @@ function getHttpContentAndStatus($url, $timeout, $user_agent, $postBody = '') $cookieHeaderValue = $_SERVER['HTTP_COOKIE']; if (isset($COOKIE_ALLOWLIST)) { if (!is_array($COOKIE_ALLOWLIST)) { - // Fail closed, not open: a misconfigured (non-array) allowlist must not silently - // revert to forwarding every cookie unfiltered, which would defeat the whole point - // of configuring one. - trigger_error( - '$COOKIE_ALLOWLIST must be an array; treating it as empty (no cookies forwarded) until fixed.', - E_USER_WARNING - ); + // Fail closed: a misconfigured allowlist must not silently forward everything + // unfiltered. error_log(), not trigger_error(), so this can never leak into the + // response body when display_errors is on. + error_log('$COOKIE_ALLOWLIST must be an array; treating it as empty (no cookies forwarded) until fixed.'); $COOKIE_ALLOWLIST = array(); } $cookieHeaderValue = filterCookieHeader($cookieHeaderValue, $COOKIE_ALLOWLIST); diff --git a/tests/ProxyTest.php b/tests/ProxyTest.php index a86245f..b66e91a 100644 --- a/tests/ProxyTest.php +++ b/tests/ProxyTest.php @@ -1062,8 +1062,7 @@ public function test_cookie_allowlist_wildcard_entry_is_treated_as_noop_not_matc $responseBody = $this->getBody($response); - // A bare '*' entry must not be treated as "allow everything" - it's a no-op, so every - // cookie is dropped, same as an explicit empty allowlist. + // '*' alone is a no-op, not "allow everything" - all cookies are dropped. $expected = << '127.0.0.1', @@ -1076,6 +1075,33 @@ public function test_cookie_allowlist_wildcard_entry_is_treated_as_noop_not_matc $this->assertEquals($expected, $responseBody); } + public function test_cookie_allowlist_empty_entry_among_real_entries_is_noop() + { + $headers = [ + 'Cookie' => 'mtm_consent=1; other=2', + // Leading comma produces an empty entry alongside a real one. + 'X-Test-Cookie-Allowlist' => ',mtm_consent', + ]; + $response = $this->send('foo=bar', null, null, $headers); + + $responseBody = $this->getBody($response); + + // The empty entry is a no-op; 'mtm_consent' still matches normally. + $expected = << '127.0.0.1', + 'token_auth' => '', + 'foo' => 'bar', +) +array ( + 'COOKIE' => 'mtm_consent=1', +) +RESPONSE; + + $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals($expected, $responseBody); + } + public function test_cookie_allowlist_with_multiple_mixed_entries() { $headers = [ @@ -1086,8 +1112,7 @@ public function test_cookie_allowlist_with_multiple_mixed_entries() $responseBody = $this->getBody($response); - // '_pk_id*' (prefix) and 'mtm_consent' (exact) are both configured: the prefix-matching - // cookie and the exact-matching cookie are kept, 'mtm_consent_removed' and 'other' are not. + // Prefix ('_pk_id*') and exact ('mtm_consent') entries combined; only matching cookies survive. $expected = << '127.0.0.1', @@ -1138,8 +1163,7 @@ public function test_non_array_cookie_allowlist_fails_closed_and_drops_all_cooki $responseBody = $this->getBody($response); - // A misconfigured (non-array) allowlist must fail closed, not open: no cookies are - // forwarded rather than silently falling back to forwarding everything. + // Non-array allowlist fails closed: no cookies forwarded, not silently forwarding everything. $expected = << '127.0.0.1',