search

Huluti / Curtail

Simple & useful image compressor
GNU General Public License v3.0
366 stars 39 forks source link

Curtail 1.11.0 can no longer compress any file #250

Closed archerallstars closed 3 weeks ago

archerallstars commented 3 weeks ago

I tried compressing PNG, JPEG, and WebP files without success. The app processes the files indefinitely.

The app worked well before version 1.11.0.

Debug info:

Python: 3.12.6

Gtk: 4.16.2

Jpegoptim: 1.5.5

Oxipng: 9.1.1

pngquant: 2.18.0

Libwebp: 1.4.0

Scour: 0.38.2
Huluti commented 3 weeks ago

Can reproduce indeed with filenames with spaces. Will try to publish a fix asap Thanks!

josec87 commented 3 weeks ago

Not only spaces. Filenames with special characters (?, ñ, ó...) don't work either.

ARAKHN1D commented 3 weeks ago

Did some quick testing, and it looks like #249 (which I opened for another issue) also fixes this one.

Huluti commented 3 weeks ago

Hmm that's strange that it fixes the issues... because the issues comes from this commit: https://github.com/Huluti/Curtail/commit/6f0774874bfd51ff445e2f9965e87e14c676d53b

I'm working on an other way to fix the vulnerability and fix this current issue.

ARAKHN1D commented 3 weeks ago

Ah, it actually doesn't fix it. I didn't have that commit in my local repo. I'll look into some potential solutions too.

Huluti commented 3 weeks ago

My thought is that we should remove the quote function, remove shell=True from the subprocess.run call and so pass command params as array and not a string. But it requires a bit of work...

ARAKHN1D commented 3 weeks ago

Does Curtail already protect against shell injections since it puts double quotes around the filenames in the commands? I will admit I'm not very experienced in this area, so I'm not sure. Removing shell=True seems like it would definitely protect against this though, so that's probably the better option.

ARAKHN1D commented 3 weeks ago

There's one issue with removing shell=True: you can't run multiple commands at once with this. That means that, afaik, there'll have to be another subprocess.run() when compressing a JPEG in safe mode (because of the > in the command), and when compressing a SVG in overwrite mode (because of the && in the command).

Edit: this would also affect lossy compression of PNGs.

ARAKHN1D commented 3 weeks ago

Made a MR (#251) that'll hopefully solve both issues.

Huluti commented 3 weeks ago

Tested it successfully. Release in progress: https://github.com/flathub/com.github.huluti.Curtail/pull/17 Many thanks again for your precious help.