fix: restrict S3 deletion to exact keys and descendants

src/core/storage/S3.ts

@@ -81,38 +81,36 @@ export default class S3Storage extends StorageBase {
   async rm(repoId: string, dir: string = ""): Promise<void> {
     if (!config.S3_BUCKET) throw new Error("S3_BUCKET not set");
     this.assertSafePath(dir);
-    const data = await this.client(200000).listObjectsV2({
+    const prefix = join(this.repoPath(repoId), dir).replace(/\/$/, "");
-      Bucket: config.S3_BUCKET,
+    const client = this.client(200000);
-      Prefix: join(this.repoPath(repoId), dir),
+    let continuationToken: string | undefined;
-      MaxKeys: 100,
+    do {
-    });
+      const data = await client.listObjectsV2({
-
+        Bucket: config.S3_BUCKET,
-    const params = {
+        Prefix: prefix,
-      Bucket: config.S3_BUCKET,
+        MaxKeys: 1000,
-      Delete: { Objects: new Array<{ Key: string }>() },
+        ContinuationToken: continuationToken,
-    };
-
-    data.Contents?.forEach(function (content) {
-      if (content.Key) {
-        params.Delete.Objects.push({ Key: content.Key });
-      }
-    });
-
-    if (params.Delete.Objects.length == 0) {
-      // nothing to remove
-      return;
-    }
-    const result = await this.client(200000).deleteObjects(params);
-    if (result.Errors?.length) {
-      throw new AnonymousError("storage_delete_failed", {
-        httpStatus: 502,
-        object: result.Errors,
       });
-    }
+      const objects = (data.Contents || [])
-
+        .filter(({ Key }) => Key === prefix || Key?.startsWith(prefix + "/"))
-    if (data.IsTruncated) {
+        .map(({ Key }) => ({ Key: Key! }));
-      await this.rm(repoId, dir);
+      if (objects.length) {
-    }
+        const result = await client.deleteObjects({
+          Bucket: config.S3_BUCKET,
+          Delete: { Objects: objects },
+        });
+        if (result.Errors?.length) {
+          throw new AnonymousError("storage_delete_failed", {
+            httpStatus: 502,
+            object: result.Errors,
+          });
+        }
+      }
+      continuationToken = data.IsTruncated ? data.NextContinuationToken : undefined;
+      if (data.IsTruncated && !continuationToken) {
+        throw new Error("S3 returned a truncated listing without a continuation token");
+      }
+    } while (continuationToken);
   }
 
   /** @override */

test/production-regressions.test.js

@@ -85,6 +85,20 @@ describe("production regressions", function () {
     const storage = new S3(); storage.client = () => client; storage.repoPath = () => "repo";
     return storage;
   }
+  it("deletes only exact S3 keys and descendants across pages", async function () {
+    const deleted = []; const tokens = [];
+    const storage = s3({
+      listObjectsV2: async ({ ContinuationToken }) => {
+        tokens.push(ContinuationToken);
+        return ContinuationToken ? { Contents: [{ Key: "repo/data/a" }, { Key: "repo/database" }] } :
+          { Contents: [{ Key: "repo/data.csv" }, { Key: "repo/data" }], IsTruncated: true, NextContinuationToken: "next" };
+      },
+      deleteObjects: async ({ Delete }) => { deleted.push(...Delete.Objects.map(x => x.Key)); return {}; },
+    });
+    await storage.rm("repo", "data");
+    expect(tokens).to.deep.equal([undefined, "next"]);
+    expect(deleted).to.deep.equal(["repo/data", "repo/data/a"]);
+  });
   it("rejects S3 per-object deletion failures", async function () {
     const storage = s3({
       listObjectsV2: async () => ({ Contents: [{ Key: "repo/data" }] }),