Skip to content

Commit ac19ddc

Browse files
smiklosovicsmiklosovic
committed
Merge branch 'cassandra-6.0' into trunk
2 parents 36b23cb + 62a3797 commit ac19ddc

5 files changed

Lines changed: 192 additions & 11 deletions

File tree

‎CHANGES.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ Merged from 6.0:
3131
Merged from 5.0:
3232
* Use estimated compressed size for tables to check if there is enough free space for a compaction (CASSANDRA-21245)
3333
* Fix failing select on system_views.settings for non-string keys (CASSANDRA-21348)
34+
Merged from 4.0:
35+
* Validate snapshot names (CASSANDRA-21389)
3436

3537

3638
6.0-alpha2

‎src/java/org/apache/cassandra/config/CassandraRelevantProperties.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,10 @@ public enum CassandraRelevantProperties
573573
SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED("cassandra.snapshot.enrich.or.create.enabled", "true"),
574574
/** minimum allowed TTL for snapshots */
575575
SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"),
576+
/**
577+
* Validates names of to-be-taken snapshots, defaults to true.
578+
*/
579+
SNAPSHOT_NAME_VALIDATION("cassandra.snapshot.validation", "true"),
576580
SSL_ENABLE("ssl.enable"),
577581
SSL_STORAGE_PORT("cassandra.ssl_storage_port"),
578582
START_GOSSIP("cassandra.start_gossip", "true"),

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

Lines changed: 71 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Collections;
2424
import java.util.Map;
2525
import java.util.function.Predicate;
26+
import java.util.regex.Pattern;
2627

2728
import com.google.common.util.concurrent.RateLimiter;
2829

@@ -33,13 +34,23 @@
3334
import org.apache.cassandra.config.DurationSpec;
3435
import org.apache.cassandra.db.ColumnFamilyStore;
3536
import org.apache.cassandra.io.sstable.format.SSTableReader;
37+
import org.apache.cassandra.io.util.File;
38+
import org.apache.cassandra.schema.SchemaConstants;
3639

3740
import static java.lang.String.format;
41+
import static org.apache.cassandra.schema.SchemaConstants.FILENAME_LENGTH;
42+
import static org.apache.cassandra.utils.FBUtilities.now;
3843

3944
public class SnapshotOptions
4045
{
4146
public static final String SKIP_FLUSH = "skipFlush";
4247
public static final String TTL = "ttl";
48+
49+
// Allowed snapshot-name characters: 0-9 a-z A-Z - _ . +
50+
// See the validation site in validateTag for the full rationale (an S3 safe-set subset, plus '+').
51+
// Hyphen is placed last in the character class, so it stays literal and never becomes a range operator.
52+
private static final Pattern SAFE_SNAPSHOT_NAME = Pattern.compile("[a-zA-Z0-9_.+-]+");
53+
4354
public final SnapshotType type;
4455
public final String tag;
4556
public final DurationSpec.IntSecondsBound ttl;
@@ -88,7 +99,7 @@ public static SnapshotOptions userSnapshot(String tag, Map<String, String> optio
8899
return builder.build();
89100
}
90101

91-
public String getSnapshotName(Instant creationTime)
102+
public static String getSnapshotName(SnapshotType type, String tag, Instant creationTime)
92103
{
93104
// Diagnostic snapshots have very specific naming convention hence we are keeping it.
94105
// Repair snapshots rely on snapshots having name of their repair session ids
@@ -189,10 +200,68 @@ public Builder rateLimiter(RateLimiter rateLimiter)
189200
}
190201

191202
public SnapshotOptions build()
203+
{
204+
validateTag(tag);
205+
validateTTL(ephemeral, ttl);
206+
207+
if (rateLimiter == null)
208+
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
209+
210+
return new SnapshotOptions(this);
211+
}
212+
213+
private void validateTag(String tag)
192214
{
193215
if (tag == null || tag.isEmpty())
194-
throw new RuntimeException("You must supply a snapshot name.");
216+
throw new IllegalArgumentException("You must supply a snapshot name.");
217+
218+
if (tag.contains(File.pathSeparator()))
219+
{
220+
throw new IllegalArgumentException("Snapshot name cannot contain " + File.pathSeparator());
221+
}
222+
223+
if (tag.equals(".") || tag.equals(".."))
224+
{
225+
throw new IllegalArgumentException("Snapshot name '" + tag + "' is reserved");
226+
}
195227

228+
if (!CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION.getBoolean())
229+
return;
230+
231+
// Pre-generate snapshot name for the sake of the validation.
232+
// getSnapshotName logic does not return raw "tag" as snapshot name every time,
233+
// it e.g. prepends timestamp and type for system snapshots, and we need to validate it as a whole.
234+
// If, for example, tag would be less than max allowed FILENAME_LENGTH,
235+
// we might in fact produce a snapshot name longer than FILENAME_LENGTH if we prepended a timestamp to it.
236+
String resolvedSnapshotName = SnapshotOptions.getSnapshotName(type, tag, now());
237+
238+
// the length of valid snapshot name has to be less than or equal to FILENAME_LEGTH - that is 255 -
239+
// we are following the max length as it is in SchemaConstants for table name.
240+
if (resolvedSnapshotName.length() > SchemaConstants.FILENAME_LENGTH)
241+
{
242+
throw new IllegalArgumentException(format("Snapshot name must not be more than %d characters long for " +
243+
"resolved snapshot name (got %d characters for \"%s\")",
244+
FILENAME_LENGTH, resolvedSnapshotName.length(), resolvedSnapshotName));
245+
}
246+
247+
// Allowed characters are a conservative subset of the AWS S3 "Safe characters" set
248+
// (https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines):
249+
// 0-9 a-z A-Z - _ .
250+
// plus '+', which is not an S3 "Safe character" but can legitimately appear in system
251+
// snapshot names via version build metadata (e.g. an upgrade snapshot "<millis>-upgrade-5.0.4+build-...").
252+
// The remaining S3-safe characters (! * ' ( )) are intentionally excluded as they are
253+
// shell-significant and error-prone in paths, and the path separator '/' is excluded too,
254+
// which is what blocks traversal attempts such as "../../mysnapshot"
255+
if (!SAFE_SNAPSHOT_NAME.matcher(resolvedSnapshotName).matches())
256+
{
257+
throw new IllegalArgumentException("Snapshot name contains illegal characters: " + resolvedSnapshotName + ". " +
258+
"Allowed characters must match the pattern: " + SAFE_SNAPSHOT_NAME.pattern() +
259+
" with a maximum of length of " + FILENAME_LENGTH + " characters.");
260+
}
261+
}
262+
263+
private void validateTTL(boolean ephemeral, DurationSpec.IntSecondsBound ttl)
264+
{
196265
if (ttl != null)
197266
{
198267
int minAllowedTtlSecs = CassandraRelevantProperties.SNAPSHOT_MIN_ALLOWED_TTL_SECONDS.getInt();
@@ -202,11 +271,6 @@ public SnapshotOptions build()
202271

203272
if (ephemeral && ttl != null)
204273
throw new IllegalStateException(format("can not take ephemeral snapshot (%s) while ttl is specified too", tag));
205-
206-
if (rateLimiter == null)
207-
rateLimiter = DatabaseDescriptor.getSnapshotRateLimiter();
208-
209-
return new SnapshotOptions(this);
210274
}
211275
}
212276

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public Map<ColumnFamilyStore, TableSnapshot> getSnapshotsToCreate()
8888
else
8989
creationTime = options.creationTime;
9090

91-
snapshotName = options.getSnapshotName(creationTime);
91+
snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, creationTime);
9292

9393
Set<ColumnFamilyStore> entitiesForSnapshot = options.cfs == null ? parseEntitiesForSnapshot(options.entities) : Set.of(options.cfs);
9494

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

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,134 @@
2626

2727
import org.junit.Test;
2828

29+
import org.apache.cassandra.config.CassandraRelevantProperties;
30+
import org.apache.cassandra.distributed.shared.WithProperties;
31+
import org.apache.cassandra.io.util.File;
32+
2933
import static java.lang.String.format;
34+
import static org.apache.cassandra.service.snapshot.SnapshotType.USER;
35+
import static org.assertj.core.api.Assertions.assertThatCode;
36+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
3037
import static org.junit.Assert.assertEquals;
3138

3239
public class SnapshotOptionsTest
3340
{
41+
@Test
42+
public void testSnapshotNameValidation()
43+
{
44+
String sep = File.pathSeparator();
45+
46+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, true))
47+
{
48+
// Only alphanumerics, '-', '_' and '.' are accepted.
49+
validate("atag", USER);
50+
validate("a-tag", USER);
51+
validate("a_tag", USER);
52+
validate("a_tag" + Instant.now().toEpochMilli(), USER);
53+
validate("a_tag_1and_something2-more", USER);
54+
validate("a".repeat(255), USER);
55+
validate("snap.2026-05-20", USER);
56+
// Dots embedded in a name are not traversal: with '/' excluded, "a..tag" is just a literal directory.
57+
validate("a..tag", USER);
58+
59+
assertThatThrownBy(() -> validate("a".repeat(256), USER))
60+
.isInstanceOf(IllegalArgumentException.class)
61+
.hasMessage("Snapshot name must not be more than 255 characters long for " +
62+
"resolved snapshot name (got 256 characters for \"" + "a".repeat(256) + "\")");
63+
64+
// this would append timestamp + type which would violate < 255 when tag is 250 chars long only
65+
assertThatThrownBy(() -> validate("a".repeat(250), SnapshotType.UPGRADE))
66+
.isInstanceOf(IllegalArgumentException.class)
67+
.hasMessageContaining("Snapshot name must not be more than 255 characters long");
68+
69+
// '/' is not in the allowed set; this is what kills traversal attempts like "../../mysnapshot".
70+
assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
71+
.isInstanceOf(IllegalArgumentException.class)
72+
.hasMessage("Snapshot name cannot contain " + sep);
73+
74+
// The shell-significant S3 "safe" characters (! * ' ( )) are deliberately NOT allowed.
75+
assertThatThrownBy(() -> validate("important!", USER))
76+
.isInstanceOf(IllegalArgumentException.class)
77+
.hasMessageContaining("Snapshot name contains illegal characters: important!");
78+
assertThatThrownBy(() -> validate("backup*", USER))
79+
.isInstanceOf(IllegalArgumentException.class)
80+
.hasMessageContaining("Snapshot name contains illegal characters: backup*");
81+
assertThatThrownBy(() -> validate("o'snap", USER))
82+
.isInstanceOf(IllegalArgumentException.class)
83+
.hasMessageContaining("Snapshot name contains illegal characters: o'snap");
84+
assertThatThrownBy(() -> validate("snap(1)", USER))
85+
.isInstanceOf(IllegalArgumentException.class)
86+
.hasMessageContaining("Snapshot name contains illegal characters: snap(1)");
87+
88+
// Other characters outside the allowed set must still be rejected.
89+
assertThatThrownBy(() -> validate("a tag", USER))
90+
.isInstanceOf(IllegalArgumentException.class)
91+
.hasMessageContaining("Snapshot name contains illegal characters: a tag");
92+
assertThatThrownBy(() -> validate("a:tag", USER))
93+
.isInstanceOf(IllegalArgumentException.class)
94+
.hasMessageContaining("Snapshot name contains illegal characters: a:tag");
95+
96+
// "." and ".." pass the charset check but resolve to the snapshots/ dir itself
97+
// and its parent (the live table dir) respectively, so they must be rejected as reserved.
98+
assertThatThrownBy(() -> validate(".", USER))
99+
.isInstanceOf(IllegalArgumentException.class)
100+
.hasMessage("Snapshot name '.' is reserved");
101+
102+
assertThatThrownBy(() -> validate("..", USER))
103+
.isInstanceOf(IllegalArgumentException.class)
104+
.hasMessage("Snapshot name '..' is reserved");
105+
106+
// "+" is part of the allowed set because it can appear in a Cassandra version
107+
// (build metadata, e.g. "7.0.0+abc123"), which ends up in system snapshot names.
108+
// The character check applies uniformly to all snapshot types, so "+" is accepted
109+
// for system and user snapshots alike.
110+
validate("this_is_system_snapshot-7.0.0+abc123", SnapshotType.UPGRADE);
111+
validate("this_is_snapshot-7.0.0+abc123", USER);
112+
}
113+
114+
try (WithProperties p = new WithProperties().set(CassandraRelevantProperties.SNAPSHOT_NAME_VALIDATION, false))
115+
{
116+
// The character check is bypassed entirely: space, ':', and the now-disallowed
117+
// shell-significant characters (! * ' ( )) are all accepted.
118+
assertThatCode(() -> validate("a tag", USER)).doesNotThrowAnyException();
119+
assertThatCode(() -> validate("a:tag", USER)).doesNotThrowAnyException();
120+
assertThatCode(() -> validate("important!", USER)).doesNotThrowAnyException();
121+
assertThatCode(() -> validate("snap(1)", USER)).doesNotThrowAnyException();
122+
123+
assertThatCode(() -> validate("a".repeat(256), USER)).doesNotThrowAnyException();
124+
assertThatCode(() -> validate("a".repeat(250), SnapshotType.UPGRADE)).doesNotThrowAnyException();
125+
126+
// Path separator and "." / ".." rejections are unconditional — they guard against
127+
// traversal regardless of the toggle.
128+
assertThatThrownBy(() -> validate('a' + sep + "tag", USER))
129+
.isInstanceOf(IllegalArgumentException.class)
130+
.hasMessage("Snapshot name cannot contain " + sep);
131+
assertThatThrownBy(() -> validate(".", USER))
132+
.isInstanceOf(IllegalArgumentException.class)
133+
.hasMessage("Snapshot name '.' is reserved");
134+
assertThatThrownBy(() -> validate("..", USER))
135+
.isInstanceOf(IllegalArgumentException.class)
136+
.hasMessage("Snapshot name '..' is reserved");
137+
}
138+
}
139+
140+
private void validate(String tag, SnapshotType type)
141+
{
142+
new SnapshotOptions.Builder(tag, type, s -> true, "ks.tb").rateLimiter(RateLimiter.create(1)).build();
143+
}
144+
34145
@Test
35146
public void testSnapshotName()
36147
{
37-
List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, SnapshotType.USER);
148+
List<SnapshotType> sameNameTypes = List.of(SnapshotType.DIAGNOSTICS, SnapshotType.REPAIR, USER);
38149

39150
for (SnapshotType type : sameNameTypes)
40151
{
41152
SnapshotOptions options = SnapshotOptions.systemSnapshot("a_name", type, "ks.tb")
42153
.rateLimiter(RateLimiter.create(5))
43154
.build();
44155

45-
String snapshotName = options.getSnapshotName(Instant.now());
156+
String snapshotName = SnapshotOptions.getSnapshotName(type, options.tag, Instant.now());
46157
assertEquals("a_name", snapshotName);
47158
}
48159

@@ -57,7 +168,7 @@ public void testSnapshotName()
57168

58169
Instant now = Instant.now();
59170

60-
String snapshotName = options.getSnapshotName(now);
171+
String snapshotName = SnapshotOptions.getSnapshotName(options.type, options.tag, now);
61172

62173
assertEquals(format("%d-%s-%s", now.toEpochMilli(), type.label, "a_name"), snapshotName);
63174
}

0 commit comments

Comments
 (0)