[RFC] [PATCH] allow bind keywords to set safe values

[RFC] [PATCH] allow bind keywords to set safe values

[Aaron Ecay]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

Hello all,

BIND keywords should be used for controlling export, rather than the
usual emacs method of setting file local variables
<http://mid.gmane.org/87eglcbv7g.fsf@selenimh.access.network>.  But,
BIND keywords are currently disabled by default.  We can’t turn these on
by default, as maliciously crafted documents could do nasty things to a
user’s emacs.  The attached patch permits many interesting usages of
BIND keywords by allowing them to set variables by default, as long as
the value thus set is safe (as implemented by emacs’s default file local
variable code).

Comments welcome.

Thanks,

-- 
Aaron Ecay

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-bind-keywords-to-set-safe-values.patch --]
[-- Type: text/x-diff, Size: 4259 bytes --]

From a0650372cafa6debf1465624c2cc23dd01aa7083 Mon Sep 17 00:00:00 2001
From: Aaron Ecay <aaronecay@gmail.com>
Date: Fri, 6 Nov 2015 17:51:09 +0000
Subject: [PATCH] Allow bind keywords to set safe values

* lisp/ox.el (org-export-allow-bind-keywords): Add new `safe' value.
(org-export--list-bound-variables): Use it.
* doc/org.texi (Export settings): Update doc.
---
 doc/org.texi | 18 +++++++++++++++---
 lisp/ox.el   | 20 ++++++++++++++++----
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/doc/org.texi b/doc/org.texi
index c57cc41..6abf5ad 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -10708,6 +10708,7 @@ properties (@pxref{Properties and columns}).  Options set at a specific level
 override options set at a more general level.
 
 @cindex #+SETUPFILE
+@anchor{SETUPFILE}
 In-buffer settings may appear anywhere in the file, either directly or
 indirectly through a file included using @samp{#+SETUPFILE: filename} syntax.
 Option keyword sets tailored to a particular back-end can be inserted from
@@ -10948,9 +10949,20 @@ properties.
 @cindex #+BIND
 @vindex org-export-allow-bind-keywords
 If @code{org-export-allow-bind-keywords} is non-@code{nil}, Emacs variables
-can become buffer-local during export by using the BIND keyword.  Its syntax
-is @samp{#+BIND: variable value}.  This is particularly useful for in-buffer
-settings that cannot be changed using specific keywords.
+can become buffer-local during export by using the BIND keyword.  Setting the
+variable to @code{t} allows variables to take on all values.  Setting it to
+the symbol @code{safe} (the default) only allows safe values.  (@pxref{Safe
+File Variables,,,emacs,The Emacs Manual}) The syntax of a BIND keyword is
+@samp{#+BIND: variable value}.  The text of @samp{value} will be passed to
+the elisp @code{read} function.@footnote{This means that strings should be
+surrounded with double quotes, but symbols and lists will be read literally
+and need not be quoted for lisp with a single quote.}  The BIND keyword is
+particularly useful for in-buffer settings that cannot be changed using
+specific keywords.@footnote{You should not use the usual emacs local variable
+convention (@pxref{Specifying File Variables,,,emacs,The Emacs Manual}),
+because these notations could be lost during the export process.}  It is also
+useful for collecting common variable settings in a setup file shared between
+several documents (@pxref{SETUPFILE}).
 
 @cindex property, EXPORT_FILE_NAME
 The name of the output file to be generated is taken from the file associated
diff --git a/lisp/ox.el b/lisp/ox.el
index eb1af9b..e257c1f 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -832,14 +832,22 @@ can also be set with the OPTIONS keyword, e.g.  \"todo:nil\"."
   :group 'org-export-general
   :type 'boolean)
 
-(defcustom org-export-allow-bind-keywords nil
+(defcustom org-export-allow-bind-keywords 'safe
   "Non-nil means BIND keywords can define local variable values.
 This is a potential security risk, which is why the default value
-is nil.  You can also allow them through local buffer variables."
+is nil.  You can also allow them through local buffer variables.
+
+See the documentation for `safe-local-variable-p' and the
+node (info \"(emacs) Safe File Variables\") for information on
+the safety setting."
   :group 'org-export-general
   :version "24.4"
   :package-version '(Org . "8.0")
-  :type 'boolean)
+  :type '(choice
+	  (const :tag "Never" nil)
+	  (const :tag "Only if safe" safe)
+	  (const :tag "Always" t))
+  :safe (lambda (x) (memq x '(nil safe))))
 
 (defcustom org-export-with-broken-links nil
   "Non-nil means do not raise an error on broken links.
@@ -1646,7 +1654,11 @@ an alist where associations are (VARIABLE-NAME VALUE)."
 			 (let ((val (org-element-property :value element)))
 			   (if (equal (org-element-property :key element)
 				      "BIND")
-			       (push (read (format "(%s)" val)) alist)
+			       (let* ((pair (read (format "(%s)" val))))
+				 (when (or (eq org-export-allow-bind-keywords t)
+					   (safe-local-variable-p
+					    (nth 0 pair) (nth 1 pair)))
+				   (push pair alist)))
 			     ;; Enter setup file.
 			     (let ((file (expand-file-name
 					  (org-remove-double-quotes val))))
-- 
2.6.2

Re: [RFC] [PATCH] allow bind keywords to set safe values

[Thomas S. Dye]

Aloha Aaron,

Aaron Ecay <aaronecay@gmail.com> writes:

> Hello all,
>
> BIND keywords should be used for controlling export, rather than the
> usual emacs method of setting file local variables
> <http://mid.gmane.org/87eglcbv7g.fsf@selenimh.access.network>.  But,
> BIND keywords are currently disabled by default.  We can’t turn these on
> by default, as maliciously crafted documents could do nasty things to a
> user’s emacs.  The attached patch permits many interesting usages of
> BIND keywords by allowing them to set variables by default, as long as
> the value thus set is safe (as implemented by emacs’s default file local
> variable code).

The prescription that BIND keywords should be used over local variables
caught me by surprise.  Nicolas' post about a vague recollection that
some local variables might not be picked up during export seems an odd
motivation for the prescription.  I've used local variables to control
export for a long time without running into this problem.

I'm not complaining, just commenting on an unusual sequence of events on
the mailing list.  

I'm happy to migrate my local variables to BIND if that is what I should
do.

That said, it would be great if one could use EXPORT_BIND to control
export at the subtree export level.  I'm keeping separate HTML and LaTeX
export projects in the same file fairly often now and it can be
difficult (for me) to structure the whole file properly so both exports
work as expected.

BTW, many thanks for your recent interest in and work on Babel.  It is
an important part of my work flow and I've been uneasy since Eric
orphaned the project a while back.  I hope you find the work there
rewarding enough to keep up.

All the best,
Tom

-- 
Thomas S. Dye
http://www.tsdye.com

Re: [RFC] [PATCH] allow bind keywords to set safe values

[Aaron Ecay]

Hi Thomas,

2015ko azaroak 6an, "Thomas S. Dye"-ek idatzi zuen:
> 
> Aloha Aaron,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> Hello all,
>> 
>> BIND keywords should be used for controlling export, rather than the
>> usual emacs method of setting file local variables
>> <http://mid.gmane.org/87eglcbv7g.fsf@selenimh.access.network>.  But,
>> BIND keywords are currently disabled by default.  We can’t turn these on
>> by default, as maliciously crafted documents could do nasty things to a
>> user’s emacs.  The attached patch permits many interesting usages of
>> BIND keywords by allowing them to set variables by default, as long as
>> the value thus set is safe (as implemented by emacs’s default file local
>> variable code).
> 
> The prescription that BIND keywords should be used over local variables
> caught me by surprise.  Nicolas' post about a vague recollection that
> some local variables might not be picked up during export seems an odd
> motivation for the prescription.  I've used local variables to control
> export for a long time without running into this problem.
> 
> I'm not complaining, just commenting on an unusual sequence of events on
> the mailing list.  
> 
> I'm happy to migrate my local variables to BIND if that is what I should
> do.

The export process is complicated, involving at least one clone being
made of the org buffer.  If it’s async export, the clone is made in
another emacs process.

There’s a concern that some time in this process, local variable lines
could be lost.  This could happen because:
- hack-local-variables doesn’t get called when it should
- the lines themselves are deleted (because of being in a COMMENT or
  noexport subtree)
BIND keywords don’t rely on hack-local-variables, and would typically be
located in the preamble of the document.  So they should be immune to
these factors.  There’s also an issue with #+INCLUDE keywords, which
will bring in BINDs from the included file, but (AFAICS) not local
variable lines.

I tried to take a look at the export code.  I’m 99% certain that any
local variables with the org- prefix will be successfully maintained at
all relevant steps of the export process.  But the code is complex, and
I couldn’t reach 100% certainty.  It would be better (more reassuring)
if someone could reach that level of certainty.  Then we could tell
people that it doesn’t matter whether they use BIND or a local variable
(which is what I wish I could say to you now).

If you’ve used local variables and are happy with how they have worked I
think nothing bad will happen if you leave them as is.  But that’s just,
like, my opinion man.

> 
> That said, it would be great if one could use EXPORT_BIND to control
> export at the subtree export level.  I'm keeping separate HTML and LaTeX
> export projects in the same file fairly often now and it can be
> difficult (for me) to structure the whole file properly so both exports
> work as expected.

This is an excellent idea IMO.

> 
> BTW, many thanks for your recent interest in and work on Babel.  It is
> an important part of my work flow and I've been uneasy since Eric
> orphaned the project a while back.  I hope you find the work there
> rewarding enough to keep up.

I’ll do my best!  Thanks for the kind words.

-- 
Aaron Ecay

Re: [RFC] [PATCH] allow bind keywords to set safe values

[Nicolas Goaziou]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> BIND keywords should be used for controlling export, rather than the
> usual emacs method of setting file local variables
> <http://mid.gmane.org/87eglcbv7g.fsf@selenimh.access.network>.

In this message, I say that file local variables may not replace BIND
keywords (although, I still cannot remember why).

However, BIND keywords cannot replace file local variables, because some
variables are used outside of `org-export-as'. `org-latex-compiler'
comes to mind.

> But, BIND keywords are currently disabled by default. We can’t turn
> these on by default, as maliciously crafted documents could do nasty
> things to a user’s emacs. The attached patch permits many interesting
> usages of BIND keywords by allowing them to set variables by default,
> as long as the value thus set is safe (as implemented by emacs’s
> default file local variable code).

Sounds good.

>  If @code{org-export-allow-bind-keywords} is non-@code{nil}, Emacs variables
> -can become buffer-local during export by using the BIND keyword.  Its syntax
> -is @samp{#+BIND: variable value}.  This is particularly useful for in-buffer
> -settings that cannot be changed using specific keywords.
> +can become buffer-local during export by using the BIND keyword.  Setting the
> +variable to @code{t} allows variables to take on all values.  Setting it to
> +the symbol @code{safe} (the default) only allows safe values.  (@pxref{Safe
> +File Variables,,,emacs,The Emacs Manual}) The syntax of a BIND keyword is
> +@samp{#+BIND: variable value}.  The text of @samp{value} will be passed to
> +the elisp @code{read} function.@footnote{This means that strings should be
> +surrounded with double quotes, but symbols and lists will be read literally
> +and need not be quoted for lisp with a single quote.}  The BIND keyword is
> +particularly useful for in-buffer settings that cannot be changed using
> +specific keywords.@footnote{You should not use the usual emacs local variable
> +convention (@pxref{Specifying File Variables,,,emacs,The Emacs Manual}),
> +because these notations could be lost during the export process.}  It is also
> +useful for collecting common variable settings in a setup file shared between
> +several documents (@pxref{SETUPFILE}).

Maybe an example would be interesting, preferably with a non trivial
value (e.g. a list or a string). WDYT?

> -			       (push (read (format "(%s)" val)) alist)
> +			       (let* ((pair (read (format "(%s)" val))))

`let*' -> `let'


Regards,

-- 
Nicolas Goaziou

Re: [RFC] [PATCH] allow bind keywords to set safe values

[Aaron Ecay]

Hi Nicolas,

2015ko azaroak 6an, Nicolas Goaziou-ek idatzi zuen:
> 
> Hello,
> 
> Aaron Ecay <aaronecay@gmail.com> writes:
> 
>> BIND keywords should be used for controlling export, rather than the
>> usual emacs method of setting file local variables
>> <http://mid.gmane.org/87eglcbv7g.fsf@selenimh.access.network>.
> 
> In this message, I say that file local variables may not replace BIND
> keywords (although, I still cannot remember why).
> 
> However, BIND keywords cannot replace file local variables, because some
> variables are used outside of `org-export-as'. `org-latex-compiler'
> comes to mind.

You are right, both are needed.  Maybe org-mode (the function, i.e. the
code executed when opening an org-mode file) should look for BIND keywords
and set local variables according to them.  Otherwise people might need
the same entry in BIND and the local variables block.  WDYT?

(Of course, it would be even better if we could convince ourselves that
local variables will work 100% of the time for export – then the use
case for BIND would be restricted to setupfiles.  I tried to see if I
could convince myself of this, but quickly got lost.)

> 
>> But, BIND keywords are currently disabled by default. We can’t turn
>> these on by default, as maliciously crafted documents could do nasty
>> things to a user’s emacs. The attached patch permits many interesting
>> usages of BIND keywords by allowing them to set variables by default,
>> as long as the value thus set is safe (as implemented by emacs’s
>> default file local variable code).
> 
> Sounds good.

OK.  I will push a patch to master with your suggestions.

Thanks,

-- 
Aaron Ecay