fix: Improved validation for the CreatePublicShare method

changelog/unreleased/fix-createpublicshare-validation.md

@@ -0,0 +1,14 @@
+Bugfix: Improved validation for the CreatePublicShare method
+
+CreatePublicShare trusted the client-supplied resource info instead of the
+one it had just verified via Stat, and never checked Stat's own status
+code. Combined with a missing nil-check on write, this let a public share
+be persisted with a nil resource_id, which crashes ListPublicShares with a
+nil-pointer panic for the whole tenant on every subsequent read.
+
+CreatePublicShare now propagates a non-OK Stat status instead of falling
+through with a nil resource info, persists the resource id verified by
+Stat instead of the client-supplied one, and the json public-share
+manager rejects a nil/empty resource id before persisting.
+
+https://github.com/owncloud/reva/pull/736

internal/grpc/services/publicshareprovider/publicshareprovider.go

@@ -227,6 +227,11 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS
 			Status: status.NewInternal(ctx, "failed to stat resource to share"),
 		}, err
 	}
+	if sRes.Status.Code != rpc.Code_CODE_OK {
+		return &link.CreatePublicShareResponse{
+			Status: sRes.GetStatus(),
+		}, nil
+	}
 
 	// all users can create internal links
 	if !isInternalLink {
@@ -333,7 +338,11 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS
 
 	user := ctxpkg.ContextMustGetUser(ctx)
 	res := &link.CreatePublicShareResponse{}
-	share, err := s.sm.CreatePublicShare(ctx, user, req.GetResourceInfo(), req.GetGrant())
+	resourceInfo := req.GetResourceInfo()
+	if resourceInfo != nil {
+		resourceInfo.Id = sRes.GetInfo().GetId()
+	}
+	share, err := s.sm.CreatePublicShare(ctx, user, resourceInfo, req.GetGrant())
 	switch {
 	case err != nil:
 		log.Error().Err(err).Interface("request", req).Msg("could not write public share")

internal/grpc/services/publicshareprovider/publicshareprovider_test.go

@@ -213,6 +213,81 @@ var _ = Describe("PublicShareProvider", func() {
 			Expect(res.GetStatus().GetCode()).To(Equal(rpc.Code_CODE_OK))
 			Expect(res.GetShare()).To(Equal(createdLink))
 		})
+		It("persists the stat-verified resource id, not the client-supplied one", func() {
+			statResourceResponse.Info.Id = &providerpb.ResourceId{
+				StorageId: "verified-storage-id",
+				SpaceId:   "verified-space-id",
+				OpaqueId:  "verified-opaque-id",
+			}
+
+			manager.
+				EXPECT().
+				CreatePublicShare(
+					mock.Anything,
+					mock.Anything,
+					mock.MatchedBy(func(rInfo *providerpb.ResourceInfo) bool {
+						return rInfo.GetId().GetStorageId() == "verified-storage-id" &&
+							rInfo.GetId().GetSpaceId() == "verified-space-id" &&
+							rInfo.GetId().GetOpaqueId() == "verified-opaque-id"
+					}),
+					mock.Anything,
+				).
+				Return(createdLink, nil)
+
+			req := &link.CreatePublicShareRequest{
+				ResourceInfo: &providerpb.ResourceInfo{
+					Owner: &userpb.UserId{
+						OpaqueId: "alice",
+					},
+					Path: "./NewFolder/file.txt",
+				},
+				Grant: &link.Grant{
+					Permissions: &link.PublicSharePermissions{
+						Permissions: linkPermissions,
+					},
+					Password: "SecretPassw0rd!",
+				},
+			}
+
+			res, err := provider.CreatePublicShare(ctx, req)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(res.GetStatus().GetCode()).To(Equal(rpc.Code_CODE_OK))
+		})
+		It("fails cleanly when stat reports a non-OK status instead of an error", func() {
+			gatewayClient.
+				EXPECT().
+				Stat(mock.Anything, mock.Anything).
+				Unset()
+			gatewayClient.
+				EXPECT().
+				CheckPermission(mock.Anything, mock.Anything).
+				Unset()
+			statResourceResponse.Status = status.NewNotFound(ctx, "not found")
+			statResourceResponse.Info = nil
+			gatewayClient.
+				EXPECT().
+				Stat(mock.Anything, mock.Anything).
+				Return(statResourceResponse, nil)
+
+			req := &link.CreatePublicShareRequest{
+				ResourceInfo: &providerpb.ResourceInfo{
+					Owner: &userpb.UserId{
+						OpaqueId: "alice",
+					},
+					Path: "./NewFolder/file.txt",
+				},
+				Grant: &link.Grant{
+					Permissions: &link.PublicSharePermissions{
+						Permissions: linkPermissions,
+					},
+					Password: "SecretPassw0rd!",
+				},
+			}
+
+			res, err := provider.CreatePublicShare(ctx, req)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(res.GetStatus().GetCode()).To(Equal(rpc.Code_CODE_NOT_FOUND))
+		})
 		It("has no user permission to create public share", func() {
 			gatewayClient.
 				EXPECT().

pkg/publicshare/manager/json/json.go

@@ -261,6 +261,10 @@ func (m *manager) Load(ctx context.Context, shareChan <-chan *publicshare.WithPa
 
 // CreatePublicShare adds a new entry to manager.shares
 func (m *manager) CreatePublicShare(ctx context.Context, u *user.User, rInfo *provider.ResourceInfo, g *link.Grant) (*link.PublicShare, error) {
+	if rInfo.GetId() == nil || rInfo.GetId().GetStorageId() == "" {
+		return nil, errtypes.BadRequest("resource id is required to create a public share")
+	}
+
 	id := &link.PublicShareId{
 		OpaqueId: utils.RandString(15),
 	}

pkg/publicshare/manager/json/json_test.go

@@ -704,6 +704,22 @@ var _ = Describe("Json", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(ps).ToNot(BeNil())
 			})
+
+			It("rejects a resource info with a nil resource id instead of persisting it", func() {
+				noID := &providerv1beta1.ResourceInfo{
+					ArbitraryMetadata: &providerv1beta1.ArbitraryMetadata{
+						Metadata: map[string]string{"name": "publicshare"},
+					},
+				}
+
+				ps, err := m.CreatePublicShare(ctx, user1, noID, grant)
+				Expect(err).To(HaveOccurred())
+				Expect(ps).To(BeNil())
+
+				shares, err := m.ListPublicShares(ctx, user1, []*link.ListPublicSharesRequest_Filter{}, false)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(shares).To(BeEmpty())
+			})
 		})
 
 		Describe("PublicShares", func() {