fix: resolve HF symlinks in from_hub via temp local_dir - #1495
Conversation
Disha714
changed the title
Fixes generative-computing#1492: snapshot_download with default cache layout returns symlinks into a sibling blobs/ directory, violating from_model_directory contract and tripping the path-escape security check. By passing local_dir to snapshot_download within a TemporaryDirectory context, from_hub resolves genuine files to pass to from_model_directory without weakening any security barriers. Assisted-by: Claude Code Signed-off-by: Disha714 <nsaidisha@gmail.com>
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
Hello, thanks for the contribution!
Overall I think this look good. One test question and I will ping @planetf1 for his 2C since he was the original issue author.
May be worth adding another test where from_hub loads adapters when snapshot_download populates local_dir with real files (as huggingface_hub does with local_dir set).
| try: | ||
| return EmbeddedIntrinsicAdapter.from_model_directory( | ||
| local_root, intrinsic_name=intrinsic_name | ||
| with tempfile.TemporaryDirectory() as local_dir: |
There was a problem hiding this comment.
Cache always cold. local_dir prevents cache_dir from ever being written to; nothing in mellea/ pre-populates io_configs/** in cache mode, so try_to_load_from_cache never hits.
Each call is ~51 HTTP round-trips (repo_info + 25 HEAD + 25 GET) with zero reuse across calls. _resolve_adapter (adapter.py:513) calls this once per distinct intrinsic name — 12 times for a full Switch model.
Measured on ibm-granite/granite-switch-4.1-3b-preview: three consecutive calls took 0.88s / 0.44s / 0.50s.
Hard offline failure: HF_HUB_OFFLINE=1 now raises LocalEntryNotFoundError because a fresh empty temp dir has nothing to read from, and the cached-snapshot fallback at _snapshot_download.py:293 is gated on local_dir is None.
Suggested fix: Use a persistent directory under cache_dir/HF_HUB_CACHE + repo + revision instead. Verified: no symlinks (escape check passes), warm online 0.9s → 0.1s, offline works. Path must include revision to avoid cross-revision collisions.
| assert len(adapters) == 1 | ||
| assert adapters[0].intrinsic_name == "citations" | ||
|
|
||
| def test_from_hub_requests_local_dir(self, model_dir): |
There was a problem hiding this comment.
Test gap: mock returns model_dir (pre-built fixture with real files) — never proves that snapshot_download(..., local_dir=...) actually produces a self-contained directory that passes the escape check.
Suggested: a side_effect-based unit test that materialises a cache-style symlink layout (relative symlinks into a sibling blobs/ dir) when local_dir is None, and real files when it is set. Fails on main, passes on this branch.
| delegates to :meth:`from_model_directory`. | ||
| Downloads `adapter_index.json` and the `io_configs/` directory into a | ||
| self-contained local directory, then delegates to | ||
| :meth:`from_model_directory`. |
There was a problem hiding this comment.
NIT: :meth:from_model_directoryis an RST cross-reference directive. AGENTS.md §5 bans RST markup in docstrings — should be from_model_directory ``. Fix all three occurrences in this docstring (lines 757, 782, 784) or leave for a sweep.
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
The symlink-escape fix is correct, but cache_dir is silently defeated:
- Cache always cold:
local_dirpreventscache_dirfrom ever being written to; each call is ~51 HTTP round-trips with zero reuse - Offline broken:
HF_HUB_OFFLINE=1raisesLocalEntryNotFoundError
Suggested fix: Use a persistent directory under cache_dir/HF_HUB_CACHE + repo + revision instead of TemporaryDirectory(). Verified: restores caching (0.9s → 0.1s warm) and offline operation while keeping the self-contained layout.
Inline comments on test gap and docstring nit follow.
3 participants
Fixes #1492.
Summary of Changes
from_hubdownloads viahuggingface_hub.snapshot_downloadwith the default cache layout, whereio_configs/*/io.yamlentries in the returned snapshot directory are symlinks into a siblingblobs/directory outside the snapshot root. This violatedfrom_model_directoryx27s documented self-contained-directory contract and tripped the path-escape check.from_model_directory, this PR updatesfrom_hubto download files into a temporary self-contained local directory viawith tempfile.TemporaryDirectory() as local_dir:and passinglocal_dir=local_dirtosnapshot_download. This populates the temporary directory with real files rather than symlinks escaping the directory.snapshot_download(downloaded_dir) tofrom_model_directoryso that both runtime operations and mocked test fixtures operate reliably.Verification & Security
test_path_traversal_in_io_config_raises,test_symlink_escape_in_io_config_raises) pass cleanly, confirming that the directory traversal / path-escape defenses remain 100% intact for any malicious input.test_from_hub_requests_local_dirto explicitly verify thatfrom_hubpasseslocal_dirtosnapshot_download. Updated existing test mocks acrossTestFromHubandTestFromSourceto expectlocal_dir=ANY.uv run --extra switch pytest test/backends/test_adapters/test_embedded_adapter.py -v; all 38 tests passed locally without errors or regressions.