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

[Ihor Radchenko]

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.

>> At first, I expected more common with the following project
>> https://github.com/abo-abo/org-download/
>> however even the approach to fetch images from clipboard is different.
>
> I have never used that package, this is what I wrote in a discussion
> with Ihor in the Matrix room which we iteratively improved.  It was
> mostly written with my workflow in mind.  Without knowledge of how
> others work, I cannot improve the implementation.  I also don't
> understand what the linked package from the Commentary section (which is
> why I never used it).

The package does similar things, except that it focuses on downloading
images from URL stored in clipboard or by making a screenshot. Dnd is
also supported there, but just for a single URL/file path.

Max, unless you see something particular that is implemented better in
org-download, let's just ignore that library. I eyeballed the code and I
do not see anything that we need to account for, except some more
specialized features (like attaching screenshots), which we can always
add in future, if necessary.

>> 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 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.
In theory, we might handle, for example, html markup in clipboard
specially, but I am not sure if that's what people expect.

>>> +  "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.

>>> +(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.

>> Is there a chance to request for link description? Unsure if
>> `org-insert-link' may be easily reused.
>
> This would include too many prompts and make the whole process annoying
> again.

+1. If users want to have a link description, they can easily follow the
prompt with C-c C-l to edit the inserted link.

>> I do not know if it can be easily implemented, but it may be useful to
>> control attachment/generic directory at the moment when a particular
>> image is yanked/dropped instead of purely relying on predefined user
>> option.
>
> Sorry, I don't understand what you mean here.  Note that my usage of
> attachments is sparse.  "Particular image" is hard to determine since
> the only reliable data at hand is its mimetype.

He meant that users might want to alternate between saving to attach
dir and saving to `org-yank-image-save-type' set to custom directory.
... which might theoretically be needed, but I see it as unnecessary
over-engineering. Let's keep things simple and consider implementing
more complex behavior if we get actual feature requests.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>