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

IgnoreCookie: added support for cookie_domain and ignore cookie signature #13301

Merged

Conversation

MichaelRoosz
Copy link
Contributor

@MichaelRoosz MichaelRoosz commented Aug 16, 2018

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

…lidation

IgnoreCookie: added support for setting "cookie_domain"
IgnoreCookie: do not validate cookie signature to allow sharing it between multiple matomo instances
@mattab mattab added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Jun 27, 2019
@tsteur tsteur added this to the 4.0.0 milestone Nov 11, 2019
@tsteur tsteur changed the base branch from 3.x-dev to 4.x-dev January 14, 2020 03:57
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't think we care if the ignore cookie's signature is absent or incorrect, @tsteur / @sgiehl ?

@tsteur
Copy link
Member

tsteur commented Feb 7, 2020

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

@diosmosis
Copy link
Member

@tsteur my guess was the ignore cookie was being created outside of matomo or something like that. But that's just a guess.

@MichaelRoosz
Copy link
Contributor Author

@tsteur @diosmosis
my guess is, that the sign check was added to avoid fake visitorids / etc created by cookie manipulation.

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)

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.

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

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?

Copy link
Contributor Author

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 👍

Copy link
Member

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

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?

Copy link
Contributor Author

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

@tsteur
Copy link
Member

tsteur commented Jun 28, 2020

@MichaelHeerklotz could you resolve the merge conflict and then ping me? I'll merge it then.

@tsteur tsteur merged commit 82d8267 into matomo-org:4.x-dev Jun 29, 2020
@MichaelRoosz MichaelRoosz deleted the ignore_cookie-dont-check-value branch October 29, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants