Fix path traversal via metadata file_name in folder-based builders

[Kaif10]

Fix #8324

Vulnerability

The folder-based builders (imagefolder, audiofolder, videofolder, pdffolder) resolve each metadata row's file_name by joining it to the metadata file's directory with no containment check. A malicious dataset can set file_name to a ../-traversal path or an absolute path, causing the builder to open and read files outside the dataset directory when the dataset is loaded — arbitrary file read (CWE-22).

Fix

FolderBasedBuilder._generate_examples now rejects, with a ValueError naming the offending file_name, any value that is absolute or uses .. traversal to escape the directory containing the metadata file. The validation is performed on the relative reference so it applies uniformly to local dataset directories and to the fsspec URLs used when reading from downloaded archives; os.path.realpath / os.path.commonpath were deliberately not used on the joined path because those functions are not fsspec-aware (unlike the streaming-patched os.path.join) and would corrupt archive URLs such as zip://...::.... For genuine local paths, symlinks are additionally resolved and containment is verified with os.path.commonpath. The containment root is the metadata file's own directory, matching the existing resolution base and preserving all legitimate relative references into subdirectories.

Tests

Added regression tests in tests/packaged_modules/test_folder_based_builder.py: ../ traversal rejected (three variants, including subdir/../../ forms), absolute path rejected, and legitimate same-dir / subdir/ / ./subdir/ references still load. These fail on the pre-fix code and pass with the fix. test_folder_based_builder.py (51 passed) and test_imagefolder.py (36 passed).

Follows the containment approach of #8303.

AI assistance (Claude) was used in preparing this change; it was reviewed and verified locally.

[lhoestq]

Thanks for reporting and for the fix, I just have one comment:

[lhoestq]

you can point to local files using file://../.. or local://../.. so this check is maybe not enough

maybe we need to forbid :// altogether ? (except for zip://<file_name>::<valid_relative_path>)

[Kaif10]

You're right — file:// and local:// (and any other fsspec scheme) resolve to local files and slip past the containment check, since the old guard skipped it whenever file_name contained ://. On Linux the scheme also survives normpath, so this is a real bypass.

I've switched to forbidding :// in the file_name value entirely, as you suggested. Legitimate archive loads are unaffected: their URL is zip://<file_name>::<container>, and the scheme is always on the container (downloaded_metadata_dir), never on the file_name itself — so I now key the realpath containment check on the metadata directory rather than the joined path. Added regression tests for file:// / local:// payloads and confirmed the existing zip-archive tests (streaming and non-streaming) still pass.

(Disclosure: I used AI assistance while preparing this change.)

[lhoestq]

Thanks ! I added a few more comments

[lhoestq]

maybe you can have a check on both a chained url/path zip://...::... and a single url/path if you split downloaded_metadata_dir and item on :: to get only the first part before passing to commonpath() ?

we will need to make sure this works for these schemes:

• /path/to/local/metadata_dir
• hf://path/to/remote/metadata_dir
• zip://metadata_dir::/path/to/local
• zip://metadata_dir::hf://path/to/remote

[Kaif10]

Thanks, both addressed.

1. dummy/../../../: this is actually already caught — file_name is run through os.path.normpath before the check, so dummy/../../../etc/passwd normalizes to ../../etc/passwd and is rejected by the existing .. guard. I added explicit regression tests (dummy/../../../etc/passwd, subdir/../../../../outside/secret.txt) so it stays that way.

2. General containment check: done as you suggested — I split both downloaded_metadata_dir and the joined reference on :: and validate the first component, since a relative file_name only ever extends that part (the path inside the archive); the trailing container component(s) must stay identical, which also rejects tampering with the archive path. URL components are normalized with posixpath (os.path rewrites /→\ and collapses // on Windows); genuine local paths still go through realpath + commonpath to resolve symlinks. This now runs for all four metadata-dir forms instead of being skipped for URLs:

• /path/to/local/metadata_dir
• hf://path/to/remote/metadata_dir
• zip://metadata_dir::/path/to/local/archive.zip
• zip://metadata_dir::hf://path/to/remote/archive.zip

Added a test covering all four (legit relative passes, sibling-dir escape and container tampering rejected), and the existing scheme-rejection and zip-archive loading tests still pass.

(Disclosure: I used AI assistance while preparing this change.)

[Kaif10]

Hi @lhoestq — I pushed the revision addressing both points: file_name values containing a scheme (file://, local://, etc.) are now rejected outright, and the containment check runs for every metadata-dir form (local, hf://, and the two zip://…::… chained variants) by splitting on :: and comparing the relevant path component, with dummy/../../../ covered by tests. The scheme-rejection, containment, and existing zip-archive tests all pass. Let me know if there's anything else you'd like adjusted.

[Kaif10]

@lhoestq gentle ping — I pushed the revised version that (1) forbids fsspec schemes in file_name, (2) keeps the dummy/../../../ case covered, and (3) generalizes the containment check across the four metadata-dir forms by splitting on :: (local, hf://, and both zip://…::… chained variants), with tests for each. Does that address your concerns, or is there anything else you'd like changed? Happy to iterate.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[lhoestq]

ah yes dummy/../../../ was already covered indeed, good catch

[lhoestq]

That also means the second check _metadata_reference_is_contained is maybe not useful. I see it doesn't impact the tests at least.

[Kaif10]

Good question on the second check — I dug into it. The _metadata_reference_is_contained realpath check does guard against a case the string checks can't: a symlink escape. If a subdirectory of the dataset is a symlink pointing outside the dataset root, a file_name like evil_link/secret.txt looks like a perfectly innocent relative path — no scheme, not absolute, no .. — so the string checks pass it, but it resolves to a file outside the dataset. Only the realpath/commonpath containment check catches that.

I confirmed it end-to-end: with the containment check in place the reference is rejected ("the resolved path escapes the dataset directory"); with it removed, the escape succeeds and the read resolves to the file outside the dataset dir. I've added a regression test (test_data_files_with_metadata_symlink_escape_is_rejected) that asserts on that specific behavior, so it fails if the check is ever dropped. It creates a real symlink and skips where symlink creation needs privileges (e.g. Windows without Developer Mode), so it still runs on CI.

You were right that the earlier dummy/../../../ case is already covered by the .. check — the containment check earns its keep specifically on symlinks.

[lhoestq]

I see, thx ! I'll merge the PR without syminks for now to keep things simple and since the TAR and RAR extractor are fixed to not allow symlinks already, ZIP in python simply doesn't support them at all, and 7z in python natively disallow them. Happy to discuss this further in a separate issue though