From b6529f45a93ceaddadf975d6df9e050faf9c42a1 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 11 Aug 2026 12:52:05 +0000 Subject: [PATCH] Mask the target URL in delivery errors, SSRF logs and log page data (closes #118) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delivery target URL is itself a credential: a Slack incoming webhook URL is a bearer token. Three paths still reproduced it in full. Transport failures were the worst of them. net/http embeds the request URL in every *url.Error it returns, so any DNS, TLS, timeout or dial failure wrote the whole webhook URL into DeliveryResult.Error — on disk, in the per-webhook database, behind a json tag that a REST API would serialize. maskURL moves to url_mask.go and is exported as MaskURL, and maskURLError joins it: it rebuilds the *url.Error with the URL masked, keeping the operation and the wrapped cause, so a refused connection still reads differently from a DNS failure or a timeout and errors.Is/As/Timeout still work. It is applied where the errors are raised — executeHTTPRequest, shared by the Slack and HTTP targets, and the request-construction paths — so downstream wrapping is safe by construction. url.Parse embeds the URL too, so ValidateTargetURL's parse branch gets the same treatment; its error is logged and shown. The SSRF rejection log now records only the masked URL, and loadTargetMap hands the event log page TargetViews and a delivery projection instead of raw target rows, so the stored config blob has no path to that template either. --- internal/delivery/ssrf.go | 7 +- internal/delivery/target_config_view.go | 24 +-- internal/delivery/target_http.go | 16 +- internal/delivery/target_slack.go | 2 +- internal/delivery/url_mask.go | 61 ++++++++ internal/delivery/url_mask_test.go | 196 ++++++++++++++++++++++++ internal/handlers/source_detail_test.go | 6 +- internal/handlers/source_logs_test.go | 134 ++++++++++++++++ internal/handlers/source_management.go | 67 ++++++-- 9 files changed, 468 insertions(+), 45 deletions(-) create mode 100644 internal/delivery/url_mask.go create mode 100644 internal/delivery/url_mask_test.go create mode 100644 internal/handlers/source_logs_test.go diff --git a/internal/delivery/ssrf.go b/internal/delivery/ssrf.go index be23746..fb6bacc 100644 --- a/internal/delivery/ssrf.go +++ b/internal/delivery/ssrf.go @@ -92,7 +92,12 @@ func ValidateTargetURL( ) error { parsed, err := url.Parse(targetURL) if err != nil { - return fmt.Errorf("invalid URL: %w", err) + // url.Parse embeds the whole URL in its error, and + // this one is logged and shown; mask it. Every other + // branch below reports only the hostname. + return fmt.Errorf( + "invalid URL: %w", maskURLError(err), + ) } err = validateScheme(parsed.Scheme) diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go index 4a6b8b7..8beb182 100644 --- a/internal/delivery/target_config_view.go +++ b/internal/delivery/target_config_view.go @@ -3,7 +3,6 @@ package delivery import ( "encoding/json" "fmt" - "net/url" "strconv" "sneak.berlin/go/webhooker/internal/database" @@ -17,9 +16,6 @@ import ( // browser history, screenshots and screen shares. const configUnavailable = "(unavailable)" -// urlPathElision stands in for a URL's elided path. -const urlPathElision = "/..." - // ConfigField is one labelled, display-safe value derived // from a target's stored configuration. type ConfigField struct { @@ -202,23 +198,5 @@ func databaseConfigFields(configJSON string) []ConfigField { // parse into a scheme and host yields the neutral // placeholder, never the raw string. func (c *SlackTargetConfig) MaskedWebhookURL() string { - return maskURL(c.WebhookURL) -} - -// maskURL renders a URL as scheme plus host with everything -// that can carry a secret removed. -func maskURL(raw string) string { - parsed, err := url.Parse(raw) - if err != nil || parsed.Scheme == "" || - parsed.Host == "" { - return configUnavailable - } - - masked := parsed.Scheme + "://" + parsed.Host - - if parsed.Path != "" && parsed.Path != "/" { - masked += urlPathElision - } - - return masked + return MaskURL(c.WebhookURL) } diff --git a/internal/delivery/target_http.go b/internal/delivery/target_http.go index 3ad6fb2..f39af45 100644 --- a/internal/delivery/target_http.go +++ b/internal/delivery/target_http.go @@ -363,7 +363,8 @@ func (t *httpTarget) doHTTPRequest( ) if reqErr != nil { return 0, "", 0, fmt.Errorf( - "creating request: %w", reqErr, + "creating request: %w", + maskURLError(reqErr), ) } @@ -492,8 +493,19 @@ func applyRequestHeaders( // executeHTTPRequest sends an HTTP request using the provided // client. URLs are validated by the config parsers and the // SSRF-safe transport before reaching here. +// +// Transport failures are masked here, at the single point +// where every target's request errors are born, because the +// caller stores them in DeliveryResult.Error: an unmasked +// *url.Error would write the target URL — the credential for +// a Slack incoming webhook — into the per-webhook database. func executeHTTPRequest( client *http.Client, req *http.Request, ) (*http.Response, error) { - return client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport + resp, err := client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport + if err != nil { + return nil, maskURLError(err) + } + + return resp, nil } diff --git a/internal/delivery/target_slack.go b/internal/delivery/target_slack.go index fdb95f6..5f98359 100644 --- a/internal/delivery/target_slack.go +++ b/internal/delivery/target_slack.go @@ -125,7 +125,7 @@ func (t *slackTarget) attempt( if err != nil { return attemptResult{ success: false, - errMsg: err.Error(), + errMsg: maskURLError(err).Error(), } } diff --git a/internal/delivery/url_mask.go b/internal/delivery/url_mask.go new file mode 100644 index 0000000..95821d2 --- /dev/null +++ b/internal/delivery/url_mask.go @@ -0,0 +1,61 @@ +package delivery + +import ( + "errors" + "net/url" +) + +// urlPathElision stands in for a URL's elided path. +const urlPathElision = "/..." + +// MaskURL renders a URL as scheme plus host with everything +// that can carry a secret removed. A delivery target URL is +// itself a credential — a Slack incoming webhook URL is a +// bearer token — so the path, query and userinfo are never +// reproduced, in a page, a log line or a stored error. A URL +// that does not parse into a scheme and host yields the +// neutral placeholder, never the raw string. +func MaskURL(raw string) string { + parsed, err := url.Parse(raw) + if err != nil || parsed.Scheme == "" || + parsed.Host == "" { + return configUnavailable + } + + masked := parsed.Scheme + "://" + parsed.Host + + if parsed.Path != "" && parsed.Path != "/" { + masked += urlPathElision + } + + return masked +} + +// maskURLError strips the credential from an error raised +// against a request URL. The net/http and net/url packages +// embed the full request URL in every *url.Error they return, +// so an unmodified transport error persisted into +// DeliveryResult.Error writes the credential to disk. +// +// The masked error keeps the operation and the wrapped cause, +// so a DNS failure still reads differently from a refused +// connection, a TLS handshake failure or a timeout, and Is, +// As, Timeout and Temporary keep working on it. Only the +// path, query and userinfo of the URL are dropped. Errors +// that carry no URL are returned unchanged. +// +// Call it where the error is raised, before any wrapping: it +// replaces the *url.Error itself, so any context wrapped +// around it first would be discarded. +func maskURLError(err error) error { + var urlErr *url.Error + if !errors.As(err, &urlErr) { + return err + } + + return &url.Error{ + Op: urlErr.Op, + URL: MaskURL(urlErr.URL), + Err: urlErr.Err, + } +} diff --git a/internal/delivery/url_mask_test.go b/internal/delivery/url_mask_test.go new file mode 100644 index 0000000..e6cc152 --- /dev/null +++ b/internal/delivery/url_mask_test.go @@ -0,0 +1,196 @@ +package delivery_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/delivery" +) + +// The path of a Slack incoming webhook URL is the credential: +// whoever holds these segments can post to the channel +// forever. None of them may reach a stored delivery error, +// which lives on disk in the per-webhook database and is +// serialized by the JSON tag on DeliveryResult.Error. +const ( + maskSecretPath = "/services/T00000000/B00000000/" + + "XXXXXXXXXXXXXXXXXXXXXXXX" +) + +// assertNoCredential fails if the whole path or any single +// segment of it survived into the message, so a partial leak +// fails the test too. +func assertNoCredential(t *testing.T, msg string) { + t.Helper() + + segments := []string{ + maskSecretPath, + "services", + "T00000000", + "B00000000", + "XXXXXXXXXXXXXXXXXXXXXXXX", + } + + for _, segment := range segments { + assert.NotContains(t, msg, segment) + } +} + +// storedDeliveryError returns the error string persisted for a +// delivery, which is what an operator and any future API read. +func storedDeliveryError( + t *testing.T, db *gorm.DB, deliveryID string, +) string { + t.Helper() + + var result database.DeliveryResult + + require.NoError(t, db.Where( + "delivery_id = ?", deliveryID, + ).First(&result).Error) + + return result.Error +} + +// deliverSlackTo runs a Slack delivery against webhookURL and +// returns the error string it persisted. +func deliverSlackTo( + t *testing.T, webhookURL string, +) string { + t.Helper() + + db := testWebhookDB(t) + e := testEngine(t, 1) + targetID := uuid.New().String() + + slackCfg, err := json.Marshal( + delivery.SlackTargetConfig{ + WebhookURL: webhookURL, + }, + ) + require.NoError(t, err) + + event := seedEvent(t, db, `{"test":true}`) + + dlv := seedDelivery( + t, db, event.ID, targetID, + database.DeliveryStatusPending, + ) + + d := buildSlackDelivery( + dlv, event, targetID, + "test-slack-mask", string(slackCfg), + ) + + e.ExportDeliverSlack(context.TODO(), db, d) + + assertDeliveryStatus(t, db, dlv.ID, + database.DeliveryStatusFailed, + ) + + return storedDeliveryError(t, db, dlv.ID) +} + +// TestDeliverSlack_TransportErrorMasksWebhookURL is the +// load-bearing regression test: a transport failure must not +// persist the webhook URL's credential into the database, and +// must still say what went wrong and where. +func TestDeliverSlack_TransportErrorMasksWebhookURL( + t *testing.T, +) { + t.Parallel() + + // A server closed before use gives a deterministic + // transport failure against a known host. + ts := httptest.NewServer(http.NewServeMux()) + host := ts.URL + + ts.Close() + + errMsg := deliverSlackTo(t, host+maskSecretPath) + + require.NotEmpty(t, errMsg) + assertNoCredential(t, errMsg) + + // The diagnostic value survives: the operation, the host + // and the transport failure are all still reported, and + // only the path is elided. + assert.Contains(t, errMsg, "sending request") + assert.Contains(t, errMsg, "Post") + assert.Contains(t, errMsg, host+"/...") + assert.Contains(t, errMsg, "connection refused") +} + +// TestDeliverSlack_UnparsableURLMasksWebhookURL covers the +// other error path out of a Slack attempt: url.Parse also +// embeds the whole URL in the error it returns. +func TestDeliverSlack_UnparsableURLMasksWebhookURL( + t *testing.T, +) { + t.Parallel() + + errMsg := deliverSlackTo( + t, + "https://hooks.slack.com"+maskSecretPath+"\n", + ) + + require.NotEmpty(t, errMsg) + assertNoCredential(t, errMsg) + assert.Contains(t, errMsg, "invalid control character") +} + +// TestDoHTTPRequest_TransportErrorMasksURL proves the HTTP +// target's transport errors are masked too; its destination +// URL can carry a token in a query string. +func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) { + t.Parallel() + + ts := httptest.NewServer(http.NewServeMux()) + host := ts.URL + + ts.Close() + + e := testEngine(t, 1) + + cfg, err := e.ExportParseHTTPConfig( + newHTTPTargetConfig(host + maskSecretPath), + ) + require.NoError(t, err) + + statusCode, _, _, reqErr := e.ExportDoHTTPRequest( + context.TODO(), cfg, + &database.Event{Body: `{"test":true}`}, + ) + require.Error(t, reqErr) + assert.Zero(t, statusCode) + + assertNoCredential(t, reqErr.Error()) + assert.Contains(t, reqErr.Error(), host+"/...") + assert.Contains( + t, reqErr.Error(), "connection refused", + ) +} + +// TestValidateTargetURL_UnparsableURLIsMasked proves the SSRF +// validator's error does not carry the submitted URL, which +// the handler both logs and shows. +func TestValidateTargetURL_UnparsableURLIsMasked(t *testing.T) { + t.Parallel() + + err := delivery.ValidateTargetURL( + context.TODO(), + "https://hooks.slack.com"+maskSecretPath+"\n", + ) + require.Error(t, err) + + assertNoCredential(t, err.Error()) + assert.Contains(t, err.Error(), "invalid URL") +} diff --git a/internal/handlers/source_detail_test.go b/internal/handlers/source_detail_test.go index 248362a..dcc129c 100644 --- a/internal/handlers/source_detail_test.go +++ b/internal/handlers/source_detail_test.go @@ -26,14 +26,14 @@ const ( ) // seedConfiguredTarget inserts a target with a stored config -// blob. +// blob and returns it. func seedConfiguredTarget( t *testing.T, db *database.Database, webhookID string, targetType database.TargetType, config string, -) { +) *database.Target { t.Helper() tgt := &database.Target{ @@ -48,6 +48,8 @@ func seedConfiguredTarget( t, db.DB().Omit(clause.Associations).Create(tgt).Error, ) + + return tgt } // renderSourceDetailPage runs the real source detail handler diff --git a/internal/handlers/source_logs_test.go b/internal/handlers/source_logs_test.go new file mode 100644 index 0000000..e4a2e55 --- /dev/null +++ b/internal/handlers/source_logs_test.go @@ -0,0 +1,134 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi" + "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" +) + +// seedDeliveredEvent records an event and a delivery for it in +// the webhook's own database, so the log page has a delivery +// to render against the target. +func seedDeliveredEvent( + t *testing.T, + dbMgr *database.WebhookDBManager, + webhookID, targetID string, +) { + 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.DeliveryStatusDelivered, + } + + require.NoError(t, webhookDB.Omit( + clause.Associations, + ).Create(dlv).Error) +} + +// renderSourceLogsPage runs the real event log handler for a +// webhook and returns the rendered HTML. +func renderSourceLogsPage( + t *testing.T, + h *handlers.Handlers, + sess *session.Session, + webhookID string, +) string { + t.Helper() + + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodGet, + "/source/"+webhookID+"/logs", + nil, + ) + + for _, c := range authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) { + req.AddCookie(c) + } + + rctx := chi.NewRouteContext() + rctx.URLParams.Add(paramSourceID, webhookID) + + req = req.WithContext( + context.WithValue( + req.Context(), chi.RouteCtxKey, rctx, + ), + ) + + w := httptest.NewRecorder() + h.HandleSourceLogs().ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + + return w.Body.String() +} + +// TestHandleSourceLogs_MasksSlackWebhookURL proves the event +// log page is handed a display-safe projection of each target +// rather than the stored row, so the credential cannot be +// rendered from its template data. +func TestHandleSourceLogs_MasksSlackWebhookURL(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) + tgt := seedConfiguredTarget( + t, db, wh.ID, + database.TargetTypeSlack, + `{"webhookUrl":"`+slackWebhookURL+`"}`, + ) + + seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID) + + body := renderSourceLogsPage(t, h, sess, wh.ID) + + assert.NotContains(t, body, slackSecretPath) + assert.NotContains(t, body, "T00000000") + assert.NotContains(t, body, "B00000000") + assert.NotContains( + t, body, "XXXXXXXXXXXXXXXXXXXXXXXX", + ) + assert.NotContains(t, body, "webhookUrl") + + // The page still identifies the delivery's target. + assert.Contains(t, body, tgt.Name) + assert.Contains(t, body, "delivered") +} diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index d6f3656..3b55e71 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -96,7 +96,17 @@ func parseRetentionDays(raw string, fallback int) (int, error) { type EventWithDeliveries struct { database.Event - Deliveries []database.Delivery + Deliveries []DeliveryView +} + +// DeliveryView is the display-safe projection of a delivery +// for the event log page. Its target is a TargetView, so the +// stored configuration blob — which holds the target's +// credential — has no path to the template. +type DeliveryView struct { + ID string + Status database.DeliveryStatus + Target delivery.TargetView } // HandleSourceList shows a list of user's webhooks. @@ -764,22 +774,27 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { } } -// loadTargetMap loads targets into a map keyed by target ID. +// loadTargetMap loads targets into a map of display-safe +// views keyed by target ID. The projection happens here so +// that no caller can hand a raw target, configuration blob +// and all, to a template. func (h *Handlers) loadTargetMap( webhookID string, -) map[string]database.Target { +) map[string]delivery.TargetView { var targets []database.Target h.db.DB().Where( "webhook_id = ?", webhookID, ).Find(&targets) + views := delivery.NewTargetViews(targets) + targetMap := make( - map[string]database.Target, len(targets), + map[string]delivery.TargetView, len(views), ) - for _, t := range targets { - targetMap[t.ID] = t + for _, v := range views { + targetMap[v.ID] = v } return targetMap @@ -804,7 +819,7 @@ func (h *Handlers) parsePage(r *http.Request) int { func (h *Handlers) loadEventsWithDeliveries( w http.ResponseWriter, webhook database.Webhook, - targetMap map[string]database.Target, + targetMap map[string]delivery.TargetView, page int, ) ([]EventWithDeliveries, int64) { var totalEvents int64 @@ -843,22 +858,39 @@ func (h *Handlers) loadEventsWithDeliveries( for i := range events { result[i].Event = events[i] + var deliveries []database.Delivery + webhookDB.Where( "event_id = ?", events[i].ID, - ).Find(&result[i].Deliveries) + ).Find(&deliveries) - for j := range result[i].Deliveries { - tid := result[i].Deliveries[j].TargetID - - if target, ok := targetMap[tid]; ok { - result[i].Deliveries[j].Target = target - } - } + result[i].Deliveries = newDeliveryViews( + deliveries, targetMap, + ) } return result, totalEvents } +// newDeliveryViews projects deliveries for rendering, +// resolving each one's target to its display-safe view. +func newDeliveryViews( + deliveries []database.Delivery, + targetMap map[string]delivery.TargetView, +) []DeliveryView { + views := make([]DeliveryView, len(deliveries)) + + for i := range deliveries { + views[i] = DeliveryView{ + ID: deliveries[i].ID, + Status: deliveries[i].Status, + Target: targetMap[deliveries[i].TargetID], + } + } + + return views +} + // HandleEntrypointCreate handles adding a new entrypoint. func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -1102,9 +1134,12 @@ func (h *Handlers) buildURLTargetConfig( r.Context(), targetURL, ) if err != nil { + // The submitted URL can be a credential (a Slack + // incoming webhook URL is a bearer token), so the log + // records only its scheme and host. h.log.Warn( "target URL blocked by SSRF protection", - "url", targetURL, + "url", delivery.MaskURL(targetURL), "error", err, ) http.Error(