Compare commits
1 Commits
ec5acee69f
...
4884581fc5
| Author | SHA1 | Date | |
|---|---|---|---|
| 4884581fc5 |
110
README.md
110
README.md
@@ -1038,77 +1038,38 @@ 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 same hook rewrites the request URL. The SDK builds it as
|
The body is replaced on every route rather than filtered by route, and
|
||||||
`scheme://host/path` from the concrete path, which on the receiver
|
that is a choice rather than a limitation: the route is reachable from
|
||||||
route is `/webhook/<uuid>` in full — and that UUID is a write
|
the hook. `sentryhttp`'s recover path puts the request on the context
|
||||||
capability, not an identifier: anyone holding it can post events this
|
it hands to `RecoverWithContext`, and the SDK carries that context
|
||||||
service accepts and its targets then deliver. A tracker has its own
|
through to `BeforeSend` as `hint.Context`, so
|
||||||
retention, access control and deletion policy, so the rule the access
|
|
||||||
log follows above does not carry across that boundary. What is sent is
|
|
||||||
the chi route pattern instead: `http://host/webhook/{uuid}`.
|
|
||||||
|
|
||||||
The scheme and the host are kept, and everything else in the URL is
|
|
||||||
discarded rather than edited, so a future SDK version that starts
|
|
||||||
appending a query string cannot widen this. The scheme has to survive
|
|
||||||
for the reason given below. The host is whatever the request's `Host`
|
|
||||||
header carried — this service validates no hostname, so on a directly
|
|
||||||
exposed deployment a client sets it — and that same header is on the
|
|
||||||
allowlist above, so scrubbing the host out of the URL would withhold
|
|
||||||
nothing that is not sent anyway.
|
|
||||||
|
|
||||||
The body, the query string and the URL are all handled on every route
|
|
||||||
rather than filtered by route. For the URL that is also what keeps the
|
|
||||||
event locatable: an error event is grouped by its exception and stack
|
|
||||||
trace, not by its URL, so replacing the path with the pattern costs no
|
|
||||||
grouping and the pattern still names the route in the UI. And an
|
|
||||||
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
|
`hint.Context.Value(sentry.RequestContextKey)` yields the live request
|
||||||
and chi's `RoutePattern()` yields the matched pattern off it. The
|
and chi's `RoutePattern()` yields the matched pattern off it. There
|
||||||
transaction dispatch has no such request: a finished span captures
|
are two reasons to redact unconditionally anyway. Nothing debuggable
|
||||||
with a nil hint, which the client replaces with an empty one, so
|
is lost:
|
||||||
`BeforeSendTransaction` sees no context at all. Tracing is off in this
|
every handler reads its fields with `PostFormValue`, so the body is
|
||||||
service, so no transaction event is produced today, but the hook is
|
exactly where the credentials are — the target destination URL, the
|
||||||
installed on both dispatches as a floor.
|
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.
|
||||||
|
|
||||||
Where the pattern is out of reach — the transaction dispatch, an event
|
The headers are an allowlist for that second reason: the SDK's own
|
||||||
captured outside the router, or a request that matched no route — the
|
filter removes four names and passes everything else, which would ship
|
||||||
fallback is never the concrete path. The path becomes the literal
|
`X-CSRF-Token` and the shared secrets senders put on the receiver
|
||||||
`/(redacted)`, so the URL reads `http://host/(redacted)`; a URL the
|
route. What survives still names the failing route — scheme, host,
|
||||||
rewrite cannot parse into a scheme is withheld whole. A transaction
|
path, method — and `X-Request-Id` ties the event to the local access
|
||||||
event additionally carries the SDK's own `METHOD /path` name, built
|
log line that holds the rest. Nothing dropped is needed for the
|
||||||
from the concrete path as well; it is rewritten on the same terms, to
|
likeliest use, debugging a CSRF rejection. Its three inputs are the
|
||||||
`POST /webhook/{uuid}` where the pattern is known and `POST
|
TLS decision, `Origin` and `Referer`; the latter two are kept, and the
|
||||||
/(redacted)` where it is not.
|
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 headers are an allowlist for the same reason the rules above are
|
byte for byte the predicate `internal/middleware/csrf.go` uses to
|
||||||
unconditional: the SDK's own filter removes four names and passes
|
choose between the `csrf.Secure(true)` and `csrf.Secure(false)`
|
||||||
everything else, which would ship `X-CSRF-Token` and the shared
|
handlers. So dropping `X-Forwarded-Proto` costs nothing. The dropped
|
||||||
secrets senders put on the receiver route. What survives still names
|
provider headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are
|
||||||
the failing route — scheme, host, route pattern, method — and
|
real signal but are recorded locally on the event, 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.
|
||||||
|
|
||||||
@@ -1170,22 +1131,11 @@ log does, so none of them can be wider than it:
|
|||||||
| `auth middleware: unauthenticated request` | `DEBUG` | path, method | yes, by definition |
|
| `auth middleware: unauthenticated request` | `DEBUG` | path, method | yes, by definition |
|
||||||
| `entrypoint not found` | `DEBUG` | entrypoint UUID | yes, on the receiver |
|
| `entrypoint not found` | `DEBUG` | entrypoint UUID | yes, on the receiver |
|
||||||
| `user not found` / `invalid password` | `DEBUG` | username | yes, on the login form |
|
| `user not found` / `invalid password` | `DEBUG` | username | yes, on the login form |
|
||||||
| `login failure limit exceeded` (429) | `WARN` | path | yes, on the login form |
|
|
||||||
| `password verification capacity exhausted` | `WARN` | path | yes, on the login form |
|
|
||||||
|
|
||||||
`DEBUG` being off by default is not a bound. An operator turning it on
|
`DEBUG` being off by default is not a bound. An operator turning it on
|
||||||
to diagnose a flood must not thereby hand the flood an unbounded write,
|
to diagnose a flood must not thereby hand the flood an unbounded write,
|
||||||
so those lines are capped too.
|
so those lines are capped too.
|
||||||
|
|
||||||
The last two rows are capped defensively rather than against a
|
|
||||||
demonstrated width: chi routes `POST /pages/login` on a static pattern,
|
|
||||||
so `r.URL.Path` there is the 12-byte constant `/pages/login` and each
|
|
||||||
line lands near 120 bytes. `RecordLoginFailure` is nonetheless an
|
|
||||||
exported method taking any `*http.Request`, so a future caller on a
|
|
||||||
route with a URL parameter would widen the line with nothing failing.
|
|
||||||
Removing either cap therefore breaks no test — recorded here because a
|
|
||||||
cap whose absence is undetectable is worth saying so about.
|
|
||||||
|
|
||||||
`internal/middleware/logbound_test.go` and
|
`internal/middleware/logbound_test.go` and
|
||||||
`internal/handlers/logbound_test.go` drive 8 KB of client-chosen text at
|
`internal/handlers/logbound_test.go` drive 8 KB of client-chosen text at
|
||||||
each of these, through both handlers and through every character the
|
each of these, through both handlers and through every character the
|
||||||
|
|||||||
@@ -120,9 +120,7 @@ func (h *Handlers) authenticateUser(
|
|||||||
if !ok {
|
if !ok {
|
||||||
h.log.Warn(
|
h.log.Warn(
|
||||||
"password verification capacity exhausted",
|
"password verification capacity exhausted",
|
||||||
"path", logfield.Truncate(
|
"path", r.URL.Path,
|
||||||
r.URL.Path, logfield.MaxBytes,
|
|
||||||
),
|
|
||||||
)
|
)
|
||||||
h.renderLoginError(
|
h.renderLoginError(
|
||||||
w, r,
|
w, r,
|
||||||
|
|||||||
@@ -7,8 +7,6 @@ import (
|
|||||||
"net/http"
|
"net/http"
|
||||||
"sync"
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"sneak.berlin/go/webhooker/internal/logfield"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
const (
|
const (
|
||||||
@@ -346,16 +344,8 @@ func (m *Middleware) RecordLoginFailure(
|
|||||||
) bool {
|
) bool {
|
||||||
throttled := m.guard().fail(m.clientKey(r), username)
|
throttled := m.guard().fail(m.clientKey(r), username)
|
||||||
if throttled {
|
if throttled {
|
||||||
// Truncated even though chi pins this route's path to
|
|
||||||
// the 12-byte constant "/pages/login": RecordLoginFailure
|
|
||||||
// is exported and takes any *http.Request, so a caller on
|
|
||||||
// a route with a URL parameter would otherwise widen this
|
|
||||||
// line with nothing failing.
|
|
||||||
m.log.Warn(
|
m.log.Warn(
|
||||||
"login failure limit exceeded",
|
"login failure limit exceeded", "path", r.URL.Path,
|
||||||
"path", logfield.Truncate(
|
|
||||||
r.URL.Path, logfield.MaxBytes,
|
|
||||||
),
|
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -86,11 +86,8 @@ const (
|
|||||||
// THROUGH SLOG that carries text an UNAUTHENTICATED client
|
// THROUGH SLOG that carries text an UNAUTHENTICATED client
|
||||||
// supplies. Those lines — the MaxBodySize rejection, the CSRF
|
// supplies. Those lines — the MaxBodySize rejection, the CSRF
|
||||||
// rejection, the rate-limit rejection, the unauthenticated-request
|
// rejection, the rate-limit rejection, the unauthenticated-request
|
||||||
// and unknown-entrypoint DEBUG lines, the failed-login DEBUG
|
// and unknown-entrypoint DEBUG lines, and the failed-login DEBUG
|
||||||
// lines, and the two login-throttle WARN lines ("login failure
|
// lines — spend the same per-field budgets, and each carries
|
||||||
// limit exceeded" in loginguard.go and "password verification
|
|
||||||
// capacity exhausted" in internal/handlers/auth.go) — spend the
|
|
||||||
// same per-field budgets, and each carries
|
|
||||||
// strictly fewer client-supplied fields than the access log does,
|
// strictly fewer client-supplied fields than the access log does,
|
||||||
// so none of them can reach a width the access log cannot. That is
|
// so none of them can reach a width the access log cannot. That is
|
||||||
// asserted directly, per line and under both handlers, rather than
|
// asserted directly, per line and under both handlers, rather than
|
||||||
|
|||||||
@@ -2,11 +2,8 @@ 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
|
||||||
@@ -14,12 +11,6 @@ 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.
|
||||||
@@ -53,43 +44,18 @@ 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,
|
||||||
hint *sentry.EventHint,
|
_ *sentry.EventHint,
|
||||||
) *sentry.Event {
|
) *sentry.Event {
|
||||||
if event == nil {
|
if event == nil || event.Request == 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
|
||||||
}
|
}
|
||||||
@@ -105,91 +71,6 @@ 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,13 +13,12 @@ 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 four markers below are the credentials a captured event could
|
// The three 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 (
|
||||||
@@ -34,12 +33,6 @@ 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
|
||||||
@@ -65,41 +58,19 @@ func (c *captureTransport) SendEvent(event *sentry.Event) {
|
|||||||
c.events = append(c.events, event)
|
c.events = append(c.events, event)
|
||||||
}
|
}
|
||||||
|
|
||||||
// sentryCase drives one request through the real sentryhttp middleware
|
// captureThroughSentryHTTP panics inside a form handler wrapped in the
|
||||||
// inside a real chi router and returns the events the SDK produced.
|
// real sentryhttp middleware and returns the event the SDK produced.
|
||||||
//
|
//
|
||||||
// Routing through a chi mux is load-bearing, not decoration. chi puts
|
// This is the only construction path on which Request.Data appears:
|
||||||
// its routing context on the request context before the middleware
|
// sentryhttp calls Scope.SetRequest, which tees r.Body into a 10 KiB
|
||||||
// chain runs and fills it in as it matches, so a hand-built request
|
// buffer, ParseForm drains the tee, and Scope.ApplyToEvent copies the
|
||||||
// carries no route pattern at all and could not distinguish the hook
|
// buffer into the event inside prepareEvent — before BeforeSend runs.
|
||||||
// working from the hook falling back.
|
// A hand-built sentry.NewRequest never reads the body and so cannot
|
||||||
|
// regress-test any of it.
|
||||||
//
|
//
|
||||||
// This is also the only construction path on which Request.Data
|
// scrub selects whether the production BeforeSend hooks are installed,
|
||||||
// appears: sentryhttp calls Scope.SetRequest, which tees r.Body into a
|
// so the same path shows both what the SDK collects and what survives.
|
||||||
// 10 KiB buffer, ParseForm drains the tee, and Scope.ApplyToEvent
|
func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event {
|
||||||
// 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{}
|
||||||
@@ -109,107 +80,57 @@ func (c sentryCase) capture(t *testing.T) []*sentry.Event {
|
|||||||
)
|
)
|
||||||
opts.Transport = transport
|
opts.Transport = transport
|
||||||
|
|
||||||
if !c.scrub {
|
if !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)
|
||||||
|
|
||||||
c.router().ServeHTTP(httptest.NewRecorder(), c.request(client))
|
handler := sentryhttp.New(sentryhttp.Options{}).Handle(
|
||||||
|
http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||||
return transport.events
|
// 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.
|
||||||
// router mirrors setupGlobalMiddleware's ordering over the two route
|
|
||||||
// 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()
|
_ = r.ParseForm()
|
||||||
|
|
||||||
if c.panics {
|
|
||||||
panic("boom")
|
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
|
handler.ServeHTTP(
|
||||||
|
httptest.NewRecorder(),
|
||||||
|
sentryLoginRequest(client),
|
||||||
|
)
|
||||||
|
|
||||||
|
require.Len(t, transport.events, 1)
|
||||||
|
|
||||||
|
return transport.events[0]
|
||||||
}
|
}
|
||||||
|
|
||||||
func recoveringMiddleware(next http.Handler) http.Handler {
|
// sentryLoginRequest builds the password POST the capture above drives,
|
||||||
return http.HandlerFunc(
|
// with a credential planted in the body, the query and a header.
|
||||||
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,
|
||||||
target,
|
"/pages/login?url=https://hooks.slack.com/services/"+
|
||||||
strings.NewReader(body),
|
sentryQueryMarker,
|
||||||
|
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
|
||||||
@@ -225,27 +146,15 @@ 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, the CSRF header and the concrete request path, none of
|
// raw query and the CSRF header, none of which SendDefaultPII=false
|
||||||
// which SendDefaultPII=false suppresses.
|
// suppresses.
|
||||||
func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
event := onlyEvent(t, sentryCase{
|
event := captureThroughSentryHTTP(t, false)
|
||||||
panics: true,
|
require.NotNil(t, event.Request)
|
||||||
request: sentryLoginRequest,
|
|
||||||
}.capture(t))
|
|
||||||
|
|
||||||
assert.Contains(
|
assert.Contains(
|
||||||
t, event.Request.Data, sentryBodyMarker,
|
t, event.Request.Data, sentryBodyMarker,
|
||||||
@@ -256,18 +165,6 @@ 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
|
||||||
@@ -276,11 +173,8 @@ func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
|
|||||||
func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
|
func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
event := onlyEvent(t, sentryCase{
|
event := captureThroughSentryHTTP(t, true)
|
||||||
scrub: true,
|
require.NotNil(t, event.Request)
|
||||||
panics: true,
|
|
||||||
request: sentryLoginRequest,
|
|
||||||
}.capture(t))
|
|
||||||
|
|
||||||
encoded := marshalEvent(t, event)
|
encoded := marshalEvent(t, event)
|
||||||
|
|
||||||
@@ -295,45 +189,16 @@ 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, its scheme and host, the method and
|
// the debugging signal: the route, the method and the metadata headers
|
||||||
// the metadata headers still identify what failed. On a static route
|
// still identify what failed.
|
||||||
// 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 := onlyEvent(t, sentryCase{
|
event := captureThroughSentryHTTP(t, true)
|
||||||
scrub: true,
|
require.NotNil(t, event.Request)
|
||||||
panics: true,
|
|
||||||
request: sentryLoginRequest,
|
|
||||||
}.capture(t))
|
|
||||||
|
|
||||||
assert.Equal(
|
assert.Contains(t, event.Request.URL, "/pages/login")
|
||||||
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,
|
||||||
@@ -347,127 +212,6 @@ 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) {
|
||||||
@@ -479,6 +223,5 @@ 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