sd-hub

[gutris1]

Info

https://github.com/gutris1/sd-hub

ps: senpai dont be too harsh on the review

Checklist:

• I have read the Readme.md
• The description is written in English.
• The index.json and extension_template.json have not been modified.
• The entry is placed in the extensions directory with the .json file extension.

[w-e-w]

first your description is not informative enough
it needs to be able to get a brief explanation of what the extension does

your current description

an Extension for Stable Diffusion WebUI and Forge. You can Download, Upload, Archive files and that's it.

"an Extension for Stable Diffusion WebUI and Forge"
the first section basically gives no information apart from its compatible with forge
I do not need to know that it's webui extension since it's already on the webui extension index

"You can Download, Upload, Archive files and that's it" and this is not enough
download what files? uploat what ?? archive??
as someone that doesn't know anything about your project I don't know what this means

after reading your readme I'm able to grasp what this is trying to do but still I need to be able to know just by reading read me at least roughly

---

some minor fix

• minor fix to install.py and .gitignore some files gutris1/sd-hub#21

---

MAJOR ISSUE: Arbitrary Code Execution

it should NOT BE allowed to download override files to ANY directories other than the models directory
allowing so it's basically Arbitrary Code Execution

why this is ACE, I can literally upload an extension
I could upload outside webui for example upload to windows startup directory, windows will automatically execute anything in the startup directory
I could you also use the override Behavior to effectively deistory any files

if you wish to perform any potentially dangerous operation then it should be blocked behind
that could result in the uploaded file being executed then it should be block behind shared.cmd_opts.disable_extension_access
this is the same admission check that we use to allow installation of extensions, see https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/04903af798f66877f8af00b778e1696c88e517db/modules/ui_extensions.py#L23-L24

---

less of an issue but still potentially is one in some cases
you might also wish to block archiving as in downloading from webui
some extensions made require a user to configure secrets such as API tokens in a DOT file (hidden file)
these file we're not supposed accessible from the frontend, but would be exposed via your extension
so I would suggest also put this behind shared.cmd_opts.disable_extension_access

[gutris1]

first your description is not informative enough it needs to be able to get a brief explanation of what the extension does

your current description

an Extension for Stable Diffusion WebUI and Forge. You can Download, Upload, Archive files and that's it.

"an Extension for Stable Diffusion WebUI and Forge" the first section basically gives no information apart from its compatible with forge I do not need to know that it's webui extension since it's already on the webui extension index

"You can Download, Upload, Archive files and that's it" and this is not enough download what files? uploat what ?? archive?? as someone that doesn't know anything about your project I don't know what this means

after reading your readme I'm able to grasp what this is trying to do but still I need to be able to know just by reading read me at least roughly

some minor fix

• minor fix to install.py and .gitignore some files gutris1/sd-hub#21

MAJOR ISSUE: Arbitrary Code Execution

it should NOT BE allowed to download override files to ANY directories other than the models directory allowing so it's basically Arbitrary Code Execution

why this is ACE, I can literally upload an extension I could upload outside webui for example upload to windows startup directory, windows will automatically execute anything in the startup directory I could you also use the override Behavior to effectively deistory any files

if you wish to perform any potentially dangerous operation then it should be blocked behind that could result in the uploaded file being executed then it should be block behind shared.cmd_opts.disable_extension_access this is the same admission check that we use to allow installation of extensions, see https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/04903af798f66877f8af00b778e1696c88e517db/modules/ui_extensions.py#L23-L24

less of an issue but still potentially is one in some cases you might also wish to block archiving as in downloading from webui some extensions made require a user to configure secrets such as API tokens in a DOT file (hidden file) these file we're not supposed accessible from the frontend, but would be exposed via your extension so I would suggest also put this behind shared.cmd_opts.disable_extension_access

ugh,, okay
I'll find something to work around that

[gutris1]

I still dont understand about that shared.cmd_opts.disable_extension_access

[w-e-w]

I still dont understand about that shared.cmd_opts.disable_extension_access

https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/04903af798f66877f8af00b778e1696c88e517db/modules/shared_cmd_options.py#L17-L18
if the webui instance is accessible externally (bacause of args like --share --listen)
cmd_opts.disable_extension_access gets set to true unless --enable-insecure-extension-access is also set

when performing sensitive operations such as installing / updateing extetnions
we check for

assert not shared.cmd_opts.disable_extension_access

https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/04903af798f66877f8af00b778e1696c88e517db/modules/ui_extensions.py#L23-L24
if the shared.cmd_opts.disable_extension_access is true raise and assertion exception

[gutris1]

@w-e-w how about current changes. will you agree? https://github.com/gutris1/sd-hub/tree/a

[w-e-w]

I'm not sure what you're trying to do here
like Path is not a string and dose not have a upper method and so Error
and even it is a string why are you checking for WINDOWS in path?

I don't think you get the point
the important thing is that your code can write to "anywhere" which can easily lead to Arbitrary Code Execution
https://en.wikipedia.org/wiki/Arbitrary_code_execution
the extension should NOT be allowed write to any place other than the "safe directories"
as long as the interface is accessible from outside
unless the user specifically understands the risks and allows it

"safe directories" basically means directories containing models (lora dir checkpoint ... etc) directory

[gutris1]

I get your point, thats why Im adding something to prevent the extension touching the WINDOWS folder
maybe I'll just block writing to the outside of webui path then.

[w-e-w]

it is not just the Windows folder and this is not just for Windows this also applies for Linux and Mac
you simply cannot account for all the possible places that could have files being automatically executed by something else
you can only account for certain places that "shouldn't" have files I will be automatically executed for example the models directory

also there's not much point in adding a specific check for Windows directory
as you need Administration permissions in order to write to that directory the first place
(for obvious reasons you should not run webui in admin)

[gutris1]

it is not just the Windows folder and this is not just for Windows this also applies for Linux and Mac you simply cannot account for all the possible places that could have files being automatically executed by something else you can only account for certain places that "shouldn't" have files I will be automatically executed for example the models directory

also there's not much point in adding a specific check for Windows directory as you need Administration permissions in order to write to that directory the first place (for obvious reasons you should not run webui in admin)

yes, thats right
I've added some logic to block downloading files to the outside of data_path or lower level
only allow if equal, but no overwrite
and higher with overwrite true

is that okay?

[w-e-w]

I've added some logic to block downloading files to the outside of data_path or lower level
......
is that okay?

NO
just blocking outside of data_path is NOT enough
it has to be ONLY in specific "safe dir"
there's a crucial difference

data_path is NOT a "safe dir" as it may contain code that will be executed
if you allow creating an inside data_path, then it means that you can literally upload extensions
furthermore if you allow overriding files then then you edit launch configurations
as most users store their launch configurations inside webui root, which is by default the same as data_path

allowing overwriting of files IS fine as long as it's only within safe directories
but the users have to understand that if they allows this then it means that their files that weren't supposed to be touched could be potentially "modified destroy deleted"

preferably it should only allow uploading of certain file types based on the target directory, but this is not essential

---

if you wish your extension to do anything more than
you have to check if the UI is accessible remotely
this can be check by

assert not shared.cmd_opts.disable_extension_access

if this assertions fails then it means that webui UI is accessible remotely and the user has not enabled
--enable-insecure-extension-access

if it's NOT accessible remotely that you can do anything I don't care
but if it IS remotely accessible, then all potentially dangerous operations has to be blocked
unless the user deliberately enables --enable-insecure-extension-access
this overrides this check and allows the user to install extensions remotely, which it is the same as allowing ACE

you either have to only have safe operations, and so whether or not it is remotely accessible it doesn't matter
or add a check before performing dangerous operations, so that it can be blocked when the user is not supposed to be able to do these operations

[gutris1]

I've added some logic to block downloading files to the outside of data_path or lower level
......
is that okay?

NO just blocking outside of data_path is NOT enough it has to be ONLY in specific "safe dir" there's a crucial difference

data_path is NOT a "safe dir" as it may contain code that will be executed if you allow creating an inside data_path, then it means that you can literally upload extensions furthermore if you allow overriding files then then you edit launch configurations as most users store their launch configurations inside webui root, which is by default the same as data_path

allowing overwriting of files IS fine as long as it's only within safe directories but the users have to understand that if they allows this then it means that their files that weren't supposed to be touched could be potentially "modified destroy deleted"

preferably it should only allow uploading of certain file types based on the target directory, but this is not essential

if you wish your extension to do anything more than you have to check if the UI is accessible remotely this can be check by

assert not shared.cmd_opts.disable_extension_access

if this assertions fails then it means that webui UI is accessible remotely and the user has not enabled --enable-insecure-extension-access

if it's NOT accessible remotely that you can do anything I don't care but if it IS remotely accessible, then all potentially dangerous operations has to be blocked unless the user deliberately enables --enable-insecure-extension-access this overrides this check and allows the user to install extensions remotely, which it is the same as allowing ACE

you either have to only have safe operations, and so whether or not it is remotely accessible it doesn't matter or add a check before performing dangerous operations, so that it can be blocked when the user is not supposed to be able to do these operations

is this good now?

[w-e-w]

your CheckPath is broken
gutris1/sd-hub@master...a#diff-3bf206c9a82ba7313fb272477afec45aac7f14b33e8c17d10e4f27567c57b786R72-R80
you're making the assumption that all model dirs (lora checkpoints ... etc) aside from embeddings are in models_path
this is not the case as you are aware
gutris1/sd-hub@master...a#diff-c73011ecc12ee17385dd8c0200bbbbbb5d2801d5595171791fbea6d5d45054d5R20-R29
the user can configure them to any arbitrary path outside of the models directory
example: I have my webui installation and models are stored on different physical drives
you need to check for all the dirs the individually, otherwise your accession would trigger check failed when it shouldn't

---

also please open a different PR for this
sd-simple-dimension-preset.json

[w-e-w]

like I expected there's major security issues with your upload

as long as someone knows the file path ANY file can be uploaded
in this demonstration I input a path to a fake SSH private key and upload it to hugginface



if it's were for real then my ssh key is now on the internet

---

they also seems to be a rather big usability issues in your syntax parseing
in a screenshot above you are splitting the lines into different segments using space but you have never considered that the path itself could contain spaces
example if I have a character lora with filename <first name> <last name>.safetensor

there are multiple ways that you can fix this
one way is to use shlex.split

[w-e-w]

your .sd-hub-token.json should also be stored in your extention and added to .gitignore
and not in stored in webui root

[gutris1]

your .sd-hub-token.json should also be stored in your extention and added to .gitignore and not in stored in webui root

yeah,, I do planning to block the uploader too after Im done with this downloader

[gutris1]

@w-e-w how about now?

[w-e-w]

@w-e-w how about now?

what do you want me to say

[gutris1]

@w-e-w how about now?

what do you want me to say

affff, I forgot about that one.. fixed now

[w-e-w]

another minor fix

• fix no ControlNet error gutris1/sd-hub#22

didn't find more major issues at the moment

---

however your description is no good

An extension for Stable Diffusion WebUI, designed to streamline your collection.\nIt lets you download files from sites like Civitai, Hugging Face, GitHub, and Google Drive, whether individually or in batch.\nYou can also upload files or entire folders to the Hugging Face model repository (with a WRITE token, of course), making sharing and access easier.\nThe extension also has functionality to archive and extract files in formats like tar.lz4, tar.gz, and zip.

your description goes from basically no information to loaded with trivial information

don't just copy you readme, because your target audience is different

in readme you're supposed to give detailed information as these people are seriously considering your extension
and wanting to know more
or they have already downloaded and install extension and wish to know how to use it

but in index people just want a brief understanding of what is this extension about
they're not interested in details such as you support tar.lz4, tar.gz, and zip
they also don't need to know that you need a WRITE token to upload hugging face

basically it's similar to writing your repo "About", with the key difference being that people know that this is an extension for webui are already

• Readme is a document that includes introduction to documentation
• an extension Description is a brief description of what is the use of this extension but not documentation

[gutris1]

another minor fix

• fix no ControlNet error gutris1/sd-hub#22

didn't find more major issues at the moment

however your description is no good

An extension for Stable Diffusion WebUI, designed to streamline your collection.\nIt lets you download files from sites like Civitai, Hugging Face, GitHub, and Google Drive, whether individually or in batch.\nYou can also upload files or entire folders to the Hugging Face model repository (with a WRITE token, of course), making sharing and access easier.\nThe extension also has functionality to archive and extract files in formats like tar.lz4, tar.gz, and zip.

your description goes from basically no information to loaded with trivial information

don't just copy you readme, because your target audience is different

in readme you're supposed to give detailed information as these people are seriously considering your extension and wanting to know more or they have already downloaded and install extension and wish to know how to use it

but in index people just want a brief understanding of what is this extension about they're not interested in details such as you support tar.lz4, tar.gz, and zip they also don't need to know that you need a WRITE token to upload hugging face

basically it's similar to writing your repo "About", with the key difference being that people know that this is an extension for webui are already

• Readme is a document that includes introduction to documentation
• an extension Description is a brief description of what is the use of this extension but not documentation

thanks again.. I think I'm done now