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

Delete client cookie with a domain scope #951

Closed
kiyonlin opened this issue Jan 23, 2021 · 8 comments
Closed

Delete client cookie with a domain scope #951

kiyonlin opened this issue Jan 23, 2021 · 8 comments

Comments

@kiyonlin
Copy link
Contributor

kiyonlin commented Jan 23, 2021

Related with gofiber/fiber#1127

Once a cookie is set with a domain scope, c.Response.Header.DelClientCookie won't work anymore. Browser still keeps the cookie.

I made a runnable demo to elaborate this issue(should append 127.0.0.1 a.example.com to local host):

package main

import (
	"time"

	"github.com/valyala/fasthttp"
)

func main() {
	fasthttp.ListenAndServe(":3000", func(c *fasthttp.RequestCtx) {
		switch string(c.Path()) {
		case "/":
			cookie := fasthttp.AcquireCookie()
			cookie.SetKey("key")
			cookie.SetValue("value")
			cookie.SetHTTPOnly(true)
			cookie.SetDomain("a.example.com")
			cookie.SetExpire(time.Now().Add(time.Hour))
			c.Response.Header.SetCookie(cookie)
			fasthttp.ReleaseCookie(cookie)
			c.SetBodyString("set domain cookie")
		case "/clear":
			// custom deletion works

			//cookie := fasthttp.AcquireCookie()
			//cookie.SetKey("key")
			//cookie.SetDomain("a.example.com")
			//cookie.SetExpire(fasthttp.CookieExpireDelete)
			//c.Response.Header.SetCookie(cookie)
			//fasthttp.ReleaseCookie(cookie)

			// DelClientCookie does not work, browser does not delete cookie
			c.Response.Header.DelClientCookie("key")
			c.SetBodyString("clear cookie")
		}
	})
}

How about adding another function to delete client cookie like

func (h *ResponseHeader) DelClientCookieWithDomain(key, domain string) {
	h.DelCookie(key)

	c := AcquireCookie()
	c.SetKey(key)
	c.SetDomain(domain)
	c.SetExpire(CookieExpireDelete)
	h.SetCookie(c)
	ReleaseCookie(c)
}

Any ideas?

@erikdubbelboer
Copy link
Collaborator

I'm not sure. Then you also kinda need to add DelClientCookieWithPath and DelClientCookieWithDomainAndPath. I think the current DelClientCookie is a good example of how to delete a cookie. If people need to delete cookies with domains or paths they can easily implement this themselves.

@kiyonlin
Copy link
Contributor Author

@erikdubbelboer I've tested path which isn't counted as a scope(I don't know why).

@erikdubbelboer
Copy link
Collaborator

Hmm I didn't know that. In that case a pull request to add DelClientCookieWithDomain is welcome.

@kiyonlin
Copy link
Contributor Author

I will test all cookie fields, and if only domain affects deletion, I will make a pull request.

@kiyonlin
Copy link
Contributor Author

kiyonlin commented Jan 25, 2021

@erikdubbelboer Bad luck, both domain and path affect deletion. I must missed something in yesterday's test.
DelClientCookieWithDomain is not a good idea. But should we document this case in DelClientCookie?

@erikdubbelboer
Copy link
Collaborator

Thanks for the research. Yes let's document it. Can you make a pull?

@kiyonlin
Copy link
Contributor Author

Sure.

@erikdubbelboer
Copy link
Collaborator

See: #956

erikdubbelboer pushed a commit that referenced this issue Feb 22, 2021
* Improve documentation about DelClientCookie which related with #951.

* Fix ms cleaner in Client
erikdubbelboer pushed a commit that referenced this issue Mar 5, 2021
* Improve documentation about DelClientCookie which related with #951.

* Add DisableHeaderNamesNormalizing to PipelineClient
erikdubbelboer pushed a commit that referenced this issue Mar 5, 2021
* Improve documentation about DelClientCookie which related with #951.

* use proxy.FromURL to support auth
erikdubbelboer pushed a commit that referenced this issue Mar 15, 2021
* Improve documentation about DelClientCookie which related with #951.

* Add pipeline name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants