chore(cookie): remove QueryEscape cookies (#1717) #3683
Conversation
Cookies values are already sanitized by the Go http library, so there is no need to invoke QueryEscape() on them. Furthermore, QueryEscape() has the undesirable effect of replacing spaces wiith "+" characters.
|
(I'm just a passerby, not a Gin maintainer) |
|
This has other potential side effects for example nextjs uses the same url encode strategy making this change means gin will no longer be compatible with cookies in nextjs |
|
Also golang std library does not encode cookie, it simply sanitized it by removing bad character sets. |
|
Why hasn't this fix been merged yet |
|
@cavedon Please help to rebase the master branch |
There was a problem hiding this comment.
Pull Request Overview
This PR removes unnecessary URL-escaping of cookie values in Context.SetCookie, relying on Go’s built-in quoting for values containing spaces, and adds a test to verify correct handling of spaces.
- Removed
url.QueryEscapeusage inSetCookieto prevent spaces from becoming "+" - Added
TestContextSetCookieWithSpaceto ensure values with spaces are quoted
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| context.go | Stop applying url.QueryEscape to cookie values |
| context_test.go | New test to verify quoting of space-containing values |
Comments suppressed due to low confidence (1)
context_test.go:630
- [nitpick] Consider adding tests for other special characters (e.g., semicolons, commas, equals) in cookie values to ensure they are correctly quoted.
func TestContextSetCookieWithSpace(t *testing.T) {
| http.SetCookie(c.Writer, &http.Cookie{ | ||
| Name: name, | ||
| Value: url.QueryEscape(value), | ||
| Value: value, |
There was a problem hiding this comment.
Consider updating the SetCookie doc comment to mention that values are no longer escaped via url.QueryEscape and now rely on Go’s standard cookie quoting behavior.
appleboy
changed the title
There was a problem hiding this comment.
https://github.com/gin-gonic/gin/pull/3683/files#diff-552f47512a00afe5fc6850cc9ddc830a6daeca162750e50aab4ed549685e0253L912
please help to remove this line as well val, _ := url.QueryUnescape(cookie.Value)
6 participants
Cookies values are already sanitized by the Go http library, so there is no need to invoke QueryEscape() on them.
Furthermore, QueryEscape() has the undesirable effect of replacing spaces wiith "+" characters.