fix: prevent rootpath duplication in OIDC redirect URLs, fixes argoproj#21857 argoproj#20790 argoproj#12195 (argoproj#22254)

Signed-off-by: Diasker <[email protected]>

Author: Diasker

server/rootpath_test.go

@@ -0,0 +1,123 @@
+package server
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestWithRootPathEmptyRootPath tests that withRootPath returns the original handler when RootPath is empty
+func TestWithRootPathEmptyRootPath(t *testing.T) {
+	// Create a simple handler
+	originalHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+
+	// Create a server with empty RootPath
+	server := &ArgoCDServer{
+		ArgoCDServerOpts: ArgoCDServerOpts{
+			RootPath: "",
+		},
+	}
+
+	// Call withRootPath
+	handler := withRootPath(originalHandler, server)
+
+	// Verify that the returned handler is the original handler
+	// Since we can't directly compare function references, we'll use a type assertion
+	_, isServeMux := handler.(*http.ServeMux)
+	assert.False(t, isServeMux, "When RootPath is empty, withRootPath should return the original handler, not a ServeMux")
+}
+
+// TestWithRootPathNonEmptyRootPath tests that withRootPath returns a ServeMux when RootPath is not empty
+func TestWithRootPathNonEmptyRootPath(t *testing.T) {
+	// Create a simple handler
+	originalHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+
+	// Create a server with non-empty RootPath
+	server := &ArgoCDServer{
+		ArgoCDServerOpts: ArgoCDServerOpts{
+			RootPath: "/argocd",
+		},
+	}
+
+	// Call withRootPath
+	handler := withRootPath(originalHandler, server)
+
+	// Verify that the returned handler is a ServeMux
+	_, isServeMux := handler.(*http.ServeMux)
+	assert.True(t, isServeMux, "When RootPath is not empty, withRootPath should return a ServeMux")
+}
+
+// TestNewRedirectServerEmptyRootPath tests that newRedirectServer correctly handles empty rootPath
+func TestNewRedirectServerEmptyRootPath(t *testing.T) {
+	// Call newRedirectServer with empty rootPath
+	server := newRedirectServer(8080, "")
+
+	// Verify the server address
+	assert.Equal(t, "localhost:8080", server.Addr, "When rootPath is empty, server address should be 'localhost:8080'")
+
+	// Test the redirect handler
+	req := httptest.NewRequest(http.MethodGet, "/applications", nil)
+	req.Host = "example.com:8080"
+	w := httptest.NewRecorder()
+
+	server.Handler.ServeHTTP(w, req)
+
+	// Verify the redirect URL
+	assert.Equal(t, http.StatusTemporaryRedirect, w.Code, "Should return a 307 Temporary Redirect status code")
+	redirectURL := w.Header().Get("Location")
+	expectedURL := "https://example.com:8080/applications"
+	assert.Equal(t, expectedURL, redirectURL, "Redirect URL should not include rootPath when rootPath is empty")
+}
+
+// TestNewRedirectServerNonEmptyRootPath tests that newRedirectServer correctly handles non-empty rootPath
+func TestNewRedirectServerNonEmptyRootPath(t *testing.T) {
+	// Call newRedirectServer with non-empty rootPath
+	server := newRedirectServer(8080, "/argocd")
+
+	// Verify the server address
+	assert.Equal(t, "localhost:8080/argocd", server.Addr, "When rootPath is '/argocd', server address should be 'localhost:8080/argocd'")
+
+	// Test the redirect handler
+	req := httptest.NewRequest(http.MethodGet, "/applications", nil)
+	req.Host = "example.com:8080"
+	w := httptest.NewRecorder()
+
+	server.Handler.ServeHTTP(w, req)
+
+	// Verify the redirect URL
+	assert.Equal(t, http.StatusTemporaryRedirect, w.Code, "Should return a 307 Temporary Redirect status code")
+	redirectURL := w.Header().Get("Location")
+	expectedURL := "https://example.com:8080/argocd/applications"
+	assert.Equal(t, expectedURL, redirectURL, "Redirect URL should include rootPath when rootPath is not empty")
+}
+
+// TestNewRedirectServerRootPathDuplication tests that newRedirectServer does not duplicate rootPath in the redirect URL
+func TestNewRedirectServerRootPathDuplication(t *testing.T) {
+	// Call newRedirectServer with non-empty rootPath
+	server := newRedirectServer(8080, "/argocd")
+
+	// Test the redirect handler with a request path that already includes rootPath
+	req := httptest.NewRequest(http.MethodGet, "/argocd/applications", nil)
+	req.Host = "example.com:8080"
+	w := httptest.NewRecorder()
+
+	server.Handler.ServeHTTP(w, req)
+
+	// Verify the redirect URL
+	assert.Equal(t, http.StatusTemporaryRedirect, w.Code, "Should return a 307 Temporary Redirect status code")
+	redirectURL := w.Header().Get("Location")
+
+	// The URL should not have duplicated rootPath
+	duplicatedURL := "https://example.com:8080/argocd/argocd/applications"
+	assert.NotEqual(t, duplicatedURL, redirectURL, "Redirect URL should not have duplicated rootPath")
+
+	// The correct URL should be
+	correctURL := "https://example.com:8080/argocd/applications"
+	assert.Equal(t, correctURL, redirectURL, "Redirect URL should be correct without duplicated rootPath")
+}

server/server.go

@@ -1125,8 +1125,13 @@ func (server *ArgoCDServer) setTokenCookie(token string, w http.ResponseWriter)
 }
 
 func withRootPath(handler http.Handler, a *ArgoCDServer) http.Handler {
+	// If RootPath is empty, directly return the original handler
+	if a.RootPath == "" {
+		return handler
+	}
+
 	// get rid of slashes
-	root := strings.TrimRight(strings.TrimLeft(a.RootPath, "/"), "/")
+	root := strings.Trim(a.RootPath, "/")
 
 	mux := http.NewServeMux()
 	mux.Handle("/"+root+"/", http.StripPrefix("/"+root, handler))
@@ -1343,15 +1348,32 @@ func (server *ArgoCDServer) registerDexHandlers(mux *http.ServeMux) {
 
 // newRedirectServer returns an HTTP server which does a 307 redirect to the HTTPS server
 func newRedirectServer(port int, rootPath string) *http.Server {
-	addr := fmt.Sprintf("localhost:%d/%s", port, strings.TrimRight(strings.TrimLeft(rootPath, "/"), "/"))
+	var addr string
+	if rootPath == "" {
+		addr = fmt.Sprintf("localhost:%d", port)
+	} else {
+		addr = fmt.Sprintf("localhost:%d/%s", port, strings.Trim(rootPath, "/"))
+	}
+
 	return &http.Server{
 		Addr: addr,
 		Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 			target := "https://" + req.Host
+
 			if rootPath != "" {
-				target += "/" + strings.TrimRight(strings.TrimLeft(rootPath, "/"), "/")
+				root := strings.Trim(rootPath, "/")
+				prefix := "/" + root
+
+				// If the request path already starts with rootPath, no need to add rootPath again
+				if strings.HasPrefix(req.URL.Path, prefix) {
+					target += req.URL.Path
+				} else {
+					target += prefix + req.URL.Path
+				}
+			} else {
+				target += req.URL.Path
 			}
-			target += req.URL.Path
+
 			if len(req.URL.RawQuery) > 0 {
 				target += "?" + req.URL.RawQuery
 			}