fix: strip $session_entry_* URL props (site admin URLs leaking to PostHog)#33
Conversation
|
Warning Review limit reached
More reviews will be available in 12 minutes and 22 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: wcpos/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe ChangesSession Entry Property Sanitization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/analytics-config.test.mjs`:
- Around line 19-20: The test loop in the for statement on line 19 currently
checks four legacy URL fields but is missing `$referring_domain`, which is also
stripped by the `before_send` function in the actual implementation. Add
`$referring_domain` to the array of keys being validated in the loop to ensure
the test covers all legacy fields that are sanitized by the analytics code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: wcpos/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57ffde85-c873-4686-b69a-ac85821acfb0
📒 Files selected for processing (3)
assets/js/welcome.jssrc/shared/analytics.tstests/analytics-config.test.mjs
Covers the full legacy URL-field list that before_send sanitizes, keeping the test aligned with the sanitization contract in analytics.ts.
… leaking) PostHog data check on prod found site admin URLs (e.g. https://shop.example/wp-admin/admin.php?page=woocommerce-pos) reaching PostHog via the newer posthog-js $session_entry_url/$session_entry_referrer/etc. family — before_send only stripped the older $current_url/$host/$pathname/$referrer/ $referring_domain. Strip the whole $session_entry_* family too (privacy / wp.org guideline 7). 326 events/day were carrying these.
Covers the full legacy URL-field list that before_send sanitizes, keeping the test aligned with the sanitization contract in analytics.ts.
e86dcba to
6c55817
Compare
Found while auditing the live PostHog install (you asked me to confirm useful data is being collected).
Leak: newer posthog-js records the entry URL/referrer/host on a
$session_entry_*family, and ourbefore_sendonly stripped the older$current_url/$host/$pathname/$referrer/$referring_domain. So site admin URLs were reaching PostHog — e.g.$session_entry_url = https://inventory.sopec.net/wp-admin/admin.php?page=woocommerce-pos. 326 events/day carried these.Fix: strip the whole
$session_entry_*family inbefore_send(no useful campaign data on an admin-screen entry). Test added intests/analytics-config.test.mjs.Note: a separate leak exists in the plugin's own PostHog tracking (
upgrade_cta_viewed, ~188 events/day with$current_url) which doesn't use this before_send — I'll file that against woocommerce-pos separately.Leaving unmerged for review.
🤖 Generated with Claude Code
Summary by CodeRabbit