diff --git a/internal/handlers/event_body.go b/internal/handlers/event_body.go new file mode 100644 index 0000000..c6e2582 --- /dev/null +++ b/internal/handlers/event_body.go @@ -0,0 +1,251 @@ +package handlers + +import ( + "errors" + "net/http" + "strconv" + + "github.com/go-chi/chi" + "github.com/google/uuid" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" +) + +// bodyChunkBytes is how much of a stored body is resident at +// once while it is being written to the client. The event log +// page caps what it renders at maxRenderedBodyBytes, so this +// route is the only way to reach a whole body and by +// construction serves the largest ones in the system. Reading +// it in fixed chunks keeps the peak a property of this constant +// rather than of the payload. +const bodyChunkBytes = 64 * 1024 + +// eventBodySizeColumn measures a stored body the same way +// eventLogColumns cuts one: the cast to blob makes length count +// bytes rather than characters, so Content-Length matches what +// substr will actually hand back. +const eventBodySizeColumn = "length(cast(body as blob))" + +// eventBodyChunkQuery reads one byte range of a stored body. +// substr over a blob is 1-indexed over bytes. The soft-delete +// predicate is spelled out because Raw bypasses GORM's default +// scope, and it is what stops a reaped event still being +// downloadable. +const eventBodyChunkQuery = "SELECT substr(cast(body as blob), ?, ?) " + + "FROM events WHERE id = ? AND webhook_id = ? AND deleted_at IS NULL" + +// errShortBodyRead reports that a chunk query returned nothing +// while bytes were still owed, which means the row went away +// mid-download. +var errShortBodyRead = errors.New("stored body ended early") + +// HandleEventBodyDownload serves one event's stored body in +// full, which the event log page cannot: it caps each rendered +// body at maxRenderedBodyBytes. +// +// The bytes are attacker-supplied — anyone who can reach the +// public receiver chooses them — and this route hands them back +// inside the operator's own authenticated origin, so the +// response is deliberately not renderable. Content-Disposition +// makes the browser download rather than display it, and the +// octet-stream type plus nosniff stop it being interpreted as +// HTML or script. Without those a stored payload would execute +// as the logged-in operator. The application's CSP does not +// help here: script-src allows 'unsafe-inline' from 'self', so +// a document served from this origin could run its own inline +// script. +func (h *Handlers) HandleEventBodyDownload() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + webhook, ok := h.ownedWebhook(w, r) + if !ok { + return + } + + // Parsing the id before use serves two purposes: a + // malformed id can never reach the SQL or the response + // header, and the canonical form below is drawn from + // uuid's own fixed alphabet rather than from the + // request, so the Content-Disposition value cannot be + // steered by a client. + eventID, err := uuid.Parse(chi.URLParam(r, "eventID")) + if err != nil { + http.NotFound(w, r) + + return + } + + h.serveEventBody(w, r, webhook, eventID.String()) + } +} + +// serveEventBody writes the named event's stored body to w. +// +// The event must belong to webhook, which is what keeps this +// route from reading any event in the system by id alone. Two +// things enforce that and they are not equally strong. The +// operative one is that events live in a per-webhook SQLite +// file, so a sibling webhook's event is not in the database +// being queried at all. The webhook_id predicate on every query +// below is the second guard, and it is currently redundant +// against that isolation; it is there so the scoping survives +// any future change that puts more than one webhook's events in +// one file. +func (h *Handlers) serveEventBody( + w http.ResponseWriter, + r *http.Request, + webhook database.Webhook, + eventID string, +) { + if !h.dbMgr.DBExists(webhook.ID) { + http.NotFound(w, r) + + return + } + + webhookDB, err := h.dbMgr.GetDB(webhook.ID) + if err != nil { + h.serverError(w, "failed to get webhook database", err) + + return + } + + size, found, err := eventBodySize(webhookDB, webhook.ID, eventID) + if err != nil { + h.serverError(w, "failed to size event body", err) + + return + } + + // A miss is a 404 whether the event belongs to another + // webhook or does not exist at all, so the response does + // not report which. + if !found { + http.NotFound(w, r) + + return + } + + setEventBodyHeaders(w, eventID, size) + + err = writeEventBody(w, webhookDB, webhook.ID, eventID, size) + if err != nil { + // The status and Content-Length are already committed, + // so the client sees a short download. There is no way + // to report a 500 from here; the log is the record. + h.log.Error( + "failed to write event body", + "webhook_id", webhook.ID, + "event_id", eventID, + "error", err, + ) + } +} + +// eventBodySize returns the stored size in bytes of an event's +// body and whether the event exists within the webhook. The +// size is read separately from the body so Content-Length can +// be set before any bytes are written. +func eventBodySize( + webhookDB *gorm.DB, + webhookID, eventID string, +) (int64, bool, error) { + var size int64 + + result := webhookDB.Model(&database.Event{}). + Select(eventBodySizeColumn). + Where( + "id = ? AND webhook_id = ?", eventID, webhookID, + ). + Limit(1). + Scan(&size) + if result.Error != nil { + return 0, false, result.Error + } + + if result.RowsAffected == 0 { + return 0, false, nil + } + + return size, true, nil +} + +// setEventBodyHeaders applies the response headers that make +// this route safe to hand attacker-supplied bytes through. See +// HandleEventBodyDownload for why they are a security control +// and not a formatting choice. +// +// nosniff is also set by the global SecurityHeaders middleware. +// It is repeated here so the guarantee belongs to the route +// that needs it rather than to a middleware someone could +// reorder or scope away. +func setEventBodyHeaders( + w http.ResponseWriter, + eventID string, + size int64, +) { + w.Header().Set("Content-Type", "application/octet-stream") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set( + "Content-Disposition", + `attachment; filename="webhooker-event-`+eventID+`.bin"`, + ) + w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) +} + +// writeEventBody copies size bytes of the event's stored body to +// w in bodyChunkBytes-sized reads. +// +// This is where the route earns its memory bound. database/sql +// exposes no incremental handle on a SQLite BLOB, so scanning +// the column would materialise the whole body regardless of the +// wrapper around it; reading byte ranges instead keeps the +// resident cost at one chunk. Nothing goes through +// renderTemplate, which buffers a whole response before writing +// it. +// +// There is deliberately no wrapping read transaction. These +// per-webhook databases run in SQLite's default journal mode, +// not WAL, so a read lock held for the length of a slow client's +// download would block the receiver from recording new events. +// The cost of that choice is that a body deleted mid-download +// ends the response short, which is reported as an error rather +// than passed off as a complete file. +func writeEventBody( + w http.ResponseWriter, + webhookDB *gorm.DB, + webhookID, eventID string, + size int64, +) error { + // Flushing each chunk keeps the claim above true at the + // socket as well as in this loop. A ResponseWriter that + // cannot flush is not an error: net/http's own output + // buffer is a fixed size either way. + flusher := http.NewResponseController(w) + + for written := int64(0); written < size; { + var chunk []byte + + err := webhookDB.Raw( + eventBodyChunkQuery, + written+1, bodyChunkBytes, eventID, webhookID, + ).Row().Scan(&chunk) + if err != nil { + return err + } + + if len(chunk) == 0 { + return errShortBodyRead + } + + n, err := w.Write(chunk) + written += int64(n) + + if err != nil { + return err + } + + _ = flusher.Flush() + } + + return nil +} diff --git a/internal/handlers/event_body_test.go b/internal/handlers/event_body_test.go new file mode 100644 index 0000000..d77849c --- /dev/null +++ b/internal/handlers/event_body_test.go @@ -0,0 +1,385 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/go-chi/chi" + "github.com/google/uuid" + "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/handlers" + "sneak.berlin/go/webhooker/internal/session" +) + +// paramEventID is the chi URL parameter the body download +// handler reads. +const paramEventID = "eventID" + +// otherTestUserID owns webhooks the session user must not be +// able to read. +const otherTestUserID = "other-user-id" + +// seedWebhookFor inserts a webhook owned by the given user. +func seedWebhookFor( + t *testing.T, + db *database.Database, + userID string, +) *database.Webhook { + t.Helper() + + wh := &database.Webhook{ + UserID: userID, + Name: "wh-" + userID, + } + + require.NoError( + t, + db.DB().Omit(clause.Associations).Create(wh).Error, + ) + + return wh +} + +// fetchEventBody runs the real download handler as the test user +// for the given source and event ids. +func fetchEventBody( + t *testing.T, + h *handlers.Handlers, + sess *session.Session, + sourceID, eventID string, +) *httptest.ResponseRecorder { + t.Helper() + + // The path is escaped and the raw id goes in the route + // context, which is what chi hands a handler: the param is + // already percent-decoded by the time it is read. + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodGet, + "/source/"+url.PathEscape(sourceID)+ + "/logs/"+url.PathEscape(eventID)+"/body", + nil, + ) + + for _, c := range authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) { + req.AddCookie(c) + } + + rctx := chi.NewRouteContext() + rctx.URLParams.Add(paramSourceID, sourceID) + rctx.URLParams.Add(paramEventID, eventID) + + req = req.WithContext( + context.WithValue( + req.Context(), chi.RouteCtxKey, rctx, + ), + ) + + w := httptest.NewRecorder() + h.HandleEventBodyDownload().ServeHTTP(w, req) + + return w +} + +// TestHandleEventBodyDownload_ServesOversizeBodyInFull is the +// capability the render cap took away: a body far above what the +// event log page will show comes back whole and byte-identical, +// with the headers that keep it from being rendered. +func TestHandleEventBodyDownload_ServesOversizeBodyInFull( + t *testing.T, +) { + t.Parallel() + + 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) + + // Larger than one read chunk as well as the render cap, so + // the chunked read has to reassemble the body in order and + // the tail past the last whole chunk is exercised. + const sentinel = "TAIL-SENTINEL-1f4a9c" + + stored := strings.Repeat("A", 200*1024) + + strings.Repeat(snowman, 1000) + sentinel + + wh := seedWebhook(t, db) + evt := seedEventWithBody(t, dbMgr, wh.ID, stored) + + w := fetchEventBody(t, h, sess, wh.ID, evt.ID) + + require.Equal(t, http.StatusOK, w.Code) + assert.Greater(t, len(stored), bodyCap) + assert.Equal(t, stored, w.Body.String()) + assert.Equal( + t, strconv.Itoa(len(stored)), + w.Header().Get("Content-Length"), + ) +} + +// TestHandleEventBodyDownload_HeadersAreNotRenderable pins the +// response headers that stop attacker-supplied bytes executing +// in the operator's own origin. They are a security control, not +// presentation. +func TestHandleEventBodyDownload_HeadersAreNotRenderable( + t *testing.T, +) { + t.Parallel() + + 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) + evt := seedEventWithBody(t, dbMgr, wh.ID, `{"small":true}`) + + w := fetchEventBody(t, h, sess, wh.ID, evt.ID) + + require.Equal(t, http.StatusOK, w.Code) + assert.Equal( + t, "application/octet-stream", + w.Header().Get("Content-Type"), + ) + assert.Equal( + t, "nosniff", + w.Header().Get("X-Content-Type-Options"), + ) + + disposition := w.Header().Get("Content-Disposition") + assert.Equal( + t, + `attachment; filename="webhooker-event-`+evt.ID+`.bin"`, + disposition, + ) +} + +// TestHandleEventBodyDownload_ScriptBodyStaysInert proves a +// stored HTML payload is handed back as an attachment of opaque +// bytes rather than as anything a browser will execute. The +// bytes themselves are unaltered: this route reports what was +// delivered. +func TestHandleEventBodyDownload_ScriptBodyStaysInert( + t *testing.T, +) { + t.Parallel() + + 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) + + const payload = `` + + wh := seedWebhook(t, db) + evt := seedEventWithBody(t, dbMgr, wh.ID, payload) + + w := fetchEventBody(t, h, sess, wh.ID, evt.ID) + + require.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, payload, w.Body.String()) + + contentType := w.Header().Get("Content-Type") + assert.Equal(t, "application/octet-stream", contentType) + assert.NotContains(t, contentType, "html") + assert.NotContains(t, contentType, "xml") + assert.NotContains(t, contentType, "javascript") + assert.Contains( + t, w.Header().Get("Content-Disposition"), "attachment", + ) + assert.Equal( + t, "nosniff", + w.Header().Get("X-Content-Type-Options"), + ) +} + +// TestHandleEventBodyDownload_OtherUsersEvent404s is the +// authorization test the definition of done asks for: an event +// stored under a webhook the session user does not own is not +// readable, and the miss does not distinguish itself from a +// nonexistent one. +func TestHandleEventBodyDownload_OtherUsersEvent404s( + t *testing.T, +) { + t.Parallel() + + 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) + + const theirPayload = "OTHER-USERS-PAYLOAD-8b1d" + + theirs := seedWebhookFor(t, db, otherTestUserID) + evt := seedEventWithBody(t, dbMgr, theirs.ID, theirPayload) + + w := fetchEventBody(t, h, sess, theirs.ID, evt.ID) + + assert.Equal(t, http.StatusNotFound, w.Code) + assert.NotContains(t, w.Body.String(), theirPayload) +} + +// TestHandleEventBodyDownload_EventOfAnotherWebhook404s pins +// that holding a valid event id is not enough: the event has to +// belong to the webhook in the path. Both webhooks here are the +// session user's and both have event databases, so the +// ownership check cannot be what produces the 404. +// +// What does produce it is the per-webhook database file rather +// than the webhook_id predicate on the query — removing that +// predicate leaves this test green, because the sibling's event +// is in a different file. The test is kept as the behavioural +// guard the route owes; see serveEventBody for which mechanism +// is load-bearing. +func TestHandleEventBodyDownload_EventOfAnotherWebhook404s( + t *testing.T, +) { + t.Parallel() + + 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) + + const other = "BELONGS-TO-THE-OTHER-WEBHOOK-3c7e" + + mine := seedWebhook(t, db) + seedEventWithBody(t, dbMgr, mine.ID, `{"mine":true}`) + + sibling := seedWebhook(t, db) + evt := seedEventWithBody(t, dbMgr, sibling.ID, other) + + w := fetchEventBody(t, h, sess, mine.ID, evt.ID) + + assert.Equal(t, http.StatusNotFound, w.Code) + assert.NotContains(t, w.Body.String(), other) +} + +// TestHandleEventBodyDownload_UnknownEvent404s covers the plain +// miss, including an id that is not a uuid at all and so never +// reaches the query or the response header. +func TestHandleEventBodyDownload_UnknownEvent404s(t *testing.T) { + t.Parallel() + + 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) + seedEventWithBody(t, dbMgr, wh.ID, `{"mine":true}`) + + for _, id := range []string{ + uuid.New().String(), + `../../etc/passwd`, + "not-a-uuid", + `x"; rm -rf /`, + } { + w := fetchEventBody(t, h, sess, wh.ID, id) + + assert.Equal( + t, http.StatusNotFound, w.Code, + "event id %q", id, + ) + assert.Empty( + t, w.Header().Get("Content-Disposition"), + "event id %q must not reach a header", id, + ) + } +} + +// TestHandleSourceLogs_TruncationMarkerLinksToDownload proves +// the page tells the reader where the rest of the body is, and +// only when there is a rest to fetch. +func TestHandleSourceLogs_TruncationMarkerLinksToDownload( + t *testing.T, +) { + t.Parallel() + + 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) + + big := seedWebhook(t, db) + bigEvt := seedEventWithBody( + t, dbMgr, big.ID, strings.Repeat("A", 4*bodyCap), + ) + + page := renderSourceLogsPage(t, h, sess, big.ID) + assert.Contains( + t, page, + "/source/"+big.ID+"/logs/"+bigEvt.ID+"/body", + ) + + small := seedWebhook(t, db) + smallEvt := seedEventWithBody( + t, dbMgr, small.ID, `{"kept":"whole"}`, + ) + + page = renderSourceLogsPage(t, h, sess, small.ID) + assert.NotContains( + t, page, + "/source/"+small.ID+"/logs/"+smallEvt.ID+"/body", + ) +} diff --git a/internal/handlers/event_log_view_test.go b/internal/handlers/event_log_view_test.go index 7ffd2fb..e1d5a41 100644 --- a/internal/handlers/event_log_view_test.go +++ b/internal/handlers/event_log_view_test.go @@ -25,13 +25,14 @@ const bodyCap = handlers.MaxRenderedBodyBytesForTest const snowman = "☃" // seedEventWithBody records one event with the given body in the -// webhook's own database. +// webhook's own database and returns it, so a caller that needs +// the generated event id can have it. func seedEventWithBody( t *testing.T, dbMgr *database.WebhookDBManager, webhookID string, body string, -) { +) *database.Event { t.Helper() webhookDB, err := dbMgr.GetDB(webhookID) @@ -47,6 +48,8 @@ func seedEventWithBody( require.NoError(t, webhookDB.Omit( clause.Associations, ).Create(event).Error) + + return event } // seedAndProject stores one body and returns the projection the diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index b538bcd..0243b76 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -713,29 +713,55 @@ func (h *Handlers) evictArchiveWriterIfUnused(webhookID string) { h.evictArchiveWriter(webhookID) } +// ownedWebhook resolves the request's sourceID parameter to a +// webhook the session's user owns. +// +// Ownership and existence are decided by one query, so a +// webhook belonging to another user is indistinguishable from +// one that does not exist: both are a 404, and neither confirms +// the id. Callers that reach further into a webhook's data — +// the event log page and the event body download — share this +// one check rather than restating it, so the download cannot +// come to authorize differently from the page that links to it. +// +// It reports false once it has written the response, which is a +// redirect to the login page for an unauthenticated request and +// a 404 otherwise. The caller returns without writing more. +func (h *Handlers) ownedWebhook( + w http.ResponseWriter, + r *http.Request, +) (database.Webhook, bool) { + var webhook database.Webhook + + userID, ok := h.getUserID(r) + if !ok { + http.Redirect( + w, r, "/pages/login", http.StatusSeeOther, + ) + + return database.Webhook{}, false + } + + sourceID := chi.URLParam(r, "sourceID") + + err := h.db.DB().Where( + "id = ? AND user_id = ?", sourceID, userID, + ).First(&webhook).Error + if err != nil { + http.NotFound(w, r) + + return database.Webhook{}, false + } + + return webhook, true +} + // HandleSourceLogs shows the request/response logs for a // webhook. func (h *Handlers) HandleSourceLogs() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - userID, ok := h.getUserID(r) + webhook, ok := h.ownedWebhook(w, r) if !ok { - http.Redirect( - w, r, "/pages/login", http.StatusSeeOther, - ) - - return - } - - sourceID := chi.URLParam(r, "sourceID") - - var webhook database.Webhook - - err := h.db.DB().Where( - "id = ? AND user_id = ?", sourceID, userID, - ).First(&webhook).Error - if err != nil { - http.NotFound(w, r) - return } diff --git a/internal/server/routes.go b/internal/server/routes.go index b282e12..a8e4b01 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -146,6 +146,15 @@ func (s *Server) setupSourceRoutes() { r.Post("/edit", s.h.HandleSourceEditSubmit()) r.Post("/delete", s.h.HandleSourceDelete()) r.Get("/logs", s.h.HandleSourceLogs()) + // The log page renders each body only up to its cap, so + // this is the only route that serves a whole one. It + // belongs to this group for its RequireAuth and + // NoCache; see HandleEventBodyDownload for the headers + // that keep the bytes it returns inert. + r.Get( + "/logs/{eventID}/body", + s.h.HandleEventBodyDownload(), + ) r.Post( "/entrypoints", s.h.HandleEntrypointCreate(), diff --git a/templates/source_logs.html b/templates/source_logs.html index e523712..cf48983 100644 --- a/templates/source_logs.html +++ b/templates/source_logs.html @@ -38,7 +38,7 @@
{{.Body}}
{{if .BodyTruncated}}
- Body truncated for display: showing {{.BodyShownBytes}} of {{.BodyBytes}} bytes. The stored body is unchanged.
+Body truncated for display: showing {{.BodyShownBytes}} of {{.BodyBytes}} bytes. The stored body is unchanged — download the full body.
{{end}}