From 3f37284ceda37032183faf8134b652814926517a Mon Sep 17 00:00:00 2001 From: Graeme Mjehovich <59065536+gmjehovich@users.noreply.github.com> Date: Tue, 18 Nov 2025 09:27:44 -0600 Subject: [PATCH] Add length validation for rename_replacement parameter in snapshot restore request (#137859) * Add length validation for rename_replacement parameter in snapshot restore * [CI] Auto commit changes from spotless * Update RestoreSnapshotIT.java * Update docs/changelog/137859.yaml * Fixup integ testing # Conflicts: # server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java * Fix integration tests * Add new IT test case for long replacement * Add buffer to rename_replacement size check; add safeRenameIndex() * Randomize rename_replacement in testRenameReplacementNameTooLong() * [CI] Auto commit changes from spotless * Add unit test for safeRenameIndex() * fix testRenameReplacementNameTooLong() to use createTestInstance() * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine (cherry picked from commit 6b59792c57d786c195151d460da887a289785ab8) # Conflicts: # server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java --- docs/changelog/137859.yaml | 6 ++ .../snapshots/RestoreSnapshotIT.java | 89 +++++++++++++++++++ .../restore/RestoreSnapshotRequest.java | 17 ++++ .../snapshots/RestoreService.java | 25 +++++- .../restore/RestoreSnapshotRequestTests.java | 14 +++ .../snapshots/RestoreServiceTests.java | 38 ++++++++ 6 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 docs/changelog/137859.yaml diff --git a/docs/changelog/137859.yaml b/docs/changelog/137859.yaml new file mode 100644 index 0000000000000..c0e8e13f9f94b --- /dev/null +++ b/docs/changelog/137859.yaml @@ -0,0 +1,6 @@ +pr: 137859 +summary: Add length validation for `rename_replacement` parameter in snapshot restore + request +area: Snapshot/Restore +type: bug +issues: [] diff --git a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java index 5db77fa1f0f42..034e3bc364f65 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/snapshots/RestoreSnapshotIT.java @@ -1025,4 +1025,93 @@ public void testNoWarningsOnRestoreOverClosedIndex() throws IllegalAccessExcepti mockLog.assertAllExpectationsMatched(); } } + + public void testRenameReplacementValidation() throws Exception { + String repoName = "test-repo"; + createRepository(repoName, "fs"); + + // Create an index with a long name (255 characters of 'b') + String indexName = "b".repeat(255); + createIndex(indexName); + indexRandomDocs(indexName, 10); + + String snapshotName = "test-snap"; + createSnapshot(repoName, snapshotName, Collections.singletonList(indexName)); + + logger.info("--> delete the index"); + cluster().wipeIndices(indexName); + + logger.info("--> attempt restore with excessively long rename_replacement (should fail validation)"); + IllegalArgumentException exception = expectThrows( + IllegalArgumentException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repoName, snapshotName) + .setIndices(indexName) + .setRenamePattern("b") + .setRenameReplacement("1".repeat(randomIntBetween(266, 10_000))) + .setWaitForCompletion(true) + .get() + ); + assertThat(exception.getMessage(), containsString("rename_replacement UTF-8 byte length")); + assertThat(exception.getMessage(), containsString("exceeds maximum allowed length")); + + logger.info("--> restore with rename pattern that creates too-long index name (should fail)"); + IllegalArgumentException exception2 = expectThrows( + IllegalArgumentException.class, + () -> client().admin() + .cluster() + .prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repoName, snapshotName) + .setIndices(indexName) + .setRenamePattern("b") + .setRenameReplacement("aa") + .setWaitForCompletion(true) + .get() + ); + assertThat(exception2.getMessage(), containsString("index name would exceed")); + assertThat(exception2.getMessage(), containsString("bytes after rename")); + + logger.info("--> restore with valid simple rename (should succeed)"); + RestoreSnapshotResponse restoreResponse = client().admin() + .cluster() + .prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repoName, snapshotName) + .setIndices(indexName) + .setRenamePattern("b+") + .setRenameReplacement("restored") + .setWaitForCompletion(true) + .get(); + assertThat(restoreResponse.getRestoreInfo().failedShards(), equalTo(0)); + assertTrue("Renamed index should exist", indexExists("restored")); + ensureGreen("restored"); + + cluster().wipeIndices("restored"); + + logger.info("--> restore with back-reference in replacement (should succeed)"); + RestoreSnapshotResponse restoreResponseBackRef = client().admin() + .cluster() + .prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repoName, snapshotName) + .setIndices(indexName) + .setRenamePattern("(b{100}).*") + .setRenameReplacement("$1-restored") + .setWaitForCompletion(true) + .get(); + assertThat(restoreResponseBackRef.getRestoreInfo().failedShards(), equalTo(0)); + String backRefIndex = "b".repeat(100) + "-restored"; + assertTrue("Back-ref index should exist", indexExists(backRefIndex)); + ensureGreen(backRefIndex); + + cluster().wipeIndices(backRefIndex); + + logger.info("--> restore with non-matching pattern (should leave name unchanged)"); + RestoreSnapshotResponse restoreResponseNoMatch = client().admin() + .cluster() + .prepareRestoreSnapshot(TEST_REQUEST_TIMEOUT, repoName, snapshotName) + .setIndices(indexName) + .setRenamePattern("z") + .setRenameReplacement("replaced") + .setWaitForCompletion(true) + .get(); + assertThat(restoreResponseNoMatch.getRestoreInfo().failedShards(), equalTo(0)); + assertTrue("Original index name should exist when pattern doesn't match", indexExists(indexName)); + } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java index e852c49dc2e3f..dbdbebbdc25e7 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java @@ -26,6 +26,7 @@ import org.elasticsearch.xcontent.XContentType; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -33,6 +34,7 @@ import java.util.Objects; import static org.elasticsearch.action.ValidateActions.addValidationError; +import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.MAX_INDEX_NAME_BYTES; import static org.elasticsearch.common.settings.Settings.readSettingsFromStream; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; @@ -152,6 +154,21 @@ public ActionRequestValidationException validate() { if (featureStates == null) { validationException = addValidationError("featureStates is missing", validationException); } + if (renameReplacement != null) { + int byteLength = renameReplacement.getBytes(StandardCharsets.UTF_8).length; + + // Allow small buffer for cases where back-references or escapes result in shorter output + if (renameReplacement.getBytes(StandardCharsets.UTF_8).length > MAX_INDEX_NAME_BYTES + 10) { + validationException = addValidationError( + "rename_replacement UTF-8 byte length [" + + byteLength + + "] exceeds maximum allowed length [" + + (MAX_INDEX_NAME_BYTES + 10) + + "] bytes", + validationException + ); + } + } if (indexSettings == null) { validationException = addValidationError("indexSettings are missing", validationException); } diff --git a/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java b/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java index f57db79d4625d..3d2ff14f32bb1 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/RestoreService.java @@ -105,6 +105,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import java.util.function.Function; +import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -116,6 +117,7 @@ import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_VERSION_CREATED; +import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.MAX_INDEX_NAME_BYTES; import static org.elasticsearch.core.Strings.format; import static org.elasticsearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; import static org.elasticsearch.snapshots.SearchableSnapshotsSettings.SEARCHABLE_SNAPSHOTS_DELETE_SNAPSHOT_ON_INDEX_DELETION; @@ -1038,7 +1040,7 @@ private static String renameIndex(String index, RestoreSnapshotRequest request, if (prefix != null) { index = index.substring(prefix.length()); } - renamedIndex = index.replaceAll(request.renamePattern(), request.renameReplacement()); + renamedIndex = safeRenameIndex(index, request.renamePattern(), request.renameReplacement()); if (prefix != null) { renamedIndex = prefix + renamedIndex; } @@ -1909,6 +1911,27 @@ private void ensureValidIndexName( createIndexService.validateIndexSettings(renamedIndexName, snapshotIndexMetadata.getSettings(), false); } + // package-private for unit testing + static String safeRenameIndex(String index, String renamePattern, String renameReplacement) { + final var matcher = Pattern.compile(renamePattern).matcher(index); + var found = matcher.find(); + if (found) { + final var sb = new StringBuilder(); + do { + matcher.appendReplacement(sb, renameReplacement); + found = matcher.find(); + } while (found && sb.length() <= MAX_INDEX_NAME_BYTES); + + if (sb.length() > MAX_INDEX_NAME_BYTES) { + throw new IllegalArgumentException("index name would exceed " + MAX_INDEX_NAME_BYTES + " bytes after rename"); + } + matcher.appendTail(sb); + return sb.toString(); + } else { + return index; + } + } + private static void ensureSearchableSnapshotsRestorable( final ClusterState currentState, final SnapshotInfo snapshotInfo, diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java index 2be8989216d71..3d6f5e80d5474 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.action.admin.cluster.snapshots.restore; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; @@ -29,6 +30,8 @@ import java.util.Map; import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; public class RestoreSnapshotRequestTests extends AbstractWireSerializingTestCase { private RestoreSnapshotRequest randomState(RestoreSnapshotRequest instance) { @@ -176,6 +179,17 @@ public void testToStringWillIncludeSkipOperatorOnlyState() { assertThat(original.toString(), containsString("skipOperatorOnlyState")); } + public void testRenameReplacementNameTooLong() { + RestoreSnapshotRequest request = createTestInstance(); + request.indices("b".repeat(255)); + request.renamePattern("b"); + request.renameReplacement("1".repeat(randomIntBetween(266, 10_000))); + + ActionRequestValidationException validation = request.validate(); + assertNotNull(validation); + assertThat(validation.getMessage(), containsString("rename_replacement")); + } + private Map convertRequestToMap(RestoreSnapshotRequest request) throws IOException { XContentBuilder builder = request.toXContent(XContentFactory.jsonBuilder(), new ToXContent.MapParams(Collections.emptyMap())); try ( diff --git a/server/src/test/java/org/elasticsearch/snapshots/RestoreServiceTests.java b/server/src/test/java/org/elasticsearch/snapshots/RestoreServiceTests.java index e45dc4e3d8cd5..b5e7605d2f268 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/RestoreServiceTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/RestoreServiceTests.java @@ -38,6 +38,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import static org.elasticsearch.core.Strings.format; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.mockito.ArgumentMatchers.any; @@ -291,6 +292,43 @@ public void testNotAllowToRestoreGlobalStateFromSnapshotWithoutOne() { ); } + public void testSafeRenameIndex() { + // Test normal rename + String result = RestoreService.safeRenameIndex("test-index", "test", "prod"); + assertEquals("prod-index", result); + + // Test pattern that creates too-long name (255×255 case) + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> RestoreService.safeRenameIndex("b".repeat(255), "b", "aa") + ); + assertThat(e.getMessage(), containsString("exceed")); + + // Test back-reference + result = RestoreService.safeRenameIndex("test-123", "(test)-(\\d+)", "$1_$2"); + assertEquals("test_123", result); + + // Test back-reference that would be too long + e = expectThrows(IllegalArgumentException.class, () -> RestoreService.safeRenameIndex("a".repeat(200), "(a+)", "$1$1")); + assertThat(e.getMessage(), containsString("exceed")); + + // Test no match - returns original + result = RestoreService.safeRenameIndex("test", "xyz", "replacement"); + assertEquals("test", result); + + // Test exactly at limit (255 chars) + result = RestoreService.safeRenameIndex("b".repeat(255), "b+", "a".repeat(255)); + assertEquals("a".repeat(255), result); + + // Test empty replacement + result = RestoreService.safeRenameIndex("test-index", "test-", ""); + assertEquals("index", result); + + // Test multiple matches accumulating + result = RestoreService.safeRenameIndex("a-b-c", "-", "_"); + assertEquals("a_b_c", result); + } + private static SnapshotInfo createSnapshotInfo(Snapshot snapshot, Boolean includeGlobalState) { var shards = randomIntBetween(0, 100); return new SnapshotInfo(