diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index dcd777fc03a1..12aea18917f5 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java @@ -238,6 +238,11 @@ public enum CassandraRelevantProperties /** minimum allowed TTL for snapshots */ SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"), + /** + * Validates names of to-be-taken snapshots, defaults to false. + */ + SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "false"), + /** what class to use for mbean registeration */ MBEAN_REGISTRATION_CLASS("org.apache.cassandra.mbean_registration_class"), diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index da859a548c1d..9d1e251e1b08 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -79,6 +79,7 @@ import org.apache.cassandra.cache.RowCacheSentinel; import org.apache.cassandra.concurrent.ExecutorPlus; import org.apache.cassandra.concurrent.FutureTask; +import org.apache.cassandra.config.CassandraRelevantProperties; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.config.DurationSpec; import org.apache.cassandra.db.commitlog.CommitLog; @@ -174,9 +175,11 @@ import org.apache.cassandra.utils.concurrent.UncheckedInterruptedException; import static com.google.common.base.Throwables.propagate; +import static java.lang.String.format; import static org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory; import static org.apache.cassandra.config.DatabaseDescriptor.getFlushWriters; import static org.apache.cassandra.db.commitlog.CommitLogPosition.NONE; +import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; import static org.apache.cassandra.utils.Clock.Global.currentTimeMillis; import static org.apache.cassandra.utils.Clock.Global.nanoTime; import static org.apache.cassandra.utils.FBUtilities.now; @@ -2011,6 +2014,8 @@ public TableSnapshot snapshotWithoutMemtable(String snapshotName, Predicate 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, snapshotName.length(), snapshotName)); + } + + // Allowed characters are a conservative subset of the AWS S3 "Safe characters" set + // (https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines): + // 0-9 a-z A-Z - _ . + // The remaining S3-safe characters (! * ' ( )) are intentionally excluded as they are + // shell-significant and error-prone in paths, and the path separator '/' is excluded too, + // which is what blocks traversal attempts such as "../../mysnapshot" + if (!Pattern.compile("[a-zA-Z0-9_.-]+").matcher(snapshotName).matches()) + { + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + snapshotName); + } + } + protected TableSnapshot createSnapshot(String tag, boolean ephemeral, DurationSpec.IntSecondsBound ttl, Set sstables, Instant creationTime) { Set snapshotDirs = sstables.stream() .map(s -> Directories.getSnapshotDirectory(s.descriptor, tag).toAbsolute()) diff --git a/test/unit/org/apache/cassandra/db/SnapshotTest.java b/test/unit/org/apache/cassandra/db/SnapshotTest.java index aa726ec3e0a1..8682881f5461 100644 --- a/test/unit/org/apache/cassandra/db/SnapshotTest.java +++ b/test/unit/org/apache/cassandra/db/SnapshotTest.java @@ -21,20 +21,32 @@ import java.nio.file.Files; import java.nio.file.StandardOpenOption; +import org.junit.Before; import org.junit.Test; +import org.apache.cassandra.config.CassandraRelevantProperties; import org.apache.cassandra.cql3.CQLTester; +import org.apache.cassandra.distributed.shared.WithProperties; import org.apache.cassandra.io.sstable.Component; import org.apache.cassandra.io.sstable.format.SSTableReader; import org.apache.cassandra.io.util.File; +import org.apache.cassandra.schema.SchemaConstants; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public class SnapshotTest extends CQLTester { - @Test - public void testEmptyTOC() throws Throwable + @Before + public void setUpTable() throws Throwable { createTable("create table %s (id int primary key, k int)"); execute("insert into %s (id, k) values (1,1)"); + } + + @Test + public void testEmptyTOC() throws Throwable + { getCurrentColumnFamilyStore().forceBlockingFlush(ColumnFamilyStore.FlushReason.UNIT_TESTS); for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables()) { @@ -43,4 +55,101 @@ public void testEmptyTOC() throws Throwable } getCurrentColumnFamilyStore().snapshot("hello"); } + + @Test + public void testSnapshotNameValidation() + { + ColumnFamilyStore cfs = getCurrentColumnFamilyStore(); + String sep = File.pathSeparator(); + + try (WithProperties p = new WithProperties()) + { + p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true); + + // Previously-allowed alphanumerics, '-' and '_' must still be accepted. + assertThatCode(() -> cfs.snapshot("atag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a-tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a_tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a_tag_1and_something2-more")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot(repeat('a', SchemaConstants.FILENAME_LENGTH))).doesNotThrowAnyException(); + + // Only alphanumerics, '-', '_' and '.' are accepted. + assertThatCode(() -> cfs.snapshot("snap.2026-05-20")).doesNotThrowAnyException(); + // Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory. + assertThatCode(() -> cfs.snapshot("a..tag")).doesNotThrowAnyException(); + + String tooLong = repeat('a', SchemaConstants.FILENAME_LENGTH + 1); + assertThatThrownBy(() -> cfs.snapshot(tooLong)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name must not be more than 255 characters long for " + + "resolved snapshot name (got 256 characters for \"" + tooLong + "\")"); + + // '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot". + assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + + // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed. + assertThatThrownBy(() -> cfs.snapshot("important!")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: important!"); + assertThatThrownBy(() -> cfs.snapshot("backup*")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: backup*"); + assertThatThrownBy(() -> cfs.snapshot("o'snap")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: o'snap"); + assertThatThrownBy(() -> cfs.snapshot("snap(1)")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: snap(1)"); + + // Other characters outside the allowed set must still be rejected. + assertThatThrownBy(() -> cfs.snapshot("a tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: a tag"); + assertThatThrownBy(() -> cfs.snapshot("a:tag")) + .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(() -> cfs.snapshot(".")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + assertThatThrownBy(() -> cfs.snapshot("..")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '..' is reserved"); + } + + try (WithProperties p = new WithProperties()) + { + p.set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false); + + // The character check is bypassed entirely: space, ':', and the now-disallowed + // shell-significant characters (! * ' ( )) are all accepted. + assertThatCode(() -> cfs.snapshot("a tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("a:tag")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("important!")).doesNotThrowAnyException(); + assertThatCode(() -> cfs.snapshot("snap(1)")).doesNotThrowAnyException(); + + // Path separator and "." / ".." rejections are unconditional — they guard against + // traversal regardless of the toggle. + assertThatThrownBy(() -> cfs.snapshot("a" + sep + "tag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + assertThatThrownBy(() -> cfs.snapshot(".")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '.' is reserved"); + assertThatThrownBy(() -> cfs.snapshot("..")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name '..' is reserved"); + } + } + + private static String repeat(char c, int times) + { + char[] chars = new char[times]; + java.util.Arrays.fill(chars, c); + return new String(chars); + } }