go-chi / chi

lightweight, idiomatic and composable router for building Go HTTP services
https://go-chi.io
MIT License
18.51k stars 988 forks source link

URLParam sometimes returns URL-encoded variables #832

Open cespedes opened 1 year ago

cespedes commented 1 year ago

Hello!

As the title says, when I try to get a URL parameter from a http.Request object (calling chi.URLParam), it sometimes returns a URL-encoded result, and it only depends on whether r.URL.RawPath is set or not.

This is because in function *Mux.routeHTTP(), it checks if r.URL.RawPath is set and uses it instead of r.URL.Path; however, r.URL.RawPath is only present sometimes:

https://github.com/go-chi/chi/blob/7f280968675bcc9f310008fc6b8abff0b923734c/mux.go#L420-L431

IMHO, the best course of action will be to use url.EscapedPath() to get the routePath and then url.PathUnescape() to set every value.

Simple program to show the bug:

package main

import (
        "fmt"
        "net/http"

        "github.com/go-chi/chi/v5"
)

func main() {
        r := chi.NewRouter()
        r.Get("/{key}", func(w http.ResponseWriter, r *http.Request) {
                key := chi.URLParam(r, "key")
                fmt.Printf("Path=%q RawPath=%q key=%q\n", r.URL.Path, r.URL.RawPath, key)
                fmt.Fprintf(w, "Path=%q RawPath=%q key=%q\n", r.URL.Path, r.URL.RawPath, key)
        })
        http.ListenAndServe(":3333", r)
}

Simple execution to see the problem:

$ curl "http://localhost:3333/It%20is%20great"
Path="/It is great" RawPath="" key="It is great"
$ curl "http://localhost:3333/It's%20great"
Path="/It's great" RawPath="/It's%20great" key="It's%20great"

If it is OK for you, I will open a Pull Request.

Thank you very much,

Juan

prm-dan commented 7 months ago

+1. I'm hitting this same issue too. The logic seems inconsistent.

http://localhost:5150/v1/content/{contentID}

This URL

http://localhost:5150/v1/content/testid%1FABC

extracts contentId as

contentID=testid\u001fABC

But this URL

http://localhost:5150/v1/content/testid%3BABC

extracts contentId as

contentID=testid%3BABC
brasic commented 1 month ago

We hit this bug. Because the data only sometimes needs unescaping depending on whatever inside go sets the URL.RawPath value in net/http, it's not safe to unconditionally apply url.PathUnescape(), since that would not correctly handle certain edge cases involving double-url-escaped strings, like http://example.com/my%2520data (with an escaped percent), which application logic should see as my%20data, but a naive approach would inappropriately yield my data. I'm using this unfortunate workaround:

func SafeURLParam(r *http.Request, key string) string {
    rctx := chi.RouteContext(r.Context())
    if rctx == nil {
        return ""
    }
    raw := rctx.URLParam(key)
    // Only apply unescaping if chi built the param map using the raw path.
    // Corresponds to the logic at
    // https://github.com/go-chi/chi/blob/v5.1.0/mux.go#L433-L437
    if r.URL.RawPath == "" {
        return raw
    }
    unescaped, err := url.PathUnescape(raw)
    if err != nil {
        slog.Warn("bad URL escape", "error", err, "param", raw)
        return ""
    }
    return unescaped
}
BladeMcCool commented 1 month ago

same as issue 641 and issue 642 right? still no fix after 3 years? yeah i know ... i should submit a PR - well i just switched to using query params instead.