Skip to content

Commit 64cfe9f

Browse files
stefanbmvdan
stefanb
authored and
mvdan
committed
net/url: improve url parsing error messages by quoting
Current implementation doesn't always make it obvious what the exact problem with the URL is, so this makes it clearer by consistently quoting the invalid URL, as is the norm in other parsing implementations, eg.: strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax Updates #29261 Change-Id: Icc6bff8b4a4584677c0f769992823e6e1e0d397d GitHub-Last-Rev: 648b9d9 GitHub-Pull-Request: #29384 Reviewed-on: https://go-review.googlesource.com/c/go/+/185117 Reviewed-by: Daniel Martí <[email protected]> Run-TryBot: Daniel Martí <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>
1 parent 5fb74fc commit 64cfe9f

File tree

4 files changed

+17
-11
lines changed

4 files changed

+17
-11
lines changed

‎src/net/http/client_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -221,27 +221,27 @@ func TestClientRedirects(t *testing.T) {
221221

222222
c := ts.Client()
223223
_, err := c.Get(ts.URL)
224-
if e, g := "Get /?n=10: stopped after 10 redirects", fmt.Sprintf("%v", err); e != g {
224+
if e, g := `Get "/?n=10": stopped after 10 redirects`, fmt.Sprintf("%v", err); e != g {
225225
t.Errorf("with default client Get, expected error %q, got %q", e, g)
226226
}
227227

228228
// HEAD request should also have the ability to follow redirects.
229229
_, err = c.Head(ts.URL)
230-
if e, g := "Head /?n=10: stopped after 10 redirects", fmt.Sprintf("%v", err); e != g {
230+
if e, g := `Head "/?n=10": stopped after 10 redirects`, fmt.Sprintf("%v", err); e != g {
231231
t.Errorf("with default client Head, expected error %q, got %q", e, g)
232232
}
233233

234234
// Do should also follow redirects.
235235
greq, _ := NewRequest("GET", ts.URL, nil)
236236
_, err = c.Do(greq)
237-
if e, g := "Get /?n=10: stopped after 10 redirects", fmt.Sprintf("%v", err); e != g {
237+
if e, g := `Get "/?n=10": stopped after 10 redirects`, fmt.Sprintf("%v", err); e != g {
238238
t.Errorf("with default client Do, expected error %q, got %q", e, g)
239239
}
240240

241241
// Requests with an empty Method should also redirect (Issue 12705)
242242
greq.Method = ""
243243
_, err = c.Do(greq)
244-
if e, g := "Get /?n=10: stopped after 10 redirects", fmt.Sprintf("%v", err); e != g {
244+
if e, g := `Get "/?n=10": stopped after 10 redirects`, fmt.Sprintf("%v", err); e != g {
245245
t.Errorf("with default client Do and empty Method, expected error %q, got %q", e, g)
246246
}
247247

@@ -1172,22 +1172,22 @@ func TestStripPasswordFromError(t *testing.T) {
11721172
{
11731173
desc: "Strip password from error message",
11741174
in: "http://user:[email protected]/",
1175-
out: "Get http://user:***@dummy.faketld/: dummy impl",
1175+
out: `Get "http://user:***@dummy.faketld/": dummy impl`,
11761176
},
11771177
{
11781178
desc: "Don't Strip password from domain name",
11791179
in: "http://user:[email protected]/",
1180-
out: "Get http://user:***@password.faketld/: dummy impl",
1180+
out: `Get "http://user:***@password.faketld/": dummy impl`,
11811181
},
11821182
{
11831183
desc: "Don't Strip password from path",
11841184
in: "http://user:[email protected]/password",
1185-
out: "Get http://user:***@dummy.faketld/password: dummy impl",
1185+
out: `Get "http://user:***@dummy.faketld/password": dummy impl`,
11861186
},
11871187
{
11881188
desc: "Strip escaped password",
11891189
in: "http://user:pa%[email protected]/",
1190-
out: "Get http://user:***@dummy.faketld/: dummy impl",
1190+
out: `Get "http://user:***@dummy.faketld/": dummy impl`,
11911191
},
11921192
}
11931193
for _, tC := range testCases {

‎src/net/http/readrequest_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ var reqTests = []reqTest{
133133
nil,
134134
noBodyStr,
135135
noTrailer,
136-
"parse ../../../../etc/passwd: invalid URI for request",
136+
`parse "../../../../etc/passwd": invalid URI for request`,
137137
},
138138

139139
// Tests missing URL:
@@ -143,7 +143,7 @@ var reqTests = []reqTest{
143143
nil,
144144
noBodyStr,
145145
noTrailer,
146-
"parse : empty url",
146+
`parse "": empty url`,
147147
},
148148

149149
// Tests chunked body with trailer:

‎src/net/url/url.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type Error struct {
2626
}
2727

2828
func (e *Error) Unwrap() error { return e.Err }
29-
func (e *Error) Error() string { return e.Op + " " + e.URL + ": " + e.Err.Error() }
29+
func (e *Error) Error() string { return fmt.Sprintf("%s %q: %s", e.Op, e.URL, e.Err) }
3030

3131
func (e *Error) Timeout() bool {
3232
t, ok := e.Err.(interface {

‎src/net/url/url_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,7 @@ var parseRequestURLTests = []struct {
668668

669669
{"foo.html", false},
670670
{"../dir/", false},
671+
{" http://foo.com", false},
671672
{"http://192.168.0.%31/", false},
672673
{"http://192.168.0.%31:8080/", false},
673674
{"http://[fe80::%31]/", false},
@@ -1440,6 +1441,11 @@ func TestParseErrors(t *testing.T) {
14401441
{"mysql://x@y(z:123)/foo", true}, // not well-formed per RFC 3986, golang.org/issue/33646
14411442
{"mysql://x@y(1.2.3.4:123)/foo", true},
14421443

1444+
{" http://foo.com", true}, // invalid character in schema
1445+
{"ht tp://foo.com", true}, // invalid character in schema
1446+
{"ahttp://foo.com", false}, // valid schema characters
1447+
{"1http://foo.com", true}, // invalid character in schema
1448+
14431449
{"http://[]%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a/", true}, // golang.org/issue/11208
14441450
{"http://a b.com/", true}, // no space in host name please
14451451
{"cache_object://foo", true}, // scheme cannot have _, relative path cannot have : in first segment

0 commit comments

Comments
 (0)