fix(sso): use relative redirect URLs. Fixes #13031

server/auth/sso/sso.go

@@ -36,6 +36,11 @@ const (
 	cookieEncryptionPrivateKeySecretKey = "cookieEncryptionPrivateKey" // the key name for the private key in the secret
 )
 
+// Copied from https://github.com/oauth2-proxy/oauth2-proxy/blob/ab448cf38e7c1f0740b3cc2448284775e39d9661/pkg/app/redirect/validator.go#L14-L16
+// Used to check final redirects are not susceptible to open redirects.
+// Matches //, /\ and both of these with whitespace in between (eg / / or / \).
+var invalidRedirectRegex = regexp.MustCompile(`[/\\](?:[\s\v]*|\.{1,2})[/\\]`)
+
 //go:generate mockery --name=Interface
 
 type Interface interface {
@@ -219,7 +224,10 @@ func newSso(
 }
 
 func (s *sso) HandleRedirect(w http.ResponseWriter, r *http.Request) {
-	redirectUrl := r.URL.Query().Get("redirect")
+	finalRedirectUrl := r.URL.Query().Get("redirect")
+	if !isValidFinalRedirectUrl(finalRedirectUrl) {
+		finalRedirectUrl = s.baseHRef
+	}
 	state, err := pkgrand.RandString(10)
 	if err != nil {
 		log.WithError(err).Error("failed to create state")
@@ -228,7 +236,7 @@ func (s *sso) HandleRedirect(w http.ResponseWriter, r *http.Request) {
 	}
 	http.SetCookie(w, &http.Cookie{
 		Name:     state,
-		Value:    redirectUrl,
+		Value:    finalRedirectUrl,
 		Expires:  time.Now().Add(3 * time.Minute),
 		HttpOnly: true,
 		SameSite: http.SameSiteLaxMode,
@@ -340,18 +348,34 @@ func (s *sso) HandleCallback(w http.ResponseWriter, r *http.Request) {
 		SameSite: http.SameSiteStrictMode,
 		Secure:   s.secure,
 	})
-	redirect := s.baseHRef
 
-	proto := "http"
-	if s.secure {
-		proto = "https"
-	}
-	prefix := fmt.Sprintf("%s://%s%s", proto, r.Host, s.baseHRef)
+	finalRedirectUrl := cookie.Value
+	if !isValidFinalRedirectUrl(cookie.Value) {
+		finalRedirectUrl = s.baseHRef
 
-	if strings.HasPrefix(cookie.Value, prefix) {
-		redirect = cookie.Value
 	}
-	http.Redirect(w, r, redirect, 302)
+	http.Redirect(w, r, finalRedirectUrl, http.StatusFound)
+}
+
+// isValidFinalRedirectUrl checks whether the final redirect URL is safe.
+//
+// We only allow path-absolute-URL strings (e.g. /foo/bar), as defined in the
+// WHATWG URL standard and RFC 3986:
+// https://url.spec.whatwg.org/#path-absolute-url-string
+// https://datatracker.ietf.org/doc/html/rfc3986#section-4.2
+//
+// It's not sufficient to only refer to RFC3986 for this validation logic
+// because modern browsers will convert back slashes (\) to forward slashes (/)
+// and will interprete percent-encoded bytes.
+//
+// We used to use absolute redirect URLs and would validate the scheme and host
+// match the request scheme and host, but this led to problems when Argo is
+// behind a TLS termination proxy, since the redirect URL would have the scheme
+// "https" while the request scheme would be "http"
+// (see https://github.com/argoproj/argo-workflows/issues/13031).
+func isValidFinalRedirectUrl(redirect string) bool {
+	// Copied from https://github.com/oauth2-proxy/oauth2-proxy/blob/ab448cf38e7c1f0740b3cc2448284775e39d9661/pkg/app/redirect/validator.go#L47
+	return strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") && !invalidRedirectRegex.MatchString(redirect)
 }
 
 // authorize verifies a bearer token and pulls user information form the claims.

server/auth/sso/sso_test.go

@@ -154,3 +154,45 @@ func TestGetSessionExpiry(t *testing.T) {
 	}
 	assert.Equal(t, 5*time.Hour, config.GetSessionExpiry())
 }
+
+func TestIsValidFinalRedirectUrl(t *testing.T) {
+	testCases := []struct {
+		name     string
+		url      string
+		expected bool
+	}{
+		// Adapted from https://github.com/oauth2-proxy/oauth2-proxy/blob/ab448cf38e7c1f0740b3cc2448284775e39d9661/pkg/app/redirect/validator_test.go#L60-L116
+		{"No Redirect", "", false},
+		{"Single Slash", "/redirect", true},
+		{"Single Slash with query parameters", "/redirect?foo=bar&baz=2", true},
+		{"Double Slash (protocol-relative URL)", "//redirect", false},
+		{"Absolute HTTP", "http://foo.bar/redirect", false},
+		{"Absolute HTTP with subdomain", "http://baz.foo.bar/", false},
+		{"Absolute HTTPS", "https://foo.bar/redirect", false},
+		{"Absolute HTTPS Port and Domain", "https://evil.corp:3838/redirect", false},
+		{"Escape Double Slash", "/\\evil.com", false},
+		{"Space Single Slash", "/ /evil.com", false},
+		{"Space Double Slash", "/ \\evil.com", false},
+		{"Tab Single Slash", "/\t/evil.com", false},
+		{"Tab Double Slash", "/\t\\evil.com", false},
+		{"Vertical Tab Single Slash", "/\v/evil.com", false},
+		{"Vertiacl Tab Double Slash", "/\v\\evil.com", false},
+		{"New Line Single Slash", "/\n/evil.com", false},
+		{"New Line Double Slash", "/\n\\evil.com", false},
+		{"Carriage Return Single Slash", "/\r/evil.com", false},
+		{"Carriage Return Double Slash", "/\r\\evil.com", false},
+		{"Double Tab", "/\t/\t\\evil.com", false},
+		{"Triple Tab 1", "/\t\t/\t/evil.com", false},
+		{"Triple Tab 2", "/\t\t\\\t/evil.com", false},
+		{"Quad Tab 1", "/\t\t/\t\t\\evil.com", false},
+		{"Quad Tab 2", "/\t\t\\\t\t/evil.com", false},
+		{"Relative Path", "/./\\evil.com", false},
+		{"Relative Subpath", "/./../../\\evil.com", false},
+		{"Missing Protocol Root Domain", "foo.bar/redirect", false},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.expected, isValidFinalRedirectUrl(tc.url))
+		})
+	}
+}

ui/src/login/login.tsx

@@ -15,12 +15,9 @@ function user(token: string) {
     document.cookie = 'authorization=' + token + ';SameSite=Strict;path=' + path;
     document.location.href = path;
 }
-function getRedirect(): string {
-    const urlParams = new URLSearchParams(new URL(document.location.href).search);
-    if (urlParams.has('redirect')) {
-        return 'redirect=' + urlParams.get('redirect');
-    }
-    return 'redirect=' + window.location.origin + '/workflows';
+function getRedirect(): URLSearchParams {
+    const urlParams = new URLSearchParams(document.location.search);
+    return new URLSearchParams({redirect: urlParams.get('redirect') ?? '/workflows'});
 }
 
 export function Login() {
@@ -47,7 +44,7 @@ export function Login() {
                             <button
                                 className='argo-button argo-button--base-o'
                                 onClick={() => {
-                                    document.location.href = uiUrlWithParams('oauth2/redirect', [getRedirect()]);
+                                    document.location.href = uiUrlWithParams('oauth2/redirect', getRedirect());
                                 }}>
                                 <i className='fa fa-sign-in-alt' /> Login
                             </button>

ui/src/shared/base.ts

@@ -7,11 +7,8 @@ export function uiUrl(uiPath: string): string {
     return baseUrl() + uiPath;
 }
 
-export function uiUrlWithParams(uiPath: string, params: string[]): string {
-    if (!params) {
-        return uiUrl(uiPath);
-    }
-    return baseUrl() + uiPath + '?' + params.join('&');
+export function uiUrlWithParams(uiPath: string, params: URLSearchParams): string {
+    return baseUrl() + uiPath + '?' + params.toString();
 }
 
 export function apiUrl(apiPath: string): string {

ui/src/shared/services/requests.ts

@@ -11,7 +11,10 @@ function auth(req: SuperAgentRequest) {
 function handle(err: any) {
     // check URL to prevent redirect loop
     if (err.status === 401 && !document.location.href.includes('login')) {
-        document.location.href = uiUrlWithParams('login', ['redirect=' + document.location.href]);
+        const params = new URLSearchParams({
+            redirect: document.location.pathname + document.location.search
+        });
+        document.location.href = uiUrlWithParams('login', params);
     }
 }