Preserve safe return paths through sign-in

doc/ideen.md

@@ -1,6 +1,7 @@
 # Muss
 
 # Demnächst und konkret
+OpenApi einbauen
 
 # Vielleicht 
 - Detailansicht und Bearbeitenansicht trennen?

internal/web/auth.go

@@ -15,6 +15,7 @@ type signInForm struct {
 	Email         string `form:"email"`
 	Password      string `form:"password"`
 	RememberEmail bool   `form:"remember_email"`
+	ReturnTo      string `form:"return_to"`
 	Errors        map[string]string
 	Message       string
 }
@@ -30,12 +31,17 @@ type activationForm struct {
 }
 
 func (app *application) signIn(w http.ResponseWriter, r *http.Request) {
+	returnTo := safeReturnPath(r.URL.Query().Get("return_to"))
 	if app.isAuthenticated(r) {
+		if returnTo != "" {
+			http.Redirect(w, r, returnTo, http.StatusSeeOther)
+			return
+		}
 		http.Redirect(w, r, app.authenticatedLandingPage(r), http.StatusSeeOther)
 		return
 	}
 	data := app.newTemplateData(r)
-	form := signInForm{Errors: make(map[string]string)}
+	form := signInForm{ReturnTo: returnTo, Errors: make(map[string]string)}
 	if cookie, err := r.Cookie("gardomatic_remembered_email"); err == nil {
 		if decoded, decodeErr := base64.RawURLEncoding.DecodeString(cookie.Value); decodeErr == nil {
 			form.Email, form.RememberEmail = string(decoded), true
@@ -52,6 +58,7 @@ func (app *application) signInPost(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	form.Email = strings.TrimSpace(form.Email)
+	form.ReturnTo = safeReturnPath(form.ReturnTo)
 	form.Errors = make(map[string]string)
 	if form.Email == "" {
 		form.Errors["email"] = "E-Mail-Adresse ist erforderlich."
@@ -99,6 +106,10 @@ func (app *application) signInPost(w http.ResponseWriter, r *http.Request) {
 		http.Redirect(w, r, webPath("activate"), http.StatusSeeOther)
 		return
 	}
+	if form.ReturnTo != "" {
+		http.Redirect(w, r, form.ReturnTo, http.StatusSeeOther)
+		return
+	}
 	http.Redirect(w, r, pathWithQuery(webPath("gardens"), "auto", 1), http.StatusSeeOther)
 }
 

internal/web/flows_test.go

@@ -272,8 +272,32 @@ func TestProtectedPageRedirectsWithoutSession(t *testing.T) {
 	if response.Code != http.StatusSeeOther {
 		t.Fatalf("status: got %d, want %d", response.Code, http.StatusSeeOther)
 	}
-	if location := response.Header().Get("Location"); location != "/login" {
-		t.Errorf("Location: got %q, want %q", location, "/login")
+	if location := response.Header().Get("Location"); location != "/login?return_to=%2Fgardens" {
+		t.Errorf("Location: got %q, want login with return path", location)
+	}
+}
+
+func TestProtectedInvitationRedirectPreservesToken(t *testing.T) {
+	apiHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/v1/session" {
+			w.WriteHeader(http.StatusUnauthorized)
+			_, _ = w.Write([]byte(`{"error":"you must be authenticated"}`))
+			return
+		}
+		http.NotFound(w, r)
+	})
+	app := newAPIBackedTestApplication(t, apiHandler)
+	request := httptest.NewRequest(http.MethodGet, "/invite?token=garden-invite-token", nil)
+	response := httptest.NewRecorder()
+
+	app.routes().ServeHTTP(response, request)
+
+	if response.Code != http.StatusSeeOther {
+		t.Fatalf("status: got %d, want %d", response.Code, http.StatusSeeOther)
+	}
+	want := "/login?return_to=%2Finvite%3Ftoken%3Dgarden-invite-token"
+	if location := response.Header().Get("Location"); location != want {
+		t.Errorf("Location: got %q, want %q", location, want)
 	}
 }
 
@@ -288,7 +312,7 @@ func TestSignInForwardsAPISessionCookie(t *testing.T) {
 		_, _ = w.Write([]byte(`{"user":{"id":7,"name":"Alice","activated":true}}`))
 	})
 	app := newAPIBackedTestApplication(t, apiHandler)
-	form := url.Values{"email": {"alice@example.com"}, "password": {"correct horse battery staple"}}
+	form := url.Values{"email": {"alice@example.com"}, "password": {"correct horse battery staple"}, "return_to": {"/invite?token=garden-invite-token"}}
 	request := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
 	request.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	request = request.WithContext(client.NewContext(request.Context(), app.apiClient))
@@ -302,6 +326,32 @@ func TestSignInForwardsAPISessionCookie(t *testing.T) {
 	if cookies := response.Result().Cookies(); len(cookies) != 1 || cookies[0].Value != "new-session" {
 		t.Fatalf("forwarded cookies: got %+v", cookies)
 	}
+	if location := response.Header().Get("Location"); location != "/invite?token=garden-invite-token" {
+		t.Errorf("Location: got %q, want invitation URL", location)
+	}
+}
+
+func TestSignInRejectsExternalReturnURL(t *testing.T) {
+	apiHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodPost || r.URL.Path != "/v1/session" {
+			http.NotFound(w, r)
+			return
+		}
+		w.WriteHeader(http.StatusCreated)
+		_, _ = w.Write([]byte(`{"user":{"id":7,"name":"Alice","activated":true}}`))
+	})
+	app := newAPIBackedTestApplication(t, apiHandler)
+	form := url.Values{"email": {"alice@example.com"}, "password": {"correct horse battery staple"}, "return_to": {"https://example.com/phishing"}}
+	request := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode()))
+	request.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	request = request.WithContext(client.NewContext(request.Context(), app.apiClient))
+	response := httptest.NewRecorder()
+
+	app.signInPost(response, request)
+
+	if location := response.Header().Get("Location"); location != "/gardens?auto=1" {
+		t.Errorf("Location: got %q, want default landing page", location)
+	}
 }
 
 func TestInactiveSignInRedirectsToActivation(t *testing.T) {

internal/web/helpers.go

@@ -184,7 +184,7 @@ func (app *application) handleAPIError(w http.ResponseWriter, r *http.Request, e
 	if errors.As(err, &apiError) {
 		switch apiError.StatusCode {
 		case http.StatusUnauthorized:
-			http.Redirect(w, r, webPath("login"), http.StatusSeeOther)
+			http.Redirect(w, r, loginPathForRequest(r), http.StatusSeeOther)
 			return
 		case http.StatusForbidden:
 			if user, ok := userFromContext(r.Context()); ok && !user.Activated {

internal/web/middleware.go

@@ -43,7 +43,7 @@ func (app *application) recoverPanic(next http.Handler) http.Handler {
 func (app *application) requireAuthentication(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if !app.isAuthenticated(r) {
-			http.Redirect(w, r, webPath("login"), http.StatusSeeOther)
+			http.Redirect(w, r, loginPathForRequest(r), http.StatusSeeOther)
 			return
 		}
 		w.Header().Set("Cache-Control", "no-store")
@@ -51,6 +51,18 @@ func (app *application) requireAuthentication(next http.Handler) http.Handler {
 	})
 }
 
+func loginPathForRequest(r *http.Request) string {
+	loginPath := webPath("login")
+	if r.Method != http.MethodGet {
+		return loginPath
+	}
+	returnTo := safeReturnPath(r.URL.RequestURI())
+	if returnTo == "" {
+		return loginPath
+	}
+	return pathWithQuery(loginPath, "return_to", returnTo)
+}
+
 func (app *application) requireActivatedUser(next http.Handler) http.Handler {
 	return app.requireAuthentication(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		user, ok := userFromContext(r.Context())

internal/web/paths.go

@@ -141,3 +141,15 @@ func pathWithQuery(path string, pairs ...any) string {
 	}
 	return path + "?" + values.Encode()
 }
+
+func safeReturnPath(value string) string {
+	value = strings.TrimSpace(value)
+	if value == "" {
+		return ""
+	}
+	target, err := url.Parse(value)
+	if err != nil || target.IsAbs() || target.Host != "" || !strings.HasPrefix(target.Path, "/") || strings.HasPrefix(target.Path, "//") || strings.Contains(target.Path, `\`) {
+		return ""
+	}
+	return target.RequestURI()
+}

internal/web/paths_test.go

@@ -33,6 +33,29 @@ func TestPathWithQueryEncodesValues(t *testing.T) {
 	}
 }
 
+func TestSafeReturnPath(t *testing.T) {
+	tests := []struct {
+		name  string
+		value string
+		want  string
+	}{
+		{name: "local path", value: "/invite?token=abc", want: "/invite?token=abc"},
+		{name: "absolute URL", value: "https://example.com/phishing"},
+		{name: "scheme relative URL", value: "//example.com/phishing"},
+		{name: "backslash", value: `/\\example.com/phishing`},
+		{name: "encoded backslash", value: `/%5C%5Cexample.com/phishing`},
+		{name: "encoded leading slashes", value: `/%2F%2Fexample.com/phishing`},
+		{name: "relative path", value: "invite?token=abc"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got := safeReturnPath(test.value); got != test.want {
+				t.Errorf("safeReturnPath(%q): got %q, want %q", test.value, got, test.want)
+			}
+		})
+	}
+}
+
 func TestGardenAwareAdminPaths(t *testing.T) {
 	garden := &client.Garden{ID: 3}
 	if got := gardenAwarePath(webPath("admin.role.new"), garden); got != "/admin/roles/new?garden=3" {

internal/web/templates/pages/signin.tmpl

@@ -7,6 +7,7 @@
     {{with $form.Message}}<p class='form-message error'>{{.}}</p>{{end}}
     <form action='{{webPath "login"}}' method='POST'>
         <input type='hidden' name='csrf_token' value='{{.CSRFToken}}'>
+        {{with $form.ReturnTo}}<input type='hidden' name='return_to' value='{{.}}'>{{end}}
         <label for='email'>E-Mail-Adresse</label>
         <input id='email' name='email' type='email' value='{{$form.Email}}' autocomplete='email' required>
         {{with index $form.Errors "email"}}<p class='field-error'>{{.}}</p>{{end}}