From 548c9043cceb32681305d5ed721e13183296d854 Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Wed, 20 Apr 2022 16:16:28 -0700 Subject: [PATCH] Manually extract SSO redirect URL to preserve its own query params (#12100) (#12125) Prevents truncating of the redirect URL if this URL had its own query params. --- lib/web/apiserver.go | 20 +++++++++-- lib/web/apiserver_test.go | 70 ++++++++++++++++++++++++++++++++++++++- lib/web/ui/webconfig.go | 13 +++++--- 3 files changed, 94 insertions(+), 9 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 5180ece62b642..7a02e38e31257 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -2710,13 +2710,27 @@ type ssoRequestParams struct { } func parseSSORequestParams(r *http.Request) (*ssoRequestParams, error) { - query := r.URL.Query() - - clientRedirectURL := query.Get("redirect_url") + // Manually grab the value from query param "redirect_url". + // + // The "redirect_url" param can contain its own query params such as in + // "https://localhost/login?connector_id=github&redirect_url=https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", + // which would be incorrectly parsed with the standard method. + // For example a call to r.URL.Query().Get("redirect_url") in the example above would return + // "https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel", + // as it would take the "&sort=hostname:asc" to be a separate query param. + // + // This logic assumes that anything coming after "redirect_url" is part of + // the redirect URL. + splittedRawQuery := strings.Split(r.URL.RawQuery, "&redirect_url=") + var clientRedirectURL string + if len(splittedRawQuery) > 1 { + clientRedirectURL, _ = url.QueryUnescape(splittedRawQuery[1]) + } if clientRedirectURL == "" { return nil, trace.BadParameter("missing redirect_url query parameter") } + query := r.URL.Query() connectorID := query.Get("connector_id") if connectorID == "" { return nil, trace.BadParameter("missing connector_id query parameter") diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 2b563b1cb55ef..27635acecfc9a 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -548,7 +548,7 @@ func (s *WebSuite) TestSAMLSuccess(c *C) { csrfToken := "2ebcb768d0090ea4368e42880c970b61865c326172a4a2343b645cf5d7f20992" - baseURL, err := url.Parse(clt.Endpoint("webapi", "saml", "sso") + `?redirect_url=http://localhost/after&connector_id=` + connector.GetName()) + baseURL, err := url.Parse(clt.Endpoint("webapi", "saml", "sso") + `?connector_id=` + connector.GetName() + `&redirect_url=http://localhost/after`) c.Assert(err, IsNil) req, err := http.NewRequest("GET", baseURL.String(), nil) c.Assert(err, IsNil) @@ -3336,6 +3336,74 @@ func TestChangeUserAuthentication_recoveryCodesReturnedForCloud(t *testing.T) { require.NotEmpty(t, re.Recovery.Created) } +func TestParseSSORequestParams(t *testing.T) { + t.Parallel() + + token := "someMeaninglessTokenString" + + tests := []struct { + name, url string + wantErr bool + expected *ssoRequestParams + }{ + { + name: "preserve redirect's query params (escaped)", + url: "https://localhost/login?connector_id=oidc&redirect_url=https:%2F%2Flocalhost:8080%2Fweb%2Fcluster%2Fim-a-cluster-name%2Fnodes%3Fsearch=tunnel&sort=hostname:asc", + expected: &ssoRequestParams{ + clientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", + connectorID: "oidc", + csrfToken: token, + }, + }, + { + name: "preserve redirect's query params (unescaped)", + url: "https://localhost/login?connector_id=github&redirect_url=https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", + expected: &ssoRequestParams{ + clientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", + connectorID: "github", + csrfToken: token, + }, + }, + { + name: "preserve various encoded chars", + url: "https://localhost/login?connector_id=saml&redirect_url=https:%2F%2Flocalhost:8080%2Fweb%2Fcluster%2Fim-a-cluster-name%2Fapps%3Fquery=search(%2522watermelon%2522%252C%2520%2522this%2522)%2520%2526%2526%2520labels%255B%2522unique-id%2522%255D%2520%253D%253D%2520%2522hi%2522&sort=name:asc", + expected: &ssoRequestParams{ + clientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/apps?query=search(%22watermelon%22%2C%20%22this%22)%20%26%26%20labels%5B%22unique-id%22%5D%20%3D%3D%20%22hi%22&sort=name:asc", + connectorID: "saml", + csrfToken: token, + }, + }, + { + name: "invalid redirect_url query param", + url: "https://localhost/login?redirect=https://localhost/nodes&connector_id=oidc", + wantErr: true, + }, + { + name: "invalid connector_id query param", + url: "https://localhost/login?redirect_url=https://localhost/nodes&connector=oidc", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequest("", tc.url, nil) + require.NoError(t, err) + addCSRFCookieToReq(req, token) + + params, err := parseSSORequestParams(req) + + switch { + case tc.wantErr: + require.Error(t, err) + default: + require.NoError(t, err) + require.Equal(t, tc.expected, params) + } + }) + } +} + type authProviderMock struct { server types.ServerV2 } diff --git a/lib/web/ui/webconfig.go b/lib/web/ui/webconfig.go index e25e177aeb57b..3932ea9396f81 100644 --- a/lib/web/ui/webconfig.go +++ b/lib/web/ui/webconfig.go @@ -21,18 +21,21 @@ import "github.com/gravitational/teleport/api/constants" const ( // WebConfigAuthProviderOIDCType is OIDC provider type WebConfigAuthProviderOIDCType = "oidc" - // WebConfigAuthProviderOIDCURL is OIDC webapi endpoint - WebConfigAuthProviderOIDCURL = "/v1/webapi/oidc/login/web?redirect_url=:redirect&connector_id=:providerName" + // WebConfigAuthProviderOIDCURL is OIDC webapi endpoint. + // redirect_url MUST be the last query param, see the comment in parseSSORequestParams for an explanation. + WebConfigAuthProviderOIDCURL = "/v1/webapi/oidc/login/web?connector_id=:providerName&redirect_url=:redirect" // WebConfigAuthProviderSAMLType is SAML provider type WebConfigAuthProviderSAMLType = "saml" - // WebConfigAuthProviderSAMLURL is SAML webapi endpoint - WebConfigAuthProviderSAMLURL = "/v1/webapi/saml/sso?redirect_url=:redirect&connector_id=:providerName" + // WebConfigAuthProviderSAMLURL is SAML webapi endpoint. + // redirect_url MUST be the last query param, see the comment in parseSSORequestParams for an explanation. + WebConfigAuthProviderSAMLURL = "/v1/webapi/saml/sso?connector_id=:providerName&redirect_url=:redirect" // WebConfigAuthProviderGitHubType is GitHub provider type WebConfigAuthProviderGitHubType = "github" // WebConfigAuthProviderGitHubURL is GitHub webapi endpoint - WebConfigAuthProviderGitHubURL = "/v1/webapi/github/login/web?redirect_url=:redirect&connector_id=:providerName" + // redirect_url MUST be the last query param, see the comment in parseSSORequestParams for an explanation. + WebConfigAuthProviderGitHubURL = "/v1/webapi/github/login/web?connector_id=:providerName&redirect_url=:redirect" ) // WebConfig is web application configuration