-
Notifications
You must be signed in to change notification settings - Fork 163
♻️ encode cookie options in cookie value #3905
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
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 793177f | Docs | Was this helpful? Give us feedback! |
99af81f to
65d47fa
Compare
e5ae822 to
7be32d5
Compare
7be32d5 to
eb9497a
Compare
packages/core/src/domain/session/storeStrategies/sessionInCookie.ts
Outdated
Show resolved
Hide resolved
7ad5a8a to
80a391e
Compare
| let byte = 0 | ||
| byte |= SESSION_COOKIE_VERSION << 6 // Store version in upper 2 bits | ||
| byte |= domainCount << 2 // Store domain count in next 4 bits | ||
| byte |= configuration.usePartitionedCrossSiteSessionCookie ? 1 : 0 << 1 // Store useCrossSiteScripting in next bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
| byte |= configuration.usePartitionedCrossSiteSessionCookie ? 1 : 0 << 1 // Store useCrossSiteScripting in next bit | |
| byte |= configuration.usePartitionedCrossSiteSessionCookie ? 1 : 0 // Store useCrossSiteScripting in next bit |
Suggestion: FMU we are using 7 bits on the 8 available bits. We might want to give the version one more bit, so we can have up to 7 versions?
packages/core/src/domain/session/storeStrategies/sessionInCookie.ts
Outdated
Show resolved
Hide resolved
packages/core/src/domain/session/storeStrategies/sessionInCookie.ts
Outdated
Show resolved
Hide resolved
packages/core/src/browser/cookie.ts
Outdated
| export function getCookies(name: string): string[] | undefined { | ||
| return findAllCommaSeparatedValues(document.cookie).get(name) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: to simplify a bit, always return an array (similar to cookieStore.getAll())
| export function getCookies(name: string): string[] | undefined { | |
| return findAllCommaSeparatedValues(document.cookie).get(name) | |
| } | |
| export function getCookies(name: string): string[] { | |
| return findAllCommaSeparatedValues(document.cookie).get(name) || [] | |
| } |
| if (sessionStoreStrategyType.type === SessionPersistence.COOKIE) { | ||
| const rawSession = retrieveSessionCookie(configuration, sessionStoreStrategyType.cookieOptions) | ||
| // monitor-until: forever, could be handy to troubleshoot issues until session manager rework | ||
| addTelemetryDebug('Unexpected session state', { | ||
| sessionStoreStrategyType: sessionStoreStrategyType.type, | ||
| session: rawSession, | ||
| isSyntheticsTest: isSyntheticsTest(), | ||
| createdTimestamp: rawSession?.created, | ||
| expireTimestamp: rawSession?.expire, | ||
| cookie: await getSessionCookies(), | ||
| currentDomain: `${window.location.protocol}//${window.location.hostname}`, | ||
| }) | ||
| } else if (sessionStoreStrategyType.type === SessionPersistence.LOCAL_STORAGE) { | ||
| const rawSession = retrieveSessionFromLocalStorage() | ||
| // monitor-until: forever, could be handy to troubleshoot issues until session manager rework | ||
| addTelemetryDebug('Unexpected session state', { | ||
| sessionStoreStrategyType: sessionStoreStrategyType.type, | ||
| session: rawSession, | ||
| isSyntheticsTest: isSyntheticsTest(), | ||
| createdTimestamp: rawSession?.created, | ||
| expireTimestamp: rawSession?.expire, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: factorize a bit
let rawSession
let cookieContext
if (sessionStoreStrategyType.type === SessionPersistence.COOKIE) {
rawSession = retrieveSessionCookie(configuration, sessionStoreStrategyType.cookieOptions)
cookieContext = {
cookie: await getSessionCookies(),
currentDomain: `${window.location.protocol}//${window.location.hostname}`,
}
} else {
rawSession = retrieveSessionFromLocalStorage()
}
// monitor-until: forever, could be handy to troubleshoot issues until session manager rework
addTelemetryDebug('Unexpected session state', {
sessionStoreStrategyType: sessionStoreStrategyType.type,
session: rawSession,
isSyntheticsTest: isSyntheticsTest(),
createdTimestamp: rawSession?.created,
expireTimestamp: rawSession?.expire,
...cookieContext
})| await page.waitForTimeout(1000) | ||
|
|
||
| if (!encodeCookieOptions) { | ||
| // ensure the test is failing when the Feature Flag is disabled | ||
| test.fail() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: after we remove the feature flag, it could be nice to remove the waitForTimeout and use something like expect.poll
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit 232186dbf0 will soon be integrated into staging-42.
Commit 232186dbf0 has been merged into staging-42 in merge commit a873893577. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
Integrated commit sha: 232186d Co-authored-by: thomas-lebeau <[email protected]>
| beforeSend: | ||
| initConfiguration.beforeSend && catchUserErrors(initConfiguration.beforeSend, 'beforeSend threw an error:'), | ||
| sessionStoreStrategyType: isWorkerEnvironment ? undefined : selectSessionStoreStrategyType(initConfiguration), | ||
| usePartitionedCrossSiteSessionCookie: initConfiguration.usePartitionedCrossSiteSessionCookie ?? false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: use cookieOption.partitioned or crossSite instead of exposing this property to the config
… from retrieval functions
…to staging-42 pm_trace_id: 79460157 feature_branch_pipeline_id: 79460157 source: to-staging * commit '88c2845f4bd4fe0446a079270832f539160a9163': 🐛 pr-feedback: add 100% telemetry sample rate 🐛 add check for messages 🐛 replace isworker by due to multiple events 🐛 replace check 🐛 pr-feedback: replace node check fix: throw the error fix: super weird linter fix fix: linter warnings fix: compatibility test fix: block logs in node ⚗️ encode cookie options in cookie value (#3905)
Motivation
Some initialization options (
usePartitionedCrossSiteSessionCookieandtrackSessionAccrossSubdomain) affects the scope of the cookie, If theses options are not in sync between each instance of the SDK (i.e. LOGS and RUM), then multiple cookies with the same name can be created can can crash one of the SDKs.Changes
Note
To reduce the risk as much as possible, I did the change only for the session cookie and kept the cookie utils used by other cookies untouched.
Test instructions
Checklist