From 7d9798c1fa055d2bc16ec043dd4ed2d82867ef29 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Sun, 6 Sep 2026 16:18:42 -0500 Subject: [PATCH] chore(lint): clear the lint findings from the client refresh change The request helpers in these two test files hand back an *http.Response and close its body from t.Cleanup. bodyclose cannot see through the function boundary, so it blamed all eight callers instead. Only three were reported, because max-same-issues stops at three, which is why fixing the named lines would have surfaced the next batch on the following run. The helpers now return the response facts the tests actually assert on, and close the body where it is read. Nothing reads the body in any of these tests. anonRequest went through http.Client.Get, which has no context, so it now builds a request the same way its two siblings do. refreshThreshold was passed 5 minutes at every call site. Rather than delete the parameter, there is now a test that pins it: the same 50 minute session that is left alone under a 5 minute threshold gets rotated under a 60 minute one. Nothing proved the configured value was read before. errcheck runs with check-blank, so the bare _ = on json.Unmarshal was not enough on its own. It carries the reason now, matching the line above it. --- middleware/client_autorefresh_test.go | 80 +++++++++++++++++---------- middleware/response_delivery_test.go | 15 +++-- sdk/go/client_refresh.go | 2 +- 3 files changed, 61 insertions(+), 36 deletions(-) diff --git a/middleware/client_autorefresh_test.go b/middleware/client_autorefresh_test.go index 38571e59..1442a190 100644 --- a/middleware/client_autorefresh_test.go +++ b/middleware/client_autorefresh_test.go @@ -129,15 +129,24 @@ func refreshRouter(stub *refreshStub, threshold time.Duration) forge.Router { return router } -// cookieRequest issues GET /test presenting token in the session cookie, the -// way a browser does, over a real connection. +// testResponse is the part of a response these tests assert on. The helpers +// hand back one of these rather than the *http.Response itself so the body is +// closed where it is read, instead of leaving every caller holding one open. +type testResponse struct { + StatusCode int + Header http.Header + Cookies []*http.Cookie +} + +// serveRequest issues GET /test against router over a real connection, letting +// decorate attach whatever credential the caller is exercising. // // Deliberately not an httptest.ResponseRecorder: a recorder's Header() map goes // on accepting writes after WriteHeader has already snapshotted it, so a header // this middleware sets too late still reads back fine. Forge streams the // response straight to the connection, so only a real server can tell whether a // header was actually delivered. -func cookieRequest(t *testing.T, router forge.Router, token string) *http.Response { +func serveRequest(t *testing.T, router forge.Router, decorate func(*http.Request)) testResponse { t.Helper() srv := httptest.NewServer(router) @@ -145,42 +154,41 @@ func cookieRequest(t *testing.T, router forge.Router, token string) *http.Respon req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL+"/test", nil) require.NoError(t, err) - req.AddCookie(&http.Cookie{Name: refreshCookieName, Value: token}) + if decorate != nil { + decorate(req) + } resp, err := srv.Client().Do(req) require.NoError(t, err) - t.Cleanup(func() { _ = resp.Body.Close() }) - return resp + defer func() { _ = resp.Body.Close() }() + + return testResponse{ + StatusCode: resp.StatusCode, + Header: resp.Header.Clone(), + Cookies: resp.Cookies(), + } } -// bearerRequest is cookieRequest's Authorization-header counterpart. -func bearerRequest(t *testing.T, router forge.Router, token string) *http.Response { +// cookieRequest presents token in the session cookie, the way a browser does. +func cookieRequest(t *testing.T, router forge.Router, token string) testResponse { t.Helper() + return serveRequest(t, router, func(req *http.Request) { + req.AddCookie(&http.Cookie{Name: refreshCookieName, Value: token}) + }) +} - srv := httptest.NewServer(router) - t.Cleanup(srv.Close) - - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL+"/test", nil) - require.NoError(t, err) - req.Header.Set("Authorization", "Bearer "+token) - - resp, err := srv.Client().Do(req) - require.NoError(t, err) - t.Cleanup(func() { _ = resp.Body.Close() }) - return resp +// bearerRequest is cookieRequest's Authorization-header counterpart. +func bearerRequest(t *testing.T, router forge.Router, token string) testResponse { + t.Helper() + return serveRequest(t, router, func(req *http.Request) { + req.Header.Set("Authorization", "Bearer "+token) + }) } // anonRequest issues GET /test with no credential at all. -func anonRequest(t *testing.T, router forge.Router) *http.Response { +func anonRequest(t *testing.T, router forge.Router) testResponse { t.Helper() - - srv := httptest.NewServer(router) - t.Cleanup(srv.Close) - - resp, err := srv.Client().Get(srv.URL + "/test") - require.NoError(t, err) - t.Cleanup(func() { _ = resp.Body.Close() }) - return resp + return serveRequest(t, router, nil) } // TestClientAutoRefresh_RotatesNearExpiry is the fix this middleware exists @@ -197,7 +205,7 @@ func TestClientAutoRefresh_RotatesNearExpiry(t *testing.T) { // The rotated cookie is replayed verbatim, so its attributes are whatever // the identity server chose rather than anything this service guessed. - cookies := resp.Cookies() + cookies := resp.Cookies require.Len(t, cookies, 1, "rotated cookie must reach the browser") assert.Equal(t, refreshCookieName, cookies[0].Name) assert.Equal(t, "rotated-token", cookies[0].Value) @@ -224,6 +232,18 @@ func TestClientAutoRefresh_SkipsWhenFarFromExpiry(t *testing.T) { assert.Empty(t, resp.Header.Get("X-Auth-Token")) } +// The window comes from the configured threshold rather than a constant baked +// into the middleware: the same 50-minute session left alone above is rotated +// once the threshold is widened past its expiry. +func TestClientAutoRefresh_HonoursConfiguredThreshold(t *testing.T) { + stub := newRefreshStub(t, "fresh-token", 50*time.Minute) + resp := cookieRequest(t, refreshRouter(stub, 60*time.Minute), "fresh-token") + + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, int32(1), stub.refreshCalls.Load(), "a wider threshold must bring this session into the window") + assert.Equal(t, "rotated-token", resp.Header.Get("X-Auth-Token")) +} + // An introspection response with no expiry gives no basis to decide, so the // session is left alone rather than rotated on every single request. func TestClientAutoRefresh_SkipsWhenExpiryUnknown(t *testing.T) { @@ -255,7 +275,7 @@ func TestClientAutoRefresh_RefreshFailureIsNonFatal(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode, "refresh failure must not change the response") assert.Equal(t, int32(1), stub.refreshCalls.Load()) assert.Empty(t, resp.Header.Get("X-Auth-Token")) - assert.Empty(t, resp.Cookies()) + assert.Empty(t, resp.Cookies) } // An unauthenticated request has no session to rotate. diff --git a/middleware/response_delivery_test.go b/middleware/response_delivery_test.go index c0c5d03f..0e44dad8 100644 --- a/middleware/response_delivery_test.go +++ b/middleware/response_delivery_test.go @@ -38,7 +38,7 @@ import ( // serveOverTheWire runs one authenticated GET against a real server and returns // the response as a client saw it. -func serveOverTheWire(t *testing.T, router forge.Router) *http.Response { +func serveOverTheWire(t *testing.T, router forge.Router) testResponse { t.Helper() srv := httptest.NewServer(router) @@ -50,8 +50,13 @@ func serveOverTheWire(t *testing.T, router forge.Router) *http.Response { resp, err := srv.Client().Do(req) require.NoError(t, err) - t.Cleanup(func() { _ = resp.Body.Close() }) - return resp + defer func() { _ = resp.Body.Close() }() + + return testResponse{ + StatusCode: resp.StatusCode, + Header: resp.Header.Clone(), + Cookies: resp.Cookies(), + } } // deliveryCookieSetter is the CookieSetter both middlewares are given, standing @@ -106,7 +111,7 @@ func TestAutoRefresh_DeliversRotatedCookieAndHeaders(t *testing.T) { "the rotated access token must reach the client") assert.NotEmpty(t, resp.Header.Get("X-Auth-Token-Expires-At")) - cookies := resp.Cookies() + cookies := resp.Cookies require.Len(t, cookies, 1, "the rotated session cookie must reach the browser") assert.Equal(t, "rotated-token", cookies[0].Value) } @@ -143,7 +148,7 @@ func TestSessionActivity_DeliversExtendedCookie(t *testing.T) { resp := serveOverTheWire(t, router) require.Equal(t, http.StatusOK, resp.StatusCode) - cookies := resp.Cookies() + cookies := resp.Cookies require.Len(t, cookies, 1, "the extended session cookie must reach the browser") assert.Equal(t, "test-token", cookies[0].Value) assert.Equal(t, int((7 * 24 * time.Hour).Seconds()), cookies[0].MaxAge, diff --git a/sdk/go/client_refresh.go b/sdk/go/client_refresh.go index a6f9e767..5ff04932 100644 --- a/sdk/go/client_refresh.go +++ b/sdk/go/client_refresh.go @@ -105,7 +105,7 @@ func (c *Client) RefreshTokensWithCookies(ctx context.Context, cookieHeader stri } msg := "" if len(raw) > 0 { - _ = json.Unmarshal(raw, &env) // best-effort + _ = json.Unmarshal(raw, &env) //nolint:errcheck // best-effort: a non-JSON error body just leaves msg empty, and RawBody still carries it msg = firstNonEmptyClientErrorMessage(env.Error, env.Message, env.Details) } return nil, &ClientError{