https://github.com/matomo-org/matomo/pull/17255 - Mouve

Site d'origine

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

set secure and samesite for cookie on delete #17255

Merged
merged 2 commits into from Mar 2, 2021
Merged

Conversation

@flamisz
Copy link
Contributor

@flamisz flamisz commented Feb 23, 2021

Description:

fix: #16637

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
@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Feb 23, 2021

I reproduced the bug and what I found is Firefox is happy to delete the cookie without the extra settings, but Chrome doesn't.

I checked the setIgnoreCookie() function, and replicated the same if-else during the delete.

The question is: what happens if the cookie is already there, but it was made without https and then they changed to https?

Other solution could be to send two setcookie headers, one without and one with the extra settings. This is what the original issue's description mentions as a solution.

@tsteur
Copy link
Member

@tsteur tsteur commented Feb 23, 2021

@flamisz I reckon it be good to have the additional core there too if it doesn't cause any issue. This way it might be more likely that any older cookie will be deleted too (or if the original cookie was set maybe on http and then now it deletes it on https etc). If there's no harm, it would be great to have.

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Feb 23, 2021

@tsteur something like this?

if (ProxyHttp::isHttps()) {
    $this->setCookie($this->name, 'deleted', time() - 31536001, $this->path, $this->domain, TRUE, FALSE, 'None');
    $this->setCookie($this->name, 'deleted', time() - 31536001, $this->path, $this->domain);
} else {
    $this->setCookie($this->name, 'deleted', time() - 31536001, $this->path, $this->domain);
}

or just simple (without if)

$this->setCookie($this->name, 'deleted', time() - 31536001, $this->path, $this->domain, TRUE, FALSE, 'None');
$this->setCookie($this->name, 'deleted', time() - 31536001, $this->path, $this->domain);
@tsteur
Copy link
Member

@tsteur tsteur commented Feb 23, 2021

would probably simply do the second one without if but I'm not too much into the details. Just seems like it could maybe cover more cases where it would otherwise maybe not delete the cookie.

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Feb 23, 2021

I tried to replicate the case, when we have a non secure cookie on a secure site. It looks like Chrome doesn't care and will delete anyway with the secure and samesite settings, and Firefox actually deleted by itself the non-secure cookie on secure domain immediately.

@flamisz
Copy link
Contributor Author

@flamisz flamisz commented Feb 23, 2021

I like the simple solution, let's go with that one. Multiple set-cookie in the header is possible and it doesn't hurt anything.

@flamisz flamisz self-assigned this Feb 25, 2021
@sgiehl sgiehl added this to the 4.3.0 milestone Mar 1, 2021
@diosmosis
Copy link
Member

@diosmosis diosmosis commented Mar 2, 2021

tested locally, seems to work. (also tested in safari & ie11)

@diosmosis diosmosis merged commit 813e637 into 4.x-dev Mar 2, 2021
1 of 3 checks passed
1 of 3 checks passed
PHPCS
Details
Travis CI - Branch Build Failed
Details
Travis CI - Pull Request Build Failed
Details
@diosmosis diosmosis deleted the 16637-cookie-delete branch Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants

Raccourcis

Commandes

Fermer