diff --git a/server/auth/sso/sso.go b/server/auth/sso/sso.go index 386d11d450f8..f5e69bad553a 100644 --- a/server/auth/sso/sso.go +++ b/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. diff --git a/server/auth/sso/sso_test.go b/server/auth/sso/sso_test.go index 217266061f4c..9189cb6c8c97 100644 --- a/server/auth/sso/sso_test.go +++ b/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)) + }) + } +} diff --git a/ui/src/login/login.tsx b/ui/src/login/login.tsx index ec0b00d1379a..ffaa6275a07c 100644 --- a/ui/src/login/login.tsx +++ b/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() { diff --git a/ui/src/shared/base.ts b/ui/src/shared/base.ts index 75b07aff065b..817151bd186a 100644 --- a/ui/src/shared/base.ts +++ b/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 { diff --git a/ui/src/shared/services/requests.ts b/ui/src/shared/services/requests.ts index afc1ccb7744d..c70a92f757af 100644 --- a/ui/src/shared/services/requests.ts +++ b/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); } }