Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rotate session id proactively #1512

Merged
merged 8 commits into from
Dec 16, 2024
Merged

fix: rotate session id proactively #1512

merged 8 commits into from
Dec 16, 2024

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Nov 5, 2024

We reviewed the session idle timeout code and some of the naming was off, so this changes the names

We also periodically get reports that some events sneak through after many tens of hours of idle activity before the session id rotates. This is very surprising from reading the code but is clearly happening, let's make it (even more) impossible by:

  • every time we see activity
  • set a timeout to reset the session id after slightly more than the idle timeout
  • if we see activity again we clear and recreate that timeout
  • otherwise we reset the session id and any return from idle will create a new session id and not be able to leak to the old one

What I'm worried about here after talking it through with @robbie-c in person is that the code makes sense but we don't know it will fix the problem since we don't know what the problem is 🤔

Copy link

vercel bot commented Nov 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Dec 16, 2024 11:31am

@pauldambra pauldambra requested review from ghoti143, robbie-c and a team November 5, 2024 09:07
Comment on lines +170 to 173
if (isArray(sessionIdInfo) && sessionIdInfo.length === 2) {
// Storage does not yet have a session start time. Add the last activity timestamp as the start time
sessionId.push(sessionId[0])
sessionIdInfo.push(sessionIdInfo[0])
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB we can probably remove this upgrade code now but I won't do it here, a lot of time of passed since we added this

Copy link

github-actions bot commented Nov 5, 2024

Size Change: +2 kB (+0.06%)

Total Size: 3.21 MB

Filename Size Change
dist/array.full.es5.js 260 kB +234 B (+0.09%)
dist/array.full.js 364 kB +196 B (+0.05%)
dist/array.full.no-external.js 363 kB +196 B (+0.05%)
dist/array.js 178 kB +196 B (+0.11%)
dist/array.no-external.js 176 kB +196 B (+0.11%)
dist/main.js 179 kB +196 B (+0.11%)
dist/module.full.js 364 kB +196 B (+0.05%)
dist/module.full.no-external.js 363 kB +196 B (+0.05%)
dist/module.js 178 kB +196 B (+0.11%)
dist/module.no-external.js 177 kB +196 B (+0.11%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 206 kB
dist/customizations.full.js 12.6 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.48 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 57.6 kB
dist/surveys.js 63.3 kB
dist/tracing-headers.js 1.75 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

@posthog-bot
Copy link
Collaborator

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 stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

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 stale label – otherwise this will be closed in another week.

@marandaneto
Copy link
Member

What I'm worried about here after talking it through with @robbie-c in person is that the code makes sense but we don't know it will fix the problem since we don't know what the problem is 🤔

I agree but, we know the problem exists, we cannot reproduce it, so every small interaction may lead to fixing it.
We can always roll back if it worsens it.

@posthog-bot posthog-bot removed the stale label Nov 22, 2024
@posthog-bot
Copy link
Collaborator

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 stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@pauldambra pauldambra reopened this Dec 16, 2024
@pauldambra pauldambra removed the stale label Dec 16, 2024
@pauldambra pauldambra added the bump minor Bump minor version when this PR gets merged label Dec 16, 2024
@pauldambra pauldambra merged commit 1c41cae into main Dec 16, 2024
16 checks passed
@pauldambra pauldambra deleted the fix/force-session-idle branch December 16, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants