Work around missing URL parameters

when passing the URL with parameters in the `go=` parameter inside
nginx. This is caused by nginx not being able to escape ampersands which
then are parsed as parameters to the login handler instead of parameters
of the redirect URL.

There is a quite old ticket in nginx to implement proper escaping of URL
elements which would be a way better solution but until someone decides
to take care of that this should at least improve the situation.

refs #39

Signed-off-by: Knut Ahlers <knut@ahlers.me>

main.go

@@ -181,14 +181,19 @@ func handleAuthRequest(res http.ResponseWriter, r *http.Request) {
 }
 
 func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
+	redirURL, err := getRedirectURL(r)
+	if err != nil {
+		http.Error(res, "Invalid redirect URL specified", http.StatusBadRequest)
+	}
+
 	if _, _, err := detectUser(res, r); err == nil {
 		// There is already a valid user
-		http.Redirect(res, r, r.URL.Query().Get("go"), http.StatusFound)
+		http.Redirect(res, r, redirURL, http.StatusFound)
 		return
 	}
 
 	auditFields := map[string]string{
-		"go": r.FormValue("go"),
+		"go": redirURL,
 	}
 
 	if r.Method == "POST" {
@@ -198,7 +203,7 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 		case plugins.ErrNoValidUserFound:
 			auditFields["reason"] = "invalid credentials"
 			mainCfg.AuditLog.Log(auditEventLoginFailure, r, auditFields) // #nosec G104 - This is only logging
-			http.Redirect(res, r, "/login?go="+url.QueryEscape(r.FormValue("go")), http.StatusFound)
+			http.Redirect(res, r, "/login?go="+url.QueryEscape(redirURL), http.StatusFound)
 			return
 		case nil:
 			// Don't handle for now, MFA validation comes first
@@ -207,7 +212,7 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 			auditFields["error"] = err.Error()
 			mainCfg.AuditLog.Log(auditEventLoginFailure, r, auditFields) // #nosec G104 - This is only logging
 			log.WithError(err).Error("Login failed with unexpected error")
-			http.Redirect(res, r, "/login?go="+url.QueryEscape(r.FormValue("go")), http.StatusFound)
+			http.Redirect(res, r, "/login?go="+url.QueryEscape(redirURL), http.StatusFound)
 			return
 		}
 
@@ -218,12 +223,12 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 			auditFields["reason"] = "invalid credentials"
 			mainCfg.AuditLog.Log(auditEventLoginFailure, r, auditFields) // #nosec G104 - This is only logging
 			res.Header().Del("Set-Cookie")                               // Remove login cookie
-			http.Redirect(res, r, "/login?go="+url.QueryEscape(r.FormValue("go")), http.StatusFound)
+			http.Redirect(res, r, "/login?go="+url.QueryEscape(redirURL), http.StatusFound)
 			return
 
 		case nil:
 			mainCfg.AuditLog.Log(auditEventLoginSuccess, r, auditFields) // #nosec G104 - This is only logging
-			http.Redirect(res, r, r.FormValue("go"), http.StatusFound)
+			http.Redirect(res, r, redirURL, http.StatusFound)
 			return
 
 		default:
@@ -232,7 +237,7 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 			mainCfg.AuditLog.Log(auditEventLoginFailure, r, auditFields) // #nosec G104 - This is only logging
 			log.WithError(err).Error("Login failed with unexpected error")
 			res.Header().Del("Set-Cookie") // Remove login cookie
-			http.Redirect(res, r, "/login?go="+url.QueryEscape(r.FormValue("go")), http.StatusFound)
+			http.Redirect(res, r, "/login?go="+url.QueryEscape(redirURL), http.StatusFound)
 			return
 		}
 	}
@@ -240,7 +245,7 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 	tpl := pongo2.Must(pongo2.FromFile(path.Join(cfg.TemplateDir, "index.html")))
 	if err := tpl.ExecuteWriter(pongo2.Context{
 		"active_methods": getFrontendAuthenticators(),
-		"go":             r.URL.Query().Get("go"),
+		"go":             redirURL,
 		"login":          mainCfg.Login,
 	}, res); err != nil {
 		log.WithError(err).Error("Unable to render template")
@@ -249,6 +254,11 @@ func handleLoginRequest(res http.ResponseWriter, r *http.Request) {
 }
 
 func handleLogoutRequest(res http.ResponseWriter, r *http.Request) {
+	redirURL, err := getRedirectURL(r)
+	if err != nil {
+		http.Error(res, "Invalid redirect URL specified", http.StatusBadRequest)
+	}
+
 	mainCfg.AuditLog.Log(auditEventLogout, r, nil) // #nosec G104 - This is only logging
 	if err := logoutUser(res, r); err != nil {
 		log.WithError(err).Error("Failed to logout user")
@@ -256,5 +266,5 @@ func handleLogoutRequest(res http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.Redirect(res, r, r.URL.Query().Get("go"), http.StatusFound)
+	http.Redirect(res, r, redirURL, http.StatusFound)
 }

redirect.go

@@ -0,0 +1,37 @@
+package main
+
+import (
+	"net/http"
+	"net/url"
+
+	"github.com/pkg/errors"
+)
+
+func getRedirectURL(r *http.Request) (string, error) {
+	var (
+		redirURL = r.URL.Query().Get("go")
+		params   = r.URL.Query()
+	)
+
+	if redirURL == "" {
+		redirURL = r.FormValue("go")
+		params = url.Values{} // No need to read other form fields
+	}
+
+	params.Del("go")
+
+	pURL, err := url.Parse(redirURL)
+	if err != nil {
+		return "", errors.Wrap(err, "Unable to parse redirect URL")
+	}
+
+	for k, vs := range pURL.Query() {
+		for _, v := range vs {
+			params.Add(k, v)
+		}
+	}
+
+	pURL.RawQuery = params.Encode()
+
+	return pURL.String(), nil
+}

redirect_test.go

@@ -0,0 +1,62 @@
+package main
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+)
+
+func TestGetRedirectGet(t *testing.T) {
+	// Constructed URL to match a nginx redirect:
+	// `return 302 https://login.luzifer.io/login?go=$scheme://$http_host$request_uri;`
+	testURL := "https://example.com/login?go=https://example.com/inner?foo=bar&bar=foo"
+	expectURL := "https://example.com/inner?bar=foo&foo=bar"
+
+	req, _ := http.NewRequest(http.MethodGet, testURL, nil)
+
+	rURL, err := getRedirectURL(req)
+	if err != nil {
+		t.Errorf("getRedirectURL caused an error in GET: %s", err)
+	}
+
+	if expectURL != rURL {
+		t.Errorf("Result did not match expected URL: %q != %q", rURL, expectURL)
+	}
+}
+
+func TestGetRedirectGetEmpty(t *testing.T) {
+	testURL := "https://example.com/login"
+	expectURL := ""
+
+	req, _ := http.NewRequest(http.MethodGet, testURL, nil)
+
+	rURL, err := getRedirectURL(req)
+	if err != nil {
+		t.Errorf("getRedirectURL caused an error in GET: %s", err)
+	}
+
+	if expectURL != rURL {
+		t.Errorf("Result did not match expected URL: %q != %q", rURL, expectURL)
+	}
+}
+
+func TestGetRedirectPost(t *testing.T) {
+	testURL := "https://example.com/login"
+	expectURL := "https://example.com/inner?foo=bar"
+
+	body := url.Values{
+		"go":    []string{expectURL},
+		"other": []string{"param"},
+	}
+	req, _ := http.NewRequest(http.MethodPost, testURL, nil)
+	req.Form = body // Force-set the form values to emulate parsed form
+
+	rURL, err := getRedirectURL(req)
+	if err != nil {
+		t.Errorf("getRedirectURL caused an error in POST: %s", err)
+	}
+
+	if expectURL != rURL {
+		t.Errorf("Result did not match expected URL: %q != %q", rURL, expectURL)
+	}
+}