providers/proxy: fix URL path getting lost when partial URL is given to rd= (#11354)

* providers/proxy: fix URL path getting lost when partial URL is given to rd= Signed-off-by: Jens Langhammer <jens@goauthentik.io> * better fallback + tests Signed-off-by: Jens Langhammer <jens@goauthentik.io> --------- Signed-off-by: Jens Langhammer <jens@goauthentik.io>
2024-09-12 18:02:08 +02:00
parent 18a43455f1
commit 171d0f55cb
4 changed files with 77 additions and 47 deletions
--- a/internal/outpost/proxyv2/application/application.go
+++ b/internal/outpost/proxyv2/application/application.go
@ -193,7 +193,17 @@ func NewApplication(p api.ProxyOutpostConfig, c *http.Client, server Server) (*A
 	})

 	mux.HandleFunc("/outpost.goauthentik.io/start", func(w http.ResponseWriter, r *http.Request) {
-		a.handleAuthStart(w, r, "")
+		fwd := ""
+		// This should only really be hit for nginx forward_auth
+		// as for that the auth start redirect URL is generated by the
+		// reverse proxy, and as such we won't have a request we just
+		// denied to reference for final URL
+		rd, ok := a.checkRedirectParam(r)
+		if ok {
+			a.log.WithField("rd", rd).Trace("Setting redirect")
+			fwd = rd
+		}
+		a.handleAuthStart(w, r, fwd)
 	})
 	mux.HandleFunc("/outpost.goauthentik.io/callback", a.handleAuthCallback)
 	mux.HandleFunc("/outpost.goauthentik.io/sign_out", a.handleSignOut)
--- a/internal/outpost/proxyv2/application/oauth.go
+++ b/internal/outpost/proxyv2/application/oauth.go
@ -15,36 +15,6 @@ const (
 	LogoutSignature   = "X-authentik-logout"
 )

-func (a *Application) checkRedirectParam(r *http.Request) (string, bool) {
-	rd := r.URL.Query().Get(redirectParam)
-	if rd == "" {
-		return "", false
-	}
-	u, err := url.Parse(rd)
-	if err != nil {
-		a.log.WithError(err).Warning("Failed to parse redirect URL")
-		return "", false
-	}
-	// Check to make sure we only redirect to allowed places
-	if a.Mode() == api.PROXYMODE_PROXY || a.Mode() == api.PROXYMODE_FORWARD_SINGLE {
-		ext, err := url.Parse(a.proxyConfig.ExternalHost)
-		if err != nil {
-			return "", false
-		}
-		ext.Scheme = ""
-		if !strings.Contains(u.String(), ext.String()) {
-			a.log.WithField("url", u.String()).WithField("ext", ext.String()).Warning("redirect URI did not contain external host")
-			return "", false
-		}
-	} else {
-		if !strings.HasSuffix(u.Host, *a.proxyConfig.CookieDomain) {
-			a.log.WithField("host", u.Host).WithField("dom", *a.proxyConfig.CookieDomain).Warning("redirect URI Host was not included in cookie domain")
-			return "", false
-		}
-	}
-	return u.String(), true
-}
-
 func (a *Application) handleAuthStart(rw http.ResponseWriter, r *http.Request, fwd string) {
 	state, err := a.createState(r, fwd)
 	if err != nil {
--- a/internal/outpost/proxyv2/application/oauth_state.go
+++ b/internal/outpost/proxyv2/application/oauth_state.go
@ -5,10 +5,13 @@ import (
 	"encoding/base64"
 	"fmt"
 	"net/http"
+	"net/url"
+	"strings"

 	"github.com/golang-jwt/jwt/v5"
 	"github.com/gorilla/securecookie"
 	"github.com/mitchellh/mapstructure"
+	"goauthentik.io/api/v3"
 )

 type OAuthState struct {
@ -27,6 +30,44 @@ func (oas *OAuthState) GetAudience() (jwt.ClaimStrings, error)       { return ni

 var base32RawStdEncoding = base32.StdEncoding.WithPadding(base32.NoPadding)

+// Validate that the given redirect parameter (?rd=...) is valid and can be used
+// For proxy/forward_single this checks that if the `rd` param has a Hostname (and is a full URL)
+// the hostname matches what's configured, or no hostname must be given
+// For forward_domain this checks if the domain of the URL in `rd` ends with the configured domain
+func (a *Application) checkRedirectParam(r *http.Request) (string, bool) {
+	rd := r.URL.Query().Get(redirectParam)
+	if rd == "" {
+		return "", false
+	}
+	u, err := url.Parse(rd)
+	if err != nil {
+		a.log.WithError(err).Warning("Failed to parse redirect URL")
+		return "", false
+	}
+	// Check to make sure we only redirect to allowed places
+	if a.Mode() == api.PROXYMODE_PROXY || a.Mode() == api.PROXYMODE_FORWARD_SINGLE {
+		ext, err := url.Parse(a.proxyConfig.ExternalHost)
+		if err != nil {
+			return "", false
+		}
+		// Either hostname needs to match the configured domain, or host name must be empty for just a path
+		if u.Host == "" {
+			u.Host = ext.Host
+			u.Scheme = ext.Scheme
+		}
+		if u.Host != ext.Host {
+			a.log.WithField("url", u.String()).WithField("ext", ext.String()).Warning("redirect URI did not contain external host")
+			return "", false
+		}
+	} else {
+		if !strings.HasSuffix(u.Host, *a.proxyConfig.CookieDomain) {
+			a.log.WithField("host", u.Host).WithField("dom", *a.proxyConfig.CookieDomain).Warning("redirect URI Host was not included in cookie domain")
+			return "", false
+		}
+	}
+	return u.String(), true
+}
+
 func (a *Application) createState(r *http.Request, fwd string) (string, error) {
 	s, _ := a.sessions.Get(r, a.SessionName())
 	if s.ID == "" {
@ -39,17 +80,6 @@ func (a *Application) createState(r *http.Request, fwd string) (string, error) {
 		SessionID: s.ID,
 		Redirect:  fwd,
 	}
-	if fwd == "" {
-		// This should only really be hit for nginx forward_auth
-		// as for that the auth start redirect URL is generated by the
-		// reverse proxy, and as such we won't have a request we just
-		// denied to reference for final URL
-		rd, ok := a.checkRedirectParam(r)
-		if ok {
-			a.log.WithField("rd", rd).Trace("Setting redirect")
-			st.Redirect = rd
-		}
-	}
 	token := jwt.NewWithClaims(jwt.SigningMethodHS256, st)
 	tokenString, err := token.SignedString([]byte(a.proxyConfig.GetCookieSecret()))
 	if err != nil {
--- a/internal/outpost/proxyv2/application/oauth_test.go
+++ b/internal/outpost/proxyv2/application/oauth_test.go
@ -8,25 +8,45 @@ import (
 	"goauthentik.io/api/v3"
 )

-func TestCheckRedirectParam(t *testing.T) {
+func TestCheckRedirectParam_None(t *testing.T) {
 	a := newTestApplication()
+	// Test no rd param
 	req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start", nil)

 	rd, ok := a.checkRedirectParam(req)

 	assert.Equal(t, false, ok)
 	assert.Equal(t, "", rd)
+}

-	req, _ = http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://google.com", nil)
+func TestCheckRedirectParam_Invalid(t *testing.T) {
+	a := newTestApplication()
+	// Test invalid rd param
+	req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://google.com", nil)

-	rd, ok = a.checkRedirectParam(req)
+	rd, ok := a.checkRedirectParam(req)

 	assert.Equal(t, false, ok)
 	assert.Equal(t, "", rd)
+}

-	req, _ = http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://ext.t.goauthentik.io/test?foo", nil)
+func TestCheckRedirectParam_ValidFull(t *testing.T) {
+	a := newTestApplication()
+	// Test valid full rd param
+	req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=https://ext.t.goauthentik.io/test?foo", nil)

-	rd, ok = a.checkRedirectParam(req)
+	rd, ok := a.checkRedirectParam(req)
+
+	assert.Equal(t, true, ok)
+	assert.Equal(t, "https://ext.t.goauthentik.io/test?foo", rd)
+}
+
+func TestCheckRedirectParam_ValidPartial(t *testing.T) {
+	a := newTestApplication()
+	// Test valid partial rd param
+	req, _ := http.NewRequest("GET", "/outpost.goauthentik.io/auth/start?rd=/test?foo", nil)
+
+	rd, ok := a.checkRedirectParam(req)

 	assert.Equal(t, true, ok)
 	assert.Equal(t, "https://ext.t.goauthentik.io/test?foo", rd)