@flamisz opened this Pull Request on February 23rd 2021 Contributor

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 commented on February 23rd 2021 Contributor

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 commented on February 23rd 2021 Member

@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 commented on February 23rd 2021 Contributor

@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 commented on February 23rd 2021 Member

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 commented on February 23rd 2021 Contributor

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 commented on February 23rd 2021 Contributor

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.

@diosmosis commented on March 2nd 2021 Member

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

This Pull Request was closed on March 2nd 2021
Powered by GitHub Issue Mirror