fix(browser): unregister $sesid on reset instead of writing null tuple#3505
Closed
posthog[bot] wants to merge 2 commits into
Closed
fix(browser): unregister $sesid on reset instead of writing null tuple#3505posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
resetSessionId calls _setSessionId(null, null, null), which was registering [$sesid: [null, null, null]] in persistence. With cookie persistence the props object is JSON.stringified and URL-encoded into the ph_<token>_posthog cookie, so the literal substring "[null,null,null]" ends up in subsequent request headers — and some WAFs match brackets+null as a SQL injection pattern, blocking real customer traffic. In the full bundle this window is short because autocapture / pageview events almost immediately overwrite the tuple via checkAndGetSessionAndWindowId. In the slim bundle (extension-bundles ship with no autocapture by default) nothing repopulates the cookie, so [null,null,null] persists indefinitely. When all three reset values are null, unregister the SESSION_ID key from persistence instead of registering a null tuple. _getSessionId already returns [0, null, 0] when the key is absent, so downstream behaviour is unchanged — the next checkAndGetSessionAndWindowId call still allocates a fresh session/window id. Also covers the forced-idle reset path (sessionid.ts:333), which goes through the same _setSessionId(null, null, null) call. Generated-By: PostHog Code Task-Id: b0f96161-eed5-4ac3-a562-aa0ca7a7250c
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Contributor
|
Size Change: +605 B (+0.01%) Total Size: 7 MB
ℹ️ View Unchanged
|
Generated-By: PostHog Code Task-Id: b0f96161-eed5-4ac3-a562-aa0ca7a7250c
Contributor
|
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, post a comment or remove the |
Contributor
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
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.
Summary
A YC customer (Thndr) reports that their WAF is flagging legitimate backend traffic as a SQL injection attack because posthog-js writes
"$sesid":[null,null,null]into theph_<token>_posthogcookie. After URL-decoding, the literal substring[null,null,null]is exposed to the firewall, which matches square brackets + the keywordnullas a SQLi pattern and blocks the request.Root cause
SessionIdManager.resetSessionId()calls_setSessionId(null, null, null), which thenregisters[SESSION_ID]: [null, null, null]intoPostHogPersistence. The cookie store JSON-stringifies the entire props object, so the null tuple lands verbatim in the cookie.resetSessionIdis invoked from:posthog.reset()(posthog-core.ts:2759)sessionid.ts:333)In the full bundle, autocapture / pageview events almost immediately call
checkAndGetSessionAndWindowId(false)and overwrite the tuple with fresh uuids — so the bad state is short-lived. In the slim bundle (entrypoints/module.slim*.es.ts, withextension-bundles.tsshipping no autocapture by default), nothing fires those events, so[null,null,null]persists in the cookie indefinitely.Fix
In
SessionIdManager._setSessionId, whensessionId,sessionActivityTimestamp, andsessionStartTimestampare all null (i.e. onresetSessionIdand the forced-idle reset path), callthis._persistence.unregister(SESSION_ID)instead of registering a[null, null, null]tuple._getSessionIdalready falls back to[0, null, 0]when the key is absent, so downstream behaviour is unchanged — the nextcheckAndGetSessionAndWindowIdcall still allocates fresh ids.Test plan
pnpm test:unit(39/39 sessionid tests, 3950 total — only failure is the unrelatedentrypoints/module.test.tswhich requiresdist/array.jsfrom a priorpnpm build)pnpm lintregister([null,null,null])assertions to expectunregister(SESSION_ID)and verify the key is removed frompersistence.propsserializes persistence without "[null,null,null]" after resetdirectly asserts thatJSON.stringify(persistence.props)does not contain the WAF-triggering substring afterresetSessionId()unregisterwas called and the key is gone frompersistence.propsCreated with PostHog Code