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

[kh3ra]

Description

The fs repository base_path setting was read verbatim from REST input with no validation. An absolute base_path causes Path.resolve to discard the path.repo-validated location, redirecting all blob-store operations outside the repository (CWE-22) and enabling arbitrary filesystem deletion via the snapshot _cleanup API.

Add two layers of defense:

• BASE_PATH_SETTING validator rejects absolute and upward-escaping (..) values after normalization (benign interior '..' that cancels out is allowed).
• validateBasePathWithinRepo() resolves base_path against the location and verifies the result stays within a configured path.repo directory.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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.

[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 Backward Compatibility The new validator rejects any base_path containing .. even when it resolves to a benign interior path (e.g., foo/../bar, a/b/../c). The PR description states "benign interior '..' that cancels out is allowed", but the implementation rejects all such values via a simple value.contains("..") check. This is a behavioral change that will break existing repository configurations using such paths and contradicts the stated intent. Consider normalizing the path first and only rejecting if the normalized form still contains .. or is absolute. 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 + "]" ); } Incomplete Substring Check The check value.contains("..") will also reject legitimate path segments that merely contain two consecutive dots as part of a filename (e.g., my..folder, version..1). A more precise check would split on the path separator and inspect segments for exact .. matches. This may cause unexpected rejection of previously valid repository configurations after upgrade. && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) {

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid false positives on '..' substring check The check value.contains("..") produces false positives for legitimate names such as my..folder or foo..bar which are valid filename characters and do not represent parent-directory traversal. Split the path on separators (/ and </code>) and reject only segments that equal exactly .., so that legitimate names containing consecutive dots are still accepted. 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]: 7 __ Why: Valid point: value.contains("..") would reject legitimate filenames like my..folder. Splitting on separators and checking for exact .. segments is more accurate and avoids false positives, though the impact is moderate since such names are uncommon.	Medium
	Normalize both paths before containment check Path.startsWith performs textual comparison and can be fooled by symlinks or non-normalized repoPath entries. Normalize (and ideally toAbsolutePath) both sides before comparison to make the containment check robust and consistent with how locationFile was normalized. 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).normalize().toAbsolutePath(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.normalize().toAbsolutePath())) { return true; } } return false; Suggestion importance[1-10]: 6 __ Why: Normalizing and converting both paths to absolute form before startsWith comparison improves robustness of the containment check, especially when repoFiles() entries are not guaranteed to be normalized or absolute. This strengthens the security-critical check.	Low

[github-actions]

✅ Gradle check result for f837d46: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.11%. Comparing base (aff3489) to head (f837d46).
⚠️ Report is 14 commits behind head on 2.19.

Files with missing lines	Patch %	Lines
...a/org/opensearch/repositories/fs/FsRepository.java	47.05%	4 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               2.19   #22381      +/-   ##
============================================
+ Coverage     71.92%   72.11%   +0.18%     
+ Complexity    66009    64486    -1523     
============================================
  Files          5342     5121     -221     
  Lines        307392   300247    -7145     
  Branches      44862    44095     -767     
============================================
- Hits         221105   216532    -4573     
+ Misses        67823    65525    -2298     
+ Partials      18464    18190     -274     

☔ View full report in Codecov by Harness.
📢 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.