feat(redirect): added support for query param redirect_uri (predates …

constants.go

@@ -30,6 +30,7 @@ const (
 	accessCookie       = "kc-access"
 	refreshCookie      = "kc-state"
 	requestURICookie   = "request_uri"
+	requestURIParam    = "request_uri_fred"
 	requestStateCookie = "OAuth_Token_Request_State"
 
 	unsecureScheme = "http"

e2e_test.go

@@ -265,10 +265,10 @@ func runTestConnect(t *testing.T, config *Config, listener, route string) (strin
 		URL:    u,
 		Header: make(http.Header),
 	}
-	// add request_uri to specify last stop redirection (inner workings since PR #440)
+	// add request_uri cookie to specify last stop redirection (inner workings since PR #440)
 	encoded := base64.StdEncoding.EncodeToString([]byte("http://" + listener + route))
 	ck := &http.Cookie{
-		Name:  "request_uri",
+		Name:  requestURICookie,
 		Value: encoded,
 		Path:  "/",
 		// real life cookie gets Secure, SameSite
@@ -338,3 +338,68 @@ func copyCookies(req *http.Request, cookies []*http.Cookie) {
 		}
 	}
 }
+
+func runTestConnectNoCookie(t *testing.T, config *Config, listener, route string) (string, []*http.Cookie, error) {
+	client := http.Client{
+		Transport: controlledRedirect{
+			CollectedCookies: make(map[string]*http.Cookie, 10),
+		},
+		CheckRedirect: onRedirect,
+	}
+	u, _ := url.Parse("http://" + config.Listen + "/oauth/authorize")
+
+	// query params
+	v := u.Query()
+	v.Set("state", "my_client_nonce") // NOTE: this state provided by the client is not currently carried on to the end (lost)
+	// add request_uri query param to specify last stop redirection (inner workings since PR #440)
+	v.Set(requestURIParam, "http://"+listener+route)
+	u.RawQuery = v.Encode()
+
+	req := &http.Request{
+		Method: "GET",
+		URL:    u,
+		Header: make(http.Header),
+	}
+
+	// attempts to login
+	resp, err := client.Do(req)
+	if !assert.NoError(t, err) {
+		return "", nil, err
+	}
+	defer func() {
+		_ = resp.Body.Close()
+	}()
+
+	// check that we get the final redirection to app correctly
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	buf, erb := ioutil.ReadAll(resp.Body)
+	assert.NoError(t, erb)
+	assert.JSONEq(t, `{"message": "ok"}`, string(buf))
+
+	// returns all collected cookies during the handshake
+	collector, ok := client.Transport.(controlledRedirect)
+	require.True(t, ok)
+
+	collected := make([]*http.Cookie, 0, 10)
+	for _, ck := range collector.CollectedCookies {
+		collected = append(collected, ck)
+	}
+
+	// assert kc-access cookie
+	var (
+		found       bool
+		accessToken string
+	)
+	for _, ck := range collected {
+		if ck.Name == config.CookieAccessName {
+			accessToken = ck.Value
+			found = true
+			break
+		}
+	}
+	assert.True(t, found)
+	if t.Failed() {
+		return "", nil, errors.New("failed to connect")
+	}
+	return accessToken, collected, nil
+}

handlers.go

@@ -233,28 +233,42 @@ func (r *oauthProxy) oauthCallbackHandler(w http.ResponseWriter, req *http.Reque
 		r.dropAccessTokenCookie(req.WithContext(ctx), w, accessToken, time.Until(identity.ExpiresAt))
 	}
 
-	// step: decode the request variable
+	// step: decode the request variable when a state is passed
 	redirectURI := "/"
+	logger.Debug("app request URL, with query [1]", zap.Stringer("URL", req.URL))
 	if req.URL.Query().Get("state") != "" {
-		// if the authorization has set a state, we now check if the calling client
-		// requested a specific landing URL to end the authentication handshake
-		if encodedRequestURI, _ := req.Cookie(requestURICookie); encodedRequestURI != nil {
-			// some clients URL-escape padding characters
-			unescapedValue, err := url.PathUnescape(encodedRequestURI.Value)
+		// 1. redirect URI may be passed by query param
+		logger.Debug("app request URL, with query", zap.String("requestURI", req.URL.RequestURI()))
+		if encodedRequestURI := req.URL.Query().Get(requestURIParam); encodedRequestURI != "" {
+			decoded, err := url.QueryUnescape(encodedRequestURI)
 			if err != nil {
-				logger.Warn("app did send a corrupted redirectURI in cookie: invalid url espcaping", zap.Error(err))
+				logger.Warn("app did send a corrupted redirect_uri param: invalid url escaping", zap.Error(err))
 			}
-			// Since the value is passed with a cookie, we do not expect the client to use base64url (but the
-			// base64-encoded value may itself be url-encoded).
-			// This is safe for browsers using atob() but needs to be treated with care for nodeJS clients,
-			// which natively use base64url encoding, and url-escape padding '=' characters.
-			decoded, err := base64.StdEncoding.DecodeString(unescapedValue)
-			if err != nil {
-				logger.Warn("app did send a corrupted redirectURI in cookie: invalid base64url encoding",
-					zap.Error(err),
-					zap.String("encoded_value", unescapedValue))
+			redirectURI = decoded
+			logger.Debug("app did send a request to be redirected", zap.String("query_param", redirectURI))
+		} else {
+			// 2. redirect URI may be passed by cookie (fallback)
+			// if the authorization has set a state, we now check if the calling client
+			// requested a specific landing URL to end the authentication handshake
+			if encodedRequestURI, _ := req.Cookie(requestURICookie); encodedRequestURI != nil {
+				// some clients URL-escape padding characters
+				unescapedValue, err := url.PathUnescape(encodedRequestURI.Value)
+				if err != nil {
+					logger.Warn("app did send a corrupted redirectURI in cookie: invalid url escaping", zap.Error(err))
+				}
+				// Since the value is passed with a cookie, we do not expect the client to use base64url (but the
+				// base64-encoded value may itself be url-encoded).
+				// This is safe for browsers using atob() but needs to be treated with care for nodeJS clients,
+				// which natively use base64url encoding, and url-escape padding '=' characters.
+				decoded, err := base64.StdEncoding.DecodeString(unescapedValue)
+				if err != nil {
+					logger.Warn("app did send a corrupted redirectURI in cookie: invalid base64url encoding",
+						zap.Error(err),
+						zap.String("encoded_value", unescapedValue))
+				}
+				redirectURI = string(decoded)
+				logger.Debug("app did send a request to be redirected", zap.String("cookie_param", redirectURI))
 			}
-			redirectURI = string(decoded)
 		}
 	}
 

upstreams_test.go

@@ -99,32 +99,43 @@ func TestUpstreams(t *testing.T) {
 	config := testBuildUpstreamsConfig()
 	require.NoError(t, config.isValid())
 
-	// launch fake oauth OIDC server
-	err := runTestAuth(t, e2eUpstreamsOauthListener, "hod-test")
-	require.NoError(t, err)
+	t.Run("should launch fake oauth OIDC server", func(t *testing.T) {
+		err := runTestAuth(t, e2eUpstreamsOauthListener, "hod-test")
+		require.NoError(t, err)
+	})
 
-	// launch fake upstream resource serverS
-	err = runTestUpstream(t, e2eUpstreamsUpstreamListener1, e2eUpstreamsUpstreamURL1, "mark1")
-	require.NoError(t, err)
+	t.Run("should launch fake upstream resource server #1", func(t *testing.T) {
+		err := runTestUpstream(t, e2eUpstreamsUpstreamListener1, e2eUpstreamsUpstreamURL1, "mark1")
+		require.NoError(t, err)
+	})
 
-	err = runTestUpstream(t, e2eUpstreamsUpstreamListener2, e2eUpstreamsUpstreamURL2+"/another-fake", "mark2")
-	require.NoError(t, err)
+	t.Run("should launch fake upstream resource server #2", func(t *testing.T) {
+		err := runTestUpstream(t, e2eUpstreamsUpstreamListener2, e2eUpstreamsUpstreamURL2+"/another-fake", "mark2")
+		require.NoError(t, err)
+	})
 
-	err = runTestUpstream(t, e2eUpstreamsUpstreamListener3, e2eUpstreamsUpstreamURL3, "mark3")
-	require.NoError(t, err)
+	t.Run("should launch fake upstream resource server #3", func(t *testing.T) {
+		err := runTestUpstream(t, e2eUpstreamsUpstreamListener3, e2eUpstreamsUpstreamURL3, "mark3")
+		require.NoError(t, err)
+	})
 
-	// launch fake app server where to land after authentication
-	err = runTestApp(t, e2eUpstreamsAppListener, e2eUpstreamsAppURL)
-	require.NoError(t, err)
+	t.Run("should launch fake app server where to land after authentication", func(t *testing.T) {
+		err := runTestApp(t, e2eUpstreamsAppListener, e2eUpstreamsAppURL)
+		require.NoError(t, err)
+	})
 
-	// launch keycloak-gatekeeper proxy
-	err = runTestGatekeeper(t, config)
-	require.NoError(t, err)
+	t.Run("should launch keycloak-gatekeeper proxy", func(t *testing.T) {
+		err := runTestGatekeeper(t, config)
+		require.NoError(t, err)
+	})
 
-	// establish an authenticated session
-	accessToken, cookies, err := runTestConnect(t, config, e2eUpstreamsAppListener, e2eUpstreamsAppURL)
-	require.NoErrorf(t, err, "could not login: %v", err)
-	require.NotEmpty(t, accessToken)
+	var cookies []*http.Cookie
+	t.Run("establish an authenticated session, with redirection from cookie", func(t *testing.T) {
+		accessToken, respCookies, err := runTestConnect(t, config, e2eUpstreamsAppListener, e2eUpstreamsAppURL)
+		require.NoErrorf(t, err, "could not login: %v", err)
+		require.NotEmpty(t, accessToken)
+		cookies = respCookies
+	})
 
 	// scenario 1: routing to different upstreams
 	client := http.Client{}
@@ -133,75 +144,88 @@ func TestUpstreams(t *testing.T) {
 	h.Add("Accept", "application/json")
 
 	// test upstream 1
-	u, _ := url.Parse("http://" + e2eUpstreamsProxyListener + "/fake")
-	req := &http.Request{
-		Method: "GET",
-		URL:    u,
-		Header: h,
-	}
-	copyCookies(req, cookies)
-
-	resp, err := client.Do(req)
-	require.NoError(t, err)
-	defer func() {
-		_ = resp.Body.Close()
-	}()
-
-	assert.Equal(t, http.StatusOK, resp.StatusCode)
-	buf, err := ioutil.ReadAll(resp.Body)
-	require.NoError(t, err)
-	assert.Contains(t, string(buf), "mark1")
-	t.Logf(string(buf))
+	t.Run("should hit upstream #1", func(t *testing.T) {
+		u, _ := url.Parse("http://" + e2eUpstreamsProxyListener + "/fake")
+		req := &http.Request{
+			Method: "GET",
+			URL:    u,
+			Header: h,
+		}
+		copyCookies(req, cookies)
+
+		resp, err := client.Do(req)
+		require.NoError(t, err)
+		defer func() {
+			_ = resp.Body.Close()
+		}()
+
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		buf, err := ioutil.ReadAll(resp.Body)
+		require.NoError(t, err)
+		assert.Contains(t, string(buf), "mark1")
+		t.Logf(string(buf))
+	})
 
 	// test upstream 2
-	u, _ = url.Parse("http://" + e2eUpstreamsProxyListener + "/another-fake")
-	req = &http.Request{
-		Method: "GET",
-		URL:    u,
-		Header: h,
-	}
-	copyCookies(req, cookies)
-
-	resp, err = client.Do(req)
-	require.NoError(t, err)
-
-	assert.Equal(t, http.StatusOK, resp.StatusCode)
-	buf, err = ioutil.ReadAll(resp.Body)
-	require.NoError(t, err)
-	assert.Contains(t, string(buf), "mark2")
-	t.Logf(string(buf))
+	t.Run("should hit upstream #2", func(t *testing.T) {
+		u, _ := url.Parse("http://" + e2eUpstreamsProxyListener + "/another-fake")
+		req := &http.Request{
+			Method: "GET",
+			URL:    u,
+			Header: h,
+		}
+		copyCookies(req, cookies)
+
+		resp, err := client.Do(req)
+		require.NoError(t, err)
+
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		buf, err := ioutil.ReadAll(resp.Body)
+		require.NoError(t, err)
+		assert.Contains(t, string(buf), "mark2")
+		t.Logf(string(buf))
+	})
 
 	// test upstream 3
-	u, _ = url.Parse("http://" + e2eUpstreamsProxyListener + e2eUpstreamsUpstreamURL3)
-	req = &http.Request{
-		Method: "GET",
-		URL:    u,
-		Header: h,
-	}
-	copyCookies(req, cookies)
-
-	resp, err = client.Do(req)
-	require.NoError(t, err)
-
-	assert.Equal(t, http.StatusOK, resp.StatusCode)
-	buf, err = ioutil.ReadAll(resp.Body)
-	require.NoError(t, err)
-	assert.Contains(t, string(buf), "mark3")
-	t.Logf(string(buf))
-
-	// this should route to {listener3}/api2 and returns 404
-	u, _ = url.Parse("http://" + e2eUpstreamsProxyListener + e2eUpstreamsUpstreamURL2)
-	req = &http.Request{
-		Method: "GET",
-		URL:    u,
-		Header: h,
-	}
-	copyCookies(req, cookies)
+	t.Run("should hit upstream #3", func(t *testing.T) {
+		u, _ := url.Parse("http://" + e2eUpstreamsProxyListener + e2eUpstreamsUpstreamURL3)
+		req := &http.Request{
+			Method: "GET",
+			URL:    u,
+			Header: h,
+		}
+		copyCookies(req, cookies)
+
+		resp, err := client.Do(req)
+		require.NoError(t, err)
+
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+		buf, err := ioutil.ReadAll(resp.Body)
+		require.NoError(t, err)
+		assert.Contains(t, string(buf), "mark3")
+		t.Logf(string(buf))
+	})
+
+	t.Run("should route to {listener3}/api2 and returns 404", func(t *testing.T) {
+		u, _ := url.Parse("http://" + e2eUpstreamsProxyListener + e2eUpstreamsUpstreamURL2)
+		req := &http.Request{
+			Method: "GET",
+			URL:    u,
+			Header: h,
+		}
+		copyCookies(req, cookies)
 
-	resp, err = client.Do(req)
-	require.NoError(t, err)
+		resp, err := client.Do(req)
+		require.NoError(t, err)
 
-	assert.Equal(t, http.StatusNotFound, resp.StatusCode)
+		assert.Equal(t, http.StatusNotFound, resp.StatusCode)
+	})
 
 	// scenario 2: more basepath & path stripping
+
+	t.Run("connect with query param instead of cookie", func(t *testing.T) {
+		accessToken, _, err := runTestConnectNoCookie(t, config, e2eUpstreamsAppListener, e2eUpstreamsAppURL)
+		require.NoErrorf(t, err, "could not login: %v", err)
+		require.NotEmpty(t, accessToken)
+	})
 }