Allow slash in snapshot file name validation

[xuxiong1]

Description

• Add validFileNameExcludingSlash() method to Strings.java that allows forward slashes (/) and backslashes () in file names.
• Update BlobStoreIndexShardSnapshot.FileInfo.fromXContent() to use this new validation for physical file names
• Add unit tests for both the new validation method and the updated parsing behavior

This enables plugins (like time-series-db) to use path-like physical file names (e.g., metrics/head/live_series_index/segments_1) in blob store snapshots, which was previously blocked by the strict validFileName() check that rejected any slashes.

Related Issues

Resolves #20593

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR introduces a new filename validation method that allows slashes while rejecting other invalid characters, and updates snapshot file name validation to use this method. This enables plugins to use path-like filenames in snapshots.

Changes

Cohort / File(s)	Summary
Validation Method Addition libs/core/src/main/java/org/opensearch/core/common/Strings.java	Added new validFileNameExcludingSlash() method that validates filenames while permitting forward and backward slashes, complementing existing filename validators.
Validation Method Tests libs/core/src/test/java/org/opensearch/core/common/StringsTests.java	Added test method testValidFileNameExcludingSlash() with duplicate entries (⚠️ potential issue: method appears twice in summary).
Snapshot File Validation Update server/src/main/java/org/opensearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java	Updated fromXContent() to use validFileNameExcludingSlash() instead of validFileName() for physical name validation.
Snapshot File Validation Tests server/src/test/java/org/opensearch/index/snapshots/blobstore/FileInfoTests.java	Added test method testPhysicalNameWithSlash() to verify that physical filenames containing slashes are correctly parsed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Allow slash in snapshot file name validation' clearly and concisely describes the main change: permitting slashes in snapshot file name validation.
Linked Issues check	✅ Passed	The PR implements all requirements from issue #20593: adds validFileNameExcludingSlash() method, updates BlobStoreIndexShardSnapshot to use it, and includes comprehensive unit tests.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objective of allowing slashes in snapshot file name validation; no extraneous modifications detected.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main
Description check	✅ Passed	The PR description follows the template structure with all required sections completed: clear description of changes, related issue reference, and completed checklist items.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

❗ AI-powered Code-Diff-Analyzer found issues on commit be8bb29.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java	343	medium	Physical file name validation changed to allow slashes/backslashes. This could enable path traversal if physicalName is used in file operations without proper sanitization (e.g., '../../etc/passwd'). However, implementation is transparent with tests, suggesting legitimate feature rather than malicious intent. Verify this change aligns with intended snapshot storage architecture.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

✅ Gradle check result for be8bb29: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.30%. Comparing base (7dfce8d) to head (8289c7c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...cluster/metadata/MetadataIndexTemplateService.java	0.00%	0 Missing and 1 partial ⚠️
...apshots/blobstore/BlobStoreIndexShardSnapshot.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20620      +/-   ##
============================================
+ Coverage     73.26%   73.30%   +0.04%     
- Complexity    72003    72039      +36     
============================================
  Files          5783     5783              
  Lines        329438   329448      +10     
  Branches      47532    47536       +4     
============================================
+ Hits         241347   241511     +164     
+ Misses        68741    68572     -169     
- Partials      19350    19365      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

❗ AI-powered Code-Diff-Analyzer found issues on commit 26c22a9.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/snapshots/blobstore/BlobStoreIndexShardSnapshot.java	343	high	Path traversal vulnerability: Validation changed from validFileName to validFileNameExcludingSlash, now explicitly allowing forward and backward slashes in physicalName parameter. This could enable directory traversal attacks (e.g., ../../etc/passwd) in blob store snapshot operations if physicalName is used in file system operations without additional sanitization.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[msfroh]

Ahh... @xuxiong1, with the new flood of AI-generated comments, I hadn't realized that this one is relevant to your change. Specifically, it says:

Validation changed from validFileName to validFileNameExcludingSlash for physicalName field in blob store snapshots. This now allows forward slashes (/) and backslashes () in physical file names, potentially enabling path traversal attacks (e.g., '../../../etc/passwd'). Physical file names in blob storage should typically be flat identifiers without directory separators. This change could allow writing files outside intended storage boundaries.

Is there anything you can do to guarantee that there is no way of reading/writing outside of the repository, especially when using the fs repository?

[xuxiong1]

Ahh... @xuxiong1, with the new flood of AI-generated comments, I hadn't realized that this one is relevant to your change. Specifically, it says:

Validation changed from validFileName to validFileNameExcludingSlash for physicalName field in blob store snapshots. This now allows forward slashes (/) and backslashes () in physical file names, potentially enabling path traversal attacks (e.g., '../../../etc/passwd'). Physical file names in blob storage should typically be flat identifiers without directory separators. This change could allow writing files outside intended storage boundaries.

Is there anything you can do to guarantee that there is no way of reading/writing outside of the repository, especially when using the fs repository?

The file name paths are all relative paths to the node shard path, so it won't be reading/writing outside of the repository.

[github-actions]

❌ Gradle check result for 268a6af: 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?

[github-actions]

❌ Gradle check result for 268a6af: 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?

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 8289c7c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Path traversal protection: The new validFileNameExcludingSlash() method attempts to prevent path traversal by rejecting ".." segments and absolute paths. However, the implementation may have gaps. The regex split [/\\\\] could miss edge cases like empty segments from consecutive slashes ("path//file"), URL-encoded traversal sequences, or OS-specific path normalization issues. Since this is used for blob store physical file names, ensure the underlying blob store implementation also validates and sanitizes paths to prevent directory traversal attacks that could allow reading/writing files outside intended directories.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Path Traversal Risk The validFileNameExcludingSlash() method splits on [/\\\\] regex to check for ".." segments. However, this approach may not catch all path traversal patterns (e.g., URL-encoded sequences like "%2e%2e", or variations with multiple dots). Consider if additional validation is needed for the blob store use case. for (String segment : fileName.split("[/\\\\]")) { if (segment.equals("..")) { return false; } Validation Inconsistency The name field uses strict validFileName() (no slashes), while physicalName uses validFileNameExcludingSlash() (allows slashes). Verify this is intentional and that the blob store implementation correctly handles physical names with slashes in all code paths (storage, retrieval, deletion). if (name == null || Strings.validFileName(name) == false) { throw new OpenSearchParseException("missing or invalid file name [" + name + "]"); } else if (physicalName == null || Strings.validFileNameExcludingSlash(physicalName) == false) { throw new OpenSearchParseException("missing or invalid physical file name [" + physicalName + "]"); Missing Edge Cases Tests don't cover edge cases like empty string, single dot ".", consecutive slashes "path//file", trailing slashes "path/", or mixed traversal patterns like "path/./../file". Consider adding these test cases to ensure robust validation. public void testValidFileNameExcludingSlash() { // Valid cases - no invalid chars assertTrue(Strings.validFileNameExcludingSlash("validfile")); assertTrue(Strings.validFileNameExcludingSlash("file123")); assertTrue(Strings.validFileNameExcludingSlash("file-name.txt")); // Valid cases - relative slashes are allowed assertTrue(Strings.validFileNameExcludingSlash("path/to/file")); assertTrue(Strings.validFileNameExcludingSlash("path\\to\\file")); assertTrue(Strings.validFileNameExcludingSlash("mixed/path\\file")); // Invalid cases - absolute paths must be rejected assertFalse(Strings.validFileNameExcludingSlash("/absolute/path")); assertFalse(Strings.validFileNameExcludingSlash("\\absolute\\path")); // Invalid cases - path traversal must be rejected assertFalse(Strings.validFileNameExcludingSlash("../etc/passwd")); assertFalse(Strings.validFileNameExcludingSlash("path/../../etc/passwd")); assertFalse(Strings.validFileNameExcludingSlash("path\\..\\..\\secret")); // Invalid cases - other invalid chars should still fail assertFalse(Strings.validFileNameExcludingSlash("file*name")); assertFalse(Strings.validFileNameExcludingSlash("file?name")); assertFalse(Strings.validFileNameExcludingSlash("file\"name")); assertFalse(Strings.validFileNameExcludingSlash("file<name")); assertFalse(Strings.validFileNameExcludingSlash("file>name")); assertFalse(Strings.validFileNameExcludingSlash("file|name")); assertFalse(Strings.validFileNameExcludingSlash("file name")); // space assertFalse(Strings.validFileNameExcludingSlash("file,name")); // comma // Edge case - path with invalid char assertFalse(Strings.validFileNameExcludingSlash("path/to/file*name")); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 8289c7c
Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Handle encoded path traversal attempts The path traversal check only rejects exact ".." segments but misses encoded variants like "%2e%2e" or URL-encoded paths. Consider normalizing the path first or checking for encoded dot sequences to prevent bypass attempts through encoding. libs/core/src/main/java/org/opensearch/core/common/Strings.java [346-351] // Reject path traversal: ".." as a path segment -for (String segment : fileName.split("[/\\\\]")) { +String normalized = fileName.replace("%2e", ".").replace("%2E", "."); +for (String segment : normalized.split("[/\\\\]")) { if (segment.equals("..")) { return false; } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential security issue where URL-encoded path traversal sequences like "%2e%2e" could bypass the ".." check. However, the context of where this method is used (file system operations vs URL handling) should be considered, and the normalization approach shown is incomplete (doesn't handle all encoding variants).	Medium
	Prevent resource exhaustion from splits The split operation creates an array that could be large for maliciously crafted inputs with many consecutive slashes. This could lead to performance issues or memory exhaustion. Consider limiting the number of segments or using a more efficient traversal check. libs/core/src/main/java/org/opensearch/core/common/Strings.java [346-351] // Reject path traversal: ".." as a path segment -for (String segment : fileName.split("[/\\\\]")) { +String[] segments = fileName.split("[/\\\\]", 100); +if (segments.length >= 100) { + return false; +} +for (String segment : segments) { if (segment.equals("..")) { return false; } } Suggestion importance[1-10]: 5 __ Why: The suggestion addresses a potential DoS vector from excessive path segments, but the severity is limited since the input length is already bounded by the earlier character-by-character validation loop. The limit of 100 segments is arbitrary and may be too restrictive for legitimate use cases.	Low
Possible issue	Add null and empty validation The method doesn't validate for null or empty input, which could cause NullPointerException or unexpected behavior. Add null/empty checks at the beginning of the method to handle edge cases gracefully. libs/core/src/main/java/org/opensearch/core/common/Strings.java [334-338] public static boolean validFileNameExcludingSlash(String fileName) { + if (fileName == null || fileName.isEmpty()) { + return false; + } // Reject absolute paths if (fileName.startsWith("/") || fileName.startsWith("\\")) { return false; } Suggestion importance[1-10]: 6 __ Why: Valid suggestion to add null/empty checks to prevent NullPointerException. However, the existing methods validFileName and validFileNameExcludingAsterisk in the same class don't have such checks, suggesting this might be handled by callers. The score reflects this as a defensive programming improvement rather than a critical fix.	Low

---

Previous suggestions

Suggestions up to commit 268a6af

Category	Suggestion	Impact
Possible issue	Add null and empty validation Add null and empty string validation at the beginning of the method to prevent NullPointerException or unexpected behavior. This ensures the method handles edge cases consistently with other validation methods in the class. libs/core/src/main/java/org/opensearch/core/common/Strings.java [334-338] public static boolean validFileNameExcludingSlash(String fileName) { + if (fileName == null || fileName.isEmpty()) { + return false; + } // Reject absolute paths if (fileName.startsWith("/") || fileName.startsWith("\\")) { return false; } Suggestion importance[1-10]: 7 __ Why: Adding null and empty string validation is a good defensive programming practice that prevents potential NullPointerException when calling startsWith() or during iteration. This aligns with robust input validation patterns, though the impact depends on how the method is called in the codebase.	Medium
General	Add edge case test coverage Add test cases for edge scenarios like empty strings, null values, single dots (".", "./file"), trailing slashes ("path/"), and consecutive slashes ("path//file") to ensure comprehensive validation coverage. libs/core/src/test/java/org/opensearch/core/common/StringsTests.java [144-147] // Invalid cases - path traversal must be rejected assertFalse(Strings.validFileNameExcludingSlash("../etc/passwd")); assertFalse(Strings.validFileNameExcludingSlash("path/../../etc/passwd")); assertFalse(Strings.validFileNameExcludingSlash("path\\..\\..\\secret")); +// Edge cases +assertFalse(Strings.validFileNameExcludingSlash("")); +assertTrue(Strings.validFileNameExcludingSlash("./file")); +assertTrue(Strings.validFileNameExcludingSlash("path/")); + Suggestion importance[1-10]: 5 __ Why: The suggestion to add more edge case tests (empty strings, single dots, trailing slashes) would improve test coverage. However, the improved_code section doesn't accurately reflect where these tests should be added (they should be separate test cases, not inserted in the middle of path traversal tests), and some suggested assertions may not align with the intended behavior (e.g., whether "./file" or "path/" should be valid).	Low
Security	Handle whitespace in path traversal check The current implementation doesn't handle edge cases like "....", ".. ", or ". ." which could bypass the check. Use segment.trim().equals("..") or check if the segment contains only dots to prevent potential path traversal vulnerabilities. libs/core/src/main/java/org/opensearch/core/common/Strings.java [346-351] // Reject path traversal: ".." as a path segment for (String segment : fileName.split("[/\\\\]")) { - if (segment.equals("..")) { + if (segment.trim().equals("..")) { return false; } } Suggestion importance[1-10]: 3 __ Why: While the suggestion to use trim() could catch edge cases like ".. " (with trailing space), the current implementation correctly rejects ".." as a path segment. The suggested improvement is marginal since path segments with whitespace around ".." are uncommon in real-world scenarios, and the existing check is sufficient for typical path traversal attacks.	Low

[github-actions]

❌ Gradle check result for 268a6af: 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?

[github-actions]

Persistent review updated to latest commit 8289c7c

[github-actions]

✅ Gradle check result for 8289c7c: SUCCESS

[srikanthpadakanti]

This is breaking the cross cluster replication plugin build

[cwperks]

@srikanthpadakanti raised opensearch-project/cross-cluster-replication#1639 to address this.