Some tweaks to is_in_or_equal #9020

freddyaboulton · 2024-08-05T23:47:17Z

Description

Consolidates usages of "safe_join" across the code base to use a common util function
Introduces DeveloperPath and UserProvidedPath type aliases to make it clearer what each argument of safe join should be
Adds a test fuzzer for "safe_join"

🎯 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.sh
You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

gradio-pr-bot · 2024-08-05T23:47:58Z

🪼 branch checks and previews

•	Name	Status	URL
	Spaces	ready!	Spaces preview
	Website	ready!	Website preview
	Storybook	ready!	Storybook preview
🦄	Changes	detected!	Details

Install Gradio from this PR

pip install https://gradio-pypi-previews.s3.amazonaws.com/8bc4c39139dd33fac41dce5e469886345407e165/gradio-4.40.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@8bc4c39139dd33fac41dce5e469886345407e165#subdirectory=client/python"

Install Gradio JS Client from this PR

npm install https://gradio-npm-previews.s3.amazonaws.com/8bc4c39139dd33fac41dce5e469886345407e165/gradio-client-1.4.0.tgz

freddyaboulton · 2024-08-05T23:49:24Z

gradio/utils.py

@@ -1466,3 +1459,23 @@ def __len__(self):

    def as_list(self):
        return [v for _, v in self.data]
+
+
+def safe_join(directory: DeveloperPath, path: UserProvidedPath) -> str:


Using these custom types so that in the future user provided paths are not accidentally sanitized before being joined

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * :[

* add max/min-imize and zoom in and out to image preview * add full screen icon to gallery * add stories * add changeset * use native full screen api * remove zoom in/out * add changeset * tweaks * remove zoom prop * fix ui test * add annotated image btns * add changeset * format * ruff format * fix test * remove * tweak * fix test * format * amend bool check --------- Co-authored-by: gradio-pr-bot <[email protected]> Co-authored-by: pngwn <[email protected]>

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * :[ * asd

* asd * asd * asd

* asd * asd * asd * asd

* asd * asd * asd * asd * adsa * asd

* asd * asd * asd * asd * adsa * asd * asd

* better header for mobile * add changeset * nicer border * style header * better search overlay * responsive changes * more mobile responsiveness * docs and guides mobile responsive * formatting * fix get started button * fix header * test without lite * formatting * secondary menu docs * fix type overflow in paramtable * playground header * playground on mobile * small changes * formatting * formatting --------- Co-authored-by: gradio-pr-bot <[email protected]>

* asd * asd * asd * asd * adsa * asd * asd * asd

* asd * asd * asd * asd * adsa * asd * asd * asd * fix

* fix * asd

gradio-pr-bot · 2024-08-06T16:19:15Z

🦄 change detected

This Pull Request includes changes to the following packages.

Package	Version
`gradio`	`patch`

Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Some tweaks to is_in_or_equal

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

Maintainers can change the version label to modify the version bump.
If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

test/test_utils.py

abidlabs · 2024-08-06T19:03:00Z

test/test_utils.py

+    path_2=create_path_string(),
+)
+def test_is_in_or_equal_fuzzer(path_1, path_2):
+    print(path_1, path_2)


Suggested change

print(path_1, path_2)

abidlabs · 2024-08-06T19:19:59Z

gradio/routes.py

-    ]
-
+def routes_safe_join(directory: DeveloperPath, path: UserProvidedPath) -> str:
+    """Safely join the user path to the directory while performing some additional http-related checks."""


Nice this makes sense. Perhaps worth mentioning the checks explicitly here:

Suggested change

"""Safely join the user path to the directory while performing some additional http-related checks."""

"""Safely join the user path to the directory while performing some additional http-related checks, e.g. ensuring that the full path exists on the local file system and is not a directory."""

abidlabs · 2024-08-06T19:20:56Z

gradio/routes.py

@@ -543,7 +559,7 @@ async def file(path_or_url: str, request: fastapi.Request):

            is_dir = abs_path.is_dir()

-            if in_blocklist or is_dir:
+            if is_dir or in_blocklist:


curious why :)

Typo hehe. Originally moved is_blocklist to be a separate check but realized we don't have to do that

abidlabs · 2024-08-06T19:22:32Z

gradio/routes.py

                temp_file.file.close()
                # we need to move the temp file to the cache directory
-                # but that's possibly blocking and we're in an async function
+                # but that's possibl y blocking and we're in an async function


Suggested change

# but that's possibl y blocking and we're in an async function

# but that's possibly blocking and we're in an async function

abidlabs · 2024-08-06T19:26:16Z

test/test_routes.py

+    import tempfile
+    from pathlib import Path


nit: might as well put these imports at the top of the file?

abidlabs · 2024-08-06T19:44:41Z

gradio/utils.py

@@ -1090,7 +1083,7 @@ def is_in_or_equal(path_1: str | Path, path_2: str | Path):
        return ".." not in str(relative_path)
    except ValueError:
        return False
-    return True
+    return False


When is this ever used? It seems like the function should terminate by lines 1083 or 1085 at the latest

Yes this is right - I think we can remove but just kept because I figured it was there for a reason lol

abidlabs · 2024-08-06T19:48:01Z

gradio/utils.py

-
-    if path.is_absolute():
-        return path
+    return Path(os.path.abspath(str(path)))


Ok so the reason we used the convoluted symlink logic is because of this issue: #3024

Basically, we don't want to resolve symlinks but we do want to make them absolute. This fix might work, let me check

Ok so this should work, nice! I would just add a test case in test_utils.py to confirm that symlinks are made absolute but not resolved.

aliabid94 · 2024-08-06T19:55:20Z

gradio/components/file_explorer.py

+    def _safe_join(self, folders: list[str]):
+        if not folders or len(folders) == 0:
+            return self.root_dir
+        head, tail = folders[0], folders[1:]


What's the purpose of splitting head and tail? Why can't we do combined_path = UserProvidedPath(os.path.join(*folders)) directly

It was originally like that but then I ran into errors in the functional tests. I thought it was because os.path.join couldn't handle a single argument but it's actually because the list was empty.

So long story short, I think we can use the line you're suggesting since I added the empty list check right above.

aliabid94 · 2024-08-06T19:57:05Z

gradio/components/file_explorer.py

@@ -85,7 +86,7 @@ def __init__(
            )
            root_dir = root
            self._constructor_args[0]["root_dir"] = root
-        self.root_dir = os.path.abspath(root_dir)
+        self.root_dir = cast(DeveloperPath, os.path.abspath(root_dir))


could we do DeveloperPath(os.path.abspath(root_dir)) instead of using casting?

Also applies to casting in routes.py, can we replace all casting with just directly creating the DeveloperPath with a string?

abidlabs

Awesome PR @freddyaboulton! I went through and reviewed it as thoroughly as I good and everything lgtm. Just a few nits above

aliabid94

LGTM, some small comments but great work freddy

freddyaboulton · 2024-08-06T22:53:07Z

Thanks for the reviews! I've addressed all of the comments

freddyaboulton added 2 commits August 5, 2024 19:32

Add code

cad9775

file_explorer test

0afb222

freddyaboulton commented Aug 5, 2024

View reviewed changes

freddyaboulton and others added 20 commits August 5, 2024 19:52

dont use fastapi

f5014d9

Add code

d6ef179

Update requirements.txt

330de63

Ci security tweaks (#9010)

ad5a358

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd

Ci security tweaks (#9012)

de33f3a

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * :[

add cursor styling (#9003)

7efc36c

Ci security tweaks (#9014)

23d7462

* asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * asd * :[ * asd

asd (#9015)

8b4556a

Ci security tweaks take seventy three (#9016)

e8c8147

* asd * asd * asd

Ci security tweaks take seventy three (#9017)

ebb3a2f

* asd * asd * asd * asd

Ci security tweaks take seventy three (#9018)

2ef2692

* asd * asd * asd * asd * adsa * asd

Ci security tweaks take seventy three (#9019)

081b496

* asd * asd * asd * asd * adsa * asd * asd

Ci security tweaks take seventy three (#9025)

edff9c1

* asd * asd * asd * asd * adsa * asd * asd * asd

Ci security tweaks take seventy three (#9026)

39dde4f

* asd * asd * asd * asd * adsa * asd * asd * asd * fix

asd (#9027)

7982d75

fix (#9028)

b265ded

Ci statuses (#9030)

24cdf81

* fix * asd

Update tests

8bc4c39

freddyaboulton and others added 3 commits August 6, 2024 12:45

merge main

8528e03

add changeset

23d70e9

Add code

4ff205b

freddyaboulton added the v: patch A change that requires a patch release label Aug 6, 2024

freddyaboulton marked this pull request as ready for review August 6, 2024 16:49

add changeset

ebda1e6

freddyaboulton requested review from abidlabs, aliabid94, whitphx, aliabd, dawoodkhan82, pngwn and hannahblair August 6, 2024 17:01

Merge branch 'main' into is-in-or-equal-tweaks

d1737a4

abidlabs reviewed Aug 6, 2024

View reviewed changes

test/test_utils.py Show resolved Hide resolved

abidlabs reviewed Aug 6, 2024

View reviewed changes

aliabid94 reviewed Aug 6, 2024

View reviewed changes

abidlabs approved these changes Aug 6, 2024

View reviewed changes

aliabid94 approved these changes Aug 6, 2024

View reviewed changes

Comments

c3f8fb5

freddyaboulton merged commit 08b5159 into main Aug 6, 2024
18 checks passed

freddyaboulton deleted the is-in-or-equal-tweaks branch August 6, 2024 22:53

gradio-pr-bot mentioned this pull request Aug 6, 2024

chore: update versions #8960

Merged

freddyaboulton mentioned this pull request Sep 12, 2024

Improve is_in_or_equal and fuzzer #9341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some tweaks to is_in_or_equal #9020

Some tweaks to is_in_or_equal #9020

freddyaboulton commented Aug 5, 2024 •

edited

Loading

gradio-pr-bot commented Aug 5, 2024 •

edited

Loading

freddyaboulton Aug 5, 2024

gradio-pr-bot commented Aug 6, 2024 •

edited

Loading

Something isn't right?

abidlabs Aug 6, 2024

abidlabs Aug 6, 2024

abidlabs Aug 6, 2024

freddyaboulton Aug 6, 2024

abidlabs Aug 6, 2024

abidlabs Aug 6, 2024

abidlabs Aug 6, 2024 •

edited

Loading

freddyaboulton Aug 6, 2024

abidlabs Aug 6, 2024

abidlabs Aug 6, 2024

aliabid94 Aug 6, 2024

freddyaboulton Aug 6, 2024

aliabid94 Aug 6, 2024

aliabid94 Aug 6, 2024

abidlabs left a comment

aliabid94 left a comment

freddyaboulton commented Aug 6, 2024

	"""Safely join the user path to the directory while performing some additional http-related checks."""
	"""Safely join the user path to the directory while performing some additional http-related checks, e.g. ensuring that the full path exists on the local file system and is not a directory."""

	# but that's possibl y blocking and we're in an async function
	# but that's possibly blocking and we're in an async function

Some tweaks to is_in_or_equal #9020

Some tweaks to is_in_or_equal #9020

Conversation

freddyaboulton commented Aug 5, 2024 • edited Loading

Description

🎯 PRs Should Target Issues

Tests

gradio-pr-bot commented Aug 5, 2024 • edited Loading

🪼 branch checks and previews

Choose a reason for hiding this comment

gradio-pr-bot commented Aug 6, 2024 • edited Loading

🦄 change detected

This Pull Request includes changes to the following packages.

With the following changelog entry.

Something isn't right?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs left a comment

Choose a reason for hiding this comment

aliabid94 left a comment

Choose a reason for hiding this comment

freddyaboulton commented Aug 6, 2024

freddyaboulton commented Aug 5, 2024 •

edited

Loading

gradio-pr-bot commented Aug 5, 2024 •

edited

Loading

gradio-pr-bot commented Aug 6, 2024 •

edited

Loading

abidlabs Aug 6, 2024 •

edited

Loading