Compare commits
1 Commits
5ff7cdbdb5
...
fb5a203896
| Author | SHA1 | Date | |
|---|---|---|---|
| fb5a203896 |
101
README.md
101
README.md
@@ -1037,38 +1037,77 @@ reduces the headers to a fixed allowlist — `Accept`, `Content-Length`,
|
|||||||
`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and
|
`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and
|
||||||
`X-Request-Id`.
|
`X-Request-Id`.
|
||||||
|
|
||||||
The body is replaced on every route rather than filtered by route, and
|
The same hook rewrites the request URL. The SDK builds it as
|
||||||
that is a choice rather than a limitation: the route is reachable from
|
`scheme://host/path` from the concrete path, which on the receiver
|
||||||
the hook. `sentryhttp`'s recover path puts the request on the context
|
route is `/webhook/<uuid>` in full — and that UUID is a write
|
||||||
it hands to `RecoverWithContext`, and the SDK carries that context
|
capability, not an identifier: anyone holding it can post events this
|
||||||
through to `BeforeSend` as `hint.Context`, so
|
service accepts and its targets then deliver. A tracker has its own
|
||||||
`hint.Context.Value(sentry.RequestContextKey)` yields the live request
|
retention, access control and deletion policy, so the rule the access
|
||||||
and chi's `RoutePattern()` yields the matched pattern off it. There
|
log follows above does not carry across that boundary. What is sent is
|
||||||
are two reasons to redact unconditionally anyway. Nothing debuggable
|
the chi route pattern instead: `http://host/webhook/{uuid}`.
|
||||||
is lost:
|
|
||||||
every handler reads its fields with `PostFormValue`, so the body is
|
|
||||||
exactly where the credentials are — the target destination URL, the
|
|
||||||
login password, both password-change fields — and the one route whose
|
|
||||||
body is genuine signal is the receiver, whose body is already stored
|
|
||||||
on the event and served from the UI, so a tracker is not where anyone
|
|
||||||
reads it. And an unconditional rule cannot leak on a route somebody
|
|
||||||
forgets to add to it, which a route-conditional one can.
|
|
||||||
|
|
||||||
The headers are an allowlist for that second reason: the SDK's own
|
The scheme and the host are kept, and everything else in the URL is
|
||||||
filter removes four names and passes everything else, which would ship
|
discarded rather than edited, so a future SDK version that starts
|
||||||
`X-CSRF-Token` and the shared secrets senders put on the receiver
|
appending a query string cannot widen this. The scheme has to survive
|
||||||
route. What survives still names the failing route — scheme, host,
|
for the reason given below. The host is whatever the request's `Host`
|
||||||
path, method — and `X-Request-Id` ties the event to the local access
|
header carried — this service validates no hostname, so on a directly
|
||||||
log line that holds the rest. Nothing dropped is needed for the
|
exposed deployment a client sets it — and that same header is on the
|
||||||
likeliest use, debugging a CSRF rejection. Its three inputs are the
|
allowlist above, so scrubbing the host out of the URL would withhold
|
||||||
TLS decision, `Origin` and `Referer`; the latter two are kept, and the
|
nothing that is not sent anyway.
|
||||||
first is already in the retained URL, because the SDK derives that
|
|
||||||
URL's scheme from `r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` —
|
The body, the query string and the URL are all handled on every route
|
||||||
byte for byte the predicate `internal/middleware/csrf.go` uses to
|
rather than filtered by route. For the URL that is also what keeps the
|
||||||
choose between the `csrf.Secure(true)` and `csrf.Secure(false)`
|
event locatable: an error event is grouped by its exception and stack
|
||||||
handlers. So dropping `X-Forwarded-Proto` costs nothing. The dropped
|
trace, not by its URL, so replacing the path with the pattern costs no
|
||||||
provider headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are
|
grouping and the pattern still names the route in the UI. And an
|
||||||
real signal but are recorded locally on the event, and
|
unconditional rule cannot leak on a route somebody forgets to add to
|
||||||
|
it, which a route-conditional one can. For the body there is a second
|
||||||
|
reason: nothing debuggable is lost, because every handler reads its
|
||||||
|
fields with `PostFormValue`, so the body is exactly where the
|
||||||
|
credentials are — the target destination URL, the login password, both
|
||||||
|
password-change fields — and the one route whose body is genuine
|
||||||
|
signal is the receiver, whose body is already stored on the event and
|
||||||
|
served from the UI, so a tracker is not where anyone reads it.
|
||||||
|
|
||||||
|
The route is reachable from the hook only on the error dispatch.
|
||||||
|
`sentryhttp`'s recover path puts the request on the context it hands
|
||||||
|
to `RecoverWithContext`, and the SDK carries that context through to
|
||||||
|
`BeforeSend` as `hint.Context`, so
|
||||||
|
`hint.Context.Value(sentry.RequestContextKey)` yields the live request
|
||||||
|
and chi's `RoutePattern()` yields the matched pattern off it. The
|
||||||
|
transaction dispatch has no such request: a finished span captures
|
||||||
|
with a nil hint, which the client replaces with an empty one, so
|
||||||
|
`BeforeSendTransaction` sees no context at all. Tracing is off in this
|
||||||
|
service, so no transaction event is produced today, but the hook is
|
||||||
|
installed on both dispatches as a floor.
|
||||||
|
|
||||||
|
Where the pattern is out of reach — the transaction dispatch, an event
|
||||||
|
captured outside the router, or a request that matched no route — the
|
||||||
|
fallback is never the concrete path. The path becomes the literal
|
||||||
|
`/(redacted)`, so the URL reads `http://host/(redacted)`; a URL the
|
||||||
|
rewrite cannot parse into a scheme is withheld whole. A transaction
|
||||||
|
event additionally carries the SDK's own `METHOD /path` name, built
|
||||||
|
from the concrete path as well; it is rewritten on the same terms, to
|
||||||
|
`POST /webhook/{uuid}` where the pattern is known and `POST
|
||||||
|
/(redacted)` where it is not.
|
||||||
|
|
||||||
|
The headers are an allowlist for the same reason the rules above are
|
||||||
|
unconditional: the SDK's own filter removes four names and passes
|
||||||
|
everything else, which would ship `X-CSRF-Token` and the shared
|
||||||
|
secrets senders put on the receiver route. What survives still names
|
||||||
|
the failing route — scheme, host, route pattern, method — and
|
||||||
|
`X-Request-Id` ties the event to the local access log line that holds
|
||||||
|
the rest. Nothing dropped is needed for the likeliest use, debugging a
|
||||||
|
CSRF rejection. Its three inputs are the TLS decision, `Origin` and
|
||||||
|
`Referer`; the latter two are kept, and the first is the scheme of the
|
||||||
|
retained URL, because the SDK derives that scheme from
|
||||||
|
`r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` — byte
|
||||||
|
for byte the predicate `internal/middleware/csrf.go` uses to choose
|
||||||
|
between the `csrf.Secure(true)` and `csrf.Secure(false)` handlers.
|
||||||
|
That is what the rewrite above preserves it for, and it is why
|
||||||
|
dropping `X-Forwarded-Proto` costs nothing. The dropped provider
|
||||||
|
headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are real
|
||||||
|
signal but are recorded locally on the event, and
|
||||||
`Sentry-Trace`/`Baggage` are already reflected in the event's trace
|
`Sentry-Trace`/`Baggage` are already reflected in the event's trace
|
||||||
context.
|
context.
|
||||||
|
|
||||||
|
|||||||
@@ -2,8 +2,11 @@ package server
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
|
"strings"
|
||||||
|
|
||||||
"github.com/getsentry/sentry-go"
|
"github.com/getsentry/sentry-go"
|
||||||
|
"github.com/go-chi/chi"
|
||||||
)
|
)
|
||||||
|
|
||||||
// sentryRedacted stands in for a withheld field on every event shipped
|
// sentryRedacted stands in for a withheld field on every event shipped
|
||||||
@@ -11,6 +14,12 @@ import (
|
|||||||
// can tell a suppressed value from an absent one.
|
// can tell a suppressed value from an absent one.
|
||||||
const sentryRedacted = "(redacted)"
|
const sentryRedacted = "(redacted)"
|
||||||
|
|
||||||
|
// sentryRedactedPath is what stands in for the request path when the
|
||||||
|
// route pattern is not reachable. It is deliberately not the concrete
|
||||||
|
// path: on the receiver route that path carries the entrypoint UUID,
|
||||||
|
// which is a write capability rather than an identifier.
|
||||||
|
const sentryRedactedPath = "/" + sentryRedacted
|
||||||
|
|
||||||
// sentryClientOptions builds the options the SDK is initialised with.
|
// sentryClientOptions builds the options the SDK is initialised with.
|
||||||
// It is its own function so a test can stand up a client wired exactly
|
// It is its own function so a test can stand up a client wired exactly
|
||||||
// as production is, with only the transport swapped.
|
// as production is, with only the transport swapped.
|
||||||
@@ -44,18 +53,43 @@ func sentryClientOptions(dsn, release string) sentry.ClientOptions {
|
|||||||
// login password and both password-change fields. None of that may
|
// login password and both password-change fields. None of that may
|
||||||
// reach a third-party service.
|
// reach a third-party service.
|
||||||
//
|
//
|
||||||
|
// URL is the third such field. NewRequest builds it as
|
||||||
|
// scheme://host/path (interfaces.go:183), and on the receiver route
|
||||||
|
// that path is /webhook/<uuid> in full — a write capability, not an
|
||||||
|
// identifier. It is rebuilt here from the chi route pattern, on every
|
||||||
|
// route, keeping the scheme and the host.
|
||||||
|
//
|
||||||
// This hook is a floor, not a default: the fields it clears stay
|
// This hook is a floor, not a default: the fields it clears stay
|
||||||
// cleared even if SendDefaultPII is ever turned on.
|
// cleared even if SendDefaultPII is ever turned on.
|
||||||
func scrubSentryRequest(
|
func scrubSentryRequest(
|
||||||
event *sentry.Event,
|
event *sentry.Event,
|
||||||
_ *sentry.EventHint,
|
hint *sentry.EventHint,
|
||||||
) *sentry.Event {
|
) *sentry.Event {
|
||||||
if event == nil || event.Request == nil {
|
if event == nil {
|
||||||
|
return event
|
||||||
|
}
|
||||||
|
|
||||||
|
pattern := sentryRoutePattern(hint)
|
||||||
|
|
||||||
|
// Only transaction events carry a Transaction name, and the SDK
|
||||||
|
// builds it from the concrete path too (sentryhttp.go:105 via
|
||||||
|
// tracing.go:553). Rewritten on the same terms.
|
||||||
|
if event.Transaction != "" {
|
||||||
|
event.Transaction = sentryTransactionName(
|
||||||
|
event.Transaction, pattern,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
if event.Request == nil {
|
||||||
return event
|
return event
|
||||||
}
|
}
|
||||||
|
|
||||||
req := event.Request
|
req := event.Request
|
||||||
|
|
||||||
|
if req.URL != "" {
|
||||||
|
req.URL = sentryRouteURL(req.URL, pattern)
|
||||||
|
}
|
||||||
|
|
||||||
if req.QueryString != "" {
|
if req.QueryString != "" {
|
||||||
req.QueryString = sentryRedacted
|
req.QueryString = sentryRedacted
|
||||||
}
|
}
|
||||||
@@ -71,6 +105,91 @@ func scrubSentryRequest(
|
|||||||
return event
|
return event
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// sentryRoutePattern returns the chi route pattern for the request the
|
||||||
|
// hint carries, or "" when it is not reachable.
|
||||||
|
//
|
||||||
|
// The request is reachable on the error dispatch only. sentryhttp's
|
||||||
|
// recover path calls RecoverWithContext with the request on the
|
||||||
|
// context under sentry.RequestContextKey (sentryhttp.go:124-125), and
|
||||||
|
// the client copies that context onto the hint (client.go:484-485)
|
||||||
|
// before handing it to BeforeSend (client.go:631). chi's routing
|
||||||
|
// context is a pointer placed on the request context before the
|
||||||
|
// middleware chain runs (chi mux.go:84) and filled in as the mux
|
||||||
|
// routes, so by the time a handler panics it names the matched route.
|
||||||
|
//
|
||||||
|
// The transaction dispatch has no such request: Span.doFinish calls
|
||||||
|
// hub.CaptureEvent (tracing.go:356), which passes a nil hint that the
|
||||||
|
// client replaces with an empty one (client.go:620-622). The pattern
|
||||||
|
// is therefore always "" there, and the callers fall back.
|
||||||
|
func sentryRoutePattern(hint *sentry.EventHint) string {
|
||||||
|
if hint == nil || hint.Context == nil {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
|
req, ok := hint.Context.Value(
|
||||||
|
sentry.RequestContextKey,
|
||||||
|
).(*http.Request)
|
||||||
|
if !ok || req == nil {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
|
rctx := chi.RouteContext(req.Context())
|
||||||
|
if rctx == nil {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
|
// Empty when no route matched, which is the fallback case too.
|
||||||
|
return rctx.RoutePattern()
|
||||||
|
}
|
||||||
|
|
||||||
|
// sentryRouteURL rebuilds an event's request URL with the route
|
||||||
|
// pattern in place of the concrete path.
|
||||||
|
//
|
||||||
|
// The scheme is load-bearing and is kept: the SDK derives it from
|
||||||
|
// r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"
|
||||||
|
// (interfaces.go:180), byte for byte the predicate
|
||||||
|
// internal/middleware/csrf.go uses, so it is the CSRF TLS decision and
|
||||||
|
// the reason dropping X-Forwarded-Proto from the header allowlist
|
||||||
|
// costs nothing. The host is parsed.Host of the SDK's
|
||||||
|
// scheme://r.Host/path, so it is whatever the client's Host header
|
||||||
|
// carried: this service validates no hostname. It is kept because that
|
||||||
|
// same header is on the allowlist, so scrubbing it here would withhold
|
||||||
|
// nothing that is not sent anyway.
|
||||||
|
//
|
||||||
|
// Everything else in the URL is discarded rather than edited, so a
|
||||||
|
// future SDK that starts appending a query string cannot widen this.
|
||||||
|
func sentryRouteURL(rawURL, pattern string) string {
|
||||||
|
parsed, err := url.Parse(rawURL)
|
||||||
|
if err != nil || parsed.Scheme == "" {
|
||||||
|
// Not a shape this can safely take apart.
|
||||||
|
return sentryRedacted
|
||||||
|
}
|
||||||
|
|
||||||
|
if pattern == "" {
|
||||||
|
pattern = sentryRedactedPath
|
||||||
|
}
|
||||||
|
|
||||||
|
return parsed.Scheme + "://" + parsed.Host + pattern
|
||||||
|
}
|
||||||
|
|
||||||
|
// sentryTransactionName rebuilds the SDK's "METHOD /path" transaction
|
||||||
|
// name with the route pattern in place of the concrete path. Method is
|
||||||
|
// kept for the same reason Request.Method is: net/http admits only a
|
||||||
|
// bounded token there. A name in any other shape is withheld whole,
|
||||||
|
// since nothing can be said about which part of it is a path.
|
||||||
|
func sentryTransactionName(name, pattern string) string {
|
||||||
|
method, _, found := strings.Cut(name, " ")
|
||||||
|
if !found {
|
||||||
|
return sentryRedacted
|
||||||
|
}
|
||||||
|
|
||||||
|
if pattern == "" {
|
||||||
|
pattern = sentryRedactedPath
|
||||||
|
}
|
||||||
|
|
||||||
|
return method + " " + pattern
|
||||||
|
}
|
||||||
|
|
||||||
// keptSentryHeaders returns the subset of headers an event may carry
|
// keptSentryHeaders returns the subset of headers an event may carry
|
||||||
// off-host. Dropping by allowlist rather than by blocklist is what
|
// off-host. Dropping by allowlist rather than by blocklist is what
|
||||||
// makes an unrecognised header safe: the SDK's own filter removes four
|
// makes an unrecognised header safe: the SDK's own filter removes four
|
||||||
|
|||||||
@@ -13,12 +13,13 @@ import (
|
|||||||
|
|
||||||
"github.com/getsentry/sentry-go"
|
"github.com/getsentry/sentry-go"
|
||||||
sentryhttp "github.com/getsentry/sentry-go/http"
|
sentryhttp "github.com/getsentry/sentry-go/http"
|
||||||
|
"github.com/go-chi/chi"
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
"sneak.berlin/go/webhooker/internal/server"
|
"sneak.berlin/go/webhooker/internal/server"
|
||||||
)
|
)
|
||||||
|
|
||||||
// The three markers below are the credentials a captured event could
|
// The four markers below are the credentials a captured event could
|
||||||
// carry off-host, one per field of sentry.Request that the SDK fills
|
// carry off-host, one per field of sentry.Request that the SDK fills
|
||||||
// from the request without a SendDefaultPII guard.
|
// from the request without a SendDefaultPII guard.
|
||||||
const (
|
const (
|
||||||
@@ -33,6 +34,12 @@ const (
|
|||||||
// sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf
|
// sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf
|
||||||
// accepts in place of the form field.
|
// accepts in place of the form field.
|
||||||
sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ"
|
sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ"
|
||||||
|
|
||||||
|
// sentryReceiverUUID is the entrypoint identifier in the path of
|
||||||
|
// a receiver request. It is a write capability: anyone holding
|
||||||
|
// it can POST events this service accepts and its targets then
|
||||||
|
// deliver, so it may not reach a third-party tracker.
|
||||||
|
sentryReceiverUUID = "6d1f9c2a-3b7e-4f58-9a0d-c0ffeebadc0d"
|
||||||
)
|
)
|
||||||
|
|
||||||
// sentryKeptUserAgent is a non-secret header value planted so the
|
// sentryKeptUserAgent is a non-secret header value planted so the
|
||||||
@@ -58,19 +65,41 @@ func (c *captureTransport) SendEvent(event *sentry.Event) {
|
|||||||
c.events = append(c.events, event)
|
c.events = append(c.events, event)
|
||||||
}
|
}
|
||||||
|
|
||||||
// captureThroughSentryHTTP panics inside a form handler wrapped in the
|
// sentryCase drives one request through the real sentryhttp middleware
|
||||||
// real sentryhttp middleware and returns the event the SDK produced.
|
// inside a real chi router and returns the events the SDK produced.
|
||||||
//
|
//
|
||||||
// This is the only construction path on which Request.Data appears:
|
// Routing through a chi mux is load-bearing, not decoration. chi puts
|
||||||
// sentryhttp calls Scope.SetRequest, which tees r.Body into a 10 KiB
|
// its routing context on the request context before the middleware
|
||||||
// buffer, ParseForm drains the tee, and Scope.ApplyToEvent copies the
|
// chain runs and fills it in as it matches, so a hand-built request
|
||||||
// buffer into the event inside prepareEvent — before BeforeSend runs.
|
// carries no route pattern at all and could not distinguish the hook
|
||||||
// A hand-built sentry.NewRequest never reads the body and so cannot
|
// working from the hook falling back.
|
||||||
// regress-test any of it.
|
|
||||||
//
|
//
|
||||||
// scrub selects whether the production BeforeSend hooks are installed,
|
// This is also the only construction path on which Request.Data
|
||||||
// so the same path shows both what the SDK collects and what survives.
|
// appears: sentryhttp calls Scope.SetRequest, which tees r.Body into a
|
||||||
func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event {
|
// 10 KiB buffer, ParseForm drains the tee, and Scope.ApplyToEvent
|
||||||
|
// copies the buffer into the event inside prepareEvent — before
|
||||||
|
// BeforeSend runs. A hand-built sentry.NewRequest never reads the body
|
||||||
|
// and so cannot regress-test any of it.
|
||||||
|
type sentryCase struct {
|
||||||
|
// scrub selects whether the production BeforeSend hooks are
|
||||||
|
// installed, so the same path shows both what the SDK collects
|
||||||
|
// and what survives.
|
||||||
|
scrub bool
|
||||||
|
|
||||||
|
// tracing enables the transaction dispatch, which the service
|
||||||
|
// leaves off. With it on, a served request produces a
|
||||||
|
// transaction event through BeforeSendTransaction.
|
||||||
|
tracing bool
|
||||||
|
|
||||||
|
// panics selects the error dispatch, via BeforeSend.
|
||||||
|
panics bool
|
||||||
|
|
||||||
|
// request builds the request to serve, given the client whose
|
||||||
|
// hub it must carry.
|
||||||
|
request func(*sentry.Client) *http.Request
|
||||||
|
}
|
||||||
|
|
||||||
|
func (c sentryCase) capture(t *testing.T) []*sentry.Event {
|
||||||
t.Helper()
|
t.Helper()
|
||||||
|
|
||||||
transport := &captureTransport{}
|
transport := &captureTransport{}
|
||||||
@@ -80,57 +109,107 @@ func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event {
|
|||||||
)
|
)
|
||||||
opts.Transport = transport
|
opts.Transport = transport
|
||||||
|
|
||||||
if !scrub {
|
if !c.scrub {
|
||||||
opts.BeforeSend = nil
|
opts.BeforeSend = nil
|
||||||
opts.BeforeSendTransaction = nil
|
opts.BeforeSendTransaction = nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if c.tracing {
|
||||||
|
opts.EnableTracing = true
|
||||||
|
opts.TracesSampleRate = 1.0
|
||||||
|
}
|
||||||
|
|
||||||
client, err := sentry.NewClient(opts)
|
client, err := sentry.NewClient(opts)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
||||||
handler := sentryhttp.New(sentryhttp.Options{}).Handle(
|
c.router().ServeHTTP(httptest.NewRecorder(), c.request(client))
|
||||||
http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
|
||||||
// This call is what drains the tee and fills the
|
|
||||||
// buffer. Its success is asserted by the unscrubbed
|
|
||||||
// case below, which sees the body in the event.
|
|
||||||
_ = r.ParseForm()
|
|
||||||
|
|
||||||
panic("boom")
|
return transport.events
|
||||||
}),
|
|
||||||
)
|
|
||||||
|
|
||||||
handler.ServeHTTP(
|
|
||||||
httptest.NewRecorder(),
|
|
||||||
sentryLoginRequest(client),
|
|
||||||
)
|
|
||||||
|
|
||||||
require.Len(t, transport.events, 1)
|
|
||||||
|
|
||||||
return transport.events[0]
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// sentryLoginRequest builds the password POST the capture above drives,
|
// router mirrors setupGlobalMiddleware's ordering over the two route
|
||||||
// with a credential planted in the body, the query and a header.
|
// patterns these tests need: a recovering middleware first, then the
|
||||||
|
// sentryhttp handler registered with Use and Repanic set, exactly as
|
||||||
|
// routes.go registers it. The local recover stands in for chi's
|
||||||
|
// middleware.Recoverer, which holds that slot in production; it is
|
||||||
|
// here only to keep panic stacks out of the test output.
|
||||||
|
func (c sentryCase) router() http.Handler {
|
||||||
|
handler := func(_ http.ResponseWriter, r *http.Request) {
|
||||||
|
// This call is what drains the body tee and fills the
|
||||||
|
// buffer. Its success is asserted by the unscrubbed case
|
||||||
|
// below, which sees the body in the event.
|
||||||
|
_ = r.ParseForm()
|
||||||
|
|
||||||
|
if c.panics {
|
||||||
|
panic("boom")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
router := chi.NewRouter()
|
||||||
|
router.Use(recoveringMiddleware)
|
||||||
|
router.Use(
|
||||||
|
sentryhttp.New(sentryhttp.Options{Repanic: true}).Handle,
|
||||||
|
)
|
||||||
|
router.HandleFunc("/pages/login", handler)
|
||||||
|
router.HandleFunc("/webhook/{uuid}", handler)
|
||||||
|
|
||||||
|
return router
|
||||||
|
}
|
||||||
|
|
||||||
|
func recoveringMiddleware(next http.Handler) http.Handler {
|
||||||
|
return http.HandlerFunc(
|
||||||
|
func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
defer func() { _ = recover() }()
|
||||||
|
|
||||||
|
next.ServeHTTP(w, r)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
// sentryLoginRequest builds the password POST most cases drive, with a
|
||||||
|
// credential planted in the body, the query and a header.
|
||||||
func sentryLoginRequest(client *sentry.Client) *http.Request {
|
func sentryLoginRequest(client *sentry.Client) *http.Request {
|
||||||
form := url.Values{}
|
form := url.Values{}
|
||||||
form.Set("username", "admin")
|
form.Set("username", "admin")
|
||||||
form.Set("password", sentryBodyMarker)
|
form.Set("password", sentryBodyMarker)
|
||||||
|
|
||||||
|
req := sentryRequest(
|
||||||
|
client,
|
||||||
|
"/pages/login?url=https://hooks.slack.com/services/"+
|
||||||
|
sentryQueryMarker,
|
||||||
|
form.Encode(),
|
||||||
|
)
|
||||||
|
|
||||||
|
req.Header.Set("X-Csrf-Token", sentryHeaderMarker)
|
||||||
|
|
||||||
|
return req
|
||||||
|
}
|
||||||
|
|
||||||
|
// sentryReceiverRequest builds a POST to the receiver route, whose
|
||||||
|
// concrete path carries the entrypoint capability.
|
||||||
|
func sentryReceiverRequest(client *sentry.Client) *http.Request {
|
||||||
|
return sentryRequest(
|
||||||
|
client, "/webhook/"+sentryReceiverUUID, "payload=hello",
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
func sentryRequest(
|
||||||
|
client *sentry.Client,
|
||||||
|
target, body string,
|
||||||
|
) *http.Request {
|
||||||
req := httptest.NewRequestWithContext(
|
req := httptest.NewRequestWithContext(
|
||||||
sentry.SetHubOnContext(
|
sentry.SetHubOnContext(
|
||||||
context.Background(),
|
context.Background(),
|
||||||
sentry.NewHub(client, sentry.NewScope()),
|
sentry.NewHub(client, sentry.NewScope()),
|
||||||
),
|
),
|
||||||
http.MethodPost,
|
http.MethodPost,
|
||||||
"/pages/login?url=https://hooks.slack.com/services/"+
|
target,
|
||||||
sentryQueryMarker,
|
strings.NewReader(body),
|
||||||
strings.NewReader(form.Encode()),
|
|
||||||
)
|
)
|
||||||
|
|
||||||
req.Header.Set(
|
req.Header.Set(
|
||||||
"Content-Type", "application/x-www-form-urlencoded",
|
"Content-Type", "application/x-www-form-urlencoded",
|
||||||
)
|
)
|
||||||
req.Header.Set("X-Csrf-Token", sentryHeaderMarker)
|
|
||||||
req.Header.Set("User-Agent", sentryKeptUserAgent)
|
req.Header.Set("User-Agent", sentryKeptUserAgent)
|
||||||
|
|
||||||
return req
|
return req
|
||||||
@@ -146,15 +225,27 @@ func marshalEvent(t *testing.T, event *sentry.Event) string {
|
|||||||
return string(encoded)
|
return string(encoded)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// onlyEvent asserts a single event was captured and returns it.
|
||||||
|
func onlyEvent(t *testing.T, events []*sentry.Event) *sentry.Event {
|
||||||
|
t.Helper()
|
||||||
|
|
||||||
|
require.Len(t, events, 1)
|
||||||
|
require.NotNil(t, events[0].Request)
|
||||||
|
|
||||||
|
return events[0]
|
||||||
|
}
|
||||||
|
|
||||||
// TestSentryScrub_SDKCollectsTheRequestUnscrubbed pins the premise the
|
// TestSentryScrub_SDKCollectsTheRequestUnscrubbed pins the premise the
|
||||||
// hook exists for. Without it the SDK ships the whole POST body, the
|
// hook exists for. Without it the SDK ships the whole POST body, the
|
||||||
// raw query and the CSRF header, none of which SendDefaultPII=false
|
// raw query, the CSRF header and the concrete request path, none of
|
||||||
// suppresses.
|
// which SendDefaultPII=false suppresses.
|
||||||
func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
event := captureThroughSentryHTTP(t, false)
|
event := onlyEvent(t, sentryCase{
|
||||||
require.NotNil(t, event.Request)
|
panics: true,
|
||||||
|
request: sentryLoginRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
assert.Contains(
|
assert.Contains(
|
||||||
t, event.Request.Data, sentryBodyMarker,
|
t, event.Request.Data, sentryBodyMarker,
|
||||||
@@ -165,6 +256,18 @@ func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
|||||||
assert.Contains(
|
assert.Contains(
|
||||||
t, marshalEvent(t, event), sentryHeaderMarker,
|
t, marshalEvent(t, event), sentryHeaderMarker,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
receiver := onlyEvent(t, sentryCase{
|
||||||
|
panics: true,
|
||||||
|
request: sentryReceiverRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
|
assert.Contains(
|
||||||
|
t, receiver.Request.URL, sentryReceiverUUID,
|
||||||
|
"the SDK is expected to build Request.URL from the "+
|
||||||
|
"concrete path; if it no longer does, the route "+
|
||||||
|
"pattern rewrite's premise changed",
|
||||||
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestSentryScrub_RedactsTheCapturedRequest is the regression test: no
|
// TestSentryScrub_RedactsTheCapturedRequest is the regression test: no
|
||||||
@@ -173,8 +276,11 @@ func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
|||||||
func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
|
func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
event := captureThroughSentryHTTP(t, true)
|
event := onlyEvent(t, sentryCase{
|
||||||
require.NotNil(t, event.Request)
|
scrub: true,
|
||||||
|
panics: true,
|
||||||
|
request: sentryLoginRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
encoded := marshalEvent(t, event)
|
encoded := marshalEvent(t, event)
|
||||||
|
|
||||||
@@ -189,16 +295,45 @@ func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
|
|||||||
assert.Empty(t, event.Request.Env)
|
assert.Empty(t, event.Request.Env)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern is the
|
||||||
|
// regression test for the receiver URL: the entrypoint UUID is a write
|
||||||
|
// capability and may not reach the tracker, while the route it names
|
||||||
|
// must still be readable there.
|
||||||
|
func TestSentryScrub_ReplacesTheCapabilityPathWithTheRoutePattern(
|
||||||
|
t *testing.T,
|
||||||
|
) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
event := onlyEvent(t, sentryCase{
|
||||||
|
scrub: true,
|
||||||
|
panics: true,
|
||||||
|
request: sentryReceiverRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
|
assert.NotContains(
|
||||||
|
t, marshalEvent(t, event), sentryReceiverUUID,
|
||||||
|
)
|
||||||
|
assert.Equal(
|
||||||
|
t, "http://example.com/webhook/{uuid}", event.Request.URL,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost
|
// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost
|
||||||
// the debugging signal: the route, the method and the metadata headers
|
// the debugging signal: the route, its scheme and host, the method and
|
||||||
// still identify what failed.
|
// the metadata headers still identify what failed. On a static route
|
||||||
|
// the pattern is the path, so the URL is unchanged there.
|
||||||
func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) {
|
func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
event := captureThroughSentryHTTP(t, true)
|
event := onlyEvent(t, sentryCase{
|
||||||
require.NotNil(t, event.Request)
|
scrub: true,
|
||||||
|
panics: true,
|
||||||
|
request: sentryLoginRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
assert.Contains(t, event.Request.URL, "/pages/login")
|
assert.Equal(
|
||||||
|
t, "http://example.com/pages/login", event.Request.URL,
|
||||||
|
)
|
||||||
assert.Equal(t, http.MethodPost, event.Request.Method)
|
assert.Equal(t, http.MethodPost, event.Request.Method)
|
||||||
assert.Equal(
|
assert.Equal(
|
||||||
t,
|
t,
|
||||||
@@ -212,6 +347,127 @@ func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestSentryScrub_RedactsTheTransactionDispatch covers the other hook.
|
||||||
|
// Span.doFinish captures with a nil hint, so BeforeSendTransaction
|
||||||
|
// gets one with no context and no request: the route pattern is out of
|
||||||
|
// reach and both the URL and the SDK-built transaction name have to
|
||||||
|
// fall back. Tracing is off in this service, so no transaction event
|
||||||
|
// is produced today; the hook is a floor against that changing.
|
||||||
|
func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
events := sentryCase{
|
||||||
|
scrub: true,
|
||||||
|
tracing: true,
|
||||||
|
request: sentryReceiverRequest,
|
||||||
|
}.capture(t)
|
||||||
|
|
||||||
|
event := onlyEvent(t, events)
|
||||||
|
require.Equal(t, "transaction", event.Type)
|
||||||
|
|
||||||
|
assert.NotContains(
|
||||||
|
t, marshalEvent(t, event), sentryReceiverUUID,
|
||||||
|
)
|
||||||
|
assert.Equal(
|
||||||
|
t, "http://example.com/(redacted)", event.Request.URL,
|
||||||
|
)
|
||||||
|
assert.Equal(t, "POST /(redacted)", event.Transaction)
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook pins
|
||||||
|
// that dispatch's premise the same way, since it is the one the
|
||||||
|
// service does not exercise today.
|
||||||
|
func TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook(
|
||||||
|
t *testing.T,
|
||||||
|
) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
event := onlyEvent(t, sentryCase{
|
||||||
|
tracing: true,
|
||||||
|
request: sentryReceiverRequest,
|
||||||
|
}.capture(t))
|
||||||
|
|
||||||
|
require.Equal(t, "transaction", event.Type)
|
||||||
|
assert.Contains(t, event.Request.URL, sentryReceiverUUID)
|
||||||
|
assert.Contains(t, event.Transaction, sentryReceiverUUID)
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSentryScrub_FallsBackWithoutARoutePattern covers every way the
|
||||||
|
// pattern can be missing. None of them may fall back to the concrete
|
||||||
|
// path, and all of them keep the scheme, which is the CSRF TLS
|
||||||
|
// decision.
|
||||||
|
func TestSentryScrub_FallsBackWithoutARoutePattern(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
concrete := "https://example.com/webhook/" + sentryReceiverUUID
|
||||||
|
|
||||||
|
// A request with no chi routing context on it at all, which is
|
||||||
|
// what an event captured outside the router would carry.
|
||||||
|
unrouted := httptest.NewRequestWithContext(
|
||||||
|
context.Background(), http.MethodPost, concrete, nil,
|
||||||
|
)
|
||||||
|
|
||||||
|
for name, hint := range map[string]*sentry.EventHint{
|
||||||
|
"no hint": nil,
|
||||||
|
"no context": {},
|
||||||
|
"no request": {Context: context.Background()},
|
||||||
|
"unrouted request": {
|
||||||
|
Context: context.WithValue(
|
||||||
|
context.Background(),
|
||||||
|
sentry.RequestContextKey,
|
||||||
|
unrouted,
|
||||||
|
),
|
||||||
|
},
|
||||||
|
} {
|
||||||
|
t.Run(name, func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
event := sentry.NewEvent()
|
||||||
|
event.Request = &sentry.Request{URL: concrete}
|
||||||
|
event.Transaction = "POST /webhook/" +
|
||||||
|
sentryReceiverUUID
|
||||||
|
|
||||||
|
scrubbed := server.ScrubSentryRequestForTest(
|
||||||
|
event, hint,
|
||||||
|
)
|
||||||
|
require.NotNil(t, scrubbed)
|
||||||
|
|
||||||
|
assert.Equal(
|
||||||
|
t,
|
||||||
|
"https://example.com/(redacted)",
|
||||||
|
scrubbed.Request.URL,
|
||||||
|
)
|
||||||
|
assert.Equal(
|
||||||
|
t, "POST /(redacted)", scrubbed.Transaction,
|
||||||
|
)
|
||||||
|
assert.NotContains(
|
||||||
|
t,
|
||||||
|
marshalEvent(t, scrubbed),
|
||||||
|
sentryReceiverUUID,
|
||||||
|
)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSentryScrub_WithholdsUnparseableValues covers the shapes the
|
||||||
|
// rewrite cannot take apart. Withholding them whole is the safe
|
||||||
|
// answer, since nothing can be said about which part is a path.
|
||||||
|
func TestSentryScrub_WithholdsUnparseableValues(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
event := sentry.NewEvent()
|
||||||
|
event.Request = &sentry.Request{
|
||||||
|
URL: "/webhook/" + sentryReceiverUUID,
|
||||||
|
}
|
||||||
|
event.Transaction = "/webhook/" + sentryReceiverUUID
|
||||||
|
|
||||||
|
scrubbed := server.ScrubSentryRequestForTest(event, nil)
|
||||||
|
require.NotNil(t, scrubbed)
|
||||||
|
|
||||||
|
assert.Equal(t, "(redacted)", scrubbed.Request.URL)
|
||||||
|
assert.Equal(t, "(redacted)", scrubbed.Transaction)
|
||||||
|
}
|
||||||
|
|
||||||
// TestSentryScrub_ToleratesEventsWithoutARequest covers the events the
|
// TestSentryScrub_ToleratesEventsWithoutARequest covers the events the
|
||||||
// hook sees outside an HTTP handler, where no request is attached.
|
// hook sees outside an HTTP handler, where no request is attached.
|
||||||
func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) {
|
func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) {
|
||||||
@@ -223,5 +479,6 @@ func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) {
|
|||||||
|
|
||||||
require.NotNil(t, scrubbed)
|
require.NotNil(t, scrubbed)
|
||||||
assert.Nil(t, scrubbed.Request)
|
assert.Nil(t, scrubbed.Request)
|
||||||
|
assert.Empty(t, scrubbed.Transaction)
|
||||||
assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil))
|
assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil))
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user