-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Block /file=
filepaths that could expose credentials on Windows
#7444
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/24bd0909a1904ebdb756087ed4f5ae8c47212477/gradio-4.19.0-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@24bd0909a1904ebdb756087ed4f5ae8c47212477#subdirectory=client/python" |
🦄 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.
|
@@ -720,25 +726,6 @@ def test_orjson_serialization(): | |||
demo.close() | |||
|
|||
|
|||
def test_file_route_does_not_allow_dot_paths(tmp_path): |
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 don't need a dedicated test suite for this since this is no longer a special case (we simply block all files in the working directory)
@@ -428,12 +428,18 @@ async def file(path_or_url: str, request: fastapi.Request): | |||
return RedirectResponse( | |||
url=path_or_url, status_code=status.HTTP_302_FOUND | |||
) | |||
|
|||
invalid_prefixes = ["//", "file://", "ftp://", "sftp://", "smb://"] |
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 believe checking whether or not abs_path
exists will also fix the issue and it doesn't involve hardcoding this list of prefixes?
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.
So I'm not sure totally sure but it seems like any file operation on Windows could leak credentials
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.
Yikes
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.
Do we care about ipfs urls here too?
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.
Probably so I don't know that much about them, but let me rewrite them to prevent any protocols except for http/https
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.
Looks good @abidlabs !
…ls on Windows) more general (#7453) * test routes * chagne * add changeset * add changeset * type fixes * fix typing issues * typed dict --------- Co-authored-by: gradio-pr-bot <gradio-pr-bot@users.noreply.github.com>
No description provided.