From 2d4ab6a723db3d2f1c46a09c3287900c6eca12f3 Mon Sep 17 00:00:00 2001 From: Haoran Geng <71596067+geng-haoran@users.noreply.github.com> Date: Thu, 3 Sep 2026 04:59:20 +0000 Subject: [PATCH] fix(utils): explain CWD / ROBOVERSE_DATA_DIR mismatches in hf_util instead of reporting traversal A roboverse_data/... asset path evaluated from a working directory that is not the parent of ROBOVERSE_DATA_DIR was rejected as a path-traversal attempt. The error now names the working directory, the configured LOCAL_DIR and, when the asset already exists there, its location, and explains that ROBOVERSE_DATA_DIR only changes where downloads are stored. The falls_back_to_private test pins LOCAL_DIR so it no longer depends on the caller's environment. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_017i6VtKoovBNed815mWFqxw --- CHANGELOG.md | 1 + metasim/test/test_hf_util.py | 5 ++++ metasim/test/test_hf_util_local_dir.py | 23 ++++++++++++++++++ metasim/utils/hf_util.py | 32 ++++++++++++++++++++++---- 4 files changed, 57 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd1f825..eb0177d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `RLTaskEnv.step` publishes the *terminal* observation in `info["observations"]["raw"]["obs"]` instead of the episode's first one (off-policy truncation bootstraps in clean_rl SAC/TD3 and FastTD3 read it). - IsaacGym and PyBullet reported `joint_pos_target` in native DoF order while `joint_pos` is in sorted-name order; both now use `get_joint_names(sort=True)` (completes #12). - `ParallelSimWrapper`: a worker that died during handler construction or `launch` surfaced as a bare `EOFError`/`ConnectionResetError` from the handshake and left the other workers running; the handshake now raises the worker's own traceback, `close()` tolerates dead workers, and a failed constructor tears the pool down. +- `hf_util.check_and_download_single`: a `roboverse_data/...` path evaluated from a working directory that is not the parent of `ROBOVERSE_DATA_DIR` was reported as a path-traversal attempt; the error now names the CWD / `ROBOVERSE_DATA_DIR` mismatch and where the asset already is. `test_check_and_download_single_falls_back_to_private_roboverse_data` no longer depends on the caller's `ROBOVERSE_DATA_DIR`. - MuJoCo: `` is reserved by default; humanoid + mesh scenes no longer die with `mj_stackAlloc: out of memory` (get_started/10_mount_camera.py). - `hf_util`: a symlinked `roboverse_data` is no longer refused as path traversal; concurrent diff --git a/metasim/test/test_hf_util.py b/metasim/test/test_hf_util.py index 9a5f88f..4b8ad35 100644 --- a/metasim/test/test_hf_util.py +++ b/metasim/test/test_hf_util.py @@ -2,6 +2,8 @@ from __future__ import annotations +import os + import pytest import metasim.utils.hf_util as hf_util @@ -50,6 +52,9 @@ def fake_file_exists(repo_id, filename, repo_type): def fake_hf_hub_download(repo_id, filename, repo_type, local_dir): download_calls.append((repo_id, filename, repo_type, local_dir)) + # LOCAL_DIR is resolved from ROBOVERSE_DATA_DIR at import time; pin it to the CWD-relative + # default so the test does not depend on the caller's environment. + monkeypatch.setattr(hf_util, "LOCAL_DIR", os.path.abspath("roboverse_data")) monkeypatch.setattr(hf_util.os.path, "exists", lambda path: False) monkeypatch.setattr(hf_util.hf_api, "file_exists", fake_file_exists) monkeypatch.setattr(hf_util, "hf_hub_download", fake_hf_hub_download) diff --git a/metasim/test/test_hf_util_local_dir.py b/metasim/test/test_hf_util_local_dir.py index bb1c0af..254183d 100644 --- a/metasim/test/test_hf_util_local_dir.py +++ b/metasim/test/test_hf_util_local_dir.py @@ -166,3 +166,26 @@ def _fake_find(relpath_posix, is_optional_file=False): hf_util.check_and_download_single(str(real_dir / "robots" / "g1" / "meshes" / "pelvis.STL")) assert "Refusing to fetch" not in str(excinfo.value) assert seen["relpath"] == "robots/g1/meshes/pelvis.STL" + + +@pytest.mark.general +def test_cwd_mismatch_error_names_the_cause(monkeypatch, tmp_path): + """A ``roboverse_data/...`` path evaluated from the wrong CWD must explain the CWD / + ROBOVERSE_DATA_DIR mismatch (and where the asset already is), not accuse the caller of traversal.""" + from metasim.utils import hf_util + + data_dir = tmp_path / "shared_data" + (data_dir / "robots").mkdir(parents=True) + (data_dir / "robots" / "franka.urdf").write_text("", encoding="utf-8") + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + monkeypatch.chdir(elsewhere) + monkeypatch.setattr(hf_util, "LOCAL_DIR", str(data_dir)) + monkeypatch.setattr(hf_util.hf_api, "file_exists", lambda *a, **kw: pytest.fail("HfApi reached")) + + with pytest.raises(ValueError) as excinfo: + hf_util.check_and_download_single(os.path.join("roboverse_data", "robots", "franka.urdf")) + msg = str(excinfo.value) + assert "working directory" in msg and str(elsewhere) in msg + assert str(data_dir / "robots" / "franka.urdf") in msg + assert "ROBOVERSE_DATA_DIR" in msg and "traversal" not in msg diff --git a/metasim/utils/hf_util.py b/metasim/utils/hf_util.py index e22697a..f24d8e4 100644 --- a/metasim/utils/hf_util.py +++ b/metasim/utils/hf_util.py @@ -120,6 +120,33 @@ def extract_texture_paths_from_mdl(mdl_file_path: str) -> list[str]: return texture_paths +_RELATIVE_DATA_PREFIX = "roboverse_data" + os.sep + + +def _outside_local_dir_message(filepath: str, relpath: str) -> str: + """Explain why ``filepath`` cannot be fetched into ``LOCAL_DIR``. + + Asset paths in configs are ``roboverse_data/...`` and are opened by the simulator backends + relative to the *current working directory*; ``ROBOVERSE_DATA_DIR`` only moves where downloads + land. So a relative path that does not resolve under ``LOCAL_DIR`` is almost always a CWD / + ``ROBOVERSE_DATA_DIR`` mismatch, not a malicious ``..`` — say so, and point at the asset when it + already exists in the configured directory. + """ + if not os.path.isabs(filepath) and filepath.startswith(_RELATIVE_DATA_PREFIX): + in_local_dir = os.path.join(LOCAL_DIR, filepath[len(_RELATIVE_DATA_PREFIX) :]) + found = f" The asset exists at {in_local_dir!r}." if os.path.exists(in_local_dir) else "" + return ( + f"Cannot fetch {filepath!r}: asset paths are relative to the working directory " + f"({os.getcwd()!r}), which is not the parent of LOCAL_DIR ({LOCAL_DIR!r}).{found} " + f"Run from the repository root, or symlink ./roboverse_data to LOCAL_DIR " + f"(ROBOVERSE_DATA_DIR only changes where downloads are stored)." + ) + return ( + f"Refusing to fetch {filepath!r}: resolves outside LOCAL_DIR ({LOCAL_DIR}) via relative-path " + f"traversal ({relpath!r})." + ) + + def check_and_download_single(filepath: str): """Check if the file exists in the local directory, and download it from the huggingface dataset if it doesn't exist. @@ -145,10 +172,7 @@ def check_and_download_single(filepath: str): # outside LOCAL_DIR — refuse it rather than send the escaped path # to the HF API. Optional files warn-and-skip; required files raise. if relpath.split(os.sep, 1)[0] == "..": - msg = ( - f"Refusing to fetch {filepath!r}: resolves outside LOCAL_DIR " - f"({LOCAL_DIR}) via relative-path traversal ({relpath!r})." - ) + msg = _outside_local_dir_message(filepath, relpath) if is_optional_file: log.warning(msg + " Skipping optional file.") return