Logging in returns to the page that was asked for (closes #384)

RequireAuth now sends a logged-out GET to /pages/login with its path
and query in a `next` parameter when they fit in 2048 bytes, the same
limit the login page applies. The login form carries it as a hidden
field, and a successful login redirects there when it is a path on
this site; anything else, plain or percent-encoded, goes to `/`, which
leads to the webhook list. A browser already logged in that opens the
login page goes to the same place. The navigation bar on the login
page no longer links to the login page.

Model: opus-5-5

README.md

@@ -145,11 +145,6 @@ TTY detection, and security headers are always applied.
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted. A set value replaces the default. If any client can reach webhooker, or the proxy in front of it, from an RFC 1918 source address, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) |
 | `ALLOWED_EGRESS_CIDRS`  | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) |
 
-The Settings page of the web UI (`/settings`, behind the login) lists
-every one of these with the value the running server loaded. It is
-read-only, and it shows `METRICS_PASSWORD` and `SENTRY_DSN` only as
-set or not set, never their values.
-
 #### Allowing egress to your own network
 
 By default every delivery target must resolve to a public address. The
@@ -2708,17 +2703,20 @@ abuse limit later; they are tracked as future work.
 
 | Method | Path            | Description |
 | ------ | --------------- | ----------- |
-| `GET`  | `/pages/login`  | Login page (not rate limited) |
-| `POST` | `/pages/login`  | Login form submission. Credentials are verified before any limit is consulted, so a correct password is never throttled; 5 FAILED attempts per minute per bucket per submitted username, then `429`. `503` if no verification slot frees up within 5s, or immediately if 16 requests are already queued for one (see [Rate Limiting](#rate-limiting)) |
+| `GET`  | `/pages/login`  | Login page (not rate limited). Its `next` parameter names the page to return to after login; anything but a path on this site is replaced with `/` |
+| `POST` | `/pages/login`  | Login form submission. On success, redirects to the form's `next` when it is a path on this site, otherwise to `/`. Credentials are verified before any limit is consulted, so a correct password is never throttled; 5 FAILED attempts per minute per bucket per submitted username, then `429`. `503` if no verification slot frees up within 5s, or immediately if 16 requests are already queued for one (see [Rate Limiting](#rate-limiting)) |
 | `POST` | `/pages/logout` | Logout (destroys session) |
 
 #### Authenticated Endpoints
 
+A logged-out `GET` of any of these is redirected to `/pages/login` with
+its path and query as `next` when they fit in 2048 bytes, so logging in
+returns to the page that was asked for.
+
 | Method | Path                     | Description |
 | ------ | ------------------------ | ----------- |
 | `GET`  | `/user/{username}`       | User profile page |
 | `POST` | `/user/{username}/password` | Change the user's password (5 per minute per bucket, then `429`; `503` if no verification slot frees up within 5s, or immediately if 16 requests are already queued for one) |
-| `GET`  | `/settings`              | Read-only list of the configuration the server is running with; `METRICS_PASSWORD` and `SENTRY_DSN` show only as set or not set |
 | `GET`  | `/sources`               | List user's webhooks |
 | `GET`  | `/sources/new`           | Create webhook form |
 | `POST` | `/sources/new`           | Create webhook submission |
@@ -2831,7 +2829,6 @@ webhooker/
 │   │   ├── healthcheck.go     # Health check handler
 │   │   ├── index.go           # Index page handler
 │   │   ├── profile.go         # User profile handler
-│   │   ├── settings.go        # Read-only Settings page handler
 │   │   ├── source_management.go  # Webhook CRUD handlers
 │   │   └── webhook.go         # Webhook receiver handler
 │   ├── healthcheck/

internal/handlers/auth.go

@@ -2,19 +2,56 @@ package handlers
 
 import (
 	"net/http"
+	"net/url"
 	"strconv"
+	"strings"
+	"unicode"
 
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/logfield"
+	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
+// loginDestination returns where a successful login sends the
+// browser: next when it is a path on this site, otherwise "/", which
+// leads to the webhook list.
+//
+// A browser reads "//host" as another site, reads "\" as "/", and
+// drops tabs and newlines before reading at all. So the value must
+// start with exactly one "/" and hold no "\" or control character
+// anywhere: http.Redirect cleans "/a/../\host" down to "/\host". It
+// is checked after percent-decoding, so an encoded form of any of
+// these is refused too.
+func loginDestination(next string) string {
+	if len(next) > middleware.MaxNextBytes {
+		return "/"
+	}
+
+	decoded, err := url.PathUnescape(next)
+	if err != nil ||
+		!strings.HasPrefix(decoded, "/") ||
+		strings.HasPrefix(decoded, "//") ||
+		strings.Contains(decoded, `\`) ||
+		strings.ContainsFunc(decoded, unicode.IsControl) {
+		return "/"
+	}
+
+	return next
+}
+
 // HandleLoginPage returns a handler for the login page (GET)
 func (h *Handlers) HandleLoginPage() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
+		next := loginDestination(
+			r.URL.Query().Get(middleware.NextParam),
+		)
+
 		// Check if already logged in
 		sess, err := h.session.Get(r)
 		if err == nil && h.session.IsAuthenticated(sess) {
-			http.Redirect(w, r, "/", http.StatusSeeOther)
+			http.Redirect( //nolint:gosec // checked by loginDestination
+				w, r, next, http.StatusSeeOther,
+			)
 
 			return
 		}
@@ -22,6 +59,7 @@ func (h *Handlers) HandleLoginPage() http.HandlerFunc {
 		// Render login page
 		data := map[string]any{
 			tmplKeyError: "",
+			tmplKeyNext:  next,
 		}
 
 		h.renderTemplate(w, r, "login.html", data)
@@ -77,8 +115,13 @@ func (h *Handlers) HandleLoginSubmit() http.HandlerFunc {
 			"user_id", user.ID,
 		)
 
-		// Redirect to home page
-		http.Redirect(w, r, "/", http.StatusSeeOther)
+		// The form value is the client's to set, so it is checked
+		// again here rather than trusted from the rendered page.
+		http.Redirect( //nolint:gosec // checked by loginDestination
+			w, r,
+			loginDestination(r.PostFormValue(middleware.NextParam)),
+			http.StatusSeeOther,
+		)
 	}
 }
 
@@ -91,6 +134,9 @@ func (h *Handlers) renderLoginError(
 ) {
 	data := map[string]any{
 		tmplKeyError: msg,
+		tmplKeyNext: loginDestination(
+			r.PostFormValue(middleware.NextParam),
+		),
 	}
 
 	w.WriteHeader(status)

internal/handlers/auth_test.go

@@ -454,6 +454,159 @@ func TestLogin_SuccessCreatesSession(t *testing.T) {
 	)
 }
 
+// TestLogin_ReturnsOnlyToAPathOnThisSite is the security half of
+// https://git.eeqj.de/sneak/webhooker/issues/384: the page a login
+// returns to is client-chosen, so anything that is not a path on this
+// site, plain or percent-encoded, must land on "/", the webhook list.
+func TestLogin_ReturnsOnlyToAPathOnThisSite(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h  *handlers.Handlers
+		db *database.Database
+	)
+
+	app := newTestApp(t, &h, &db)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	seedOperator(t, db)
+
+	cases := []struct{ next, want string }{
+		{"/source/abc/logs?page=2", "/source/abc/logs?page=2"},
+		{"", "/"},
+		{"https://evil.example/", "/"},
+		{"https%3A%2F%2Fevil.example%2F", "/"},
+		{"//evil.example/", "/"},
+		{"%2F%2Fevil.example/", "/"},
+		{"/%2Fevil.example/", "/"},
+		{`/\evil.example/`, "/"},
+		{"%2F%5Cevil.example/", "/"},
+		{"/%5Cevil.example/", "/"},
+		{`/a/../\evil.example/`, "/"},
+		{"/\t/evil.example/", "/"},
+		{"/%09/evil.example/", "/"},
+		{"/\n/evil.example/", "/"},
+		{"/%0A/evil.example/", "/"},
+		{"/\r/evil.example/", "/"},
+		{"/%0D/evil.example/", "/"},
+		{"/%00/evil.example/", "/"},
+		{"/%7F/evil.example/", "/"},
+		{"%252F%252Fevil.example/", "/"},
+		{"https%253A%252F%252Fevil.example%252F", "/"},
+		{"/" + strings.Repeat("a", 4096), "/"},
+	}
+
+	for _, c := range cases {
+		form := url.Values{}
+		form.Set("username", operatorUser)
+		form.Set("password", operatorPassword)
+		form.Set("next", c.next)
+
+		req := httptest.NewRequestWithContext(
+			context.Background(),
+			http.MethodPost,
+			"/pages/login",
+			strings.NewReader(form.Encode()),
+		)
+		req.Header.Set(
+			"Content-Type", "application/x-www-form-urlencoded",
+		)
+		req.RemoteAddr = sharedProxyPeer
+
+		w := httptest.NewRecorder()
+		h.HandleLoginSubmit().ServeHTTP(w, req)
+
+		assert.Equal(t, http.StatusSeeOther, w.Code, "next %q", c.next)
+		assert.Equal(
+			t, c.want, w.Header().Get("Location"), "next %q", c.next,
+		)
+	}
+}
+
+// loginPageGet renders the login page as a GET with the given next
+// value and cookies.
+func loginPageGet(
+	h *handlers.Handlers, next string, cookies []*http.Cookie,
+) *httptest.ResponseRecorder {
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet,
+		"/pages/login?"+url.Values{"next": {next}}.Encode(), nil,
+	)
+
+	for _, c := range cookies {
+		req.AddCookie(c)
+	}
+
+	w := httptest.NewRecorder()
+	h.HandleLoginPage().ServeHTTP(w, req)
+
+	return w
+}
+
+// TestLoginPage_CarriesOnlyAPathOnThisSite covers the login page
+// itself: its form carries the requested page only when it is a path
+// on this site, and a browser already logged in goes straight there,
+// or to "/" when it is not.
+func TestLoginPage_CarriesOnlyAPathOnThisSite(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h    *handlers.Handlers
+		sess *session.Session
+	)
+
+	app := newTestApp(t, &h, &sess)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	assert.Contains(
+		t, loginPageGet(h, "/source/abc", nil).Body.String(),
+		`name="next" value="/source/abc"`,
+	)
+	assert.Contains(
+		t, loginPageGet(h, "//evil.example/", nil).Body.String(),
+		`name="next" value="/"`,
+	)
+
+	cookies := authenticatedCookies(t, sess, "test-user-id", "testuser")
+
+	cases := []struct{ next, want string }{
+		{"/source/abc", "/source/abc"},
+		{"//evil.example/", "/"},
+		{`/\evil.example/`, "/"},
+	}
+
+	for _, c := range cases {
+		w := loginPageGet(h, c.next, cookies)
+
+		assert.Equal(t, http.StatusSeeOther, w.Code, "next %q", c.next)
+		assert.Equal(
+			t, c.want, w.Header().Get("Location"), "next %q", c.next,
+		)
+	}
+}
+
+// TestLoginPage_HasNoLinkToItself: the navigation bar on the login
+// page offers no link to the login page.
+func TestLoginPage_HasNoLinkToItself(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	w := loginPageGet(h, "", nil)
+
+	require.Equal(t, http.StatusOK, w.Code)
+	assert.NotContains(t, w.Body.String(), `href="/pages/login"`)
+}
+
 // TestLogin_UsernameAtLimitCanLogIn shows that a username of exactly
 // database.MaxUsernameBytes still fits in the session cookie. Past
 // what the cookie can carry, a correct login answers 500.

internal/handlers/handlers.go

@@ -13,7 +13,6 @@ import (
 	"sync/atomic"
 
 	"go.uber.org/fx"
-	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -37,6 +36,9 @@ const (
 	tmplKeyError = "Error"
 	// tmplKeyWebhook is the template data key for a webhook.
 	tmplKeyWebhook = "Webhook"
+	// tmplKeyNext is the template data key for the page to return
+	// to after login.
+	tmplKeyNext = "Next"
 )
 
 // errInvalidPassword is returned when a password does not match.
@@ -54,7 +56,6 @@ type HandlersParams struct {
 
 	Logger       *logger.Logger
 	Globals      *globals.Globals
-	Config       *config.Config
 	Database     *database.Database
 	WebhookDBMgr *database.WebhookDBManager
 	Healthcheck  *healthcheck.Healthcheck
@@ -131,7 +132,6 @@ func New(
 	s.templates = map[string]*template.Template{
 		"login.html":         parsePageTemplate("login.html"),
 		"profile.html":       parsePageTemplate("profile.html"),
-		"settings.html":      parsePageTemplate("settings.html"),
 		"sources_list.html":  parsePageTemplate("sources_list.html"),
 		"sources_new.html":   parsePageTemplate("sources_new.html"),
 		"source_detail.html": parsePageTemplate("source_detail.html"),

internal/handlers/handlers_test.go

@@ -83,25 +83,16 @@ func newTestApp(
 ) *fxtest.App {
 	t.Helper()
 
-	return newTestAppWithConfig(
-		t, &config.Config{DataDir: t.TempDir()}, targets...,
-	)
-}
-
-// newTestAppWithConfig is newTestApp over a caller-supplied Config.
-func newTestAppWithConfig(
-	t *testing.T,
-	cfg *config.Config,
-	targets ...any,
-) *fxtest.App {
-	t.Helper()
-
 	return fxtest.New(
 		t,
 		fx.Provide(
 			globals.New,
 			logger.New,
-			func() *config.Config { return cfg },
+			func() *config.Config {
+				return &config.Config{
+					DataDir: t.TempDir(),
+				}
+			},
 			database.New,
 			database.NewWebhookDBManager,
 			healthcheck.New,

internal/handlers/profile_test.go

@@ -160,7 +160,10 @@ func TestUserRoute_Unauthenticated_RedirectedByMiddleware(t *testing.T) {
 		"handler must not be reached for unauthenticated request",
 	)
 	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
+	assert.Equal(
+		t, "/pages/login?next=%2Fuser%2Ftestuser",
+		w.Header().Get("Location"),
+	)
 }
 
 // passwordChangeRequest builds a POST request to the password-change

internal/handlers/settings.go

@@ -1,130 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-	"net/netip"
-	"strconv"
-	"strings"
-
-	"sneak.berlin/go/webhooker/internal/config"
-)
-
-// notSet is what the Settings page shows for a value that is empty.
-const notSet = "not set"
-
-// settingRow is one line of the Settings page: an environment
-// variable, what it controls, and the value the server loaded for it.
-type settingRow struct {
-	Name        string
-	Description string
-	Value       string
-}
-
-// HandleSettings returns a handler for the read-only Settings page,
-// which lists the configuration the server started with.
-func (h *Handlers) HandleSettings() http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		h.renderTemplate(w, r, "settings.html", map[string]any{
-			"Settings": settingRows(h.params.Config),
-		})
-	}
-}
-
-// settingRows lists every field of cfg under the environment variable
-// it is read from, in the order of the README's configuration table.
-// METRICS_PASSWORD and SENTRY_DSN are credentials, so their values
-// never reach the page: only whether they are set.
-func settingRows(cfg *config.Config) []settingRow {
-	metricsUsername := cfg.MetricsUsername
-	if metricsUsername == "" {
-		metricsUsername = notSet
-	}
-
-	return []settingRow{
-		{"WEBHOOKER_ENVIRONMENT", "dev or prod", cfg.Environment},
-		{"PORT", "HTTP listen port", strconv.Itoa(cfg.Port)},
-		{
-			"BIND_ADDRESS",
-			"IP address the HTTP listener binds",
-			cfg.BindAddress,
-		},
-		{
-			"DATA_DIR",
-			"Directory for all SQLite databases",
-			cfg.DataDir,
-		},
-		{
-			"DEBUG",
-			"Enable debug logging",
-			strconv.FormatBool(cfg.Debug),
-		},
-		{
-			"MAINTENANCE_MODE",
-			"Report maintenanceMode: true in the healthcheck JSON. " +
-				"It does not change how any request is served",
-			strconv.FormatBool(cfg.MaintenanceMode),
-		},
-		{
-			"METRICS_USERNAME",
-			"Basic auth username for /metrics",
-			metricsUsername,
-		},
-		{
-			"METRICS_PASSWORD",
-			"Basic auth password for /metrics",
-			setOrNotSet(cfg.MetricsPassword),
-		},
-		{
-			"SENTRY_DSN",
-			"Error reporting DSN. Unset leaves error reporting off",
-			setOrNotSet(cfg.SentryDSN),
-		},
-		{
-			"RETENTION_SWEEP_INTERVAL",
-			"How often the retention reaper and archive sweeper run",
-			cfg.RetentionSweepInterval.String(),
-		},
-		{
-			"SESSION_IDLE_TIMEOUT",
-			"Idle session timeout. Zero or negative disables idle " +
-				"expiry",
-			cfg.SessionIdleTimeout.String(),
-		},
-		{
-			"RECEIVER_RATE_LIMIT",
-			"Receiver requests per minute per IP per entrypoint " +
-				"(10x that per IP across the route)",
-			strconv.Itoa(cfg.ReceiverRateLimit),
-		},
-		{
-			"TRUSTED_PROXIES",
-			"CIDRs whose forwarded headers are trusted",
-			cidrList(cfg.TrustedProxies),
-		},
-		{
-			"ALLOWED_EGRESS_CIDRS",
-			"CIDRs that delivery targets may reach despite the " +
-				"SSRF blocklist",
-			cidrList(cfg.AllowedEgressCIDRs),
-		},
-	}
-}
-
-// setOrNotSet is how the Settings page shows a credential: whether it
-// has a value, never the value itself.
-func setOrNotSet(value string) string {
-	if value == "" {
-		return notSet
-	}
-
-	return "set"
-}
-
-// cidrList renders a CIDR list setting for the Settings page.
-func cidrList(prefixes []netip.Prefix) string {
-	if len(prefixes) == 0 {
-		return "none"
-	}
-
-	return strings.Join(config.PrefixStrings(prefixes), ", ")
-}

internal/handlers/settings_test.go

@@ -1,139 +0,0 @@
-package handlers_test
-
-import (
-	"context"
-	"html"
-	"net/http"
-	"net/http/httptest"
-	"net/netip"
-	"regexp"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// settingsShown renders the Settings page over cfg as a logged-in user
-// and returns the value it shows for each variable name, plus the
-// whole page.
-func settingsShown(
-	t *testing.T, cfg *config.Config,
-) (map[string]string, string) {
-	t.Helper()
-
-	var h *handlers.Handlers
-
-	var sess *session.Session
-
-	app := newTestAppWithConfig(t, cfg, &h, &sess)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/settings", nil,
-	)
-	for _, c := range authenticatedCookies(t, sess, "id", "admin") {
-		req.AddCookie(c)
-	}
-
-	w := httptest.NewRecorder()
-	h.HandleSettings().ServeHTTP(w, req)
-	require.Equal(t, http.StatusOK, w.Code)
-
-	body := w.Body.String()
-
-	row := regexp.MustCompile(
-		`<code[^>]*>([A-Z_]+)</code>\s*<code[^>]*>([^<]*)</code>`,
-	)
-
-	shown := map[string]string{}
-	for _, match := range row.FindAllStringSubmatch(body, -1) {
-		shown[match[1]] = html.UnescapeString(match[2])
-	}
-
-	return shown, body
-}
-
-func TestSettingsPageShowsLoadedConfiguration(t *testing.T) {
-	t.Parallel()
-
-	const metricsPassword = "metrics-password-1f9a"
-
-	// No two rows show the same value: DEBUG and MAINTENANCE_MODE, and
-	// METRICS_PASSWORD and SENTRY_DSN, get opposite values, so each row
-	// is checked against its own field.
-	cfg := &config.Config{
-		DataDir:                t.TempDir(),
-		Debug:                  true,
-		MaintenanceMode:        false,
-		Environment:            config.EnvironmentDev,
-		MetricsUsername:        "scraper",
-		MetricsPassword:        metricsPassword,
-		Port:                   9123,
-		SentryDSN:              "",
-		BindAddress:            "192.0.2.10",
-		RetentionSweepInterval: 17 * time.Minute,
-		SessionIdleTimeout:     3 * time.Hour,
-		ReceiverRateLimit:      77,
-		TrustedProxies: []netip.Prefix{
-			netip.MustParsePrefix("10.1.0.0/16"),
-		},
-		AllowedEgressCIDRs: []netip.Prefix{
-			netip.MustParsePrefix("192.168.5.0/24"),
-			netip.MustParsePrefix("fd00::/8"),
-		},
-	}
-
-	shown, body := settingsShown(t, cfg)
-
-	assert.Equal(t, map[string]string{
-		"WEBHOOKER_ENVIRONMENT":    "dev",
-		"PORT":                     "9123",
-		"BIND_ADDRESS":             "192.0.2.10",
-		"DATA_DIR":                 cfg.DataDir,
-		"DEBUG":                    "true",
-		"MAINTENANCE_MODE":         "false",
-		"METRICS_USERNAME":         "scraper",
-		"METRICS_PASSWORD":         "set",
-		"SENTRY_DSN":               "not set",
-		"RETENTION_SWEEP_INTERVAL": "17m0s",
-		"SESSION_IDLE_TIMEOUT":     "3h0m0s",
-		"RECEIVER_RATE_LIMIT":      "77",
-		"TRUSTED_PROXIES":          "10.1.0.0/16",
-		"ALLOWED_EGRESS_CIDRS":     "192.168.5.0/24, fd00::/8",
-	}, shown)
-
-	assert.NotContains(t, body, metricsPassword)
-	assert.Contains(
-		t, body, `href="/settings"`,
-		"the navigation bar links to the page",
-	)
-}
-
-func TestSettingsPageShowsUnsetValues(t *testing.T) {
-	t.Parallel()
-
-	const (
-		sentryKey = "dsnkey7c2e"
-		sentryDSN = "https://" + sentryKey + "@errors.example.com/42"
-	)
-
-	// SENTRY_DSN is set here and empty in the test above, the opposite
-	// of METRICS_PASSWORD, so each secret is seen both set and not set.
-	shown, body := settingsShown(t, &config.Config{
-		DataDir:   t.TempDir(),
-		SentryDSN: sentryDSN,
-	})
-
-	assert.Equal(t, "not set", shown["METRICS_USERNAME"])
-	assert.Equal(t, "not set", shown["METRICS_PASSWORD"])
-	assert.Equal(t, "set", shown["SENTRY_DSN"])
-	assert.NotContains(t, body, sentryKey)
-	assert.Equal(t, "none", shown["TRUSTED_PROXIES"])
-	assert.Equal(t, "none", shown["ALLOWED_EGRESS_CIDRS"])
-}

internal/middleware/middleware.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"net"
 	"net/http"
+	"net/url"
 	"sync"
 	"time"
 
@@ -366,6 +367,30 @@ func (s *Middleware) CORS() func(http.Handler) http.Handler {
 	}
 }
 
+// NextParam is the query parameter on the login redirect, and the
+// login form field, that holds the page to return to after login.
+const NextParam = "next"
+
+// MaxNextBytes bounds the NextParam value. The login page writes it
+// into its form, and every page is rendered into a buffer first, so
+// without a bound a request would choose the size of that buffer.
+const MaxNextBytes = 2048
+
+// loginURL is the login page RequireAuth redirects to. A GET carries
+// its own path and query in NextParam so that logging in returns to
+// it, unless they are longer than MaxNextBytes; loginDestination in
+// the handlers package checks whether that value is safe to follow.
+// Other methods carry nothing, since a redirect cannot repeat them.
+func loginURL(r *http.Request) string {
+	next := r.URL.RequestURI()
+
+	if r.Method != http.MethodGet || len(next) > MaxNextBytes {
+		return "/pages/login"
+	}
+
+	return "/pages/login?" + url.Values{NextParam: {next}}.Encode()
+}
+
 // RequireAuth returns middleware that checks for a valid session.
 // Unauthenticated users are redirected to the login page.
 func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
@@ -381,7 +406,7 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
 					"error", err,
 				)
 				http.Redirect(
-					w, r, "/pages/login", http.StatusSeeOther,
+					w, r, loginURL(r), http.StatusSeeOther,
 				)
 
 				return
@@ -409,7 +434,7 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
 					),
 				)
 				http.Redirect(
-					w, r, "/pages/login", http.StatusSeeOther,
+					w, r, loginURL(r), http.StatusSeeOther,
 				)
 
 				return

internal/middleware/middleware_test.go

@@ -338,6 +338,76 @@ func TestRequireAuth_NoSession_RedirectsToLogin(t *testing.T) {
 			"unauthenticated request",
 	)
 	assert.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(
+		t, "/pages/login?next=%2Fdashboard", w.Header().Get("Location"),
+	)
+}
+
+// TestRequireAuth_LoginRedirectCarriesOnlyAGet pins what the login
+// redirect carries: a GET's path and query, so logging in can return
+// there, and nothing for a POST, which a redirect cannot repeat.
+func TestRequireAuth_LoginRedirectCarriesOnlyAGet(t *testing.T) {
+	t.Parallel()
+
+	m, _ := testMiddleware(t, config.EnvironmentDev)
+
+	handler := m.RequireAuth()(http.HandlerFunc(
+		func(_ http.ResponseWriter, _ *http.Request) {},
+	))
+
+	get := httptest.NewRequestWithContext(
+		context.Background(),
+		http.MethodGet, "/source/abc/logs?page=2", nil,
+	)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, get)
+
+	assert.Equal(
+		t, "/pages/login?next=%2Fsource%2Fabc%2Flogs%3Fpage%3D2",
+		w.Header().Get("Location"),
+	)
+
+	post := httptest.NewRequestWithContext(
+		context.Background(),
+		http.MethodPost, "/source/abc/delete", nil,
+	)
+	w = httptest.NewRecorder()
+	handler.ServeHTTP(w, post)
+
+	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
+}
+
+// TestRequireAuth_LoginRedirectLeavesOutALongURL: a GET whose path
+// and query are longer than the login page accepts goes to the plain
+// login page, so a long URL does not make the redirect long.
+func TestRequireAuth_LoginRedirectLeavesOutALongURL(t *testing.T) {
+	t.Parallel()
+
+	m, _ := testMiddleware(t, config.EnvironmentDev)
+
+	handler := m.RequireAuth()(http.HandlerFunc(
+		func(_ http.ResponseWriter, _ *http.Request) {},
+	))
+
+	atLimit := "/" + strings.Repeat("a", middleware.MaxNextBytes-1)
+
+	get := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, atLimit, nil,
+	)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, get)
+
+	assert.Equal(
+		t, "/pages/login?next=%2F"+atLimit[1:],
+		w.Header().Get("Location"),
+	)
+
+	get = httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, atLimit+"a", nil,
+	)
+	w = httptest.NewRecorder()
+	handler.ServeHTTP(w, get)
+
 	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
 }
 
@@ -443,7 +513,9 @@ func TestRequireAuth_UnauthenticatedSession_RedirectsToLogin(
 			"unauthenticated session",
 	)
 	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
+	assert.Equal(
+		t, "/pages/login?next=%2Fdashboard", w.Header().Get("Location"),
+	)
 }
 
 // --- RequireAuth Session Expiry Tests ---
@@ -541,7 +613,9 @@ func TestRequireAuth_IdleExpiredSession_RedirectsToLogin(
 		"handler should not run for an idle-expired session",
 	)
 	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
+	assert.Equal(
+		t, "/pages/login?next=%2Fdashboard", w.Header().Get("Location"),
+	)
 	assert.Empty(
 		t, sessionCookies(w),
 		"an expired session must not be refreshed",

internal/server/routes.go

@@ -141,7 +141,6 @@ func (s *Server) setupRoutes() {
 
 	s.setupPageRoutes()
 	s.setupUserRoutes()
-	s.setupSettingsRoutes()
 	s.setupSourceRoutes()
 	s.setupWebhookRoutes()
 }
@@ -183,21 +182,6 @@ func (s *Server) setupUserRoutes() {
 	})
 }
 
-// setupSettingsRoutes serves the Settings page. It is GET only:
-// configuration comes from the environment and nothing here changes
-// it.
-func (s *Server) setupSettingsRoutes() {
-	s.router.Route("/settings", func(r chi.Router) {
-		// MaxBodySize precedes CSRF and RequireAuth deliberately;
-		// see maxFormBodySize for why, and for what it costs.
-		r.Use(s.mw.MaxBodySize(maxFormBodySize))
-		r.Use(s.mw.CSRF())
-		r.Use(s.mw.NoCache())
-		r.Use(s.mw.RequireAuth())
-		r.Get("/", s.h.HandleSettings())
-	})
-}
-
 func (s *Server) setupSourceRoutes() {
 	s.router.Route("/sources", func(r chi.Router) {
 		// MaxBodySize precedes CSRF and RequireAuth deliberately;

internal/server/routes_test.go

@@ -680,6 +680,44 @@ func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) {
 	)
 }
 
+// TestPagesLogin_ReturnsToTheRequestedPage is
+// https://git.eeqj.de/sneak/webhooker/issues/384: a page opened while
+// logged out leads to the login page, and logging in from there lands
+// on that page, query included.
+func TestPagesLogin_ReturnsToTheRequestedPage(t *testing.T) {
+	t.Parallel()
+
+	const (
+		username = "operator"
+		password = "correct-horse-battery-staple"
+	)
+
+	env := newTestEnv(t)
+	userID, _ := env.seedUser(t, username, password)
+	asked := "/source/" + env.seedWebhook(t, userID).ID + "/logs?page=2"
+
+	bounced := env.get(asked, nil)
+	require.Equal(t, http.StatusSeeOther, bounced.Code)
+
+	loginPage := bounced.Header().Get("Location")
+
+	match := regexp.MustCompile(`name="next" value="([^"]*)"`).
+		FindStringSubmatch(env.get(loginPage, nil).Body.String())
+	require.Len(t, match, 2, "the login form must carry the page")
+
+	token, cookies := env.csrfFrom(t, loginPage, nil)
+
+	form := url.Values{}
+	form.Set("csrf_token", token)
+	form.Set("username", username)
+	form.Set("password", password)
+	form.Set("next", html.UnescapeString(match[1]))
+
+	w := env.post("/pages/login", form, cookies)
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(t, asked, w.Header().Get("Location"))
+}
+
 // --- /user/{username} group ---
 
 // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged
@@ -830,7 +868,10 @@ func TestSourceLogsBody_OtherUser404s(t *testing.T) {
 
 	anon := env.get(path, nil)
 	assert.Equal(t, http.StatusSeeOther, anon.Code)
-	assert.Equal(t, "/pages/login", anon.Header().Get("Location"))
+	assert.Equal(
+		t, "/pages/login?next="+url.QueryEscape(path),
+		anon.Header().Get("Location"),
+	)
 }
 
 // TestDeliveryReplay_PostOnlyAndCSRFProtected walks the replay action

internal/server/settings_test.go

@@ -1,22 +0,0 @@
-package server_test
-
-import (
-	"net/http"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestSettingsPageIsBehindLogin(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	w := env.get("/settings", nil)
-	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
-
-	w = env.get("/settings", env.authCookies(t, "id", "admin"))
-	assert.Equal(t, http.StatusOK, w.Code)
-	assert.Contains(t, w.Body.String(), "WEBHOOKER_ENVIRONMENT")
-}

templates/login.html

@@ -24,6 +24,7 @@
 
             <form method="POST" action="/pages/login" class="space-y-6">
                 <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">
+                <input type="hidden" name="next" value="{{.Next}}">
                 <div class="form-group">
                     <label for="username" class="label">Username</label>
                     <input

templates/navbar.html

@@ -6,18 +6,19 @@
         </div>
 
         <!-- Mobile menu button -->
+        {{if .User}}
         <button @click="open = !open" class="md:hidden p-2 rounded-md text-gray-500 hover:bg-gray-100">
             <svg class="w-6 h-6" fill="none" stroke="currentColor" viewBox="0 0 24 24">
                 <path x-show="!open" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M4 6h16M4 12h16M4 18h16"/>
                 <path x-show="open" x-cloak stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M6 18L18 6M6 6l12 12"/>
             </svg>
         </button>
+        {{end}}
 
         <!-- Desktop navigation -->
         <div class="hidden md:flex items-center gap-4">
             {{if .User}}
             <a href="/sources" class="btn-text">Webhooks</a>
-            <a href="/settings" class="btn-text">Settings</a>
             <a href="/user/{{.User.Username}}" class="btn-text">
                 <svg class="w-5 h-5 mr-1" fill="currentColor" viewBox="0 0 16 16">
                     <path d="M11 6a3 3 0 1 1-6 0 3 3 0 0 1 6 0z"/>
@@ -29,8 +30,6 @@
                 <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">
                 <button type="submit" class="btn-text">Logout</button>
             </form>
-            {{else}}
-            <a href="/pages/login" class="btn-primary">Login</a>
             {{end}}
         </div>
     </div>
@@ -40,14 +39,11 @@
         <div class="flex flex-col gap-2">
             {{if .User}}
             <a href="/sources" class="btn-text w-full text-left">Webhooks</a>
-            <a href="/settings" class="btn-text w-full text-left">Settings</a>
             <a href="/user/{{.User.Username}}" class="btn-text w-full text-left">Profile</a>
             <form method="POST" action="/pages/logout">
                 <input type="hidden" name="csrf_token" value="{{.CSRFToken}}">
                 <button type="submit" class="btn-text w-full text-left">Logout</button>
             </form>
-            {{else}}
-            <a href="/pages/login" class="btn-primary w-full">Login</a>
             {{end}}
         </div>
     </div>

templates/settings.html

@@ -1,24 +0,0 @@
-{{template "base" .}}
-
-{{define "title"}}Settings - Webhooker{{end}}
-
-{{define "content"}}
-<div class="max-w-6xl mx-auto px-6 py-8">
-    <h1 class="text-2xl font-medium text-gray-900">Settings</h1>
-    <p class="text-sm text-gray-500 mt-1 mb-6">The configuration this server started with. It is set in the server's environment and cannot be changed here.</p>
-
-    <div class="card">
-        <div class="divide-y divide-gray-100">
-            {{range .Settings}}
-            <div class="p-4">
-                <div class="flex justify-between items-start gap-4">
-                    <code class="text-sm font-medium text-gray-900">{{.Name}}</code>
-                    <code class="text-sm text-gray-900 break-all">{{.Value}}</code>
-                </div>
-                <p class="text-sm text-gray-500 mt-1">{{.Description}}</p>
-            </div>
-            {{end}}
-        </div>
-    </div>
-</div>
-{{end}}