Fixes #20464: [BugFix] Restrict rename replacement field size for snapshot restore#20465
Fixes #20464: [BugFix] Restrict rename replacement field size for snapshot restore#20465cwperks merged 1 commit intoopensearch-project:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR addresses a denial-of-service vulnerability in snapshot restore operations by adding validation to limit the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java`:
- Around line 262-271: The byte-length check for renameReplacement uses
getBytes() which depends on the platform charset; change it to compute UTF-8
bytes explicitly (use StandardCharsets.UTF_8) when comparing against
MetadataCreateIndexService.MAX_INDEX_NAME_BYTES so the validation matches
MetadataCreateIndexService.validateIndexOrAliasName; update the check in
RestoreSnapshotRequest where renameReplacement is validated to call
renameReplacement.getBytes(StandardCharsets.UTF_8) (or equivalent UTF-8 charset
API) and keep the existing validationException handling.
🧹 Nitpick comments (2)
server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java (1)
1367-1385: Test coverage looks good; consider adding boundary test cases.The test correctly validates that oversized replacement strings are rejected. For completeness, consider adding boundary tests at exactly 255 bytes and 256 bytes to ensure the boundary condition is handled correctly.
Also, the method name
testInvalidRenameReplacementPatternsuggests testing the pattern, but it's actually testing the replacement string. A name liketestInvalidRenameReplacementSizeortestRenameReplacementExceedsMaxSizewould be more precise.💡 Suggested boundary test
public void testRenameReplacementBoundarySize() { setupSnapshotRestore(); // Test at exactly MAX_INDEX_NAME_BYTES (255) - should pass validation String validReplacement = "a".repeat(255); // This should not throw ValidationException for size // (may fail for other reasons like index already exists) // Test at MAX_INDEX_NAME_BYTES + 1 (256) - should fail validation String invalidReplacement = "a".repeat(256); ValidationException exception = expectThrows( ValidationException.class, () -> client().admin() .cluster() .prepareRestoreSnapshot(snapshotRepo, snapshotName1) .setWaitForCompletion(false) .setIndices(index) .setRenamePattern(index) .setRenameReplacement(invalidReplacement) .get() ); assertTrue(exception.getMessage().contains("rename_replacement string size exceeds max allowed size")); }server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java (1)
262-271: Add length validation forrenameAliasReplacementto prevent DoS via oversized alias names.The
renameAliasReplacementfield uses the samereplaceAllmechanism for alias renaming asrenameReplacementdoes for index renaming. Since alias names are subject to the same 255-byte limit as index names (enforced byMetadataCreateIndexService.validateIndexOrAliasName), the resulting alias name after applying the replacement pattern could exceed this limit. Add equivalent validation forrenameAliasReplacementlength in thevalidate()method to match the protection already provided forrenameReplacement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.javaserver/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.
Applied to files:
server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
🧬 Code graph analysis (1)
server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java (1)
server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java (1)
MetadataCreateIndexService(171-2093)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
🔇 Additional comments (1)
server/src/internalClusterTest/java/org/opensearch/snapshots/RestoreSnapshotIT.java (1)
50-50: LGTM!The import for
ValidationExceptionis correctly added to support the new test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
.../main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java
Show resolved
Hide resolved
|
❌ Gradle check result for 52a49a4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
meet-v25
force-pushed
the
main
branch
2 times, most recently
from
b80d9b0 to
20e0866
Compare
|
❌ Gradle check result for 20e0866: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
meet-v25
force-pushed
the
main
branch
2 times, most recently
from
668682a to
eeaff26
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20465 +/- ##
============================================
+ Coverage 73.23% 73.34% +0.11%
- Complexity 71953 72024 +71
============================================
Files 5795 5795
Lines 329248 329254 +6
Branches 47410 47413 +3
============================================
+ Hits 241122 241505 +383
+ Misses 68841 68404 -437
- Partials 19285 19345 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
❌ Gradle check result for 9b90c6c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
cwperks
changed the title
|
@meet-v25 can you please fix the precommit issues? |
Signed-off-by: meetvm <meetvm@amazon.com>
|
❌ Gradle check result for a785f6d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❕ Gradle check result for a785f6d: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Storage Project Board
…pensearch-project#20465) Signed-off-by: meetvm <meetvm@amazon.com> Co-authored-by: meetvm <meetvm@amazon.com>
…pensearch-project#20465) Signed-off-by: meetvm <meetvm@amazon.com> Co-authored-by: meetvm <meetvm@amazon.com>
2 participants
Description
Fixes an issueupon snapshot restore functionality. An attacker with snapshot restore permissions can crash cluster nodes by exploiting unbounded string expansion in the renameIndex() method. More details in Issue #20464 .
Related Issues
Resolves #20464
Check List
./gradlew :server:internalClusterTest --tests "org.opensearch.snapshots.RestoreSnapshotIT"By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.