Skip to content

Commit beacf08

Browse files
parhummparhumm
committed
security(gdpr): restore nonce verification for all consent endpoints
Revert the anonymous nonce bypass — consent is a state-changing operation. A cross-site POST without nonce verification could force-accept consent, enabling PII tracking without genuine user action (GDPR violation). On cached pages, anonymous consent REST calls return 403, but the JS cookie still records consent client-side, and tracking works via the /hit endpoint (PR #235). This is an acceptable trade-off for security. Changes: - ConsentChangeRestController: restore nonce verification for all users - GDPRBannerRestController: restore nonce required + verification - ConsentHandler: restore check_ajax_referer and wp_verify_nonce - wp-slimstat.js: skip X-WP-Nonce header when nonce is empty to avoid WordPress core rest_cookie_check_errors 403 before handler runs - Unit tests: replace inline pattern tests with actual handler invocations - E2E test 5: verify stale nonce causes 403 (correct security behavior) while client-side cookie is still set Refs #240, #241
1 parent 0fb1179 commit beacf08

File tree

8 files changed

+154
-106
lines changed

8 files changed

+154
-106
lines changed

‎src/Controllers/Rest/ConsentChangeRestController.php‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public function register_routes(): void
127127
'sanitize_callback' => 'sanitize_text_field',
128128
],
129129
'nonce' => [
130-
'required' => false,
130+
'required' => true,
131131
'type' => 'string',
132132
'sanitize_callback' => 'sanitize_text_field',
133133
],
@@ -146,19 +146,18 @@ public function handle_consent_change(\WP_REST_Request $request)
146146
{
147147
$this->currentRequest = $request;
148148

149-
// Only verify nonce for logged-in users. Anonymous users on cached pages
150-
// don't have a valid nonce (wp_rest_nonce is only generated for logged-in
151-
// users). This endpoint only modifies the caller's own consent state
152-
// (cookie-based), so there is no CSRF attack surface for anonymous users.
153-
if (get_current_user_id() > 0) {
154-
$nonce = $request->get_param('nonce');
155-
if (!wp_verify_nonce($nonce, 'wp_rest')) {
156-
return new \WP_Error(
157-
'rest_forbidden',
158-
__('Invalid security token.', 'wp-slimstat'),
159-
['status' => 403]
160-
);
161-
}
149+
// Verify nonce for all users — consent is a state-changing operation.
150+
// Without verification, a cross-site POST could force-accept consent,
151+
// enabling PII tracking without genuine user action (GDPR violation).
152+
// On cached pages with stale nonces, this returns 403 — the JS cookie
153+
// still records consent client-side, and tracking works via /hit (PR #235).
154+
$nonce = $request->get_param('nonce');
155+
if (!wp_verify_nonce($nonce, 'wp_rest')) {
156+
return new \WP_Error(
157+
'rest_forbidden',
158+
__('Invalid security token.', 'wp-slimstat'),
159+
['status' => 403]
160+
);
162161
}
163162

164163
$source = $request->get_param('source');

‎src/Controllers/Rest/GDPRBannerRestController.php‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public function register_routes(): void
4949
'sanitize_callback' => 'sanitize_text_field',
5050
],
5151
'nonce' => [
52-
'required' => false,
52+
'required' => true,
5353
'type' => 'string',
5454
'sanitize_callback' => 'sanitize_text_field',
5555
],
@@ -66,18 +66,19 @@ public function register_routes(): void
6666
*/
6767
public function handle_consent(\WP_REST_Request $request)
6868
{
69-
// Only verify nonce for logged-in users. Anonymous users on cached pages
70-
// don't have a valid nonce. This endpoint only sets the caller's own
71-
// consent cookie, so there is no CSRF attack surface for anonymous users.
72-
if (get_current_user_id() > 0) {
73-
$nonce = $request->get_param('nonce');
74-
if (!wp_verify_nonce($nonce, 'wp_rest')) {
75-
return new \WP_Error(
76-
'rest_forbidden',
77-
__('Invalid security token.', 'wp-slimstat'),
78-
['status' => 403]
79-
);
80-
}
69+
// Verify nonce for all users — consent is a state-changing operation.
70+
// A cross-site POST without nonce verification could force-accept consent,
71+
// enabling PII tracking without genuine user action (GDPR violation).
72+
// On cached pages, anonymous users may have a stale/empty nonce — the
73+
// request will fail with 403, but the JS cookie still records consent
74+
// client-side, and tracking works via the /hit endpoint (PR #235).
75+
$nonce = $request->get_param('nonce');
76+
if (!wp_verify_nonce($nonce, 'wp_rest')) {
77+
return new \WP_Error(
78+
'rest_forbidden',
79+
__('Invalid security token.', 'wp-slimstat'),
80+
['status' => 403]
81+
);
8182
}
8283

8384
// Check if SlimStat banner is enabled

‎src/Services/Privacy/ConsentHandler.php‎

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,7 @@ class ConsentHandler
3636
*/
3737
public static function handleConsentRevoked()
3838
{
39-
// Only verify nonce for logged-in users. Anonymous users on cached pages
40-
// don't have a valid nonce. This endpoint only deletes the caller's own
41-
// tracking cookie, so there is no CSRF attack surface for anonymous users.
42-
if (get_current_user_id() > 0) {
43-
check_ajax_referer('wp_rest', 'nonce');
44-
}
39+
check_ajax_referer('wp_rest', 'nonce');
4540

4641
if (function_exists('wp_cache_delete')) {
4742
wp_cache_delete('slimstat_consent_state', 'slimstat');
@@ -100,19 +95,14 @@ public static function handleBannerConsent(bool $return_json = true, array $data
10095
$pageview_id_raw = isset($_POST['pageview_id']) ? sanitize_text_field(wp_unslash($_POST['pageview_id'])) : '';
10196
}
10297

103-
// Only verify nonce for logged-in users. Anonymous users on cached pages
104-
// don't have a valid nonce. This endpoint only sets the caller's own
105-
// consent cookie, so there is no CSRF attack surface for anonymous users.
106-
if (get_current_user_id() > 0) {
107-
if (empty($nonce) || !wp_verify_nonce($nonce, 'wp_rest')) {
108-
if ($return_json) {
109-
wp_send_json_error([
110-
'message' => __('Invalid security token.', 'wp-slimstat'),
111-
]);
112-
return;
113-
}
114-
return false;
98+
if (empty($nonce) || !wp_verify_nonce($nonce, 'wp_rest')) {
99+
if ($return_json) {
100+
wp_send_json_error([
101+
'message' => __('Invalid security token.', 'wp-slimstat'),
102+
]);
103+
return;
115104
}
105+
return false;
116106
}
117107

118108
if (empty(\wp_slimstat::$settings['use_slimstat_banner']) ||

‎tests/e2e/gdpr-banner-consent-persistence.spec.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -451,11 +451,18 @@ test.describe('GDPR Banner Consent Persistence — #240 #241', () => {
451451
'"wp_rest_nonce":"stale_expired_nonce_12345"',
452452
);
453453

454-
// Step 3: Serve modified HTML via page.route()
454+
// Step 3: Serve modified HTML via page.route() and track consent-change responses
455455
const testCtx = await browser.newContext();
456456
await testCtx.addCookies(COOKIEYES_DISMISS_COOKIES);
457457
const testPage = await testCtx.newPage();
458458

459+
const consentResponses: { status: number; url: string }[] = [];
460+
testPage.on('response', (res) => {
461+
if (isConsentChangeRequest({ url: () => res.url(), method: () => 'POST' } as any)) {
462+
consentResponses.push({ status: res.status(), url: res.url() });
463+
}
464+
});
465+
459466
await testPage.route(`**/?e2e_marker=stale-nonce*`, (route) =>
460467
route.fulfill({
461468
status: 200,
@@ -479,16 +486,29 @@ test.describe('GDPR Banner Consent Persistence — #240 #241', () => {
479486
});
480487
await testPage.waitForTimeout(3000);
481488

482-
// Step 5: Consent cookie should be set (proves consent flow completed despite stale nonce)
489+
// Step 5: Client-side cookie MUST be set (JS handles consent regardless of server response)
483490
const cookies = await testCtx.cookies();
484491
const consentCookie = cookies.find(
485492
(c) => c.name === 'slimstat_gdpr_consent',
486493
);
487494
expect(
488495
consentCookie,
489-
'slimstat_gdpr_consent cookie should be set after accepting on stale-nonce cached page',
496+
'slimstat_gdpr_consent cookie should be set client-side even with stale nonce',
490497
).toBeTruthy();
491498

499+
// Step 6: Server-side consent-change POST should return 403 (nonce verified for all users).
500+
// This is the correct security behavior — stale nonce is rejected.
501+
// Consent is still recorded via the client-side cookie.
502+
if (consentResponses.length > 0) {
503+
const has403 = consentResponses.some((r) => r.status === 403);
504+
expect(
505+
has403,
506+
`Stale nonce should cause 403 on consent-change endpoint (got: ${JSON.stringify(consentResponses)})`,
507+
).toBe(true);
508+
}
509+
// Note: if no consent-change responses captured (evaluate-click limitation), the
510+
// cookie assertion above still validates the client-side consent flow works.
511+
492512
await testPage.close();
493513
await testCtx.close();
494514
});

‎tests/gdpr-consent-cookie-test.php‎

Lines changed: 76 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,28 @@ function is_ssl(): bool
2626
}
2727
}
2828

29+
// ─── Stub namespaced classes for ConsentHandler dependencies ─────
30+
namespace SlimStat\Providers {
31+
class IPHashProvider {
32+
public static function upgradeToPii(array $stat): array { return $stat; }
33+
}
34+
}
35+
36+
namespace SlimStat\Tracker {
37+
class Session {
38+
public static function deleteTrackingCookie(): void {}
39+
}
40+
class Utils {
41+
public static function getValueWithoutChecksum($v) { return $v; }
42+
}
43+
}
44+
45+
namespace SlimStat\Utils {
46+
class Consent {
47+
public static function normalizeConsent($c) { return is_array($c) ? $c : ['statistics' => $c]; }
48+
}
49+
}
50+
2951
// ─── Everything else in global namespace ─────────────────────────
3052
namespace {
3153

@@ -339,67 +361,75 @@ class wp_slimstat
339361
assert_true(strlen($html) > 0, 'getBannerHtml should return non-empty HTML when no consent');
340362

341363
// ═══════════════════════════════════════════════════════════════════════════
342-
// Section 2: Nonce Skip Pattern for Anonymous Users
364+
// Section 2: Actual Handler Nonce Verification
365+
// Tests call the real controller/handler methods to verify nonce is enforced.
343366
// ═══════════════════════════════════════════════════════════════════════════
344367

345-
// ─── Test 6: Nonce verification skipped when user_id = 0 (anonymous) ───
346-
347-
$_stub_user_id = 0;
348-
$_stub_nonce_valid = false; // nonce is invalid/stale
349-
$nonce = 'stale_nonce_123';
350-
351-
// Simulate the fixed pattern
352-
$should_reject = false;
353-
$user_id = get_current_user_id();
354-
if ($user_id > 0) {
355-
if (!wp_verify_nonce($nonce, 'wp_rest')) {
356-
$should_reject = true;
357-
}
358-
}
359-
assert_false($should_reject, 'Anonymous user with stale nonce should NOT be rejected');
368+
// Load the actual handlers under test
369+
require_once __DIR__ . '/../src/Interfaces/RestControllerInterface.php';
370+
require_once __DIR__ . '/../src/Controllers/Rest/GDPRBannerRestController.php';
371+
require_once __DIR__ . '/../src/Services/Privacy/ConsentHandler.php';
360372

361-
// ─── Test 7: Nonce verification enforced when user_id > 0 with bad nonce ──
373+
// ─── Test 6: GDPRBannerRestController rejects request with bad nonce ───
362374

363-
$_stub_user_id = 1;
364375
$_stub_nonce_valid = false;
365-
$nonce = 'bad_nonce';
376+
\wp_slimstat::$settings = ['use_slimstat_banner' => 'on'];
366377

367-
$should_reject = false;
368-
$user_id = get_current_user_id();
369-
if ($user_id > 0) {
370-
if (!wp_verify_nonce($nonce, 'wp_rest')) {
371-
$should_reject = true;
372-
}
373-
}
374-
assert_true($should_reject, 'Logged-in user with bad nonce should be rejected');
378+
$controller = new \SlimStat\Controllers\Rest\GDPRBannerRestController();
379+
$request = new \WP_REST_Request(['consent' => 'accepted', 'nonce' => 'bad_nonce_123']);
380+
$result = $controller->handle_consent($request);
375381

376-
// ─── Test 8: Nonce verification passes for logged-in user with valid nonce ──
382+
assert_true($result instanceof \WP_Error, 'handle_consent should return WP_Error when nonce is invalid');
383+
assert_same('rest_forbidden', $result->get_error_code(), 'handle_consent should return rest_forbidden error code');
384+
assert_same(403, $result->get_error_data()['status'], 'handle_consent should return 403 status');
385+
386+
// ─── Test 7: GDPRBannerRestController accepts request with valid nonce ───
377387

378-
$_stub_user_id = 1;
379388
$_stub_nonce_valid = true;
380-
$nonce = 'valid_nonce';
389+
$_setcookie_calls = [];
390+
\wp_slimstat::$settings = ['use_slimstat_banner' => 'on'];
381391

382-
$should_reject = false;
383-
$user_id = get_current_user_id();
384-
if ($user_id > 0) {
385-
if (!wp_verify_nonce($nonce, 'wp_rest')) {
386-
$should_reject = true;
387-
}
388-
}
389-
assert_false($should_reject, 'Logged-in user with valid nonce should NOT be rejected');
392+
$controller = new \SlimStat\Controllers\Rest\GDPRBannerRestController();
393+
$request = new \WP_REST_Request(['consent' => 'accepted', 'nonce' => 'valid_nonce']);
394+
$result = $controller->handle_consent($request);
390395

391-
// ─── Test 9: check_ajax_referer skipped for anonymous in handleConsentRevoked ──
396+
assert_true($result instanceof \WP_REST_Response, 'handle_consent should return WP_REST_Response on valid nonce');
397+
assert_same(200, $result->status, 'handle_consent should return 200 on success');
398+
assert_true($result->data['success'] ?? false, 'handle_consent response should indicate success');
399+
400+
// ─── Test 8: GDPRBannerRestController rejects empty nonce ───
392401

393-
$_stub_user_id = 0;
394402
$_stub_nonce_valid = false;
403+
\wp_slimstat::$settings = ['use_slimstat_banner' => 'on'];
404+
405+
$controller = new \SlimStat\Controllers\Rest\GDPRBannerRestController();
406+
$request = new \WP_REST_Request(['consent' => 'accepted', 'nonce' => '']);
407+
$result = $controller->handle_consent($request);
408+
409+
assert_true($result instanceof \WP_Error, 'handle_consent should return WP_Error when nonce is empty');
410+
assert_same('rest_forbidden', $result->get_error_code(), 'handle_consent should return rest_forbidden for empty nonce');
395411

396-
// Simulate the fixed pattern for handleConsentRevoked
397-
$was_checked = false;
398-
if (get_current_user_id() > 0) {
399-
// In real code: check_ajax_referer('wp_rest', 'nonce');
400-
$was_checked = true;
412+
// ─── Test 9: ConsentHandler::handleBannerConsent rejects empty nonce ───
413+
414+
$_stub_nonce_valid = false;
415+
\wp_slimstat::$settings = ['use_slimstat_banner' => 'on'];
416+
$_stub_json_error = null;
417+
418+
$caught_error = false;
419+
try {
420+
\SlimStat\Services\Privacy\ConsentHandler::handleBannerConsent(false, [
421+
'nonce' => '',
422+
'consent' => 'accepted',
423+
]);
424+
} catch (\Throwable $e) {
425+
// handleBannerConsent returns false for non-JSON mode with bad nonce
401426
}
402-
assert_false($was_checked, 'check_ajax_referer should be skipped for anonymous users');
427+
// In non-JSON mode, it returns false when nonce fails
428+
$nonce_result = \SlimStat\Services\Privacy\ConsentHandler::handleBannerConsent(false, [
429+
'nonce' => 'stale_nonce',
430+
'consent' => 'accepted',
431+
]);
432+
assert_false($nonce_result, 'handleBannerConsent should return false when nonce verification fails');
403433

404434
// ═══════════════════════════════════════════════════════════════════════════
405435

‎wp-slimstat.js‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -932,13 +932,19 @@ var SlimStat = (function () {
932932
payload.pageview_id = String(pageviewId);
933933
}
934934

935+
// Build headers — only include X-WP-Nonce when nonce is non-empty.
936+
// Anonymous users on cached pages have no valid nonce. Sending an
937+
// empty/stale X-WP-Nonce causes WordPress core (rest_cookie_check_errors)
938+
// to reject with 403 before the controller handler even runs.
939+
var headers = { "Content-Type": "application/json" };
940+
if (nonce) {
941+
headers["X-WP-Nonce"] = nonce;
942+
}
943+
935944
if (typeof window.fetch === "function") {
936945
fetch(endpoint, {
937946
method: "POST",
938-
headers: {
939-
"Content-Type": "application/json",
940-
"X-WP-Nonce": nonce,
941-
},
947+
headers: headers,
942948
credentials: "same-origin",
943949
body: JSON.stringify(payload),
944950
})
@@ -953,7 +959,9 @@ var SlimStat = (function () {
953959
var xhr = new XMLHttpRequest();
954960
xhr.open("POST", endpoint, true);
955961
xhr.setRequestHeader("Content-Type", "application/json");
956-
xhr.setRequestHeader("X-WP-Nonce", nonce);
962+
if (nonce) {
963+
xhr.setRequestHeader("X-WP-Nonce", nonce);
964+
}
957965
xhr.onload = function () {
958966
if (xhr.status >= 200 && xhr.status < 300) {
959967
try {

0 commit comments

Comments
 (0)