From 0036c260968d083e56e7336259e5464facb1dc73 Mon Sep 17 00:00:00 2001 From: feistel <6742251-feistel@users.noreply.gitlab.com> Date: Tue, 17 Aug 2021 15:57:33 +0000 Subject: test: fix response body not being closed nolint is added when the body is nil or if the body can't be closed --- internal/auth/auth_test.go | 18 ++++++++++++++---- internal/handlers/handlers_test.go | 4 ++++ internal/httptransport/transport_test.go | 2 ++ internal/rejectmethods/middleware_test.go | 2 ++ test/acceptance/artifacts_test.go | 1 + test/acceptance/auth_test.go | 7 ++++++- test/acceptance/metrics_test.go | 1 + test/acceptance/serving_test.go | 3 +++ test/acceptance/unknown_http_method_test.go | 1 + 9 files changed, 34 insertions(+), 5 deletions(-) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index c1fc834a..d03407a5 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -65,7 +65,10 @@ func setSessionValues(t *testing.T, r *http.Request, store sessions.Store, value session.Values = values session.Save(tmpRequest, result) - for _, cookie := range result.Result().Cookies() { + res := result.Result() + defer res.Body.Close() + + for _, cookie := range res.Cookies() { r.AddCookie(cookie) } } @@ -206,10 +209,14 @@ func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) { result := httptest.NewRecorder() require.Equal(t, true, auth.TryAuthenticate(result, r, source.NewMockSource())) + + res := result.Result() + defer res.Body.Close() + require.Equal(t, http.StatusFound, result.Code) require.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location")) - require.Equal(t, 600, result.Result().Cookies()[0].MaxAge) - require.Equal(t, https, result.Result().Cookies()[0].Secure) + require.Equal(t, 600, res.Cookies()[0].MaxAge) + require.Equal(t, https, res.Cookies()[0].Secure) } func TestTryAuthenticateWithCodeAndStateOverHTTP(t *testing.T) { @@ -465,9 +472,12 @@ func TestCheckResponseForInvalidTokenWhenInvalidToken(t *testing.T) { r := &http.Request{URL: reqURL, Host: "pages.gitlab-example.com", RequestURI: "/test"} resp := &http.Response{StatusCode: http.StatusUnauthorized, Body: ioutil.NopCloser(bytes.NewReader([]byte("{\"error\":\"invalid_token\"}")))} + defer resp.Body.Close() require.Equal(t, true, auth.CheckResponseForInvalidToken(result, r, resp)) - require.Equal(t, http.StatusFound, result.Result().StatusCode) + res := result.Result() + defer res.Body.Close() + require.Equal(t, http.StatusFound, res.StatusCode) require.Equal(t, "http://pages.gitlab-example.com/test", result.Header().Get("Location")) } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index aa664266..3c14de35 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -79,6 +79,7 @@ func TestNotFoundWithTokenIsNotHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} + // nolint:bodyclose // FIXME handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "token")(response) require.False(t, handled) @@ -97,6 +98,7 @@ func TestNotFoundWithoutTokenIsNotHandledWhenNotAuthSupport(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} + // nolint:bodyclose // FIXME handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "")(response) require.False(t, handled) @@ -115,6 +117,7 @@ func TestNotFoundWithoutTokenIsHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} + // nolint:bodyclose // FIXME handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "")(response) require.True(t, handled) @@ -133,6 +136,7 @@ func TestInvalidTokenResponseIsHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusUnauthorized} + // nolint:bodyclose // FIXME handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "token")(response) require.True(t, handled) diff --git a/internal/httptransport/transport_test.go b/internal/httptransport/transport_test.go index feaf63b6..7fce1b15 100644 --- a/internal/httptransport/transport_test.go +++ b/internal/httptransport/transport_test.go @@ -58,6 +58,7 @@ func Test_withRoundTripper(t *testing.T) { mtr := &meteredRoundTripper{next: next, durations: histVec, counter: counterVec, ttfbTimeout: DefaultTTFBTimeout} r := httptest.NewRequest("GET", "/", nil) + // nolint:bodyclose // res.Body is nil res, err := mtr.RoundTrip(r) if tt.err != nil { counterCount := testutil.ToFloat64(counterVec.WithLabelValues("error")) @@ -90,6 +91,7 @@ func TestRoundTripTTFBTimeout(t *testing.T) { req, err := http.NewRequest("GET", "https://gitlab.com", nil) require.NoError(t, err) + // nolint:bodyclose // res is nil res, err := mtr.RoundTrip(req) require.Nil(t, res) require.True(t, errors.Is(err, context.Canceled), "context must have been canceled after ttfb timeout") diff --git a/internal/rejectmethods/middleware_test.go b/internal/rejectmethods/middleware_test.go index 2921975a..b1c6a771 100644 --- a/internal/rejectmethods/middleware_test.go +++ b/internal/rejectmethods/middleware_test.go @@ -25,6 +25,7 @@ func TestNewMiddleware(t *testing.T) { middleware.ServeHTTP(recorder, tmpRequest) result := recorder.Result() + defer result.Body.Close() require.Equal(t, http.StatusOK, result.StatusCode) }) @@ -37,6 +38,7 @@ func TestNewMiddleware(t *testing.T) { middleware.ServeHTTP(recorder, tmpRequest) result := recorder.Result() + defer result.Body.Close() require.Equal(t, http.StatusMethodNotAllowed, result.StatusCode) }) diff --git a/test/acceptance/artifacts_test.go b/test/acceptance/artifacts_test.go index 60b18ae3..2b1976a8 100644 --- a/test/acceptance/artifacts_test.go +++ b/test/acceptance/artifacts_test.go @@ -280,6 +280,7 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { // Request auth callback in project domain authrsp, err = GetRedirectPageWithCookie(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery, cookie) require.NoError(t, err) + defer authrsp.Body.Close() // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp.StatusCode) diff --git a/test/acceptance/auth_test.go b/test/acceptance/auth_test.go index d18c2d77..7ec55d95 100644 --- a/test/acceptance/auth_test.go +++ b/test/acceptance/auth_test.go @@ -37,6 +37,7 @@ func TestWhenAuthIsEnabledPrivateWillRedirectToAuthorize(t *testing.T) { require.NoError(t, err) rsp, err = GetRedirectPage(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery) require.NoError(t, err) + defer rsp.Body.Close() require.Equal(t, http.StatusFound, rsp.StatusCode) require.Equal(t, 1, len(rsp.Header["Location"])) @@ -174,6 +175,7 @@ func TestAccessControlUnderCustomDomain(t *testing.T) { // Fetch page in custom domain authrsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path, cookie) require.NoError(t, err) + defer authrsp.Body.Close() require.Equal(t, http.StatusOK, authrsp.StatusCode) }) } @@ -272,6 +274,7 @@ func TestCustomErrorPageWithAuth(t *testing.T) { // Fetch page in custom domain anotherResp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path, groupCookie) require.NoError(t, err) + defer anotherResp.Body.Close() require.Equal(t, http.StatusNotFound, anotherResp.StatusCode) @@ -340,6 +343,7 @@ func TestAccessControlUnderCustomDomainWithHTTPSProxy(t *testing.T) { authrsp, err = GetProxyRedirectPageWithCookie(t, proxyListener, "private.domain.com", "/", cookie, true) require.NoError(t, err) + defer authrsp.Body.Close() require.Equal(t, http.StatusOK, authrsp.StatusCode) } @@ -475,6 +479,7 @@ func testAccessControl(t *testing.T, runPages runPagesFunc) { // Request auth callback in project domain authrsp2, err := GetRedirectPageWithCookie(t, httpsListener, authLoc.Host, authLoc.Path+"?"+authLoc.RawQuery, cookie) require.NoError(t, err) + defer authrsp2.Body.Close() // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp2.StatusCode) @@ -558,7 +563,7 @@ func TestHijackedCode(t *testing.T) { impersonatingRes, err := GetProxyRedirectPageWithCookie(t, proxyListener, targetDomain, "/auth?code="+hijackedCode+"&state="+attackerState, attackerCookie, true) require.NoError(t, err) - defer authrsp.Body.Close() + defer impersonatingRes.Body.Close() require.Equal(t, impersonatingRes.StatusCode, http.StatusInternalServerError, "should fail to decode code") } diff --git a/test/acceptance/metrics_test.go b/test/acceptance/metrics_test.go index 37a3c2b9..ab220c51 100644 --- a/test/acceptance/metrics_test.go +++ b/test/acceptance/metrics_test.go @@ -21,6 +21,7 @@ func TestPrometheusMetricsCanBeScraped(t *testing.T) { res, err := GetPageFromListener(t, httpListener, "zip.gitlab.io", "/symlink.html") require.NoError(t, err) + defer res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) resp, err := http.Get("http://127.0.0.1:42345/metrics") diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go index a4f3085b..c42866ee 100644 --- a/test/acceptance/serving_test.go +++ b/test/acceptance/serving_test.go @@ -217,6 +217,7 @@ func TestCORSWhenDisabled(t *testing.T) { for _, spec := range supportedListeners() { for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodOptions} { rsp := doCrossOriginRequest(t, spec, method, method, spec.URL("project/")) + defer rsp.Body.Close() require.Equal(t, http.StatusOK, rsp.StatusCode) require.Equal(t, "", rsp.Header.Get("Access-Control-Allow-Origin")) @@ -264,6 +265,7 @@ func TestCORSAllowsMethod(t *testing.T) { t.Run(tt.name, func(t *testing.T) { for _, spec := range supportedListeners() { rsp := doCrossOriginRequest(t, spec, tt.method, tt.method, spec.URL("project/")) + defer rsp.Body.Close() require.Equal(t, tt.expectedStatus, rsp.StatusCode) require.Equal(t, tt.expectedOrigin, rsp.Header.Get("Access-Control-Allow-Origin")) @@ -281,6 +283,7 @@ func TestCustomHeaders(t *testing.T) { for _, spec := range supportedListeners() { rsp, err := GetPageFromListener(t, spec, "group.gitlab-example.com:", "project/") require.NoError(t, err) + defer rsp.Body.Close() require.Equal(t, http.StatusOK, rsp.StatusCode) require.Equal(t, "Testing1", rsp.Header.Get("X-Test1")) require.Equal(t, "Testing2", rsp.Header.Get("X-Test2")) diff --git a/test/acceptance/unknown_http_method_test.go b/test/acceptance/unknown_http_method_test.go index 87422292..dfe9c82f 100644 --- a/test/acceptance/unknown_http_method_test.go +++ b/test/acceptance/unknown_http_method_test.go @@ -18,6 +18,7 @@ func TestUnknownHTTPMethod(t *testing.T) { resp, err := DoPagesRequest(t, httpListener, req) require.NoError(t, err) + defer resp.Body.Close() require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode) } -- cgit v1.2.3 From 99bc678dfe8fc6f41a415c19ed5bfc64e162adc7 Mon Sep 17 00:00:00 2001 From: feistel <6742251-feistel@users.noreply.gitlab.com> Date: Thu, 19 Aug 2021 05:34:56 +0000 Subject: docs: add related issue to TODO comment --- internal/handlers/handlers_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 3c14de35..710a167c 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -79,7 +79,7 @@ func TestNotFoundWithTokenIsNotHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} - // nolint:bodyclose // FIXME + // nolint:bodyclose // TODO investigate https://gitlab.com/gitlab-org/gitlab-pages/-/issues/606 handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "token")(response) require.False(t, handled) @@ -98,7 +98,7 @@ func TestNotFoundWithoutTokenIsNotHandledWhenNotAuthSupport(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} - // nolint:bodyclose // FIXME + // nolint:bodyclose // TODO investigate https://gitlab.com/gitlab-org/gitlab-pages/-/issues/606 handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "")(response) require.False(t, handled) @@ -117,7 +117,7 @@ func TestNotFoundWithoutTokenIsHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusNotFound} - // nolint:bodyclose // FIXME + // nolint:bodyclose // TODO investigate https://gitlab.com/gitlab-org/gitlab-pages/-/issues/606 handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "")(response) require.True(t, handled) @@ -136,7 +136,7 @@ func TestInvalidTokenResponseIsHandled(t *testing.T) { reqURL, _ := url.Parse("/") r := &http.Request{URL: reqURL} response := &http.Response{StatusCode: http.StatusUnauthorized} - // nolint:bodyclose // FIXME + // nolint:bodyclose // TODO investigate https://gitlab.com/gitlab-org/gitlab-pages/-/issues/606 handled := handlers.checkIfLoginRequiredOrInvalidToken(w, r, "token")(response) require.True(t, handled) -- cgit v1.2.3