From 64dc1531c6d2b9aba715cfac5f2031f5e44f1a60 Mon Sep 17 00:00:00 2001 From: Geoff Franks Date: Fri, 4 Feb 2022 14:29:35 -0500 Subject: [PATCH] Add warnings when requests come into gorouter with unescaped semicolons (#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 Co-authored-by: Chris Selzo --- handlers/query_param.go | 34 ++++++++++ handlers/query_param_test.go | 116 +++++++++++++++++++++++++++++++++++ proxy/proxy.go | 1 + proxy/proxy_test.go | 24 ++++++++ 4 files changed, 175 insertions(+) create mode 100644 handlers/query_param.go create mode 100644 handlers/query_param_test.go diff --git a/handlers/query_param.go b/handlers/query_param.go new file mode 100644 index 000000000..7452dc197 --- /dev/null +++ b/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") + } +} diff --git a/handlers/query_param_test.go b/handlers/query_param_test.go new file mode 100644 index 000000000..3ab87e381 --- /dev/null +++ b/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¶m2" + handler.ServeHTTP(resp, req) + + Expect(fakeLogger.WarnCallCount()).To(Equal(0)) + Expect(resp.Header().Get(router_http.CfRouterError)).To(Equal("")) + }) + }) + }) + +}) diff --git a/proxy/proxy.go b/proxy/proxy.go index dbd174c83..be9947ba7 100644 --- a/proxy/proxy.go +++ b/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)) diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index ef3c93c71..9f0d4ac11 100644 --- a/proxy/proxy_test.go +++ b/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) {