diff --git a/README.md b/README.md
index f6a682d..4206497 100644
--- a/README.md
+++ b/README.md
@@ -2695,12 +2695,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 request to any of these is redirected to `/pages/login`. A
+`GET` carries its path and query there as `next`, so logging in returns
+to the page that was asked for.
+
| Method | Path | Description |
| ------ | ------------------------ | ----------- |
| `GET` | `/user/{username}` | User profile page |
diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go
index 39fa5dc..e3ef9a8 100644
--- a/internal/handlers/auth.go
+++ b/internal/handlers/auth.go
@@ -2,19 +2,60 @@ 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(w, r, next, http.StatusSeeOther)
return
}
@@ -22,6 +63,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 +119,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(
+ w, r,
+ loginDestination(r.PostFormValue(middleware.NextParam)),
+ http.StatusSeeOther,
+ )
}
}
@@ -91,6 +138,9 @@ func (h *Handlers) renderLoginError(
) {
data := map[string]any{
tmplKeyError: msg,
+ tmplKeyNext: loginDestination(
+ r.PostFormValue(middleware.NextParam),
+ ),
}
w.WriteHeader(status)
diff --git a/internal/handlers/auth_test.go b/internal/handlers/auth_test.go
index 1fdbcb1..3211085 100644
--- a/internal/handlers/auth_test.go
+++ b/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.
diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go
index 349771e..16e8194 100644
--- a/internal/handlers/handlers.go
+++ b/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.
diff --git a/internal/handlers/profile_test.go b/internal/handlers/profile_test.go
index 18692c3..64ab020 100644
--- a/internal/handlers/profile_test.go
+++ b/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
diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go
index 98140fb..f4bc0b4 100644
--- a/internal/middleware/middleware.go
+++ b/internal/middleware/middleware.go
@@ -6,6 +6,7 @@ import (
"log/slog"
"net"
"net/http"
+ "net/url"
"sync"
"time"
@@ -366,6 +367,24 @@ 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"
+
+// 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; HandleLoginSubmit decides whether that value is safe to follow.
+// Other methods carry nothing, since a redirect cannot repeat them.
+func loginURL(r *http.Request) string {
+ if r.Method != http.MethodGet {
+ return "/pages/login"
+ }
+
+ return "/pages/login?" + url.Values{
+ NextParam: {r.URL.RequestURI()},
+ }.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 +400,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 +428,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
diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go
index 097162d..3d71395 100644
--- a/internal/middleware/middleware_test.go
+++ b/internal/middleware/middleware_test.go
@@ -338,6 +338,42 @@ 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"))
}
@@ -443,7 +479,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 +579,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",
diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go
index a06272f..e453a0f 100644
--- a/internal/server/routes_test.go
+++ b/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
diff --git a/templates/login.html b/templates/login.html
index 6e45e67..6fd64f8 100644
--- a/templates/login.html
+++ b/templates/login.html
@@ -24,6 +24,7 @@
- {{else}}
- Login
{{end}}