Merge pull request #37 from ycjlin/fix/listobjects-nosuchbucket-prefix

fix: ListObjects should return NoSuchBucket for prefix on missing bucket

cmd/bucket-handlers_test.go

@@ -24,12 +24,52 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strconv"
 	"testing"
 
 	"github.com/minio/minio/internal/auth"
 )
 
+func TestListObjectsNonExistentBucketHandler(t *testing.T) {
+	ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: testListObjectsNonExistentBucketHandler})
+}
+
+func testListObjectsNonExistentBucketHandler(_ ObjectLayer, instanceType, _ string, apiRouter http.Handler,
+	credentials auth.Credentials, t *testing.T,
+) {
+	const bucket = "missing-bucket"
+	testCases := []struct {
+		name  string
+		query url.Values
+	}{
+		{name: "ListObjects", query: url.Values{"prefix": {"/"}}},
+		{name: "ListObjectsV2", query: url.Values{"list-type": {"2"}, "prefix": {"/"}}},
+		{name: "ListObjectVersions", query: url.Values{"versions": {""}, "prefix": {"/"}}},
+	}
+
+	for _, tc := range testCases {
+		req, err := newTestSignedRequestV4(http.MethodGet, makeTestTargetURL("", bucket, "", tc.query), 0, nil,
+			credentials.AccessKey, credentials.SecretKey, nil)
+		if err != nil {
+			t.Fatalf("%s: %s: failed to create request: %v", instanceType, tc.name, err)
+		}
+		rec := httptest.NewRecorder()
+		apiRouter.ServeHTTP(rec, req)
+		if rec.Code != http.StatusNotFound {
+			t.Errorf("%s: %s: expected status %d, got %d", instanceType, tc.name, http.StatusNotFound, rec.Code)
+		}
+
+		var apiErr APIErrorResponse
+		if err = xml.Unmarshal(rec.Body.Bytes(), &apiErr); err != nil {
+			t.Fatalf("%s: %s: failed to decode error response: %v", instanceType, tc.name, err)
+		}
+		if apiErr.Code != "NoSuchBucket" {
+			t.Errorf("%s: %s: expected NoSuchBucket, got %q", instanceType, tc.name, apiErr.Code)
+		}
+	}
+}
+
 // Wrapper for calling RemoveBucket HTTP handler tests for both Erasure multiple disks and single node setup.
 func TestRemoveBucketHandler(t *testing.T) {
 	ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{t: t, objAPITest: testRemoveBucketHandler, endpoints: []string{"RemoveBucket"}})

cmd/metacache-server-pool.go

@@ -71,13 +71,13 @@ func (z *erasureServerPools) listPath(ctx context.Context, o *listPathOptions) (
 	if o.Marker != "" && o.Prefix != "" {
 		// Marker not common with prefix is not implemented. Send an empty response
 		if !HasPrefix(o.Marker, o.Prefix) {
-			return entries, io.EOF
+			return entries, z.listPathShortcutEOF(ctx, o.Bucket)
 		}
 	}
 
 	// With max keys of zero we have reached eof, return right here.
 	if o.Limit == 0 {
-		return entries, io.EOF
+		return entries, z.listPathShortcutEOF(ctx, o.Bucket)
 	}
 
 	// For delimiter and prefix as '/' we do not list anything at all
@@ -85,7 +85,7 @@ func (z *erasureServerPools) listPath(ctx context.Context, o *listPathOptions) (
 	// as '/' we don't have any entries, since all the keys are
 	// of form 'keyName/...'
 	if strings.HasPrefix(o.Prefix, SlashSeparator) {
-		return entries, io.EOF
+		return entries, z.listPathShortcutEOF(ctx, o.Bucket)
 	}
 
 	// If delimiter is slashSeparator we must return directories of
@@ -254,6 +254,16 @@ func (z *erasureServerPools) listPath(ctx context.Context, o *listPathOptions) (
 	return entries, nil
 }
 
+// listPathShortcutEOF verifies bucket existence for shortcuts that return
+// without consulting the storage layer. Keep this check out of the normal
+// listing path since GetBucketInfo fans out to peers and disks.
+func (z *erasureServerPools) listPathShortcutEOF(ctx context.Context, bucket string) error {
+	if _, err := z.GetBucketInfo(ctx, bucket, BucketOptions{}); err != nil {
+		return err
+	}
+	return io.EOF
+}
+
 // listMerged will list across all sets and return a merged results stream.
 // The result channel is closed when no more results are expected.
 func (z *erasureServerPools) listMerged(ctx context.Context, o listPathOptions, results chan<- metaCacheEntry) error {

cmd/object_api_suite_test.go

@@ -849,6 +849,45 @@ func testListObjectsTestsForNonExistentBucket(obj ObjectLayer, instanceType stri
 	}
 }
 
+// Wrapper for calling testListObjectsShortcutsForNonExistentBucket for both
+// single-drive and multi-drive erasure setups.
+func TestListObjectsShortcutsForNonExistentBucket(t *testing.T) {
+	ExecObjectLayerTest(t, testListObjectsShortcutsForNonExistentBucket)
+}
+
+// Tests validate that storage-bypassing list shortcuts do not mask a missing
+// bucket as an empty result. The regular prefix case is a control for the
+// storage-backed listing path.
+func testListObjectsShortcutsForNonExistentBucket(obj ObjectLayer, instanceType string, t TestErrHandler) {
+	testCases := []struct {
+		name    string
+		prefix  string
+		marker  string
+		maxKeys int
+	}{
+		{name: "slash-prefixed prefix", prefix: "/", maxKeys: 1000},
+		{name: "zero limit", prefix: "obj", maxKeys: 0},
+		{name: "marker outside prefix", prefix: "a", marker: "b", maxKeys: 1000},
+		{name: "regular prefix", prefix: "foo/bar", maxKeys: 1000},
+	}
+	for _, tc := range testCases {
+		_, err := obj.ListObjects(context.Background(), "bucket", tc.prefix, tc.marker, "", tc.maxKeys)
+		if !isErrBucketNotFound(err) {
+			t.Errorf("%s: ListObjects %s: expected BucketNotFound, got %v", instanceType, tc.name, err)
+		}
+
+		_, err = obj.ListObjectsV2(context.Background(), "bucket", tc.prefix, "", "", tc.maxKeys, false, tc.marker)
+		if !isErrBucketNotFound(err) {
+			t.Errorf("%s: ListObjectsV2 %s: expected BucketNotFound, got %v", instanceType, tc.name, err)
+		}
+
+		_, err = obj.ListObjectVersions(context.Background(), "bucket", tc.prefix, tc.marker, "", "", tc.maxKeys)
+		if !isErrBucketNotFound(err) {
+			t.Errorf("%s: ListObjectVersions %s: expected BucketNotFound, got %v", instanceType, tc.name, err)
+		}
+	}
+}
+
 // Wrapper for calling testNonExistentObjectInBucket for both Erasure and FS.
 func TestNonExistentObjectInBucket(t *testing.T) {
 	ExecObjectLayerTest(t, testNonExistentObjectInBucket)