CASSANDRA-21389 trunk Harden snapshot names on server side

src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java

@@ -23,6 +23,7 @@
 import java.util.Collections;
 import java.util.Map;
 import java.util.function.Predicate;
+import java.util.regex.Pattern;
 
 import com.google.common.util.concurrent.RateLimiter;
 
@@ -33,13 +34,21 @@
 import org.apache.cassandra.config.DurationSpec;
 import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.schema.SchemaConstants;
 
 import static java.lang.String.format;
+import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
 
 public class SnapshotOptions
 {
     public static final String SKIP_FLUSH = "skipFlush";
     public static final String TTL = "ttl";
+
+    // AWS S3 "Safe characters" for object keys:
+    //   0-9  a-z  A-Z  !  -  _  .  *  '  (  )
+    // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
+    // Hyphen is placed last in the character class, so it stays literal and never becomes a range operator.
+    private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9!_.*'()-]+");
     public final SnapshotType type;
     public final String tag;
     public final DurationSpec.IntSecondsBound ttl;
@@ -88,7 +97,7 @@ public static SnapshotOptions userSnapshot(String tag, Map<String, String> optio
         return builder.build();
     }
 
-    public String getSnapshotName(Instant creationTime)
+    public static String getSnapshotName(SnapshotType type, String tag, Instant creationTime)
     {
         // Diagnostic snapshots have very specific naming convention hence we are keeping it.
         // Repair snapshots rely on snapshots having name of their repair session ids
@@ -189,10 +198,58 @@ public Builder rateLimiter(RateLimiter rateLimiter)
         }
 
         public SnapshotOptions build()
+        {
+            validateTag(tag);
+            validateTTL(ephemeral, ttl);
+
+            if (rateLimiter == null)
+                rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
+
+            return new SnapshotOptions(this);
+        }
+
+        private void validateTag(String tag)
         {
             if (tag == null || tag.isEmpty())
                 throw new RuntimeException("You must supply a snapshot name.");
 
+            // Pre-generate snapshot name for the sake of the validation.
+            // getSnapshotName logic does not return raw "tag" as snapshot name every time,
+            // it e.g. prepends timestamp and type for system snapshots, and we need to validate it as a whole.
+            // If, for example, tag would be less than max allowed FILENAME_LENGTH,
+            // we might in fact produce a snapshot name longer than FILENAME_LENGTH if we prepended a timestamp to it.
+            String resolvedSnapshotname = SnapshotOptions.getSnapshotName(type, tag, Instant.now());
+
+            // the length of valid snapshot name has to be less than or equal to FILENAME_LEGTH - that is 255 -
+            // we are following the max length as it is in SchemaConstants for table name.
+            if (resolvedSnapshotname.length() > SchemaConstants.FILENAME_LENGTH)
+            {
+                throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " +
+                                                          "resolved snapshot name (got %d characters for \"%s\")",
+                                                          FILENAME_LENGTH, resolvedSnapshotname.length(), resolvedSnapshotname));
+            }
+
+            // Allowed characters follow the AWS S3 "Safe characters" set documented at
+            // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines :
+            //   0-9  a-z  A-Z  !  -  _  .  *  '  (  )
+            // The path separator '/' is intentionally excluded,
+            // which is what blocks traversal attempts such as "../../mysnapshot"
+            if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches())
+            {
+                throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname);
+            }
+
+            // Because '.' is part of the safe set, the names "." and ".." would pass the charset check
+            // but resolve to the snapshots/ directory itself or its parent (the live table directory)
+            // when used as a path component. Reject them explicitly.
+            if (resolvedSnapshotname.equals(".") || resolvedSnapshotname.equals(".."))
+            {
+                throw new IllegalArgumentException("Snapshot name '" + resolvedSnapshotname + "' is reserved");
+            }
+        }
+
+        private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl)
+        {
             if (ttl != null)
             {
                 int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt();
@@ -202,11 +259,6 @@ public SnapshotOptions build()
 
             if (ephemeral && ttl != null)
                 throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag));
-
-            if (rateLimiter == null)
-                rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
-
-            return new SnapshotOptions(this);
         }
     }
 

src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java

@@ -88,7 +88,7 @@ public Map<ColumnFamilyStore, TableSnapshot> getSnapshotsToCreate()
         else
             creationTime = options.creationTime;
 
-        snapshotName = options.getSnapshotName(creationTime);
+        snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime);
 
         Set<ColumnFamilyStore> entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs);
 

test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java

@@ -26,23 +26,89 @@
 
 import org.junit.Test;
 
+import org.apache.cassandra.io.util.File;
+
 import static java.lang.String.format;
+import static org.apache.cassandra.service.snapshot.SnapshotType.USER;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.Assert.assertEquals;
 
 public class SnapshotOptionsTest
 {
+    @Test
+    public void testSnapshotNameValidation()
+    {
+        // Previously-allowed alphanumerics, '-' and '_' must still be accepted.
+        validate("atag", USER);
+        validate("a-tag", USER);
+        validate("a_tag", USER);
+        validate("a_tag" + Instant.now().toEpochMilli(), USER);
+        validate("a_tag_1and_something2-more", USER);
+        validate("a".repeat(255), USER);
+
+        // AWS S3 "Safe characters" newly accepted by the relaxed allowlist:
+        //   !  .  *  '  (  )
+        // See https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
+        validate("snap.2026-05-20", USER);
+        validate("important!", USER);
+        validate("backup*", USER);
+        validate("o'snap", USER);
+        validate("snap(1)", USER);
+        validate("!._-*'()", USER);
+        // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
+        validate("a..tag", USER);
+
+        assertThatThrownBy(() -> validate("a".repeat(256), USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name must not be more than 255 characters long for " +
+                    "resolved snapshot name (got 256 characters for \"" + "a".repeat(256) + "\")");
+
+        // this would append timestamp + type which would violate < 255 when tag is 250 chars long only
+        assertThatThrownBy(() -> validate("a".repeat(250), SnapshotType.UPGRADE))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Snapshot name must not be more than 255 characters long");
+
+        // '/' is not in the S3-safe set; this is what kills traversal attempts like "../../mysnapshot".
+        assertThatThrownBy(() -> validate('a' + File.pathSeparator() + "tag", USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name contains illegal characters: a/tag");
+
+        // Other characters outside the S3-safe set must still be rejected.
+        assertThatThrownBy(() -> validate("a tag", USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name contains illegal characters: a tag");
+        assertThatThrownBy(() -> validate("a:tag", USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name contains illegal characters: a:tag");
+
+        // "." and ".." pass the charset check but resolve to the snapshots/ dir itself
+        // and its parent (the live table dir) respectively, so they must be rejected as reserved.
+        assertThatThrownBy(() -> validate(".", USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name '.' is reserved");
+
+        assertThatThrownBy(() -> validate("..", USER))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Snapshot name '..' is reserved");
+    }
+
+    private void validate(String tag, SnapshotType type)
+    {
+        new SnapshotOptions.Builder(tag, type, s -> true, "ks.tb").rateLimiter(RateLimiter.create(1)).build();
+    }
+
     @Test
     public void testSnapshotName()
     {
-        List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER);
+        List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER);
 
         for (SnapshotType type : sameNameTypes)
         {
             SnapshotOptions options = SnapshotOptions.systemSnapshot("a_name", type, "ks.tb")
                                                      .rateLimiter(RateLimiter.create(5))
                                                      .build();
 
-            String snapshotName = options.getSnapshotName(Instant.now());
+            String snapshotName = SnapshotOptions.getSnapshotName(type, options.tag, Instant.now());
             assertEquals("a_name", snapshotName);
         }
 
@@ -57,7 +123,7 @@ public void testSnapshotName()
 
             Instant now = Instant.now();
 
-            String snapshotName = options.getSnapshotName(now);
+            String snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, now);
 
             assertEquals(format("%d-%s-%s", now.toEpochMilli(), type.label, "a_name"), snapshotName);
         }