Add a target edit form with headers and timeout fields (closes #127) (#229)

internal/handlers/target_edit_test.go

@@ -0,0 +1,637 @@
+package handlers_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"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/delivery"
+)
+
+// The destinations the target edit tests configure. Both are literal
+// public addresses rather than hostnames so the SSRF check resolves
+// nothing: with a hostname, a sandbox without DNS would reject the
+// URL for the wrong reason and a test asserting rejection would pass
+// even with the guard removed.
+const (
+	editOriginalURL = "https://93.184.216.34/hooks/original"
+	editReplacedURL = "https://93.184.216.34/hooks/replaced"
+	// editBlockedURL resolves to loopback, which the SSRF guard
+	// refuses. It is what proves the guard runs on the edit path.
+	editBlockedURL = "http://127.0.0.1/hooks/internal"
+)
+
+// editAuthHeader carries a bearer credential, the case the headers
+// field exists for.
+const (
+	editBearerSecret = "QQEDITSECRETQQ"
+	editAuthHeader   = "Authorization: Bearer " + editBearerSecret
+)
+
+// targetRouter mounts the target create and edit routes on a chi
+// router so the handlers see the URL parameters they read.
+func targetRouter(env *sourceTestEnv) *chi.Mux {
+	router := chi.NewRouter()
+	router.Post(
+		"/source/{sourceID}/targets",
+		env.handlers.HandleTargetCreate(),
+	)
+	router.Get(
+		"/source/{sourceID}/targets/{targetID}/edit",
+		env.handlers.HandleTargetEdit(),
+	)
+	router.Post(
+		"/source/{sourceID}/targets/{targetID}/edit",
+		env.handlers.HandleTargetEditSubmit(),
+	)
+
+	return router
+}
+
+// serveTarget drives one request through the target routes as the
+// authenticated test user.
+func serveTarget(
+	env *sourceTestEnv,
+	method, path string,
+	form url.Values,
+) *httptest.ResponseRecorder {
+	body := ""
+	if form != nil {
+		body = form.Encode()
+	}
+
+	req := httptest.NewRequestWithContext(
+		context.Background(), method, path,
+		strings.NewReader(body),
+	)
+	if form != nil {
+		req.Header.Set(
+			"Content-Type",
+			"application/x-www-form-urlencoded",
+		)
+	}
+
+	for _, c := range env.cookies {
+		req.AddCookie(c)
+	}
+
+	w := httptest.NewRecorder()
+	targetRouter(env).ServeHTTP(w, req)
+
+	return w
+}
+
+// seedHTTPTarget creates a webhook and an HTTP target on it through
+// the real create handler, so every case starts from a target the
+// production path produced rather than a hand-written row.
+//
+// Standing the fx app up is what a handler test mostly costs, and
+// internal/handlers is already the slowest package in the suite, so
+// the tests below share one env per test function and give each case
+// its own webhook rather than its own app.
+func seedHTTPTarget(
+	t *testing.T,
+	env *sourceTestEnv,
+	headers, timeout string,
+) (database.Webhook, database.Target) {
+	t.Helper()
+
+	webhook := seedWebhookWithRetention(t, env.db, 30)
+
+	form := url.Values{}
+	form.Set("name", "original-name")
+	form.Set("type", string(database.TargetTypeHTTP))
+	form.Set("url", editOriginalURL)
+	form.Set("headers", headers)
+	form.Set("timeout", timeout)
+	form.Set("max_retries", "3")
+
+	w := serveTarget(
+		env, http.MethodPost,
+		"/source/"+webhook.ID+"/targets", form,
+	)
+	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+	targets := targetsForWebhook(t, env.db, webhook.ID)
+	require.Len(t, targets, 1)
+
+	return webhook, targets[0]
+}
+
+// storedTarget reloads a target row.
+func storedTarget(
+	t *testing.T,
+	env *sourceTestEnv,
+	targetID string,
+) database.Target {
+	t.Helper()
+
+	var target database.Target
+
+	require.NoError(
+		t,
+		env.db.DB().Where("id = ?", targetID).
+			First(&target).Error,
+	)
+
+	return target
+}
+
+// storedHTTPConfig reloads a target and parses its stored HTTP
+// configuration.
+func storedHTTPConfig(
+	t *testing.T,
+	env *sourceTestEnv,
+	targetID string,
+) delivery.HTTPTargetConfig {
+	t.Helper()
+
+	var cfg delivery.HTTPTargetConfig
+
+	require.NoError(
+		t,
+		json.Unmarshal(
+			[]byte(storedTarget(t, env, targetID).Config), &cfg,
+		),
+	)
+
+	return cfg
+}
+
+// editForm is the fully populated edit submission for an HTTP
+// target.
+func editForm(targetURL, headers, timeout string) url.Values {
+	form := url.Values{}
+	form.Set("name", "edited-name")
+	form.Set("url", targetURL)
+	form.Set("headers", headers)
+	form.Set("timeout", timeout)
+	form.Set("max_retries", "5")
+
+	return form
+}
+
+// submitTargetEdit posts the edit form for a target.
+func submitTargetEdit(
+	env *sourceTestEnv,
+	webhookID, targetID string,
+	form url.Values,
+) *httptest.ResponseRecorder {
+	return serveTarget(
+		env, http.MethodPost,
+		"/source/"+webhookID+"/targets/"+targetID+"/edit",
+		form,
+	)
+}
+
+// TestHandleTargetCreate_Configuration covers the half of the gap
+// that is not about editing at all: HTTPTargetConfig has carried
+// Headers and Timeout, and the delivery path has honoured them, but
+// the create form wrote {"url":...} and nothing else, so a
+// destination needing an Authorization header could not be
+// configured through the UI at all.
+func TestHandleTargetCreate_Configuration(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	t.Run("stores headers and timeout", func(t *testing.T) {
+		t.Parallel()
+		assertCreateStoresHeadersAndTimeout(t, env)
+	})
+
+	t.Run("without them keeps a url-only config", func(t *testing.T) {
+		t.Parallel()
+		assertCreateKeepsURLOnlyConfig(t, env)
+	})
+}
+
+func assertCreateStoresHeadersAndTimeout(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	_, target := seedHTTPTarget(
+		t, env, editAuthHeader+"\nX-Tenant: acme\n", "12",
+	)
+
+	cfg := storedHTTPConfig(t, env, target.ID)
+
+	assert.Equal(t, editOriginalURL, cfg.URL)
+	assert.Equal(t, 12, cfg.Timeout)
+	assert.Equal(
+		t,
+		map[string]string{
+			"Authorization": "Bearer " + editBearerSecret,
+			"X-Tenant":      "acme",
+		},
+		cfg.Headers,
+	)
+}
+
+// Without the new fields the stored shape must be the same
+// {"url":...} the create form wrote before they existed, so no
+// existing target's configuration is rewritten by this change.
+func assertCreateKeepsURLOnlyConfig(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	_, target := seedHTTPTarget(t, env, "", "")
+
+	assert.JSONEq(
+		t, `{"url":"`+editOriginalURL+`"}`, target.Config,
+	)
+}
+
+// TestHandleTargetEditSubmit_Saves is the round trip the issue asks
+// for: create a target, edit it, and confirm the stored config
+// changed.
+func TestHandleTargetEditSubmit_Saves(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	t.Run("changes the destination URL", func(t *testing.T) {
+		t.Parallel()
+		assertEditChangesDestination(t, env)
+	})
+
+	t.Run("round trips headers and timeout", func(t *testing.T) {
+		t.Parallel()
+		assertEditRoundTripsHeadersAndTimeout(t, env)
+	})
+
+	t.Run("clearing them removes them", func(t *testing.T) {
+		t.Parallel()
+		assertEditClearingRemovesThem(t, env)
+	})
+
+	t.Run("absent max_retries is not zeroed", func(t *testing.T) {
+		t.Parallel()
+		assertEditKeepsAbsentMaxRetries(t, env)
+	})
+}
+
+func assertEditChangesDestination(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(editReplacedURL, "", ""),
+	)
+	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+	assert.Equal(
+		t,
+		editReplacedURL,
+		storedHTTPConfig(t, env, target.ID).URL,
+	)
+
+	reloaded := storedTarget(t, env, target.ID)
+	assert.Equal(t, "edited-name", reloaded.Name)
+	assert.Equal(t, 5, reloaded.MaxRetries)
+	assert.Equal(
+		t, database.TargetTypeHTTP, reloaded.Type,
+		"the edit form must not change a target's type",
+	)
+}
+
+// The two previously unreachable fields must survive create,
+// pre-fill and save.
+func assertEditRoundTripsHeadersAndTimeout(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(
+			editOriginalURL,
+			"Authorization: Bearer rotated\nX-Trace: on",
+			"21",
+		),
+	)
+	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+	cfg := storedHTTPConfig(t, env, target.ID)
+
+	assert.Equal(t, 21, cfg.Timeout)
+	assert.Equal(
+		t,
+		map[string]string{
+			"Authorization": "Bearer rotated",
+			"X-Trace":       "on",
+		},
+		cfg.Headers,
+	)
+}
+
+// The direction a naive "only set what was submitted" implementation
+// gets wrong: an emptied field must remove the stored value, not
+// leave the previous one in place.
+func assertEditClearingRemovesThem(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(editOriginalURL, "", ""),
+	)
+	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+	cfg := storedHTTPConfig(t, env, target.ID)
+
+	assert.Empty(t, cfg.Headers)
+	assert.Zero(t, cfg.Timeout)
+}
+
+// Retries are offered only by the forms for target types that retry.
+// An absent field means the form does not edit retries, not that
+// they should be turned off.
+func assertEditKeepsAbsentMaxRetries(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "")
+	require.Equal(t, 3, target.MaxRetries)
+
+	form := editForm(editOriginalURL, "", "")
+	form.Del("max_retries")
+
+	w := submitTargetEdit(env, webhook.ID, target.ID, form)
+	require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+	assert.Equal(
+		t, 3, storedTarget(t, env, target.ID).MaxRetries,
+	)
+}
+
+// TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked covers the
+// deliberate exception to the masking rule. The operator cannot
+// correct a value they cannot see, so this page — and only this page
+// — renders the destination and the header values in full.
+func TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+	w := serveTarget(
+		env, http.MethodGet,
+		"/source/"+webhook.ID+"/targets/"+target.ID+"/edit",
+		nil,
+	)
+	require.Equal(t, http.StatusOK, w.Code)
+
+	page := w.Body.String()
+
+	assert.Contains(t, page, editOriginalURL)
+	assert.Contains(t, page, "Bearer "+editBearerSecret)
+	assert.Contains(t, page, `value="7"`)
+	assert.Contains(t, page, "original-name")
+}
+
+// TestHandleTargetEditSubmit_Rejects covers every submission that
+// must not reach storage.
+//
+// The SSRF case is the most important assertion on this change: the
+// edited destination goes through the same guard the create path
+// uses. An edit that stored an unvalidated URL would reopen a closed
+// hole, since a target could then be created public and edited to
+// point at loopback.
+//
+// The header and timeout cases keep input that could not be
+// delivered as written out of storage: a stored value that provably
+// never reaches the wire reports a configuration that did not take
+// effect.
+func TestHandleTargetEditSubmit_Rejects(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	t.Run("an SSRF-blocked destination", func(t *testing.T) {
+		t.Parallel()
+		assertEditRejectsBlockedDestination(t, env)
+	})
+
+	t.Run("a query-string destination", func(t *testing.T) {
+		t.Parallel()
+		assertEditIgnoresQueryString(t, env)
+	})
+
+	headerCases := map[string]string{
+		"no colon":        "Authorization Bearer token",
+		"empty name":      ": value",
+		"invalid name":    "X Bad Name: value",
+		"reserved header": "User-Agent: curl/8",
+		"duplicate name":  "X-A: one\nx-a: two",
+	}
+
+	for name, headers := range headerCases {
+		t.Run("headers: "+name, func(t *testing.T) {
+			t.Parallel()
+			assertEditRejectsHeaders(t, env, headers)
+		})
+	}
+
+	timeoutCases := map[string]string{
+		"not a number": "soon",
+		"negative":     "-1",
+		"over ceiling": "100000",
+	}
+
+	for name, timeout := range timeoutCases {
+		t.Run("timeout: "+name, func(t *testing.T) {
+			t.Parallel()
+			assertEditRejectsTimeout(t, env, timeout)
+		})
+	}
+}
+
+func assertEditRejectsBlockedDestination(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(editBlockedURL, "", ""),
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(t, w.Body.String(), "Invalid target URL")
+	assert.Equal(
+		t, editOriginalURL,
+		storedHTTPConfig(t, env, target.ID).URL,
+		"a rejected edit must leave the stored config alone",
+	)
+}
+
+// The ingress rule the create path already follows applies to the
+// edit path too: reading a field with FormValue would let the request
+// line carry the credential, and the request line is what logs,
+// proxies and Referer headers record.
+func assertEditIgnoresQueryString(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "")
+
+	form := url.Values{}
+	form.Set("name", "edited-name")
+
+	w := serveTarget(
+		env, http.MethodPost,
+		"/source/"+webhook.ID+"/targets/"+target.ID+
+			"/edit?url="+url.QueryEscape(editReplacedURL)+
+			"&headers="+url.QueryEscape(editAuthHeader),
+		form,
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+
+	cfg := storedHTTPConfig(t, env, target.ID)
+	assert.Equal(t, editOriginalURL, cfg.URL)
+	assert.Empty(t, cfg.Headers)
+}
+
+func assertEditRejectsHeaders(
+	t *testing.T, env *sourceTestEnv, headers string,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(editOriginalURL, headers, ""),
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(t, w.Body.String(), "Invalid headers")
+	assert.Empty(
+		t, storedHTTPConfig(t, env, target.ID).Headers,
+		"a rejected header must not be stored",
+	)
+}
+
+func assertEditRejectsTimeout(
+	t *testing.T, env *sourceTestEnv, timeout string,
+) {
+	t.Helper()
+
+	webhook, target := seedHTTPTarget(t, env, "", "9")
+
+	w := submitTargetEdit(
+		env, webhook.ID, target.ID,
+		editForm(editOriginalURL, "", timeout),
+	)
+
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+	assert.Contains(t, w.Body.String(), "Invalid timeout")
+	assert.Equal(
+		t, 9, storedHTTPConfig(t, env, target.ID).Timeout,
+		"a rejected timeout must leave the stored one alone",
+	)
+}
+
+// TestHandleTargetEdit_Scoping keeps the edit routes scoped the way
+// the delete and toggle routes are: ownership is decided by the
+// webhook, and the target is then scoped to it.
+func TestHandleTargetEdit_Scoping(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	t.Run("a target of another webhook", func(t *testing.T) {
+		t.Parallel()
+		assertTargetOfAnotherWebhook404s(t, env)
+	})
+
+	t.Run("a webhook of another user", func(t *testing.T) {
+		t.Parallel()
+		assertWebhookOfAnotherUser404s(t, env)
+	})
+}
+
+// A target id from elsewhere must not become editable by pairing it
+// with a webhook the user does own.
+func assertTargetOfAnotherWebhook404s(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	mine := seedWebhookWithRetention(t, env.db, 30)
+	_, target := seedHTTPTarget(t, env, "", "")
+
+	get := serveTarget(
+		env, http.MethodGet,
+		"/source/"+mine.ID+"/targets/"+target.ID+"/edit", nil,
+	)
+	assert.Equal(t, http.StatusNotFound, get.Code)
+
+	post := submitTargetEdit(
+		env, mine.ID, target.ID,
+		editForm(editReplacedURL, "", ""),
+	)
+	assert.Equal(t, http.StatusNotFound, post.Code)
+
+	assert.Equal(
+		t, editOriginalURL,
+		storedHTTPConfig(t, env, target.ID).URL,
+	)
+}
+
+func assertWebhookOfAnotherUser404s(
+	t *testing.T, env *sourceTestEnv,
+) {
+	t.Helper()
+
+	other := &database.Webhook{
+		UserID:        "some-other-user",
+		Name:          "not mine",
+		RetentionDays: 30,
+	}
+	require.NoError(
+		t,
+		env.db.DB().Omit(clause.Associations).Create(other).Error,
+	)
+
+	target := seedConfiguredTarget(
+		t, env.db, other.ID, database.TargetTypeHTTP,
+		`{"url":"`+editOriginalURL+`"}`,
+	)
+
+	w := serveTarget(
+		env, http.MethodGet,
+		"/source/"+other.ID+"/targets/"+target.ID+"/edit", nil,
+	)
+
+	assert.Equal(t, http.StatusNotFound, w.Code)
+}