Repository navigation
Harden shared cache downloads against paths outside the output folder - #2789
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 3 commits into
Conversation
Reject blob names that resolve outside the target directory when downloading from the shared cache, store local model paths relative to the cached model folder, and keep 16 hex characters of the run hash in cache model ids to make collisions on a shared container less likely.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The blob-path check still allows unresolved traversal paths to create directories outside the output folder; resolve the path once and use it for both validation and downloading.
1 open issue
What changed in this PR
This PR hardens Olive’s shared cache by checking download destinations, normalizing cached local model paths, and reducing model ID collision risk.
Changes:
- Reject blob destinations that resolve outside the intended output directory.
- Store local model paths as file or folder names rather than machine-specific paths.
- Extend output model IDs from 8 to 16 hexadecimal characters and add download-validation and ONNX round-trip tests.
| File | Description |
|---|---|
test/test_cache.py |
Adds valid-download, path-traversal rejection, and absolute-path ONNX round-trip tests. |
olive/cache.py |
Adds destination containment checks, normalizes cached local paths, and widens model IDs. |
Review effort: Balanced
Resolve the local path once and use it for both the containment check and the download, so names with ".." segments can't create directories outside the output folder.
|
Heads up for reviewers: the failing "Ubuntu test model fast" check is not caused by this PR. Every smoke test in that job errors during setup with I opened #2790 to track it. #2788 (removing the NumPy pin) already passes this check, so once it is merged I'll merge |
Describe your changes
The shared cache part of #2737, resubmitted on its own now that the Qualcomm SDK scripts are deprecated (the Miniconda pinning from that PR is dropped).
SharedCache._download_blob_listresolves each local destination and raisesValueErrorif a blob name would land outside the intended output directory (names with..segments, absolute or drive paths). Before,blob.name[len(prefix) + 1:]was joined onto the output path without any check.SharedCache.cache_modelstores any local model path as its file or folder name, relative to the cached model folder, instead of only doing so for HF models. This keeps machine specific absolute paths out of the blob names and the cached config, so entries written by the uploader stay downloadable with the check above.OliveCache.get_output_model_idkeeps 16 hex characters of the run config hash instead of 8, which makes ids much harder to collide with on a shared container. Existing local caches simply miss and rerun the pass.Checklist before requesting a review
pytest test/test_cache.py: 39 passed)lintrunner -a(ruff 0.15.11 and pylint 3.3.6 on the changed files)Shared cache downloads reject blob names that escape the output directory, and cache model ids are 16 characters long.
(Optional) Issue link
Addresses #2674