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.

Sign up for GitHub

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

Jump to bottom

Make header.Set reset cookies instead of adding them #1864

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Make header.Set reset cookies instead of adding them #1864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
439995e
Clear cookies before setting
sigmundxia Sep 11, 2024
306ebbe
Adjust set cookie test
sigmundxia Sep 11, 2024
58f8ccd
Adjust for lint
sigmundxia Sep 11, 2024
baf3dbf
Use empty string description in test
sigmundxia Sep 11, 2024
c2f49c8
Adjust the method of resetting cookies && add corresponding tests
sigmundxia Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions header.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setSpecialHeader method is used in both the Set and Add methods of Header, so modifying it will affect its usage in Add. To reduce code duplication, an additional parameter can be added to setSpecialHeader, or h.Cookies can be reset before calling setSpecialHeader based on the parent method. Resetting h.Cookies can be done using RequestHeader.DelAllCookies's zero slice strategy, as using make may result in unnecessary memory allocation.

Such a pull request may not necessarily be accepted by @erikdubbelboer, but personally, I would accept it. Why not give it a try? 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! I didn't notice that Add also uses the setSpecialHeader method, sorry for that oversight. I reimplemented the process of resetting cookies based on your suggestion.

I also noticed that ResponseHeader should also have corresponding changes, so I changed it accordingly. In addition, I added tests for each case.

Copy link
Contributor

@newacorn newacorn Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the downstream issue you mentioned.
gofiber/fiber#3089 should be fixed under your two pull requests: this and #1860.

gofiber/fiber#3089 (comment)
Error: "[cookieOne=valueCookieOne cookieTwo=valueCookieTwo 0ookieOne=valueCookieOne cookieTwo=valueCookieTwo]" should have 2 item(s), but has 4

You're great!

Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ func (h *RequestHeader) del(key []byte) {
}

// setSpecialHeader handles special headers and return true when a header is processed.
func (h *ResponseHeader) setSpecialHeader(key, value []byte) bool {
func (h *ResponseHeader) setSpecialHeader(key, value []byte, add bool) bool {
if len(key) == 0 {
return false
}
Expand Down Expand Up @@ -1417,6 +1417,9 @@ func (h *ResponseHeader) setSpecialHeader(key, value []byte) bool {
return true
} else if caseInsensitiveCompare(strSetCookie, key) {
var kv *argsKV
if !add {
h.DelAllCookies()
}
h.cookies, kv = allocArg(h.cookies)
kv.key = getCookieKey(kv.key, value)
kv.value = append(kv.value[:0], value...)
Expand Down Expand Up @@ -1446,7 +1449,7 @@ func (h *ResponseHeader) setNonSpecial(key, value []byte) {
}

// setSpecialHeader handles special headers and return true when a header is processed.
func (h *RequestHeader) setSpecialHeader(key, value []byte) bool {
func (h *RequestHeader) setSpecialHeader(key, value []byte, add bool) bool {
if len(key) == 0 || h.disableSpecialHeader {
return false
}
Expand All @@ -1473,6 +1476,9 @@ func (h *RequestHeader) setSpecialHeader(key, value []byte) bool {
return true
case caseInsensitiveCompare(strCookie, key):
h.collectCookies()
if !add {
h.DelAllCookies()
}
h.cookies = parseRequestCookies(h.cookies, value)
return true
}
Expand Down Expand Up @@ -1561,7 +1567,7 @@ func (h *ResponseHeader) AddBytesV(key string, value []byte) {
// If the header is set as a Trailer (forbidden trailers will not be set, see AddTrailer for more details),
// it will be sent after the chunked response body.
func (h *ResponseHeader) AddBytesKV(key, value []byte) {
if h.setSpecialHeader(key, value) {
if h.setSpecialHeader(key, value, true) {
return
}

Expand Down Expand Up @@ -1620,7 +1626,7 @@ func (h *ResponseHeader) SetBytesKV(key, value []byte) {
// If the header is set as a Trailer (forbidden trailers will not be set, see SetTrailer for more details),
// it will be sent after the chunked response body.
func (h *ResponseHeader) SetCanonical(key, value []byte) {
if h.setSpecialHeader(key, value) {
if h.setSpecialHeader(key, value, false) {
return
}
h.setNonSpecial(key, value)
Expand Down Expand Up @@ -1772,7 +1778,7 @@ func (h *RequestHeader) AddBytesV(key string, value []byte) {
// If the header is set as a Trailer (forbidden trailers will not be set, see AddTrailer for more details),
// it will be sent after the chunked request body.
func (h *RequestHeader) AddBytesKV(key, value []byte) {
if h.setSpecialHeader(key, value) {
if h.setSpecialHeader(key, value, true) {
return
}

Expand Down Expand Up @@ -1831,7 +1837,7 @@ func (h *RequestHeader) SetBytesKV(key, value []byte) {
// If the header is set as a Trailer (forbidden trailers will not be set, see SetTrailer for more details),
// it will be sent after the chunked request body.
func (h *RequestHeader) SetCanonical(key, value []byte) {
if h.setSpecialHeader(key, value) {
if h.setSpecialHeader(key, value, false) {
return
}
h.setNonSpecial(key, value)
Expand Down
61 changes: 61 additions & 0 deletions header_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1643,7 +1643,32 @@ func TestRequestHeaderSetCookie(t *testing.T) {
var h RequestHeader

h.Set("Cookie", "foo=bar; baz=aaa")
if string(h.Cookie("foo")) != "bar" {
t.Fatalf("Unexpected cookie %q. Expecting %q", h.Cookie("foo"), "bar")
}
if string(h.Cookie("baz")) != "aaa" {
t.Fatalf("Unexpected cookie %q. Expecting %q", h.Cookie("baz"), "aaa")
}

h.Set("cOOkie", "xx=yyy")
if len(h.Cookie("foo")) != 0 {
t.Fatalf("Unexpected cookie %q. Expecting empty string", h.Cookie("foo"))
}
if len(h.Cookie("baz")) != 0 {
t.Fatalf("Unexpected cookie %q. Expecting empty string", h.Cookie("baz"))
}
if string(h.Cookie("xx")) != "yyy" {
t.Fatalf("unexpected cookie %q. Expecting %q", h.Cookie("xx"), "yyy")
}
}

func TestRequestHeaderAddCookie(t *testing.T) {
t.Parallel()

var h RequestHeader

h.Add("Cookie", "foo=bar; baz=aaa")
h.Add("cOOkie", "xx=yyy")

if string(h.Cookie("foo")) != "bar" {
t.Fatalf("Unexpected cookie %q. Expecting %q", h.Cookie("foo"), "bar")
Expand All @@ -1662,8 +1687,44 @@ func TestResponseHeaderSetCookie(t *testing.T) {
var h ResponseHeader

h.Set("set-cookie", "foo=bar; path=/aa/bb; domain=aaa.com")

var c Cookie
c.SetKey("foo")
if !h.Cookie(&c) {
t.Fatalf("cannot obtain %q cookie", c.Key())
}
if string(c.Value()) != "bar" {
t.Fatalf("unexpected cookie value %q. Expected %q", c.Value(), "bar")
}
if string(c.Path()) != "/aa/bb" {
t.Fatalf("unexpected cookie path %q. Expected %q", c.Path(), "/aa/bb")
}
if string(c.Domain()) != "aaa.com" {
t.Fatalf("unexpected cookie domain %q. Expected %q", c.Domain(), "aaa.com")
}

h.Set(HeaderSetCookie, "aaaaa=bxx")

if h.Cookie(&c) {
t.Fatalf("obtain %q cookie", c.Key())
}
c.SetKey("aaaaa")
if !h.Cookie(&c) {
t.Fatalf("cannot obtain %q cookie", c.Key())
}
if string(c.Value()) != "bxx" {
t.Fatalf("unexpected cookie value %q. Expecting %q", c.Value(), "bxx")
}
}

func TestResponseHeaderAddCookie(t *testing.T) {
t.Parallel()

var h ResponseHeader

h.Add("set-cookie", "foo=bar; path=/aa/bb; domain=aaa.com")
h.Add(HeaderSetCookie, "aaaaa=bxx")

var c Cookie
c.SetKey("foo")
if !h.Cookie(&c) {
Expand Down