feat(web-analytics): remove cookieless_server_hash_mode team setting#51311
feat(web-analytics): remove cookieless_server_hash_mode team setting#51311lricoy wants to merge 9 commits into
Conversation
3832369 to
be01514
Compare
|
Size Change: -6.91 kB (-0.01%) Total Size: 81 MB 📦 View Changed
ℹ️ View Unchanged
|
Visual regression: Storybook UI snapshots updatedChanges: 3 snapshots (3 modified, 0 added, 0 deleted) What this means:
Next steps:
|
7ad6047 to
efe2abb
Compare
Visual regression: Storybook UI snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
f23bd72 to
7c04ab7
Compare
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
9334952 to
a5df90f
Compare
Storybook visual regression tests failedIf your changes intentionally update UI snapshots: add the If you didn't change any UI: this is likely a flaky snapshot — wait for a fix to land on master. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
d0194a7 to
2edf3dc
Compare
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
2edf3dc to
26a676c
Compare
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
|
🤖 Brought the branch up to master (was ~1k commits behind) and tightened a few loose ends:
Prod stateless usage check — the PR description said no team uses stateless mode; reality is a small number do. Behavior under this PR is to auto-migrate them to stateful, which matches what we want. Per-region check (Postgres
CI is rerunning against fresh master. Asking for bot reviews — would love to merge as soon as they're happy. |
|
Marking it ready so I can check the bot reviews first |
|
Reviews (1): Last reviewed commit: "Merge branch 'master' into lricoy/remove..." | Re-trigger Greptile |
| human_friendly_comparison_periods = field_access_control( | ||
| models.BooleanField(default=False, null=True, blank=True), "project", "admin" | ||
| ) | ||
| # DEPRECATED: This field is no longer read. All cookieless events are processed as stateful. |
There was a problem hiding this comment.
Just to be clear, I plan to remove it in a follow-up PR, just focusing on making the functionality sound first and then do the clean up
ba06366 to
cfcfe73
Compare
There was a problem hiding this comment.
This is a cross-team removal of a cookieless server hash mode setting touching feature-flags and ingestion teams, and the size gate tripped (509L/17F). The change is low-risk cleanup, but cross-team behavioral changes to the Rust cookieless manager and Node.js ingestion path need sign-off from @PostHog/team-feature-flags and @PostHog/team-ingestion, not just web-analytics.
18 updated Run: 6e789da3-5617-4c20-bd69-778db24d0e0e Co-authored-by: lricoy <2034367+lricoy@users.noreply.github.com>
cfcfe73 to
20a9f58
Compare
| results[i] = drop('cookieless_team_disabled') | ||
| continue | ||
| } | ||
| const timestamp = event.timestamp ?? event.sent_at ?? event.now |
There was a problem hiding this comment.
Medium: Project cookieless opt-in bypass
An unauthenticated sender with a public project token can set $cookieless_mode and have the event processed as cookieless even when cookieless mode is not enabled for the project. That path strips $ip, $raw_user_agent, and event.ip, so the sender can bypass the project-level admin choice and avoid IP/user-agent based enrichment or bot detection; keep a server-side project allowlist/gate here, or only accept the marker from a trusted SDK-controlled path.
There was a problem hiding this comment.
This bot's comment seems relevant btw
PR overviewThis pull request removes the There is one open security issue: clients can currently mark events as cookieless even when the project has not opted into cookieless mode. This lets an unauthenticated sender with a public project token bypass the project’s configured ingestion behavior and avoid IP/user-agent based enrichment or bot-detection paths. No issues have been addressed yet, so the PR still needs a server-side project gate or trusted-source check before this behavior is safe. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
Problem
Users need to enable cookieless tracking in two places: the SDK config AND the project settings. This is confusing and causes support tickets when users enable it in the SDK but forget the project setting.
Additionally, the stateless mode is not used by any projects and adds unnecessary complexity.
Changes
cookieless_server_hash_modefrom exposed fieldsHow did you test this code?
Publish to changelog?
Yes