From 45f15de654ddbbef94a9a3579c9d1c600b2ab3e0 Mon Sep 17 00:00:00 2001 From: Knut Ahlers Date: Sun, 21 Apr 2019 00:15:36 +0200 Subject: [PATCH] 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 --- main.go | 28 +++++++++++++++------- redirect.go | 37 +++++++++++++++++++++++++++++ redirect_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 redirect.go create mode 100644 redirect_test.go diff --git a/main.go b/main.go index 6354074..d84cd5b 100644 --- a/main.go +++ b/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) } diff --git a/redirect.go b/redirect.go new file mode 100644 index 0000000..9d51225 --- /dev/null +++ b/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 +} diff --git a/redirect_test.go b/redirect_test.go new file mode 100644 index 0000000..bd433b3 --- /dev/null +++ b/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) + } +}