Mask the webhook credential in delivery errors and logs (closes #118)
All checks were successful
check / check (push) Successful in 9s
All checks were successful
check / check (push) Successful in 9s
Go embeds the request URL in *url.Error, so any transport failure — DNS, TLS, refused, timeout, SSRF dial block — persisted the full Slack webhook URL into the per-webhook SQLite database via DeliveryResult.Error. That field is tagged json:"error,omitempty", so a future REST API would have served it. maskURLError rebuilds the error preserving Op and the wrapped cause, so DNS vs TLS vs timeout still read differently and errors.Is/As and Timeout() keep working; only path, query and userinfo are dropped. Applied where the errors are born, which covers both the Slack and HTTP targets. url.Parse embeds the URL too, so ValidateTargetURL's parse branch gets the same treatment. The SSRF rejection log now logs the masked URL, and source_logs.html receives view types rather than raw rows, so no config blob is reachable from that template. MaskURL is now the single masker for the whole tree.
This commit was merged in pull request #121.
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