Is the goal of this to have privacy by deleting images after loading?

[shawnington]

If it is, and you look at my last commit, in the server.py there is the code the deals with the image saving. I included code to stop images from being duplicated if they already exists in the database, this can be extended to have an option not to save the image at all.

Comfy commit #3472

I implemented basic deduplication using a hash comparison so duplicate images are not saved, but since it's already hooking the function that determines if an image is saved or not.

It already has an unimplemented option for overwriting an image, that is not implemented, although I didn't look at how that option is intended to be injected, but it can definitely be extended to have a do not save image flag.

Also most of the UI stuff is controlled in app.js calling functions in server.py

The IS_CHANGED functions is hooked for all nodes that have it in execution.py

You can follow the execution path from my commit and figure it out.

If your intention is to have privacy by deleting the image after use, the issue with your implementation is it allows saving of the image, and a very simple script can monitor the server directory and copy new files saved before they can be deleted. So it's not really privacy, as the image can be duplicated before you have a chance to remove it in theory. You should be preventing the image from being saved in the first place, as it doesn't actually impact the function of any workflows, as once an image is loaded in the LoadImage node, the image is passed along as a tensor from there.

Here is how I prevent saving:

if not image_is_duplicate: 
    if image_save_function is not None: 
        image_save_function(image, post, filepath)
    else:
        with open(filepath, "wb") as f:
        f.write(image.file.read())

you can extend this with a flag like "disable_saving"

if not image_is_duplicate or disable_saving: 
    if image_save_function is not None: 
        image_save_function(image, post, filepath)
    else:
        with open(filepath, "wb") as f:
        f.write(image.file.read())

As can be seen from the code, the function also takes custom image_save_function as an input, so you can hook through that potentially to disable saving by just injecting an image save function that does nothing.

I do however really like this idea, especially for servers having the option to not have the images or anything saved or cached for privacy is a good step, especially with the exploits that are starting to emerge, the potential for people to save malicious data is a real problem.

Good work @liusida @rizlas

[liusida]

Thanks, @shawnington.

Yeah, I was thinking about only keeping the uploaded image in memory without saving it. However, if I understand correctly, the /upload/image and the /prompt are two seperate requests to the server, meaning the /upload/image request doesn't know which node is related.

I think there are two options to achieve the goal of "upload->process without saving":

1. When uploading, also submit some metadata about the parent node and keep a variable for that. When processing, check that variable and get the image from memory.

2. Only show the image on GUI when choosing file to upload, and attach the whole image again when submitting the prompt.

Either modification is not trivial, so we need to discuss this first.

[liusida]

I'm definitely onto something. Now I am implementing this:

Encrypt the image using a randomly generated key in JavaScript before upload. Upload it. Pass the key to the server along with other prompt. Decode the image file and use it on server side, Never save the key to the disk. Remove the encrypted file after use.

This will be much safer then passing the raw image file via internet.

[shawnington]

I definitely onto something. Now I am implementing this:

Encrypt the image using JavaScript before upload. Upload it. Pass the key to the server along with other prompt. Decode the image file and use it on server side, Never save the key to the disk. Remove the encrypted file after use.

This will be much safer then passing the raw image file via internet.

I was considering encrypting the path before it gets to the server, but wondered if that would throw some kind of error so didnt suggest it, I like the thought process though. AI run local should be anonymous, and not offer training data. The beauty of FOSS is that I can fix a bug in comfy that you had, and you can create a node that does something I will use. I personally host a server that my friends use for generation, and not having to clean up the input folder besides being great for user privacy, is nice from an administrative side also.

What actually got me to commit to the input deduplication commit, if it gets merged or not, was that shockingly... and i say that ironically, my friends were making memes of the same images over and over so i had the same image duplicated 1000 times in my server.

Encryption and not having server side saving that is duplicated is great.

If its the same image encrypted 1000 times, great, thats secure, just as long as i cant tell what the current meme is from the inputs folder of my server.

[liusida]

I personally host a server that my friends use for generation, and not having to clean up the input folder besides being great for user privacy, is nice from an administrative side also.

Me, too.

And after the latest commit dae19dd, now we don't need to clean the input folder anymore. Just connect another "Remove" node, so the encrypted image will be deleted after a prompt is executed.

And I randomly generated the filename as well, just in case people upload something like "naked me.jpg". XD

[shawnington]

I personally host a server that my friends use for generation, and not having to clean up the input folder besides being great for user privacy, is nice from an administrative side also.

Me, too.

And after the latest commit dae19dd, now we don't need to clean the input folder anymore. Just connect another "Remove" node, so the encrypted image will be deleted after a prompt is executed.

And I randomly generated the filename as well, just in case people upload something like "naked me.jpg". XD

Slow clap for your sir, I just reviewed the code, and for someone that professed to have no experience with js a few days ago, consider me very impressed. It appears secure in a way I cant see any errors with on a cursory review, and I look forward to what else you will contribute to the community, probably much more than myself!

I am however tempted to make a pull request swapping all your const for lets, and all your lets for costs since they are the same thing in js, and nobody knows why they both exists.

Im kidding of course. Good work though. I will give the code a more thorough review in the morning and see if I spot any security holes, but nothing obvious is apparent on first reviews, so again, good job sir.

[shawnington]

I personally host a server that my friends use for generation, and not having to clean up the input folder besides being great for user privacy, is nice from an administrative side also.

Me, too. And after the latest commit dae19dd, now we don't need to clean the input folder anymore. Just connect another "Remove" node, so the encrypted image will be deleted after a prompt is executed. And I randomly generated the filename as well, just in case people upload something like "naked me.jpg". XD

Slow clap for your sir, I just reviewed the code, and for someone that professed to have no experience with js a few days ago, consider me very impressed. It appears secure in a way I cant see any errors with on a cursory review, and I look forward to what else you will contribute to the community, probably much more than myself!

I am however tempted to make a pull request swapping all your const for lets, and all your lets for costs since they are the same thing in js, and nobody knows why they both exists.

Im kidding of course. Good work though. I will give the code a more thorough review in the morning and see if I spot any security holes, but nothing obvious is apparent on first reviews, so again, good job sir.

So the only thing that popped to my head after posting is this. Is there a way to make the remove node merged with the load node based in on a launch flag, so if there is a flag --privacy-mode, the load image node defaults to the connection with that node and the privacy features for server use and not user use.

It would be great to be able to make it the default and only node behavior server side for load image.

Also, have you noticed that the VAE sometimes encodes an image very well almost lossless, but other times encodes it in a loony toons level or fun house mirror not even resembling the original image level or distortion?

It makes me slightly worried when ever I read a paper and the us the SDXL Vae.

Again, Im about to head to bed, so I dont consider my comments a comprehensive code review of any kind.

[liusida]

I am however tempted to make a pull request swapping all your const for lets, and all your lets for costs since they are the same thing in js, and nobody knows why they both exists.

Im kidding of course.

You've got me! xD

And for those who don't know Javascript, here is ChatGPT's response:

No, that's not true. In JavaScript, `const` and `let` have different purposes:

`const` declares a variable that cannot be reassigned after its initial assignment. However, if the variable is an object or array, its contents can still be modified.
`let` declares a block-scoped variable that can be reassigned.

They exist to provide more control over variable behavior and scope in your code.

[liusida]

So the only thing that popped to my head after posting is this. Is there a way to make the remove node merged with the load node based in on a launch flag, so if there is a flag --privacy-mode, the load image node defaults to the connection with that node and the privacy features for server use and not user use.

This idea actually makes sens. Why another "Remove" node while the file can be removed in "Load" node immediately after reading it to memory.

[liusida]

e4eb70b remove "remove" node entirely.

[liusida]

[shawnington]

e4eb70b remove "remove" node entirely.

Good work, another though, since the filename is now being randomly generated, is there any need to have the display for the filename that is used to select from saved images in the node anymore? Im assuming you can no longer re-select files that were not saved.

Im assuming its not actually used to store anything useful, so just deleting the input from required in the node should do it or is it still storing encryption data you need?

I guess if you implement saving the image in an encrypted format, then for individual users they can still access serve side saved encrypted images that are decrypted on their side. In that case decrypting the file name for display to the user shouldn't hurt as the code can be run locally.

So theoretical ideal behavior, flag to disable saving all together for the server side.

Toggle to enable disable saving encrypted images for the user that shows up conditionally based on the flag being set or not (not sure if nodes can conditionally display option toggles)

so --privacy_mode_enabled or whatever no toggle shows up, default server wont save anything.

flag not set, the node shows an enable saving encrypted image toggle that defaults to disabled.

Another interesting direction would be an option to encrypt models or their name in the workflow data included in the png meta data, or modify the save image node in a way to where you that information can also be encrypted to only work with the users key. Definitely encrypt prompts.

[liusida]

I tried to hide the "image" COMBO (filename text input) yesterday, but unsuccessfully. Need some help on that.

And yes, I think the behavior can be improved. I'll come back to this issue.

[liusida]

I tried to hide the "image" COMBO (filename text input) yesterday, but unsuccessfully. Need some help on that.

And yes, I think the behavior can be improved. I'll come back to this issue.

ok, I disabled the "image" COMBO.