From 065aa362326aae3ec05958436053c72299bdad7d Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@tomclegg.ca>
Date: Fri, 28 Aug 2020 11:28:29 -0400
Subject: [PATCH] 16535: Avoid including empty CommonPrefixes tag in list
 response.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>
---
 services/keep-web/s3.go      | 36 +++++++++++++++++++++++-------------
 services/keep-web/s3_test.go | 17 +++++++++++++++++
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index 01bc8b7047..4e8028ae6e 100644
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -348,12 +348,25 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 		walkpath = ""
 	}
 
-	resp := s3.ListResp{
-		Name:      strings.SplitN(r.URL.Path[1:], "/", 2)[0],
-		Prefix:    params.prefix,
-		Delimiter: params.delimiter,
-		Marker:    params.marker,
-		MaxKeys:   params.maxKeys,
+	type commonPrefix struct {
+		Prefix string
+	}
+	type listResp struct {
+		XMLName string `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListBucketResult"`
+		s3.ListResp
+		// s3.ListResp marshals an empty tag when
+		// CommonPrefixes is nil, which confuses some clients.
+		// Fix by using this nested struct instead.
+		CommonPrefixes []commonPrefix
+	}
+	resp := listResp{
+		ListResp: s3.ListResp{
+			Name:      strings.SplitN(r.URL.Path[1:], "/", 2)[0],
+			Prefix:    params.prefix,
+			Delimiter: params.delimiter,
+			Marker:    params.marker,
+			MaxKeys:   params.maxKeys,
+		},
 	}
 	commonPrefixes := map[string]bool{}
 	err := walkFS(fs, strings.TrimSuffix(bucketdir+"/"+walkpath, "/"), true, func(path string, fi os.FileInfo) error {
@@ -435,18 +448,15 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 		return
 	}
 	if params.delimiter != "" {
+		resp.CommonPrefixes = make([]commonPrefix, 0, len(commonPrefixes))
 		for prefix := range commonPrefixes {
-			resp.CommonPrefixes = append(resp.CommonPrefixes, prefix)
-			sort.Strings(resp.CommonPrefixes)
+			resp.CommonPrefixes = append(resp.CommonPrefixes, commonPrefix{prefix})
 		}
+		sort.Slice(resp.CommonPrefixes, func(i, j int) bool { return resp.CommonPrefixes[i].Prefix < resp.CommonPrefixes[j].Prefix })
 	}
-	wrappedResp := struct {
-		XMLName string `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListBucketResult"`
-		s3.ListResp
-	}{"", resp}
 	w.Header().Set("Content-Type", "application/xml")
 	io.WriteString(w, xml.Header)
-	if err := xml.NewEncoder(w).Encode(wrappedResp); err != nil {
+	if err := xml.NewEncoder(w).Encode(resp); err != nil {
 		ctxlog.FromContext(r.Context()).WithError(err).Error("error writing xml response")
 	}
 }
diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index b82f1efd78..c6d53238e8 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -394,6 +394,23 @@ func (s *IntegrationSuite) TestS3GetBucketVersioning(c *check.C) {
 	}
 }
 
+// If there are no CommonPrefixes entries, the CommonPrefixes XML tag
+// should not appear at all.
+func (s *IntegrationSuite) TestS3ListNoCommonPrefixes(c *check.C) {
+	stage := s.s3setup(c)
+	defer stage.teardown(c)
+
+	req, err := http.NewRequest("GET", stage.collbucket.URL("/"), nil)
+	c.Assert(err, check.IsNil)
+	req.Header.Set("Authorization", "AWS "+arvadostest.ActiveTokenV2+":none")
+	req.URL.RawQuery = "prefix=asdfasdfasdf&delimiter=/"
+	resp, err := http.DefaultClient.Do(req)
+	c.Assert(err, check.IsNil)
+	buf, err := ioutil.ReadAll(resp.Body)
+	c.Assert(err, check.IsNil)
+	c.Check(string(buf), check.Not(check.Matches), `(?ms).*CommonPrefixes.*`)
+}
+
 func (s *IntegrationSuite) TestS3CollectionList(c *check.C) {
 	stage := s.s3setup(c)
 	defer stage.teardown(c)
-- 
2.30.2