Re: [BUG] [PATCH] Add yank-media and DND handler [9.6.7 (9.6.7-g6eb773 @ /home/viz/lib/emacs/straight/build/org/)]

[Visuwesh]

[செவ்வாய் செப்டம்பர் 26, 2023] Ihor Radchenko wrote:

> Visuwesh <visuweshm@gmail.com> 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.

0001-Add-support-for-yank-media-and-DND.patch
Description: Text Data