diff --git a/context.go b/context.go index a00d1e55..92fb3704 100644 --- a/context.go +++ b/context.go @@ -1056,6 +1056,7 @@ func (c *Context) requestHeader(key string) string { /************************************/ // bodyAllowedForStatus is a copy of http.bodyAllowedForStatus non-exported function. +// Uses http.StatusContinue constant for better code clarity. func bodyAllowedForStatus(status int) bool { switch { case status >= http.StatusContinue && status < http.StatusOK: diff --git a/context_test.go b/context_test.go index 41ec7bd5..44db7475 100644 --- a/context_test.go +++ b/context_test.go @@ -1031,6 +1031,7 @@ func TestContextGetCookie(t *testing.T) { } func TestContextBodyAllowedForStatus(t *testing.T) { + assert.False(t, bodyAllowedForStatus(http.StatusContinue)) assert.False(t, bodyAllowedForStatus(http.StatusProcessing)) assert.False(t, bodyAllowedForStatus(http.StatusNoContent)) assert.False(t, bodyAllowedForStatus(http.StatusNotModified)) @@ -2947,6 +2948,16 @@ func TestContextGetRawData(t *testing.T) { assert.Equal(t, "Fetch binary post data", string(data)) } +func TestContextGetRawDataNilBody(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodPost, "/", nil) + + data, err := c.GetRawData() + assert.Nil(t, data) + require.Error(t, err) + assert.Equal(t, "cannot read nil body", err.Error()) +} + func TestContextRenderDataFromReader(t *testing.T) { w := httptest.NewRecorder() c, _ := CreateTestContext(w) @@ -3535,6 +3546,24 @@ func TestContextSetCookieData(t *testing.T) { setCookie := c.Writer.Header().Get("Set-Cookie") assert.Contains(t, setCookie, "SameSite=None") }) + + // Test that SameSiteDefaultMode inherits from context's sameSite + t.Run("SameSiteDefaultMode inherits context sameSite", func(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.SetSameSite(http.SameSiteStrictMode) + cookie := &http.Cookie{ + Name: "user", + Value: "gin", + Path: "/", + Domain: "localhost", + Secure: true, + HttpOnly: true, + SameSite: http.SameSiteDefaultMode, + } + c.SetCookieData(cookie) + setCookie := c.Writer.Header().Get("Set-Cookie") + assert.Contains(t, setCookie, "SameSite=Strict") + }) } func TestGetMapFromFormData(t *testing.T) { diff --git a/docs/doc.md b/docs/doc.md index 449c8d02..7201df5c 100644 --- a/docs/doc.md +++ b/docs/doc.md @@ -22,6 +22,7 @@ - [How to write log file](#how-to-write-log-file) - [Custom Log Format](#custom-log-format) - [Controlling Log output coloring](#controlling-log-output-coloring) + - [Avoid logging query strings](#avoid-loging-query-strings) - [Model binding and validation](#model-binding-and-validation) - [Custom Validators](#custom-validators) - [Only Bind Query String](#only-bind-query-string) @@ -592,6 +593,20 @@ func main() { } ``` +### Avoid logging query strings + +```go +func main() { + router := gin.New() + + // SkipQueryString indicates that the logger should not log the query string. + // For example, /path?q=1 will be logged as /path + loggerConfig := gin.LoggerConfig{SkipQueryString: true} + + router.Use(gin.LoggerWithConfig(loggerConfig)) +} +``` + ### Model binding and validation To bind a request body into a type, use model binding. We currently support binding of JSON, XML, YAML, TOML and standard form values (foo=bar&boo=baz). diff --git a/logger.go b/logger.go index 6441f7ea..cf92553a 100644 --- a/logger.go +++ b/logger.go @@ -48,6 +48,11 @@ type LoggerConfig struct { // Optional. SkipPaths []string + // SkipQueryString indicates that query strings should not be written + // for cases such as when API keys are passed via query strings. + // Optional. Default value is false. + SkipQueryString bool + // Skip is a Skipper that indicates which logs should not be written. // Optional. Skip Skipper @@ -298,7 +303,7 @@ func LoggerWithConfig(conf LoggerConfig) HandlerFunc { param.BodySize = c.Writer.Size() - if raw != "" { + if raw != "" && !conf.SkipQueryString { path = path + "?" + raw } diff --git a/logger_test.go b/logger_test.go index 53d0df95..395d97e6 100644 --- a/logger_test.go +++ b/logger_test.go @@ -318,20 +318,21 @@ func TestColorForStatus(t *testing.T) { } func TestColorForLatency(t *testing.T) { - colorForLantency := func(latency time.Duration) string { + colorForLatency := func(latency time.Duration) string { p := LogFormatterParams{ Latency: latency, } return p.LatencyColor() } - assert.Equal(t, white, colorForLantency(time.Duration(0)), "0 should be white") - assert.Equal(t, white, colorForLantency(time.Millisecond*20), "20ms should be white") - assert.Equal(t, green, colorForLantency(time.Millisecond*150), "150ms should be green") - assert.Equal(t, cyan, colorForLantency(time.Millisecond*250), "250ms should be cyan") - assert.Equal(t, yellow, colorForLantency(time.Millisecond*600), "600ms should be yellow") - assert.Equal(t, magenta, colorForLantency(time.Millisecond*1500), "1.5s should be magenta") - assert.Equal(t, red, colorForLantency(time.Second*3), "other things should be red") + assert.Equal(t, white, colorForLatency(time.Duration(0)), "0 should be white") + assert.Equal(t, white, colorForLatency(time.Millisecond*20), "20ms should be white") + assert.Equal(t, green, colorForLatency(time.Millisecond*150), "150ms should be green") + assert.Equal(t, cyan, colorForLatency(time.Millisecond*250), "250ms should be cyan") + assert.Equal(t, blue, colorForLatency(time.Millisecond*400), "400ms should be blue") + assert.Equal(t, yellow, colorForLatency(time.Millisecond*600), "600ms should be yellow") + assert.Equal(t, magenta, colorForLatency(time.Millisecond*1500), "1.5s should be magenta") + assert.Equal(t, red, colorForLatency(time.Second*3), "other things should be red") } func TestResetColor(t *testing.T) { @@ -471,3 +472,17 @@ func TestForceConsoleColor(t *testing.T) { // reset console color mode. consoleColorMode = autoColor } + +func TestLoggerWithConfigSkipQueryString(t *testing.T) { + buffer := new(strings.Builder) + router := New() + router.Use(LoggerWithConfig(LoggerConfig{ + Output: buffer, + SkipQueryString: true, + })) + router.GET("/logged", func(c *Context) { c.Status(http.StatusOK) }) + + PerformRequest(router, "GET", "/logged?a=21") + assert.Contains(t, buffer.String(), "200") + assert.NotContains(t, buffer.String(), "a=21") +} diff --git a/render/data.go b/render/data.go index a653ea30..2c0ad5e3 100644 --- a/render/data.go +++ b/render/data.go @@ -4,7 +4,10 @@ package render -import "net/http" +import ( + "net/http" + "strconv" +) // Data contains ContentType and bytes data. type Data struct { @@ -15,6 +18,9 @@ type Data struct { // Render (Data) writes data with custom ContentType. func (r Data) Render(w http.ResponseWriter) (err error) { r.WriteContentType(w) + if len(r.Data) > 0 { + w.Header().Set("Content-Length", strconv.Itoa(len(r.Data))) + } _, err = w.Write(r.Data) return } diff --git a/render/render_test.go b/render/render_test.go index 40e7e7be..b48ab3d3 100644 --- a/render/render_test.go +++ b/render/render_test.go @@ -8,6 +8,7 @@ import ( "encoding/xml" "errors" "html/template" + "io" "net" "net/http" "net/http/httptest" @@ -15,7 +16,6 @@ import ( "strings" "testing" - "github.com/gin-gonic/gin/codec/json" testdata "github.com/gin-gonic/gin/testdata/protoexample" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -23,9 +23,6 @@ import ( "google.golang.org/protobuf/proto" ) -// TODO unit tests -// test errors - func TestRenderJSON(t *testing.T) { w := httptest.NewRecorder() data := map[string]any{ @@ -140,19 +137,44 @@ func TestRenderJsonpJSON(t *testing.T) { } type errorWriter struct { - bufString string + bufString string + ErrThreshold int // 1-based threshold. If 1, errors on the 1st Write call. + writeCount int *httptest.ResponseRecorder } var _ http.ResponseWriter = (*errorWriter)(nil) +func (w *errorWriter) Header() http.Header { + if w.ResponseRecorder == nil { + w.ResponseRecorder = httptest.NewRecorder() + } + return w.ResponseRecorder.Header() +} + +func (w *errorWriter) WriteHeader(statusCode int) { + if w.ResponseRecorder == nil { + w.ResponseRecorder = httptest.NewRecorder() + } + w.ResponseRecorder.WriteHeader(statusCode) +} + func (w *errorWriter) Write(buf []byte) (int, error) { - if string(buf) == w.bufString { - return 0, errors.New(`write "` + w.bufString + `" error`) + w.writeCount++ + if (w.bufString != "" && string(buf) == w.bufString) || (w.ErrThreshold > 0 && w.writeCount >= w.ErrThreshold) { + return 0, errors.New(`write error`) + } + if w.ResponseRecorder == nil { + w.ResponseRecorder = httptest.NewRecorder() } return w.ResponseRecorder.Write(buf) } +func (w *errorWriter) reset() { + w.writeCount = 0 + w.ResponseRecorder = httptest.NewRecorder() +} + func TestRenderJsonpJSONError(t *testing.T) { ew := &errorWriter{ ResponseRecorder: httptest.NewRecorder(), @@ -165,23 +187,33 @@ func TestRenderJsonpJSONError(t *testing.T) { }, } - cb := template.JSEscapeString(jsonpJSON.Callback) - ew.bufString = cb - err := jsonpJSON.Render(ew) // error was returned while writing callback - assert.Equal(t, `write "`+cb+`" error`, err.Error()) + // error was returned while writing callback + ew.reset() + ew.ErrThreshold = 1 + err := jsonpJSON.Render(ew) + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) - ew.bufString = `(` + // error was returned while writing "(" + ew.reset() + ew.ErrThreshold = 2 err = jsonpJSON.Render(ew) - assert.Equal(t, `write "`+`(`+`" error`, err.Error()) + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) - data, _ := json.API.Marshal(jsonpJSON.Data) // error was returned while writing data - ew.bufString = string(data) + // error was returned while writing data + ew.reset() + ew.ErrThreshold = 3 err = jsonpJSON.Render(ew) - assert.Equal(t, `write "`+string(data)+`" error`, err.Error()) + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) - ew.bufString = `);` + // error was returned while writing ");" + ew.reset() + ew.ErrThreshold = 4 err = jsonpJSON.Render(ew) - assert.Equal(t, `write "`+`);`+`" error`, err.Error()) + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) } func TestRenderJsonpJSONError2(t *testing.T) { @@ -385,6 +417,30 @@ func TestRenderBSON(t *testing.T) { assert.Equal(t, "application/bson", w.Header().Get("Content-Type")) } +func TestRenderBSONError(t *testing.T) { + w := httptest.NewRecorder() + data := make(chan int) + + err := (BSON{data}).Render(w) + require.Error(t, err) +} + +func TestRenderBSONWriteError(t *testing.T) { + type testStruct struct { + Value string + } + data := &testStruct{Value: "test"} + + ew := &errorWriter{ + ErrThreshold: 1, + ResponseRecorder: httptest.NewRecorder(), + } + + err := (BSON{data}).Render(ew) + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) +} + func TestRenderXML(t *testing.T) { w := httptest.NewRecorder() data := xmlmap{ @@ -401,6 +457,15 @@ func TestRenderXML(t *testing.T) { assert.Equal(t, "application/xml; charset=utf-8", w.Header().Get("Content-Type")) } +func TestRenderXMLError(t *testing.T) { + w := httptest.NewRecorder() + data := make(chan int) + + err := (XML{data}).Render(w) + require.Error(t, err) + assert.Contains(t, err.Error(), "xml: unsupported type: chan int") +} + func TestRenderRedirect(t *testing.T) { req, err := http.NewRequest(http.MethodGet, "/test-redirect", nil) require.NoError(t, err) @@ -453,6 +518,52 @@ func TestRenderData(t *testing.T) { require.NoError(t, err) assert.Equal(t, "#!PNG some raw data", w.Body.String()) assert.Equal(t, "image/png", w.Header().Get("Content-Type")) + assert.Equal(t, "19", w.Header().Get("Content-Length")) +} + +func TestRenderDataContentLength(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + size, err := strconv.Atoi(r.URL.Query().Get("size")) + assert.NoError(t, err) + + data := Data{ + ContentType: "application/octet-stream", + Data: make([]byte, size), + } + assert.NoError(t, data.Render(w)) + })) + t.Cleanup(srv.Close) + + for _, size := range []int{0, 1, 100, 100_000} { + t.Run(strconv.Itoa(size), func(t *testing.T) { + resp, err := http.Get(srv.URL + "?size=" + strconv.Itoa(size)) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/octet-stream", resp.Header.Get("Content-Type")) + assert.Equal(t, strconv.Itoa(size), resp.Header.Get("Content-Length")) + + actual, err := io.Copy(io.Discard, resp.Body) + require.NoError(t, err) + assert.EqualValues(t, size, actual) + }) + } +} + +func TestRenderDataError(t *testing.T) { + ew := &errorWriter{ + ErrThreshold: 1, + ResponseRecorder: httptest.NewRecorder(), + } + data := []byte("#!PNG some raw data") + + err := (Data{ + ContentType: "image/png", + Data: data, + }).Render(ew) + + require.Error(t, err) + assert.Equal(t, "write error", err.Error()) } func TestRenderString(t *testing.T) { @@ -594,6 +705,32 @@ func TestRenderHTMLDebugPanics(t *testing.T) { assert.Panics(t, func() { htmlRender.Instance("", nil) }) } +func TestRenderHTMLTemplateError(t *testing.T) { + w := httptest.NewRecorder() + templ := template.Must(template.New("t").Parse(`Hello {{if .name}}{{.name.DoesNotExist}}{{end}}`)) + + htmlRender := HTMLProduction{Template: templ} + instance := htmlRender.Instance("t", map[string]any{ + "name": "alexandernyquist", + }) + + err := instance.Render(w) + require.Error(t, err) +} + +func TestRenderHTMLTemplateExecuteError(t *testing.T) { + w := httptest.NewRecorder() + templ := template.Must(template.New("t").Parse(`Hello {{.name.invalid}}`)) + + htmlRender := HTMLProduction{Template: templ} + instance := htmlRender.Instance("t", map[string]any{ + "name": "alexandernyquist", + }) + + err := instance.Render(w) + require.Error(t, err) +} + func TestRenderReader(t *testing.T) { w := httptest.NewRecorder() @@ -645,10 +782,10 @@ func TestRenderWriteError(t *testing.T) { prefix := "my-prefix:" r := SecureJSON{Data: data, Prefix: prefix} ew := &errorWriter{ - bufString: prefix, + ErrThreshold: 1, ResponseRecorder: httptest.NewRecorder(), } err := r.Render(ew) require.Error(t, err) - assert.Equal(t, `write "my-prefix:" error`, err.Error()) + assert.Equal(t, "write error", err.Error()) } diff --git a/tree.go b/tree.go index 3ac0a3b1..580abbaf 100644 --- a/tree.go +++ b/tree.go @@ -818,7 +818,72 @@ walk: // Outer loop for walking the tree return nil } - n = n.children[0] + // When wildChild is true, try static children first (via indices) + // before falling back to the wildcard child. This ensures that + // case-insensitive lookups prefer static routes over param routes + // (e.g., /PREFIX/XXX should resolve to /prefix/xxx, not match :id). + if len(n.indices) > 0 { + rb = shiftNRuneBytes(rb, npLen) + + if rb[0] != 0 { + idxc := rb[0] + for i, c := range []byte(n.indices) { + if c == idxc { + if out := n.children[i].findCaseInsensitivePathRec( + path, ciPath, rb, fixTrailingSlash, + ); out != nil { + return out + } + break + } + } + } else { + var rv rune + var off int + for max_ := min(npLen, 3); off < max_; off++ { + if i := npLen - off; utf8.RuneStart(oldPath[i]) { + rv, _ = utf8.DecodeRuneInString(oldPath[i:]) + break + } + } + + lo := unicode.ToLower(rv) + utf8.EncodeRune(rb[:], lo) + rb = shiftNRuneBytes(rb, off) + + idxc := rb[0] + for i, c := range []byte(n.indices) { + if c == idxc { + if out := n.children[i].findCaseInsensitivePathRec( + path, ciPath, rb, fixTrailingSlash, + ); out != nil { + return out + } + break + } + } + + if up := unicode.ToUpper(rv); up != lo { + utf8.EncodeRune(rb[:], up) + rb = shiftNRuneBytes(rb, off) + + idxc := rb[0] + for i, c := range []byte(n.indices) { + if c == idxc { + if out := n.children[i].findCaseInsensitivePathRec( + path, ciPath, rb, fixTrailingSlash, + ); out != nil { + return out + } + break + } + } + } + } + } + + // Fall back to wildcard child, which is always at the end of the array + n = n.children[len(n.children)-1] switch n.nType { case param: // Find param end (either '/' or path end) diff --git a/tree_test.go b/tree_test.go index b580007d..23339af4 100644 --- a/tree_test.go +++ b/tree_test.go @@ -1018,3 +1018,96 @@ func TestWildcardInvalidSlash(t *testing.T) { } } } + +func TestTreeFindCaseInsensitivePathWithMultipleChildrenAndWildcard(t *testing.T) { + tree := &node{} + + // Setup routes that create a node with both static children and a wildcard child. + // This configuration previously caused a panic ("invalid node type") in + // findCaseInsensitivePathRec because it accessed children[0] instead of the + // wildcard child (which is always at the end of the children array). + // See: https://github.com/gin-gonic/gin/issues/2959 + routes := [...]string{ + "/aa/aa", + "/:bb/aa", + } + + for _, route := range routes { + recv := catchPanic(func() { + tree.addRoute(route, fakeHandler(route)) + }) + if recv != nil { + t.Fatalf("panic inserting route '%s': %v", route, recv) + } + } + + // These lookups previously panicked with "invalid node type" because + // findCaseInsensitivePathRec picked children[0] (a static node) instead + // of the wildcard child at the end of the array. + out, found := tree.findCaseInsensitivePath("/aa", true) + if found { + t.Errorf("Expected no match for '/aa', but got: %s", string(out)) + } + + out, found = tree.findCaseInsensitivePath("/aa/aa/aa/aa", true) + if found { + t.Errorf("Expected no match for '/aa/aa/aa/aa', but got: %s", string(out)) + } + + // Case-insensitive lookup should match the static route /aa/aa + out, found = tree.findCaseInsensitivePath("/AA/AA", true) + if !found { + t.Error("Route '/AA/AA' not found via case-insensitive lookup") + } else if string(out) != "/aa/aa" { + t.Errorf("Wrong result for '/AA/AA': expected '/aa/aa', got: %s", string(out)) + } +} + +func TestTreeFindCaseInsensitivePathWildcardParamAndStaticChild(t *testing.T) { + tree := &node{} + + // Another variant: param route + static route under same prefix + routes := [...]string{ + "/prefix/:id", + "/prefix/xxx", + } + + for _, route := range routes { + recv := catchPanic(func() { + tree.addRoute(route, fakeHandler(route)) + }) + if recv != nil { + t.Fatalf("panic inserting route '%s': %v", route, recv) + } + } + + // Should NOT panic even for paths that don't match any route + out, found := tree.findCaseInsensitivePath("/prefix/a/b/c", true) + if found { + t.Errorf("Expected no match for '/prefix/a/b/c', but got: %s", string(out)) + } + + // Exact match should still work + out, found = tree.findCaseInsensitivePath("/prefix/xxx", true) + if !found { + t.Error("Route '/prefix/xxx' not found") + } else if string(out) != "/prefix/xxx" { + t.Errorf("Wrong result for '/prefix/xxx': %s", string(out)) + } + + // Case-insensitive match should work + out, found = tree.findCaseInsensitivePath("/PREFIX/XXX", true) + if !found { + t.Error("Route '/PREFIX/XXX' not found via case-insensitive lookup") + } else if string(out) != "/prefix/xxx" { + t.Errorf("Wrong result for '/PREFIX/XXX': expected '/prefix/xxx', got: %s", string(out)) + } + + // Param route should still match + out, found = tree.findCaseInsensitivePath("/prefix/something", true) + if !found { + t.Error("Route '/prefix/something' not found via param match") + } else if string(out) != "/prefix/something" { + t.Errorf("Wrong result for '/prefix/something': %s", string(out)) + } +} diff --git a/utils_test.go b/utils_test.go index 893ebc88..e1f2c332 100644 --- a/utils_test.go +++ b/utils_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func init() { @@ -145,6 +146,17 @@ func TestMarshalXMLforH(t *testing.T) { assert.Error(t, e) } +func TestMarshalXMLforHSuccess(t *testing.T) { + h := H{ + "key1": "value1", + "key2": 123, + } + data, err := xml.Marshal(h) + require.NoError(t, err) + assert.Contains(t, string(data), "value1") + assert.Contains(t, string(data), "123") +} + func TestIsASCII(t *testing.T) { assert.True(t, isASCII("test")) assert.False(t, isASCII("๐Ÿงก๐Ÿ’›๐Ÿ’š๐Ÿ’™๐Ÿ’œ"))