[செவ்வாய் செப்டம்பர் 26, 2023] Ihor Radchenko wrote: > Visuwesh writes: > >>> Please, use `make-temp-file' to create files in the temporary >>> directory that may be world writable. Predictable file names there are >>> undesired from security point of view. >> >> What harm does it cause? > > /tmp directory can be written by any program and the file, while kept > there, might be modified by malicious code. > > Not that I know a concrete example what harm can be done in this > particular case, but it is generally a good practice to make file names > in /tmp random. I don't see a way in org-attach-attach's mv method to change the basename of the file post attachment so we have to live with this harm. >>> I am in doubts if the following code is more suitable for org.el or >>> for org-attach.el >> >> I have the same doubts. > > The patch implements dnd and media handlers, which constitute Org mode > integration with the rest of Emacs. So, they are a part of major mode > definition. If we want to keep things clean, we may create org-dnd.el > and org-yank-media.el and put the relevant functions there, only leaving > the major mode setup in org.el I think it would be better to keep the registration part in org-mode's definition in that case since if a user wants to override this functionality, they can easily do so in org-mode-hook. > I do not think that org-attach is the right place for this new > functionality. > >>> Could it be extended to any mime type? If so I would avoid "image" in >>> its name. `org-yank-media-save-dir'? >> >> It should be easy enough to do it but do we want to go there? > > Isn't the patch handling non-images as well? > AFAIU, the only case when images are considered separately is when image > data is in clipboard. The patch handles non-images in the sense that they are simply attached using cp/mv method when they are copy/cut to the clipboard from a file manager. But if your clipboard data contains video/mpeg for example, then the patch does nothing. yank-media would complain about no registered handlers for video/mpeg. BTW, before I forget again for the Nth time: there's an issue with how yank-media works so cut files will not be handled properly unless bug#65892 gets its clearance. I hope the description in the linked bug is clear enough. >>>> + "Method to save images yanked from clipboard and dropped to Emacs. >>>> +It can be the symbol `attach' to add it as an attachment, or a >>>> +directory name to copy/cut the image to that directory." >>>> + :group 'org >>>> + :package-version '(Org . "9.7") >>>> + :type '(choice (const :tag "Add it as attachment" attach) >>>> + (directory :tag "Save it in directory")) >>>> + :safe (lambda (x) (or (stringp x) (eq x 'attach)))) >>> >>> Unsure if every directory may be considered as safe (e.g. ~/.ssh/) >> >> Is there a reliable way to know which directory is "safe"? If not, then >> let the users shoot themselves in the foot. > > We can just say :safe nil (omit the keyword). Then, users will be > prompted and can decide which directories are truly safe for them. That would be quite annoying IMO. I say we let the user shoot themselves in the foot. >>>> +(declare-function org-attach-attach "org-attach" (file &optional visit-dir method)) >>>> + >>>> +(defun org--image-yank-media-handler (mimetype data) >>>> + "Save image DATA of mime-type MIMETYPE and insert link at point. >>>> +It is saved as per `org-media-image-save-type'. The name for the >>>> +image is prompted and the extension is automatically added to the >>>> +end." >>>> + (let* ((ext (symbol-name (mailcap-mime-type-to-extension mimetype))) >>>> + (iname (read-string "Insert filename for image: ")) >>> >>> Unless 'attach is used, `read-file-name' would allow saving to >>> arbitrary directory with path completion. >> >> Sorry, I don't understand what you mean here. I am not really familiar >> with attachment facilities of org-mode. > > Imagine that user enters something like > "/tmp/non-existing-directory/image-file" as an answer to > "Insert filename for image: " prompt. How about the following prompt instead? Basename for image file without extension: Attached patch has several of the reviews considered.