Skip to content
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

Path Parameters don't consistently decode #2168

Open
3 tasks done
sazzer opened this issue Apr 30, 2022 · 4 comments
Open
3 tasks done

Path Parameters don't consistently decode #2168

sazzer opened this issue Apr 30, 2022 · 4 comments

Comments

@sazzer
Copy link

sazzer commented Apr 30, 2022

Issue Description

This was noticed as part of #2165. When a handler has path parameters, any URL encoded hex pairs are correctly decoded if they are in uppercase but not in lowercase.

That is:

  • %3F => ?
  • %3f => %3f

It is impossible to safely decode this in the handler if it might or might not have been decoded in the router - or worse, if some of it has been decoded and other bits haven't.

For example, if the handler sees %3f then it's impossible to know if this is:

  • An actual %3f that wasn't decoded.
  • A %253f that was decoded.

Note that RFC-3986 states:

The uppercase hexadecimal digits 'A' through 'F' are equivalent to the lowercase digits 'a' through 'f', respectively.

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Expected behaviour

Path parameters should be URL decoded regardless of the case of the hex pairs.

Actual behaviour

Lowercase hex pairs do not get decoded correctly.

Steps to reproduce

Working code to debug

package main

import (
    "fmt"

    "github.com/labstack/echo/v4"
)

func main() {
    server := echo.New()
    server.Add("GET", "/example/:value", func (c echo.Context) error {
        value := c.Param("value")

        fmt.Printf("Raw value: %s\n", value)

        return c.String(200, value)
    });
    server.Start(":8000");
}

If you call this as /example/ab%3Fde then the output is ab?de. If however you call it as /example/ab%3fde then the output is ab%3fde.

Version/commit

  • Echo version github.com/labstack/echo/v4 v4.7.2
  • Go version 1.18.1
@aldas
Copy link
Contributor

aldas commented Apr 30, 2022

This seems to be more standard library problem as uppercase/lowercase hex escaping is done there. This is where Echo chooses path

echo/echo.go

Lines 902 to 908 in ec92fed

func GetPath(r *http.Request) string {
path := r.URL.RawPath
if path == "" {
path = r.URL.Path
}
return path
}

so when you do request curl -v http://localhost:8000/example/ab%3Fde
this is what Go standard library has parsed into Request.URL structure

image

as you see path has already unescaped ? and URL.RawPath is empty. This seems to be how standard library handles unescaping in URL.Parse()

func TestURL_ParseEscapingHexValues(t *testing.T) {
	var testCases = []struct {
		name          string
		when          string
		expectPath    string
		expectRawPath string
	}{
		{
			name:          "uppercase F in %3f",
			when:          "/example/ab%3Fde",
			expectPath:    "/example/ab?de",
			expectRawPath: `/example/ab%3Fde`, // actual RawPath is empty and test fails
		},
		{
			name:          "lowercase f in %3f",
			when:          "/example/ab%3fde",
			expectPath:    "/example/ab?de",
			expectRawPath: `/example/ab%3fde`,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			u, _ := url.Parse(tc.when)

			if u.Path != tc.expectPath {
				t.Errorf("path `%s` is not `%s`", u.Path, tc.expectPath)
			}
			if u.RawPath != tc.expectRawPath {
				t.Errorf("RawPath `%s` is not `%s`", u.RawPath, tc.expectRawPath)
			}
		})
	}
}

This is where https://github.com/golang/go/blob/fd6c556dc82253722a7f7b9f554a1892b0ede36e/src/net/url/url.go#L676 RawPath gets filled but when your escaped values had upper case values you get same value and therefore RawPath is not filled. But when you provided lower case hex value 3f it would be different as escape creates upper case hex values.

@sazzer
Copy link
Author

sazzer commented Apr 30, 2022

golang/go#33596 looks to be exactly it. Somewhat concerning that it's been untouched for 3 years though, given the potential implications that it has :\

@subnut
Copy link

subnut commented Jul 13, 2022

golang/go#53848 should fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants