From 360402d2fa39b3c823bc15141ab1e99b0cc83be7 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Thu, 27 Jun 2024 14:26:23 -0400
Subject: [PATCH] 21701: Reject requests with unused non-empty manifest_text.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 .../methods/collections.html.textile.liquid   | 11 ++++++++
 lib/controller/localdb/collection.go          | 13 ++++++++++
 lib/controller/localdb/collection_test.go     | 25 +++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/doc/api/methods/collections.html.textile.liquid b/doc/api/methods/collections.html.textile.liquid
index 58b3403b27..da70afbf66 100644
--- a/doc/api/methods/collections.html.textile.liquid
+++ b/doc/api/methods/collections.html.textile.liquid
@@ -166,6 +166,17 @@ A target path with a non-empty source cannot be the ancestor of another target p
 }
 </pre></notextile>
 
+It is an error to supply a non-empty @manifest_text@ that is unused, i.e., the @replace_files@ argument does not contain any values beginning with @"manifest_text/"@. For example, the following request is invalid:
+
+<notextile><pre>
+"replace_files": {
+  "/foo": "current/bar"
+},
+"collection": {
+  "manifest_text": ". acbd18db4cc2f85cedef654fccc4a4d8+3+A82740cd577ff5745925af5780de5992cbb25d937@668efec4 0:3:new_file.txt\n"
+}
+</pre></notextile>
+
 h2. Methods
 
 See "Common resource methods":{{site.baseurl}}/api/methods.html for more information about @create@, @delete@, @get@, @list@, and @update@.
diff --git a/lib/controller/localdb/collection.go b/lib/controller/localdb/collection.go
index a661c30560..1a4e2c692f 100644
--- a/lib/controller/localdb/collection.go
+++ b/lib/controller/localdb/collection.go
@@ -153,7 +153,20 @@ func (conn *Conn) applyReplaceFilesOption(ctx context.Context, fromUUID string,
 	if len(replaceFiles) == 0 {
 		return attrs, nil
 	}
+
 	providedManifestText, _ := attrs["manifest_text"].(string)
+	if providedManifestText != "" {
+		used := false
+		for _, src := range replaceFiles {
+			if strings.HasPrefix(src, "manifest_text/") {
+				used = true
+				break
+			}
+		}
+		if !used {
+			return nil, httpserver.Errorf(http.StatusBadRequest, "invalid request: attrs['manifest_text'] was provided, but would not be used because it is not referenced by any 'replace_files' entry")
+		}
+	}
 
 	// Load the current collection (if any) and set up an
 	// in-memory filesystem.
diff --git a/lib/controller/localdb/collection_test.go b/lib/controller/localdb/collection_test.go
index fd373c9fce..2818284920 100644
--- a/lib/controller/localdb/collection_test.go
+++ b/lib/controller/localdb/collection_test.go
@@ -410,6 +410,31 @@ func (s *replaceFilesSuite) TestConcurrentCopyFromProvidedManifestText(c *check.
 	s.expectFileSizes(c, final, expectFileSizes)
 }
 
+func (s *replaceFilesSuite) TestUnusedManifestText_Create(c *check.C) {
+	blockLocator := strings.Split(s.tmp.ManifestText, " ")[1]
+	_, err := s.localdb.CollectionCreate(s.userctx, arvados.CreateOptions{
+		Attrs: map[string]interface{}{
+			"manifest_text": ". " + blockLocator + " 0:3:foo\n",
+		},
+		ReplaceFiles: map[string]string{
+			"/foo.txt": "",
+		}})
+	c.Check(err, check.ErrorMatches, `.*manifest_text.*would not be used.*`)
+}
+
+func (s *replaceFilesSuite) TestUnusedManifestText_Update(c *check.C) {
+	blockLocator := strings.Split(s.tmp.ManifestText, " ")[1]
+	_, err := s.localdb.CollectionUpdate(s.userctx, arvados.UpdateOptions{
+		UUID: s.tmp.UUID,
+		Attrs: map[string]interface{}{
+			"manifest_text": ". " + blockLocator + " 0:3:foo\n",
+		},
+		ReplaceFiles: map[string]string{
+			"/foo.txt": "",
+		}})
+	c.Check(err, check.ErrorMatches, `.*manifest_text.*would not be used.*`)
+}
+
 func (s *replaceFilesSuite) TestConcurrentRename(c *check.C) {
 	var wg sync.WaitGroup
 	var renamed atomic.Int32
-- 
2.30.2