Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upnet/url: URL allows malformed query round trip #22907
Comments
This comment has been minimized.
This comment has been minimized.
note: |
This comment has been minimized.
This comment has been minimized.
I have investigated the issue and confirmed that this comment is incorrect, or at least the current |
This comment has been minimized.
This comment has been minimized.
@namusyaka I think you are mistaken. The problem is with all forms of Parse which does no validation on the query or fragment pieces. The parse of the uri should have failed. Since it does not, the Fragment or RawQuery fields are incorrect and hence String() which is doing the right thing appears to provide a bad url. All that has to happen is to apply validation of the query and fragment pieces in parse, and that should solve the issues. It might start breaking code that is sending around "bad" URLs. |
This comment has been minimized.
This comment has been minimized.
Its not clear that this can be fixed given the behavior of shouldEscape() which states that most of the sub-delims should escape. |
This comment has been minimized.
This comment has been minimized.
@fraenkel Yes, I knew it's more easy to understand the behavior. |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/99135 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
Copying my comment from the CL: I changed the commit message to accurately reflect what later versions of this CL did. It now says:
But while reviewing this, I'm worried it might change the behavior of: https://golang.org/pkg/net/url/#URL.Query
Before it silently discarded things, and with this CL it would start returning escaped versions instead. Do we care? I think we probably should wait for Go 1.12 at this point. This needs a decision on what to do. |
This comment has been minimized.
This comment has been minimized.
Reopening, as the fix is being reverted. |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/137716 mentions this issue: |
…characters" This reverts commit CL 99135 (git rev 1040626). Reason for revert: breaks valid code; see #27302 Fixes #27302 Updates #22907 Change-Id: I82bb0c28ae1683140c71e7a2224c4ded3f4acea1 Reviewed-on: https://go-review.googlesource.com/137716 Reviewed-by: Ian Lance Taylor <iant@golang.org>
This comment has been minimized.
This comment has been minimized.
@bradfitz - Any thoughts on this ? Or do we want to push to 1.13 ? |
This comment has been minimized.
This comment has been minimized.
I'm kinda done with net/url.URL changes. They're always problematic compatibility-wise. Definitely not for Go 1.12. |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/159157 mentions this issue: |
This is a more conservative version of the reverted CL 99135 (which was reverted in CL 137716) The net/url part rejects URLs with ASCII CTLs from being parsed and the net/http part rejects writing them if a bogus url.URL is constructed otherwise. Updates #27302 Updates #22907 Change-Id: I09a2212eb74c63db575223277aec363c55421ed8 Reviewed-on: https://go-review.googlesource.com/c/159157 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>
This comment has been minimized.
This comment has been minimized.
@gopherbot please open backport issues. This has security implications, and CL 159157 is safe enough to backport. |
This comment has been minimized.
This comment has been minimized.
Backport issue(s) opened: #29922 (for 1.10), #29923 (for 1.11). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/159478 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/160178 mentions this issue: |
CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates #27302 Updates #22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/160678 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/160798 mentions this issue: |
…in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29923 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160798 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
…in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29922 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160678 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
This comment has been minimized.
This comment has been minimized.
Is there anything else to do for this issue? |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/162960 mentions this issue: |
This comment has been minimized.
This comment has been minimized.
Change https://golang.org/cl/162826 mentions this issue: |
Updates #27302 Updates #22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>
…ing ASCII CTLs Updates #27302 Updates #22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit ef454fd) Reviewed-on: https://go-review.googlesource.com/c/162826
CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates golang#27302 Updates golang#22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Updates golang#27302 Updates golang#22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>
CL 159157 was doing UTF-8 decoding of URLs. URLs aren't really UTF-8, even if sometimes they are in some contexts. Instead, only reject ASCII CTLs. Updates golang#27302 Updates golang#22907 Change-Id: Ibd64efa5d3a93263d175aadf1c9f87deb4670c62 Reviewed-on: https://go-review.googlesource.com/c/160178 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Updates golang#27302 Updates golang#22907 Change-Id: Iac6957f3517265dfb9c662efb7af31192e3bfd6c Reviewed-on: https://go-review.googlesource.com/c/162960 Reviewed-by: Ian Lance Taylor <iant@golang.org>
What did you do?
https://play.golang.org/p/hdX1zpv3BN
What did you expect to see?
I expect either url.Parse return a non-nil error or URL.String method return fully escaped url representation —
http://example.com/bad%20path/?bad%20query#bad%20fragment
— with query being escaped the same way as path or fragment.What did you see instead?
For the reference, such url is rejected by net/http.Server: https://play.golang.org/p/2gujmbXZlu
Does this issue reproduce with the latest release (go1.9.2)?
Yes
System details
https://tools.ietf.org/html/rfc3986#section-3.4 states that query component should be defined as (appendix A):
There's no whitespace character in this list. whatwg agrees on that: