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

@@ -2703,12 +2703,16 @@ 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 |

internal/delivery/export_test.go

@@ -40,6 +40,11 @@ const (
 	ExportPendingSweepMinAge = pendingSweepMinAge
 )
 
+// ExportIsBlockedIP exposes isBlockedIP for testing.
+func ExportIsBlockedIP(ip net.IP) bool {
+	return isBlockedIP(ip)
+}
+
 // NewTestGuard builds an SSRF Guard from an explicit egress
 // allowlist, without going through config. Passing no prefixes
 // yields the default guard, which blocks every private/reserved
@@ -65,11 +70,6 @@ func ExportBlockedNetworks() []*net.IPNet {
 	return blockedNetworks
 }
 
-// ExportBlockedPublicNetworks exposes blockedPublicNetworks.
-func ExportBlockedPublicNetworks() []*net.IPNet {
-	return blockedPublicNetworks
-}
-
 // ExportIsForwardableHeader exposes isForwardableHeader.
 func ExportIsForwardableHeader(name string) bool {
 	return isForwardableHeader(name)

internal/delivery/ssrf.go

@@ -25,16 +25,8 @@ var (
 	errNoIPs      = errors.New(
 		"hostname resolved to no IP addresses",
 	)
-	// ErrBlockedPrivateOrReservedIP reports an address in the
-	// default blocklist's private and reserved ranges,
-	// blockedNetworks.
-	ErrBlockedPrivateOrReservedIP = errors.New(
-		"blocked private or reserved address",
-	)
-	// errBlockedPublicMetadata reports a public address on the
-	// default blocklist, one in blockedPublicNetworks.
-	errBlockedPublicMetadata = errors.New(
-		"blocked cloud metadata address",
+	errBlockedIP = errors.New(
+		"blocked private, reserved or cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -45,31 +37,21 @@ var (
 	)
 )
 
-// blockedNetworks and blockedPublicNetworks together are the
-// default blocklist: the private and reserved IP ranges, plus
-// the public cloud metadata addresses, that are blocked to
-// prevent SSRF attacks. An operator can permit specific blocks
-// out of this set with ALLOWED_EGRESS_CIDRS; see Guard.
-//
-// blockedNetworks holds the private and reserved IP ranges.
-//
-//nolint:gochecknoglobals // package-level network list is appropriate here
-var blockedNetworks []*net.IPNet
-
-// blockedPublicNetworks holds the default blocklist's public
-// addresses, kept apart from blockedNetworks so that they are
-// refused as cloud metadata addresses, never as private or
-// reserved ones.
+// blockedNetworks is the default blocklist: the private and
+// reserved IP ranges, plus the public cloud metadata addresses,
+// that are blocked to prevent SSRF attacks. An operator can
+// permit specific blocks out of this set with
+// ALLOWED_EGRESS_CIDRS; see Guard.
 //
 // A public address belongs on the default blocklist only if it
 // hands credentials, user data or bootstrap material to whatever
-// can reach it, without the caller presenting anything; it goes
-// in this list. A provider's other public addresses are not
-// refused, since reaching them can be legitimate and no list of
-// them could be complete.
+// can reach it, without the caller presenting anything. A
+// provider's other public addresses are not refused, since
+// reaching them can be legitimate and no list of them could be
+// complete.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
-var blockedPublicNetworks []*net.IPNet
+var blockedNetworks []*net.IPNet
 
 // alwaysBlockedNetworks are the ranges no configuration can
 // open: the link-local blocks and the cloud instance metadata
@@ -106,8 +88,8 @@ var blockedPublicNetworks []*net.IPNet
 // when it clears both halves. Nothing in this list can be
 // reopened, so putting a public address here leaves the operator
 // no escape hatch at all — the condition ALLOWED_EGRESS_CIDRS
-// exists to remove. Default-block it in blockedPublicNetworks
-// instead, which an allowlist can override.
+// exists to remove. Default-block it in blockedNetworks instead,
+// which an allowlist can override.
 //
 // This is a criterion, not an enumeration of every metadata
 // address in existence.
@@ -148,9 +130,6 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-	})
-
-	blockedPublicNetworks = mustParseCIDRs([]string{
 		// Azure WireServer, a public address that serves VM credentials.
 		"168.63.129.16/32",
 	})
@@ -246,6 +225,13 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 	return false
 }
 
+// isBlockedIP checks whether an IP address falls within
+// the default blocklist, before any operator allowlist is
+// considered.
+func isBlockedIP(ip net.IP) bool {
+	return matchesAny(blockedNetworks, ip)
+}
+
 // Guard makes every SSRF decision in the process.
 //
 // It holds the operator's ALLOWED_EGRESS_CIDRS allowlist and
@@ -346,8 +332,7 @@ func (g *Guard) allows(ip net.IP) bool {
 //     consulted, so no configured CIDR reaches link-local or a
 //     cloud metadata endpoint at a non-public address.
 //  2. The allowlist is consulted next, so a listed private
-//     network, or a listed public address on the default
-//     blocklist, becomes reachable.
+//     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.
 func (g *Guard) checkIP(ip net.IP) error {
 	if matchesAny(alwaysBlockedNetworks, ip) {
@@ -360,15 +345,9 @@ func (g *Guard) checkIP(ip net.IP) error {
 		return nil
 	}
 
-	if matchesAny(blockedNetworks, ip) {
+	if isBlockedIP(ip) {
 		return fmt.Errorf(
-			"target IP %s: %w", ip, ErrBlockedPrivateOrReservedIP,
-		)
-	}
-
-	if matchesAny(blockedPublicNetworks, ip) {
-		return fmt.Errorf(
-			"target IP %s: %w", ip, errBlockedPublicMetadata,
+			"target IP %s: %w", ip, errBlockedIP,
 		)
 	}
 

internal/delivery/ssrf_allowlist_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http/httptest"
 	"net/netip"
 	"net/url"
-	"slices"
 	"testing"
 	"time"
 
@@ -24,10 +23,6 @@ const (
 	metadataIP  = "169.254.169.254"
 	metadataURL = "http://" + metadataIP + "/latest/meta-data/"
 
-	// linkLocalIPv4 is the IPv4 link-local block, which holds
-	// metadataIP.
-	linkLocalIPv4 = "169.254.0.0/16"
-
 	// loopbackHookURL is a target on this host: blocked by
 	// default, reachable only once an operator allowlists
 	// loopback.
@@ -242,7 +237,7 @@ func linkLocalRefusedCases() []metadataAlwaysRefusedCase {
 		},
 		{
 			name:   "whole link-local block",
-			allow:  linkLocalIPv4,
+			allow:  "169.254.0.0/16",
 			target: metadataURL,
 		},
 		{
@@ -417,9 +412,6 @@ func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
 		"WireServer must be refused by the default blocklist, "+
 			"which an allowlist can override",
 	)
-	require.NotErrorIs(t, err, delivery.ErrBlockedPrivateOrReservedIP,
-		"WireServer is public, not private or reserved",
-	)
 
 	assertDialRefused(t, defaultGuard, target)
 
@@ -504,7 +496,7 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	want := []string{
 		// IPv4 link-local: the 169.254.169.254 metadata
 		// service on AWS, Azure and others.
-		linkLocalIPv4,
+		"169.254.0.0/16",
 		// IPv6 link-local.
 		"fe80::/10",
 		// AWS IPv6 IMDS, inside the ULA space an operator may
@@ -534,76 +526,6 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	assert.Equal(t, want, got)
 }
 
-// TestDefaultBlocklist_PinnedSet pins the default blocklist, its
-// private and reserved ranges and its public addresses together,
-// and how ALLOWED_EGRESS_CIDRS opens each entry: listing an entry
-// opens it unless the unconditional set also holds it.
-func TestDefaultBlocklist_PinnedSet(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		cidr       string
-		reopenable bool
-	}{
-		{"127.0.0.0/8", true},
-		{"10.0.0.0/8", true},
-		{"172.16.0.0/12", true},
-		{"192.168.0.0/16", true},
-		{linkLocalIPv4, false},
-		{"0.0.0.0/8", true},
-		{"100.64.0.0/10", true},
-		{"192.0.0.0/24", true},
-		{"192.0.2.0/24", true},
-		{"198.18.0.0/15", true},
-		{"198.51.100.0/24", true},
-		{"203.0.113.0/24", true},
-		{"224.0.0.0/4", true},
-		{"240.0.0.0/4", true},
-		{"::1/128", true},
-		{"fc00::/7", true},
-		{"fe80::/10", false},
-		{"168.63.129.16/32", true},
-	}
-
-	want := make([]string, 0, len(tests))
-	for _, tt := range tests {
-		want = append(want, tt.cidr)
-	}
-
-	nets := slices.Concat(
-		delivery.ExportBlockedNetworks(),
-		delivery.ExportBlockedPublicNetworks(),
-	)
-
-	got := make([]string, 0, len(nets))
-	for _, n := range nets {
-		got = append(got, n.String())
-	}
-
-	assert.ElementsMatch(t, want, got)
-
-	for _, tt := range tests {
-		t.Run(tt.cidr, func(t *testing.T) {
-			t.Parallel()
-
-			prefix := netip.MustParsePrefix(tt.cidr)
-			ip := net.IP(prefix.Addr().AsSlice())
-
-			require.Error(t,
-				delivery.NewTestGuard().ExportCheckIP(ip),
-				"the default guard must refuse %s", ip,
-			)
-
-			err := delivery.NewTestGuard(prefix).ExportCheckIP(ip)
-			if tt.reopenable {
-				assert.NoError(t, err, "listing %s must open it", tt.cidr)
-			} else {
-				assert.Error(t, err, "listing %s must not open it", tt.cidr)
-			}
-		})
-	}
-}
-
 // requireLoopback fails the test unless rawURL's host is a
 // loopback address, so the allowlist test cannot silently stop
 // exercising a blocked range.

internal/delivery/ssrf_test.go

@@ -10,7 +10,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-func TestGuardCheckIP_PrivateRanges(t *testing.T) {
+func TestIsBlockedIP_PrivateRanges(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -56,14 +56,12 @@ func TestGuardCheckIP_PrivateRanges(t *testing.T) {
 				"failed to parse IP %s", tt.ip,
 			)
 
-			refused := delivery.NewTestGuard().ExportCheckIP(ip) != nil
-
 			assert.Equal(t,
 				tt.blocked,
-				refused,
-				"default guard refuses %s = %v, want %v",
+				delivery.ExportIsBlockedIP(ip),
+				"isBlockedIP(%s) = %v, want %v",
 				tt.ip,
-				refused,
+				delivery.ExportIsBlockedIP(ip),
 				tt.blocked,
 			)
 		})

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

@@ -36,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.

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/source_management.go

@@ -1577,22 +1577,11 @@ func (h *Handlers) validateTargetURL(
 			"url", delivery.MaskURL(targetURL),
 			"error", err,
 		)
-
-		msg := "Invalid target URL: " + err.Error()
-
-		// Only a private or reserved address's refusal says how
-		// to allow it. Metadata refusals never do: link-local and
-		// the other unconditional metadata addresses cannot be
-		// opened, and the default blocklist's public addresses,
-		// which listing does open, hand out credentials.
-		if errors.Is(err, delivery.ErrBlockedPrivateOrReservedIP) {
-			msg += ". Private and reserved addresses are refused " +
-				"by default; the server's ALLOWED_EGRESS_CIDRS " +
-				"setting allows named networks (see \"Allowing " +
-				"egress to your own network\" in the README)."
-		}
-
-		http.Error(w, msg, http.StatusBadRequest)
+		http.Error(
+			w,
+			"Invalid target URL: "+err.Error(),
+			http.StatusBadRequest,
+		)
 
 		return err
 	}

internal/handlers/target_private_refusal_test.go

@@ -1,116 +0,0 @@
-package handlers_test
-
-import (
-	"net/http"
-	"net/url"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// privateRefusalHint is the sentence that tells an operator a private
-// destination is refused on purpose, and how to allow one.
-const privateRefusalHint = "Private and reserved addresses are " +
-	"refused by default; the server's ALLOWED_EGRESS_CIDRS setting " +
-	"allows named networks (see \"Allowing egress to your own " +
-	"network\" in the README)."
-
-// TestTargetRefusal_PrivateDestinationSaysHowToAllowIt covers both
-// target types that take a URL, on add and on edit.
-func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	targetTypes := []database.TargetType{
-		database.TargetTypeHTTP,
-		database.TargetTypeSlack,
-	}
-
-	for _, targetType := range targetTypes {
-		t.Run(string(targetType), func(t *testing.T) {
-			t.Parallel()
-
-			webhook := seedWebhookWithRetention(t, env.db, 30)
-			targetsPath := "/source/" + webhook.ID + "/targets"
-
-			form := url.Values{}
-			form.Set("name", "private")
-			form.Set("type", string(targetType))
-			form.Set("url", editBlockedURL)
-
-			added := serveTarget(
-				env, http.MethodPost, targetsPath, form,
-			)
-			assert.Equal(t, http.StatusBadRequest, added.Code)
-			assert.Contains(
-				t, added.Body.String(), privateRefusalHint,
-			)
-
-			form.Set("url", editOriginalURL)
-
-			created := serveTarget(
-				env, http.MethodPost, targetsPath, form,
-			)
-			require.Equal(
-				t, http.StatusSeeOther, created.Code,
-				created.Body.String(),
-			)
-
-			targets := targetsForWebhook(t, env.db, webhook.ID)
-			require.Len(t, targets, 1)
-
-			form.Set("url", editBlockedURL)
-
-			edited := submitTargetEdit(
-				env, webhook.ID, targets[0].ID, form,
-			)
-			assert.Equal(t, http.StatusBadRequest, edited.Code)
-			assert.Contains(
-				t, edited.Body.String(), privateRefusalHint,
-			)
-		})
-	}
-}
-
-// TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt: no
-// setting opens a link-local address, and Azure's WireServer hands out
-// VM credentials, so neither refusal points at the setting.
-func TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-
-	metadataURLs := map[string]string{
-		"link-local": "http://169.254.169.254/latest/meta-data/",
-		"wireserver": "http://168.63.129.16/?comp=versions",
-	}
-
-	for name, metadataURL := range metadataURLs {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			webhook := seedWebhookWithRetention(t, env.db, 30)
-
-			form := url.Values{}
-			form.Set("name", "metadata")
-			form.Set("type", string(database.TargetTypeHTTP))
-			form.Set("url", metadataURL)
-
-			w := serveTarget(
-				env, http.MethodPost,
-				"/source/"+webhook.ID+"/targets", form,
-			)
-			assert.Equal(t, http.StatusBadRequest, w.Code)
-			assert.NotContains(
-				t, w.Body.String(), privateRefusalHint,
-			)
-		})
-	}
-}

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_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

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,12 +6,14 @@
         </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">
@@ -28,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>
@@ -44,8 +44,6 @@
                 <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>