Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mpbuettner opened this issue Jun 11, 2013 · 15 comments
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@mpbuettner
Copy link

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-
@robpike
Copy link
Contributor

robpike commented Jun 12, 2013

Comment 1:

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

Owner changed to @bradfitz.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 2:

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

Labels changed: added go1.2maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 3:

Labels changed: added feature.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 8, 2013

Comment 4:

Labels changed: added suggested.

@robpike
Copy link
Contributor

robpike commented Sep 3, 2013

Comment 5:

Not in time for 1.2.

Labels changed: removed go1.2maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 6:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 7:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 8:

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

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 9:

Labels changed: added repo-main.

@bradfitz
Copy link
Contributor

Comment 10:

Issue #6784 has been merged into this issue.

@andybalholm
Copy link
Contributor

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.

@bradfitz
Copy link
Contributor

bradfitz commented May 1, 2014

Comment 12:

Issue #7914 has been merged into this issue.

@bradfitz
Copy link
Contributor

bradfitz commented May 1, 2014

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 ---.

@gopherbot
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

Comment 15:

Issue #7975 has been merged into this issue.

@mpbuettner mpbuettner added accepted Suggested Issues that may be good for new contributors looking for work to do. labels May 12, 2014
richardiux added a commit to wanelo/image-server that referenced this issue Apr 2, 2015
Go has a bug where it does not include all allowed reserved sub-delimiters
As a workaround, a http.Get is extended to keep the paths un-escaped.

It is expected that URL are previously escaped, so there should be no 
negative side effect

For more information on bug: golang/go#5684
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed accepted labels Apr 14, 2015
@rsc rsc changed the title net/url: shouldEscape does not check all reserved characters net/url: url.Parse("example.com/oid/[order_id]") escapes [ ] in String() method Aug 6, 2015
@rsc rsc modified the milestones: Go1.5, Unplanned Aug 6, 2015
@rsc rsc closed this as completed in fced03a Aug 6, 2015
@golang golang locked and limited conversation to collaborators Aug 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

6 participants