ziglang
std.Uri: Don't double-escape escaped query parameters
Zig's HTTP client breaks down when redirected to a URL with escaped query parameters. It seems to be doubly escaping them; this PR simply exempts the % character from being escaped in query parameters. I'm not sure if this will have unanticipated effects; we could also try a more complicated fix by looking ahead to see if the % in question is part of an escape sequence (and escaping it only if it's not).
I added a unit test. I'm also not 100% confident that this is the correct place to fix the problem — maybe the HTTP client shouldn't be re-escaping URLs provided in redirects in the first place? But this does fix the problem.
Based on RFC3986 I think percent encoded characters are allowed in more places than just the query string - it seems like user info, host, path, query, fragment can all contains % some way or another that would be vulnerable to this double escaping. I think its probably better to move this condition to isPathChar to cover the double escaping in queries and paths.
Comparing this PR to mine (#16533):
- I forgot Github doesn't search open PRs when searching issues, so I didn't see this one when I searched for issues around std.Uri and decided to open my own
- I think this PR is a more minimal change to the standard library that unblocks using the built in HTTP client with a bunch of common request patterns. However, I think it's odd behavior for std.Uri, since when given an unescaped query string,
writeEscapedQueryStringcan't be correct in all cases. - My PR punts the whole problem to the caller, but doesn't provide a built-in solution for them to escape a set of K/V pairs, because I was shooting for a small change and I'm not sure if there's a process for proposing larger stdlib changes.
My suggestion is that we merge this PR now, because it makes it possible to use the HTTP library with non-alphanumeric query strings without changing any API signatures. At some point, std.Uri probably needs a more consistent escaping story, but that could be a larger PR that addresses fields besides query, as noted in https://github.com/ziglang/zig/pull/16043#issuecomment-1593911082, and optionally adds something like https://github.com/nektro/zig-UrlValues.
