Postprocess hardening #9122
Postprocess hardening #9122
Conversation
🪼 branch checks and previews
|
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
freddyaboulton
requested review from
aliabid94,
pngwn,
abidlabs,
aliabd,
dawoodkhan82 and
hannahblair
and removed request for
pngwn
| raise HTTPException(403, f"File not allowed: {path_or_url}.") | ||
|
|
||
| if not abs_path.exists(): | ||
| raise HTTPException(404, f"File not found: {path_or_url}.") |
There was a problem hiding this comment.
According to the unit tests, we actually don't raise 404 if the file does not exist
| @@ -1910,6 +1909,10 @@ async def process_api( | |||
| state_ids_to_track, hashed_values = self.get_state_ids_to_track(block_fn, state) | |||
| changed_state_ids = [] | |||
|
|
|||
| from gradio.context import LocalContext | |||
|
|
|||
| LocalContext.blocks.set(self) | |||
There was a problem hiding this comment.
Interesting, why do we set it here and not in the init of Blocks?
There was a problem hiding this comment.
I thought this would be more "future proof" as it's the most up to date version of the Blocks before any sort of processing happens. But I don't think we need this actually.
There was a problem hiding this comment.
We need this to be done after init for is_running to be set. I think actually this is a good place for it.
gradio/processing_utils.py
Outdated
| if blocks is None or not blocks.is_running: | ||
| return | ||
|
|
||
| import tempfile |
There was a problem hiding this comment.
suggest putting this import at top of file
gradio/processing_utils.py
Outdated
| msg += "located in either the current working directory or your system's temp directory. " | ||
| msg += "To fix this error, please ensure your function returns files located in either " | ||
| msg += f"the current working directory ({os.getcwd()}), your system's temp directory ({tempfile.gettempdir()}) " | ||
| msg += f"or add {str(abs_path.parent)} to the allowed_paths parameter." |
There was a problem hiding this comment.
I would add something like, "if you'd like to specifically allow this file to be served, you can add it to the allowed_paths parameter of launch()"
gradio/processing_utils.py
Outdated
| ): | ||
| raise InvalidPathError( | ||
| "Dotfiles located in the temporary directory cannot be moved to the cache for security reasons. " | ||
| "Please check whether this file is valid and rename it if so." |
| allowed_paths=create_path_list(), | ||
| file_sets=st.lists(create_path_set(), min_size=0, max_size=3), | ||
| ) | ||
| def test_is_allowed_file_fuzzer( |
There was a problem hiding this comment.
Cool idea to use the fuzzer here. But if I'm understanding correctly, randomly generating paths is very, very unlikely to produce any overlaps between the parameters, right? So this is actually unlikely to test what happens if a path is in blocked_paths or allowed_paths or file_sets? Perhaps part of the path should be manually specified or the values should be chosen from a set with smaller cardinality
There was a problem hiding this comment.
I greatly reduced the cardinality of the allowed possible paths set and also added some corner cases manually in a different test.
| is_dir = abs_path.is_dir() | ||
|
|
||
| if is_dir or in_blocklist: | ||
| if abs_path.is_dir() or not abs_path.exists(): | ||
| raise HTTPException(403, f"File not allowed: {path_or_url}.") |
There was a problem hiding this comment.
nit: it would be better to set an allowed flag here, and raise a single 403 later, after the utils.is_allowed_file has completed so that malicious attackers cannot using timing attacks to infer whether a particular filepath even exists (that itself can be insecure in certain settings)
There was a problem hiding this comment.
Yea good call.
|
Note: I believe the |
|
Left some comments but overall this works great. Good stuff @freddyaboulton, will go ahead and approve |
Description
Ensures only files located in the current working directory or tempdir are able to be moved to the cache
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Tests
PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests:
bash scripts/run_all_tests.shYou may need to run the linters:
bash scripts/format_backend.shandbash scripts/format_frontend.sh