Merge branch 'delete-set-cookie-header-from-archive-raw-blob' into 'master'

Remove Set-Cookie header from archive and raw blob responses

See merge request gitlab-org/gitlab-workhorse!475

internal/git/archive.go

@@ -160,6 +160,10 @@ func handleArchiveWithGitaly(r *http.Request, params archiveParams, format gital
 func setArchiveHeaders(w http.ResponseWriter, format gitalypb.GetArchiveRequest_Format, archiveFilename string) {
 	w.Header().Del("Content-Length")
 	w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, archiveFilename))
+	// Caching proxies usually don't cache responses with Set-Cookie header
+	// present because it implies user-specific data, which is not the case
+	// for repository archives.
+	w.Header().Del("Set-Cookie")
 	if format == gitalypb.GetArchiveRequest_ZIP {
 		w.Header().Set("Content-Type", "application/zip")
 	} else {

internal/git/archive_test.go

@@ -68,6 +68,9 @@ func TestSetArchiveHeaders(t *testing.T) {
 		w.Header().Set("Content-Length", "test")
 		w.Header().Set("Content-Disposition", "test")
 
+		// This should be deleted
+		w.Header().Set("Set-Cookie", "test")
+
 		// This should be preserved
 		w.Header().Set("Cache-Control", "public, max-age=3600")
 
@@ -77,5 +80,6 @@ func TestSetArchiveHeaders(t *testing.T) {
 		testhelper.AssertResponseWriterHeader(t, w, "Content-Length")
 		testhelper.AssertResponseWriterHeader(t, w, "Content-Disposition", `attachment; filename="filename"`)
 		testhelper.AssertResponseWriterHeader(t, w, "Cache-Control", "public, max-age=3600")
+		testhelper.AssertAbsentResponseWriterHeader(t, w, "Set-Cookie")
 	}
 }

internal/git/blob.go

@@ -32,8 +32,16 @@ func (b *blob) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 		return
 	}
 
+	setBlobHeaders(w)
 	if err := blobClient.SendBlob(ctx, w, &params.GetBlobRequest); err != nil {
 		helper.Fail500(w, r, fmt.Errorf("blob.GetBlob: %v", err))
 		return
 	}
 }
+
+func setBlobHeaders(w http.ResponseWriter) {
+	// Caching proxies usually don't cache responses with Set-Cookie header
+	// present because it implies user-specific data, which is not the case
+	// for blobs.
+	w.Header().Del("Set-Cookie")
+}

internal/git/blob_test.go

+package git
+
+import (
+	"net/http/httptest"
+	"testing"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+)
+
+func TestSetBlobHeaders(t *testing.T) {
+	w := httptest.NewRecorder()
+	w.Header().Set("Set-Cookie", "gitlab_cookie=123456")
+
+	setBlobHeaders(w)
+
+	testhelper.AssertAbsentResponseWriterHeader(t, w, "Set-Cookie")
+}

internal/testhelper/testhelper.go

@@ -77,6 +77,14 @@ func AssertResponseWriterHeader(t *testing.T, w http.ResponseWriter, header stri
 	assertHeaderExists(t, header, actual, expected)
 }
 
+func AssertAbsentResponseWriterHeader(t *testing.T, w http.ResponseWriter, header string) {
+	actual := w.Header()[http.CanonicalHeaderKey(header)]
+
+	if len(actual) != 0 {
+		t.Fatalf("for HTTP request expected not to receive the header %q, got %+v", header, actual)
+	}
+}
+
 func AssertResponseHeader(t *testing.T, w interface{}, header string, expected ...string) {
 	var actual []string