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

Add SameSite None to fix embedded version #610

Conversation

GSokol
Copy link
Contributor

@GSokol GSokol commented Aug 11, 2022

The embedded version fof the site failed to use cookies, so it generates
new anonymous id. Fixing it.

Description of the change

Description here

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

This change is Reviewable

@saikumarrs
Copy link
Member

@GSokol Thanks for raising the PR. However, the changes could inadvertently turn out to be a breaking change for some SDK users.

Instead, you can add support for SameSite storage option here. We already support an option for the Secure header.

@GSokol
Copy link
Contributor Author

GSokol commented Aug 18, 2022

@saikumarrs, yes, great advice! Thank you, I will do so!

utils/storage/cookie.js Outdated Show resolved Hide resolved
@saikumarrs
Copy link
Member

@saikumarrs, yes, great advice! Thank you, I will do so!

Thanks. The changes look good to me. I left a comment to address a minor issue.

As a next step, please sign the CLA using this form.

@saikumarrs saikumarrs changed the base branch from production to production-staging August 18, 2022 15:35
@GSokol GSokol force-pushed the Add_samesite_none_to_make_embedded_version_work_proper branch from bbef228 to 28ed782 Compare August 18, 2022 16:01
@GSokol
Copy link
Contributor Author

GSokol commented Aug 18, 2022

@saikumarrs done

@GSokol GSokol requested a review from saikumarrs August 19, 2022 07:00
@saikumarrs
Copy link
Member

@saikumarrs done

@GSokol Thanks. We'll absorb this PR and release a new SDK version next week.
Thanks for understanding.

@GSokol GSokol force-pushed the Add_samesite_none_to_make_embedded_version_work_proper branch 2 times, most recently from ea7e68c to 14c2c4c Compare August 23, 2022 10:32
Move the setting to config.
@GSokol GSokol force-pushed the Add_samesite_none_to_make_embedded_version_work_proper branch from 14c2c4c to 8c4ae88 Compare August 24, 2022 10:05
@saikumarrs
Copy link
Member

@GSokol Thanks for your patience. We are currently swamped and won't be able to release a new version of the SDK this week. However, we've added this to our immediate sprint roadmap and will include it in the next week's release.
CC @MoumitaM @pallabmaiti

MoumitaM
MoumitaM previously approved these changes Aug 25, 2022
@saikumarrs saikumarrs merged commit f7d6a37 into rudderlabs:production-staging Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants