fix lib/dtutils/system/open_default_app() #172
Conversation
| end | ||
| return dtutils_system.external_command(open_cmd..' "'..path..'"') | ||
| return dtutils_system.external_command(open_cmd .. path) |
There was a problem hiding this comment.
@wpferguson path is not sanitized, this breaks Linux version when there is space in the path
There was a problem hiding this comment.
The caller should send a sanitized path to the function, rather than the function assuming the path isn't sanitized. Also windows and linux/macos have different quoting requirements. lib/dtutils/file/sanitize_filename() is provided for that purpose. The author of the script calling the function should know whether the path being passed needs to be sanitized or not and pass the argument appropriately. Or, they can be safe and sanitize it anyway.
Think of the libraries as little black boxes. There is documentation about what the input should be and what output is expected. So, I decide to use this function. I pass it a path and sanitize it because it includes a space. Unknown to me, unless I went in an looked at the code, the function attempts to sanitize my sanitized string. The quotes cancel out and the command crashes because the string wasn't sanitized. I would be very confused if this happened, because I knew that I had passed a sanitized string and my script crashed because the string wasn't sanitized. And the solution wouldn't really make sense because I would have to not sanitize my string in order for it to be sanitized.
There was a problem hiding this comment.
For me both options (expect sanitized path or not) would work if it's covered in documentation, but I personally prefer to have this in the lib itself. It was my fault that it wasn't documented properly. Anyway, seems like #173 fix problem in video_ffmpg.
I think, we should then remove sanitize from lib/dtutils/file/rm and lib/dtutils/file/mkdir to be consistent or change sanitize method to be idempotent (preferred).
Added a space after the commands and removed quotes around the path argument in the command invocation.