From 46679c82eb242cd52657681f3ba8e04b94e37677 Mon Sep 17 00:00:00 2001
From: Lukasz Raczylo <lukasz@raczylo.com>
Date: Tue, 19 May 2026 14:07:44 +0100
Subject: [PATCH] refactor(oidcgate): simplify cloneAndRewrite, flip ?rd
 precedence, assert XFU passthrough

---
 cmd/oidcgate/endpoints.go      | 16 +++++++++-------
 cmd/oidcgate/endpoints_test.go | 17 +++++++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/cmd/oidcgate/endpoints.go b/cmd/oidcgate/endpoints.go
index a577910..0a3ef73 100644
--- a/cmd/oidcgate/endpoints.go
+++ b/cmd/oidcgate/endpoints.go
@@ -28,7 +28,10 @@ func newAuthHandler(next http.Handler) http.Handler {
 func newStartHandler(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		r2 := cloneAndRewrite(req, sentinelPath)
-		if rd := req.URL.Query().Get("rd"); rd != "" && r2.Header.Get("X-Forwarded-Uri") == "" {
+		// Precedence: explicit ?rd= wins over an ambient upstream
+		// X-Forwarded-Uri so /oauth2/start?rd=/dashboard does not get
+		// silently overridden by the proxy's current-URL forwarding.
+		if rd := req.URL.Query().Get("rd"); rd != "" {
 			r2.Header.Set("X-Forwarded-Uri", rd)
 		}
 		next.ServeHTTP(rw, r2)
@@ -55,13 +58,12 @@ func newLogoutHandler(next http.Handler, logoutURL string) http.Handler {
 	})
 }
 
-// cloneAndRewrite returns a shallow clone of req with URL.Path set to newPath.
-// The query string is preserved verbatim — middleware logic for code/state
-// extraction reads URL.Query() which still works.
+// cloneAndRewrite returns a clone of req with URL.Path set to newPath.
+// req.Clone deep-copies URL via net/http's cloneURL, so mutating
+// r2.URL.Path does not affect the original req. RawQuery, Host,
+// Fragment, RawPath are preserved unchanged.
 func cloneAndRewrite(req *http.Request, newPath string) *http.Request {
 	r2 := req.Clone(req.Context())
-	u := *req.URL
-	u.Path = newPath
-	r2.URL = &u
+	r2.URL.Path = newPath
 	return r2
 }
diff --git a/cmd/oidcgate/endpoints_test.go b/cmd/oidcgate/endpoints_test.go
index c5c1e02..8ecb8ca 100644
--- a/cmd/oidcgate/endpoints_test.go
+++ b/cmd/oidcgate/endpoints_test.go
@@ -49,6 +49,9 @@ func TestAuth_RewritesToSentinel_AndConverts302To401(t *testing.T) {
 	if rec.Header().Get("X-Auth-Redirect") == "" {
 		t.Error("X-Auth-Redirect should carry Location")
 	}
+	if got := stub.calls[0].header.Get("X-Forwarded-Uri"); got != "/protected/page" {
+		t.Errorf("X-Forwarded-Uri must pass through to middleware: want /protected/page, got %q", got)
+	}
 }
 
 func TestAuth_AuthenticatedReturnsHeadersAnd200(t *testing.T) {
@@ -107,6 +110,20 @@ func TestStart_ForwardsRdAsXForwardedURI(t *testing.T) {
 	}
 }
 
+func TestStart_RdQueryWinsOverUpstreamHeader(t *testing.T) {
+	stub := &stubMiddleware{
+		fn: func(rw http.ResponseWriter, req *http.Request) { rw.WriteHeader(http.StatusFound) },
+	}
+	h := newStartHandler(stub)
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/oauth2/start?rd=/explicit", nil)
+	req.Header.Set("X-Forwarded-Uri", "/ambient")
+	h.ServeHTTP(rec, req)
+	if got := stub.calls[0].header.Get("X-Forwarded-Uri"); got != "/explicit" {
+		t.Fatalf("?rd= must win over upstream X-Forwarded-Uri: want /explicit, got %q", got)
+	}
+}
+
 func TestCallback_RewritesToConfiguredCallbackURL(t *testing.T) {
 	var seenPath, seenQuery string
 	stub := &stubMiddleware{