echo icon indicating copy to clipboard operation
echo copied to clipboard

Path Parameters don't consistently decode

Open sazzer opened this issue 3 years ago • 4 comments

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

  • [x] Dependencies installed
  • [x] No typos
  • [x] 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

sazzer avatar Apr 30 '22 18:04 sazzer

This seems to be more standard library problem as uppercase/lowercase hex escaping is done there. This is where Echo chooses path https://github.com/labstack/echo/blob/ec92fedf21e817d2d52004a4178292404beb9eaa/echo.go#L902-L908

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.

aldas avatar Apr 30 '22 18:04 aldas

Somewhat related issues

  • https://github.com/labstack/echo/issues/1974
  • https://github.com/labstack/echo/issues/1777

Also

  • https://github.com/golang/go/issues/48854
  • https://github.com/golang/go/issues/33596

aldas avatar Apr 30 '22 19:04 aldas

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

sazzer avatar Apr 30 '22 20:04 sazzer

golang/go#53848 should fix this issue.

subnut avatar Jul 13 '22 18:07 subnut