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

header.Set does not reset but adds cookies #1862

Open
sigmundxia opened this issue Sep 11, 2024 · 2 comments
Open

header.Set does not reset but adds cookies #1862

sigmundxia opened this issue Sep 11, 2024 · 2 comments

Comments

@sigmundxia
Copy link
Contributor

Hello developers,

I noticed that the header.Set function seems to have a problem when handling cookies. According to the definition of Set, it seems that when operating cookies, all current cookies should be cleared and then new values ​​should be added. However, when the following code actually performs the operation, the original cookies are still retained, making the actual effect become adding cookies:

fasthttp/header.go

Lines 1474 to 1477 in 65e989e

case caseInsensitiveCompare(strCookie, key):
h.collectCookies()
h.cookies = parseRequestCookies(h.cookies, value)
return true

fasthttp/cookie.go

Lines 505 to 516 in 65e989e

func parseRequestCookies(cookies []argsKV, src []byte) []argsKV {
var s cookieScanner
s.b = src
var kv *argsKV
cookies, kv = allocArg(cookies)
for s.next(kv) {
if len(kv.key) > 0 || len(kv.value) > 0 {
cookies, kv = allocArg(cookies)
}
}
return releaseArg(cookies)
}

This also caused a downstream issue.

May I ask if I have misunderstood the meaning of the operation, and this effect is actually expected by fasthttp? Or is there indeed some problem? If there is indeed a problem, I am happy to submit a pull request to fix it.

Best wishes!

@newacorn
Copy link
Contributor

Yes, among all special headers, Cookie is indeed unique in that it is the only one that allows multiple values and should be considered separately. Although there is a Header.SetCookie method that performs similar behavior, it is specifically designed for the Cookie header. Even if the semantics are slightly incorrect, it can be justified as long as documentation is provided to explain it.

Given that Request.Header includes Add and Set methods, which were designed with precise semantics in mind, addressing the issue would ensure consistency and accuracy in handling multi-value headers.

I think this has almost no backward compatibility issues.

I personally support your submission of this pull request.

@sigmundxia
Copy link
Contributor Author

Thanks for your detailed reply! I'll try to make a pull request.

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