diff --git a/README.md b/README.md index fcdf1fe..988201a 100644 --- a/README.md +++ b/README.md @@ -1022,6 +1022,33 @@ buy the same amplification as an invented path. Nothing debuggable is lost: `page`, on the authenticated pagination links, is the only query parameter this service reads. +Client-supplied request content does not leave the host by the other +route either. The Sentry SDK attaches the request to every event it +captures, independently of the access log, and `SendDefaultPII=false` +does not cover all of what it copies: the raw query string and the +first 10 KiB of the request body are both taken unconditionally, the +body precisely because these handlers call `ParseForm`. A `BeforeSend` +hook therefore replaces the query string and the body with +`(redacted)`, drops cookies and the remote-address environment, and +reduces the headers to a fixed allowlist — `Accept`, `Content-Length`, +`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and +`X-Request-Id`. + +The body is replaced rather than filtered because the hook cannot tell +which route it is on: the SDK hands `BeforeSend` no request, so a +route-conditional rule would have to guess, and an unrecognised route +must not leak. Nothing debuggable is lost by it. 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 on the receiver route, the one route +whose body is genuine signal, that body is already stored on the event +and served from the UI. The headers are an allowlist for the same +reason: 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, path, method — and `X-Request-Id` ties the event +to the local access log line that holds the rest. + The remaining client-supplied fields are truncated rather than dropped, each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`, 128 for `request_id` (chi passes an inbound `X-Request-Id` header diff --git a/internal/database/model_apikey.go b/internal/database/model_apikey.go index a1bd96b..5e8cee7 100644 --- a/internal/database/model_apikey.go +++ b/internal/database/model_apikey.go @@ -2,12 +2,16 @@ package database import "time" -// APIKey represents an API key for a user +// APIKey represents an API key for a user. +// +// Key is a bearer credential, so it is never marshalled with the +// model. A creation handler that has to show it once returns it in its +// own response type. type APIKey struct { BaseModel UserID string `gorm:"type:uuid;not null" json:"userId"` - Key string `gorm:"uniqueIndex;not null" json:"key"` + Key string `gorm:"uniqueIndex;not null" json:"-"` Description string `json:"description"` LastUsedAt *time.Time `json:"lastUsedAt,omitempty"` diff --git a/internal/database/model_secrets_test.go b/internal/database/model_secrets_test.go new file mode 100644 index 0000000..84bd98f --- /dev/null +++ b/internal/database/model_secrets_test.go @@ -0,0 +1,107 @@ +package database_test + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" +) + +// keptField is a non-secret value planted alongside each secret, so +// the assertions below cannot pass by the model marshalling to nothing. +const keptField = "keepme" + +// marshalModel encodes a model the way a future JSON handler would. +func marshalModel(t *testing.T, v any) string { + t.Helper() + + encoded, err := json.Marshal(v) + require.NoError(t, err) + + return string(encoded) +} + +// TestModelsDoNotMarshalTheirSecrets pins the barrier for the JSON +// path. The /api/v1 route group exists and is empty; delivery's +// TargetView masks the credential for the HTML path only, so without +// these tags the first handler that marshals a model serialises the +// secret with it. Each field below is a live credential: +// +// - Target.Config holds an incoming-webhook URL whose path segments +// are the bearer token. +// - APIKey.Key is a bearer token outright. +// - Setting.Value holds the session encryption key. +// - User.Password holds the Argon2 hash, and was already tagged. +func TestModelsDoNotMarshalTheirSecrets(t *testing.T) { + t.Parallel() + + const marker = "QQMODELMARKERQQ" + + cases := []struct { + name string + model any + }{ + { + name: "target config", + model: database.Target{ + Name: keptField, + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + }, + }, + { + name: "api key", + model: database.APIKey{ + Description: keptField, + Key: marker, + }, + }, + { + name: "setting value", + model: database.Setting{ + Key: keptField, + Value: marker, + }, + }, + { + name: "user password hash", + model: database.User{ + Username: keptField, + Password: marker, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + encoded := marshalModel(t, tc.model) + + assert.NotContains(t, encoded, marker) + assert.Contains(t, encoded, keptField) + }) + } +} + +// TestWebhookMarshalsNoTargetConfig covers the nested case: a webhook +// marshalled with its targets preloaded must not carry the credential +// through the association either. +func TestWebhookMarshalsNoTargetConfig(t *testing.T) { + t.Parallel() + + const marker = "QQNESTEDMARKERQQ" + + encoded := marshalModel(t, database.Webhook{ + Name: keptField, + Targets: []database.Target{{ + Name: "slack", + Config: `{"webhookUrl":"https://h/s/` + marker + `"}`, + }}, + }) + + assert.NotContains(t, encoded, marker) + assert.Contains(t, encoded, keptField) +} diff --git a/internal/database/model_setting.go b/internal/database/model_setting.go index f120fec..cdd068b 100644 --- a/internal/database/model_setting.go +++ b/internal/database/model_setting.go @@ -3,6 +3,9 @@ package database // Setting stores application-level key-value configuration. // Used for auto-generated values like the session encryption key. type Setting struct { - Key string `gorm:"primaryKey" json:"key"` - Value string `gorm:"type:text;not null" json:"value"` + Key string `gorm:"primaryKey" json:"key"` + + // Value holds the session encryption key, so it is never + // marshalled with the model. + Value string `gorm:"type:text;not null" json:"-"` } diff --git a/internal/database/model_target.go b/internal/database/model_target.go index 9c8aa83..9c5f95f 100644 --- a/internal/database/model_target.go +++ b/internal/database/model_target.go @@ -20,8 +20,14 @@ type Target struct { Type TargetType `gorm:"not null" json:"type"` Active bool `gorm:"default:true" json:"active"` - // Configuration fields (JSON stored based on type) - Config string `gorm:"type:text" json:"config"` // JSON configuration + // Configuration fields (JSON stored based on type). + // + // json:"-" because the blob holds the target's credential — a + // Slack incoming-webhook URL, or an http destination whose path + // segments are the secret. delivery.TargetView is the masking + // barrier for the HTML path; this tag is the barrier for any + // handler that marshals the model itself. + Config string `gorm:"type:text" json:"-"` // JSON configuration // For HTTP targets (max_retries=0 means fire-and-forget, // >0 enables retries with backoff) diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index e79d7f5..20f2310 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -39,8 +39,10 @@ func (h *Handlers) HandleLoginSubmit() http.HandlerFunc { return } - username := r.FormValue("username") - password := r.FormValue("password") + // PostFormValue, not FormValue: the credential must come + // from the body, never from the query string. + username := r.PostFormValue("username") + password := r.PostFormValue("password") // Validate input if username == "" || password == "" { diff --git a/internal/handlers/profile.go b/internal/handlers/profile.go index 027027e..dde5c71 100644 --- a/internal/handlers/profile.go +++ b/internal/handlers/profile.go @@ -44,9 +44,11 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc { successMessage, errorMessage, handled := h.applyPasswordChange( w, sessionUsername, - r.FormValue("current_password"), - r.FormValue("new_password"), - r.FormValue("confirm_password"), + // PostFormValue, not FormValue: the credential must + // come from the body, never from the query string. + r.PostFormValue("current_password"), + r.PostFormValue("new_password"), + r.PostFormValue("confirm_password"), ) if !handled { return diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 0243b76..5e4602c 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -227,9 +227,9 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { return } - name := r.FormValue("name") - description := r.FormValue("description") - retentionStr := r.FormValue("retention_days") + name := r.PostFormValue("name") + description := r.PostFormValue("description") + retentionStr := r.PostFormValue("retention_days") if name == "" { w.WriteHeader(http.StatusBadRequest) @@ -509,7 +509,7 @@ func (h *Handlers) applyWebhookEdit( ) { // The body size cap is enforced by the MaxBodySize middleware, // which runs before CSRF parses the form. - name := r.FormValue("name") + name := r.PostFormValue("name") if name == "" { data := map[string]any{ tmplKeyWebhook: webhook, @@ -523,12 +523,12 @@ func (h *Handlers) applyWebhookEdit( } webhook.Name = name - webhook.Description = r.FormValue("description") + webhook.Description = r.PostFormValue("description") // An empty field falls back to the stored value, so submitting the // form without touching retention leaves the policy alone. retentionDays, retErr := parseRetentionDays( - r.FormValue("retention_days"), webhook.RetentionDays, + r.PostFormValue("retention_days"), webhook.RetentionDays, ) if retErr != nil { data := map[string]any{ @@ -950,7 +950,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { return } - description := r.FormValue("description") + description := r.PostFormValue("description") entrypoint := &database.Entrypoint{ WebhookID: webhook.ID, @@ -1020,11 +1020,18 @@ func (h *Handlers) processTargetCreate( ) { // The body size cap is enforced by the MaxBodySize middleware, // which runs before CSRF parses the form. - name := r.FormValue("name") - targetType := database.TargetType(r.FormValue("type")) - targetURL := r.FormValue("url") - maxRetriesStr := r.FormValue("max_retries") - expiry := r.FormValue("expiry") + // + // Every field here is read with PostFormValue, not FormValue. + // FormValue falls back to the query string, which would let + // `POST /source/{id}/targets?url=https://hooks.slack.com/...` + // configure a target from a value the request line carries — and + // the request line, unlike the body, is what logs, proxies, + // Referer headers and error trackers record. + name := r.PostFormValue("name") + targetType := database.TargetType(r.PostFormValue("type")) + targetURL := r.PostFormValue("url") + maxRetriesStr := r.PostFormValue("max_retries") + expiry := r.PostFormValue("expiry") if name == "" { http.Error( diff --git a/internal/handlers/target_create_query_test.go b/internal/handlers/target_create_query_test.go new file mode 100644 index 0000000..84b0b5e --- /dev/null +++ b/internal/handlers/target_create_query_test.go @@ -0,0 +1,206 @@ +package handlers_test + +import ( + "bytes" + "context" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/go-chi/chi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/middleware" +) + +// targetSecretSegments are the path segments of an incoming-webhook +// URL. For Slack, Discord and Teams the path IS the bearer credential, +// so this string must not reach storage or the access log by way of +// the request line. +const targetSecretSegments = "T00000000/B00000000/QQTARGETSECRETQQ" + +// targetSecretURL is a destination whose secret lives in its path. It +// uses a literal public address rather than a hostname so the SSRF +// check resolves nothing: with a hostname, a sandbox without DNS would +// reject the URL for the wrong reason and the test would pass even +// with the defect reintroduced. +const targetSecretURL = "https://93.184.216.34/services/" + + targetSecretSegments + +// targetsForWebhook returns every target stored against a webhook. +func targetsForWebhook( + t *testing.T, + db *database.Database, + webhookID string, +) []database.Target { + t.Helper() + + var targets []database.Target + + require.NoError( + t, + db.DB().Where("webhook_id = ?", webhookID). + Find(&targets).Error, + ) + + return targets +} + +// postTargetCreate drives HandleTargetCreate through the production +// access-log middleware and a chi route, so the logged url field is +// produced exactly as it ships, and returns the recorder plus the +// captured log. +func postTargetCreate( + t *testing.T, + env *sourceTestEnv, + webhookID string, + query string, + form url.Values, +) (*httptest.ResponseRecorder, string) { + t.Helper() + + logBuf := new(bytes.Buffer) + mw := middleware.NewForTest( + slog.New(slog.NewJSONHandler( + logBuf, &slog.HandlerOptions{Level: slog.LevelInfo}, + )), + &config.Config{Environment: config.EnvironmentDev}, + nil, + ) + + router := chi.NewRouter() + router.Use(mw.Logging()) + router.Post( + "/source/{sourceID}/targets", + env.handlers.HandleTargetCreate(), + ) + + target := "/source/" + webhookID + "/targets" + if query != "" { + target += "?" + query + } + + body := "" + if form != nil { + body = form.Encode() + } + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, + target, + strings.NewReader(body), + ) + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + + for _, c := range env.cookies { + req.AddCookie(c) + } + + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + return w, logBuf.String() +} + +// TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget is the +// regression test for the ingress leak. r.FormValue falls back to the +// query string when a field is absent from the POST body, so +// +// POST /source/{id}/targets?url=https://hooks.slack.com/services/... +// +// with an empty url field used to create a working target from a value +// carried on the request line — where logs, proxies, Referer headers +// and error trackers record it. The handler reads the body only, so +// the request is rejected for a missing URL and stores nothing. +// +// name and type are sent in the BODY on purpose: the request has to +// get past those two validations for the assertion to be about the url +// read specifically. +func TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + webhook := seedWebhookWithRetention(t, env.db, 30) + + body := url.Values{} + body.Set("name", "leaky") + body.Set("type", string(database.TargetTypeSlack)) + + w, logged := postTargetCreate( + t, env, webhook.ID, + "url="+url.QueryEscape(targetSecretURL), + body, + ) + + assert.Equal(t, http.StatusBadRequest, w.Code) + + targets := targetsForWebhook(t, env.db, webhook.ID) + assert.Empty( + t, targets, + "a query-string value must not populate a target config", + ) + + assert.NotContains(t, logged, targetSecretSegments) + assert.NotContains(t, logged, "93.184.216.34") + assert.NotEmpty(t, logged, "the access log line must still be written") +} + +// TestHandleTargetCreate_BodyURLStillCreatesTheTarget is the positive +// control for the test above: the rejection has to come from where the +// value was read, not from the handler being broken. +func TestHandleTargetCreate_BodyURLStillCreatesTheTarget(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + webhook := seedWebhookWithRetention(t, env.db, 30) + + body := url.Values{} + body.Set("name", "legit") + body.Set("type", string(database.TargetTypeSlack)) + body.Set("url", targetSecretURL) + + w, logged := postTargetCreate(t, env, webhook.ID, "", body) + + assert.Equal(t, http.StatusSeeOther, w.Code) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Contains(t, targets[0].Config, targetSecretSegments) + + // The body carried the credential, so the access log must still + // not have it: the log records the request line only. + assert.NotContains(t, logged, targetSecretSegments) +} + +// TestHandleTargetCreate_QueryStringCannotSupplyNameOrType covers the +// rest of the converted reads on this handler in one request: with an +// empty body, nothing the query carries is visible to it. +func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + webhook := seedWebhookWithRetention(t, env.db, 30) + + w, _ := postTargetCreate( + t, env, webhook.ID, + "name=leaky&type=slack&max_retries=9&expiry=30d&url="+ + url.QueryEscape(targetSecretURL), + url.Values{}, + ) + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.Contains(t, w.Body.String(), "Name is required") + assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID)) +} diff --git a/internal/server/export_test.go b/internal/server/export_test.go index a4ed0ca..2118a1d 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -4,6 +4,7 @@ import ( "log/slog" "net/http" + "github.com/getsentry/sentry-go" "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/middleware" @@ -13,6 +14,25 @@ import ( // build requests that sit exactly at, below, and above it. const MaxFormBodySizeForTest = maxFormBodySize +// ScrubSentryRequestForTest exposes the BeforeSend hook that +// enableSentry installs, so a test can assert on what it leaves in an +// event without standing up a Sentry client. +func ScrubSentryRequestForTest( + event *sentry.Event, + hint *sentry.EventHint, +) *sentry.Event { + return scrubSentryRequest(event, hint) +} + +// SentryClientOptionsForTest exposes the exact options enableSentry +// initialises the SDK with, so a test can capture events through the +// production hook wiring rather than a hand-built equivalent. +func SentryClientOptionsForTest( + dsn, release string, +) sentry.ClientOptions { + return sentryClientOptions(dsn, release) +} + // NewRouterForTest builds the real route tree via SetupRoutes with // the supplied middleware and handlers, bypassing the fx lifecycle // and the HTTP listener. Tests use it so that route-group middleware diff --git a/internal/server/sentry.go b/internal/server/sentry.go new file mode 100644 index 0000000..af795aa --- /dev/null +++ b/internal/server/sentry.go @@ -0,0 +1,117 @@ +package server + +import ( + "net/http" + + "github.com/getsentry/sentry-go" +) + +// sentryRedacted stands in for a withheld field on every event shipped +// to Sentry. It is a marker rather than an empty string so a reader +// can tell a suppressed value from an absent one. +const sentryRedacted = "(redacted)" + +// sentryClientOptions builds the options the SDK is initialised with. +// It is its own function so a test can stand up a client wired exactly +// as production is, with only the transport swapped. +func sentryClientOptions(dsn, release string) sentry.ClientOptions { + return sentry.ClientOptions{ + Dsn: dsn, + Release: release, + // Both hooks, because the SDK runs one for error events + // and the other for transactions. + BeforeSend: scrubSentryRequest, + BeforeSendTransaction: scrubSentryRequest, + } +} + +// scrubSentryRequest strips client-supplied content from an event's +// request context before it leaves the process. +// +// sentryhttp attaches the whole *http.Request to the scope +// (sentryhttp.go:113), and Scope.ApplyToEvent fills the event's +// Request from it inside prepareEvent, which runs before this hook. +// Two of the fields it fills are copied with no SendDefaultPII guard: +// +// - QueryString, verbatim from r.URL.RawQuery. +// - Data, the first 10 KiB of the request body, teed off r.Body by +// SetRequest and filled precisely because the handlers call +// ParseForm. +// +// Since every form field in this service is read with PostFormValue, +// the body is the only place a credential is submitted: a target's +// destination URL, whose path segments are the bearer token, plus the +// login password and both password-change fields. None of that may +// reach a third-party service. +// +// This hook is a floor, not a default: the fields it clears stay +// cleared even if SendDefaultPII is ever turned on. +func scrubSentryRequest( + event *sentry.Event, + _ *sentry.EventHint, +) *sentry.Event { + if event == nil || event.Request == nil { + return event + } + + req := event.Request + + if req.QueryString != "" { + req.QueryString = sentryRedacted + } + + if req.Data != "" { + req.Data = sentryRedacted + } + + req.Cookies = "" + req.Env = nil + req.Headers = keptSentryHeaders(req.Headers) + + return event +} + +// keptSentryHeaders returns the subset of headers an event may carry +// off-host. Dropping by allowlist rather than by blocklist is what +// makes an unrecognised header safe: the SDK's own filter removes four +// names and passes everything else, so X-Csrf-Token — which +// gorilla/csrf accepts in place of the form field — and the shared +// secrets senders put on the receiver route (X-Gitlab-Token and the +// per-provider signature headers) would otherwise ship verbatim. +func keptSentryHeaders(headers map[string]string) map[string]string { + if len(headers) == 0 { + return headers + } + + kept := make(map[string]string, len(headers)) + + for name, value := range headers { + if sentryKeepsHeader(name) { + kept[name] = value + } + } + + return kept +} + +// sentryKeepsHeader reports whether a request header is routing or +// content metadata rather than client-chosen payload. Referer is kept +// on the reasoning that it is browser-set, that this service emits +// only ?page= in its own links, and that Referrer-Policy is set to +// strict-origin-when-cross-origin. X-Request-Id ties the event to the +// local access log line, which holds the rest of the detail. +func sentryKeepsHeader(name string) bool { + switch http.CanonicalHeaderKey(name) { + case "Accept", + "Content-Length", + "Content-Type", + "Host", + "Origin", + "Referer", + "User-Agent", + "X-Request-Id": + return true + default: + return false + } +} diff --git a/internal/server/sentry_test.go b/internal/server/sentry_test.go new file mode 100644 index 0000000..6ebb756 --- /dev/null +++ b/internal/server/sentry_test.go @@ -0,0 +1,227 @@ +package server_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + "time" + + "github.com/getsentry/sentry-go" + sentryhttp "github.com/getsentry/sentry-go/http" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/server" +) + +// The three markers below are the credentials a captured event could +// carry off-host, one per field of sentry.Request that the SDK fills +// from the request without a SendDefaultPII guard. +const ( + // sentryBodyMarker is submitted as a form value. Since every + // handler reads its fields with PostFormValue, the body is the + // only place a password or a target URL is ever supplied. + sentryBodyMarker = "QQSENTRYBODYMARKERQQ" + + // sentryQueryMarker rides the request line. + sentryQueryMarker = "T00000000/B00000000/QQSENTRYQUERYMARKERQQ" + + // sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf + // accepts in place of the form field. + sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ" +) + +// sentryKeptUserAgent is a non-secret header value planted so the +// assertions below cannot pass by the event carrying no headers at +// all. +const sentryKeptUserAgent = "webhooker-test-agent" + +// captureTransport records events instead of shipping them, so a test +// sees exactly the payload the SDK would have put on the wire. +type captureTransport struct { + mu sync.Mutex + events []*sentry.Event +} + +func (c *captureTransport) Configure(sentry.ClientOptions) {} + +func (c *captureTransport) Flush(time.Duration) bool { return true } + +func (c *captureTransport) SendEvent(event *sentry.Event) { + c.mu.Lock() + defer c.mu.Unlock() + + c.events = append(c.events, event) +} + +// captureThroughSentryHTTP panics inside a form handler wrapped in the +// real sentryhttp middleware and returns the event the SDK produced. +// +// This is the only construction path on which Request.Data appears: +// sentryhttp calls Scope.SetRequest, which tees r.Body into a 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. +// +// scrub selects whether the production BeforeSend hooks are installed, +// so the same path shows both what the SDK collects and what survives. +func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event { + t.Helper() + + transport := &captureTransport{} + + opts := server.SentryClientOptionsForTest( + "https://public@sentry.invalid/1", "webhooker-test", + ) + opts.Transport = transport + + if !scrub { + opts.BeforeSend = nil + opts.BeforeSendTransaction = nil + } + + client, err := sentry.NewClient(opts) + require.NoError(t, err) + + 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() + + panic("boom") + }), + ) + + 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, +// with a credential planted in the body, the query and a header. +func sentryLoginRequest(client *sentry.Client) *http.Request { + form := url.Values{} + form.Set("username", "admin") + form.Set("password", sentryBodyMarker) + + req := httptest.NewRequestWithContext( + sentry.SetHubOnContext( + context.Background(), + sentry.NewHub(client, sentry.NewScope()), + ), + http.MethodPost, + "/pages/login?url=https://hooks.slack.com/services/"+ + sentryQueryMarker, + strings.NewReader(form.Encode()), + ) + + req.Header.Set( + "Content-Type", "application/x-www-form-urlencoded", + ) + req.Header.Set("X-Csrf-Token", sentryHeaderMarker) + req.Header.Set("User-Agent", sentryKeptUserAgent) + + return req +} + +// marshalEvent encodes an event the way the transport does. +func marshalEvent(t *testing.T, event *sentry.Event) string { + t.Helper() + + encoded, err := json.Marshal(event) + require.NoError(t, err) + + return string(encoded) +} + +// TestSentryScrub_SDKCollectsTheRequestUnscrubbed pins the premise 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 +// suppresses. +func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) { + t.Parallel() + + event := captureThroughSentryHTTP(t, false) + require.NotNil(t, event.Request) + + assert.Contains( + t, event.Request.Data, sentryBodyMarker, + "the SDK is expected to collect the POST body; if it no "+ + "longer does, the scrub hook's premise changed", + ) + assert.Contains(t, event.Request.QueryString, sentryQueryMarker) + assert.Contains( + t, marshalEvent(t, event), sentryHeaderMarker, + ) +} + +// TestSentryScrub_RedactsTheCapturedRequest is the regression test: no +// byte of any planted credential may survive into the marshalled event +// that leaves the process. +func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) { + t.Parallel() + + event := captureThroughSentryHTTP(t, true) + require.NotNil(t, event.Request) + + encoded := marshalEvent(t, event) + + assert.NotContains(t, encoded, sentryBodyMarker) + assert.NotContains(t, encoded, sentryQueryMarker) + assert.NotContains(t, encoded, sentryHeaderMarker) + assert.NotContains(t, encoded, "hooks.slack.com") + + assert.Equal(t, "(redacted)", event.Request.Data) + assert.Equal(t, "(redacted)", event.Request.QueryString) + assert.Empty(t, event.Request.Cookies) + assert.Empty(t, event.Request.Env) +} + +// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost +// the debugging signal: the route, the method and the metadata headers +// still identify what failed. +func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { + t.Parallel() + + event := captureThroughSentryHTTP(t, true) + require.NotNil(t, event.Request) + + assert.Contains(t, event.Request.URL, "/pages/login") + assert.Equal(t, http.MethodPost, event.Request.Method) + assert.Equal( + t, + sentryKeptUserAgent, + event.Request.Headers["User-Agent"], + ) + assert.Equal( + t, + "application/x-www-form-urlencoded", + event.Request.Headers["Content-Type"], + ) +} + +// TestSentryScrub_ToleratesEventsWithoutARequest covers the events the +// hook sees outside an HTTP handler, where no request is attached. +func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) { + t.Parallel() + + scrubbed := server.ScrubSentryRequestForTest( + sentry.NewEvent(), nil, + ) + + require.NotNil(t, scrubbed) + assert.Nil(t, scrubbed.Request) + assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil)) +} diff --git a/internal/server/server.go b/internal/server/server.go index 5246198..0bc2f4f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -141,14 +141,14 @@ func (s *Server) enableSentry() { return } - err := sentry.Init(sentry.ClientOptions{ - Dsn: s.params.Config.SentryDSN, - Release: fmt.Sprintf( + err := sentry.Init(sentryClientOptions( + s.params.Config.SentryDSN, + fmt.Sprintf( "%s-%s", s.params.Globals.Appname, s.params.Globals.Version, ), - }) + )) if err != nil { s.log.Error("sentry init failure", "error", err) // Don't use fatal since we still want the service to run