Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
106 changes: 106 additions & 0 deletions cors.go
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) {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P0: Security: strings.Contains for origin matching allows bypass via substring. An attacker-controlled origin like evil-example.com will match an allowed origin of example.com. Use exact string equality instead.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cors.go, line 67:

<comment>Security: `strings.Contains` for origin matching allows bypass via substring. An attacker-controlled origin like `evil-example.com` will match an allowed origin of `example.com`. Use exact string equality instead.</comment>

<file context>
@@ -0,0 +1,106 @@
+		allowed := allowAll
+		if !allowed {
+			for _, o := range allowOrigins {
+				if strings.Contains(origin, o) {
+					allowed = true
+					break
</file context>
Suggested change
if strings.Contains(origin, o) {
if origin == o {
Fix with Cubic

allowed = true
break
}
}
}
Comment on lines +65 to +72
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Origin validation uses substring match, allowing bypass.

strings.Contains(origin, o) means an attacker with domain evil-trusted.com can bypass CORS restrictions if trusted.com is in AllowOrigins. This is a serious security vulnerability.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if !allowed {
for _, o := range allowOrigins {
if strings.Contains(origin, o) {
allowed = true
break
}
}
}
if !allowed {
for _, o := range allowOrigins {
if origin == o {
allowed = true
break
}
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cors.go` around lines 65 - 72, The current origin check uses
strings.Contains(origin, o) (variables allowOrigins, origin, allowed) which
allows substring bypasses; replace this with an exact match by parsing the
origin (url.Parse) and comparing either the full origin string with strict
equality (strings.EqualFold) or, better, compare parsedURL.Scheme + "://" +
parsedURL.Host (or parsedURL.Hostname() and port separately) against each
allowOrigins entry to ensure exact host (and optionally scheme/port) matching
instead of substring checks.


if allowed {
if allowAll {
c.Header("Access-Control-Allow-Origin", "*")
} else {
c.Header("Access-Control-Allow-Origin", origin)
}
}

if cfg.AllowCredentials {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P1: The CORS spec forbids Access-Control-Allow-Credentials: true with Access-Control-Allow-Origin: * — browsers will reject the response. The struct comment documents this constraint but the code doesn't enforce it. Either skip setting the credentials header when allowAll is true, or validate the config upfront and return an error/panic.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cors.go, line 82:

<comment>The CORS spec forbids `Access-Control-Allow-Credentials: true` with `Access-Control-Allow-Origin: *` — browsers will reject the response. The struct comment documents this constraint but the code doesn't enforce it. Either skip setting the credentials header when `allowAll` is true, or validate the config upfront and return an error/panic.</comment>

<file context>
@@ -0,0 +1,106 @@
+			}
+		}
+
+		if cfg.AllowCredentials {
+			c.Header("Access-Control-Allow-Credentials", "true")
+		}
</file context>
Fix with Cubic

c.Header("Access-Control-Allow-Credentials", "true")
}
Comment on lines +82 to +84
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

AllowCredentials: true with wildcard origin violates CORS specification.

The comment on line 29 correctly documents that AllowCredentials cannot be used with AllowOrigins: ["*"], but the code doesn't enforce this. Browsers will reject credentials when Access-Control-Allow-Origin: * is combined with Access-Control-Allow-Credentials: true.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if cfg.AllowCredentials {
c.Header("Access-Control-Allow-Credentials", "true")
}
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")
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cors.go` around lines 82 - 84, The middleware currently sets
Access-Control-Allow-Credentials when cfg.AllowCredentials is true
(c.Header("Access-Control-Allow-Credentials", "true")) but does not enforce the
rule that AllowCredentials cannot be used with a wildcard origin. Add a
validation at middleware creation (e.g., in the function that builds the CORS
middleware) that checks cfg.AllowCredentials and whether cfg.AllowOrigins
contains "*" (or a single "*") and return an error (or panic) if both are set;
alternatively, if you prefer runtime-safe behavior, change the header-setting
logic so that when cfg.AllowCredentials is true and cfg.AllowOrigins contains
"*", you do NOT set the credentials header (or instead echo the request Origin
and set a concrete Access-Control-Allow-Origin). Reference cfg.AllowCredentials,
cfg.AllowOrigins, and the c.Header("Access-Control-Allow-Credentials", "true")
site to locate and modify the code.


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))}, ""))
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P0: Bug: string(rune(cfg.MaxAge)) does not produce a numeric string — it interprets the integer as a Unicode codepoint. For MaxAge=3600, this emits the Thai character 'ฐ' (U+0E10) instead of "3600". Use strconv.Itoa(cfg.MaxAge) (and add "strconv" to imports).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cors.go, line 96:

<comment>Bug: `string(rune(cfg.MaxAge))` does not produce a numeric string — it interprets the integer as a Unicode codepoint. For `MaxAge=3600`, this emits the Thai character 'ฐ' (U+0E10) instead of `"3600"`. Use `strconv.Itoa(cfg.MaxAge)` (and add `"strconv"` to imports).</comment>

<file context>
@@ -0,0 +1,106 @@
+			c.Header("Access-Control-Expose-Headers", expose)
+		}
+		if cfg.MaxAge > 0 {
+			c.Header("Access-Control-Max-Age", strings.Join([]string{string(rune(cfg.MaxAge))}, ""))
+		}
+
</file context>
Fix with Cubic

}
Comment on lines +95 to +97
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Bug: MaxAge conversion produces wrong value.

string(rune(cfg.MaxAge)) converts the integer to a Unicode code point, not a numeric string. For MaxAge: 3600, this produces the character 'ม' (U+0E10), not the string "3600".

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
Verify each finding against the current code and only fix it if needed.

In `@cors.go` around lines 95 - 97, The Access-Control-Max-Age header is being set
using string(rune(cfg.MaxAge)), which converts the int to a Unicode code point
rather than a numeric string; replace that conversion by formatting the integer
to a decimal string (e.g., use strconv.Itoa(cfg.MaxAge) or fmt.Sprintf("%d",
cfg.MaxAge)) when calling c.Header("Access-Control-Max-Age", ...), and add the
necessary import for strconv (or fmt) so the header contains the correct numeric
value; references: cfg.MaxAge and the c.Header("Access-Control-Max-Age", ...)
call.


if c.Request.Method == http.MethodOptions {
c.AbortWithStatus(http.StatusNoContent)
return
}

c.Next()
}
}
106 changes: 106 additions & 0 deletions cors_test.go
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")
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

The 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: strings.Contains(origin, o) in cors.go:72 allows https://evil-trusted.com to bypass the origin allowlist because it's a substring match, not an exact match.

Replace the discarded value with an actual assertion, and fix the origin check in cors.go to use exact equality (origin == o) instead of strings.Contains.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cors_test.go, line 71:

<comment>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: `strings.Contains(origin, o)` in `cors.go:72` allows `https://evil-trusted.com` to bypass the origin allowlist because it's a substring match, not an exact match.

Replace the discarded value with an actual assertion, and fix the origin check in `cors.go` to use exact equality (`origin == o`) instead of `strings.Contains`.</comment>

<file context>
@@ -0,0 +1,106 @@
+	router.ServeHTTP(w, req)
+
+	// does not assert header is absent — substring bypass silently passes
+	_ = w.Header().Get("Access-Control-Allow-Origin")
+}
+
</file context>
Fix with Cubic

}
Comment on lines +57 to +72
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Test doesn't catch the substring bypass vulnerability.

The test name suggests evil-trusted.com should not receive CORS headers, but due to the strings.Contains bug in cors.go, it actually does. The test discards the header value (line 71) instead of asserting it's empty.

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 cors.go.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cors_test.go` around lines 57 - 72, Update the test
TestCORS_UnknownOriginGetNoHeader to actually assert that the CORS header is not
returned for a non-allowed origin: after calling router.ServeHTTP(w, req) check
w.Header().Get("Access-Control-Allow-Origin") and assert it is empty (or that
the header is absent) so the substring-bypass is detected; keep the existing
setup (router.Use(CORS(...)), CORSConfig with AllowOrigins
["https://trusted.com"], and Origin "https://evil-trusted.com") and fail the
test if Access-Control-Allow-Origin is present.


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")
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P1: This test never asserts the Access-Control-Max-Age value, hiding a bug in the implementation: string(rune(3600)) converts the int to a Unicode code point (Thai character ), not the string "3600". The implementation should use strconv.Itoa(cfg.MaxAge), and this test should assert the expected value.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cors_test.go, line 105:

<comment>This test never asserts the `Access-Control-Max-Age` value, hiding a bug in the implementation: `string(rune(3600))` converts the int to a Unicode code point (Thai character `ฐ`), not the string `"3600"`. The implementation should use `strconv.Itoa(cfg.MaxAge)`, and this test should assert the expected value.</comment>

<file context>
@@ -0,0 +1,106 @@
+
+	// 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")
+}
</file context>
Suggested change
_ = w.Header().Get("Access-Control-Max-Age")
assert.Equal(t, "3600", w.Header().Get("Access-Control-Max-Age"))
Fix with Cubic

}
Comment on lines +90 to +106
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Test doesn't verify MaxAge header value.

The test configures MaxAge: 3600 but discards the header value without assertion. This misses the bug where string(rune(3600)) produces an incorrect value.

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 cors.go.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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")
}
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)
assert.Equal(t, "3600", w.Header().Get("Access-Control-Max-Age"))
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cors_test.go` around lines 90 - 106, The test TestCORS_MaxAge currently reads
but does not assert the Access-Control-Max-Age header, so add an assertion that
w.Header().Get("Access-Control-Max-Age") equals "3600" to catch the bug; update
the test that builds a router using CORS(CORSConfig{MaxAge: 3600}) to verify the
exact string "3600" is returned, which will fail and point to the incorrect
conversion in cors.go where MaxAge is turned into string(rune(MaxAge)) instead
of strconv.Itoa or fmt.Sprintf.

83 changes: 83 additions & 0 deletions recoveryjson.go
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 {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P2: Missing broken pipe handling that the existing Recovery middleware provides. When the panic value wraps EPIPE, ECONNRESET, or ErrAbortHandler, the connection is dead and c.JSON(…) will fail. Check for broken pipe before writing, mirroring recovery.go lines 63–84.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At recoveryjson.go, line 55:

<comment>Missing broken pipe handling that the existing `Recovery` middleware provides. When the panic value wraps `EPIPE`, `ECONNRESET`, or `ErrAbortHandler`, the connection is dead and `c.JSON(…)` will fail. Check for broken pipe before writing, mirroring `recovery.go` lines 63–84.</comment>

<file context>
@@ -0,0 +1,83 @@
+
+	return func(c *Context) {
+		defer func() {
+			if rec := recover(); rec != nil {
+				stack := debug.Stack()
+
</file context>
Fix with Cubic

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)
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P1: Missing c.Writer.Written() guard before writing JSON response. 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. The built-in recovery.go handles this; this middleware should too.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At recoveryjson.go, line 73:

<comment>**Missing `c.Writer.Written()` guard before writing JSON response.** 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. The built-in `recovery.go` handles this; this middleware should too.</comment>

<file context>
@@ -0,0 +1,83 @@
+				// 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()
+
</file context>
Fix with Cubic

c.Abort()
Comment on lines +69 to +74
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Check c.Writer.Written() before writing the JSON response.

The comment correctly identifies that if a handler already wrote a partial response before panicking, calling c.JSON corrupts the HTTP stream. Unlike the existing recovery.go which handles broken pipes gracefully, this middleware unconditionally writes.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 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()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@recoveryjson.go` around lines 69 - 74, Before calling c.JSON in the recovery
middleware, check c.Writer.Written() and only write the JSON response (and call
c.Abort()) if nothing has been written yet; if c.Writer.Written() is true, skip
c.JSON to avoid corrupting the HTTP stream. Locate the recovery handler in
recoveryjson.go where c.JSON(http.StatusInternalServerError, body) is called and
wrap that call with a conditional that inspects c.Writer.Written(), using the
existing body variable and keeping the broken-pipe handling intact.


if cfg.OnPanic != nil {
cfg.OnPanic(c, rec)
}
}
}()
c.Next()
}
}
82 changes: 82 additions & 0 deletions recoveryjson_test.go
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
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

P2: This test claims to cover IncludeStack but never asserts the "stack" key is present in the response body — the unmarshaled body is discarded with _ = body. Add an assertion so the feature is actually exercised.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At recoveryjson_test.go, line 59:

<comment>This test claims to cover `IncludeStack` but never asserts the `"stack"` key is present in the response body — the unmarshaled `body` is discarded with `_ = body`. Add an assertion so the feature is actually exercised.</comment>

<file context>
@@ -0,0 +1,82 @@
+	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
+}
+
</file context>
Suggested change
_ = body
assert.NotEmpty(t, body["stack"], "expected stack trace in response when IncludeStack is true")
Fix with Cubic

}
Comment on lines +56 to +60
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Test does not verify IncludeStack behavior.

The test unmarshals the JSON response but discards it without asserting that the stack field is present. This means the IncludeStack: true configuration is not actually tested.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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
}
var body map[string]any
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body))
assert.Contains(t, body, "stack", "expected stack field when IncludeStack is true")
assert.NotEmpty(t, body["stack"], "stack trace should not be empty")
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@recoveryjson_test.go` around lines 56 - 60, The test currently unmarshals the
response into the variable body but never asserts the IncludeStack behavior;
update the test to assert that body contains a "stack" key when IncludeStack:
true (e.g., check that body["stack"] exists and is a non-empty string or array
as expected). Locate the json.Unmarshal(w.Body.Bytes(), &body) call and add
assertions (using require or assert) that the "stack" field is present and of
the expected type/value to verify IncludeStack=true is effective.


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")
}
}
Loading