diff --git a/README.md b/README.md index 7bc0010..77b9bb0 100644 --- a/README.md +++ b/README.md @@ -1020,6 +1020,15 @@ 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. +The query string does not leave the host by the other route either. +The Sentry SDK attaches the request to every event it captures and +copies the raw query into it, independently of the access log, so a +`BeforeSend` hook clears that field before the event is sent. The event +still carries the scheme, host, path and method, which is what names +the failing route. Nothing in this service reads a form field from the +query: every handler uses `PostFormValue`, so a value on the request +line cannot configure anything. + 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..0a9b81f 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,16 @@ 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) +} + // 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..bd972d9 --- /dev/null +++ b/internal/server/sentry.go @@ -0,0 +1,40 @@ +package server + +import "github.com/getsentry/sentry-go" + +// sentryRedactedQuery stands in for the query string on every event +// shipped to Sentry. +const sentryRedactedQuery = "(redacted)" + +// scrubSentryRequest drops the query string from an event's request +// context before it leaves the process. +// +// sentryhttp attaches the whole *http.Request to the scope, and +// sentry.NewRequest copies r.URL.RawQuery verbatim into +// Request.QueryString. That path is independent of the access log: it +// is populated from the request even though the log line for the same +// request records only the route pattern or a redacted query. Any +// error or panic captured while serving a request would therefore ship +// the query string to a third-party service, and a query string is +// client-chosen text that a mistyped or hand-built request can put a +// credential into. +// +// The query is not debugging signal here. One route in the service +// reads a query parameter at all — `page`, on the authenticated +// pagination links in internal/handlers/source_management.go — and +// Request.URL still carries scheme, host and path, which is what +// identifies the failing route. +func scrubSentryRequest( + event *sentry.Event, + _ *sentry.EventHint, +) *sentry.Event { + if event == nil || event.Request == nil { + return event + } + + if event.Request.QueryString != "" { + event.Request.QueryString = sentryRedactedQuery + } + + return event +} diff --git a/internal/server/sentry_test.go b/internal/server/sentry_test.go new file mode 100644 index 0000000..ecaf4ae --- /dev/null +++ b/internal/server/sentry_test.go @@ -0,0 +1,105 @@ +package server_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/getsentry/sentry-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/server" +) + +// sentrySecretSegment is the path segment of a Slack incoming-webhook +// URL — the part that is the bearer credential. No Sentry event may +// carry it. +const sentrySecretSegment = "T00000000/B00000000/QQSENTRYSECRETQQ" + +// sentryEventFor builds the event Sentry would ship for a request +// carrying the given raw query, using the SDK's own request +// conversion rather than a hand-built Request, so the test tracks +// what the SDK actually collects. +func sentryEventFor(t *testing.T, rawQuery string) *sentry.Event { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPost, + "/source/abc/targets?"+rawQuery, + nil, + ) + + event := sentry.NewEvent() + event.Request = sentry.NewRequest(req) + + return event +} + +// TestSentryScrub_QueryStringIsCollectedUnscrubbed pins the reason the +// hook exists: the SDK copies the raw query into the event on its own, +// independently of the access log, which records only the route +// pattern or a redacted query for the same request. +func TestSentryScrub_QueryStringIsCollectedUnscrubbed(t *testing.T) { + t.Parallel() + + event := sentryEventFor( + t, "url=https://hooks.slack.com/services/"+sentrySecretSegment, + ) + + require.Contains( + t, event.Request.QueryString, sentrySecretSegment, + "the SDK is expected to collect the raw query; "+ + "if it no longer does, the scrub hook's premise changed", + ) +} + +// TestSentryScrub_RedactsQueryString is the regression test: the hook +// installed on both BeforeSend and BeforeSendTransaction must leave no +// byte of the query in the event that goes off-host. +func TestSentryScrub_RedactsQueryString(t *testing.T) { + t.Parallel() + + event := sentryEventFor( + t, "url=https://hooks.slack.com/services/"+sentrySecretSegment, + ) + + scrubbed := server.ScrubSentryRequestForTest(event, nil) + require.NotNil(t, scrubbed) + + encoded, err := json.Marshal(scrubbed) + require.NoError(t, err) + + assert.NotContains(t, string(encoded), sentrySecretSegment) + assert.NotContains(t, string(encoded), "hooks.slack.com") +} + +// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost +// the debugging signal: the path still identifies the failing route. +func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { + t.Parallel() + + event := sentryEventFor(t, "page=2") + + scrubbed := server.ScrubSentryRequestForTest(event, nil) + require.NotNil(t, scrubbed) + + assert.Contains(t, scrubbed.Request.URL, "/source/abc/targets") + assert.Equal(t, http.MethodPost, scrubbed.Request.Method) +} + +// 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() + + event := sentry.NewEvent() + + scrubbed := server.ScrubSentryRequestForTest(event, 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..2910597 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -148,6 +148,10 @@ func (s *Server) enableSentry() { s.params.Globals.Appname, s.params.Globals.Version, ), + // Both hooks, because the SDK runs one for error events + // and the other for transactions. + BeforeSend: scrubSentryRequest, + BeforeSendTransaction: scrubSentryRequest, }) if err != nil { s.log.Error("sentry init failure", "error", err)