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

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

@flamisz flamisz added the Needs Review PRs that need a code review label Feb 23, 2021
@tsteur
Copy link
Member

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 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 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 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 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
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 1, 2021
@diosmosis
Copy link
Member

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

@diosmosis diosmosis merged commit 813e637 into 4.x-dev Mar 2, 2021
@diosmosis diosmosis deleted the 16637-cookie-delete branch March 2, 2021 05:19
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 not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core/Cookie.php:Cookie->delete() don't set secure and SameSite.
4 participants