Repository: apigee/registry (go)

Issue:
updateSpec does not validate file when mime type is +gzip in the same way as createSpec does

Pull request:
Fix gzip validation for UpdateApiSpec
Fixes #626 by applying updates to spec contents only _after_ updating every other field on the model. This guarantees the `mime_type` is up-to-date when we check whether to decompress the contents before recomputing `size_bytes` and `hash`. 

See https://github.com/apigee/registry/issues/626#issuecomment-1165796491 for a full description of the bug and solution.

Patch:
diff --git a/server/registry/actions_specs.go b/server/registry/actions_specs.go
index 15dedd3..58bc57c 100644
--- a/server/registry/actions_specs.go
+++ b/server/registry/actions_specs.go
@@ -302,7 +302,7 @@ func (s *RegistryServer) UpdateApiSpec(ctx context.Context, req *rpc.UpdateApiSp
 	// Apply the update to the spec - possibly changing the revision ID.
 	maskExpansion := models.ExpandMask(req.GetApiSpec(), req.GetUpdateMask())
 	if err := spec.Update(req.GetApiSpec(), maskExpansion); err != nil {
-		return nil, status.Error(codes.Internal, err.Error())
+		return nil, err
 	}
 
 	// Save the updated/current spec. This creates a new revision or updates the previous one.
diff --git a/server/registry/internal/storage/models/spec.go b/server/registry/internal/storage/models/spec.go
index 7d5ee00..9f6af01 100644
--- a/server/registry/internal/storage/models/spec.go
+++ b/server/registry/internal/storage/models/spec.go
@@ -163,6 +163,7 @@ func (s *Spec) BasicMessage(name string) (message *rpc.ApiSpec, err error) {
 // Update modifies a spec using the contents of a message.
 func (s *Spec) Update(message *rpc.ApiSpec, mask *fieldmaskpb.FieldMask) error {
 	s.RevisionUpdateTime = time.Now().Round(time.Microsecond)
+	var hasContents bool
 	for _, field := range mask.Paths {
 		switch field {
 		case "filename":
@@ -170,16 +171,7 @@ func (s *Spec) Update(message *rpc.ApiSpec, mask *fieldmaskpb.FieldMask) error {
 		case "description":
 			s.Description = message.GetDescription()
 		case "contents":
-			contents := message.GetContents()
-			// if contents are gzipped, uncompress before computing size and hash.
-			if strings.Contains(s.MimeType, "+gzip") && len(contents) > 0 {
-				var err error
-				contents, err = GUnzippedBytes(contents)
-				if err != nil {
-					return status.Error(codes.InvalidArgument, err.Error())
-				}
-			}
-			s.updateContents(contents)
+			hasContents = true
 		case "mime_type":
 			s.MimeType = message.GetMimeType()
 		case "source_uri":
@@ -187,20 +179,35 @@ func (s *Spec) Update(message *rpc.ApiSpec, mask *fieldmaskpb.FieldMask) error {
 		case "labels":
 			var err error
 			if s.Labels, err = bytesForMap(message.GetLabels()); err != nil {
-				return err
+				return status.Error(codes.Internal, err.Error())
 			}
 		case "annotations":
 			var err error
 			if s.Annotations, err = bytesForMap(message.GetAnnotations()); err != nil {
-				return err
+				return status.Error(codes.Internal, err.Error())
 			}
 		}
 	}
 
+	// Content updates depend on the current MIME type of the spec, so they
+	// should only happen after we update the MIME type field.
+	if hasContents {
+		return s.updateContents(message.GetContents())
+	}
+
 	return nil
 }
 
-func (s *Spec) updateContents(contents []byte) {
+func (s *Spec) updateContents(contents []byte) error {
+	// Compute size and hash using uncompressed spec.
+	if strings.Contains(s.MimeType, "+gzip") && len(contents) > 0 {
+		var err error
+		contents, err = GUnzippedBytes(contents)
+		if err != nil {
+			return status.Error(codes.InvalidArgument, err.Error())
+		}
+	}
+
 	if hash := hashForBytes(contents); hash != s.Hash {
 		s.Hash = hash
 		s.RevisionID = newRevisionID()
@@ -210,6 +217,8 @@ func (s *Spec) updateContents(contents []byte) {
 		s.RevisionCreateTime = now
 		s.RevisionUpdateTime = now
 	}
+
+	return nil
 }
 
 // LabelsMap returns a map representation of stored labels.


Test patch:
diff --git a/server/registry/actions_specs_test.go b/server/registry/actions_specs_test.go
index 1aec060..a96739c 100644
--- a/server/registry/actions_specs_test.go
+++ b/server/registry/actions_specs_test.go
@@ -247,6 +247,19 @@ func TestCreateApiSpecResponseCodes(t *testing.T) {
 			},
 			want: codes.InvalidArgument,
 		},
+		{
+			desc: "invalid contents for gzip mime type",
+			seed: &rpc.ApiVersion{Name: "projects/my-project/locations/global/apis/my-api/versions/v1"},
+			req: &rpc.CreateApiSpecRequest{
+				Parent:    "projects/my-project/locations/global/apis/my-api/versions/v1",
+				ApiSpecId: "my-spec",
+				ApiSpec: &rpc.ApiSpec{
+					MimeType: "application/x.openapi+gzip",
+					Contents: []byte("these contents are not gzipped"),
+				},
+			},
+			want: codes.InvalidArgument,
+		},
 	}
 
 	for _, test := range tests {
@@ -1284,6 +1297,23 @@ func TestUpdateApiSpecResponseCodes(t *testing.T) {
 			},
 			want: codes.InvalidArgument,
 		},
+		{
+			desc: "invalid contents for gzip mime type",
+			seed: &rpc.ApiSpec{
+				Name:     "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+				MimeType: "application/x.openapi",
+				Contents: []byte("these contents are not gzipped"),
+			},
+			req: &rpc.UpdateApiSpecRequest{
+				ApiSpec: &rpc.ApiSpec{
+					Name:     "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+					MimeType: "application/x.openapi+gzip",
+					Contents: []byte("these contents are not gzipped"),
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"contents", "mime_type"}},
+			},
+			want: codes.InvalidArgument,
+		},
 	}
 
 	for _, test := range tests {