Compare commits

1 Commits

Author SHA1 Message Date
4884581fc5 Bound every slog line against client-chosen text (closes #176)
All checks were successful
check / check (push) Successful in 2m53s
MaxBodySize logged r.URL.Path untruncated at WARN, and routes.go
registers it ahead of RequireAuth, so an unauthenticated
POST /source/<8 KB>/edit with an oversize declared Content-Length wrote
attacker-chosen text of attacker-chosen length into the operator's log,
for the cost of a request with no body. The 2,560-byte per-line budget
from #146 did not reach it: that budget lives in the access log's field
capping and this is a separate slog call.

The capping mechanism moves out of internal/middleware into
internal/logfield so there is one budget and one implementation rather
than a second ad-hoc truncation. Truncate and EncodedBytes are
unchanged; the access log now spends logfield.MaxBytes where it spent
maxLogFieldBytes.

The sweep the issue asked for found five more call sites of the same
shape, all reachable unauthenticated, all now capped: the CSRF 403
(also registered ahead of RequireAuth), the rate limiters' 429 (the
per-entrypoint receiver limiter is unauthenticated), RequireAuth's own
DEBUG line, the unknown-entrypoint DEBUG line on the receiver, and the
failed-login DEBUG lines. 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. Every other slog call in the tree was read
and judged; the PR body lists all of them, including the ones left
alone and why.

MaxBodySize stays ahead of RequireAuth. An oversize body should be
refused before the request buys a cookie decrypt and a session load,
and rejecting first is what keeps an unauthenticated flood from
choosing how much session work the process does. The ordering and what
it costs are now written at the registration, on maxFormBodySize.

MaxAccessLogLineBytes is restated as the ceiling on every slog line
carrying text an UNAUTHENTICATED client supplies, not just the access
log's: each of these lines carries strictly fewer client-supplied
fields than the access log does, so none can be wider. That is asserted
per line under both handlers rather than argued. The claim is qualified
rather than universal because three kinds of writer are outside it, and
the README and the constant now name all three: lines carrying an
authenticated operator's own input, which are not truncated at all (the
webhook name on "webhook created" reaches 600 KB on one line from a
100 KB form field, measured; the SSRF-rejection url and the target_name
lines are the same shape) and are left uncapped deliberately, since
truncating the operator's own configuration echoed back costs
debuggability against no adversary; the log delivery target, which
exists to emit the whole event; and GORM's default logger, which prints
the interpolated SQL to stdout on a record-not-found and is unbounded
on the receiver and login lookups. That last one is a real defect this
audit turned up and is filed separately as #178, not fixed here.

Tests drive 8 KB of client-chosen text at all six sites, through both
handlers internal/logger can install and through each character they
escape — including a bare C0 control, which costs six bytes on the line
against the one it cost to send and is the case a raw-byte budget
breaks on first. Each holds the encoded line to the ceiling, holds the
whole flood's output to what that ceiling allows, and asserts the
markers at the far end of the input are absent, so a value that merely
happened to be short cannot pass. The two login lines past the username
lookup, capped for uniformity rather than need, are pinned too.
internal/logfield gains a test that measures the per-rune charge
against what the handlers really emit over roughly 3,000 code points on
each, so an undercharged rune fails a test instead of quietly
falsifying the ceiling.

Verified by mutation: reverting the MaxBodySize cap alone fails 28
subtests with a 16,583-byte line against the 2,560 ceiling; reverting
the other five fails 70; uncapping the two login lines past the lookup
fails 2; budgeting raw bytes instead of encoded ones fails 23 across
three packages.
2026-08-18 00:24:08 +00:00
6 changed files with 81 additions and 522 deletions

110
README.md
View File

@@ -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

View File

@@ -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,

View File

@@ -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,
),
) )
} }

View File

@@ -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

View File

@@ -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

View File

@@ -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) {
// 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()
return transport.events
}
// 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()
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))
} }