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
SameSite cookie attribute can now be configured in JS tracker #16724
Conversation
js/piwik.js
Outdated
* @param string either Lax, None or Strict | ||
*/ | ||
this.setCookieSameSite = function (sameSite) { | ||
if (sameSite != 'None' && sameSite != 'Lax' && sameSite != 'Strict') { |
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.
btw not too important but be great to use !==
and below ===
for exact comparison.
Also should we maybe compare lowercase and then convert the first letter to upper case when setting the value? Just mentioning this because some users will try to set lax
or strict
.
Meaning be like
this.setCookieSameSite = function (sameSite) {
sameSite = String(sameSite).toLowerCase();
if (sameSite === 'none' || sameSite === 'lax' || sameSite === 'strict') {
sameSite = sameSite.charAt(0).toUpperCase() + sameSite.slice(1)
} else {
logConsoleError('ignored value for sameSite. Please use either lax, none, or strict')
}
see above example be also good to not silently convert value to Lax
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.
If a user has a site available through HTTP and HTTPS, then setting sameSite('None')
would only work on HTTPS. On HTTP it would log an error but still try to set sameSite=None
. I wonder if we would need to additionally add something like below
if (sameSite === 'none' && location.protocol !== 'https:') {
logConsoleError('sameSite=none wont work because it is not using HTTPS')
return;
}
this way we would keep Lax
and a user can conveniently set sameSite('None')
and know it will only be applied on HTTPS traffic.
We would still keep the additional if(location.protocol !== 'https:') {
check in setSecureCookie
as this is useful too.
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.
As Lax is the Default value, I was reverting to that, so that a non-valid combination can't happen.
Will change in next commit.
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.
I don't think it is necessary to revert to Lax on HTTP, as the usecase for Samesite=None will only work on https and the cookie will not be accepted by the browser in that case anyways.
js/piwik.js
Outdated
* Set the SameSite attribute for cookies to a custom value. | ||
* You might want to use this if your site is running in an iframe since | ||
* then it will only be able to access the cookies if SameSite is set to 'None'. | ||
* Sets to Lax if invalid parameter is passed, sets CookieIsSecure to true on None. |
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.
could we mention that None
will only work on HTTPS?
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.
btw thinking we maybe could also mention that None
might cause issues if the user is sometimes on HTTPS and sometimes not (eg a site allows access through HTTP and HTTPS).
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.
also thinking we might need to mention that setting None
will also directly set secure cookie flag
and if users browse HTTP and HTTPS during same visits it would create two visits because two different cookies would be created AFAIK
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.
Along the lines of this?
setSecureCookie
"Warning: If your site is available under http and https, setting this might lead to duplicate or incomplete visits."
setCookieSameSite
"Warning: Sets CookieIsSecure to true on None, because None will only work with Secure; cookies If your site is available under http and https, using "None" might lead to duplicate or incomplete visits."
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.
Thanks for this @fealXX very appreciated . Left a few comments 👍
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.
👍 thanks for the quick changes @fealXX. Left another minor comment. Once this is changed will do some functional tests
js/piwik.js
Outdated
* @param string either Lax, None or Strict | ||
*/ | ||
this.setCookieSameSite = function (sameSite) { | ||
if (sameSite === 'none' || sameSite === 'lax' || sameSite === 'strict') { |
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.
@fealXX could we do add a sameSite = String(sameSite).toLowerCase()
as a first line? So it also works for None
and NONE
etc? then below else if
could be changed to else
like
this.setCookieSameSite = function (sameSite) {
sameSite = String(sameSite).toLowerCase()
if (sameSite === 'none' || sameSite === 'lax' || sameSite === 'strict') {
sameSite = sameSite.charAt(0).toUpperCase() + sameSite.toLowerCase().slice(1);
} else (sameSite !== 'None' && sameSite !== 'Lax' && sameSite !== 'Strict') {
logConsoleError('Ignored value for sameSite. Please use either Lax, None, or Strict.');
return;
}
....
(also added a .toLowerCase()
where it converts the sameSite to upper case first then lower case
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.
actually I just realise now that the code below also supports already like lowercase and upper case first so could also ignore this change. It be only interesting if we wanted to support NONE
etc but that's not needed.
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.
I do think that is a more elegant solution though.
logConsoleError('Ignored value for sameSite. Please use either Lax, None, or Strict.'); | ||
return; | ||
} | ||
if (sameSite === 'None') { |
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.
I reckon it be still great to not set sameSite=None
on HTTP just to make sure the cookie still works on HTTP. So be great to convert it to Lax
on HTTP protocol when it is otherwise none. I mean like
if (sameSite === 'None') {
if (location.protocol === 'https:') {
this.setSecureCookie(true);
} else { sameSite = 'Lax' }
}
I tested it with document.cookie = 'foo=bar;path=/;secure;SameSite=None'
on HTTP and it would otherwise show issues like this
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.
The cookie will be set, but it will not be sent with subsequent requests, if you're in a situation where sameSite=None would be needed. Added the fallback and a warning for reverting to Lax nonetheless
Worked for me. Wasn't sure if it affects eg deleteCookie but all seems to work and seems safe to merge for 4.0 release. We will next need to merge this into a different branch in our repo, build the minified JS and add a simple test to |
Description:
Re-implements the Changes made by @beger in PR#16504, added the discussed functionality
Review