-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Postprocess hardening #9122
Postprocess hardening #9122
Changes from all commits
ed77182
57a8ff6
fa7f9c1
6a44a2d
accb0eb
151bfa0
4e56101
4cc96fe
45e957c
4fdfd67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"gradio": minor | ||
--- | ||
|
||
feat:Postprocess hardening |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -547,44 +547,21 @@ async def file(path_or_url: str, request: fastapi.Request): | |
raise HTTPException(403, f"File not allowed: {path_or_url}.") | ||
|
||
abs_path = utils.abspath(path_or_url) | ||
|
||
in_blocklist = any( | ||
utils.is_in_or_equal(abs_path, blocked_path) | ||
for blocked_path in blocks.blocked_paths | ||
) | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it would be better to set an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea good call. |
||
|
||
created_by_app = False | ||
for temp_file_set in blocks.temp_file_sets: | ||
if abs_path in temp_file_set: | ||
created_by_app = True | ||
break | ||
in_allowlist = any( | ||
utils.is_in_or_equal(abs_path, allowed_path) | ||
for allowed_path in blocks.allowed_paths | ||
) | ||
is_static_file = utils.is_static_file(abs_path) | ||
was_uploaded = utils.is_in_or_equal(abs_path, app.uploaded_file_dir) | ||
is_cached_example = utils.is_in_or_equal( | ||
abs_path, utils.abspath(utils.get_cache_folder()) | ||
) | ||
from gradio.data_classes import _StaticFiles | ||
|
||
if not ( | ||
created_by_app | ||
or in_allowlist | ||
or was_uploaded | ||
or is_cached_example | ||
or is_static_file | ||
): | ||
allowed, _ = utils.is_allowed_file( | ||
abs_path, | ||
blocked_paths=blocks.blocked_paths, | ||
allowed_paths=blocks.allowed_paths | ||
+ [app.uploaded_file_dir, utils.get_cache_folder()] | ||
+ _StaticFiles.all_paths, | ||
) | ||
if not allowed: | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the unit tests, we actually don't raise 404 if the file does not exist |
||
|
||
range_val = request.headers.get("Range", "").strip() | ||
if range_val.startswith("bytes=") and "-" in range_val: | ||
range_val = range_val[6:] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import gradio as gr | ||
|
||
|
||
def test_download_button_sets_origname(): | ||
value = gr.DownloadButton().postprocess("/home/image.png") | ||
assert value.orig_name == "image.png" # type: ignore |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import sys | ||
import warnings | ||
from pathlib import Path | ||
from typing import Sequence | ||
from unittest.mock import MagicMock, patch | ||
|
||
import numpy as np | ||
|
@@ -31,6 +32,7 @@ | |
get_function_params, | ||
get_type_hints, | ||
ipython_check, | ||
is_allowed_file, | ||
is_in_or_equal, | ||
is_special_typed_parameter, | ||
kaggle_check, | ||
|
@@ -376,7 +378,7 @@ def create_path_string(): | |
return st.lists( | ||
st.one_of( | ||
st.text( | ||
alphabet="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-", | ||
alphabet="abcd", | ||
min_size=1, | ||
), | ||
st.just(".."), | ||
|
@@ -387,6 +389,10 @@ def create_path_string(): | |
).map(lambda x: os.path.join(*x)) | ||
|
||
|
||
def create_path_list(): | ||
return st.lists(create_path_string(), min_size=0, max_size=5) | ||
|
||
|
||
def my_check(path_1, path_2): | ||
try: | ||
path_1 = Path(path_1).resolve() | ||
|
@@ -414,6 +420,54 @@ def test_is_in_or_equal_fuzzer(path_1, path_2): | |
pytest.fail(f"Exception raised: {e}") | ||
|
||
|
||
@settings(derandomize=os.getenv("CI") is not None) | ||
@given( | ||
path=create_path_string(), | ||
blocked_paths=create_path_list(), | ||
allowed_paths=create_path_list(), | ||
) | ||
def test_is_allowed_file_fuzzer( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I greatly reduced the cardinality of the allowed possible paths set and also added some corner cases manually in a different test. |
||
path: Path, | ||
blocked_paths: Sequence[Path], | ||
allowed_paths: Sequence[Path], | ||
): | ||
result, reason = is_allowed_file(path, blocked_paths, allowed_paths) | ||
|
||
assert isinstance(result, bool) | ||
assert reason in [ | ||
"in_blocklist", | ||
"allowed", | ||
"not_created_or_allowed", | ||
"created_by_app", | ||
] | ||
|
||
if result: | ||
assert reason == "allowed" | ||
elif reason == "in_blocklist": | ||
assert any(is_in_or_equal(path, blocked_path) for blocked_path in blocked_paths) | ||
elif reason == "not_created_or_allowed": | ||
assert not any( | ||
is_in_or_equal(path, allowed_path) for allowed_path in allowed_paths | ||
) | ||
|
||
if reason == "allowed": | ||
assert any(is_in_or_equal(path, allowed_path) for allowed_path in allowed_paths) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"path,blocked_paths,allowed_paths", | ||
[ | ||
("/a/foo.txt", ["/a"], ["/b"], False), | ||
("/b/foo.txt", ["/a"], ["/b"], True), | ||
("/a/../c/foo.txt", ["/c/"], ["/a/"], False), | ||
("/c/../a/foo.txt", ["/c/"], ["/a/"], True), | ||
("/c/foo.txt", ["/c/"], ["/c/foo.txt"], True), | ||
], | ||
) | ||
def is_allowed_file_corner_cases(path, blocked_paths, allowed_paths, result): | ||
assert is_allowed_file(path, blocked_paths, allowed_paths) == result | ||
|
||
|
||
# Additional test for known edge cases | ||
@pytest.mark.parametrize( | ||
"path_1,path_2,expected", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, why do we set it here and not in the init of Blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this to be done after init for
is_running
to be set. I think actually this is a good place for it.