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

@@ -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")

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
 }

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