danielgtaylor / huma

Huma REST/HTTP API Framework for Golang with OpenAPI 3.1
https://huma.rocks/
MIT License
1.87k stars 138 forks source link

fix: Add support for Percent-Encoding RFC 3986 #411

Closed chilic closed 4 months ago

chilic commented 4 months ago

Problem: The current implementation does not allow to use of encoded named parameters like: /path/:param - /path-params/a%3A%2F%2Fb%2Fc

Which is fully legal by RFC 3986

Solution: Use EscapedPath() instead of direct access to a u.Path

Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/. A consequence is that it is impossible to tell which slashes in the Path were slashes in the raw URL and which were %2f. This distinction is rarely important, but when it is, the code should use the URL.EscapedPath method, which preserves the original encoding of Path.

Details: https://pkg.go.dev/net/url#URL

PR into the original package: https://github.com/alexedwards/flow/pull/11

danielgtaylor commented 4 months ago

@chilic I would assume most users want the decoded param value. What's the use-case for the raw value?

Unless I'm missing something, Go 1.22 behaves the same way (automatically decoding the values), see https://go.dev/play/p/rlvkKwpgs_I for example.

chilic commented 4 months ago

@danielgtaylor that is interesting. The use case is to pass parameters with symbols that should be encoded, like HEX value or encoded URL. In Go 1.22 routing works correctly. Example: https://go.dev/play/p/Pv0xuSEKPth

But the flow uses decoded value to slit URL parts:

urlSegments := strings.Split(r.URL.Path, "/")

Which is return 404 error because URL became /demo/a/b/c instead of /demo/a%2Fb%2Fc. I mean it's okay to encode param, but it's a mistake to use encoded param to build routing.

P.S. chi example: https://go.dev/play/p/W3mc2lHq2ez (200 code) flow example: https://go.dev/play/p/rYwsgOX58by (404 code)

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.65%. Comparing base (dcfa3e9) to head (32f0c2d). Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #411 +/- ## ======================================= Coverage 92.65% 92.65% ======================================= Files 21 21 Lines 3539 3539 ======================================= Hits 3279 3279 Misses 221 221 Partials 39 39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielgtaylor commented 4 months ago

@chilic so sorry I misunderstood what this was about! You are right, the routing is incorrect! However, when getting a path param value it should be decoded rather than in its raw encoded state, so I've added a commit for that to ensure param values are decoded before storing them. Let me know what you think and I can merge it in if everything looks okay!

chilic commented 4 months ago

I agree with you. Better to keep the behavior the same as the HTTP package. Thank you!