Re: [PATCH] New remote resource download policy

[Max Nikulin <manikulin@gmail.com>]

On 12/06/2022 21:43, Timothy wrote:
> 
> As was raised in the #+include: URL thread 
> (https://list.orgmode.org/877d5sd7yu.fsf@gmail.com), currently Org will 
> automatically download files without confirmation in various circumstances.
> 
> This patch introduces two variables to control Org’s attitude towards 
> downloading files, and hooks them into the relevant parts of the codebase.

Timothy, thank you for efforts in this direction. In some sense you have 
done even more than I asked for. I tried you patch mostly to confirm 
that the protection can not be bypassed using file local variables. 
Since custom variables are not marked as safe, user is asked if values 
should be applied. Such behavior is consistent with my expectation.

> --- a/lisp/org-attach.el
> +++ b/lisp/org-attach.el
> @@ -525,7 +525,11 @@ (defun org-attach-attach (file &optional visit-dir method)
>         ((eq method 'cp) (copy-file file attach-file))
>         ((eq method 'ln) (add-name-to-file file attach-file))
>         ((eq method 'lns) (make-symbolic-link file attach-file))
> -       ((eq method 'url) (url-copy-file file attach-file)))
> +       ((eq method 'url)
> +        (if (or (not noninteractive) (org--should-fetch-remote-resource-p file))

I am confused by (not noninteractive). Does it mean that interactive 
call is enough to bypass protection? It may have sense it at this step 
there is no ambiguity what resources is fetched. On the other hand I am 
unsure concerning a case when `org-attach-attach' is a part of a larger 
command.

> +            (url-copy-file file attach-file)
> +          (error "The remote resource %S is considered unsafe, and will not be downloaded."
> +                 file))))


> +(defcustom org-download-remote-resources 'prompt

The name sounds like some function.

> +(defun org--confirm-resource-safe (uri)
> +  "Ask the user if URI should be considered safe, returning non-nil if so."
> +    (unless noninteractive
> +      (let ((buf (get-buffer-create "*Org Remote Resource*")))

I see your intention to add something fancy to the dialog. May `org-mks' 
be reused instead to avoid proliferation variants of rather similar UI code?

> +        ;; Set up the contents of the *Local Variables* buffer.
> +        (with-current-buffer buf
> +          (erase-buffer)
> +          (insert "An org-mode document would like to download "
> +                  (propertize uri 'face '(:inherit org-link :weight normal))
> +                  ", which is not considered safe.\n\n"
> +                  "Do you want to download this?  You can type\n "
> +                  (propertize "!" 'face 'success)
> +                  " to download this resource, and permanantly mark it as safe.\n "
> +                  (propertize "y" 'face 'warning)
> +                  " to download this resource, just this once.\n "

I am in doubts concerning "once". I tried "y" in a file having to 
"#+include:" of the same file. I did not get question for second 
include. I did not get prompt for this file anymore at all, even during 
next export. I modified the remote file, but stale content appeared 
during export. So the file was really downloaded once, but it is hardly 
in agreement with my expectations. Behavior is unrelated to this patch, 
concerning wording I am not sure, but I have no a better variant.

> +                  (propertize "n" 'face 'error)
> +                  " to skip this resource.\n")

 From "skip" I do not expect aborting of export.

I have an idea but unsure if it should be implemented. Consider 
`org-remote-resources-policy' custom variable that is a list of pairs 
(url-regexp . policy) for fine grain tuning instead of 2 variables. The 
price is more complicated structure, so higher chance of user error.