Repository navigation
Harden shared cache downloads and pin the Miniconda installer - #2737
Open
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 3 commits into
Open
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 3 commits into
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 3 commits into
Conversation
- Reject shared cache blobs whose names resolve outside of the local output directory (for example names containing ".." segments or absolute paths) instead of writing them wherever the path points. - Widen the output model id derived from the run config hash from 8 to 16 hex characters so cache entries are much harder to collide with. - Download a pinned Miniconda release in the Qualcomm python env setup scripts and verify its published SHA-256 before running it, instead of executing the moving "latest" installer unchecked. Addresses microsoft#2674
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The containment check rejects absolute-path ONNX entries produced by the existing shared-cache uploader.
1 open finding
What changed in this PR
Hardens Olive’s shared cache and Qualcomm environment setup, addressing part of #2674.
Changes:
- Rejects blob destinations outside the download directory.
- Expands output model IDs to 16 hexadecimal characters.
- Pins Miniconda installers and verifies their SHA-256 checksums.
| File | Description |
|---|---|
| test/test_cache.py | Tests valid downloads and path traversal rejection. |
| olive/platform_sdk/qualcomm/create_python_env.sh | Pins and verifies the Linux installer. |
| olive/platform_sdk/qualcomm/create_python_env.ps1 | Pins and verifies the Windows installer. |
| olive/cache.py | Adds download containment checks and widens model IDs. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
@microsoft-github-policy-service agree |
cache_model kept absolute model paths for non-HF models, so an ONNX model at /tmp/model.onnx was uploaded as <id>/model/model//tmp/model.onnx and its cached config pointed at the producer machine path. The new download containment check rejects those names. Upload any local model file or folder under its name instead, so blob names and the cached config are relative to the cached model folder, and add an upload/download round-trip test.
5 tasks
The new _download_blob_list tests call the private helper directly, which PyLint flags as protected-access (W0212) in the lint CI job.
Jambay Kinley (jambayk)
previously approved these changes
Oct 30, 2026
Jambay Kinley (jambayk)
dismissed
their stale review
October 30, 2026 01:44
The copied PR has lint issues and CI didn't pass
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Hardening for the shared cache and the Qualcomm python env setup, following the suggestions in #2674.
Shared cache
SharedCache._download_blob_listnow resolves 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.OliveCache.get_output_model_idnow keeps 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.Miniconda installer
create_python_env.shandcreate_python_env.ps1download a pinned release (Miniconda3-py312_26.7.1-1) and verify its SHA-256 (from https://repo.anaconda.com/miniconda/) before executing it, instead of running the movingMiniconda3-latestinstaller unchecked. The.shscript also switches from the legacyrepo.continuum.iohost torepo.anaconda.com. Users can still pass their own installer withCONDA_INSTALLER.Not included: binding cached
model.jsonconfigs to the run that produced them. That needs a design decision on what to store next to the blob, so I left it for a follow-up.Checklist before requesting a review
pytest test/test_cache.py test/engine: all pass excepttest_plot_pareto_frontier, which also fails onmainin my env because plotly is not installed)lintrunner -a(ruff 0.15.11 check and format on changed Python files;bash -nand the PowerShell parser on the scripts)Shared cache downloads reject blob names that escape the output directory, cache model ids are 16 characters long, and the Qualcomm env setup verifies a pinned Miniconda installer.
(Optional) Issue link
Addresses #2674