From 1e63131ee2601594d5c51ab9b2d20a0bfcee912e Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Tue, 19 May 2026 13:34:29 +0200 Subject: [PATCH 1/5] Harden snapshot names on server side --- .../config/CassandraRelevantProperties.java | 4 + .../service/snapshot/SnapshotOptions.java | 72 +++++++++++-- .../service/snapshot/TakeSnapshotTask.java | 2 +- .../service/snapshot/SnapshotOptionsTest.java | 102 +++++++++++++++++- 4 files changed, 169 insertions(+), 11 deletions(-) diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index 4775a8425f9d..2be55fbe5c69 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java @@ -573,6 +573,10 @@ public enum CassandraRelevantProperties SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED("cassandra.snapshot.enrich.or.create.enabled", "true"), /** 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 true. + */ + SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "true"), SSL_ENABLE("ssl.enable"), SSL_STORAGE_PORT("cassandra.ssl_storage_port"), START_GOSSIP("cassandra.start_gossip", "true"), diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index ce35c9ca1bd2..e77c1cd962b3 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/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,22 @@ import org.apache.cassandra.config.DurationSpec; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.io.sstable.format.SSTableReader; +import org.apache.cassandra.io.util.File; +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"; + + // Follows 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 +98,7 @@ public static SnapshotOptions userSnapshot(String tag, Map 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 +199,63 @@ 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."); + throw new IllegalArgumentException("You must supply a snapshot name."); + + if (tag.contains(File.pathSeparator())) + { + throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator()); + } + + if (tag.equals(".") || tag.equals("..")) + { + throw new IllegalArgumentException("Snapshot name '" + tag + "' is reserved"); + } + if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean()) + return; + + // 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); + } + } + + private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl) + { if (ttl != null) { int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt(); @@ -202,11 +265,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); } } diff --git a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java index 236a3ab783aa..2c69309d0b79 100644 --- a/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java +++ b/src/java/org/apache/cassandra/service/snapshot/TakeSnapshotTask.java @@ -88,7 +88,7 @@ public Map getSnapshotsToCreate() else creationTime = options.creationTime; - snapshotName = options.getSnapshotName(creationTime); + snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime); Set entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs); diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index 30b94d32b1c5..f88b55703344 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -26,15 +26,111 @@ import org.junit.Test; +import org.apache.cassandra.config.CassandraRelevantProperties; +import org.apache.cassandra.distributed.shared.WithProperties; +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.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.Assert.assertEquals; public class SnapshotOptionsTest { + @Test + public void testSnapshotNameValidation() + { + String sep = File.pathSeparator(); + + try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true)) + { + // 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' + sep + "tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + + // 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"); + } + + try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false)) + { + // Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars. + assertThatCode(() -> validate("a tag", USER)).doesNotThrowAnyException(); + assertThatCode(() -> validate("a:tag", USER)).doesNotThrowAnyException(); + + assertThatCode(() -> validate("a".repeat(256), USER)).doesNotThrowAnyException(); + assertThatCode(() -> validate("a".repeat(250), SnapshotType.UPGRADE)).doesNotThrowAnyException(); + + // Path separator and "." / ".." rejections are unconditional — they guard against + // traversal regardless of the toggle. + assertThatThrownBy(() -> validate('a' + sep + "tag", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name cannot contain " + sep); + 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 sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER); + List sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER); for (SnapshotType type : sameNameTypes) { @@ -42,7 +138,7 @@ public void testSnapshotName() .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 +153,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); } From 0fc5787a38a9fcf877f375053bb937d71fb84d9b Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Fri, 29 May 2026 11:33:19 +0200 Subject: [PATCH 2/5] further restrict safe set --- .../service/snapshot/SnapshotOptions.java | 11 +++--- .../service/snapshot/SnapshotOptionsTest.java | 34 ++++++++++++------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index e77c1cd962b3..90436a967a84 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -49,7 +49,7 @@ public class SnapshotOptions // 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!_.*'()-]+"); + 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; @@ -243,10 +243,11 @@ private void validateTag(String tag) 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, + // 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 (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches()) { diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index f88b55703344..5ec1b29e36a0 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -45,23 +45,14 @@ public void testSnapshotNameValidation() try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true)) { - // Previously-allowed alphanumerics, '-' and '_' must still be accepted. + // Only alphanumerics, '-', '_' and '.' are 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); @@ -75,12 +66,26 @@ public void testSnapshotNameValidation() .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". + // '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot". assertThatThrownBy(() -> validate('a' + sep + "tag", USER)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name cannot contain " + sep); - // Other characters outside the S3-safe set must still be rejected. + // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed. + assertThatThrownBy(() -> validate("important!", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: important!"); + assertThatThrownBy(() -> validate("backup*", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: backup*"); + assertThatThrownBy(() -> validate("o'snap", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: o'snap"); + assertThatThrownBy(() -> validate("snap(1)", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Snapshot name contains illegal characters: snap(1)"); + + // Other characters outside the allowed set must still be rejected. assertThatThrownBy(() -> validate("a tag", USER)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name contains illegal characters: a tag"); @@ -101,9 +106,12 @@ public void testSnapshotNameValidation() try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false)) { - // Previously-rejected characters are now accepted: space, ':', and other non-S3-safe chars. + // The character check is bypassed entirely: space, ':', and the now-disallowed + // shell-significant characters (! * ' ( )) are all accepted. assertThatCode(() -> validate("a tag", USER)).doesNotThrowAnyException(); assertThatCode(() -> validate("a:tag", USER)).doesNotThrowAnyException(); + assertThatCode(() -> validate("important!", USER)).doesNotThrowAnyException(); + assertThatCode(() -> validate("snap(1)", USER)).doesNotThrowAnyException(); assertThatCode(() -> validate("a".repeat(256), USER)).doesNotThrowAnyException(); assertThatCode(() -> validate("a".repeat(250), SnapshotType.UPGRADE)).doesNotThrowAnyException(); From 2b84f00a473b3f245b17362614e1a0efd127c4bd Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Fri, 29 May 2026 17:03:48 +0200 Subject: [PATCH 3/5] review --- .../service/snapshot/SnapshotOptions.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index 90436a967a84..af3c96f52956 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -39,17 +39,18 @@ import static java.lang.String.format; import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH; +import static org.apache.cassandra.utils.FBUtilities.now; public class SnapshotOptions { public static final String SKIP_FLUSH = "skipFlush"; public static final String TTL = "ttl"; - // Follows 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 + // Conservative subset of the AWS S3 "Safe characters" set: 0-9 a-z A-Z - _ . + // See the validation site in validateTag for the full rationale on excluded characters. // 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; @@ -232,15 +233,15 @@ private void validateTag(String tag) // 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()); + String resolvedSnapshotName = SnapshotOptions.getSnapshotName(type, tag, 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) + 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)); + FILENAME_LENGTH, resolvedSnapshotName.length(), resolvedSnapshotName)); } // Allowed characters are a conservative subset of the AWS S3 "Safe characters" set @@ -249,9 +250,9 @@ private void validateTag(String tag) // 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 (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotname).matches()) + if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches()) { - throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotname); + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName); } } From 2267fc81521333b23876df445482391521899711 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Mon, 1 Jun 2026 16:25:37 +0200 Subject: [PATCH 4/5] Francisco review --- .../service/snapshot/SnapshotOptions.java | 6 ++++-- .../service/snapshot/SnapshotOptionsTest.java | 20 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index af3c96f52956..0808c2f9d719 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -250,9 +250,11 @@ private void validateTag(String tag) // 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 (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches()) + if (type == SnapshotType.USER && !SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches()) { - throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName); + throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName + ". " + + "Allowed characters must match the pattern: " + SAFE_SNAPSHOT_NAME.pattern() + + " with a maximum of length of " + FILENAME_LENGTH + " characters."); } } diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index 5ec1b29e36a0..46af4e944e57 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -74,24 +74,24 @@ public void testSnapshotNameValidation() // The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed. assertThatThrownBy(() -> validate("important!", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: important!"); + .hasMessageContaining("Snapshot name contains illegal characters: important!"); assertThatThrownBy(() -> validate("backup*", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: backup*"); + .hasMessageContaining("Snapshot name contains illegal characters: backup*"); assertThatThrownBy(() -> validate("o'snap", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: o'snap"); + .hasMessageContaining("Snapshot name contains illegal characters: o'snap"); assertThatThrownBy(() -> validate("snap(1)", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: snap(1)"); + .hasMessageContaining("Snapshot name contains illegal characters: snap(1)"); // Other characters outside the allowed set must still be rejected. assertThatThrownBy(() -> validate("a tag", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: a tag"); + .hasMessageContaining("Snapshot name contains illegal characters: a tag"); assertThatThrownBy(() -> validate("a:tag", USER)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Snapshot name contains illegal characters: a:tag"); + .hasMessageContaining("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. @@ -102,6 +102,14 @@ public void testSnapshotNameValidation() assertThatThrownBy(() -> validate("..", USER)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name '..' is reserved"); + + // snapshot name contains "+" which might be present in a Cassandra version + // the usage of "+" is forbidden for USER snapshots + validate("this_is_system_snapshot-7.0.0+abc123", SnapshotType.UPGRADE); + + assertThatThrownBy(() -> validate("this_is_snapshot-7.0.0+abc123", USER)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Snapshot name contains illegal characters: this_is_snapshot-7.0.0+abc123"); } try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false)) From abfd237dafc940f9ea01e4f794d05c0c49272104 Mon Sep 17 00:00:00 2001 From: Stefan Miklosovic Date: Wed, 3 Jun 2026 00:59:46 +0200 Subject: [PATCH 5/5] added + --- .../cassandra/service/snapshot/SnapshotOptions.java | 10 ++++++---- .../service/snapshot/SnapshotOptionsTest.java | 11 +++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java index 0808c2f9d719..96b268a9e3d7 100644 --- a/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java +++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotOptions.java @@ -46,10 +46,10 @@ public class SnapshotOptions public static final String SKIP_FLUSH = "skipFlush"; public static final String TTL = "ttl"; - // Conservative subset of the AWS S3 "Safe characters" set: 0-9 a-z A-Z - _ . - // See the validation site in validateTag for the full rationale on excluded characters. + // Allowed snapshot-name characters: 0-9 a-z A-Z - _ . + + // See the validation site in validateTag for the full rationale (an S3 safe-set subset, plus '+'). // 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_.-]+"); + private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9_.+-]+"); public final SnapshotType type; public final String tag; @@ -247,10 +247,12 @@ private void validateTag(String tag) // 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 - _ . + // plus '+', which is not an S3 "Safe character" but can legitimately appear in system + // snapshot names via version build metadata (e.g. an upgrade snapshot "-upgrade-5.0.4+build-..."). // 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 (type == SnapshotType.USER && !SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches()) + if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches()) { throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName + ". " + "Allowed characters must match the pattern: " + SAFE_SNAPSHOT_NAME.pattern() + diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java index 46af4e944e57..6d5459e52bda 100644 --- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java +++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotOptionsTest.java @@ -103,13 +103,12 @@ public void testSnapshotNameValidation() .isInstanceOf(IllegalArgumentException.class) .hasMessage("Snapshot name '..' is reserved"); - // snapshot name contains "+" which might be present in a Cassandra version - // the usage of "+" is forbidden for USER snapshots + // "+" is part of the allowed set because it can appear in a Cassandra version + // (build metadata, e.g. "7.0.0+abc123"), which ends up in system snapshot names. + // The character check applies uniformly to all snapshot types, so "+" is accepted + // for system and user snapshots alike. validate("this_is_system_snapshot-7.0.0+abc123", SnapshotType.UPGRADE); - - assertThatThrownBy(() -> validate("this_is_snapshot-7.0.0+abc123", USER)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Snapshot name contains illegal characters: this_is_snapshot-7.0.0+abc123"); + validate("this_is_snapshot-7.0.0+abc123", USER); } try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false))