Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Max Nikulin]

On 11/12/2022 03:28, Tom Gillespie wrote:
>      Here is a patch that improves the ergonomics and thus hopefully
> the security for the recent changes to check evaluation for cells.

Tom, thank you for the patch. Frankly speaking, I was expecting this 
kind of complains, but I could not suggest any solution. I am not 
familiar with org-babel code, so my comments may be false alarms.

> * lisp/ob-core.el (org-confirm-babel-evaluate-cell): Added to control
> execution of cells separate from execution of src blocks, it works in
> exactly the same way as org-confirm-babel-evaluate.

I am not sure concerning "exactly".

lisp/ob-core.el:248
`org-confirm-babel-evaluate' is called with 2 arguments. In your patch 
`org-confirm-babel-evaluate-cell' has a single argument.

> This commit resolves the issue by making it possible to ignore checks
> on cells (the old behavior) without compromising general security for
> running src blocks.

It seems, you do not change defaults. Could you, please, provide an 
example of configuration that is less annoying, but still safe?

> This is necessary because there is no easy way to hop swap
> org-confirm-babel-evaluate between org-get-src-block-info where
> org-babel-read is called and the execution of that src block. It could
> probably be done using advice around org-babel-read, but that is a
> level of hackery that should be avoided.

I was thinking if it is possible to collect requests to confirm and to 
allow the user to decide for the whole bunch of expressions and code 
blocks. Besides implementation issues, there is a question concerning UI 
that will allow to inspect code to be evaluated.

> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
...> +(defcustom org-confirm-babel-evaluate-cell t
> +  "Confirm before evaluating a cell."
Calling convention for the case of function value is not described. If 
it is really the same as for `org-confirm-babel-evaluate' then this user 
option should be mentioned in the docstring.

> +  :group 'org-babel
> +  :version "29.1"

:package-version instead of :version?

> +  :type '(choice boolean function))
> +;; don't allow this variable to be changed through file settings
> +(put 'org-confirm-babel-evaluate-cell 'safe-local-variable (lambda (x) (eq x 
> t)))

Is there any reason to not use the :safe property of `defcustom'? I see 
that you take definition of `org-confirm-babel-evaluate' as a template 
so I wonder if there is some particular reason or the original code was 
just written before introducing of :safe.