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
IgnoreCookie: added support for cookie_domain and ignore cookie signature #13301
IgnoreCookie: added support for cookie_domain and ignore cookie signature #13301
Conversation
…lidation IgnoreCookie: added support for setting "cookie_domain" IgnoreCookie: do not validate cookie signature to allow sharing it between multiple matomo instances
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.
@MichaelHeerklotz @diosmosis I'm actually not quite getting why something should be or should not be signed. Think I'm missing something. What and how exactly are we signing things here? For what reason? Sorry I'm not much into this. |
@tsteur my guess was the ignore cookie was being created outside of matomo or something like that. But that's just a guess. |
@tsteur @diosmosis for the ignore cookie it makes no sense , so I have removed the check for it. this allows using the same ignore cookie for multiple matomo setups (if everything is running under the same root domain (big setups) |
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.
@MichaelHeerklotz left a minor comment, otherwise looks all good to merge
core/Cookie.php
Outdated
* @return string|bool Content or false if unsigned | ||
*/ | ||
private function extractSignedContent($content) | ||
private function extractSignedContent($content, $validate = true) |
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.
Not too important but maybe the parameter doesn't need to have a default value?
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 this function is private, it makes sense 👍
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 you have time be good to change otherwise not too important
@@ -227,9 +229,9 @@ private function extractSignedContent($content) | |||
* Unserialize the array when necessary. | |||
* Decode the non numeric values that were base64 encoded. | |||
*/ | |||
protected function loadContentFromCookie() | |||
protected function loadContentFromCookie($validateSignature = true) |
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.
Not too important but maybe the parameter doesn't need to have a default value?
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 this function is protected it maybe could break classes that extend it (some plugins maybe)? I put the default value for safety, but if you do not like it, I have nothing against removing it :)
@MichaelHeerklotz could you resolve the merge conflict and then ping me? I'll merge it then. |
Cookie: added parameter 'validateSignature' to switch off signature validation
IgnoreCookie: added support for setting "cookie_domain"
IgnoreCookie: do not validate cookie signature to allow sharing it between multiple matomo instances