diff --git a/internal/delivery/target_redact.go b/internal/delivery/target_redact.go new file mode 100644 index 0000000..0607732 --- /dev/null +++ b/internal/delivery/target_redact.go @@ -0,0 +1,136 @@ +package delivery + +import ( + "net/url" + "strings" + + "sneak.berlin/go/webhooker/internal/database" +) + +// RedactionMarker stands in for a target credential found in +// text the target's remote peer chose. +const RedactionMarker = "(redacted)" + +// Redactor removes one target's own credential material from +// text that target's remote peer chose: a delivery response +// body, or a delivery error stored before the delivery path +// learned to mask the URLs it embeds. +// +// It matches literally, against strings taken from the +// target's stored configuration, so it guesses nothing about +// what a secret looks like. That also bounds what it can +// promise: it removes the credential this service handed the +// remote, and it cannot remove a secret the remote invented. +// +// The zero Redactor removes nothing, which is what a caller +// holding no target for a delivery gets. +type Redactor struct { + secrets []string +} + +// NewRedactor builds the redactor for one target. +func NewRedactor(t *database.Target) Redactor { + return Redactor{secrets: targetSecrets(t)} +} + +// Redact replaces every occurrence of the target's credential +// material in s. +func (r Redactor) Redact(s string) string { + if s == "" { + return s + } + + for _, secret := range r.secrets { + s = strings.ReplaceAll(s, secret, RedactionMarker) + } + + return s +} + +// targetSecrets returns the credential-bearing strings a +// target's configuration carries, longest first so that +// replacing one never leaves a fragment of a longer one +// behind. +// +// Only the destination URL contributes. Its path, query and +// userinfo are the credential for both target types that have +// one — an incoming-webhook URL is a bearer token, which is +// why MaskURL elides exactly those parts — and they are the +// material this service actually sends, so a remote that +// echoes the request back echoes them. +// +// Configured HTTP headers are deliberately not included. +// Their values are as often routine as secret (Accept, +// User-Agent), and redacting them from remote text would +// replace ordinary response content with the marker. A remote +// that echoes an Authorization header into its response body +// is therefore not covered. +func targetSecrets(t *database.Target) []string { + if t == nil { + return nil + } + + switch t.Type { + case database.TargetTypeSlack: + cfg, err := parseSlackConfig(t.Config) + if err != nil { + return nil + } + + return urlSecrets(cfg.WebhookURL) + case database.TargetTypeHTTP: + cfg, err := parseHTTPConfig(t.Config) + if err != nil { + return nil + } + + return urlSecrets(cfg.URL) + case database.TargetTypeDatabase, database.TargetTypeLog: + // Neither has a destination URL, so neither has + // anything to redact. + return nil + default: + return nil + } +} + +// urlSecrets returns the substrings of a destination URL that +// must not survive into a rendered page: the whole URL, the +// parts of it MaskURL elides, and any userinfo. +// +// No length floor is applied to the path. A short path is +// treated as a credential exactly like a long one, because +// the field takes an arbitrary URL and no segment can be +// assumed non-secret — the same rule MaskURL applies. +func urlSecrets(raw string) []string { + raw = strings.TrimSpace(raw) + if raw == "" { + return nil + } + + secrets := []string{raw} + + parsed, err := url.Parse(raw) + if err != nil { + return secrets + } + + if parsed.Path != "" && parsed.Path != "/" { + requestURI := parsed.RequestURI() + secrets = append(secrets, requestURI) + + if escaped := parsed.EscapedPath(); escaped != requestURI { + secrets = append(secrets, escaped) + } + } + + if parsed.User != nil { + secrets = append(secrets, parsed.User.String()) + + if pw, ok := parsed.User.Password(); ok && pw != "" { + secrets = append(secrets, pw) + } + } + + return secrets +} diff --git a/internal/delivery/target_redact_test.go b/internal/delivery/target_redact_test.go new file mode 100644 index 0000000..efb2e1b --- /dev/null +++ b/internal/delivery/target_redact_test.go @@ -0,0 +1,137 @@ +package delivery_test + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +// The secret path segments of a Slack incoming webhook URL. +const ( + redactSecretPath = "/services/T11111111/B11111111/" + + "YYYYYYYYYYYYYYYYYYYYYYYY" + redactWebhookURL = "https://hooks.slack.com" + + redactSecretPath +) + +func TestRedactor_RemovesSlackWebhookURL(t *testing.T) { + t.Parallel() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + }) + + got := r.Redact("no_service for " + redactWebhookURL) + + assert.NotContains(t, got, redactSecretPath) + assert.NotContains(t, got, "T11111111") + assert.Contains(t, got, delivery.RedactionMarker) + assert.Contains(t, got, "no_service for ") +} + +// TestRedactor_RemovesBarePath covers a remote that echoes +// only the request path rather than the whole URL. The path +// segments are the credential on their own. +func TestRedactor_RemovesBarePath(t *testing.T) { + t.Parallel() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + }) + + got := r.Redact("POST " + redactSecretPath + " 404") + + assert.NotContains(t, got, redactSecretPath) + assert.Equal( + t, + "POST "+delivery.RedactionMarker+" 404", + got, + ) +} + +// TestRedactor_RemovesHTTPURLQueryAndUserinfo covers the HTTP +// target, whose destination is an arbitrary URL: the query +// string and the userinfo carry credentials as readily as the +// path does. +func TestRedactor_RemovesHTTPURLQueryAndUserinfo(t *testing.T) { + t.Parallel() + + // Assembled rather than written out, so the literal is + // not itself a credential-shaped string. + dest := url.URL{ + Scheme: "https", + User: url.UserPassword("user", "hunter2"), + Host: "example.com", + Path: "/in", + RawQuery: "token=s3cr3t", + } + raw := dest.String() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"` + raw + `"}`, + }) + + for _, echoed := range []string{ + raw, + "/in?token=s3cr3t", + "hunter2", + } { + got := r.Redact("rejected: " + echoed) + + assert.NotContains(t, got, "s3cr3t", echoed) + assert.NotContains(t, got, "hunter2", echoed) + assert.Contains( + t, got, delivery.RedactionMarker, echoed, + ) + } +} + +// TestRedactor_LeavesUnrelatedTextAlone pins that the +// redactor matches literally: it does not guess at what a +// secret looks like, so ordinary response content survives. +func TestRedactor_LeavesUnrelatedTextAlone(t *testing.T) { + t.Parallel() + + const response = "ok=false error=channel_not_found" + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + }) + + assert.Equal(t, response, r.Redact(response)) +} + +// TestRedactor_ZeroValueAndConfiglessTargets pins that a +// caller with no target, an unparseable config, or a target +// type with no destination URL gets a redactor that changes +// nothing rather than one that panics. +func TestRedactor_ZeroValueAndConfiglessTargets(t *testing.T) { + t.Parallel() + + const text = "some response body" + + var zero delivery.Redactor + + assert.Equal(t, text, zero.Redact(text)) + assert.Equal(t, text, delivery.NewRedactor(nil).Redact(text)) + + for _, tgt := range []database.Target{ + {Type: database.TargetTypeLog}, + {Type: database.TargetTypeDatabase}, + {Type: database.TargetTypeSlack, Config: "not json"}, + {Type: database.TargetTypeHTTP, Config: ""}, + } { + assert.Equal( + t, text, + delivery.NewRedactor(&tgt).Redact(text), + tgt.Type, + ) + } +} diff --git a/internal/handlers/delivery_result_view.go b/internal/handlers/delivery_result_view.go new file mode 100644 index 0000000..36935ca --- /dev/null +++ b/internal/handlers/delivery_result_view.go @@ -0,0 +1,126 @@ +package handlers + +import ( + "sneak.berlin/go/webhooker/internal/delivery" +) + +// maxRenderedResponseBytes caps how many bytes of one stored +// delivery response body reach the event log page. +// +// It matches the cap the delivery engine applies when it +// records a result, so nothing written by the current engine +// is cut twice. The bound is enforced here anyway, and in +// SQL: this page's memory profile must not depend on a +// constant in another package staying where it is, and rows +// predating that cap or restored from an archive are not +// covered by it at all. +const maxRenderedResponseBytes = 4096 + +// deliveryResultColumns is the delivery attempt projection. +// The casts to blob are load-bearing for the same reason they +// are in eventLogColumns: they make substr and length count +// bytes rather than characters, and they make SQLite do the +// cut, so an oversized stored response never becomes a Go +// string at all. +const deliveryResultColumns = "delivery_id, attempt_num, success, " + + "status_code, error, duration, " + + "substr(cast(response_body as blob), 1, ?) AS response_body, " + + "length(cast(response_body as blob)) AS response_bytes" + +// DeliveryResultView is the display-safe projection of one +// delivery attempt for the event log page. It carries a +// capped response body plus the true stored size, so the page +// can mark a response as truncated without holding the whole +// thing. +// +// Both Error and ResponseBody have been through the target's +// Redactor. The engine already masks the URL out of the +// errors it stores, so for errors this is a second line +// covering rows written before it did; for response bodies it +// is the only line, and its reach is what +// delivery.Redactor documents. +type DeliveryResultView struct { + AttemptNum int + Success bool + + // StatusCode is 0 when the attempt never got a response, + // which is why the page asks HasStatusCode rather than + // printing the number. + StatusCode int + + // Error is the stored failure message, redacted. + Error string + + // DurationMS is how long the attempt took. + DurationMS int64 + + // ResponseBody holds at most maxRenderedResponseBytes + // bytes of the stored response, redacted. It is remote + // content and must only ever be rendered escaped. + ResponseBody string + + // ResponseBytes is the true size of the stored response + // body, before the cut and before redaction. + ResponseBytes int64 + + // ResponseShownBytes is how much of that the page is + // showing. It is the size of the cut, taken before + // redaction, so the truncation marker reports what SQLite + // returned rather than how much the marker substitution + // then changed the length. + ResponseShownBytes int + + // ResponseTruncated reports that the stored response was + // larger than the cap, so the page owes the reader a + // marker. + ResponseTruncated bool +} + +// HasStatusCode reports whether the attempt got as far as an +// HTTP response. A transport failure stores no status code, +// and rendering that as "0" would read as a real status. +func (v DeliveryResultView) HasStatusCode() bool { + return v.StatusCode != 0 +} + +// deliveryResultRow is one row of the delivery attempt +// projection. Its response body arrives already cut to the +// cap by SQLite, with the true size beside it. +type deliveryResultRow struct { + DeliveryID string + AttemptNum int + Success bool + StatusCode int + Error string + Duration int64 + ResponseBody []byte + ResponseBytes int64 +} + +// view projects a loaded row for rendering, stripping the +// target's own credential out of the two fields a remote peer +// gets to influence. +func (r *deliveryResultRow) view( + redactor delivery.Redactor, +) DeliveryResultView { + body := r.ResponseBody + truncated := r.ResponseBytes > int64(len(body)) + + // Only a cut response can have been left mid-sequence by + // this query, exactly as with an event body. + if truncated { + body = trimPartialRune(body) + } + + return DeliveryResultView{ + AttemptNum: r.AttemptNum, + Success: r.Success, + StatusCode: r.StatusCode, + Error: redactor.Redact(r.Error), + DurationMS: r.Duration, + ResponseBody: redactor.Redact(string(body)), + ResponseBytes: r.ResponseBytes, + ResponseShownBytes: len(body), + ResponseTruncated: truncated, + } +} diff --git a/internal/handlers/delivery_result_view_test.go b/internal/handlers/delivery_result_view_test.go new file mode 100644 index 0000000..a48d216 --- /dev/null +++ b/internal/handlers/delivery_result_view_test.go @@ -0,0 +1,306 @@ +package handlers_test + +import ( + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm/clause" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" + "sneak.berlin/go/webhooker/internal/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +// responseCap is the number of response bytes the event log +// page is allowed to render for one delivery attempt. +const responseCap = handlers.MaxRenderedResponseBytesForTest + +// failedAttempt describes the failed delivery every test in +// this file seeds. The values are distinctive so that finding +// them in the rendered page cannot be a coincidence. +const ( + attemptStatusCode = 502 + attemptDurationMS = 1234 + attemptNumber = 3 + attemptError = "upstream returned 502 Bad Gateway" +) + +// seedFailedDelivery records an event, a failed delivery +// against targetID, and one delivery result carrying the +// given response body. It returns the delivery. +func seedFailedDelivery( + t *testing.T, + dbMgr *database.WebhookDBManager, + webhookID, targetID, responseBody string, +) *database.Delivery { + t.Helper() + + webhookDB, err := dbMgr.GetDB(webhookID) + require.NoError(t, err) + + event := &database.Event{ + WebhookID: webhookID, + Method: http.MethodPost, + Body: `{"test":true}`, + ContentType: "application/json", + } + + require.NoError(t, webhookDB.Omit( + clause.Associations, + ).Create(event).Error) + + dlv := &database.Delivery{ + EventID: event.ID, + TargetID: targetID, + Status: database.DeliveryStatusFailed, + } + + require.NoError(t, webhookDB.Omit( + clause.Associations, + ).Create(dlv).Error) + + result := &database.DeliveryResult{ + DeliveryID: dlv.ID, + AttemptNum: attemptNumber, + Success: false, + StatusCode: attemptStatusCode, + ResponseBody: responseBody, + Error: attemptError, + Duration: attemptDurationMS, + } + + require.NoError(t, webhookDB.Omit( + clause.Associations, + ).Create(result).Error) + + return dlv +} + +// seedFailureAndRender seeds a failed delivery against a +// target of the given type and config, and returns the +// rendered event log page. +func seedFailureAndRender( + t *testing.T, + targetType database.TargetType, + config, responseBody string, +) string { + t.Helper() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + dbMgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &sess, &db, &dbMgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + tgt := seedConfiguredTarget( + t, db, wh.ID, targetType, config, + ) + + seedFailedDelivery(t, dbMgr, wh.ID, tgt.ID, responseBody) + + return renderSourceLogsPage(t, h, sess, wh.ID) +} + +// TestHandleSourceLogs_RendersFailedAttempt is the regression +// test for the reported gap: a failed delivery used to render +// as the status word alone, so diagnosing it meant opening the +// per-webhook SQLite file by hand. +func TestHandleSourceLogs_RendersFailedAttempt(t *testing.T) { + t.Parallel() + + body := seedFailureAndRender( + t, + database.TargetTypeHTTP, + `{"url":"https://example.com/hook/abc"}`, + "upstream exploded", + ) + + assert.Contains( + t, body, strconv.Itoa(attemptStatusCode), + "the attempt's status code must reach the page", + ) + assert.Contains( + t, body, attemptError, + "the attempt's error must reach the page", + ) + assert.Contains( + t, body, strconv.Itoa(attemptDurationMS), + "the attempt's duration must reach the page", + ) + assert.Contains( + t, body, "Attempt "+strconv.Itoa(attemptNumber), + "the attempt number must reach the page", + ) + assert.Contains( + t, body, "upstream exploded", + "the attempt's response body must reach the page", + ) +} + +// TestHandleSourceLogs_EscapesResponseBody proves the +// response body is treated as the untrusted remote content it +// is. The remote chooses these bytes and the page is rendered +// inside the operator's authenticated origin, where the +// application's own CSP allows inline script from 'self'. +func TestHandleSourceLogs_EscapesResponseBody(t *testing.T) { + t.Parallel() + + const payload = `` + + body := seedFailureAndRender( + t, + database.TargetTypeHTTP, + `{"url":"https://example.com/hook/abc"}`, + payload, + ) + + assert.NotContains(t, body, payload) + assert.NotContains(t, body, "