Repository navigation
Conversation
fetch wrote each download to a predictable <target>.part in a /tmp directory it never checked, so another local user could plant a symlink there and have curl overwrite its target. Downloads now go only into a directory owned by the caller with no group or other write bit (created 0700), through an mkstemp file moved into place with os.replace and removed on failure. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 of 2 tasks
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.
Closes #265
Summary
The dataset builders download their non-Hub files into
--raw, which defaults to/tmp/devtools-rawand/tmp/breadth-raw(finding 1 of a security scan of 2074cc4).fetchwrote each download to the predictable<target>.partwithcurl -o, in a directory it created withexist_ok=Trueand never checked. On a machine shared with other local users, someone could create the directory first and plant<target>.partas a symlink. curl follows it and overwrites the symlink's target with the download, with the builder's permissions.1, downloads could follow a planted symlink in a directory someone else controls.
fetchnow does two things:chmod go-w <dir>, or--rawpointing at a directory of your own.mkstemp, which has a random name and is created exclusively, so nothing planted beside the target is ever opened. The file is moved into place withos.replaceand removed if the download fails.mainchecks--rawbefore its first use. That also protects subdirectories such asflakeflagger/: in a directory only you can write, no one else can create or rename entries.The
/tmp/*-rawdefaults stay, so caches from earlier runs (created by you with mode 0755) still work and nothing is downloaded again. Downloaded bytes are checked against the same pins as before, so--reproduce-v1and breadth-v1's byte-for-byte rebuild are unaffected.Behavior relative to main
--rawmissing--rawowned by another user, writable by group or others, or a symlink<target>.part: predictable, follows symlinksmkstempfile in the checked directory, thenos.replace.partleft behindRed / green
tests/test_devtools_v1.py::test_fetch_downloads_through_a_private_temp_file: adata.bin.partsymlink planted next to the target, pointing at a victim file. On main curl writes through it onto the victim. Here the victim and the symlink are untouched, the target has the right bytes and hash, and no temporary file remains.test_fetch_refuses_a_writable_directory: a 0777 download directory is refused before anything is fetched.test_private_dir_accepts_owned_dirs_and_creates_missing: a missing directory is created 0700, an existing 0755 directory you own passes, and a symlink to it is refused.test_fetch_a_failed_download_leaves_nothing: a failing curl leaves neither the temporary file nor the target.Verification
uv run --extra serve python -m pytest tests/test_devtools_v1.py tests/test_breadth_v1.py -q: 82 passed, 1 skipped (macOS).scripts/build_devtools_v1.py --helpandscripts/build_breadth_v1.py --helprun.Merge Danger
--rawdirectory that is group-writable (for example one created under umask 002) is now refused;chmod go-wit. A symlinked--rawis refused too; pass the real path.Not in this PR: the FlakeFlagger GitHub tarball is still fetched without a sha256 pin (its hash is recorded in the manifest after the build). With the directory private, only you can place a file there. Pinning it is a separate decision, because GitHub's codeload archives are not promised to stay byte-identical.
Checklist
tests/test_devtools_v1.pyandtests/test_breadth_v1.pypass locally.--rawrequirement is noted in AGENTS.md.Generated with Devin