Repository navigation
Fix external data validation for Hugging Face Hub shared blob store - #34099
Open
Tianlei Wu (tianleiwu) wants to merge 1 commit into
Open
Tianlei Wu (tianleiwu) wants to merge 1 commit into
Tianlei Wu (tianleiwu) wants to merge 1 commit into
Conversation
huggingface_hub >= 1.32 stores Xet files once per cache in <cache>/blobs/<hash prefix>/<hash> and makes the repo's blobs/<etag> a symlink to it. A model and its external data in a snapshot can then resolve into different folders, so the symlink fallback, which only accepts data next to the resolved model, fails with "External data path escapes model directory". Also accept external data of a symlinked model in a snapshot of a Hugging Face Hub cache repo when the data path stays under the model directory before symlinks are resolved and both the model and the data resolve into the blob stores of that cache (the repo's blobs/ folder or the shared blobs/ folder). Apply the same check to the opened file. Fixes #33950 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The fallback expands external-data access into cache-wide blob stores, requiring human security review of cache-layout trust assumptions and isolation between repositories.
No open issues 🎉
What changed in this PR
This PR addresses #33950 by extending ONNX Runtime’s external-data validation to support Hugging Face Hub’s shared blob store, where a model and its weights can resolve into different directories.
Changes:
- Adds cache-aware fallback checks to both path validation and opened-file validation, preserving the existing validation passes.
- Adds C++ tests for shared and mixed blob layouts, nested snapshots, and rejected paths.
- Adds Python end-to-end inference coverage for shared-store and mixed repo-local/shared layouts.
No actionable findings were identified.
| File | Description |
|---|---|
onnxruntime/test/python/onnxruntime_test_python_symlink_data.py |
Adds synthetic cache helpers and end-to-end inference tests. |
onnxruntime/test/framework/tensorutils_test.cc |
Adds cache-layout acceptance, rejection, and external-data loading tests. |
onnxruntime/core/framework/tensorprotoutils.cc |
Recognizes cache blob stores in pre-open and opened-file validation. |
Review effort: Balanced
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.
Description
Since
huggingface_hub1.32, the Hugging Face Hub cache stores Xet files once per cache in a blob store shared across repos (<cache>/blobs/<hash prefix>/<hash>), and the repo'sblobs/<etag>becomes a relative symlink to it. A model and its external data in one snapshot can then resolve into different folders (for exampleblobs/28/…andblobs/d4/…, or one in the repo'sblobs/and one in the shared store). The symlink fallback from #27374 only accepts external data next to the resolved model, so models with external data loaded from a defaulthf_hub_downloadpath fail withExternal data path escapes model directory. This PR teaches both external data validators about the shared store without trusting anything outside the cache's blob folders.Summary of Changes
Validation (
onnxruntime/core/framework/tensorprotoutils.cc)New Pass 3 in
ValidateExternalDataPath, reached only when the model is a symlink and Passes 1–2 fail. If the canonical model directory is in a snapshot of a cache repo (<cache>/<type>s--<repo id>/snapshots/<revision>/…, also from a subfolder such asonnx/), external data is accepted when:..escape), andblobs/folder or the cache-wideblobs/folder.ValidateOpenedExternalDataPathapplies the same blob-store check to the canonical path of the opened file, so the opened-file validation from #33063 also covers the new case. The cost is one extra canonicalization, and only after the existing passes fail.onnxruntime/core/framework/tensorprotoutils.ccIsInHuggingFaceCacheBlobStoresand use it in both validators.onnxruntime/test/framework/tensorutils_test.ccHuggingFaceCachePathValidationTest: 11 tests on a cache built likehuggingface_hubdoes, with two-hop relative symlinks (6 accepted layouts, 5 rejected).onnxruntime/test/python/onnxruntime_test_python_symlink_data.pyInferenceSessiontest for the shared store and for a mixed repo-local/shared layout.Security notes
<cache>/blobsis stored in that real folder. If ablobs/folder is itself a symlink or junction, nothing matches (huggingface_hubalso never uses a symlinkedblobs/folder as the shared store).symlink/..tricks in the model path...requirement keeps a malicious model from reaching other cached repos' files. In a real cache, the entries of a snapshot folder are symlinks thathuggingface_hubcreates to files of the same repo.huggingface_hubalso gives the store and shard folders the same permissions as the cache root.Comparison with #33974
#33974 targets the same issue. It also checks the
.huggingface-shared-blobsmarker (version1) and blob names (hex, 40/64 characters, 2-character shard), and requires amodels--repo folder. These checks are not security boundaries, since anyone who can create the layout can also create the marker and names. They do tie ORT tohuggingface_hubinternals, so a future layout change would break loading again. I built both and loaded the same synthetic caches:datasets--…repo1../x; data symlink outside the cache; symlinkedblobs/Motivation and Context
Fixes #33950. Downstream projects currently work around it by pinning
huggingface_hub<=1.31, settingHF_HUB_DISABLE_SHARED_BLOBS=1, or copying files out of the cache.Testing
onnxruntime==1.31.1andhuggingface_hub==2.4.0(onnx-community/sam2.1-hiera-tiny-ONNX, freshHF_HOME). With this PR, the same cache loads. The two mixed layouts created by downloading one of the files withHF_HUB_DISABLE_SHARED_BLOBS=1also fail with 1.31.1 and load with this PR.onnxruntime_test_all --gtest_filter='HuggingFaceCachePathValidationTest.*:PathValidationTest.*:GetExtDataFromTensorProtoTest.*': all pass. Without thetensorprotoutils.ccchange, the 6 tests for accepted layouts fail. Removing any one of the conditions (no..escape, model in a blob store,blobs/folder bound, repo folder name, symlinkedblobs/, opened-file check) makes the targeted test fail.python -m pytest onnxruntime/test/python/onnxruntime_test_python_symlink_data.py: 4 passed (plus 2 subtests).onnxruntime_test_allon Linux x64 (CPU, Release): 2897 passed.lintrunner: no issues.Checklist