Mask the target URL in delivery errors, SSRF logs and log page data (closes #118)
All checks were successful
check / check (push) Successful in 3m30s
All checks were successful
check / check (push) Successful in 3m30s
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.
This commit is contained in:
@@ -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
|
||||
|
||||
134
internal/handlers/source_logs_test.go
Normal file
134
internal/handlers/source_logs_test.go
Normal file
@@ -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")
|
||||
}
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user