Skip to content

avoid double unescaping #4447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AgNess-G wants to merge 9 commits into
base: master
Choose a base branch
from
Open

avoid double unescaping #4447

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e2184ff
avoid double unescaping
Agnes-George Nov 21, 2025
67c0ef7
Merge branch 'master' into resolvePRISMA
appleboy Nov 23, 2025
cf959d7
worked on review comments
Agnes-George Nov 24, 2025
d26a652
formatted
Agnes-George Nov 24, 2025
a61c9c6
edit gin.yml to checkout, setup go, cache
Agnes-George Nov 24, 2025
9b3d7f6
improve code coverage
Agnes-George Nov 24, 2025
a9b0085
formatted
Agnes-George Nov 24, 2025
1c407a2
revert gin.yml edit
AgNess-G Dec 1, 2025
f8b6b4a
Revert "edit gin.yml to checkout, setup go, cache"
Agnes-George Dec 1, 2025
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
43 changes: 26 additions & 17 deletions tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package gin

import (
"math"
"bytes"
"net/url"
"strings"
"unicode"
Expand All @@ -14,6 +14,12 @@ import (
"github.com/gin-gonic/gin/internal/bytesconv"
)

var (
strColon = []byte(":")
strStar = []byte("*")
strSlash = []byte("/")
)

// Param is a single URL parameter, consisting of a key and a value.
type Param struct {
Key string
Expand Down Expand Up @@ -78,22 +84,17 @@ func (n *node) addChild(child *node) {
}
}

// safeUint16 converts int to uint16 safely, capping at math.MaxUint16
func safeUint16(n int) uint16 {
if n > math.MaxUint16 {
return math.MaxUint16
}
return uint16(n)
}

func countParams(path string) uint16 {
colons := strings.Count(path, ":")
stars := strings.Count(path, "*")
return safeUint16(colons + stars)
var n uint16
s := bytesconv.StringToBytes(path)
n += uint16(bytes.Count(s, strColon))
n += uint16(bytes.Count(s, strStar))
return n
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The removal of safeUint16 safety check could cause overflow issues. The old implementation capped values at math.MaxUint16 (65535), but the new code directly casts to uint16 without bounds checking.

If bytes.Count() returns a value greater than 65535, the cast uint16(bytes.Count(...)) will silently overflow. While unlikely for typical path strings, extremely long paths with many colons or slashes could potentially cause this.

Consider either:

  1. Keeping the safeUint16 helper function, or
  2. Adding overflow protection in countParams and countSections

Example problematic case:

path := strings.Repeat("/:param", 70000)  // 70000 > 65535
n := uint16(bytes.Count(s, strColon))  // Overflows, wraps around

Copilot uses AI. Check for mistakes.
}

func countSections(path string) uint16 {
return safeUint16(strings.Count(path, "/"))
s := bytesconv.StringToBytes(path)
return uint16(bytes.Count(s, strSlash))
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Similar to countParams, this function is now missing overflow protection that was previously provided by safeUint16.

If a path has more than 65535 slashes, the cast to uint16 will silently overflow, potentially causing incorrect behavior in the routing logic.

Suggested change
return uint16(bytes.Count(s, strSlash))
count := bytes.Count(s, strSlash)
if count > 0xFFFF {
count = 0xFFFF
}
return uint16(count)

Copilot uses AI. Check for mistakes.
}

type nodeType uint8
Expand Down Expand Up @@ -520,8 +521,12 @@ walk: // Outer loop for walking the tree
*value.params = (*value.params)[:i+1]
val := path[:end]
if unescape {
if v, err := url.QueryUnescape(val); err == nil {
val = v
// Only unescape if the value contains percent-encoded characters or plus signs
// This prevents double unescaping and potential path traversal
if strings.ContainsAny(val, "%+") {
if v, err := url.QueryUnescape(val); err == nil {
val = v
}
}
}
(*value.params)[i] = Param{
Expand Down Expand Up @@ -573,8 +578,12 @@ walk: // Outer loop for walking the tree
*value.params = (*value.params)[:i+1]
val := path
if unescape {
if v, err := url.QueryUnescape(path); err == nil {
val = v
// Only unescape if the value contains percent-encoded characters or plus signs
// This prevents double unescaping and potential path traversal
if strings.ContainsAny(path, "%+") {
if v, err := url.QueryUnescape(path); err == nil {
val = v
}
}
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The same unescape logic is duplicated in two places (param and catchAll cases). Consider extracting this into a helper function to reduce code duplication and make maintenance easier.

Suggested refactoring:

func unescapeValue(val string, unescape bool) string {
	if unescape && strings.ContainsAny(val, "%+") {
		if v, err := url.QueryUnescape(val); err == nil {
			return v
		}
	}
	return val
}

Then use it in both locations:

val := unescapeValue(path[:end], unescape)

Copilot uses AI. Check for mistakes.
}
(*value.params)[i] = Param{
Expand Down
Loading
Loading