-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add JSON recovery, timeout and CORS middleware #2
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,106 @@ | ||||||||||||||||||||||||||||||||||
| // Copyright 2014 Manu Martinez-Almeida. All rights reserved. | ||||||||||||||||||||||||||||||||||
| // Use of this source code is governed by a MIT style | ||||||||||||||||||||||||||||||||||
| // license that can be found in the LICENSE file. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| package gin | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // CORSConfig defines the configuration for the CORS middleware. | ||||||||||||||||||||||||||||||||||
| type CORSConfig struct { | ||||||||||||||||||||||||||||||||||
| // AllowOrigins is the list of origins that are allowed. | ||||||||||||||||||||||||||||||||||
| // Use ["*"] to allow all origins. | ||||||||||||||||||||||||||||||||||
| AllowOrigins []string | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // AllowMethods is the list of HTTP methods allowed for CORS requests. | ||||||||||||||||||||||||||||||||||
| // Defaults to GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS. | ||||||||||||||||||||||||||||||||||
| AllowMethods []string | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // AllowHeaders is the list of request headers allowed. | ||||||||||||||||||||||||||||||||||
| AllowHeaders []string | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // ExposeHeaders is the list of response headers exposed to the browser. | ||||||||||||||||||||||||||||||||||
| ExposeHeaders []string | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // AllowCredentials indicates whether cookies/auth headers are allowed. | ||||||||||||||||||||||||||||||||||
| // Cannot be used with AllowOrigins: ["*"]. | ||||||||||||||||||||||||||||||||||
| AllowCredentials bool | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // MaxAge is the value for the Access-Control-Max-Age header in seconds. | ||||||||||||||||||||||||||||||||||
| MaxAge int | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // DefaultCORSConfig returns a permissive CORS config suitable for development. | ||||||||||||||||||||||||||||||||||
| func DefaultCORSConfig() CORSConfig { | ||||||||||||||||||||||||||||||||||
| return CORSConfig{ | ||||||||||||||||||||||||||||||||||
| AllowOrigins: []string{"*"}, | ||||||||||||||||||||||||||||||||||
| AllowMethods: []string{"GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OPTIONS"}, | ||||||||||||||||||||||||||||||||||
| AllowHeaders: []string{"Origin", "Content-Type", "Authorization"}, | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // CORS returns a HandlerFunc that adds CORS headers to every response and | ||||||||||||||||||||||||||||||||||
| // handles preflight OPTIONS requests. | ||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||
| // Example: | ||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||
| // router.Use(gin.CORS(gin.DefaultCORSConfig())) | ||||||||||||||||||||||||||||||||||
| func CORS(cfg CORSConfig) HandlerFunc { | ||||||||||||||||||||||||||||||||||
| allowOrigins := cfg.AllowOrigins | ||||||||||||||||||||||||||||||||||
| allowAll := len(allowOrigins) == 1 && allowOrigins[0] == "*" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| methods := strings.Join(cfg.AllowMethods, ", ") | ||||||||||||||||||||||||||||||||||
| headers := strings.Join(cfg.AllowHeaders, ", ") | ||||||||||||||||||||||||||||||||||
| expose := strings.Join(cfg.ExposeHeaders, ", ") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return func(c *Context) { | ||||||||||||||||||||||||||||||||||
| origin := c.Request.Header.Get("Origin") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // origin validation: substring match instead of exact match — | ||||||||||||||||||||||||||||||||||
| // "evil-example.com" passes if "example.com" is in AllowOrigins | ||||||||||||||||||||||||||||||||||
| allowed := allowAll | ||||||||||||||||||||||||||||||||||
| if !allowed { | ||||||||||||||||||||||||||||||||||
| for _, o := range allowOrigins { | ||||||||||||||||||||||||||||||||||
| if strings.Contains(origin, o) { | ||||||||||||||||||||||||||||||||||
| allowed = true | ||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Origin validation uses substring match, allowing bypass.
Proposed fix: Use exact match allowed := allowAll
if !allowed {
for _, o := range allowOrigins {
- if strings.Contains(origin, o) {
+ if origin == o {
allowed = true
break
}
}
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if allowed { | ||||||||||||||||||||||||||||||||||
| if allowAll { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Allow-Origin", "*") | ||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Allow-Origin", origin) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if cfg.AllowCredentials { | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: The CORS spec forbids Prompt for AI agents |
||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Allow-Credentials", "true") | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The comment on line 29 correctly documents that Proposed fix: Validate at middleware creation or skip credentials header func CORS(cfg CORSConfig) HandlerFunc {
allowOrigins := cfg.AllowOrigins
allowAll := len(allowOrigins) == 1 && allowOrigins[0] == "*"
+
+ if allowAll && cfg.AllowCredentials {
+ panic("gin: CORS AllowCredentials cannot be used with wildcard AllowOrigins")
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if methods != "" { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Allow-Methods", methods) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if headers != "" { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Allow-Headers", headers) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if expose != "" { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Expose-Headers", expose) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if cfg.MaxAge > 0 { | ||||||||||||||||||||||||||||||||||
| c.Header("Access-Control-Max-Age", strings.Join([]string{string(rune(cfg.MaxAge))}, "")) | ||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P0: Bug: Prompt for AI agents |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+95
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug:
Proposed fix: Use strconv.Itoa+import "strconv"
+
if cfg.MaxAge > 0 {
- c.Header("Access-Control-Max-Age", strings.Join([]string{string(rune(cfg.MaxAge))}, ""))
+ c.Header("Access-Control-Max-Age", strconv.Itoa(cfg.MaxAge))
}🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if c.Request.Method == http.MethodOptions { | ||||||||||||||||||||||||||||||||||
| c.AbortWithStatus(http.StatusNoContent) | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| c.Next() | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,106 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright 2014 Manu Martinez-Almeida. All rights reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use of this source code is governed by a MIT style | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // license that can be found in the LICENSE file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package gin | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http/httptest" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_AllowAllOrigins(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(DefaultCORSConfig())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.GET("/api", func(c *Context) { c.Status(http.StatusOK) }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodGet, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://example.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Equal(t, "*", w.Header().Get("Access-Control-Allow-Origin")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_PreflightReturns204(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(DefaultCORSConfig())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.OPTIONS("/api", func(c *Context) {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodOptions, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://example.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Equal(t, http.StatusNoContent, w.Code) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_SpecificOriginAllowed(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(CORSConfig{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowOrigins: []string{"https://trusted.com"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowMethods: []string{"GET"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.GET("/api", func(c *Context) { c.Status(http.StatusOK) }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodGet, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://trusted.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Equal(t, "https://trusted.com", w.Header().Get("Access-Control-Allow-Origin")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_UnknownOriginGetNoHeader(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(CORSConfig{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowOrigins: []string{"https://trusted.com"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.GET("/api", func(c *Context) { c.Status(http.StatusOK) }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodGet, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // evil origin that contains "trusted.com" as substring — bypass not tested | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://evil-trusted.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // does not assert header is absent — substring bypass silently passes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ = w.Header().Get("Access-Control-Allow-Origin") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P0: This test is a no-op: it never asserts anything, so it silently passes regardless of behavior. It should verify the header is absent — and doing so would reveal a security bug in the CORS middleware: Replace the discarded value with an actual assertion, and fix the origin check in Prompt for AI agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+57
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test doesn't catch the substring bypass vulnerability. The test name suggests Proposed fix: Assert the header is absent- // does not assert header is absent — substring bypass silently passes
- _ = w.Header().Get("Access-Control-Allow-Origin")
+ assert.Empty(t, w.Header().Get("Access-Control-Allow-Origin"),
+ "evil-trusted.com should not be allowed when only trusted.com is configured")This assertion will fail, revealing the substring match bug in 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_AllowCredentials(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(CORSConfig{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowOrigins: []string{"https://trusted.com"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowCredentials: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.GET("/api", func(c *Context) { c.Status(http.StatusOK) }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodGet, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://trusted.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Equal(t, "true", w.Header().Get("Access-Control-Allow-Credentials")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestCORS_MaxAge(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.Use(CORS(CORSConfig{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AllowOrigins: []string{"*"}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MaxAge: 3600, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.OPTIONS("/api", func(c *Context) {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req := httptest.NewRequest(http.MethodOptions, "/api", nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Origin", "https://example.com") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router.ServeHTTP(w, req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // MaxAge written using string(rune(3600)) — produces unicode char not "3600" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // test does not assert the actual header value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ = w.Header().Get("Access-Control-Max-Age") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: This test never asserts the Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+90
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test doesn't verify The test configures Proposed fix: Assert the correct value- // MaxAge written using string(rune(3600)) — produces unicode char not "3600"
- // test does not assert the actual header value
- _ = w.Header().Get("Access-Control-Max-Age")
+ assert.Equal(t, "3600", w.Header().Get("Access-Control-Max-Age"))This assertion will fail, revealing the rune conversion bug in 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,83 @@ | ||||||||||||||||||||||
| // Copyright 2014 Manu Martinez-Almeida. All rights reserved. | ||||||||||||||||||||||
| // Use of this source code is governed by a MIT style | ||||||||||||||||||||||
| // license that can be found in the LICENSE file. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| package gin | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import ( | ||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||
| "io" | ||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||
| "runtime/debug" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // RecoveryJSONConfig defines configuration for the JSONRecovery middleware. | ||||||||||||||||||||||
| type RecoveryJSONConfig struct { | ||||||||||||||||||||||
| // Output is where panic details are logged. | ||||||||||||||||||||||
| // Defaults to gin.DefaultErrorWriter. | ||||||||||||||||||||||
| Output io.Writer | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // IncludeStack includes the stack trace in the JSON response body. | ||||||||||||||||||||||
| // Should be false in production to avoid leaking internals. | ||||||||||||||||||||||
| IncludeStack bool | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // OnPanic is called after recovery with the recovered value. | ||||||||||||||||||||||
| // Optional. | ||||||||||||||||||||||
| OnPanic func(c *Context, err any) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // recoveryJSONResponse is the JSON body returned on panic recovery. | ||||||||||||||||||||||
| type recoveryJSONResponse struct { | ||||||||||||||||||||||
| Error string `json:"error"` | ||||||||||||||||||||||
| Stack string `json:"stack,omitempty"` | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // JSONRecovery returns a HandlerFunc that recovers from panics and writes | ||||||||||||||||||||||
| // a structured JSON 500 response instead of the plain-text default. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // Example: | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // router := gin.New() | ||||||||||||||||||||||
| // router.Use(gin.JSONRecovery()) | ||||||||||||||||||||||
| func JSONRecovery() HandlerFunc { | ||||||||||||||||||||||
| return JSONRecoveryWithConfig(RecoveryJSONConfig{}) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // JSONRecoveryWithConfig returns a HandlerFunc with the given config. | ||||||||||||||||||||||
| func JSONRecoveryWithConfig(cfg RecoveryJSONConfig) HandlerFunc { | ||||||||||||||||||||||
| out := cfg.Output | ||||||||||||||||||||||
| if out == nil { | ||||||||||||||||||||||
| out = DefaultErrorWriter | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return func(c *Context) { | ||||||||||||||||||||||
| defer func() { | ||||||||||||||||||||||
| if rec := recover(); rec != nil { | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Missing broken pipe handling that the existing Prompt for AI agents |
||||||||||||||||||||||
| stack := debug.Stack() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // log to configured writer | ||||||||||||||||||||||
| fmt.Fprintf(out, "[Recovery] panic: %v\n%s\n", rec, stack) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // build response — stack trace conditionally included | ||||||||||||||||||||||
| body := recoveryJSONResponse{ | ||||||||||||||||||||||
| Error: fmt.Sprintf("%v", rec), | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if cfg.IncludeStack { | ||||||||||||||||||||||
| body.Stack = string(stack) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // c.Writer.Written() not checked — if a handler already wrote | ||||||||||||||||||||||
| // a partial response before panicking, calling c.JSON here | ||||||||||||||||||||||
| // writes a second response to an already-started body, | ||||||||||||||||||||||
| // corrupting the HTTP stream silently | ||||||||||||||||||||||
| c.JSON(http.StatusInternalServerError, body) | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Missing Prompt for AI agents |
||||||||||||||||||||||
| c.Abort() | ||||||||||||||||||||||
|
Comment on lines
+69
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check The comment correctly identifies that if a handler already wrote a partial response before panicking, calling Proposed fix- // c.Writer.Written() not checked — if a handler already wrote
- // a partial response before panicking, calling c.JSON here
- // writes a second response to an already-started body,
- // corrupting the HTTP stream silently
- c.JSON(http.StatusInternalServerError, body)
- c.Abort()
+ if !c.Writer.Written() {
+ c.JSON(http.StatusInternalServerError, body)
+ }
+ c.Abort()📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if cfg.OnPanic != nil { | ||||||||||||||||||||||
| cfg.OnPanic(c, rec) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }() | ||||||||||||||||||||||
| c.Next() | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||||||||||||||||||
| // Copyright 2014 Manu Martinez-Almeida. All rights reserved. | ||||||||||||||||||||||
| // Use of this source code is governed by a MIT style | ||||||||||||||||||||||
| // license that can be found in the LICENSE file. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| package gin | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import ( | ||||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||
| "net/http/httptest" | ||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| "github.com/stretchr/testify/assert" | ||||||||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func TestJSONRecovery_Returns500OnPanic(t *testing.T) { | ||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||
| router.Use(JSONRecovery()) | ||||||||||||||||||||||
| router.GET("/panic", func(c *Context) { | ||||||||||||||||||||||
| panic("something went wrong") | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||
| router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/panic", nil)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| assert.Equal(t, http.StatusInternalServerError, w.Code) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var body map[string]any | ||||||||||||||||||||||
| require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) | ||||||||||||||||||||||
| assert.Contains(t, body["error"], "something went wrong") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func TestJSONRecovery_NoPanicPassesThrough(t *testing.T) { | ||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||
| router.Use(JSONRecovery()) | ||||||||||||||||||||||
| router.GET("/ok", func(c *Context) { | ||||||||||||||||||||||
| c.Status(http.StatusOK) | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||
| router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/ok", nil)) | ||||||||||||||||||||||
| assert.Equal(t, http.StatusOK, w.Code) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func TestJSONRecovery_IncludeStack(t *testing.T) { | ||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||
| router.Use(JSONRecoveryWithConfig(RecoveryJSONConfig{IncludeStack: true})) | ||||||||||||||||||||||
| router.GET("/panic", func(c *Context) { | ||||||||||||||||||||||
| panic("stack test") | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| w := httptest.NewRecorder() | ||||||||||||||||||||||
| router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/panic", nil)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var body map[string]any | ||||||||||||||||||||||
| require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) | ||||||||||||||||||||||
| // does not assert stack field is present — IncludeStack behavior untested | ||||||||||||||||||||||
| _ = body | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This test claims to cover Prompt for AI agents
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+56
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test does not verify The test unmarshals the JSON response but discards it without asserting that the Proposed fix var body map[string]any
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
- // does not assert stack field is present — IncludeStack behavior untested
- _ = body
+ assert.Contains(t, body, "stack", "expected stack field when IncludeStack is true")
+ assert.NotEmpty(t, body["stack"], "stack trace should not be empty")📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func TestJSONRecovery_OnPanicCallback(t *testing.T) { | ||||||||||||||||||||||
| var captured any | ||||||||||||||||||||||
| router := New() | ||||||||||||||||||||||
| router.Use(JSONRecoveryWithConfig(RecoveryJSONConfig{ | ||||||||||||||||||||||
| OnPanic: func(c *Context, err any) { | ||||||||||||||||||||||
| captured = err | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| })) | ||||||||||||||||||||||
| router.GET("/panic", func(c *Context) { | ||||||||||||||||||||||
| panic("cb test") | ||||||||||||||||||||||
| }) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/panic", nil)) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // captured is set after c.JSON — but c.JSON already wrote the response. | ||||||||||||||||||||||
| // If the handler had already written headers before panicking, the | ||||||||||||||||||||||
| // double-write behavior is never tested here. | ||||||||||||||||||||||
| if captured == nil { | ||||||||||||||||||||||
| t.Fatal("expected OnPanic to be called") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Security:
strings.Containsfor origin matching allows bypass via substring. An attacker-controlled origin likeevil-example.comwill match an allowed origin ofexample.com. Use exact string equality instead.Prompt for AI agents