feat: Refactor save_root handling for safety #16
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.
Normalize and validate save_root to ensure safe directory usage. fix uncontrolled command line issues you must ensure that any user-influenced values passed to
subprocessare either (1) chosen from a hard-coded allowlist, or (2) rigorously validated and normalized before use. It is also important to avoidshell=Trueand building shell command strings when passing user-controlled data.For this case, the untrusted value is
save_root, which contributes totrial_dirand is passed as a command-line argument (--trial-dir) to the watcher process. The best fix that doesn’t change existing functionality is to constrainsave_rootto be a safe, absolute directory path inside a known base directory, and to bail out if the provided value attempts path traversal or is otherwise unsafe. We can do this by introducing a small validation/normalization step inrun: compute a safe base directory (e.g., a default root under the current working directory), resolvesave_rootto an absolute path, and verify that it is contained within that base. If the check fails, we either reject with an exception or fall back to the default. We then use the normalizedsafe_save_rootto constructtrial_dir. This removes the tainted flow while preserving the intended behavior for legitimate inputs.Concretely, in
extern/threestudio/gradio_app.pywithinrun, beforetrial_diris built, we will:pathlib’sPathat the top of the file (this is a standard library module).base_root = Path.cwd() / EXP_ROOT_DIR(or similar).save_rootviaPath(save_root).expanduser().resolve()and verify it is underbase_rootusingrelative_toinside atryblock.safe_save_rootto this resolved path; if it fails, setsafe_save_roottobase_root.safe_save_rootwhen constructingtrial_dirand when passingexp_root_dirto the training command.