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

internal/handlers/auth.go

@@ -2,19 +2,62 @@ 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"
 )
 
+// maxNextBytes bounds the page to return to after login. The login
+// page writes it into its form, and every page is rendered into a
+// buffer first (see executeTemplate), so without a bound a request
+// would choose the size of that buffer.
+const maxNextBytes = 2048
+
+// 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) > 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 +65,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 +121,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 +140,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,139 @@ 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/", "/"},
+		{"/" + 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.
+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")
+	w := loginPageGet(h, "/source/abc", cookies)
+
+	assert.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(t, "/source/abc", w.Header().Get("Location"))
+}
+
+// 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

@@ -158,7 +158,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