]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: add AllowQuerySemicolons
authorFilippo Valsorda <filippo@golang.org>
Wed, 9 Jun 2021 11:43:57 +0000 (07:43 -0400)
committerFilippo Valsorda <filippo@golang.org>
Wed, 9 Jun 2021 16:59:02 +0000 (16:59 +0000)
Fixes #45973

Change-Id: I6cbe05f5d1d3c324900c74314b0ea0e12524d7f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/326309
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Katie Hockman <katie@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>

src/net/http/serve_test.go
src/net/http/server.go

index a9714682c7ebb56bc05222798214923aef47d760..c2f881146975f61bafb8c3c4ee74a2f5776120d5 100644 (file)
@@ -6524,3 +6524,87 @@ func TestMuxRedirectRelative(t *testing.T) {
                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)
+               })
+       }
+}
+
+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())
+               }
+       }
+}
index 8a1847e67a59a5f13e04894052ba56f63821ad1e..50fab4520dd47c67a0a1fc99a9c623f4a236dde8 100644 (file)
@@ -2862,12 +2862,49 @@ func (sh serverHandler) ServeHTTP(rw ResponseWriter, req *Request) {
        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