From db5e5daf4334b2c9b5341cfcb3bbd1535b923c18 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Tue, 25 Feb 2020 16:54:04 +0100 Subject: [PATCH] feat: Limit reading bytes from request bodies (#168) This change both limits the number of bytes that the SDK attempts to read and buffer in memory, as well as delays as much work as possible. It requires breaking changes to the exported API. Notes on API changes: - Request.FromHTTPRequest + NewRequest The difference is that NewRequest does not read the request body. FromHTTPRequest was called from HTTP middleware in integrations in the beginning of the request handling flow. Attempting to read the whole body there is often wasteful, since most requests should not result in reporting an error to Sentry. With this change, Scope.SetRequest keeps a lazy buffer that clones a limited portion of the request body only if it is read by the user. - Scope.SetRequest(Request) + Scope.SetRequest(*http.Request) For most integrations (fasthttp being the only exception), this means less work needs to be done when handling a request. Converting from http.Request to sentry.Request is only done when there is an error to be reported. It also means we can keep a reference to the request body without reading all bytes upfront. - Event.Request type Request + Event.Request type *Request Avoids copying of possibly large struct. --- echo/sentryecho.go | 2 +- fasthttp/sentryfasthttp.go | 37 ++++----- fasthttp/sentryfasthttp_test.go | 63 ++++++++++++++- gin/sentrygin.go | 2 +- http/sentryhttp.go | 4 +- http/sentryhttp_test.go | 64 ++++++++++++++- interfaces.go | 61 +++++++-------- interfaces_test.go | 45 ++++++----- iris/sentryiris.go | 2 +- martini/sentrymartini.go | 2 +- negroni/sentrynegroni.go | 4 +- scope.go | 135 ++++++++++++++++++++++++++++---- scope_concurrency_test.go | 3 +- scope_test.go | 49 +++++++----- transport_test.go | 4 +- 15 files changed, 353 insertions(+), 124 deletions(-) diff --git a/echo/sentryecho.go b/echo/sentryecho.go index b4ff7578c..a38521b43 100644 --- a/echo/sentryecho.go +++ b/echo/sentryecho.go @@ -56,7 +56,7 @@ func New(options Options) echo.MiddlewareFunc { func (h *handler) handle(next echo.HandlerFunc) echo.HandlerFunc { return func(ctx echo.Context) error { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(ctx.Request())) + hub.Scope().SetRequest(ctx.Request()) ctx.Set(valuesKey, hub) defer h.recoverWithSentry(hub, ctx.Request()) return next(ctx) diff --git a/fasthttp/sentryfasthttp.go b/fasthttp/sentryfasthttp.go index 39bd8cd46..8255e33eb 100644 --- a/fasthttp/sentryfasthttp.go +++ b/fasthttp/sentryfasthttp.go @@ -1,10 +1,12 @@ package sentryfasthttp import ( + "bytes" "context" "fmt" - "net" - "strings" + "io/ioutil" + "net/http" + "net/url" "time" "github.com/getsentry/sentry-go" @@ -62,7 +64,9 @@ func New(options Options) *Handler { func (h *Handler) Handle(handler fasthttp.RequestHandler) fasthttp.RequestHandler { return func(ctx *fasthttp.RequestCtx) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(extractRequestData(ctx)) + scope := hub.Scope() + scope.SetRequest(convert(ctx)) + scope.SetRequestBody(ctx.Request.Body()) ctx.SetUserValue(valuesKey, hub) defer h.recoverWithSentry(hub, ctx) handler(ctx) @@ -93,44 +97,41 @@ func GetHubFromContext(ctx *fasthttp.RequestCtx) *sentry.Hub { return nil } -func extractRequestData(ctx *fasthttp.RequestCtx) sentry.Request { +func convert(ctx *fasthttp.RequestCtx) *http.Request { defer func() { if err := recover(); err != nil { sentry.Logger.Printf("%v", err) } }() - r := sentry.Request{} + r := new(http.Request) r.Method = string(ctx.Method()) uri := ctx.URI() - r.URL = fmt.Sprintf("%s://%s%s", uri.Scheme(), uri.Host(), uri.Path()) + // Ignore error. + r.URL, _ = url.Parse(fmt.Sprintf("%s://%s%s", uri.Scheme(), uri.Host(), uri.Path())) // Headers - headers := make(map[string]string) + r.Header = make(http.Header) + r.Header.Add("Host", string(ctx.Host())) ctx.Request.Header.VisitAll(func(key, value []byte) { - headers[string(key)] = string(value) + r.Header.Add(string(key), string(value)) }) - headers["Host"] = string(ctx.Host()) - r.Headers = headers + r.Host = string(ctx.Host()) // Cookies - cookies := []string{} ctx.Request.Header.VisitAllCookie(func(key, value []byte) { - cookies = append(cookies, fmt.Sprintf("%s=%s", key, value)) + r.AddCookie(&http.Cookie{Name: string(key), Value: string(value)}) }) - r.Cookies = strings.Join(cookies, "; ") // Env - if addr, port, err := net.SplitHostPort(ctx.RemoteAddr().String()); err == nil { - r.Env = map[string]string{"REMOTE_ADDR": addr, "REMOTE_PORT": port} - } + r.RemoteAddr = ctx.RemoteAddr().String() // QueryString - r.QueryString = string(ctx.URI().QueryString()) + r.URL.RawQuery = string(ctx.URI().QueryString()) // Body - r.Data = string(ctx.Request.Body()) + r.Body = ioutil.NopCloser(bytes.NewReader(ctx.Request.Body())) return r } diff --git a/fasthttp/sentryfasthttp_test.go b/fasthttp/sentryfasthttp_test.go index 462018eed..d8752af29 100644 --- a/fasthttp/sentryfasthttp_test.go +++ b/fasthttp/sentryfasthttp_test.go @@ -1,8 +1,10 @@ package sentryfasthttp_test import ( + "fmt" "net" "net/http" + "strings" "testing" "time" @@ -15,6 +17,8 @@ import ( ) func TestIntegration(t *testing.T) { + largePayload := strings.Repeat("Large", 3*1024) // 15 KB + tests := []struct { Path string Method string @@ -32,7 +36,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelFatal, Message: "test", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "http://example.com/panic", Method: "GET", Headers: map[string]string{ @@ -55,7 +59,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelInfo, Message: "post: payload", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "http://example.com/post", Method: "POST", Data: "payload", @@ -78,7 +82,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelInfo, Message: "get", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "http://example.com/get", Method: "GET", Headers: map[string]string{ @@ -89,6 +93,59 @@ func TestIntegration(t *testing.T) { }, }, }, + { + Path: "/post/large", + Method: "POST", + Body: largePayload, + Handler: func(ctx *fasthttp.RequestCtx) { + hub := sentryfasthttp.GetHubFromContext(ctx) + hub.CaptureMessage(fmt.Sprintf("post: %d KB", len(ctx.Request.Body())/1024)) + }, + + WantEvent: &sentry.Event{ + Level: sentry.LevelInfo, + Message: "post: 15 KB", + Request: &sentry.Request{ + URL: "http://example.com/post/large", + Method: "POST", + // Actual request body omitted because too large. + Data: "", + Headers: map[string]string{ + "Content-Length": "15360", + "Content-Type": "application/x-www-form-urlencoded", + "Host": "example.com", + "User-Agent": "fasthttp", + }, + }, + }, + }, + { + Path: "/post/body-ignored", + Method: "POST", + Body: "client sends, fasthttp always reads, SDK reports", + Handler: func(ctx *fasthttp.RequestCtx) { + hub := sentryfasthttp.GetHubFromContext(ctx) + hub.CaptureMessage("body ignored") + }, + + WantEvent: &sentry.Event{ + Level: sentry.LevelInfo, + Message: "body ignored", + Request: &sentry.Request{ + URL: "http://example.com/post/body-ignored", + Method: "POST", + // Actual request body included because fasthttp always + // reads full request body. + Data: "client sends, fasthttp always reads, SDK reports", + Headers: map[string]string{ + "Content-Length": "48", + "Content-Type": "application/x-www-form-urlencoded", + "Host": "example.com", + "User-Agent": "fasthttp", + }, + }, + }, + }, } eventsCh := make(chan *sentry.Event, len(tests)) diff --git a/gin/sentrygin.go b/gin/sentrygin.go index f1d3eb3a2..f6a23a338 100644 --- a/gin/sentrygin.go +++ b/gin/sentrygin.go @@ -58,7 +58,7 @@ func New(options Options) gin.HandlerFunc { func (h *handler) handle(ctx *gin.Context) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(ctx.Request)) + hub.Scope().SetRequest(ctx.Request) ctx.Set(valuesKey, hub) defer h.recoverWithSentry(hub, ctx.Request) ctx.Next() diff --git a/http/sentryhttp.go b/http/sentryhttp.go index 617d83c1e..b429413f4 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -52,7 +52,7 @@ func New(options Options) *Handler { func (h *Handler) Handle(handler http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(r)) + hub.Scope().SetRequest(r) ctx := sentry.SetHubOnContext( r.Context(), hub, @@ -66,7 +66,7 @@ func (h *Handler) Handle(handler http.Handler) http.Handler { func (h *Handler) HandleFunc(handler http.HandlerFunc) http.HandlerFunc { return func(rw http.ResponseWriter, r *http.Request) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(r)) + hub.Scope().SetRequest(r) ctx := sentry.SetHubOnContext( r.Context(), hub, diff --git a/http/sentryhttp_test.go b/http/sentryhttp_test.go index d0e2ef9fa..307c1fbef 100644 --- a/http/sentryhttp_test.go +++ b/http/sentryhttp_test.go @@ -1,6 +1,7 @@ package sentryhttp_test import ( + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -15,6 +16,8 @@ import ( ) func TestIntegration(t *testing.T) { + largePayload := strings.Repeat("Large", 3*1024) // 15 KB + tests := []struct { Path string Method string @@ -32,7 +35,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelFatal, Message: "test", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "/panic", Method: "GET", Headers: map[string]string{ @@ -58,7 +61,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelInfo, Message: "post: payload", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "/post", Method: "POST", Data: "payload", @@ -80,7 +83,7 @@ func TestIntegration(t *testing.T) { WantEvent: &sentry.Event{ Level: sentry.LevelInfo, Message: "get", - Request: sentry.Request{ + Request: &sentry.Request{ URL: "/get", Method: "GET", Headers: map[string]string{ @@ -90,6 +93,60 @@ func TestIntegration(t *testing.T) { }, }, }, + { + Path: "/post/large", + Method: "POST", + Body: largePayload, + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hub := sentry.GetHubFromContext(r.Context()) + body, err := ioutil.ReadAll(r.Body) + if err != nil { + t.Error(err) + } + hub.CaptureMessage(fmt.Sprintf("post: %d KB", len(body)/1024)) + }), + + WantEvent: &sentry.Event{ + Level: sentry.LevelInfo, + Message: "post: 15 KB", + Request: &sentry.Request{ + URL: "/post/large", + Method: "POST", + // Actual request body omitted because too large. + Data: "", + Headers: map[string]string{ + "Accept-Encoding": "gzip", + "Content-Length": "15360", + "User-Agent": "Go-http-client/1.1", + }, + }, + }, + }, + { + Path: "/post/body-ignored", + Method: "POST", + Body: "client sends, server ignores, SDK doesn't read", + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hub := sentry.GetHubFromContext(r.Context()) + hub.CaptureMessage("body ignored") + }), + + WantEvent: &sentry.Event{ + Level: sentry.LevelInfo, + Message: "body ignored", + Request: &sentry.Request{ + URL: "/post/body-ignored", + Method: "POST", + // Actual request body omitted because not read. + Data: "", + Headers: map[string]string{ + "Accept-Encoding": "gzip", + "Content-Length": "46", + "User-Agent": "Go-http-client/1.1", + }, + }, + }, + }, } eventsCh := make(chan *sentry.Event, len(tests)) @@ -124,7 +181,6 @@ func TestIntegration(t *testing.T) { wantRequest := tt.WantEvent.Request wantRequest.URL = srv.URL + wantRequest.URL wantRequest.Headers["Host"] = srv.Listener.Addr().String() - tt.WantEvent.Request = wantRequest want = append(want, tt.WantEvent) req, err := http.NewRequest(tt.Method, srv.URL+tt.Path, strings.NewReader(tt.Body)) diff --git a/interfaces.go b/interfaces.go index 15c0d007b..01cb03158 100644 --- a/interfaces.go +++ b/interfaces.go @@ -1,10 +1,8 @@ package sentry import ( - "bytes" "context" "fmt" - "io/ioutil" "net" "net/http" "strings" @@ -71,47 +69,42 @@ type Request struct { Env map[string]string `json:"env,omitempty"` } -func (r Request) FromHTTPRequest(request *http.Request) Request { - // Method - r.Method = request.Method - - // URL +// NewRequest returns a new Sentry Request from the given http.Request. +// +// NewRequest avoids operations that depend on network access. In particular, it +// does not read r.Body. +func NewRequest(r *http.Request) *Request { protocol := schemeHTTP - if request.TLS != nil || request.Header.Get("X-Forwarded-Proto") == "https" { + if r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" { protocol = schemeHTTPS } - r.URL = fmt.Sprintf("%s://%s%s", protocol, request.Host, request.URL.Path) + url := fmt.Sprintf("%s://%s%s", protocol, r.Host, r.URL.Path) + + // We read only the first Cookie header because of the specification: + // https://tools.ietf.org/html/rfc6265#section-5.4 + // When the user agent generates an HTTP request, the user agent MUST NOT + // attach more than one Cookie header field. + cookies := r.Header.Get("Cookie") - // Headers - headers := make(map[string]string, len(request.Header)) - for k, v := range request.Header { + headers := make(map[string]string, len(r.Header)) + for k, v := range r.Header { headers[k] = strings.Join(v, ",") } - headers["Host"] = request.Host - r.Headers = headers - - // Cookies - r.Cookies = request.Header.Get("Cookie") + headers["Host"] = r.Host - // Env - if addr, port, err := net.SplitHostPort(request.RemoteAddr); err == nil { - r.Env = map[string]string{"REMOTE_ADDR": addr, "REMOTE_PORT": port} + var env map[string]string + if addr, port, err := net.SplitHostPort(r.RemoteAddr); err == nil { + env = map[string]string{"REMOTE_ADDR": addr, "REMOTE_PORT": port} } - // QueryString - r.QueryString = request.URL.RawQuery - - // Body - if request.Body != nil { - bodyBytes, err := ioutil.ReadAll(request.Body) - _ = request.Body.Close() - if err == nil { - // We have to restore original state of *request.Body - request.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) - r.Data = string(bodyBytes) - } + return &Request{ + URL: url, + Method: r.Method, + QueryString: r.URL.RawQuery, + Cookies: cookies, + Headers: headers, + Env: env, } - return r } // https://docs.sentry.io/development/sdk-dev/event-payloads/exception/ @@ -147,7 +140,7 @@ type Event struct { User User `json:"user,omitempty"` Logger string `json:"logger,omitempty"` Modules map[string]string `json:"modules,omitempty"` - Request Request `json:"request,omitempty"` + Request *Request `json:"request,omitempty"` Exception []Exception `json:"exception,omitempty"` } diff --git a/interfaces_test.go b/interfaces_test.go index 976bb3a62..ad4508ddd 100644 --- a/interfaces_test.go +++ b/interfaces_test.go @@ -1,28 +1,31 @@ package sentry import ( - "bytes" - "io/ioutil" - "net/http" + "net/http/httptest" + "strings" "testing" -) - -func TestRequestFromHTTPRequest(t *testing.T) { - var testPayload = `{"test_data": true}` - - t.Run("reading_body", func(t *testing.T) { - payload := bytes.NewBufferString(testPayload) - req, err := http.NewRequest("POST", "/test/", payload) - assertEqual(t, err, nil) - assertNotEqual(t, req, nil) - sentryRequest := Request{} - sentryRequest = sentryRequest.FromHTTPRequest(req) - assertEqual(t, sentryRequest.Data, testPayload) + "github.com/google/go-cmp/cmp" +) - // Re-reading original *http.Request.Body - reqBody, err := ioutil.ReadAll(req.Body) - assertEqual(t, err, nil) - assertEqual(t, string(reqBody), testPayload) - }) +func TestNewRequest(t *testing.T) { + const payload = `{"test_data": true}` + got := NewRequest(httptest.NewRequest("POST", "/test/?q=sentry", strings.NewReader(payload))) + want := &Request{ + URL: "http://example.com/test/", + Method: "POST", + Data: "", + QueryString: "q=sentry", + Cookies: "", + Headers: map[string]string{ + "Host": "example.com", + }, + Env: map[string]string{ + "REMOTE_ADDR": "192.0.2.1", + "REMOTE_PORT": "1234", + }, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Request mismatch (-want +got):\n%s", diff) + } } diff --git a/iris/sentryiris.go b/iris/sentryiris.go index 16210464d..06c5c1dda 100644 --- a/iris/sentryiris.go +++ b/iris/sentryiris.go @@ -57,7 +57,7 @@ func New(options Options) iris.Handler { func (h *handler) handle(ctx iris.Context) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(ctx.Request())) + hub.Scope().SetRequest(ctx.Request()) ctx.Values().Set(valuesKey, hub) defer h.recoverWithSentry(hub, ctx.Request()) ctx.Next() diff --git a/martini/sentrymartini.go b/martini/sentrymartini.go index 0232a24d2..d090739c3 100644 --- a/martini/sentrymartini.go +++ b/martini/sentrymartini.go @@ -53,7 +53,7 @@ func New(options Options) martini.Handler { func (h *handler) handle(rw http.ResponseWriter, r *http.Request, ctx martini.Context) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(r)) + hub.Scope().SetRequest(r) ctx.Map(hub) defer h.recoverWithSentry(hub, r) ctx.Next() diff --git a/negroni/sentrynegroni.go b/negroni/sentrynegroni.go index 78f7730aa..ab286cbe8 100644 --- a/negroni/sentrynegroni.go +++ b/negroni/sentrynegroni.go @@ -53,7 +53,7 @@ func New(options Options) negroni.Handler { func (h *handler) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) { hub := sentry.CurrentHub().Clone() - hub.Scope().SetRequest(sentry.Request{}.FromHTTPRequest(r)) + hub.Scope().SetRequest(r) ctx := sentry.SetHubOnContext( context.WithValue(r.Context(), sentry.RequestContextKey, r), hub, @@ -82,7 +82,7 @@ func (h *handler) recoverWithSentry(hub *sentry.Hub, r *http.Request) { func PanicHandlerFunc(info *negroni.PanicInformation) { hub := sentry.CurrentHub().Clone() hub.WithScope(func(scope *sentry.Scope) { - scope.SetRequest(sentry.Request{}.FromHTTPRequest(info.Request)) + scope.SetRequest(info.Request) hub.RecoverWithContext( context.WithValue(context.Background(), sentry.RequestContextKey, info.Request), info.RecoveredPanic, diff --git a/scope.go b/scope.go index f0ed3e146..aa582af20 100644 --- a/scope.go +++ b/scope.go @@ -1,6 +1,9 @@ package sentry import ( + "bytes" + "io" + "net/http" "reflect" "sync" "time" @@ -22,16 +25,25 @@ import ( // Note that the scope can only be modified but not inspected. // Only the client can use the scope to extract information currently. type Scope struct { - mu sync.RWMutex - breadcrumbs []*Breadcrumb - user User - tags map[string]string - contexts map[string]interface{} - extra map[string]interface{} - fingerprint []string - level Level - transaction string - request Request + mu sync.RWMutex + breadcrumbs []*Breadcrumb + user User + tags map[string]string + contexts map[string]interface{} + extra map[string]interface{} + fingerprint []string + level Level + transaction string + request *http.Request + // requestBody holds a reference to the original request.Body. + requestBody interface { + // Bytes returns bytes from the original body, lazily buffered as the + // original body is read. + Bytes() []byte + // Overflow returns true if the body is larger than the maximum buffer + // size. + Overflow() bool + } eventProcessors []EventProcessor } @@ -82,11 +94,94 @@ func (scope *Scope) SetUser(user User) { } // SetRequest sets the request for the current scope. -func (scope *Scope) SetRequest(request Request) { +func (scope *Scope) SetRequest(r *http.Request) { scope.mu.Lock() defer scope.mu.Unlock() - scope.request = request + scope.request = r + + if r == nil { + return + } + + // Don't buffer request body if we know it is oversized. + if r.ContentLength > maxRequestBodyBytes { + return + } + // Don't buffer if there is no body. + if r.Body == nil || r.Body == http.NoBody { + return + } + buf := &limitedBuffer{Capacity: maxRequestBodyBytes} + r.Body = readCloser{ + Reader: io.TeeReader(r.Body, buf), + Closer: r.Body, + } + scope.requestBody = buf +} + +// SetRequestBody sets the request body for the current scope. +// +// This method should only be called when the body bytes are already available +// in memory. Typically, the request body is buffered lazily from the +// Request.Body from SetRequest. +func (scope *Scope) SetRequestBody(b []byte) { + scope.mu.Lock() + defer scope.mu.Unlock() + + capacity := maxRequestBodyBytes + overflow := false + if len(b) > capacity { + overflow = true + b = b[:capacity] + } + scope.requestBody = &limitedBuffer{ + Capacity: capacity, + Buffer: *bytes.NewBuffer(b), + overflow: overflow, + } +} + +// maxRequestBodyBytes is the default maximum request body size to send to +// Sentry. +const maxRequestBodyBytes = 10 * 1024 + +// A limitedBuffer is like a bytes.Buffer, but limited to store at most Capacity +// bytes. Any writes past the capacity are silently discarded, similar to +// ioutil.Discard. +type limitedBuffer struct { + Capacity int + + bytes.Buffer + overflow bool +} + +// Write implements io.Writer. +func (b *limitedBuffer) Write(p []byte) (n int, err error) { + // Silently ignore writes after overflow. + if b.overflow { + return len(p), nil + } + left := b.Capacity - b.Len() + if left < 0 { + left = 0 + } + if len(p) > left { + b.overflow = true + p = p[:left] + } + return b.Buffer.Write(p) +} + +// Overflow returns true if the limitedBuffer discarded bytes written to it. +func (b *limitedBuffer) Overflow() bool { + return b.overflow +} + +// readCloser combines an io.Reader and an io.Closer to implement io.ReadCloser. +type readCloser struct { + io.Reader + io.Closer } // SetTag adds a tag to the current scope. @@ -292,8 +387,20 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event { event.Transaction = scope.transaction } - if (reflect.DeepEqual(event.Request, Request{})) { - event.Request = scope.request + if event.Request == nil && scope.request != nil { + event.Request = NewRequest(scope.request) + // NOTE: The SDK does not attempt to send partial request body data. + // + // The reason being that Sentry's ingest pipeline and UI are optimized + // to show structured data. Additionally, tooling around PII scrubbing + // relies on structured data; truncated request bodies would create + // invalid payloads that are more prone to leaking PII data. + // + // Users can still send more data along their events if they want to, + // for example using Event.Extra. + if scope.requestBody != nil && !scope.requestBody.Overflow() { + event.Request.Data = string(scope.requestBody.Bytes()) + } } for _, processor := range scope.eventProcessors { diff --git a/scope_concurrency_test.go b/scope_concurrency_test.go index 190269a21..1840d983c 100644 --- a/scope_concurrency_test.go +++ b/scope_concurrency_test.go @@ -2,6 +2,7 @@ package sentry_test import ( "errors" + "net/http/httptest" "sync" "testing" @@ -55,7 +56,7 @@ func touchScope(scope *sentry.Scope, x int) { scope.SetFingerprint([]string{"foo"}) scope.AddBreadcrumb(&sentry.Breadcrumb{Timestamp: 1337, Message: "foo"}, 100) scope.SetUser(sentry.User{ID: "foo"}) - scope.SetRequest(sentry.Request{URL: "foo"}) + scope.SetRequest(httptest.NewRequest("GET", "/foo", nil)) sentry.CaptureException(errors.New(string(x))) diff --git a/scope_test.go b/scope_test.go index 5bc096981..56ebaaa81 100644 --- a/scope_test.go +++ b/scope_test.go @@ -1,6 +1,8 @@ package sentry import ( + "net/http" + "net/http/httptest" "testing" "time" ) @@ -14,7 +16,7 @@ func fillScopeWithData(scope *Scope) *Scope { scope.fingerprint = []string{"scopeFingerprintOne", "scopeFingerprintTwo"} scope.level = LevelDebug scope.transaction = "wat" - scope.request = Request{URL: "wat"} + scope.request = httptest.NewRequest("GET", "/wat", nil) return scope } @@ -27,7 +29,7 @@ func fillEventWithData(event *Event) *Event { event.Fingerprint = []string{"eventFingerprintOne", "eventFingerprintTwo"} event.Level = LevelInfo event.Transaction = "aye" - event.Request = Request{URL: "aye"} + event.Request = &Request{URL: "aye"} return event } @@ -46,17 +48,21 @@ func TestScopeSetUserOverrides(t *testing.T) { } func TestScopeSetRequest(t *testing.T) { + r := httptest.NewRequest("GET", "/foo", nil) scope := NewScope() - scope.SetRequest(Request{URL: "foo"}) - assertEqual(t, Request{URL: "foo"}, scope.request) + scope.SetRequest(r) + + assertEqual(t, r, scope.request) } func TestScopeSetRequestOverrides(t *testing.T) { + r1 := httptest.NewRequest("GET", "/foo", nil) + r2 := httptest.NewRequest("GET", "/bar", nil) scope := NewScope() - scope.SetRequest(Request{URL: "foo"}) - scope.SetRequest(Request{URL: "bar"}) + scope.SetRequest(r1) + scope.SetRequest(r2) - assertEqual(t, Request{URL: "bar"}, scope.request) + assertEqual(t, r2, scope.request) } func TestScopeSetTag(t *testing.T) { @@ -376,7 +382,8 @@ func TestScopeParentChangedInheritance(t *testing.T) { clone.SetFingerprint([]string{"foo"}) clone.AddBreadcrumb(&Breadcrumb{Timestamp: 1337, Message: "foo"}, maxBreadcrumbs) clone.SetUser(User{ID: "foo"}) - clone.SetRequest(Request{URL: "foo"}) + r1 := httptest.NewRequest("GET", "/foo", nil) + clone.SetRequest(r1) scope.SetTag("foo", "baz") scope.SetContext("foo", "baz") @@ -386,7 +393,8 @@ func TestScopeParentChangedInheritance(t *testing.T) { scope.SetFingerprint([]string{"bar"}) scope.AddBreadcrumb(&Breadcrumb{Timestamp: 1337, Message: "bar"}, maxBreadcrumbs) scope.SetUser(User{ID: "bar"}) - scope.SetRequest(Request{URL: "bar"}) + r2 := httptest.NewRequest("GET", "/bar", nil) + scope.SetRequest(r2) assertEqual(t, map[string]string{"foo": "bar"}, clone.tags) assertEqual(t, map[string]interface{}{"foo": "bar"}, clone.contexts) @@ -396,7 +404,7 @@ func TestScopeParentChangedInheritance(t *testing.T) { assertEqual(t, []string{"foo"}, clone.fingerprint) assertEqual(t, []*Breadcrumb{{Timestamp: 1337, Message: "foo"}}, clone.breadcrumbs) assertEqual(t, User{ID: "foo"}, clone.user) - assertEqual(t, Request{URL: "foo"}, clone.request) + assertEqual(t, r1, clone.request) assertEqual(t, map[string]string{"foo": "baz"}, scope.tags) assertEqual(t, map[string]interface{}{"foo": "baz"}, scope.contexts) @@ -406,7 +414,7 @@ func TestScopeParentChangedInheritance(t *testing.T) { assertEqual(t, []string{"bar"}, scope.fingerprint) assertEqual(t, []*Breadcrumb{{Timestamp: 1337, Message: "bar"}}, scope.breadcrumbs) assertEqual(t, User{ID: "bar"}, scope.user) - assertEqual(t, Request{URL: "bar"}, scope.request) + assertEqual(t, r2, scope.request) } func TestScopeChildOverrideInheritance(t *testing.T) { @@ -420,7 +428,8 @@ func TestScopeChildOverrideInheritance(t *testing.T) { scope.SetFingerprint([]string{"bar"}) scope.AddBreadcrumb(&Breadcrumb{Timestamp: 1337, Message: "bar"}, maxBreadcrumbs) scope.SetUser(User{ID: "bar"}) - scope.SetRequest(Request{URL: "bar"}) + r1 := httptest.NewRequest("GET", "/bar", nil) + scope.SetRequest(r1) clone := scope.Clone() clone.SetTag("foo", "bar") @@ -431,7 +440,8 @@ func TestScopeChildOverrideInheritance(t *testing.T) { clone.SetFingerprint([]string{"foo"}) clone.AddBreadcrumb(&Breadcrumb{Timestamp: 1337, Message: "foo"}, maxBreadcrumbs) clone.SetUser(User{ID: "foo"}) - clone.SetRequest(Request{URL: "foo"}) + r2 := httptest.NewRequest("GET", "/foo", nil) + clone.SetRequest(r2) assertEqual(t, map[string]string{"foo": "bar"}, clone.tags) assertEqual(t, map[string]interface{}{"foo": "bar"}, clone.contexts) @@ -444,7 +454,7 @@ func TestScopeChildOverrideInheritance(t *testing.T) { {Timestamp: 1337, Message: "foo"}, }, clone.breadcrumbs) assertEqual(t, User{ID: "foo"}, clone.user) - assertEqual(t, Request{URL: "foo"}, clone.request) + assertEqual(t, r2, clone.request) assertEqual(t, map[string]string{"foo": "baz"}, scope.tags) assertEqual(t, map[string]interface{}{"foo": "baz"}, scope.contexts) @@ -454,7 +464,7 @@ func TestScopeChildOverrideInheritance(t *testing.T) { assertEqual(t, []string{"bar"}, scope.fingerprint) assertEqual(t, []*Breadcrumb{{Timestamp: 1337, Message: "bar"}}, scope.breadcrumbs) assertEqual(t, User{ID: "bar"}, scope.user) - assertEqual(t, Request{URL: "bar"}, scope.request) + assertEqual(t, r1, scope.request) } func TestClear(t *testing.T) { @@ -469,7 +479,7 @@ func TestClear(t *testing.T) { assertEqual(t, []string{}, scope.fingerprint) assertEqual(t, Level(""), scope.level) assertEqual(t, "", scope.transaction) - assertEqual(t, Request{}, scope.request) + assertEqual(t, (*http.Request)(nil), scope.request) } func TestClearAndReconfigure(t *testing.T) { @@ -484,7 +494,8 @@ func TestClearAndReconfigure(t *testing.T) { scope.SetFingerprint([]string{"foo"}) scope.AddBreadcrumb(&Breadcrumb{Timestamp: 1337, Message: "foo"}, maxBreadcrumbs) scope.SetUser(User{ID: "foo"}) - scope.SetRequest(Request{URL: "foo"}) + r := httptest.NewRequest("GET", "/foo", nil) + scope.SetRequest(r) assertEqual(t, map[string]string{"foo": "bar"}, scope.tags) assertEqual(t, map[string]interface{}{"foo": "bar"}, scope.contexts) @@ -494,7 +505,7 @@ func TestClearAndReconfigure(t *testing.T) { assertEqual(t, []string{"foo"}, scope.fingerprint) assertEqual(t, []*Breadcrumb{{Timestamp: 1337, Message: "foo"}}, scope.breadcrumbs) assertEqual(t, User{ID: "foo"}, scope.user) - assertEqual(t, Request{URL: "foo"}, scope.request) + assertEqual(t, r, scope.request) } func TestClearBreadcrumbs(t *testing.T) { @@ -552,7 +563,7 @@ func TestApplyToEventUsingEmptyEvent(t *testing.T) { assertEqual(t, processedEvent.Fingerprint, scope.fingerprint, "should use scope fingerprint") assertEqual(t, processedEvent.Level, scope.level, "should use scope level") assertEqual(t, processedEvent.Transaction, scope.transaction, "should use scope transaction") - assertEqual(t, processedEvent.Request, scope.request, "should use scope request") + assertEqual(t, processedEvent.Request, NewRequest(scope.request), "should use scope request") } func TestEventProcessorsModifiesEvent(t *testing.T) { diff --git a/transport_test.go b/transport_test.go index 545423280..fe260e89e 100644 --- a/transport_test.go +++ b/transport_test.go @@ -15,10 +15,10 @@ type unserializableType struct { UnsupportedField func() } -const basicEvent = "{\"message\":\"mkey\",\"sdk\":{},\"user\":{},\"request\":{}}" +const basicEvent = "{\"message\":\"mkey\",\"sdk\":{},\"user\":{}}" const enhancedEvent = "{\"extra\":{\"info\":\"Original event couldn't be marshalled. Succeeded by stripping " + "the data that uses interface{} type. Please verify that the data you attach to the scope is serializable.\"}," + - "\"message\":\"mkey\",\"sdk\":{},\"user\":{},\"request\":{}}" + "\"message\":\"mkey\",\"sdk\":{},\"user\":{}}" func TestGetRequestBodyFromEventValid(t *testing.T) { body := getRequestBodyFromEvent(&Event{