Repository navigation
fix(browser): sanitize persisted landing URLs - #5550
Draft
dustinbyrne wants to merge 1 commit into
Draft
dustinbyrne wants to merge 1 commit into
dustinbyrne wants to merge 1 commit into
Conversation
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 20b5640: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| { | ||
| [INITIAL_PERSON_INFO]: this._storesIdentityInCookie | ||
| ? { | ||
| ...personInfo, |
Contributor
There was a problem hiding this comment.
P2 · Bound campaign overrides before persisting them in cookies
With url_sanitization: { query: false }, an utm_source containing 800 backslashes produces a cookie exceeding 4 KB. Spreading personInfo retains c without the encoded-size protection applied to u, so browsers reject the identity cookie and cookie persistence loses the anonymous ID on reload. Apply an encoded-size budget to campaign overrides too.
Contributor
Coherence0 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 0 problems Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
Contributor
Contributor
|
Size Change: +8.58 kB (+0.03%) Total Size: 25.3 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Node SDK acceptance v2Commit: 20b5640 Capture v0✅ 83 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1✅ 148 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Stacked on #5450. Follow-up for #5362: initial/session landing URLs can retain and transmit sensitive path/query values even when other automatically collected URLs are sanitized. Campaign attribution is derived from these stored URLs, so filtering them must preserve attribution separately.
Changes
url_sanitizationoption.This adds no further public configuration option. Behavior is unchanged when
url_sanitizationis unset. Configuration affects future writes; previously persisted values are retained. Campaign properties remain available when their URL parameters are removed, subject to existing personal-data masking.Validation
Release info
Includes a patch changeset for the affected packages. All commits are signed.
Agent context
Autonomy: Human-driven (agent-assisted).
Implemented with Pi, followed by fresh read-only review and a behavior-preserving simplify pass. Requires human review.