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

SameSite cookie attribute can now be configured in JS tracker #16724

Merged
merged 6 commits into from Nov 17, 2020
Merged

SameSite cookie attribute can now be configured in JS tracker #16724

merged 6 commits into from Nov 17, 2020

Conversation

fealXX
Copy link

@fealXX fealXX commented Nov 16, 2020

Description:

Re-implements the Changes made by @beger in PR#16504, added the discussed functionality

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

js/piwik.js Show resolved Hide resolved
js/piwik.js Outdated
* @param string either Lax, None or Strict
*/
this.setCookieSameSite = function (sameSite) {
if (sameSite != 'None' && sameSite != 'Lax' && sameSite != 'Strict') {
Copy link
Member

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

Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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.
Copy link
Member

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?

Copy link
Member

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).

Copy link
Member

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

Copy link
Author

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."

Copy link
Member

@tsteur tsteur left a 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 👍

@fealXX fealXX requested a review from tsteur November 16, 2020 22:38
Copy link
Member

@tsteur tsteur left a 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') {
Copy link
Member

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

Copy link
Member

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.

Copy link
Author

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') {
Copy link
Member

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
image

Copy link
Author

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

@fealXX fealXX requested a review from tsteur November 17, 2020 00:22
@tsteur
Copy link
Member

tsteur commented Nov 17, 2020

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 API methods that the method exists and update the documentation. Hoping to do this tomorrow.

@tsteur tsteur changed the base branch from 4.x-dev to m16724 November 17, 2020 20:13
@tsteur tsteur merged commit 995f2c0 into matomo-org:m16724 Nov 17, 2020
@mattab mattab changed the title Re-Implement #16161: SameSite cookie attribute can be configured for JS tracker for 4.x-dev SameSite cookie attribute can now be configured in JS tracker Nov 23, 2020
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants