Keep model cache paths inside the cache root

[b4sjoo]

Description

Model name and version are appended to cache directories with Path.resolve. An absolute component replaces the cache prefix, and a relative component containing .. can leave the cache after normalization. Deploy then deletes that directory and extracts the model archive there.

resolveWithin rejects empty components, absolute components, and any normalized path outside the corresponding cache root. getModelCachePath, getRegisterModelPath, and getDeployModelZipPath use it. Relative names such as huggingface/sentence-transformers/all-MiniLM-L6-v2 still stay inside the cache.

This pull request targets main only. Backports can follow after review.

Related Issues

No existing issue is linked.

Check List

• New functionality includes testing.
• New functionality has been documented. Not applicable: no user-facing API or documentation change.
• API changes companion pull request created. Not applicable.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created. Not 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 🔍

(Review updated until commit 9f9bf91)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9f9bf91
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle invalid path parsing exceptions On Windows, getPath(component) throws InvalidPathException for strings containing characters like : (e.g., "C:\foo" or even unusual model names). Catch or validate to produce a consistent IllegalArgumentException, and consider using componentPath (already parsed) in resolve to avoid re-parsing with different semantics. ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java [138-142] -Path componentPath = root.getFileSystem().getPath(component); +Path componentPath; +try { + componentPath = root.getFileSystem().getPath(component); +} catch (java.nio.file.InvalidPathException e) { + throw new IllegalArgumentException("Invalid model path component: " + component, e); +} if (componentPath.isAbsolute()) { throw new IllegalArgumentException("Model path component must be a relative path inside the cache directory"); } -resolved = resolved.resolve(component); +resolved = resolved.resolve(componentPath); Suggestion importance[1-10]: 5 __ Why: Catching InvalidPathException to produce a consistent IllegalArgumentException is a reasonable robustness improvement, particularly for cross-platform safety, though it's a minor error-handling refinement.	Low
	Verify path component ordering preserved The original path was register_root/modelId/version/modelName, but the new call passes (modelId, version, modelName) to resolveWithin(getRegisterModelRootPath(), ...), which still produces register_root/modelId/version/modelName — however, since modelId here is user/caller supplied, make sure the order of the arguments reflects the original intent. More importantly, getRegisterModelPath(String modelId) resolves against the register-root, so the layout is preserved. Verify the three-arg method and single-arg method stay consistent for callers that chain them. ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java [81] public Path getRegisterModelPath(String modelId, String modelName, String version) { -- return getRegisterModelPath(modelId).resolve(version).resolve(modelName); -+ return resolveWithin(getRegisterModelRootPath(), modelId, version, modelName); - } + return resolveWithin(getRegisterModelRootPath(), modelId, version, modelName); +} Suggestion importance[1-10]: 2 __ Why: This suggestion only asks to verify the ordering, which is already correctly preserved in the PR. It provides no actionable change.	Low

---

Previous suggestions

Suggestions up to commit ec10acf

Category	Suggestion	Impact
Security	Harden path validation for cross-platform safety On Windows, Path.of("/tmp/outside") is not reported as absolute, and a component like C:\foo would be parsed differently. Also reject components containing a drive/root or backslash separators, and additionally reject any component whose own normalized form escapes (e.g., leading ..) before resolving, to make the check robust across platforms. ml-algorithms/src/main/java/org/opensearch/ml/engine/MLEngine.java [129-146] private Path resolveWithin(Path root, String... components) { Path resolved = root; for (String component : components) { if (component == null || component.isBlank()) { throw new IllegalArgumentException("Model path component must not be empty"); } + if (component.contains("\\") || component.startsWith("/")) { + throw new IllegalArgumentException("Model path component must be a relative path inside the cache directory"); + } Path componentPath = Path.of(component); - if (componentPath.isAbsolute()) { + if (componentPath.isAbsolute() || componentPath.getRoot() != null) { throw new IllegalArgumentException("Model path component must be a relative path inside the cache directory"); } resolved = resolved.resolve(componentPath); } Path normalized = resolved.normalize(); if (!normalized.startsWith(root.normalize())) { throw new IllegalArgumentException("Model path must stay inside the cache directory"); } return normalized; } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid cross-platform concern about path handling on Windows (backslashes, drive letters) that could allow path traversal to bypass the current check. However, the final normalized.startsWith(root.normalize()) check already catches most escape attempts, so the impact is moderate.	Low

[mingshl]

this changed failed the IT

Suite: Test class org.opensearch.ml.action.models.SearchModelITTests

  2> REPRODUCE WITH: ./gradlew ':opensearch-ml-plugin:test' --tests 'org.opensearch.ml.action.models.SearchModelITTests.test_all' -Dtests.seed=BA400E28AF76C383 -Dtests.security.manager=false -Dtests.locale=dsb-Latn-DE -Dtests.timezone=America/Santarem -Druntime.java=21 -Djava.security.properties=/__w/ml-commons/ml-commons/distribution/src/config/fips_java.security
SearchModelITTests > test_all FAILED
  2> java.lang.AssertionError: expected:<1> but was:<0>
    java.lang.AssertionError: expected:<1> but was:<0>
        at __randomizedtesting.SeedInfo.seed([BA400E28AF76C383:9875D8BF4EB8AB13]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at __randomizedtesting.SeedInfo.seed([BA400E28AF76C383:9875D8BF4EB8AB13]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:633)
        at org.opensearch.ml.action.models.SearchModelITTests.test_empty_body_search(SearchModelITTests.java:117)
        at org.opensearch.ml.action.models.SearchModelITTests.test_all(SearchModelITTests.java:101)
  2> NOTE: leaving temporary files on disk at: /__w/ml-commons/ml-commons/plugin/build/testrun/test/temp/org.opensearch.ml.action.models.SearchModelITTests_BA400E28AF76C383-001
  2> NOTE: test params are: codec=Asserting(Lucene104): {index_uuid=PostingsFormat(name=Lucene104), type=PostingsFormat(name=Lucene104)}, docValues:{}, maxPointsInLeafNode=1949, maxMBSortInHeap=5.707490919974225, sim=Asserting(RandomSimilarity(queryNorm=true): {}), locale=dsb-Latn-DE, timezone=America/Santarem

[github-actions]

Persistent review updated to latest commit 9f9bf91

[b4sjoo]

@mingshl Thanks for pointing out!

The model name and version are appended to a directory under ml_cache. The old code joined them with Path.resolve. When that method receives an absolute path, it drops the cache prefix. A relative path containing .. can also leave the cache root after normalization. On deploy, the code deletes whatever is already in that directory and then extracts the model archive there, so a caller with ML privileges can write files outside ml_cache.

The fix adds resolveWithin. getModelCachePath, getRegisterModelPath, and getDeployModelZipPath all use it. Empty segments and absolute paths are rejected, as is any result that leaves the cache root after normalization. Legitimate relative names that contain /, such as huggingface/sentence-transformers/..., still stay inside the cache. Each path segment is resolved on the cache directory's own filesystem, so Lucene's mock filesystem in integration tests does not collide with a default-provider path created by Path.of().