net/url: url.Parse("example.com/oid/[order_id]") escapes [ ] in String() method

Question

net/url: url.Parse("example.com/oid/[order_id]") escapes [ ] in String() method

mpbuettner opened this issue 12 years ago · 15 comments

RFC 3986 specifies that reserved characters in URL paths should not be escaped, and
shouldEscape() is designed to enforce that.

The RFC specifies the following set as reserved:
gen-delims:  ":", "/", "?", "#", "[",
"]", "@"
sub-delims: "!", "$", "&", "'",
"(", ")", "*", "+", ",",
";", "="

shouldEscape() only checks the following subset, and escapes the rest:
'$', '&', '+', ',', '/', ':', ';', '=', '?', '@'

When proxying, "http://example.com/oid/[order_id]", the "[" and
"]" get escaped, which is a bug. Servers may well handle
".../oid/[order_id]" and ".../oid/%5Border_id%5D" differently. 

Example below:
http://play.golang.org/p/Vczuon9WR-

Answer 1 · 2013-06-12T04:10:14.000Z

Comment 1:

Labels changed: added priority-soon, packagechange, removed priority-triage.

Owner changed to @bradfitz.

Status changed to Accepted.

Answer 2 · 2013-07-30T16:08:02.000Z

Comment 2:

I am sure this will break something. If you prepare a CL, look at the code history.

Labels changed: added go1.2maybe.

Answer 3 · 2013-07-30T22:41:08.000Z

Comment 3:

Labels changed: added feature.

Answer 4 · 2013-08-08T20:02:28.000Z

Comment 4:

Labels changed: added suggested.

Answer 5 · 2013-09-03T23:35:15.000Z

Comment 5:

Not in time for 1.2.

Labels changed: removed go1.2maybe.

Answer 6 · 2013-11-27T18:45:41.000Z

Comment 6:

Labels changed: added go1.3maybe.

Answer 7 · 2013-11-27T20:29:16.000Z

Comment 7:

Labels changed: removed feature.

Answer 8 · 2013-12-04T01:29:04.000Z

Comment 8:

Labels changed: added release-none, removed go1.3maybe.

Answer 9 · 2013-12-04T01:48:38.000Z

Comment 9:

Labels changed: added repo-main.

Answer 10 · 2014-01-15T20:55:57.000Z

Comment 10:

Issue #6784 has been merged into this issue.

Answer 11 · 2014-01-22T22:45:54.000Z

Comment 11:

Russ Cox pointed out that a workaround is documented at http://godoc.org/net/url#URL, in
the paragraph starting with "Note that the path field…"
That worked for me, once I realized that if the request is going through a proxy, the
host should be included in the Opaque field, but if it isn't, the Opaque field should be
just the path.

Answer 12 · 2014-05-01T16:16:54.000Z

Comment 12:

Issue #7914 has been merged into this issue.

Answer 13 · 2014-05-01T16:18:17.000Z

Comment 13:

Any code change (trivial) would have to be accompanied by convincing research that this
won't break anything (non-trivial).

Labels changed: removed priority-soon.

Owner changed to ---.

Answer 14 · 2014-05-01T19:30:33.000Z

Comment 14 by aaron.blohowiak:

What kind of shape would convincing research take? Here are my thoughts.
1. Evaluate how other proxies (mod_proxy, nginx proxy_pass, haproxy) handle the
sub-delims in each of path, userinfo and password.
2. Evaluate how url.RequestURI is being used in the stdlib, identifying any places that
might be impacted negatively by this change and a plan to test / evaluate the impact.
I will also review the history of the CL as per rsc in #2

Answer 15 · 2014-05-12T17:23:47.000Z

Comment 15:

Issue #7975 has been merged into this issue.