webhooker/internal/handlers/target_create_query_test.go

package handlers_test

import (
	"bytes"
	"context"
	"log/slog"
	"net/http"
	"net/http/httptest"
	"net/url"
	"strings"
	"testing"

	"github.com/go-chi/chi"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"sneak.berlin/go/webhooker/internal/config"
	"sneak.berlin/go/webhooker/internal/database"
	"sneak.berlin/go/webhooker/internal/middleware/middlewaretest"
)

// targetSecretSegments are the path segments of an incoming-webhook
// URL. For Slack, Discord and Teams the path IS the bearer credential,
// so this string must not reach storage or the access log by way of
// the request line.
const targetSecretSegments = "T00000000/B00000000/QQTARGETSECRETQQ"

// targetSecretURL is a destination whose secret lives in its path. It
// uses a literal public address rather than a hostname so the SSRF
// check resolves nothing: with a hostname, a sandbox without DNS would
// reject the URL for the wrong reason and the test would pass even
// with the defect reintroduced.
const targetSecretURL = "https://93.184.216.34/services/" +
	targetSecretSegments

// targetsForWebhook returns every target stored against a webhook.
func targetsForWebhook(
	t *testing.T,
	db *database.Database,
	webhookID string,
) []database.Target {
	t.Helper()

	var targets []database.Target

	require.NoError(
		t,
		db.DB().Where("webhook_id = ?", webhookID).
			Find(&targets).Error,
	)

	return targets
}

// postTargetCreate drives HandleTargetCreate through the production
// access-log middleware and a chi route, so the logged url field is
// produced exactly as it ships, and returns the recorder plus the
// captured log.
func postTargetCreate(
	t *testing.T,
	env *sourceTestEnv,
	webhookID string,
	query string,
	form url.Values,
) (*httptest.ResponseRecorder, string) {
	t.Helper()

	logBuf := new(bytes.Buffer)
	mw := middlewaretest.NewForTest(
		t,
		slog.New(slog.NewJSONHandler(
			logBuf, &slog.HandlerOptions{Level: slog.LevelInfo},
		)),
		&config.Config{Environment: config.EnvironmentDev},
		nil,
	)

	router := chi.NewRouter()
	router.Use(mw.Logging())
	router.Post(
		"/hook/{sourceID}/targets",
		env.handlers.HandleTargetCreate(),
	)

	target := "/hook/" + webhookID + "/targets"
	if query != "" {
		target += "?" + query
	}

	body := ""
	if form != nil {
		body = form.Encode()
	}

	req := httptest.NewRequestWithContext(
		context.Background(),
		http.MethodPost,
		target,
		strings.NewReader(body),
	)
	req.Header.Set(
		"Content-Type", "application/x-www-form-urlencoded",
	)

	for _, c := range env.cookies {
		req.AddCookie(c)
	}

	w := httptest.NewRecorder()
	router.ServeHTTP(w, req)

	return w, logBuf.String()
}

// TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget is the
// regression test for the ingress leak. r.FormValue falls back to the
// query string when a field is absent from the POST body, so
//
//	POST /hook/{id}/targets?url=https://hooks.slack.com/services/...
//
// with an empty url field used to create a working target from a value
// carried on the request line — where logs, proxies, Referer headers
// and error trackers record it. The handler reads the body only, so
// the request is rejected for a missing URL and stores nothing.
//
// name and type are sent in the BODY on purpose: the request has to
// get past those two validations for the assertion to be about the url
// read specifically.
func TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget(
	t *testing.T,
) {
	t.Parallel()

	env := setupSourceTest(t)
	webhook := seedWebhookWithRetention(t, env.db, 30)

	body := url.Values{}
	body.Set("name", "leaky")
	body.Set("type", string(database.TargetTypeSlack))

	w, logged := postTargetCreate(
		t, env, webhook.ID,
		"url="+url.QueryEscape(targetSecretURL),
		body,
	)

	assert.Equal(t, http.StatusBadRequest, w.Code)

	targets := targetsForWebhook(t, env.db, webhook.ID)
	assert.Empty(
		t, targets,
		"a query-string value must not populate a target config",
	)

	assert.NotContains(t, logged, targetSecretSegments)
	assert.NotContains(t, logged, "93.184.216.34")
	assert.NotEmpty(t, logged, "the access log line must still be written")
}

// TestHandleTargetCreate_BodyURLStillCreatesTheTarget is the positive
// control for the test above: the rejection has to come from where the
// value was read, not from the handler being broken.
func TestHandleTargetCreate_BodyURLStillCreatesTheTarget(t *testing.T) {
	t.Parallel()

	env := setupSourceTest(t)
	webhook := seedWebhookWithRetention(t, env.db, 30)

	body := url.Values{}
	body.Set("name", "legit")
	body.Set("type", string(database.TargetTypeSlack))
	body.Set("url", targetSecretURL)

	w, logged := postTargetCreate(t, env, webhook.ID, "", body)

	assert.Equal(t, http.StatusSeeOther, w.Code)

	targets := targetsForWebhook(t, env.db, webhook.ID)
	require.Len(t, targets, 1)
	assert.Contains(t, targets[0].Config, targetSecretSegments)

	// The body carried the credential, so the access log must still
	// not have it: the log records the request line only.
	assert.NotContains(t, logged, targetSecretSegments)
}

// TestHandleTargetCreate_QueryStringCannotSupplyNameOrType covers the
// rest of the converted reads on this handler in one request: with an
// empty body, nothing the query carries is visible to it.
func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType(
	t *testing.T,
) {
	t.Parallel()

	env := setupSourceTest(t)
	webhook := seedWebhookWithRetention(t, env.db, 30)

	w, _ := postTargetCreate(
		t, env, webhook.ID,
		"name=leaky&type=slack&max_retries=9&expiry=30d&url="+
			url.QueryEscape(targetSecretURL),
		url.Values{},
	)

	assert.Equal(t, http.StatusBadRequest, w.Code)
	assert.Contains(t, w.Body.String(), "Name is required")
	assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID))
}