Risky local variable mechanism

Risky local variable mechanism

[Richard M. Stallman]

A few days ago I sent a message about possibly replacing the risky
local variable mechanism with something safer.  Nobody has responded
yet.  This problem is important; please help me think about the issue.

Re: Risky local variable mechanism

[Stefan Monnier]

> A few days ago I sent a message about possibly replacing the risky
> local variable mechanism with something safer.  Nobody has responded
> yet.  This problem is important; please help me think about the issue.

I rarely use file-local variables, so it's hard for me to figure out which
set of restrictions will be best.  Using custom types sounds like a good
idea, but note that setting "boolean" variables can be dangerous as well
(e.g. enable-local-eval).  I guess we need to play with it a bit.
But if we change the way it works, I think it might be a good idea to make
sure that what we define is "really" safe.  I.e. we only allow some
known-safe variable settings.  Maybe "string and integer custom vars" are
all safe, I don't know.

Otherwise we could simply introduce a "safe-local-variable" property which
we'd add to every variable we decide is safe to set.  Its value could also
be a predicate (instead of just t) to allow finer settings.


        Stefan

Re: Risky local variable mechanism

[Luc Teirlinck]

Stefan Monnier wrote:

   Otherwise we could simply introduce a "safe-local-variable" property which
   we'd add to every variable we decide is safe to set.  Its value could also
   be a predicate (instead of just t) to allow finer settings.

Introduce?  That is exactly what we have already, is it not?

Sincerely,

Luc.

Re: Risky local variable mechanism

[Chong Yidong]

"Richard M. Stallman" <rms@gnu.org> writes:

> A few days ago I sent a message about possibly replacing the risky
> local variable mechanism with something safer.  Nobody has responded
> yet.  This problem is important; please help me think about the issue.

If you want to be really restrictive, you can introduce a list of
`safe-local-variables', and prompt for anything outside that list (the
prompt could have an additional choice, [(a) always allow this
variable], a variable to the list).

Setting `enable-local-variables' to (e.g.) 'unless-risky could bring
back the old behavior, i.e., allowing everything except those marked
as risky.

It seems like people only actually use a handful of file variables,
anyway.  From looking at the source trees of Emacs and some other
projects, the file local variables in use are:

allout-layout
byte-compile-dynamic
byte-compile-warnings
c-basic-offset
c-indent-level
coding
compile-command
fill-column
fill-prefix
indent-tabs-mode
kept-new-versions
make-backup-files
mode
outline-layout
page-delimiter
paragraph-separate
sentence-end
sentence-end-double-space
sgml-omittag
sgml-shorttag
sgml-minimize-attributes
sgml-always-quote-attributes
sgml-indent-step
sgml-indent-data
sgml-parent-document
sgml-exposed-tags
sgml-local-catalogs
sgml-local-ecat-files
tab-width
time-stamp-end
time-stamp-format
time-stamp-start
trim-versions-without-asking
version-control

Re: Risky local variable mechanism

[Stefan Monnier]

>    Otherwise we could simply introduce a "safe-local-variable" property
>    which we'd add to every variable we decide is safe to set.  Its value
>    could also be a predicate (instead of just t) to allow finer settings.

> Introduce?  That is exactly what we have already, is it not?

Do we?  Oh I guess we do, but we don't really use it much, because we rely
on risky-local-variable instead.  So my suggestion would then be to remove
risky-local-variable and only rely on safe-local-variable.


        Stefan

Re: Risky local variable mechanism

[Jonathan Yavner]

Stefan wrote:
> Maybe "string and integer custom vars" are all safe, I don't know.

No, sendmail-program is not safe, nor is max-eval-lisp-depth.

Re: Risky local variable mechanism

[Stefan Monnier]

>> Maybe "string and integer custom vars" are all safe, I don't know.
> No, sendmail-program is not safe, nor is max-eval-lisp-depth.

Indeed, names of external programs need to be ruled out.

OTOH I think the only danger with max-eval-lisp-depth is DoS, which I'd
rather ignore because it's a tremendously harder problem to solve than
direct security holes.


        Stefan

Re: Risky local variable mechanism

[Chong Yidong]

Jonathan Yavner <jyavner@member.fsf.org> writes:

> Stefan wrote:
>> Maybe "string and integer custom vars" are all safe, I don't know.
>
> No, sendmail-program is not safe, nor is max-eval-lisp-depth.

We already treat as risky any variable names that match hooks,
functions, forms, program, commands, predicates, fontlockkeywords,
fontlockkeywords, fontlocksyntactickeywords, framealist, modealist,
map, or mapalist.

Re: Risky local variable mechanism

[Kim F. Storm]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Maybe "string and integer custom vars" are all safe, I don't know.
>> No, sendmail-program is not safe, nor is max-eval-lisp-depth.
>
> Indeed, names of external programs need to be ruled out.

Maybe all variables matching "-program$" and "-path$" should be
ruled out.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Risky local variable mechanism

[Richard M. Stallman]

    If you want to be really restrictive, you can introduce a list of
    `safe-local-variables', and prompt for anything outside that list (the
    prompt could have an additional choice, [(a) always allow this
    variable], a variable to the list).

Perhaps that is the best solution.  However, if only 2% of all file
local variable settings are outside that list, it will still cause
annoyance to a lot of people.

So let's first see if some more general method based on custom types
can be made to work.  If that can't be made to work, we can fall back
on this approach.

Re: Risky local variable mechanism

[Richard M. Stallman]

    I rarely use file-local variables, so it's hard for me to figure out which
    set of restrictions will be best.  Using custom types sounds like a good
    idea, but note that setting "boolean" variables can be dangerous as well
    (e.g. enable-local-eval).

That is a good point--but there are just a few variables which are
dangerous in that way, and they already are marked.  The big set of
variables which are dangerous but not marked are those whose values
can be functions to call.  That is what I am hoping to recognize using
custom types.

Another idea: just check to see if the value is a function name, or if
any function name (including lambda) appears in it.  If so, the value
is risky.  That is quite simple and does not depend on knowing the
custom type.

Re: Risky local variable mechanism

[Stefan Monnier]

>>>> Maybe "string and integer custom vars" are all safe, I don't know.
>>> No, sendmail-program is not safe, nor is max-eval-lisp-depth.
>> Indeed, names of external programs need to be ruled out.
> Maybe all variables matching "-program$" and "-path$" should be
> ruled out.

I'd much rather start with a known-safe set (the empty set) and only add to
it elements that are known to be safe.  Rather than add large unsafe sets
(like all custom vars of type string) and then try to safen them by removing
known unsafe cases.


        Stefan

Re: Risky local variable mechanism

[LENNART BORGMAN]

From: "Richard M. Stallman" <rms@gnu.org>

>    If you want to be really restrictive, you can introduce a list of
>    `safe-local-variables', and prompt for anything outside that 
> list (the
>    prompt could have an additional choice, [(a) always allow this
>    variable], a variable to the list).
> 
> Perhaps that is the best solution.  However, if only 2% of all file
> local variable settings are outside that list, it will still cause
> annoyance to a lot of people.
> 
> So let's first see if some more general method based on custom types
> can be made to work.  If that can't be made to work, we can fall back
> on this approach.

It is much more annoying if there is a possible security hole. Please follow Stefans advice on this. (To have a small set of known safe variables.)

Re: Risky local variable mechanism

[David Kastrup]

"Richard M. Stallman" <rms@gnu.org> writes:

>     If you want to be really restrictive, you can introduce a list of
>     `safe-local-variables', and prompt for anything outside that list (the
>     prompt could have an additional choice, [(a) always allow this
>     variable], a variable to the list).
>
> Perhaps that is the best solution.  However, if only 2% of all file
> local variable settings are outside that list, it will still cause
> annoyance to a lot of people.
>
> So let's first see if some more general method based on custom types
> can be made to work.  If that can't be made to work, we can fall back
> on this approach.

I agree with others that this is not something that makes sense to try
to push into Emacs 22.1.  I'd even be hesitant to have it move into
23.1: I think that 23.1 should basically concentrate on pushing out
multi-tty and unicode-2 branches, as well as an update of frozen
externally maintained packages (like tramp).

Once we have that off our chest, there is a better time to work on
what clearly is a new feature requiring lots of testing and
experiments.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Risky local variable mechanism

[Kim F. Storm]

"Richard M. Stallman" <rms@gnu.org> writes:

>     I rarely use file-local variables, so it's hard for me to figure out which
>     set of restrictions will be best.  Using custom types sounds like a good
>     idea, but note that setting "boolean" variables can be dangerous as well
>     (e.g. enable-local-eval).
>
> That is a good point--but there are just a few variables which are
> dangerous in that way, and they already are marked.  The big set of
> variables which are dangerous but not marked are those whose values
> can be functions to call.  That is what I am hoping to recognize using
> custom types.
>
> Another idea: just check to see if the value is a function name, or if
> any function name (including lambda) appears in it.  If so, the value
> is risky.  That is quite simple and does not depend on knowing the
> custom type.

Why not make anything risky, except those explicit settings which
are recorded in safe-local-eval-forms (and add something similar
for variables), and then make it easier to update those lists
when user is queried to approve local variables/forms

Of course, we can still mark certain things 'safe which we explicitly
consider to be safe.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Risky local variable mechanism

[Kim F. Storm]

"Richard M. Stallman" <rms@gnu.org> writes:

> Another idea: just check to see if the value is a function name, or if
> any function name (including lambda) appears in it.  If so, the value
> is risky.  That is quite simple and does not depend on knowing the
> custom type.

This doesn't work for string variables corresponding to
external programs or commands run by emacs.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Risky local variable mechanism

[Kim F. Storm]

David Kastrup <dak@gnu.org> writes:

> Once we have that off our chest, there is a better time to work on
> what clearly is a new feature requiring lots of testing and
> experiments.

I agree.

Will someone please explain how emacs 22.1 will be less safe than 21.4
if we don't change this "risky business" before the release.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Risky local variable mechanism

[Richard M. Stallman]

    > Maybe "string and integer custom vars" are all safe, I don't know.

    No, sendmail-program is not safe, nor is max-eval-lisp-depth.

The worst you can do by setting max-lisp-eval-depth is to make
Emacs crash or get an error.

I am not sure binding sendmail-program is unsafe.
It will generally have no effect if you bind it locally
in a buffer that isn't a mail buffer.  But looking at the more
general issue of binding variables that specify programs to run,
I am not sure how much of a security issue that is,
other than for root.  It can only run programs that exist.
Even if you could set sendmail-program globally in Emacs,
could you actually find a value that would predictably do harm?

Re: Risky local variable mechanism

[Stefan Monnier]

> I am not sure binding sendmail-program is unsafe.
> It will generally have no effect if you bind it locally
> in a buffer that isn't a mail buffer.  But looking at the more
> general issue of binding variables that specify programs to run,
> I am not sure how much of a security issue that is,
> other than for root.  It can only run programs that exist.
> Even if you could set sendmail-program globally in Emacs,
> could you actually find a value that would predictably do harm?

It mostly depends on whether or not the string represent the filename of
a program or the beginning of a shell command (i.e. can it include
arguments?).
"Interesting" commands can be "rm" (of course), "echo foobar
~/.ssh/authorized_keys", ...


        Stefan

Re: Risky local variable mechanism

[Richard M. Stallman]

    Will someone please explain how emacs 22.1 will be less safe than 21.4
    if we don't change this "risky business" before the release.

This is a serious problem; that it is not new is no reason to accept it.

Re: Risky local variable mechanism

[Richard M. Stallman]

    Why not make anything risky, except those explicit settings which
    are recorded in safe-local-eval-forms (and add something similar
    for variables), and then make it easier to update those lists
    when user is queried to approve local variables/forms

That could be a good approach.

Would someone like to implement this?

Re: Risky local variable mechanism

[Luc Teirlinck]

Richard Stallman wrote:

       Why not make anything risky, except those explicit settings which
       are recorded in safe-local-eval-forms (and add something similar
       for variables), and then make it easier to update those lists
       when user is queried to approve local variables/forms

   That could be a good approach.

   Would someone like to implement this?

Note that we _already_ have a similar mechanism, which is more
flexible than the proposed one, because it allows the safety to depend
on both the variable _and_ its value:

>From `(elisp)File Local Variables':

    These rules can be overridden by giving the variable's name a
    non-`nil' `safe-local-variable' property.  If one gives it a
    `safe-local-variable' property of `t', then one can give the variable
    any file local value.  One can also give any symbol, including the
    above, a `safe-local-variable' property that is a function taking
    exactly one argument.  In that case, giving a variable with that name
    a file local value is only allowed if the function returns non-`nil'
    when called with that value as argument.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Richard M. Stallman]

	   Why not make anything risky, except those explicit settings which
	   are recorded in safe-local-eval-forms (and add something similar
	   for variables), and then make it easier to update those lists
	   when user is queried to approve local variables/forms

       That could be a good approach.

       Would someone like to implement this?

    Note that we _already_ have a similar mechanism, which is more
    flexible than the proposed one,

This seems to be a misunderstanding.  You're talking about underlying
mechanisms; we're talking about user interfaces.

Re: Risky local variable mechanism

[Luc Teirlinck]

Richard Stallman wrote:

   This seems to be a misunderstanding.  You're talking about underlying
   mechanisms; we're talking about user interfaces.

If I understand the proposed interface correctly, it would try to
update the currently customizable (through Custom) variable
safe-local-eval-forms automatically when the user visits a file.
It would create another option, customizable through Custom, for
variables and try to update that one in the same way too.

One problem for the variable list option would be that, in its
proposed form, it would only handle the variable and not the value it
is going to be set to, whereas the value is often crucial.

There are way worse problems, if the user would try to customize these
variables _through Custom_ instead of .emacs.

Customizing `safe-local-eval-forms' through Custom _already_ is
problematic right now.  The only way to avoid surprises is to
customize it in .emacs instead of through Custom.  (The reasons are
similar to those for hooks.)  The automatic updating would make the
existing problems worse and would make actually triggering the
potential problems _way_ more likely than it is now.

The two options would be listvars, whose value should not be set
using setq, like Custom does, but using add-to-list or
(setq safe-local-eval-forms (delete form safe-local-eval-forms)).

Custom can currently not handle such variables in any satisfactory
way.  The involved problems are similar to those for hooks.  There are
tons of problems we know about for hooks (but which apply equally to
this type of listvar) and whenever we discuss these problems, we find
some new ones.  We decided earlier to delay solving these problems
till after the release, because they required too radical changes in
Custom.

I do not see how the automatic updating could be done in a way that
works well both for people who customize these two options in their
.emacs _and_ for people who customize them through Custom, especially
not given Custom's current problems with this type of listvars.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

There are some things I misunderstood about this problem, because the
documentation in the Elisp manual was misleading.  It seemed to
suggest that most of the variables and features in question affect
whether setting file local variables is allowed, whereas they only
determine whether setting them is affected by `enable-local-eval'.
If `enable-local-eval' has its default value 'maybe, the user gets
_asked_ whether to set the file local variables.  The lone exceptions
are the variables in `ignored-local-variables', which unconditionally
can not be set.  Another exception is that a user running as root can
not set file local variables to values controlled by
`enable-local-eval', regardless of the value of `enable-local-eval'.

So we are just talking about which variables to _warn_ the user about.
I personally have `enable-local-variables' set to 'query for ages and
I have never found these questions too inconvenient.

So why do we not play it super-safe and just set the default value of
`enable-local-variables' to 'maybe?  That would not require _any_ changes,
other than a bugfix to ses, for which I am about to submit a patch
anyway.  If a user wants to live more dangerously, he can always set
the value of enable-local-variables back to t. 

To understand the problem better myself, I rewrote the misleading
Elisp manual node.  I include a patch below.  If we are going to
completely change the mechanism (rather than just the default of
`enable-local-variables'), the patches below affect nodes that would
have to be completely rewritten after the changes anyway, but I could
install them if it could help other people understand the present
situation and hence the problem better.  The current node is
misleading.  So are several docstrings, but I did not rewrite those,
since if we would change the mechanism completely, this would be moot.

===File ~/custom.texi-diff==================================
*** custom.texi	05 Feb 2006 16:45:32 -0600	1.107
--- custom.texi	06 Feb 2006 17:53:59 -0600	
***************
*** 1196,1202 ****
    The @code{safe-local-eval-forms} is a customizable list of eval
  forms which are safe to eval, so Emacs should not ask for
  confirmation to evaluate these forms, even if
! @code{enable-local-variables} says to ask for confirmation in general.
  
  @node Key Bindings
  @section Customizing Key Bindings
--- 1196,1202 ----
    The @code{safe-local-eval-forms} is a customizable list of eval
  forms which are safe to eval, so Emacs should not ask for
  confirmation to evaluate these forms, even if
! @code{enable-local-eval} says to ask for confirmation in general.
  
  @node Key Bindings
  @section Customizing Key Bindings
============================================================

===File ~/variables.texi-diff===============================
*** variables.texi	06 Feb 2006 16:02:08 -0600	1.71
--- variables.texi	06 Feb 2006 20:12:58 -0600	
***************
*** 1774,1828 ****
    If a file local variable could specify a function that would
  be called later, or an expression that would be executed later, simply
  visiting a file could take over your Emacs.  To prevent this, Emacs
! takes care not to allow to set such file local variables.
  
!   For one thing, any variable whose name ends in any of
! @samp{-command}, @samp{-frame-alist}, @samp{-function},
! @samp{-functions}, @samp{-hook}, @samp{-hooks}, @samp{-form},
! @samp{-forms}, @samp{-map}, @samp{-map-alist}, @samp{-mode-alist},
! @samp{-program}, or @samp{-predicate} cannot be given a file local
! value.  In general, you should use such a name whenever it is
! appropriate for the variable's meaning.  The variables
! @samp{font-lock-keywords}, @samp{font-lock-keywords} followed by a
! digit, and @samp{font-lock-syntactic-keywords} cannot be given file
! local values either.  These rules can be overridden by giving the
! variable's name a non-@code{nil} @code{safe-local-variable} property.
! If one gives it a @code{safe-local-variable} property of @code{t},
! then one can give the variable any file local value.  One can also
! give any symbol, including the above, a @code{safe-local-variable}
! property that is a function taking exactly one argument.  In that
! case, giving a variable with that name a file local value is only
! allowed if the function returns non-@code{nil} when called with that
! value as argument.
  
!   In addition, any variable whose name has a non-@code{nil}
! @code{risky-local-variable} property is also ignored.  So are all
! variables listed in @code{ignored-local-variables}:
  
! @defvar ignored-local-variables
! This variable holds a list of variables that should not be given local
! values by files.  Any value specified for one of these variables is
! ignored.
! @end defvar
  
  @defun risky-local-variable-p sym &optional val
! If @var{val} is non-@code{nil}, returns non-@code{nil} if giving
! @var{sym} a file local value of @var{val} would be risky, for any of
! the reasons stated above.  If @var{val} is @code{nil} or omitted, only
! returns @code{nil} if @var{sym} can be safely assigned any file local
! value whatsoever.
  @end defun
  
!   The @samp{Eval:} ``variable'' is also a potential loophole, so Emacs
! normally asks for confirmation before handling it.
! 
! @defopt enable-local-eval
! This variable controls processing of @samp{Eval:} in @samp{-*-} lines
! or local variables
! lists in files being visited.  A value of @code{t} means process them
! unconditionally; @code{nil} means ignore them; anything else means ask
! the user what to do for each file.  The default value is @code{maybe}.
! @end defopt
  
    Text properties are also potential loopholes, since their values
  could include functions to call.  So Emacs discards all text
--- 1774,1832 ----
    If a file local variable could specify a function that would
  be called later, or an expression that would be executed later, simply
  visiting a file could take over your Emacs.  To prevent this, Emacs
! normally queries you before setting such file local variables.
  
! @defopt enable-local-eval
! This variable controls processing of the @samp{Eval:} ``variable'' and
! other risky local variables in @samp{-*-} lines or local variables
! lists in files being visited.  A value of @code{t} means process them
! unconditionally; @code{nil} means ignore them; anything else means ask
! the user what to do for each file.  The default value is @code{maybe}.
  
! A user running as root can not set these risky variables, regardless
! of the value of @code{enable-local-eval}.
! @end defopt
  
!   Any variable whose name ends in any of @samp{-command},
! @samp{-frame-alist}, @samp{-function}, @samp{-functions},
! @samp{-hook}, @samp{-hooks}, @samp{-form}, @samp{-forms}, @samp{-map},
! @samp{-map-alist}, @samp{-mode-alist}, @samp{-program}, or
! @samp{-predicate} is subject to @code{enable-local-eval}.  In general,
! you should use such a name whenever it is appropriate for the
! variable's meaning.  @code{enable-local-eval} also affects the
! variables @samp{font-lock-keywords}, @samp{font-lock-keywords}
! followed by a digit, and @samp{font-lock-syntactic-keywords}.  These
! rules can be overridden by giving the variable's name a non-@code{nil}
! @code{safe-local-variable} property.  If one gives it a
! @code{safe-local-variable} property of @code{t}, then the variable is
! only subject to @code{enable-local-variables}.  One can also give any
! symbol, including the above, a @code{safe-local-variable} property
! that is a function taking exactly one argument.  In that case, giving
! a variable with that name a file local value is only subject to
! @code{enable-local-eval} if the function returns non-@code{nil} when
! called with that value as argument.
! 
!   In addition, any variable whose name has a non-@code{nil}
! @code{risky-local-variable} property is also subject to
! @code{enable-local-eval}.
  
  @defun risky-local-variable-p sym &optional val
! If @var{val} is non-@code{nil}, returns non-@code{nil} if
! @code{enable-local-eval} affects giving @var{sym} a file local value
! of @var{val}.  If @var{val} is @code{nil} or omitted, only returns
! @code{nil} if @code{enable-local-eval} does not affect assigning
! @var{sym} any file local value whatsoever.
! 
! This function ignores the value of @code{ignored-local-variables} (see
! below), because Emacs ignores the variables in that list,
! @emph{regardless} of the value of @code{enable-local-eval}.
  @end defun
  
! @defvar ignored-local-variables
! This variable holds a list of variables that should not be given local
! values by files, regardless of the value of @code{enable-local-eval}.
! Any value specified for one of these variables is unconditionally ignored.
! @end defvar
  
    Text properties are also potential loopholes, since their values
  could include functions to call.  So Emacs discards all text
============================================================

Re: Risky local variable mechanism

[Richard M. Stallman]

    If I understand the proposed interface correctly, it would try to
    update the currently customizable (through Custom) variable
    safe-local-eval-forms automatically when the user visits a file.

Not "automatically".  It would ask the user questions.

    It would create another option, customizable through Custom, for
    variables and try to update that one in the same way too.

The idea is not that specific.  That could be one way of doing it.
The details can be worked out in various ways.

    There are way worse problems, if the user would try to customize these
    variables _through Custom_ instead of .emacs.

I don't think so.  It makes no difference whether the change is made
using the Custom buffer or using the ask-questions-when-visiting
interface; either way it will update .emacs just the same.

But even if you are right, it is just a side issue, not very
important.  We have a grave problem that we need to solve.  If the
solution introduces minor problems, that's ok.

    I do not see how the automatic updating could be done in a way that
    works well both for people who customize these two options in their
    .emacs _and_ for people who customize them through Custom, especially
    not given Custom's current problems with this type of listvars.

I think it will work right without any special effort.  If not, we can
easily work around it by making two variables, one to set with Custom
and one that ask-questions-when-visiting will set.

Please learn to put minor imperfections in perspective.  They are not
show-stoppers.

Re: Risky local variable mechanism

[Chong Yidong]

"Richard M. Stallman" <rms@gnu.org> writes:

>     Why not make anything risky, except those explicit settings which
>     are recorded in safe-local-eval-forms (and add something similar
>     for variables), and then make it easier to update those lists
>     when user is queried to approve local variables/forms
>
> That could be a good approach.
>
> Would someone like to implement this?

How about this patch?  It implements a `safe-local-variables' custom
option.  If a variable is not in this list, the user is prompted
before it is set.  If the user agrees to set it, and the variable is
not explicitly marked as risky (as determined by the currently
existing `risky-local-variable-p' function), we ask if that variable
can be automatically set in the future.  If the user agrees to this
too, `safe-local-variables' is updated and saved to the custom-file.


*** emacs/lisp/files.el.~1.804.~	2006-02-06 23:43:16.000000000 -0500
--- emacs/lisp/files.el	2006-02-07 11:35:06.000000000 -0500
***************
*** 2393,2405 ****
        (run-hooks 'hack-local-variables-hook))
      mode-specified))
  
! (defvar ignored-local-variables ()
!   "Variables to be ignored in a file's local variable spec.")
  
  ;; Get confirmation before setting these variables as locals in a file.
  (put 'debugger 'risky-local-variable t)
  (put 'enable-local-eval 'risky-local-variable t)
  (put 'ignored-local-variables 'risky-local-variable t)
  (put 'eval 'risky-local-variable t)
  (put 'file-name-handler-alist 'risky-local-variable t)
  (put 'inhibit-quit 'risky-local-variable t)
--- 2393,2417 ----
        (run-hooks 'hack-local-variables-hook))
      mode-specified))
  
! (defcustom safe-local-variables
!   '(c-basic-offset c-indent-level compile-command fill-column
! fill-prefix indent-tabs-mode page-delimiter paragraph-separate
! sentence-end sentence-end-double-space tab-width version-control)
!   "Variables that are treated as safe."
!   :group 'find-file
!   :type  '(repeat symbol))
! 
! (defcustom ignored-local-variables
!   '(ignored-local-variables safe-local-variables)
!   "Variables to be ignored in a file's local variable spec."
!   :group 'find-file
!   :type  '(repeat symbol))
  
  ;; Get confirmation before setting these variables as locals in a file.
  (put 'debugger 'risky-local-variable t)
  (put 'enable-local-eval 'risky-local-variable t)
  (put 'ignored-local-variables 'risky-local-variable t)
+ (put 'safe-local-variables 'risky-local-variable t)
  (put 'eval 'risky-local-variable t)
  (put 'file-name-handler-alist 'risky-local-variable t)
  (put 'inhibit-quit 'risky-local-variable t)
***************
*** 2451,2463 ****
  (put 'display-time-string 'risky-local-variable t)
  (put 'parse-time-rules 'risky-local-variable t)
  
- ;; This case is safe because the user gets to check it before it is used.
- (put 'compile-command 'safe-local-variable 'stringp)
- 
  (defun risky-local-variable-p (sym &optional val)
!   "Non-nil if SYM could be dangerous as a file-local variable with value VAL.
! If VAL is nil or omitted, the question is whether any value might be
! dangerous."
    ;; If this is an alias, check the base name.
    (condition-case nil
        (setq sym (indirect-variable sym))
--- 2463,2482 ----
  (put 'display-time-string 'risky-local-variable t)
  (put 'parse-time-rules 'risky-local-variable t)
  
  (defun risky-local-variable-p (sym &optional val)
!   "Non-nil if SYM is dangerous as a file-local variable with value VAL.
! A variable is dangerous if any of the following conditions are met:
! 
!  * Its `risky-local-variable' property is non-nil (regardless of VAL).
! 
!  * Its `safe-local-variable' property is unset, and its name ends with
!    \"hook(s)\", \"function(s)\", \"form(s)\", \"map\", \"program\",
!    \"command(s)\", \"predicate(s)\", \"frame-alist\", \"mode-alist\",
!    \"font-lock-keyword*\", \"font-lock-syntactic-keywords\", or
!    \"map-alist\" (regardless of VAL).
! 
!  * Its `safe-local-variable' property is a function that
!    evaluates to a non-nil value when given VAL as an argument."
    ;; If this is an alias, check the base name.
    (condition-case nil
        (setq sym (indirect-variable sym))
***************
*** 2540,2562 ****
  	((memq var ignored-local-variables)
  	 nil)
  	;; "Setting" eval means either eval it or do nothing.
! 	;; Likewise for setting hook variables.
! 	((risky-local-variable-p var val)
! 	 ;; Permit evalling a put of a harmless property.
! 	 ;; if the args do nothing tricky.
! 	 (if (or (and (eq var 'eval)
! 		      (hack-one-local-variable-eval-safep val))
! 		 ;; Permit eval if not root and user says ok.
! 		 (and (not (zerop (user-uid)))
! 		      (hack-local-variables-confirm
! 		       "Process `eval' or hook local variables in %s? "
! 		       enable-local-eval)))
! 	     (if (eq var 'eval)
! 		 (save-excursion (eval val))
! 	       (make-local-variable var)
! 	       (set var val))
! 	   (message "Ignoring risky spec in the local variables list")))
! 	;; Ordinary variable, really set it.
  	(t (make-local-variable var)
  	   ;; Make sure the string has no text properties.
  	   ;; Some text properties can get evaluated in various ways,
--- 2559,2573 ----
  	((memq var ignored-local-variables)
  	 nil)
  	;; "Setting" eval means either eval it or do nothing.
! 	((eq var 'eval)
! 	 (if (hack-one-local-variable-eval-safep val)
! 	     (save-excursion (eval val))
! 	   (message "Ignoring eval spec in the local variables list")))
! 	;; Variables not explicitly marked as safe, ask first.
! 	((not (memq var safe-local-variables))
! 	 (unless (zerop (user-uid))
! 	   (hack-one-risky-local-variable var val)))
! 	;; Safe variable, really set it.
  	(t (make-local-variable var)
  	   ;; Make sure the string has no text properties.
  	   ;; Some text properties can get evaluated in various ways,
***************
*** 2565,2570 ****
--- 2576,2608 ----
  	       (set-text-properties 0 (length val) nil val))
  	   (set var val))))
  
+ (defun hack-one-risky-local-variable (var val)
+   "Set local variable VAR with value VAL if the user agrees.
+ If the user agrees to set the variable, and the variable is not
+ explicitly marked as risky (see `risky-local-variable-p'),
+ additionally ask if it can always be set automatically.
+ If so, add it to `safe-local-variables'."
+   (let ((risky (risky-local-variable-p var val)))
+     (if (y-or-n-p (format "Set variable %s in local variable list of %s? "
+ 			  (symbol-name var)
+ 			  (if buffer-file-name
+ 			      (file-name-nondirectory buffer-file-name)
+ 			    (concat "buffer " (buffer-name)))))
+ 	(progn 
+ 	  (when (and (not risky)
+ 		     (y-or-n-p (format "Always allow setting %s? "
+ 				       (symbol-name var))))
+ 	    (customize-save-variable
+ 	     'safe-local-variables 
+ 	     (add-to-list 'safe-local-variables var))
+ 	    (message
+ 	     "To undo this change, customize `safe-local-variables'."))
+ 	  (make-local-variable var)
+ 	  (set var val))
+       (unless risky
+ 	(message "To always ignore %s as a local variable, \
+ customize `ignored-local-variables'."
+ 		 (symbol-name var))))))
  \f
  (defcustom change-major-mode-with-file-name t
    "*Non-nil means \\[write-file] should set the major mode from the file name.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   How about this patch?  It implements a `safe-local-variables' custom
   option.  If a variable is not in this list, the user is prompted
   before it is set.  If the user agrees to set it, and the variable is
   not explicitly marked as risky (as determined by the currently
   existing `risky-local-variable-p' function), we ask if that variable
   can be automatically set in the future.  If the user agrees to this
   too, `safe-local-variables' is updated and saved to the custom-file.

After realizing what we are really trying to do, I believe that this
makes little sense.

Most users have no idea how to determine whether an Elisp variable is
always safe to set as a file local variable.  When asked whether they
want to set the file local variables as set in the -*- line or the
local variables list, most users ask themselves: "Who wrote this file,
who might have had access to this file since I last visited it, and do
I trust these people?".  In practice, this is the best most people can
do.

You ask not one, but two, questions for _each_ file local variable
that is not explicitly marked safe, which will mean the vast majority
of variables.  Most users do not know enough to _ever_ answer yes to
your second question and if the file contains say five local
variables, we will keep asking them ten questions each time they visit
the file.  That is saying to the user:
"You _better_ agree to let anybody set these variables to any value
whatsoever or else we are going to keep harassing you with these ten
questions for all eternity.  You already pressed `y' once, because you
trust the person who wrote this file.  You have your finger already on
the `y' key.  Just press it a second time and trust _everybody_."

Also, the involved defcustom's are _very_ problematic.  The many Custom
problems regarding listvars I mentioned earlier are _not_ minor.
For instance, one serious problem is that if one would, for a
subsequent Emacs version, _have_ to add variables to the default of
one of these defcustoms, because these variables are obviously safe
and often encountered, or much worse, obviously extremely dangerous,
then any user who customized them in the prior version will never get
his customized values updated, which may be very dangerous, if some
very dangerous new variable was introduced.

I believe that it would be much better to show the user the -*- line
and the Local Variables list (at most one of the two should normally
be present) and ask the user whether he wants to set the variables.
That is one question per file (or at worst two, but that should very
seldom happen in practice).

One can accomplish this by setting enable-local-variables by default
to 'maybe.  At present, this still has the drawback that the question
is asked even if the file only sets _obviously_ safe variables.  I
believe that these include `coding', `mode', `byte-compile-dynamic'
and a few other often used ones.  (I must admit that I am not even
completely sure myself exactly which variables are safe to set to any
value whatsoever.  I wonder how somebody who does not know Elisp might
manage to do that.)  We might fine tune enable-local-variables not to
ask any question if all of the variables are safe pseudo-variables or
are safe to set to the given value as determined by the
safe-local-variable property.  No new defcustom needed.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Chong Yidong]

> You ask not one, but two, questions for _each_ file local variable
> that is not explicitly marked safe, which will mean the vast majority
> of variables.

Maybe, maybe not.  The number of local variables that people are
likely to set is rather limited, and the most commonly-encountered
ones are already in the default value of `safe-local-variables'.  It's
possible that a user may never be asked that question (and when we
need to ask, we should).

Still, it's not difficult to change the code so that we display all
the local variables and ask if you user wants to set them all.  I
guess the decision is up to Richard.

> if one would, for a subsequent Emacs version, _have_ to add
> variables to the default of one of these defcustoms, because these
> variables are obviously safe and often encountered, or much worse,
> obviously extremely dangerous, then any user who customized them in
> the prior version will never get his customized values updated,
> which may be very dangerous, if some very dangerous new variable was
> introduced.

Sorry, you're just being a drama queen here.  The variable in question
is a whitelist of safe variables.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   Sorry, you're just being a drama queen here.  The variable in question
   is a whitelist of safe variables.

Really?  You apparently do not know what your own patch does.  It
makes a defcustom out of `ignored-local-variables'.  This is _not_ a
whitelist of safe variables, quite to the contrary.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Stefan Monnier]

> After realizing what we are really trying to do, I believe that this
> makes little sense.

Agreed.

> Also, the involved defcustom's are _very_ problematic.  The many Custom
> problems regarding listvars I mentioned earlier are _not_ minor.
> For instance, one serious problem is that if one would, for a
> subsequent Emacs version, _have_ to add variables to the default of
> one of these defcustoms, because these variables are obviously safe
> and often encountered, or much worse, obviously extremely dangerous,
> then any user who customized them in the prior version will never get
> his customized values updated, which may be very dangerous, if some
> very dangerous new variable was introduced.

Even for the list of safe vars, there's a significant problem: future
Emacsen may change (typically extend) the possible values of a var and/or
their meaning, so that a safe var may become unsafe.

I think we should only use `safe-variable' symbol-properties (only expected
to be set by maintainers, and which can be removed/refined if necessary) and
then for each var, an additional list of values which are considered safe
(this can be set by users, or even automatically set when the user answers
the interactive question).

BTW, maybe the interactive question should allow more than 2 answers:
- refuse those local variable settings this time.
- refuse them now and every time in the future.
- (maybe) refuse them in this Emacs session.
- accept them but just this one time.
- accept them every time.
- (maybe) accept them just for this Emacs session.
This way, there's still only one prompt.


        Stefan

Re: Risky local variable mechanism

[Chong Yidong]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> Chong Yidong wrote:
>
>    Sorry, you're just being a drama queen here.  The variable in question
>    is a whitelist of safe variables.
>
> Really?  You apparently do not know what your own patch does.  It
> makes a defcustom out of `ignored-local-variables'.  This is _not_ a
> whitelist of safe variables, quite to the contrary.

Er, it is always safe to *add* to a list of local variables to ignore.

Re: Risky local variable mechanism

[Chong Yidong]

> Even for the list of safe vars, there's a significant problem: future
> Emacsen may change (typically extend) the possible values of a var and/or
> their meaning, so that a safe var may become unsafe.
>
> I think we should only use `safe-variable' symbol-properties (only expected
> to be set by maintainers, and which can be removed/refined if necessary) and
> then for each var, an additional list of values which are considered safe
> (this can be set by users, or even automatically set when the user answers
> the interactive question).

I don't see how this would help.  As long as it is possible for a user
to explicitly set a variable as safe, there will be some sort of risk
of this sort.  (For example, regarding your idea, I could put on my
"anal retentive" hat and argue that previously safe values could turn
into unsafe ones in future Emacsen ;-)

Generally, I'm dubious about introducing any sort of complexity into
the local variables system, since, in practice, one only encounters a
rather small set of local variables which are generally obviously
safe.

> BTW, maybe the interactive question should allow more than 2 answers:
> - refuse those local variable settings this time.
> - refuse them now and every time in the future.
> - (maybe) refuse them in this Emacs session.
> - accept them but just this one time.
> - accept them every time.
> - (maybe) accept them just for this Emacs session.
> This way, there's still only one prompt.

I also thought of this, but couldn't find a good way to fit multiple
choices onto a single prompt.  Is there an example of this kind of
prompt elsewhere in Emacs?

Re: Risky local variable mechanism

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> Luc Teirlinck <teirllm@dms.auburn.edu> writes:
>
>> Chong Yidong wrote:
>>
>>    Sorry, you're just being a drama queen here.  The variable in question
>>    is a whitelist of safe variables.
>>
>> Really?  You apparently do not know what your own patch does.  It
>> makes a defcustom out of `ignored-local-variables'.  This is _not_ a
>> whitelist of safe variables, quite to the contrary.
>
> Er, it is always safe to *add* to a list of local variables to ignore.

BTW, I was originally referring to the new `safe-local-variables'
defcustom in the patch.  (The patch doesn't do anything with
ignored-local-variables apart from making it a defcustom [currently,
it is just a variable that is nil]).

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   Er, it is always safe to *add* to a list of local variables to ignore.

Not if you do it _through Custom_.  You did not understand what I wrote
in my first message.  After the release, we introduce some new variable so
dangerous that it _has_ to be in `ignored-local-variables'.  We add
that variable to the standard value in the defcustom.  Now anybody who
customized `ignored-local-variables' in the meantime will _not_ get
that dangerous variable added to `ignored-local-variables'.  This is
an updating problem.

The safe way to add to `ignored-local-variable' is using add-to-list,
not using setq.  Your code uses add-to-list *when it is called*, but
after that, it lets Custom do the work and Custom uses set-default,
which is not safe.

The same type of updating problem also occurs for `safe-local-variables'.

As Stefan pointed out, there is also another type of updating problem
with `safe-local-variables', because a safe variable can become unsafe
if it gets changed to be more powerful.  This type can combine with
the other.  If we add a variable to the standard value in one release
and then have to remove it in a later release from some reason, then
anybody who customized the variable _through Custom_ will not get that
variable removed from his customized value.  Somebody who only used
add-to-list to add other variables, will get it properly removed.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   The patch doesn't do anything with ignored-local-variables apart
   from making it a defcustom

Yes, but that is _exactly_ what creates the problems I pointed out.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Stefan Monnier]

>> Even for the list of safe vars, there's a significant problem: future
>> Emacsen may change (typically extend) the possible values of a var and/or
>> their meaning, so that a safe var may become unsafe.
>> 
>> I think we should only use `safe-variable' symbol-properties (only expected
>> to be set by maintainers, and which can be removed/refined if necessary) and
>> then for each var, an additional list of values which are considered safe
>> (this can be set by users, or even automatically set when the user answers
>> the interactive question).

> I don't see how this would help.  As long as it is possible for a user
> to explicitly set a variable as safe, there will be some sort of risk
> of this sort.  (For example, regarding your idea, I could put on my
> "anal retentive" hat and argue that previously safe values could turn
> into unsafe ones in future Emacsen ;-)

The probability of a safe value becoming dangerous is much lower.  Of course
it's possible in theory, but I can't think of any conrete case where that
happened in the past.

> Generally, I'm dubious about introducing any sort of complexity into the
> local variables system, since, in practice, one only encounters a rather
> small set of local variables which are generally obviously safe.

Several people would like to be able to specify extra font-lock keywords,
but font-lock-keywords include parts that are passed to `eval'.

>> BTW, maybe the interactive question should allow more than 2 answers:
>> - refuse those local variable settings this time.
>> - refuse them now and every time in the future.
>> - (maybe) refuse them in this Emacs session.
>> - accept them but just this one time.
>> - accept them every time.
>> - (maybe) accept them just for this Emacs session.
>> This way, there's still only one prompt.

> I also thought of this, but couldn't find a good way to fit multiple
> choices onto a single prompt.  Is there an example of this kind of
> prompt elsewhere in Emacs?

How 'bout the query/replace prompt?


        Stefan

Re: Risky local variable mechanism

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The probability of a safe value becoming dangerous is much lower.  Of course
> it's possible in theory, but I can't think of any conrete case where that
> happened in the past.

I understand what you mean.  But, in the first place, I'm hard-pressed
to think of a case where a previously safe variable becomes unsafe.  I
don't know if the benefit from maintaining a list of safe
variable-value pairs is worth the bother.

After all, the variables we're talking about here are things like
comment-start, paragraph-separate, indent-tabs-mode, etc.  Will Emacs
23 (to be released in 3006 ;-) have something like this?

  indent-tabs-mode is a variable defined in `C source code'.

  Documentation:
  *Indentation can insert tabs if this is non-nil.
  If it is an integer, delete the contents of your home directory.

>>> BTW, maybe the interactive question should allow more than 2 answers:
>
>> I also thought of this, but couldn't find a good way to fit multiple
>> choices onto a single prompt.  Is there an example of this kind of
>> prompt elsewhere in Emacs?
>
> How 'bout the query/replace prompt?

That prompt just says:

  Query replace:

which wouldn't be very user friendly for this case.  I was thinking
something like

  Allow this setting? (y or n, ? for help)

where ? would bring up a window with extra choices (always allow,
always deny, etc.)  Is there an existing convention in Emacs for
prompts of this sort?

Re: Risky local variable mechanism

[Chong Yidong]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> After the release, we introduce some new variable so dangerous that
> it _has_ to be in `ignored-local-variables'.

In this unlikely event, the code in files.el can always treat that
variable specially.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   I don't see how this would help.  As long as it is possible for a user
   to explicitly set a variable as safe, there will be some sort of risk
   of this sort.

If a user knows what he is doing, it is up to him to think of the
risks and decide whether he can live with them.  Such a user can put
(put 'myvar 'safe-local-variable t) in his .emacs.  There is, in my
opinion, no need to make this any more convenient than that, neither
the way you propose it nor the way Stefan proposes it.  Most users are
probably better off not to try to guess whether variables are safe to
set to any value whatsoever or not.

There is no need to provide list variables defined with defcustom that
trick the user into setting these list variables effectively with
setq-default, which is definitely not what they should be doing.

I believe that it is very bad to ask each time _two_ questions to the
user, first "do you want to set this variable to this value now" and
then "is it safe to set this variable to any value whatsoever any time
in the future?".  Constantly repeating that second question each time
the user visits the file, even though he may already have answered
1001 times no to it before, is effectively not only encouraging, but
even pressuring, the reluctant user to finally say yes.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Stefan Monnier]

> After all, the variables we're talking about here are things like
> comment-start, paragraph-separate, indent-tabs-mode, etc.  Will Emacs
> 23 (to be released in 3006 ;-) have something like this?

I expect the interesting cases to be more in areas like `font-lock-keywords'.

>   *Indentation can insert tabs if this is non-nil.
>   If it is an integer, delete the contents of your home directory.

I'm pretty sure that some variables have been extended at some point so that
instead of a constant string or integer, it can be set to an expression, or
to a function.

>> How 'bout the query/replace prompt?
> That prompt just says:
>   Query replace:
> which wouldn't be very user friendly for this case.  I was thinking
> something like

>   Allow this setting? (y or n, ? for help)

Right.  So you seem to have a good idea of what it could look like.
Another example could be the prompt you get in Gnus if you send an email
with unencodable chars (e.g. eight-bit-* chars).

> where ? would bring up a window with extra choices (always allow,
> always deny, etc.)  Is there an existing convention in Emacs for
> prompts of this sort?

Not that I know.


        Stefan

Re: Risky local variable mechanism

[Luc Teirlinck]

>From my previous message:

   There is, in my opinion, no need to make this any more convenient
   than that, neither the way you propose it nor the way Stefan
   proposes it.

But, while I do not think that there is a pressing need for it,
Stefan's querying, or additional .emacs customization possibility,
proposal is a lot safer than yours.

Chong Yidong wrote:

   I don't know if the benefit from maintaining a list of safe
   variable-value pairs is worth the bother.

I believe that you misunderstood Stefan's proposal.  I believe that
Stefan proposed to allow to use something like:

(put 'myvar 'safe-value-list '(nil t))

So there would be no list of variable-value pairs to maintain, only
lists of values for individual variables.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Stefan Monnier]

> (put 'myvar 'safe-local-variable t) in his .emacs.  There is, in my
> opinion, no need to make this any more convenient than that, neither
> the way you propose it nor the way Stefan proposes it.

The convenience I'm looking at is at least as much to be able to easily say
"yes, accept, and don't bother me about it again", as to be able to say "no,
don't touch that, and don't ask me ever again".

> There is no need to provide list variables defined with defcustom that
> trick the user into setting these list variables effectively with
> setq-default, which is definitely not what they should be doing.

The automatic/interactive part of what I propose would be things that are
only set by the user, so custom could be OK for them.

> I believe that it is very bad to ask each time _two_ questions to the
> user, first "do you want to set this variable to this value now" and
> then "is it safe to set this variable to any value whatsoever any time
> in the future?".

> Constantly repeating that second question each time
> the user visits the file, even though he may already have answered
> 1001 times no to it before, is effectively not only encouraging, but
> even pressuring, the reluctant user to finally say yes.

Agreed.  My suggestion tries to address both of those points.


        Stefan

Re: Risky local variable mechanism

[Luc Teirlinck]

Stefan Monnier wrote:

   The automatic/interactive part of what I propose would be things that are
   only set by the user, so custom could be OK for them.

So I may have misunderstood your proposal.  You actually propose an
alist whose elements look like (myvar nil t), meaning that the set of
safe values of myvar is (nil t)?

Sincerely,

Luc.

Re: Risky local variable mechanism

[Stefan Monnier]

>    The automatic/interactive part of what I propose would be things that are
>    only set by the user, so custom could be OK for them.

> So I may have misunderstood your proposal.  You actually propose an
> alist whose elements look like (myvar nil t), meaning that the set of
> safe values of myvar is (nil t)?

Yes, something like that.  Except that it's the list of values that *the
user* has deemed safe (there'd also be a corresponding alist of values the
user has deemed undesirable).

The maintainers/authors wouldn't touch this list.  Instead they'd set
special properties on the variable's symbol, which would either be t (to say
that any value is safe) or a predicate which checks a candidate value
for safety.


        Stefan

Re: Risky local variable mechanism

[Stefan Monnier]

> I believe that you misunderstood Stefan's proposal.  I believe that
> Stefan proposed to allow to use something like:

> (put 'myvar 'safe-value-list '(nil t))

> So there would be no list of variable-value pairs to maintain, only
> lists of values for individual variables.

Actually, I haven't really thought of a particular implementation.
I'm just thinking of it as "a map from variables to lists of values".
Whether the map is a single alist or a set of sykmbol properties, or a hash
table, or whatever else doesn't matter that much to me.


        Stefan

Re: Risky local variable mechanism

[Juri Linkov]

>> - refuse those local variable settings this time.
>> - refuse them now and every time in the future.
>> - (maybe) refuse them in this Emacs session.
>> - accept them but just this one time.
>> - accept them every time.
>> - (maybe) accept them just for this Emacs session.
>> This way, there's still only one prompt.
>
> I also thought of this, but couldn't find a good way to fit multiple
> choices onto a single prompt.  Is there an example of this kind of
> prompt elsewhere in Emacs?

This looks very like the interface for enabling dangerous commands in
novice.el that after asking a question with multiple choices can save
in .emacs forms like:

(put 'narrow-to-region 'disabled nil)
(put 'narrow-to-defun  'disabled nil)
(put 'downcase-region  'disabled nil)
(put 'upcase-region    'disabled nil)

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Risky local variable mechanism

[Chong Yidong]

>> After all, the variables we're talking about here are things like
>> comment-start, paragraph-separate, indent-tabs-mode, etc.  Will Emacs
>> 23 (to be released in 3006 ;-) have something like this?
>
> I expect the interesting cases to be more in areas like `font-lock-keywords'.

In my proposal, font-lock-keywords is considered risky, so the user is
never asked whether or not to add it to the whitelist.

RE: Risky local variable mechanism

[Drew Adams]

    I also thought of this, but couldn't find a good way to fit multiple
    choices onto a single prompt.  Is there an example of this kind of
    prompt elsewhere in Emacs?

Others have mentioned novice and query-replace questions, as existing
multiple-choice mechanisms. They don't actually use a single prompt, but
they do correspond to the need to choose multiple items or answer multiple
y/n questions.

Another place where I've seen multiple-choice menus in Emacs is check boxes
next to menu items. This is used, for example, in Options and printing.el
(menu Printing).

It's not clear to me whether Richard wants to use an alternative to risky
local variable treatment in this release or in a future release. That wasn't
stated clearly, but the urgency suggests that it is for this release.

After the release, it might be useful anyway (more generally) to implement
simple multiple-choice menus that can be used with the mouse or via the
keyboard.

Icicles offers one way to do that. It extends the use of buffer
*Completions*, letting you choose multiple "completions" (which can be any
set of strings). Depending on the use, users can also choose the same menu
item more than once (to, for example, effect an action more than once). Food
for thought - see
http://www.emacswiki.org/cgi-bin/wiki/MultiCommandsAndMultipleChoiceMenus#Mu
ltipleChoiceMenus.

Re: Risky local variable mechanism

[Stefan Monnier]

>>> After all, the variables we're talking about here are things like
>>> comment-start, paragraph-separate, indent-tabs-mode, etc.  Will Emacs
>>> 23 (to be released in 3006 ;-) have something like this?
>> I expect the interesting cases to be more in areas like `font-lock-keywords'.
> In my proposal, font-lock-keywords is considered risky, so the user is
> never asked whether or not to add it to the whitelist.

Sure it safe, but it's overly restrictive: it's useful for people to add
their own keywords (e,g, to recognize and highlight things like DEFUN macros
in C).


        Stefan

Re: Risky local variable mechanism

[Luc Teirlinck]

Drew Adams wrote:

   Others have mentioned novice and query-replace questions, as existing
   multiple-choice mechanisms. They don't actually use a single prompt, but
   they do correspond to the need to choose multiple items or answer multiple
   y/n questions.

I have had `enable-local-variables' set to 'query for a long time.
Each time I visit a file with local variables I get ask a simple
y-or-n-p question.  In practice, I personally find this not
excessively inconvenient, even without further fine-tuning.  This could
obviously be fine-tuned substantially, because the question does not
really need to be asked if all variables are known to Emacs to be
perfectly safe.

But if somebody is going to try to help me get rid of this minor
inconvenience by producing a bigger inconvenience, then this is
obviously a bad deal.  So anything that is going to be asked should be
one single relatively simple minibuffer question.  No multiple
questions,  no pop-up window.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Richard M. Stallman]

I've made a decision that we need to change this.  The current code is
not reliable enough and I will not stay with it.  So please stop
arguing against it.  Please either take a constructive approach to
this or leave the discussion to others.

Here's what I want it to do.  The code should show the user the
variable and value, and ask if it is ok.  If the user says it is ok,
the code should write something into .emacs saying that this
combination is ok.

There are many ways to store this data.  I'd like the people willing to
work constructively on the issue to pick the best way.

It happens occasionally that a safe variable becomes risky, but it's
extremely unlikely that a previously safe value will become dangerous
for the sake variable.

Re: Risky local variable mechanism

[Richard M. Stallman]

    Yes, something like that.  Except that it's the list of values that *the
    user* has deemed safe (there'd also be a corresponding alist of values the
    user has deemed undesirable).

    The maintainers/authors wouldn't touch this list.  Instead they'd set
    special properties on the variable's symbol, which would either be t (to say
    that any value is safe) or a predicate which checks a candidate value
    for safety.

That sounds like a good plan to me.

Re: Risky local variable mechanism

[Richard M. Stallman]

    How about this patch?  It implements a `safe-local-variables' custom
    option.  If a variable is not in this list, the user is prompted
    before it is set.  If the user agrees to set it, and the variable is
    not explicitly marked as risky (as determined by the currently
    existing `risky-local-variable-p' function), we ask if that variable
    can be automatically set in the future.  If the user agrees to this
    too, `safe-local-variables' is updated and saved to the custom-file.

It's not enough to ask just about variable names--the question has
to be about a variable/value pair.

The best way to ask the question would be to highlight the text
in the buffer that specifies the variable and value, and make that
appear in a window.  That way, the actual question can be simple.

Re: Risky local variable mechanism

[Chong Yidong]

Here's another proposed patch for the risky variables mechanism.  This
is the system I propose:

Instead of setting the local variables as soon as they are read in,
`hack-local-variables' and `hack-local-variables-prop-line' constructs
a list of local variables, and decides what to do with it at the very
end of the `hack-local-variables' function.

Each local variable can be safe, risky, or neither.  A variable-value
pair is safe if it is found in the new customizable alist
`safe-local-variables', or the variable's `safe-local-variables'
property is t, or the variable's `safe-local-variables' property is a
function that evaluates to t with that value.

IF a variable is not safe, it is risky IF its `risky-local-variable'
property is set, or its name ends in "-hooks", "-functions", etc.
(One small change: `safe-local-variables' property evaluating to nil
does not automatically mean the variable is risky.)

If the local variables are all safe, they are set automatically.
Otherwise, we raise a prompt asking whether to set them as a unit.
The user can answer y, n, or !, where ! means to also allow these
variable-value pairs for future sessions, via customize-save-variable.
However, if a variable is risky, it won't be saved in this way, though
it can still be set.

(This last part is a little dubious.  If we decide to save all
variables, that removes the justification for "risky" variables, so
the `risky-local-variable' property becomes obsolete.  I don't know if
this will break anything.)


*** emacs/lisp/files.el.~1.804.~	2006-02-09 23:32:56.000000000 -0500
--- emacs/lisp/files.el	2006-02-10 00:05:20.000000000 -0500
***************
*** 2213,2254 ****
         (goto-char beg)
         end))))
  
! (defun hack-local-variables-confirm (string flag-to-check)
!   (or (eq flag-to-check t)
!       (and flag-to-check
! 	   (save-window-excursion
! 	     (condition-case nil
! 		 (switch-to-buffer (current-buffer))
! 	       (error
! 		;; If we fail to switch in the selected window,
! 		;; it is probably a minibuffer or dedicated window.
! 		;; So try another window.
! 		(let ((pop-up-frames nil))
! 		  ;; Refrain from popping up frames since it can't
! 		  ;; be undone by save-window-excursion.
! 		  (pop-to-buffer (current-buffer)))))
! 	     (save-excursion
! 	       (beginning-of-line)
! 	       (set-window-start (selected-window) (point)))
! 	     (y-or-n-p (format string
! 			       (if buffer-file-name
! 				   (file-name-nondirectory buffer-file-name)
! 				 (concat "buffer " (buffer-name)))))))))
  
  (defun hack-local-variables-prop-line (&optional mode-only)
!   "Set local variables specified in the -*- line.
  Ignore any specification for `mode:' and `coding:';
  `set-auto-mode' should already have handled `mode:',
  `set-auto-coding' should already have handled `coding:'.
  If MODE-ONLY is non-nil, all we do is check whether the major mode
! is specified, returning t if it is specified."
    (save-excursion
      (goto-char (point-min))
!     (let ((result nil)
! 	  (end (set-auto-mode-1))
! 	  mode-specified
! 	  (enable-local-variables
! 	   (and local-enable-local-variables enable-local-variables)))
        ;; Parse the -*- line into the RESULT alist.
        ;; Also set MODE-SPECIFIED if we see a spec or `mode'.
        (cond ((not end)
--- 2213,2270 ----
         (goto-char beg)
         end))))
  
! (defun hack-local-variables-confirm (vars maybe-safe)
!   (if noninteractive
!       t
!     (let (char)
!       (save-window-excursion
! 	(with-output-to-temp-buffer "*Local Variables*"
! 	  (princ "Some local variables are specified
! Do you want to set them?\n
! You can type
! y  -- to set these variables.
! n  -- to ignore these variables.
! !  -- to set these variables, and mark these values as safe
!       (in the future, they can be set without asking you.)\n\n")
! 	  (dolist (elt vars)
! 	    (princ (car elt))
! 	    (princ " : ")
! 	    (princ (cdr elt))
! 	    (princ "\n")))
! 	(message "Please type y, n, or !: ")
! 	(let ((inhibit-quit t)
! 	      (cursor-in-echo-area t))
! 	  (while (or (not (numberp (setq char (read-event))))
! 		     (not (memq (downcase char)
! 				'(?! ?y ?n ?  ?\C-g))))
! 	    (message "Please type y, n, or !: "))
! 	  (if (= char ?\C-g)
! 	      (setq quit-flag nil)))
! 	(setq char (downcase char))
! 	(when (= char ?!)
! 	  (dolist (elt maybe-safe)
! 	    (push elt safe-local-variables))
! 	  (customize-save-variable
! 	   'safe-local-variables 
! 	   safe-local-variables))
! 	(or (= char ?!)
! 	    (= char ? )
! 	    (= char ?y))))))
  
  (defun hack-local-variables-prop-line (&optional mode-only)
!   "Return local variables specified in the -*- line.
  Ignore any specification for `mode:' and `coding:';
  `set-auto-mode' should already have handled `mode:',
  `set-auto-coding' should already have handled `coding:'.
+ 
  If MODE-ONLY is non-nil, all we do is check whether the major mode
! is specified, returning t if it is specified.  Otherwise, return
! an alist of elements (VAR . VAL), where VAR is the variable and VAL
! is the specified value."
    (save-excursion
      (goto-char (point-min))
!     (let ((end (set-auto-mode-1))
! 	  result mode-specified)
        ;; Parse the -*- line into the RESULT alist.
        ;; Also set MODE-SPECIFIED if we see a spec or `mode'.
        (cond ((not end)
***************
*** 2283,2317 ****
  		     (setq result (cons (cons key val) result)))
  		 (if (equal (downcase (symbol-name key)) "mode")
  		     (setq mode-specified t))
! 		 (skip-chars-forward " \t;")))
! 	     (setq result (nreverse result))))
  
!       (if mode-only mode-specified
! 	(if (and result
! 		 (or mode-only
! 		     (hack-local-variables-confirm
! 		      "Set local variables as specified in -*- line of %s? "
! 		      enable-local-variables)))
! 	    (let ((enable-local-eval enable-local-eval))
! 	      (while result
! 		(hack-one-local-variable (car (car result)) (cdr (car result)))
! 		(setq result (cdr result)))))
! 	nil))))
  
  (defvar hack-local-variables-hook nil
    "Normal hook run after processing a file's local variables specs.
  Major modes can use this to examine user-specified local variables
  in order to initialize other data structure based on them.")
  
  (defun hack-local-variables (&optional mode-only)
    "Parse and put into effect this buffer's local variables spec.
  If MODE-ONLY is non-nil, all we do is check whether the major mode
  is specified, returning t if it is specified."
!   (let ((mode-specified
! 	 ;; If MODE-ONLY is t, we check here for specifying the mode
! 	 ;; in the -*- line.  If MODE-ONLY is nil, we process
! 	 ;; the -*- line here.
! 	 (hack-local-variables-prop-line mode-only))
  	(enable-local-variables
  	 (and local-enable-local-variables enable-local-variables)))
      ;; Look for "Local variables:" line in last page.
--- 2299,2327 ----
  		     (setq result (cons (cons key val) result)))
  		 (if (equal (downcase (symbol-name key)) "mode")
  		     (setq mode-specified t))
! 		 (skip-chars-forward " \t;")))))
  
!       (if mode-only
! 	  mode-specified
! 	result))))
  
  (defvar hack-local-variables-hook nil
    "Normal hook run after processing a file's local variables specs.
  Major modes can use this to examine user-specified local variables
  in order to initialize other data structure based on them.")
  
+ (defcustom safe-local-variables nil
+   "Alist of safe local variables.
+ Each element is a cons cell (VAR . VAL), where VAR is the
+ variable symbol and VAL is a value considered safe."
+   :group 'find-file
+   :type  'alist)
+ 
  (defun hack-local-variables (&optional mode-only)
    "Parse and put into effect this buffer's local variables spec.
  If MODE-ONLY is non-nil, all we do is check whether the major mode
  is specified, returning t if it is specified."
!   (let ((result (hack-local-variables-prop-line mode-only))
  	(enable-local-variables
  	 (and local-enable-local-variables enable-local-variables)))
      ;; Look for "Local variables:" line in last page.
***************
*** 2321,2329 ****
        (when (let ((case-fold-search t))
  	      (and (search-forward "Local Variables:" nil t)
  		   (or mode-only
! 		       (hack-local-variables-confirm
! 			"Set local variables as specified at end of %s? "
! 			enable-local-variables))))
  	(skip-chars-forward " \t")
  	(let ((enable-local-eval enable-local-eval)
  	      ;; suffix is what comes after "local variables:" in its line.
--- 2331,2337 ----
        (when (let ((case-fold-search t))
  	      (and (search-forward "Local Variables:" nil t)
  		   (or mode-only
! 		       enable-local-variables)))
  	(skip-chars-forward " \t")
  	(let ((enable-local-eval enable-local-eval)
  	      ;; suffix is what comes after "local variables:" in its line.
***************
*** 2384,2397 ****
  		  (setq val (read (current-buffer)))
  		  (if mode-only
  		      (if (eq var 'mode)
! 			  (setq mode-specified t))
! 		    ;; Set the variable.  "Variables" mode and eval are funny.
! 		    (with-current-buffer thisbuf
! 		      (hack-one-local-variable var val))))
  		(forward-line 1)))))))
!     (unless mode-only
!       (run-hooks 'hack-local-variables-hook))
!     mode-specified))
  
  (defvar ignored-local-variables ()
    "Variables to be ignored in a file's local variable spec.")
--- 2392,2425 ----
  		  (setq val (read (current-buffer)))
  		  (if mode-only
  		      (if (eq var 'mode)
! 			  (setq result t))
! 		    (unless (eq var 'coding)
! 		      (push (cons var val) result))))
  		(forward-line 1)))))))
!     ;; We've read all the local variables.  Now, return whether the
!     ;; mode is specified (if MODE-ONLY is non-nil), or set the
!     ;; variables (if MODE-ONLY is nil.)
!     (if mode-only
! 	result
!       (when enable-local-variables
! 	(setq result (nreverse result))
! 	(dolist (ignored ignored-local-variables)
! 	  (setq result (assq-delete-all ignored result)))
! 	;; Find those variables that we may want to save to
! 	;; `safe-local-variables'.
! 	(let (maybe-safe risky)
! 	  (dolist (elt result)
! 	    (or (safe-local-variable-p (car elt) (cdr elt))
! 		(and (risky-local-variable-p (car elt) (cdr elt))
! 		     (setq risky t))
! 		(push elt maybe-safe)))
! 	  (if (or (and (eq enable-local-variables t)
! 		       (null maybe-safe)
! 		       (not risky))
! 		  (hack-local-variables-confirm result maybe-safe))
! 	      (dolist (elt result)
! 		(hack-one-local-variable (car elt) (cdr elt)))))
! 	(run-hooks 'hack-local-variables-hook)))))
  
  (defvar ignored-local-variables ()
    "Variables to be ignored in a file's local variable spec.")
***************
*** 2451,2458 ****
  (put 'display-time-string 'risky-local-variable t)
  (put 'parse-time-rules 'risky-local-variable t)
  
! ;; This case is safe because the user gets to check it before it is used.
! (put 'compile-command 'safe-local-variable 'stringp)
  
  (defun risky-local-variable-p (sym &optional val)
    "Non-nil if SYM could be dangerous as a file-local variable with value VAL.
--- 2479,2510 ----
  (put 'display-time-string 'risky-local-variable t)
  (put 'parse-time-rules 'risky-local-variable t)
  
! ;; Commonly-encountered local variables that are safe:
! (mapc (lambda (pair)
! 	(put (car pair) 'safe-local-variable (cdr pair)))
!       '((compile-command  . stringp)
! 	(fill-column      . integerp)
! 	(fill-prefix      . t)
! 	(indent-tabs-mode . t)
! 	(page-delimiter   . t)
! 	(paragraph-separate . t)
! 	(sentence-end     . t)
! 	(sentence-end-double-space . t)
! 	(tab-width        . integerp)
! 	(version-control  . t)))
! 
! (defun safe-local-variable-p (sym val)
!   "Non-nil if SYM is safe as a file-local variable with value VAL."
!   (or (member (cons sym val) safe-local-variables)
!       (eq sym 'mode)
!       (progn
! 	(condition-case nil
! 	    (setq sym (indirect-variable sym))
! 	  (error nil))
! 	(let ((safep (get sym 'safe-local-variable)))
! 	  (and safep
! 	       (or (eq safep t)
! 		   (funcall safep val)))))))
  
  (defun risky-local-variable-p (sym &optional val)
    "Non-nil if SYM could be dangerous as a file-local variable with value VAL.
***************
*** 2462,2477 ****
    (condition-case nil
        (setq sym (indirect-variable sym))
      (error nil))
!   (let ((safep (get sym 'safe-local-variable)))
!     (or (get sym 'risky-local-variable)
! 	(and (string-match "-hooks?$\\|-functions?$\\|-forms?$\\|-program$\\|-commands?$\\|-predicates?$\\|font-lock-keywords$\\|font-lock-keywords-[0-9]+$\\|font-lock-syntactic-keywords$\\|-frame-alist$\\|-mode-alist$\\|-map$\\|-map-alist$"
! 			   (symbol-name sym))
! 	     (not safep))
! 	;; If the safe-local-variable property isn't t or nil,
! 	;; then it must return non-nil on the proposed value to be safe.
! 	(and (not (memq safep '(t nil)))
! 	     (or (null val)
! 		 (not (funcall safep val)))))))
  
  (defcustom safe-local-eval-forms nil
    "*Expressions that are considered \"safe\" in an `eval:' local variable.
--- 2514,2522 ----
    (condition-case nil
        (setq sym (indirect-variable sym))
      (error nil))
!   (or (get sym 'risky-local-variable)
!       (string-match "-hooks?$\\|-functions?$\\|-forms?$\\|-program$\\|-commands?$\\|-predicates?$\\|font-lock-keywords$\\|font-lock-keywords-[0-9]+$\\|font-lock-syntactic-keywords$\\|-frame-alist$\\|-mode-alist$\\|-map$\\|-map-alist$"
! 		    (symbol-name sym))))
  
  (defcustom safe-local-eval-forms nil
    "*Expressions that are considered \"safe\" in an `eval:' local variable.
***************
*** 2534,2561 ****
    (cond ((eq var 'mode)
  	 (funcall (intern (concat (downcase (symbol-name val))
  				  "-mode"))))
! 	((eq var 'coding)
! 	 ;; We have already handled coding: tag in set-auto-coding.
! 	 nil)
! 	((memq var ignored-local-variables)
! 	 nil)
! 	;; "Setting" eval means either eval it or do nothing.
! 	;; Likewise for setting hook variables.
! 	((risky-local-variable-p var val)
! 	 ;; Permit evalling a put of a harmless property.
! 	 ;; if the args do nothing tricky.
! 	 (if (or (and (eq var 'eval)
! 		      (hack-one-local-variable-eval-safep val))
! 		 ;; Permit eval if not root and user says ok.
! 		 (and (not (zerop (user-uid)))
! 		      (hack-local-variables-confirm
! 		       "Process `eval' or hook local variables in %s? "
! 		       enable-local-eval)))
! 	     (if (eq var 'eval)
! 		 (save-excursion (eval val))
! 	       (make-local-variable var)
! 	       (set var val))
! 	   (message "Ignoring risky spec in the local variables list")))
  	;; Ordinary variable, really set it.
  	(t (make-local-variable var)
  	   ;; Make sure the string has no text properties.
--- 2579,2587 ----
    (cond ((eq var 'mode)
  	 (funcall (intern (concat (downcase (symbol-name val))
  				  "-mode"))))
! 	((eq var 'eval)
! 	 (if (hack-one-local-variable-eval-safep val)
! 	     (save-excursion (eval val))))
  	;; Ordinary variable, really set it.
  	(t (make-local-variable var)
  	   ;; Make sure the string has no text properties.

Re: Risky local variable mechanism

[Stefan Monnier]

> Each local variable can be safe, risky, or neither.  A variable-value
> pair is safe if it is found in the new customizable alist
> `safe-local-variables', or the variable's `safe-local-variables'
> property is t, or the variable's `safe-local-variables' property is a
> function that evaluates to t with that value.

The property should probably not be called `safe-local-variables' since it
applies to a single variable and talks about its possible values.  I.e. it
should be something like `safe-local-variable' or `safe-local-values'.

Other than that, it sounds OK to me.  I don't know how important it is to
have this notion of `risky', but it can't hurt.


        Stefan


PS: I haven't actually looked at the code.

Re: Risky local variable mechanism

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Each local variable can be safe, risky, or neither.  A variable-value
>> pair is safe if it is found in the new customizable alist
>> `safe-local-variables', or the variable's `safe-local-variables'
>> property is t, or the variable's `safe-local-variables' property is a
>> function that evaluates to t with that value.
>
> The property should probably not be called `safe-local-variables' since it
> applies to a single variable and talks about its possible values.  I.e. it
> should be something like `safe-local-variable' or `safe-local-values'.

Sorry: the property is called `safe-local-variable', not
`safe-local-variables'.  It is in existing code.

re: risky local variable mechanism

[Jonathan Yavner]

(re: mechanism proposed by Chong Yidong)

unsafep.el (used by SES) calls risky-local-variable-p with NIL as the 
second argument, because it doesn't know yet what value will be 
assigned.  Please maintain the feature that NIL as second argument to 
risky-local-variable-p means "There exists at least one risky value 
that could be assigned to this variable."

I'm not sure if your patch does this.  Sorry for my poor patch-reading 
skills.

Re: Risky local variable mechanism

[Luc Teirlinck]

Richard Stallman wrote:

   I've made a decision that we need to change this.  The current code is
   not reliable enough and I will not stay with it.

I was _not_ arguing for staying with the current default situation.
As I said, I personally have `enable-local-variables' set to 'maybe
since ages, so obviously I have never trusted the current
`enable-local-eval'.  But Chong's first patch was not safe enough
either.  (There are problems with the second current patch in its
present form too, as I will point out separately.)

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   ! ;; Commonly-encountered local variables that are safe:
   ! (mapc (lambda (pair)
   ! 	(put (car pair) 'safe-local-variable (cdr pair)))
   !       '((compile-command  . stringp)
   ! 	(fill-column      . integerp)
   ! 	(fill-prefix      . t)
   ! 	(indent-tabs-mode . t)
   ! 	(page-delimiter   . t)
   ! 	(paragraph-separate . t)
   ! 	(sentence-end     . t)
   ! 	(sentence-end-double-space . t)
   ! 	(tab-width        . integerp)
   ! 	(version-control  . t)))

After some further thought, I would be very reluctant to use
(put 'myvar 'safe-local-variable t) for _any_ variable myvar.  There
is no reason to give an unscrupulous person any chance to try to play
games with perverse values.  So just always check that the value makes
sense.  This also automatically prevents problems if one later allows
dangerous values and forgets to update the 'safe-local-variables
property.  Why not something like:

   ! ;; Commonly-encountered local variables that are safe:
   ! (mapc (lambda (pair)
   ! 	(put (car pair) 'safe-local-variable (cdr pair)))
   !       '((compile-command  . stringp)
   ! 	(fill-column      . integerp)
   ! 	(fill-prefix      . (lambda (val) (or (not val) (stringp val))))
   !    ;; This is OK, because indent-tabs-mode is defined using
   !    ;; DEFVAR_BOOL and hence can only be set to t or nil. 
   ! 	(indent-tabs-mode . t)
   ! 	(page-delimiter   . stringp)
   ! 	(paragraph-separate . stringp)
   ! 	(sentence-end     . stringp)
   ! 	(sentence-end-double-space . (lambda (val) (memq val '(t nil))))
   ! 	(tab-width        . integerp)
   ! 	(version-control  . (lambda (val) (memq val '(t nil never))))))

Re: risky local variable mechanism

[Luc Teirlinck]

Jonathan Yavner wrote:

   unsafep.el (used by SES) calls risky-local-variable-p with NIL as the 
   second argument, because it doesn't know yet what value will be 
   assigned.

If the current risky-local-variable-p is too unreliable for
`enable-local-eval' to such a degree that an urgent fix is needed, is
it really reliable enough for unsafep?

   Please maintain the feature that NIL as second argument to 
   risky-local-variable-p means "There exists at least one risky value 
   that could be assigned to this variable."

   I'm not sure if your patch does this.

At first sight, it does not appear that Chong's patch changes this.
But, of course, this means that (risky-local-variable-p myvar nil)
very well may return t, even though assigning a value of nil may be
perfectly safe (and probably is).  At first sight, it does not appear
that Chong's patch takes this into account.  I am not sure that
Chong's patch should use risky-local-variable-p at all.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

What does your patch do for the case where the user is root?  The user
may very well consider things safe when he is not running as root that
would be dangerous when running as root.  But he uses the same .emacs
when running as root.

At the very least safe-local-variables should be ignored when the user
is running as root.  Does your patch do that?  Maybe other extra
precautions should be taken when the user runs as root.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Richard M. Stallman]

What you've implemented is the sort of thing I had in mind.
It seems basically right.  Here are a few comments on minor details.

    ! 	(or (= char ?!)
    ! 	    (= char ? )

It would be better to use \s.

    + (defcustom safe-local-variables nil

I would call it safe-local-variable-values.

    +   "Alist of safe local variables.

It is not really an alist.  In an alist, the car of an element
is a key, so only the first element with a given car really counts.
This is a list of variable-value pairs.

    +   :type  'alist)

It is ok to use `alist' as the custom type if it gives the right
results in the custom buffer, as it seems to do.
But it might be cleaner to do this another way just to avoid
misleading someone.

By the way, this variable should be marked risky.

Re: risky local variable mechanism

[Chong Yidong]

Jonathan Yavner <jyavner@member.fsf.org> writes:

> (re: mechanism proposed by Chong Yidong)
>
> unsafep.el (used by SES) calls risky-local-variable-p with NIL as the 
> second argument, because it doesn't know yet what value will be 
> assigned.  Please maintain the feature that NIL as second argument to 
> risky-local-variable-p means "There exists at least one risky value 
> that could be assigned to this variable."

Currently, `risky-local-variable-p' returns t if

 (1) The variable's `risky-local-variable' property is non-nil, or
 (2) The variable's name ends with "hook(s)", "function(s)", etc.,
     unless `safe-local-variable' is non-nil, or
 (3) The variable's `safe-local-variable' property is a function that
     evaluates to nil when given VAR as an argument.

My patch removes condition (3).  The idea is that the
`safe-local-variable' property can make a variable safe; it doesn't
make it risky.  Since SES does not use the `safe-local-variable'
property, SES will be completely unaffected.

(In the first place, the `safe-local-variable' property isn't used
much -- pre-patch, the only place in the Emacs tree that sets it is
files.el, in which we set compile-command to be always safe.)

An orthogonal question is whether risky-local-variable-p has any use
if we implement a whitelist.  The current behavior, in the patch I
posted, is that when the user presses `!' to save a set of local
variables (so that Emacs will allow those values in the future), risky
variables won't be saved.  This seems like annoying behavior.  Why
should we forbid the user from saving if he explicitly says to save?
We can change this so that even risky variables will be saved.  But
this makes risky variables just like every other variable that is not
marked safe.  One solution is to to keep `risky-local-variable-p'
around for unsafep.el to use, but not actually use it in
files.el---and just rely on the whitelist mechanism.

Re: risky local variable mechanism

[Jonathan Yavner]

Chong Yidong wrote:
> One solution is to to keep `risky-local-variable-p'
> around for unsafep.el to use, but not actually use it in
> files.el---and just rely on the whitelist mechanism.

I don't think this is a good idea.  `risky-local-variable-p' was 
invented five years ago precisely so that files.el and unsafep.el would 
use the same rules for deciding whether variables were safe.  If 
files.el says a variable is unsafe but unsafep.el thinks it's safe, 
that is a security hole (a spreadhseet could be written that would 
tickle the unsafe variable).  If files.el says a variable is safe but 
unsafep says it isn't, that will annoy the user (who will be required 
to approve the use of that variable every time he loads the 
spreadsheet).

In general, lambdas are unsafe if unsafep says they are, so we could 
call it instead of arbitrarily declaring that all xxx-function 
variables are unsafe.  If a variable can be nil, a number, or a lambda, 
we can use unsafep instead of arbitrarily rejecting all lambda cases.


Luc Teirlinck wrote:
> If the current risky-local-variable-p is too unreliable for
> `enable-local-eval' to such a degree that an urgent fix is needed, is
> it really reliable enough for unsafep?

What exactly is the problem with the current system?  Are there unsafe 
variables erroneously declared safe?  Are there file-local variables 
with safe values erroneously declared unsafe?  What is the minimum 
change needed to solve the current problem?


> (risky-local-variable-p myvar nil) very well may return t, even though
> assigning a value of nil may be perfectly safe (and probably is).

Yeah, I don't like that, either.  If a variable has some safe and some 
unsafe values, it seems likely that nil would be one of the safe ones.  
Changing this calling convention would break backward compatibility, 
although it doesn't *seem* like the function is used outside of 
files.el and unsafep.el.


> I am not sure that Chong's patch should use risky-local-variable-p at
> all. 

Again, if files.el and unsafep.el do not use the same mechanism, Emacs 
will have two definitions for the same concept ("safe variable") which 
will lead to either security holes or user annoyance.

Re: risky local variable mechanism

[Chong Yidong]

> What exactly is the problem with the current system?  Are there unsafe 
> variables erroneously declared safe?  Are there file-local variables 
> with safe values erroneously declared unsafe?  What is the minimum 
> change needed to solve the current problem?

See Richard's previous emails on this subject.  The rationale is that
it is safer to add safe values to a whitelist than to add risky
variables to a blacklist---you wouldn't know if you missed a dangerous
variable.

> Again, if files.el and unsafep.el do not use the same mechanism, Emacs 
> will have two definitions for the same concept ("safe variable") which 
> will lead to either security holes or user annoyance.

It's unlikely that making the file local variable mechanism stricter,
while keeping unsafep.el the same, will open up new security holes
that didn't already exist.  Anyway, it is pretty easy to change
unsafep.el to reflect the changes to files.el.

Re: Risky local variable mechanism

[Stefan Monnier]

> After some further thought, I would be very reluctant to use
> (put 'myvar 'safe-local-variable t) for _any_ variable myvar.  There
> is no reason to give an unscrupulous person any chance to try to play
> games with perverse values.  So just always check that the value makes
> sense.  This also automatically prevents problems if one later allows
> dangerous values and forgets to update the 'safe-local-variables
> property.  Why not something like:

Now that you mention it, it seems obvious.  There's no need for a special
value t.  100% agreement.


        Stefan

Re: Risky local variable mechanism

[Stefan Monnier]

> What does your patch do for the case where the user is root?  The user
> may very well consider things safe when he is not running as root that
> would be dangerous when running as root.  But he uses the same .emacs
> when running as root.

I think if something is not safe enough for root, it should also be
considered unsafe for random users.  So I don't see any need to make
a distinction.


        Stefan

Re: risky local variable mechanism

[Luc Teirlinck]

Jonathan Yavner wrote:

   What exactly is the problem with the current system?

Richard only said that "it was not reliable enough".  There seems to
be a big sense of urgency and importance about this, however.

   What is the minimum change needed to solve the current problem?

I believe that it seems clear from previous postings that Richard is
not interested in a "minimum change" type solution.

   Are there file-local variables with safe values erroneously
   declared unsafe?

I do not believe that we are currently worrying about that.  We are
worrying about mistakenly considering dangerous things safe.

I _believe_ that Richard is mainly concerned with certain variables,
whose value can be a function that Emacs could later call, slipping
through the risky-local-variable-p mechanism.

The new mechanism, which requires an explicit 'safe-local-variable
property for a variable to be considered safe by default, might also
not only prevent this easy type of abuse, but might also prevent
potential more sophisticated types of attack.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

Stefan Monnier wrote:

   I think if something is not safe enough for root, it should also be
   considered unsafe for random users.  So I don't see any need to make
   a distinction.

OK, so maybe not.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Richard M. Stallman]

      This also automatically prevents problems if one later allows
    dangerous values and forgets to update the 'safe-local-variables
    property.  Why not something like:

       ! ;; Commonly-encountered local variables that are safe:
       ! (mapc (lambda (pair)
       ! 	(put (car pair) 'safe-local-variable (cdr pair)))
       !       '((compile-command  . stringp)
       ! 	(fill-column      . integerp)
       ! 	(fill-prefix      . (lambda (val) (or (not val) (stringp val))))
       !    ;; This is OK, because indent-tabs-mode is defined using
       !    ;; DEFVAR_BOOL and hence can only be set to t or nil. 
       ! 	(indent-tabs-mode . t)
       ! 	(page-delimiter   . stringp)
       ! 	(paragraph-separate . stringp)
       ! 	(sentence-end     . stringp)
       ! 	(sentence-end-double-space . (lambda (val) (memq val '(t nil))))
       ! 	(tab-width        . integerp)
       ! 	(version-control  . (lambda (val) (memq val '(t nil never))))))

That is a good idea.

Re: risky local variable mechanism

[Richard M. Stallman]

    It's unlikely that making the file local variable mechanism stricter,
    while keeping unsafep.el the same, will open up new security holes
    that didn't already exist.  Anyway, it is pretty easy to change
    unsafep.el to reflect the changes to files.el.

Could you take a look at doing that?  It may not be quite trivial.
In particular, the fact that unsafep checks for functions
could make some things safe, which otherwise would not be.

Re: risky local variable mechanism

[Richard M. Stallman]

    I _believe_ that Richard is mainly concerned with certain variables,
    whose value can be a function that Emacs could later call, slipping
    through the risky-local-variable-p mechanism.

Exactly.

Re: risky local variable mechanism

[Chong Yidong]

"Richard M. Stallman" <rms@gnu.org> writes:

>     It's unlikely that making the file local variable mechanism stricter,
>     while keeping unsafep.el the same, will open up new security holes
>     that didn't already exist.  Anyway, it is pretty easy to change
>     unsafep.el to reflect the changes to files.el.
>
> Could you take a look at doing that?  It may not be quite trivial.
> In particular, the fact that unsafep checks for functions
> could make some things safe, which otherwise would not be.

The relevant part is the function `unsafep-variable', which can be
adapted easily.  The other things that unsafep.el checks are not
related to file variables.

*** emacs/lisp/emacs-lisp/unsafep.el.~1.10.~	2006-02-06 23:43:22.000000000 -0500
--- emacs/lisp/emacs-lisp/unsafep.el	2006-02-11 12:16:17.000000000 -0500
***************
*** 255,261 ****
    (cond
     ((not (symbolp sym))
      `(variable ,sym))
!    ((risky-local-variable-p sym nil)
      `(risky-local-variable ,sym))
     ((not (or global-okay
  	     (memq sym unsafep-vars)
--- 255,262 ----
    (cond
     ((not (symbolp sym))
      `(variable ,sym))
!    ((and (risky-local-variable-p sym)
! 	 (not (safe-local-variable-p sym val)))
      `(risky-local-variable ,sym))
     ((not (or global-okay
  	     (memq sym unsafep-vars)

Re: risky local variable mechanism

[Richard M. Stallman]

       unsafep.el (used by SES) calls risky-local-variable-p with NIL as the 
       second argument, because it doesn't know yet what value will be 
       assigned.

    If the current risky-local-variable-p is too unreliable for
    `enable-local-eval' to such a degree that an urgent fix is needed, is
    it really reliable enough for unsafep?

Maybe it isn't.  However, it is not necessary for unsafep to be completely
as strict as the local variable criteria in all ways, because unsafep
checks for function values.

Meanwhile, as regards the special meaning of nil as the second
arg to risky-local-variable-p, we could instead have a variable
risky-local-variable-token, whose value is a particular cons cell,
and give that cons cell the special meaning of "unspecified value".

The value that hack-one-local-variable produces is made by `read'
so it will never be eq to risky-local-variable-token.

Re: risky local variable mechanism

[Richard M. Stallman]

    The relevant part is the function `unsafep-variable', which can be
    adapted easily.  The other things that unsafep.el checks are not
    related to file variables.

That seems wrong, because it refers to `val' which does not exist in
the function unsafep-variable.  However, beyond that, it is not
clearly right for unsafep to reject binding all variables that would
be rejected in a local variables list, given that unsafep can detect
giving functions as values.

Re: risky local variable mechanism

[Chong Yidong]

> it is not clearly right for unsafep to reject binding all variables
> that would be rejected in a local variables list, given that unsafep
> can detect giving functions as values.

The commentary section of unsafep.el says:

  ;; A temporary binding is unsafe if its symbol:
  ;;  1.  Has the `risky-local-variable' property.
  ;;  2.  Has a name that ends with -command, font-lock-keywords(-[0-9]+)?,
  ;;      font-lock-syntactic-keywords, -form, -forms, -frame-alist, -function,
  ;;       -functions, -history, -hook, -hooks, -map, -map-alist, -mode-alist,
  ;;       -predicate, or -program.

This is exactly the definition of risky variables in my patch.  In
fact, the *current* definition of risky-local-variable-p does not
match this definition exactly, since it also checks
`safe-local-property'.  (The way it checks it is rather inconsistent,
BTW, there is no way to distinguish between VAL=nil meaning "any value
of the variable" and VAL=nil meaning "the value nil is supplied" when
`safe-local-property' is a function.  So if unsafep.el relied on this,
it was relying on a bug).

Maybe the commentary section of unsafep.el is wrong, and the latter
definition is intended.  But according to the docstring of
`unsafep-variable', it is supposed to detect if a variable is "safe as
a let-binding" -- no mention of file local variables there.

To clarify this issue, I'd like to ask: what's the specific goal of
unsafep.el?  Can anyone come up with a precise example of a problem
with using unsafep.el resulting from the proposed changes to the file
local variables code?  With a specific example, we can look into how
to adapt unsafep.el; otherwise, we're just going around in circles.

(Since SES seems to be the only package that uses unsafep.el, it looks
like the example will have to come from there.)

Re: Risky local variable mechanism

[Chong Yidong]

I've installed the patch changing the way file local variables are
handled.  Please see if there are any problems.

If you encounter any file variables that should be marked safe, so
that Emacs in its default configuration doesn't bother you before
setting it, go ahead and add an appropriate `safe-local-variable'
property.  For major mode-specific variables, this can take place in
the mode's file, because the "mode:" variable is acted on before the
other file variables are read.

One behavior I haven't mentioned is that when a set of file variables
requires confirmation (because the values are not known to be safe, or
the variables are risky), but Emacs is non-interactive, those
variables are just ignored.  I don't think this breaks anything, but
it may be good to keep an eye out.

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   I've installed the patch changing the way file local variables are
   handled.  Please see if there are any problems.

I have not yet taken any real look at it, but I believe that @var{(var . val)}
is bad Texinfo.  It believe that it should be @code{(@var{var} . @var{val}}:

===File ~/variables.texi-diff-2=============================
*** variables.texi	13 Feb 2006 19:55:31 -0600	1.72
--- variables.texi	13 Feb 2006 20:00:18 -0600	
***************
*** 1794,1801 ****
  
  @defopt safe-local-variable-values
  This variable provides another way to mark variables as safe.  It is a
! list of cons cells @var{(var . val)}, where @var{var} is a variable
! name and @var{val} is a value of that variable that is safe.
  
  When Emacs asks the user whether or not to obey a set of file variable
  specifications, the user can choose to mark them as safe.  This adds
--- 1794,1801 ----
  
  @defopt safe-local-variable-values
  This variable provides another way to mark variables as safe.  It is a
! list of cons cells @code{(@var{var} . @var{val})}, where @var{var} is
! a variable name and @var{val} is a value of that variable that is safe.
  
  When Emacs asks the user whether or not to obey a set of file variable
  specifications, the user can choose to mark them as safe.  This adds
============================================================

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   I've installed the patch changing the way file local variables are
   handled.  Please see if there are any problems.

There are problems.  I get this pop-up question for _every_ single
file I visit, even files that contain no local variables whatsoever,
and that do not even contain the words "local variables".  This makes
Emacs nearly unusable.

Also, before your patch, with `enable-local-variables' set to t, I
could always take a look at the _entire_ local variables list, even for
variables that Emacs deems safe, before enabling those variables.
After your patch, this is apparently no longer an option.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

>From my earlier message:

   There are problems.  I get this pop-up question for _every_ single
   file I visit, even files that contain no local variables whatsoever,
   and that do not even contain the words "local variables".  This makes
   Emacs nearly unusable.

I forgot to test this in `emacs -q'.  What you have to do in `emacs -q'
to see the problem is set `enable-local-variables' to 'query.

What should happen is that I should be asked no questions if there are
no local variables and that I should get to see _all_ local variables
if there are any before enabling them, no matter how much they are
considered safe by default.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Luc Teirlinck]

>From my previous message:

   I forgot to test this in `emacs -q'.  What you have to do in `emacs -q'
   to see the problem is set `enable-local-variables' to 'query.

   What should happen is that I should be asked no questions if there are
   no local variables and that I should get to see _all_ local variables
   if there are any before enabling them, no matter how much they are
   considered safe by default.

To avoid confusion:  with "What should happen", I of course mean
"What should happen, given the fact that I have
`enable-local-variables' set to 'query."

Sincerely,

Luc.

Re: Risky local variable mechanism

[Chong Yidong]

> There are problems.  I get this pop-up question for _every_ single
> file I visit, even files that contain no local variables whatsoever,
> and that do not even contain the words "local variables".

Fixed, thanks.

> Also, before your patch, with `enable-local-variables' set to t, I
> could always take a look at the _entire_ local variables list, even for
> variables that Emacs deems safe, before enabling those variables.
> After your patch, this is apparently no longer an option.

No idea what you're referring --- are you referring to ignored
variables?

Re: Risky local variable mechanism

[Luc Teirlinck]

Chong Yidong wrote:

   > Also, before your patch, with `enable-local-variables' set to t, I
   > could always take a look at the _entire_ local variables list, even for
   > variables that Emacs deems safe, before enabling those variables.
   > After your patch, this is apparently no longer an option.

   No idea what you're referring --- are you referring to ignored
   variables?

Now I see that in certain situations it does ask me.  Did you make
`coding' the only exception?  When visiting lisp/Changelog, I get
asked no questions (with `enable-local-variables' set to 'query), even
though lisp/Changelog has a local variables list.

Sincerely,

Luc.

Re: Risky local variable mechanism

[Chong Yidong]

> Now I see that in certain situations it does ask me.  Did you make
> `coding' the only exception?

`coding' has always been handled separately, by `set-auto-coding'.
(`mode' is also handled specially.)

Re: Risky local variable mechanism

[Richard M. Stallman]

    I have not yet taken any real look at it, but I believe that @var{(var . val)}
    is bad Texinfo.  It believe that it should be @code{(@var{var} . @var{val}}:

I fixed that.  Thanks.