[go] net/http: add AllowQuerySemicolons
574 views
Skip to first unread message
Filippo Valsorda (Gerrit)
Jun 9, 2021, 7:45:09 AM6/9/21
to Katie Hockman, Russ Cox, goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Katie Hockman, Russ Cox.
Filippo Valsorda would like Katie Hockman and Russ Cox to review this change.
net/http: add AllowQuerySemicolons
Fixes #45973
Change-Id: I6cbe05f5d1d3c324900c74314b0ea0e12524d7f2
---
M src/net/http/serve_test.go
M src/net/http/server.go
2 files changed, 123 insertions(+), 4 deletions(-)
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index a971468..ed896c9 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -6524,3 +6524,85 @@
t.Errorf("Expected response code %d; got %d", want, got)
}
}
+
+// TestQuerySemicolon tests the behavior of semicolons in queries. See Issue 25192.
+func TestQuerySemicolon(t *testing.T) {
+ t.Cleanup(func() { afterTest(t) })
+
+ tests := []struct {
+ query string
+ xNoSemicolons string
+ xWithSemicolons string
+ warning bool
+ }{
+ {"?a=1;x=bad&x=good", "good", "bad", true},
+ {"?a=1;b=bad&x=good", "good", "good", true},
+ {"?a=1%3Bx=bad&x=good%3B", "good;", "good;", false},
+ {"?a=1;x=good;x=bad", "", "good", true},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.query+"/allow=false", func(t *testing.T) {
+ testQuerySemicolon(t, tt.query, tt.xNoSemicolons, false, tt.warning)
+ })
+ t.Run(tt.query+"/allow=true", func(t *testing.T) {
+ testQuerySemicolon(t, tt.query, tt.xWithSemicolons, true, false)
+ })
+ }
+}
+
+func testQuerySemicolon(t *testing.T, query string, wantX string, allowSemicolons, expectWarning bool) {
+ setParallel(t)
+
+ writeBackX := func(w ResponseWriter, r *Request) {
+ x := r.URL.Query().Get("x")
+ if expectWarning {
+ if err := r.ParseForm(); err == nil || !strings.Contains(err.Error(), "semicolon") {
+ t.Errorf("expected error mentioning semicolons from ParseForm, got %v", err)
+ }
+ } else {
+ if err := r.ParseForm(); err != nil {
+ t.Errorf("expected no error from ParseForm, got %v", err)
+ }
+ }
+ if got := r.FormValue("x"); x != got {
+ t.Errorf("got %q from FormValue, want %q", got, x)
+ }
+ fmt.Fprintf(w, "%s", x)
+ }
+
+ h := Handler(HandlerFunc(writeBackX))
+ if allowSemicolons {
+ h = AllowQuerySemicolons(h)
+ }
+
+ ts := httptest.NewUnstartedServer(h)
+ logBuf := &bytes.Buffer{}
+ ts.Config.ErrorLog = log.New(logBuf, "", 0)
+ ts.Start()
+ defer ts.Close()
+
+ req, _ := NewRequest("GET", ts.URL+query, nil)
+ res, err := ts.Client().Do(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ slurp, _ := io.ReadAll(res.Body)
+ res.Body.Close()
+ if got, want := res.StatusCode, 200; got != want {
+ t.Errorf("Status = %d; want = %d", got, want)
+ }
+ if got, want := string(slurp), wantX; got != want {
+ t.Errorf("Body = %q; want = %q", got, want)
+ }
+
+ if expectWarning {
+ if !strings.Contains(logBuf.String(), "semicolon") {
+ t.Errorf("got %q from ErrorLog, expected a mention of semicolons", logBuf.String())
+ }
+ } else {
+ if strings.Contains(logBuf.String(), "semicolon") {
+ t.Errorf("got %q from ErrorLog, expected no mention of semicolons", logBuf.String())
+ }
+ }
+}
diff --git a/src/net/http/server.go b/src/net/http/server.go
index 72376d0..6c7f47a 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2862,12 +2862,49 @@
if req.RequestURI == "*" && req.Method == "OPTIONS" {
handler = globalOptionsHandler{}
}
- handler.ServeHTTP(rw, req)
+
if req.URL != nil && strings.Contains(req.URL.RawQuery, ";") {
- // TODO(filippo): update this not to log if the special
- // semicolon handler was called.
- sh.srv.logf("http: URL query contains semicolon, which is no longer a supported separator. Parts of the query containing a semicolon may be stripped when being parsed")
+ var allowQuerySemicolonsInUse int32
+ req = req.WithContext(context.WithValue(req.Context(), silenceSemWarnContextKey, func() {
+ atomic.StoreInt32(&allowQuerySemicolonsInUse, 1)
+ }))
+ defer func() {
+ if atomic.LoadInt32(&allowQuerySemicolonsInUse) == 0 {
+ sh.srv.logf("http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192")
+ }
+ }()
}
+
+ handler.ServeHTTP(rw, req)
+}
+
+var silenceSemWarnContextKey = &contextKey{"silence-semicolons"}
+
+// AllowQuerySemicolons returns a handler that serves requests by converting any
+// unescaped semicolons in the URL query to amperstands, and invoking the
+// handler h.
+//
+// This restores the pre-Go 1.17 behavior of splitting query parameters on both
+// semicolons and amperstands. Note that this behavior doesn't match that of
+// many proxies, and the mismatch can lead to security issues.
+//
+// AllowQuerySemicolons should be invoked before Request.ParseForm is called.
+func AllowQuerySemicolons(h Handler) Handler {
+ return HandlerFunc(func(w ResponseWriter, r *Request) {
+ if silenceSemicolonsWarning, ok := r.Context().Value(silenceSemWarnContextKey).(func()); ok {
+ silenceSemicolonsWarning()
+ }
+ if strings.Contains(r.URL.RawQuery, ";") {
+ r2 := new(Request)
+ *r2 = *r
+ r2.URL = new(url.URL)
+ *r2.URL = *r.URL
+ r2.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&")
+ h.ServeHTTP(w, r2)
+ } else {
+ h.ServeHTTP(w, r)
+ }
+ })
}
// ListenAndServe listens on the TCP network address srv.Addr and then
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Katie Hockman (Gerrit)
Jun 9, 2021, 12:33:20 PM6/9/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox, Filippo Valsorda.
Patch set 1:Run-TryBot +1Code-Review +2Trust +1
4 comments:
File src/net/http/serve_test.go:
Patch Set #1, Line 6546:
testQuerySemicolon(t, tt.query, tt.xNoSemicolons, false, tt.warning)For readability, consider
t.Run(tt.query+"/allow=false", func(t *testing.T) {
allowSemicolons := false
testQuerySemicolon(t, tt.query, tt.xNoSemicolons, allowSemicolons, tt.warning)
})
t.Run(tt.query+"/allow=true", func(t *testing.T) {
allowSemicolons, expectWarning := true, false
testQuerySemicolon(t, tt.query, tt.xWithSemicolons, allowSemicolons, expectWarning)
})
File src/net/http/server.go:
Patch Set #1, Line 2884:
amperstandsampersands (here and below)
"...and ampersands (See golang.org/issue/25192). Note..." ?
Patch Set #1, Line 2891:
should"should" or "must" for it to be effective?
Maybe instead: "AllowQuerySemicolons must be invoked before Request.ParseForm or Request.ParseMultipartForm is called in order to be applied"
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Jun 9, 2021, 12:42:04 PM6/9/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Katie Hockman, Go Bot, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: Katie Hockman, Russ Cox.
4 comments:
File src/net/http/serve_test.go:
Patch Set #1, Line 6546:
testQuerySemicolon(t, tt.query, tt.xNoSemicolons, false, tt.warning)
For readability, consider […]
Done
File src/net/http/server.go:
Patch Set #1, Line 2884:
amperstandsampersands (here and below)
Done
"...and ampersands (See golang.org/issue/25192). Note... […]
Done
Patch Set #1, Line 2891:
should
"should" or "must" for it to be effective? […]
Should, because it will still be effective on r.URL.Query, but you might get an avoidable error from ParseForm.
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Jun 9, 2021, 12:42:05 PM6/9/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Katie Hockman, Russ Cox.
Filippo Valsorda uploaded patch set #2 to this change.
net/http: add AllowQuerySemicolons
Fixes #45973
Change-Id: I6cbe05f5d1d3c324900c74314b0ea0e12524d7f2
---
M src/net/http/serve_test.go
M src/net/http/server.go
2 files changed, 125 insertions(+), 4 deletions(-)
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Katie Hockman (Gerrit)
Jun 9, 2021, 12:44:28 PM6/9/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, Go Bot, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox, Filippo Valsorda.
Patch set 2:Code-Review +2Trust +1
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 2891:
shouldShould, because it will still be effective on r.URL. […]
👍
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Filippo Valsorda (Gerrit)
Jun 9, 2021, 12:59:10 PM6/9/21
to Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Katie Hockman, Russ Cox, golang-co...@googlegroups.com
Filippo Valsorda submitted this change.
Approvals:
Katie Hockman: Looks good to me, approved; Trusted
Filippo Valsorda: Trusted; Run TryBots
Go Bot: TryBots succeeded
net/http: add AllowQuerySemicolons
Fixes #45973
Change-Id: I6cbe05f5d1d3c324900c74314b0ea0e12524d7f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/326309
Run-TryBot: Filippo Valsorda <fil...@golang.org>
Reviewed-by: Katie Hockman <ka...@golang.org>
Trust: Katie Hockman <ka...@golang.org>
Trust: Filippo Valsorda <fil...@golang.org>
TryBot-Result: Go Bot <go...@golang.org>
---
M src/net/http/serve_test.go
M src/net/http/server.go
2 files changed, 125 insertions(+), 4 deletions(-)
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index a971468..c2f8811 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -6524,3 +6524,87 @@
t.Errorf("Expected response code %d; got %d", want, got)
}
}
+
+// TestQuerySemicolon tests the behavior of semicolons in queries. See Issue 25192.
+func TestQuerySemicolon(t *testing.T) {
+ t.Cleanup(func() { afterTest(t) })
+
+ tests := []struct {
+ query string
+ xNoSemicolons string
+ xWithSemicolons string
+ warning bool
+ }{
+ {"?a=1;x=bad&x=good", "good", "bad", true},
+ {"?a=1;b=bad&x=good", "good", "good", true},
+ {"?a=1%3Bx=bad&x=good%3B", "good;", "good;", false},
+ {"?a=1;x=good;x=bad", "", "good", true},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.query+"/allow=false", func(t *testing.T) {
+ allowSemicolons := false
+ testQuerySemicolon(t, tt.query, tt.xNoSemicolons, allowSemicolons, tt.warning)
+ })
+ t.Run(tt.query+"/allow=true", func(t *testing.T) {
+ allowSemicolons, expectWarning := true, false
+ testQuerySemicolon(t, tt.query, tt.xWithSemicolons, allowSemicolons, expectWarning)
index 8a1847e..50fab45 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2862,12 +2862,49 @@
if req.RequestURI == "*" && req.Method == "OPTIONS" {
handler = globalOptionsHandler{}
}
- handler.ServeHTTP(rw, req)
+
if req.URL != nil && strings.Contains(req.URL.RawQuery, ";") {
- // TODO(filippo): update this not to log if the special
- // semicolon handler was called.
- sh.srv.logf("http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192")
+ var allowQuerySemicolonsInUse int32
+ req = req.WithContext(context.WithValue(req.Context(), silenceSemWarnContextKey, func() {
+ atomic.StoreInt32(&allowQuerySemicolonsInUse, 1)
+ }))
+ defer func() {
+ if atomic.LoadInt32(&allowQuerySemicolonsInUse) == 0 {
+ sh.srv.logf("http: URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192")
+ }
+ }()
}
+
+ handler.ServeHTTP(rw, req)
+}
+
+var silenceSemWarnContextKey = &contextKey{"silence-semicolons"}
+
+// AllowQuerySemicolons returns a handler that serves requests by converting any
+// unescaped semicolons in the URL query to ampersands, and invoking the handler h.
+//
+// This restores the pre-Go 1.17 behavior of splitting query parameters on both
+// semicolons and ampersands. (See golang.org/issue/25192). Note that this
+// behavior doesn't match that of many proxies, and the mismatch can lead to
+// security issues.
+//
+// AllowQuerySemicolons should be invoked before Request.ParseForm is called.
+func AllowQuerySemicolons(h Handler) Handler {
+ return HandlerFunc(func(w ResponseWriter, r *Request) {
+ if silenceSemicolonsWarning, ok := r.Context().Value(silenceSemWarnContextKey).(func()); ok {
+ silenceSemicolonsWarning()
+ }
+ if strings.Contains(r.URL.RawQuery, ";") {
+ r2 := new(Request)
+ *r2 = *r
+ r2.URL = new(url.URL)
+ *r2.URL = *r.URL
+ r2.URL.RawQuery = strings.ReplaceAll(r.URL.RawQuery, ";", "&")
+ h.ServeHTTP(w, r2)
+ } else {
+ h.ServeHTTP(w, r)
+ }
+ })
}
// ListenAndServe listens on the TCP network address srv.Addr and then
To view, visit change 326309. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages