From 79d5c238ccfedd54e71e85ef7e1754511ac8b256 Mon Sep 17 00:00:00 2001
From: Steven Masley <Emyrk@users.noreply.github.com>
Date: Thu, 1 Feb 2024 10:13:34 -0600
Subject: [PATCH] fix: always return a clean http client for promoauth (#11963)

* fix: add unit test to verify default client is not broken

* always return a clean http client
* No need to clone the tripper
---
 coderd/promoauth/oauth2.go      | 18 +++++++++---------
 coderd/promoauth/oauth2_test.go | 15 +++++++++++++--
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/coderd/promoauth/oauth2.go b/coderd/promoauth/oauth2.go
index 30e5269cd3..b3d4d6df30 100644
--- a/coderd/promoauth/oauth2.go
+++ b/coderd/promoauth/oauth2.go
@@ -214,10 +214,16 @@ func (c *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.To
 	return c.underlying.TokenSource(c.wrapClient(ctx, SourceTokenSource), token)
 }
 
+// InstrumentHTTPClient will always return a new http client. The new client will
+// match the one passed in, but will have an instrumented round tripper.
 func (c *Config) InstrumentHTTPClient(hc *http.Client, source Oauth2Source) *http.Client {
-	// The new tripper will instrument every request made by the oauth2 client.
-	hc.Transport = newInstrumentedTripper(c, source, hc.Transport)
-	return hc
+	return &http.Client{
+		// The new tripper will instrument every request made by the oauth2 client.
+		Transport:     newInstrumentedTripper(c, source, hc.Transport),
+		CheckRedirect: hc.CheckRedirect,
+		Jar:           hc.Jar,
+		Timeout:       hc.Timeout,
+	}
 }
 
 // wrapClient is the only way we can accurately instrument the oauth2 client.
@@ -257,12 +263,6 @@ func newInstrumentedTripper(c *Config, source Oauth2Source, under http.RoundTrip
 		under = http.DefaultTransport
 	}
 
-	// If the underlying transport is the default, we need to clone it.
-	// We should also clone it if it supports cloning.
-	if tr, ok := under.(*http.Transport); ok {
-		under = tr.Clone()
-	}
-
 	return &instrumentedTripper{
 		c:          c,
 		source:     source,
diff --git a/coderd/promoauth/oauth2_test.go b/coderd/promoauth/oauth2_test.go
index 4dce3d6248..845e2dbcb5 100644
--- a/coderd/promoauth/oauth2_test.go
+++ b/coderd/promoauth/oauth2_test.go
@@ -60,12 +60,23 @@ func TestInstrument(t *testing.T) {
 	// 0 Requests before we start
 	require.Nil(t, metricValue(t, reg, metricname, labels), "no metrics at start")
 
+	noClientCtx := ctx
+	// This should never be done, but promoauth should not break the default client
+	// even if this happens. So intentionally do this to verify nothing breaks.
+	ctx = context.WithValue(ctx, oauth2.HTTPClient, http.DefaultClient)
 	// Exchange should trigger a request
 	code := idp.CreateAuthCode(t, "foo")
-	token, err := cfg.Exchange(ctx, code)
+	_, err := cfg.Exchange(ctx, code)
 	require.NoError(t, err)
 	require.Equal(t, count("Exchange"), 1)
 
+	// Do an exchange without a default http client as well to verify original
+	// transport is not broken.
+	code = idp.CreateAuthCode(t, "bar")
+	token, err := cfg.Exchange(noClientCtx, code)
+	require.NoError(t, err)
+	require.Equal(t, count("Exchange"), 2)
+
 	// Force a refresh
 	token.Expiry = time.Now().Add(time.Hour * -1)
 	src := cfg.TokenSource(ctx, token)
@@ -92,7 +103,7 @@ func TestInstrument(t *testing.T) {
 	require.NoError(t, err)
 	_ = resp.Body.Close()
 
-	require.NoError(t, compare(reg, snapshot), "no metric changes")
+	require.NoError(t, compare(reg, snapshot), "http default client corrupted")
 }
 
 func TestGithubRateLimits(t *testing.T) {