16535: Fix error response codes for invalid names (4xx, not 5xx).

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index c6f501e492e6b5f627e9dbae79ec12aec98b1214..5043c65ec54e23c66e301b87f8f99ce7aeada255 100644 (file)
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -5,6 +5,7 @@
 package main
 
 import (
+       "errors"
        "fmt"
        "io"
        "net/http"
@@ -43,27 +44,38 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
        fs := client.SiteFileSystem(kc)
        fs.ForwardSlashNameSubstitution(h.Config.cluster.Collections.ForwardSlashNameSubstitution)
 
+       fi, err := fs.Stat(r.URL.Path)
        switch r.Method {
        case "GET":
-               fi, err := fs.Stat(r.URL.Path)
-               if os.IsNotExist(err) {
-                       http.Error(w, err.Error(), http.StatusNotFound)
-                       return true
-               } else if err != nil {
-                       http.Error(w, err.Error(), http.StatusInternalServerError)
-                       return true
-               } else if fi.IsDir() {
+               if os.IsNotExist(err) ||
+                       (err != nil && err.Error() == "not a directory") ||
+                       (fi != nil && fi.IsDir()) {
                        http.Error(w, "not found", http.StatusNotFound)
+                       return true
                }
                http.FileServer(fs).ServeHTTP(w, r)
                return true
        case "PUT":
+               if strings.HasSuffix(r.URL.Path, "/") {
+                       http.Error(w, "invalid object name (trailing '/' char)", http.StatusBadRequest)
+                       return true
+               }
+               if err != nil && err.Error() == "not a directory" {
+                       // requested foo/bar, but foo is a file
+                       http.Error(w, "object name conflicts with existing object", http.StatusBadRequest)
+                       return true
+               }
                f, err := fs.OpenFile(r.URL.Path, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
                if os.IsNotExist(err) {
                        // create missing intermediate directories, then try again
                        for i, c := range r.URL.Path {
                                if i > 0 && c == '/' {
                                        dir := r.URL.Path[:i]
+                                       if strings.HasSuffix(dir, "/") {
+                                               err = errors.New("invalid object name (consecutive '/' chars)")
+                                               http.Error(w, err.Error(), http.StatusBadRequest)
+                                               return true
+                                       }
                                        err := fs.Mkdir(dir, 0755)
                                        if err != nil && err != os.ErrExist {
                                                err = fmt.Errorf("mkdir %q failed: %w", dir, err)

diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index fbfef7b91b19d8af73081bebf2c8be4f68769717..c4c216c8b9c647aae6fc676a79b43f76b43e70e6 100644 (file)
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -9,6 +9,7 @@ import (
        "crypto/rand"
        "io/ioutil"
        "os"
+       "sync"
        "time"
 
        "git.arvados.org/arvados.git/sdk/go/arvados"
@@ -110,7 +111,7 @@ func (s *IntegrationSuite) testS3GetObject(c *check.C, bucket *s3.Bucket, prefix
        c.Check(err, check.IsNil)
 
        rdr, err = bucket.GetReader(prefix + "missingfile")
-       c.Check(err, check.NotNil)
+       c.Check(err, check.ErrorMatches, `404 Not Found`)
 
        rdr, err = bucket.GetReader(prefix + "sailboat.txt")
        c.Assert(err, check.IsNil)
@@ -152,7 +153,7 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket,
                objname := prefix + trial.path
 
                _, err := bucket.GetReader(objname)
-               c.Assert(err, check.NotNil)
+               c.Assert(err, check.ErrorMatches, `404 Not Found`)
 
                buf := make([]byte, trial.size)
                rand.Read(buf)
@@ -167,7 +168,7 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket,
                buf2, err := ioutil.ReadAll(rdr)
                c.Check(err, check.IsNil)
                c.Check(buf2, check.HasLen, len(buf))
-               c.Check(buf2, check.DeepEquals, buf)
+               c.Check(bytes.Equal(buf, buf2), check.Equals, true)
        }
 }
 
@@ -182,6 +183,7 @@ func (s *IntegrationSuite) TestS3ProjectPutObjectFailure(c *check.C) {
        s.testS3PutObjectFailure(c, stage.projbucket, stage.coll.Name+"/")
 }
 func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket, prefix string) {
+       var wg sync.WaitGroup
        for _, trial := range []struct {
                path string
        }{
@@ -209,19 +211,25 @@ func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket,
                        path: "",
                },
        } {
-               c.Logf("=== %v", trial)
+               trial := trial
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       c.Logf("=== %v", trial)
 
-               objname := prefix + trial.path
+                       objname := prefix + trial.path
 
-               buf := make([]byte, 1234)
-               rand.Read(buf)
+                       buf := make([]byte, 1234)
+                       rand.Read(buf)
 
-               err := bucket.PutReader(objname, bytes.NewReader(buf), int64(len(buf)), "application/octet-stream", s3.Private, s3.Options{})
-               if !c.Check(err, check.NotNil, check.Commentf("name %q should be rejected", objname)) {
-                       continue
-               }
+                       err := bucket.PutReader(objname, bytes.NewReader(buf), int64(len(buf)), "application/octet-stream", s3.Private, s3.Options{})
+                       if !c.Check(err, check.ErrorMatches, `400 Bad.*`, check.Commentf("PUT %q should fail", objname)) {
+                               return
+                       }
 
-               _, err = bucket.GetReader(objname)
-               c.Check(err, check.NotNil)
+                       _, err = bucket.GetReader(objname)
+                       c.Check(err, check.ErrorMatches, `404 Not Found`, check.Commentf("GET %q should return 404", objname))
+               }()
        }
+       wg.Wait()
 }