Merge branch '1078-gracefully-handle-missing-content-length-header-from-api' into 'master'

Resolve "Gracefully handle missing Content-Length Header from API"

Closes #1078

See merge request https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/891

Merged-by: Jaime Martinez <jmartinez@gitlab.com>
Approved-by: Vishal Tak <vtak@gitlab.com>
Approved-by: Jaime Martinez <jmartinez@gitlab.com>
Co-authored-by: janis <jaltherr@gitlab.com>

2 files changed, 46 insertions, 14 deletions

diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go
index 69b74322..de79bc8d 100644
--- a/internal/artifact/artifact.go
+++ b/internal/artifact/artifact.go
@@ -133,7 +133,15 @@ func (a *Artifact) makeRequest(w http.ResponseWriter, r *http.Request, reqURL *u
 	}
 
 	w.Header().Set("Content-Type", resp.Header.Get("Content-Type"))
-	w.Header().Set("Content-Length", strconv.FormatInt(resp.ContentLength, 10))
+
+	// If the API uses chunked encoding, it will omit the Content-Length Header.
+	// Go uses a value of -1, which is an invalid value.
+	// We should omit the header entirely in that case.
+	// see: https://gitlab.com/gitlab-org/gitlab-pages/-/issues/1078
+	if resp.ContentLength >= 0 {
+		w.Header().Set("Content-Length", strconv.FormatInt(resp.ContentLength, 10))
+	}
+
 	w.WriteHeader(resp.StatusCode)
 	io.Copy(w, resp.Body)
 }
diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go
index 6da0e2d9..8f40b842 100644
--- a/internal/artifact/artifact_test.go
+++ b/internal/artifact/artifact_test.go
@@ -17,22 +17,24 @@ func TestTryMakeRequest(t *testing.T) {
 	contentType := "text/html; charset=utf-8"
 
 	cases := []struct {
-		Path         string
-		Token        string
-		Status       int
-		Content      string
-		Length       string
-		CacheControl string
-		ContentType  string
-		Description  string
-		RemoteAddr   string
-		ForwardedIP  string
+		Path                   string
+		Token                  string
+		Status                 int
+		Content                string
+		APIUsesChunkedEncoding bool
+		Length                 string
+		CacheControl           string
+		ContentType            string
+		Description            string
+		RemoteAddr             string
+		ForwardedIP            string
 	}{
 		{
 			"/200.html",
 			"",
 			http.StatusOK,
 			content,
+			false,
 			"90",
 			"max-age=3600",
 			"text/html; charset=utf-8",
@@ -45,6 +47,7 @@ func TestTryMakeRequest(t *testing.T) {
 			"token",
 			http.StatusOK,
 			content,
+			false,
 			"90",
 			"",
 			"text/html; charset=utf-8",
@@ -57,6 +60,7 @@ func TestTryMakeRequest(t *testing.T) {
 			"",
 			http.StatusIMUsed,
 			content,
+			false,
 			"90",
 			"max-age=3600",
 			"text/html; charset=utf-8",
@@ -69,6 +73,7 @@ func TestTryMakeRequest(t *testing.T) {
 			"",
 			http.StatusTeapot,
 			content,
+			false,
 			"90",
 			"",
 			"text/html; charset=utf-8",
@@ -76,11 +81,24 @@ func TestTryMakeRequest(t *testing.T) {
 			"1.2.3.4",
 			"1.2.3.4",
 		},
+		{
+			"/200.html",
+			"",
+			http.StatusOK,
+			content,
+			true,
+			"",
+			"max-age=3600",
+			"text/html; charset=utf-8",
+			"basic successful request",
+			"1.2.3.4:8000",
+			"1.2.3.4",
+		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.Description, func(t *testing.T) {
-			testServer := makeArtifactServerStub(t, content, contentType, c.ForwardedIP)
+			testServer := makeArtifactServerStub(t, content, contentType, c.ForwardedIP, c.APIUsesChunkedEncoding)
 			defer testServer.Close()
 
 			url := "https://group.gitlab-example.io/-/subgroup/project/-/jobs/1/artifacts" + c.Path
@@ -104,7 +122,8 @@ func TestTryMakeRequest(t *testing.T) {
 }
 
 // provide stub for testing different artifact responses
-func makeArtifactServerStub(t *testing.T, content string, contentType string, expectedForwardedIP string) *httptest.Server {
+func makeArtifactServerStub(t *testing.T, content string, contentType string,
+	expectedForwardedIP string, chunkedEncoding bool) *httptest.Server {
 	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		require.Equal(t, expectedForwardedIP, r.Header.Get("X-Forwarded-For"))
 
@@ -125,6 +144,11 @@ func makeArtifactServerStub(t *testing.T, content string, contentType string, ex
 			w.WriteHeader(999)
 		}
 		fmt.Fprint(w, content)
+		if chunkedEncoding {
+			// this forces go to use chunked encoding
+			// and omitting the "Content-Length" Header
+			w.(http.Flusher).Flush()
+		}
 	}))
 }
 
@@ -296,7 +320,7 @@ func TestBuildURL(t *testing.T) {
 func TestContextCanceled(t *testing.T) {
 	content := "<!DOCTYPE html><html><head><title>Title of the document</title></head><body></body></html>"
 	contentType := "text/html; charset=utf-8"
-	testServer := makeArtifactServerStub(t, content, contentType, "")
+	testServer := makeArtifactServerStub(t, content, contentType, "", false)
 	t.Cleanup(testServer.Close)
 
 	result := httptest.NewRecorder()