From 3296b166b14033a5d851333ae043698789deb476 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 04:14:20 +0000 Subject: [PATCH] Render delivery attempt detail in the event log (closes #202) Expanding a delivery on the event log page now shows each recorded attempt: attempt number, outcome, status code, duration, error and response body. Previously a failure rendered as "target: failed" and diagnosing it meant opening the per-webhook SQLite file by hand. The response body is cut by SQLite via substr over a blob cast, the same projection the event body uses, so an oversized stored response never becomes a Go string. The page reports the cut with a marker. Response bodies and errors are remote content, so both go through a new delivery.Redactor that strips the target's own destination URL, path, query and userinfo, plus the values of credential-shaped request headers, before rendering. Target configuration keeps reaching the template only as a TargetView. A body that reaches the cap is treated as cut whether or not SQLite is what cut it. The delivery engine stops reading a response at its own cap, which is the same number of bytes this page renders, and the row it writes records that cut length as the whole length, so nothing in the row separates a response that ended at the cap from one severed there. Such a body goes through RedactCut, which drops any tail that is a proper prefix of a secret: the remote chooses the padding in front of a credential it echoes, so it chooses where the cut falls inside that credential. Its marker says the response reached the recording limit rather than quoting a total the row does not know. Redactors are built from an unscoped target load. Deleting a target only soft deletes the row while its deliveries survive, and a scoped load would leave exactly those deliveries rendering unredacted. The views the page lists stay scoped. Attempt loading is chunked so the IN clause cannot exceed SQLite's bound-parameter limit, and a chunk that fails fails the page rather than rendering the deliveries it covered as never having run. The page renders at most 20 attempts per delivery, counting what it leaves out. static/css/tailwind.css is regenerated with tailwindcss for the utility classes the new markup uses. --- internal/delivery/engine_integration_test.go | 77 +++ internal/delivery/export_test.go | 6 + internal/delivery/target_redact.go | 272 ++++++++++ internal/delivery/target_redact_test.go | 284 ++++++++++ internal/handlers/delivery_result_view.go | 169 ++++++ .../handlers/delivery_result_view_test.go | 509 ++++++++++++++++++ internal/handlers/export_test.go | 12 +- internal/handlers/source_management.go | 260 +++++++-- static/css/tailwind.css | 2 +- templates/source_logs.html | 55 ++ 10 files changed, 1612 insertions(+), 34 deletions(-) create mode 100644 internal/delivery/target_redact.go create mode 100644 internal/delivery/target_redact_test.go create mode 100644 internal/handlers/delivery_result_view.go create mode 100644 internal/handlers/delivery_result_view_test.go diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index 0f4738b..2976a2b 100644 --- a/internal/delivery/engine_integration_test.go +++ b/internal/delivery/engine_integration_test.go @@ -1201,6 +1201,83 @@ func TestDeliverHTTP_TargetTimeout(t *testing.T) { iAssertResultFailed(t, db, del.ID) } +// TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog pins the size +// this engine stores for an oversized response, because the +// event log's redaction is written against it: the row holds +// exactly maxBodyLog bytes and records nothing about how much +// more the remote sent, so a credential echoed across that +// boundary reaches the database already severed and no reader +// of the row can tell the cut happened. +func TestDeliverHTTP_CutsStoredResponseAtMaxBodyLog( + t *testing.T, +) { + t.Parallel() + + // Padded so the cut falls five bytes before the end of the + // echoed webhook URL. + const ( + severedTail = 5 + overshoot = 100000 + ) + + sent := strings.Repeat( + "A", + delivery.ExportMaxBodyLog-len(slackWebhookURL)+ + severedTail, + ) + slackWebhookURL + strings.Repeat("Z", overshoot) + + s := newISetup(t) + + ts := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = io.WriteString(w, sent) + }, + )) + defer ts.Close() + + cfgJSON := iHTTPConfig(ts.URL) + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, `{"cut":"test"}`, + ) + targetID := uuid.New().String() + + del := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusPending, + ) + + bodyStr := event.Body + task := iTask( + del, event, s.WebhookID, targetID, + "cut-target", cfgJSON, 0, 1, &bodyStr, + ) + + s.Engine.ExportProcessNewTask(context.TODO(), &task) + + results := iResults(t, s.WebhookDB, del.ID) + require.Len(t, results, 1) + + stored := results[0].ResponseBody + + assert.Len( + t, stored, delivery.ExportMaxBodyLog, + "an oversized response is stored at exactly the cap", + ) + assert.Equal( + t, sent[:delivery.ExportMaxBodyLog], stored, + ) + assert.NotContains( + t, stored, slackWebhookURL, + "the echoed URL is severed by the cut", + ) + assert.Contains( + t, stored, "T00000000", + "the severed prefix still carries the credential", + ) +} + // iSeedEventAndDelivery creates event + delivery // for standalone tests. func iSeedEventAndDelivery( diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index fb509fa..890c3ce 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -25,6 +25,12 @@ const ( ExportRetryChannelSize = retryChannelSize ExportDefaultFailureThreshold = defaultFailureThreshold ExportDefaultCooldown = defaultCooldown + + // ExportMaxBodyLog is the cap the engine applies to a + // recorded response body. The event log's handling of a cut + // response is written against this number, so a test has to + // be able to name it. + ExportMaxBodyLog = maxBodyLog ) // ExportIsBlockedIP exposes isBlockedIP for testing. diff --git a/internal/delivery/target_redact.go b/internal/delivery/target_redact.go new file mode 100644 index 0000000..a9050db --- /dev/null +++ b/internal/delivery/target_redact.go @@ -0,0 +1,272 @@ +package delivery + +import ( + "net/url" + "slices" + "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 removes byte-identical echoes of strings taken from the +// target's stored configuration, and nothing else. Anything +// the remote re-encodes survives: JSON "\/" escaping (what +// PHP's json_encode emits by default), percent-encoding, HTML +// entities, and an echo of only part of a path. 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 { + // Drop empty strings here rather than at the site that + // produced one. strings.ReplaceAll with an empty old string + // inserts the marker at every byte boundary, so a single + // empty secret destroys every body and error the target + // renders; filtering at the collection point means no field + // added to targetSecrets later can reintroduce that. + // url.Parse("https://@example.com/in") is the known + // producer: a non-nil User whose String is "". + secrets := slices.DeleteFunc( + targetSecrets(t), + func(s string) bool { return s == "" }, + ) + + // Longest first, so replacing a secret that is contained + // in a longer one cannot leave a fragment of the longer + // one behind. Configured headers arrive in map order, so + // the sort is also what makes the result deterministic. + slices.SortFunc(secrets, func(a, b string) int { + if d := len(b) - len(a); d != 0 { + return d + } + + return strings.Compare(a, b) + }) + + return Redactor{secrets: secrets} +} + +// 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 +} + +// RedactCut redacts s, which its caller has already cut to a +// byte budget, and additionally drops any tail of s that is a +// proper prefix of a secret. +// +// The cut lands wherever the remote's padding puts it, so the +// remote chooses where inside the credential it falls. The +// severed prefix left behind equals no secret, so plain +// Redact would render it verbatim. +func (r Redactor) RedactCut(s string) string { + s = r.Redact(s) + + if n := r.secretPrefixSuffix(s); n > 0 { + return s[:len(s)-n] + RedactionMarker + } + + return s +} + +// secretPrefixSuffix returns the length of the longest suffix +// of s that is a proper prefix of one of the secrets, or 0 +// when there is none. +func (r Redactor) secretPrefixSuffix(s string) int { + longest := 0 + + for _, secret := range r.secrets { + // Proper prefixes only: a whole secret at the tail was + // already replaced by Redact. + n := min(len(secret)-1, len(s)) + for ; n > longest; n-- { + if strings.HasSuffix(s, secret[:n]) { + longest = n + + break + } + } + } + + return longest +} + +// targetSecrets returns the credential-bearing strings a +// target's configuration carries. +// +// 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 request headers contribute their values, but +// only for the credential-shaped names isCredentialHeaderName +// picks out. That is the same class-based rule applied to +// URLs: an echoed Accept or User-Agent still renders, an +// echoed Authorization does not. +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 append( + urlSecrets(cfg.URL), + headerSecrets(cfg.Headers)..., + ) + 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, and none to the +// userinfo. A short path or a four-byte username is treated as +// a credential exactly like a long one, because the field takes +// an arbitrary URL and no part of it can be assumed non-secret — +// the same rule MaskURL applies. headerSecrets does carry a +// floor, and the difference is deliberate: a header is picked +// out by a name-shaped guess and its value may be ordinary +// text, whereas a URL's path and userinfo are credential +// material by position. +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 +} + +// minHeaderSecretBytes is the shortest header value treated as +// a credential. Unlike a URL path, a header value can be a +// couple of bytes long, and redacting those would scatter the +// marker through ordinary response text for no gain. +const minHeaderSecretBytes = 4 + +// headerSecrets returns the values of the configured headers +// whose names are credential-shaped. +func headerSecrets(headers map[string]string) []string { + var secrets []string + + for name, value := range headers { + value = strings.TrimSpace(value) + if len(value) < minHeaderSecretBytes { + continue + } + + if isCredentialHeaderName(name) { + secrets = append(secrets, value) + } + } + + return secrets +} + +// isCredentialHeaderName classifies a header by its name. The +// value is never inspected, so the rule is the same +// class-based one MaskURL applies to a destination URL. +// +// The fragments are short on purpose, and match anywhere in +// the name, so abbreviations an operator might use are covered +// too: X-Sig, X-Pass, X-HMAC. That over-matches — a header +// named X-Design contains "sig" — and over-matching is the +// safe direction here: the cost is a marker where an echoed +// header value would have rendered. +func isCredentialHeaderName(name string) bool { + name = strings.ToLower(strings.TrimSpace(name)) + + // Names that carry a credential by definition. + switch name { + case "authorization", "proxy-authorization", "cookie": + return true + } + + // What operators call their own credential headers: + // X-Api-Key, X-Hub-Signature, X-Auth-Token. + for _, fragment := range []string{ + "auth", + "credential", + "hmac", + "key", + "pass", + "secret", + "sig", + "token", + } { + if strings.Contains(name, fragment) { + return true + } + } + + return false +} diff --git a/internal/delivery/target_redact_test.go b/internal/delivery/target_redact_test.go new file mode 100644 index 0000000..e28e302 --- /dev/null +++ b/internal/delivery/target_redact_test.go @@ -0,0 +1,284 @@ +package delivery_test + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "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") + + // One marker, not a marker with the host left in front of + // it: the whole URL is replaced before the path it + // contains, which is what sorting the secrets longest + // first buys. + assert.Equal( + t, + "no_service for "+delivery.RedactionMarker, + got, + ) +} + +// TestRedactor_RemovesSecretSeveredByACut covers the input the +// redactor exists for: text cut to a byte budget with the +// credential straddling the cut. The remote chooses the +// padding, so it chooses where the cut lands inside the +// credential, and the severed prefix that remains equals no +// secret. +func TestRedactor_RemovesSecretSeveredByACut(t *testing.T) { + t.Parallel() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeSlack, + Config: `{"webhookUrl":"` + redactWebhookURL + `"}`, + }) + + // Every cut position inside the credential, not just a + // convenient one. + for n := 1; n < len(redactWebhookURL); n++ { + severed := redactWebhookURL[:n] + cut := "padding " + severed + + got := r.RedactCut(cut) + + assert.Equal( + t, + "padding "+delivery.RedactionMarker, + got, + "cut after %d bytes of the credential", n, + ) + } +} + +// TestRedactor_RedactsCredentialShapedHeaderValues pins the +// class-based header rule: a header whose name says credential +// has its value redacted, and a routine header does not, so +// ordinary response content survives. +func TestRedactor_RedactsCredentialShapedHeaderValues( + t *testing.T, +) { + t.Parallel() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"https://example.com/in",` + + `"headers":{` + + `"Authorization":"Bearer AAAAAAAAAAAA",` + + `"Cookie":"session=BBBBBBBBBBBB",` + + `"X-Api-Key":"CCCCCCCCCCCC",` + + `"X-Hub-Signature":"sha256=DDDDDDDDDDDD",` + + `"X-Sig":"EEEEEEEEEEEE",` + + `"X-Pass":"FFFFFFFFFFFF",` + + `"X-HMAC":"GGGGGGGGGGGG",` + + `"X-Credential":"HHHHHHHHHHHH",` + + `"Accept":"application/json",` + + `"User-Agent":"webhooker/1.0"}}`, + }) + + for _, secret := range []string{ + "Bearer AAAAAAAAAAAA", + "session=BBBBBBBBBBBB", + "CCCCCCCCCCCC", + "sha256=DDDDDDDDDDDD", + // Abbreviated names an operator might use. + "EEEEEEEEEEEE", + "FFFFFFFFFFFF", + "GGGGGGGGGGGG", + "HHHHHHHHHHHH", + } { + got := r.Redact("echo: " + secret) + + assert.Equal( + t, + "echo: "+delivery.RedactionMarker, + got, + secret, + ) + } + + const routine = "Accept: application/json, " + + "User-Agent: webhooker/1.0" + + assert.Equal(t, routine, r.Redact(routine)) +} + +// TestRedactor_IgnoresVeryShortHeaderValues pins the floor +// under a header value. Redacting a two-byte value would put +// the marker through every response that happens to contain +// those bytes. +func TestRedactor_IgnoresVeryShortHeaderValues(t *testing.T) { + t.Parallel() + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"https://example.com/in",` + + `"headers":{"X-Api-Key":"ab"}}`, + }) + + const response = "rabbit" + + assert.Equal(t, response, r.Redact(response)) +} + +// 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_EmptyUserinfoDoesNotShredTheBody covers a +// destination URL written with a bare "@" and no userinfo: +// url.Parse returns a non-nil User whose String is empty. An +// empty secret in the list would make strings.ReplaceAll +// insert the marker at every byte boundary, destroying every +// body and error string the target renders. +func TestRedactor_EmptyUserinfoDoesNotShredTheBody(t *testing.T) { + t.Parallel() + + const dest = "https://@example.com/in" + + // The premise: this URL really does parse to a non-nil + // User contributing an empty string. + parsed, err := url.Parse(dest) + require.NoError(t, err) + require.NotNil(t, parsed.User) + require.Empty(t, parsed.User.String()) + + r := delivery.NewRedactor(&database.Target{ + Type: database.TargetTypeHTTP, + Config: `{"url":"` + dest + `"}`, + }) + + const body = "ok=false error=channel_not_found" + + assert.Equal(t, body, r.Redact(body)) + assert.Equal(t, body, r.RedactCut(body)) + + // The real credential material still goes, so filtering the + // empty string out did not disarm the redactor. + assert.Equal( + t, + "POST "+delivery.RedactionMarker, + r.Redact("POST "+dest), + ) +} + +// 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..e3bb27b --- /dev/null +++ b/internal/handlers/delivery_result_view.go @@ -0,0 +1,169 @@ +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. +// +// The bound is enforced here and in SQL, because this page's +// memory profile must not depend on a constant in another +// package staying where it is, and because rows predating the +// delivery engine's own cap or restored from an archive are +// not covered by it at all. +// +// It happens to equal that engine cap, so a row written by the +// current engine reaches this one exactly and is never cut +// twice. Nothing here may assume the two differ: see view. +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 size of the stored response body, + // before the cut and before redaction. It is what the + // remote sent only when ResponseSizeKnown is set. + 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 body shown may be + // incomplete, so the page owes the reader a marker. Every + // body that reaches the cap counts, because one the + // delivery engine cut at its own equal cap is + // indistinguishable from a complete one. + ResponseTruncated bool + + // ResponseSizeKnown reports that ResponseBytes is the whole + // response the remote sent, so the marker may quote it. It + // is false for a body that only reaches the cap, where how + // much came after it was never recorded. + ResponseSizeKnown 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 + + // Two different cuts can have shortened this body, and the + // row records only one of them. SQLite cuts here, whenever + // the stored value is larger than the cap. The delivery + // engine cut earlier, whenever the remote sent more than + // its own maxBodyLog — which is this same number, so such a + // row stores the cut length as its whole length and nothing + // in it separates a response that ended at the cap from one + // severed there. + // + // So a body that reaches the cap is treated as cut either + // way. Gating on ResponseBytes alone would assume the two + // caps differ, and they do not: under the current engine + // that gate never opens. + cut := r.ResponseBytes > int64(len(body)) || + len(body) >= maxRenderedResponseBytes + + // The row holds more than the page shows only in the first + // of those cases. In the second the stored row is all there + // is, and its size is a floor rather than the true one. + sizeKnown := r.ResponseBytes > int64(len(body)) + + // Only a cut response can have been left mid-sequence, + // exactly as with an event body. + if cut { + body = trimPartialRune(body) + } + + // A cut body goes through RedactCut: the remote controls + // the padding ahead of a credential it echoes, so it + // controls where the cut falls inside that credential, and + // the severed prefix left behind matches no secret whole. + rendered := string(body) + if cut { + rendered = redactor.RedactCut(rendered) + } else { + rendered = redactor.Redact(rendered) + } + + return DeliveryResultView{ + AttemptNum: r.AttemptNum, + Success: r.Success, + StatusCode: r.StatusCode, + Error: redactor.Redact(r.Error), + DurationMS: r.Duration, + ResponseBody: rendered, + ResponseBytes: r.ResponseBytes, + ResponseShownBytes: len(body), + ResponseTruncated: cut, + ResponseSizeKnown: sizeKnown, + } +} diff --git a/internal/handlers/delivery_result_view_test.go b/internal/handlers/delivery_result_view_test.go new file mode 100644 index 0000000..63f387a --- /dev/null +++ b/internal/handlers/delivery_result_view_test.go @@ -0,0 +1,509 @@ +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, "