URL Encoded Parameters Still Not Working in Go 1.5.1

[derekdowling]

So as a follow up to #77, I'm running Go 1.5.1 which claims to have been the cause of that issue: #77 (comment) and I am still seeing issues with uriEncoded paths.

Example:

    // router build
    router = mux.NewRouter()
    router.HandleFunc("/status", status).Methods("GET")

    apiRouter := mux.NewRouter()
    apiRouter.HandleFunc("/foo", getFooList).Methods("GET")
    apiRouter.HandleFunc("/foo/{bar}", getFoo).Methods("GET")

    router.PathPrefix("/foo").Handler(apiRouter)

    url := httptest.NewServer(router).URL
    url1 := strings.Join([]string{url, "/foo/bar%2ftest"}, "")
    // url2 := strings.Join([]string{url, "/foo/bar.test}", "")

    req, _ := http.NewRequest("GET", url1, nil)

    client := &http.Client{}
    req, err := client.Do(req)
    fmt.Printf("Req: %+v \n Err: %+v \n", req, err)

If I use url1 I get a 404, if I use url2 I get a 200. Thoughts? I am using Ember-Data to generate API calls so short of performing a huge hack on both ends which doesn't seem reasonable.

[garyburd]

The problem is that the mux matches the fully decoded path in req.URL.Path. This issue can be fixed by changing the mux to match req.RequestURI, but that change might break existing applications. RequestURI has been available since Go 1.

A workaround is to change the pattern to:

     apiRouter.HandleFunc("/foo/{bar:.*}", getFoo).Methods("GET")

This pattern matches "/foo/bar%2ftest" and "/foo/bar/test". If you don't want "/foo/bar/test" to match, the modify the handler to check req.RequestURI or the req.URL.RawPath and return status 404 as appropriate.

[derekdowling]

@garyburd thanks for the speedy response and getting me unstuck! Okay, while this isn't exactly ideal, it should at least work. How then would I handle:
apiRouter.HandleFunc("/foo/{bar%2ftest}/{baz}", getFoo).Methods("GET")? I assume that :.* is an inline regex, so do I need to come up with a regex rule to accommodate this?

[vaikas]

I am also running into this (last issue in this thread) and have made the following unit tests and changes to use EscapedPath() instead of Path directly and it now works great for my needs. Is this something that might be considered for a pull request?
master...vaikas-google:master

[elithrar]

Note that using the EscapedPath() method would require us to drop support for all versions of Go prior to 1.5. I don't think we're ready to do that just yet.

The alternative would be to implement our own function that takes a *url.URL and performs the same logic, with the maintenance overhead that goes along with such an approach.

[vaikas]

Thanks much!

Aha, I was not aware of that :) Seems like a solid reason. I can try to
take a crack at implementing a method like that if it seems like a viable
approach?

Also, I currently return the encoded variable, it's probably better to
return the decoded one, but thoughts on that?

On Tue, Nov 24, 2015 at 4:00 PM, Matt Silverlock notifications@github.com
wrote:

Note that using the EscapedPath() method would require us to drop support
for all versions of Go prior to 1.5. I don't think we're ready to do that
just yet.

The alternative would be to implement our own function that takes a
*url.URL and performs the same logic, with the maintenance overhead that
goes along with such an approach.

—
Reply to this email directly or view it on GitHub
#132 (comment).

[duglin]

Would using URL.RawPath instead of URL.Path at this line: https://github.com/gorilla/mux/blob/master/mux.go#L71 be an option? I guess if support for pre-Go 1.5 is required then probably not.

It was mentioned that using RequestURI might break existing apps, but couldn't treating %2F as / be considered a bug and therefore the change in behavior needed/expected?

We're running into a similar issue: moby/moby#16577

[duglin]

Ah, never mind about RawPath - I see that EscapedPath() is the proper way to get the same info.

[duglin]

@elithrar is this now fixed?

[elithrar]

No - I'm open to a PR, but as this has been quiet I'd intepreted the feature wasn't required. I'm not entirely convinced it's a "bug".

The fix is: either break compat for all users pre Go-1.5 (not an option) or add in logic to handle that ourselves.

[duglin]

@vaikas-google were you going to submit a PR?

[chrisdostert]

+1. I'm also in need of this

[chrisdostert]

@elithrar in re: "I'm not entirely convinced it's a bug"
throwing the rfc @ you in hopes of buy in ; )

see rfc3986#section-2.2
"When a URI is dereferenced, the components and subcomponents
significant to the scheme-specific dereferencing process (if any)
must be parsed and separated before the percent-encoded octets within
those components can be safely decoded, as otherwise the data may be
mistaken for component delimiters."

[elithrar]

@chrisdostert Alright—that's all I needed. Are you able to submit a PR? Happy to work with you on it/review, but I don't have time to fix this any time in the near future.

Any fix must be backwards compat.

[hsinhoyeh]

hey guys, do we have any update here? I also hit into this.

[elithrar]

Open to any pull requests to fix this.
On Tue, Jun 7, 2016 at 2:29 AM hsinhoyeh notifications@github.com wrote:

hey guys, do we have any update here? I also hit into this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#132 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AABIcH-wgT0MiEiRwwSIVp9y4qkr5EJeks5qJTnfgaJpZM4GdG7A
.

[kushmansingh]

#184 created, does not break backwards compatibility.

[kushmansingh]

#184 merged. This issue can be closed?

[kushmansingh]

#190 also merged. @elithrar, issue resolved?

[elithrar]

Done.