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.

Sign up for GitHub

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

Jump to bottom

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

Closed
mpbuettner opened this issue Jun 11, 2013 · 15 comments
Closed

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

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

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
This was referenced Dec 8, 2014
Closed
Closed
Closed
@mikioh mikioh mentioned this issue Jan 24, 2015
Closed
@ghost ghost mentioned this issue Feb 7, 2015
Closed
@mna mna mentioned this issue Feb 8, 2015
Closed
richardiux added a commit to wanelo/image-server that referenced this issue Apr 2, 2015
@richardiux
Allow to fetch from URLs that contain allowed reserved delimiters
10134fc 
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
@pegli pegli mentioned this issue Apr 15, 2015
Closed
@rlmcpherson rlmcpherson mentioned this issue May 20, 2015
Closed
@mikioh mikioh mentioned this issue Jun 13, 2015
Closed
This was referenced Aug 5, 2015
Closed
Closed
@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
@ghost ghost mentioned this issue Aug 23, 2015
Open
@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.
Assignees
No one assigned
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
6 participants
@bradfitz @rsc @andybalholm @robpike @gopherbot @mpbuettner