[Backport 2.19] Validate base_path in FsRepository to prevent path.repo containment bypass

[opensearch-ci-bot]

Backport 629a185 from #22328.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Overly Broad '..' Rejection The validator rejects any base_path containing the substring .., not just .. path segments. Legitimate directory names that happen to contain two consecutive dots (e.g., my..dir, version..1, foo..bar) will be rejected. Consider splitting by path separators and checking segments equal to .., rather than a substring match, to avoid false positives on valid relative paths. if (Strings.hasLength(value) && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" ); } Windows Path Separator Handling The setting validator only checks / and \\ prefixes and drive-letter absolute paths, but does not reject mixed separators inside the value (e.g., foo\\..\\bar). On Windows, Path.resolve treats \\ as a separator and the .. substring check would catch this particular case, but arbitrary backslash-separated segments are not otherwise normalized in the string validator. The isUnderRepo containment check is the backstop, but it's worth confirming the intended cross-platform semantics. if (Strings.hasLength(value) && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" ); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid false positives on '..' substring The value.contains("..") check will incorrectly reject legitimate base paths containing .. as part of a filename (e.g., my..folder or file..txt). To only reject actual parent-directory traversal segments, split the path on separators and check whether any segment equals ... server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [109-114] if (Strings.hasLength(value) - && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) { + && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*"))) { throw new IllegalArgumentException( - "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" + "[base_path] must be a relative path; got [" + value + "]" ); } +if (Strings.hasLength(value)) { + for (String segment : value.split("[/\\\\]")) { + if ("..".equals(segment)) { + throw new IllegalArgumentException( + "[base_path] must not contain '..' segments; got [" + value + "]" + ); + } + } +} Suggestion importance[1-10]: 6 __ Why: Valid point: value.contains("..") will reject legitimate filenames like my..folder. Splitting on separators and comparing segments is more accurate, though such filenames are uncommon in practice.	Low
Security	Normalize both sides before containment check Path.startsWith compares path components literally and does not follow symbolic links, so a symlink under path.repo pointing outside could still bypass containment. Consider using toRealPath() (or comparing normalized absolute paths of both sides) to make the check robust against symlink-based escapes. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [175-181] -final Path resolved = locationFile.resolve(basePath).normalize(); +final Path resolved = locationFile.resolve(basePath).toAbsolutePath().normalize(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.toAbsolutePath().normalize())) { return true; } } return false; Suggestion importance[1-10]: 6 __ Why: Reasonable defense-in-depth improvement to normalize both sides to absolute paths before comparison, though repoFiles() typically already returns absolute normalized paths. The toRealPath() suggestion for symlinks is a valid concern but not fully addressed in the improved code.	Low

[github-actions]

❌ Gradle check result for 83dc30e: TIMEOUT

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]

Superseded by #22381