Add warnings when requests come into gorouter with unescaped semicolo…

…ns (cloudfoundry#308)

Golang 1.17 will no longer accept them when url.Parse() is called.
Gorouter does not look at/modify the request params, however we're
adding warnings to the gorouter log + updating the X-Cf-RouterError
header so app devs + operators will be able to trace back requests
if they have an app performing strangely

Signed-off-by: Geoff Franks <[email protected]>

Co-authored-by: Chris Selzo <[email protected]>

handlers/query_param.go

@@ -0,0 +1,34 @@
+package handlers
+
+import (
+	"net/http"
+	"strings"
+
+	router_http "code.cloudfoundry.org/gorouter/common/http"
+	"code.cloudfoundry.org/gorouter/logger"
+
+	"github.com/uber-go/zap"
+	"github.com/urfave/negroni"
+)
+
+type queryParam struct {
+	logger logger.Logger
+}
+
+// NewQueryParam creates a new handler that emits warnings if requests came in with semicolons un-escaped
+func NewQueryParam(logger logger.Logger) negroni.Handler {
+	return &queryParam{logger: logger}
+}
+
+func (q *queryParam) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
+	semicolonInParams := strings.Contains(r.RequestURI, ";")
+	if semicolonInParams {
+		q.logger.Warn("deprecated-semicolon-params", zap.String("vcap_request_id", r.Header.Get(VcapRequestIdHeader)))
+	}
+
+	next(rw, r)
+
+	if semicolonInParams {
+		rw.Header().Add(router_http.CfRouterError, "deprecated-semicolon-params")
+	}
+}

handlers/query_param_test.go

@@ -0,0 +1,116 @@
+package handlers_test
+
+import (
+	"bytes"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+
+	router_http "code.cloudfoundry.org/gorouter/common/http"
+	"code.cloudfoundry.org/gorouter/common/uuid"
+	"code.cloudfoundry.org/gorouter/handlers"
+	logger_fakes "code.cloudfoundry.org/gorouter/logger/fakes"
+	"code.cloudfoundry.org/gorouter/route"
+	"code.cloudfoundry.org/gorouter/test_util"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/uber-go/zap"
+	"github.com/urfave/negroni"
+)
+
+var _ = Describe("QueryParamHandler", func() {
+	var (
+		handler *negroni.Negroni
+
+		resp http.ResponseWriter
+		req  *http.Request
+
+		fakeLogger *logger_fakes.FakeLogger
+
+		nextCalled bool
+
+		reqChan chan *http.Request
+	)
+	testEndpoint := route.NewEndpoint(&route.EndpointOpts{
+		Host: "host",
+		Port: 1234,
+	})
+
+	nextHandler := negroni.HandlerFunc(func(rw http.ResponseWriter, req *http.Request, next http.HandlerFunc) {
+		_, err := ioutil.ReadAll(req.Body)
+		Expect(err).NotTo(HaveOccurred())
+
+		rw.WriteHeader(http.StatusTeapot)
+		rw.Write([]byte("I'm a little teapot, short and stout."))
+
+		reqInfo, err := handlers.ContextRequestInfo(req)
+		if err == nil {
+			reqInfo.RouteEndpoint = testEndpoint
+		}
+
+		if next != nil {
+			next(rw, req)
+		}
+
+		reqChan <- req
+		nextCalled = true
+	})
+
+	BeforeEach(func() {
+		body := bytes.NewBufferString("What are you?")
+		req = test_util.NewRequest("GET", "example.com", "/", body)
+		resp = httptest.NewRecorder()
+
+		fakeLogger = new(logger_fakes.FakeLogger)
+
+		handler = negroni.New()
+		handler.Use(handlers.NewRequestInfo())
+		handler.Use(handlers.NewProxyWriter(fakeLogger))
+		handler.Use(handlers.NewQueryParam(fakeLogger))
+		handler.Use(nextHandler)
+
+		reqChan = make(chan *http.Request, 1)
+
+		nextCalled = false
+	})
+
+	AfterEach(func() {
+		Expect(nextCalled).To(BeTrue(), "Expected the next handler to be called.")
+		close(reqChan)
+	})
+
+	Context("when checking if semicolons are in the request", func() {
+		var id string
+		var err error
+		BeforeEach(func() {
+			id, err = uuid.GenerateUUID()
+			Expect(err).ToNot(HaveOccurred())
+			req.Header.Add(handlers.VcapRequestIdHeader, id)
+		})
+
+		Context("when semicolons are present", func() {
+			It("logs a warning", func() {
+				req.RequestURI = "/example?param1;param2"
+				handler.ServeHTTP(resp, req)
+
+				Expect(fakeLogger.WarnCallCount()).To(Equal(1))
+				msg, fields := fakeLogger.WarnArgsForCall(0)
+				Expect(msg).To(Equal("deprecated-semicolon-params"))
+				Expect(fields).To(Equal([]zap.Field{zap.String("vcap_request_id", id)}))
+
+				Expect(resp.Header().Get(router_http.CfRouterError)).To(Equal("deprecated-semicolon-params"))
+			})
+		})
+		Context("when semicolons are not present", func() {
+			It("does not log a warning", func() {
+				req.RequestURI = "/example?param1&param2"
+				handler.ServeHTTP(resp, req)
+
+				Expect(fakeLogger.WarnCallCount()).To(Equal(0))
+				Expect(resp.Header().Get(router_http.CfRouterError)).To(Equal(""))
+			})
+		})
+	})
+
+})

proxy/proxy.go

@@ -169,6 +169,7 @@ func NewProxy(
 		n.Use(handlers.NewHTTPStartStop(dropsonde.DefaultEmitter, logger))
 	}
 	n.Use(handlers.NewAccessLog(accessLogger, headersToLog, logger))
+	n.Use(handlers.NewQueryParam(logger))
 	n.Use(handlers.NewReporter(reporter, logger))
 	n.Use(handlers.NewHTTPRewriteHandler(cfg.HTTPRewrite, headersToAlwaysRemove))
 	n.Use(handlers.NewProxyHealthcheck(cfg.HealthCheckUserAgent, p.health, logger))

proxy/proxy_test.go

@@ -32,6 +32,7 @@ import (
 	"github.com/cloudfoundry/sonde-go/events"
 	uuid "github.com/nu7hatch/gouuid"
 	"github.com/openzipkin/zipkin-go/propagation/b3"
+	"github.com/uber-go/zap"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -1211,6 +1212,29 @@ var _ = Describe("Proxy", func() {
 		})
 	})
 
+	Describe("QueryParam", func() {
+		It("logs requests with semicolons in them", func() {
+			ln := test_util.RegisterConnHandler(r, "query-param-test", func(conn *test_util.HttpConn) {
+				_, err := http.ReadRequest(conn.Reader)
+				Expect(err).NotTo(HaveOccurred())
+
+				resp := test_util.NewResponse(http.StatusOK)
+				conn.WriteResponse(resp)
+				conn.Close()
+			})
+			defer ln.Close()
+
+			conn := dialProxy(proxyServer)
+
+			req := test_util.NewRequest("GET", "query-param-test", "/?param1;param2", nil)
+			conn.WriteRequest(req)
+
+			resp, _ := conn.ReadResponse()
+			Expect(resp.StatusCode).To(Equal(http.StatusOK))
+			Expect(testLogger.(*test_util.TestZapLogger).Lines(zap.WarnLevel)).To(ContainElement(ContainSubstring("deprecated-semicolon-params")))
+
+		})
+	})
 	Describe("Access Logging", func() {
 		It("logs a request", func() {
 			ln := test_util.RegisterConnHandler(r, "test", func(conn *test_util.HttpConn) {