GNU bug report logs - #61901
30.0.50; [PATCH] Add permanently-enabled-local-variable-dirs variable.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61901 in the body.
You can then email your comments to 61901 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Add permanently-enabled-local-variable-dirs variable.
Date: Wed, 01 Mar 2023 22:20:33 +0000

[Message part 1 (text/plain, inline)]

This patch allows users to trust directories to load dir-local variables
from, so they don't have to do something lile this:
(defun risky-local-variable-p (sym &optional _ignored) nil)
as suggested here:
https://emacs.stackexchange.com/questions/10983/remember-permission-to-execute-risky-local-variables

It also works over TRAMP if enable-remote-dir-locals is true.

[0001-Add-permanently-enabled-local-variable-dirs-variable.patch (text/x-patch, attachment)]

Message #8 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50;
 [PATCH] Add permanently-enabled-local-variable-dirs variable.
Date: Thu, 02 Mar 2023 08:57:21 +0200

> Date: Wed, 01 Mar 2023 22:20:33 +0000
> From:  Antero Mejr via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> This patch allows users to trust directories to load dir-local variables
> from, so they don't have to do something lile this:
> (defun risky-local-variable-p (sym &optional _ignored) nil)
> as suggested here:
> https://emacs.stackexchange.com/questions/10983/remember-permission-to-execute-risky-local-variables
> 
> It also works over TRAMP if enable-remote-dir-locals is true.

Thanks, IMO this is a very useful feature.

> --- a/doc/lispref/variables.texi
> +++ b/doc/lispref/variables.texi
> @@ -1974,6 +1974,12 @@ File Local Variables
>  symbols.
>  @end defvar
>  
> +@defvar permanently-enabled-local-variable-dirs
> +This is a list of trusted directories that contain local variables.
> +Local variables in these directories will always be enabled, regardless
> +of whether they are risky.
> +@end defvar

This should explicitly allude to the '.dir-locals.el' files in those
directories, since otherwise talking about "directories that contain
variables" could be confusing.

I also suggest to rename the variable to something like
'permanently-safe-local-variable-directories', or maybe just
'safe-local-variable-directories' which IMO should express the purpose
better.

> -Also see the `permanently-enabled-local-variables' variable."
> +Also see the `permanently-enabled-local-variables' and
> +'permanently-enabled-local-variable-dirs' variables."
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We quote `like this' in doc strings, to produce links in the *Help*
buffers.

> +(defcustom permanently-enabled-local-variable-dirs '()
> +  "A list of directories that contain local variables that are always
> +enabled, regardless of whether they are risky."

The first line of a doc string should be a single complete sentence.
(This is because the various apropos commands show only the first line
of the doc string.)

> @@ -3730,7 +3739,9 @@ hack-local-variables-confirm
>  !  -- to apply the local variables list, and permanently mark these
>        values (*) as safe (in the future, they will be set automatically.)
>  i  -- to ignore the local variables list, and permanently mark these
> -      values (*) as ignored\n\n")
> +      values (*) as ignored
> ++  -- to apply the local variables list, and permanently trust "
> +                    name "\n\n")

"permanently trust name" sounds confusing (what is "name"?).  How
about this variant:

  +  -- to apply the local variables list, and permanently trust
        all directory-local variables in this directory

> @@ -3762,8 +3773,13 @@ hack-local-variables-confirm
>  	       char)
>  	  (when offer-save
>              (push ?i exit-chars)
> -            (push ?! exit-chars))
> +            (push ?! exit-chars)
> +            (push ?+ exit-chars))
>  	  (setq char (read-char-choice prompt exit-chars))
> +          (when (and offer-save (= char ?+))
> +            (customize-push-and-save
> +             'permanently-enabled-local-variable-dirs
> +             (list dir-name)))

Bother: AFAIU here we modify the user's custom file without asking for
an explicit permission.  Should we ask for permission?

Last, but not least: this change is larger than what we can accept
without you assigning to FSF the copyright for your changes, and I
don't see any copyright assignment in your name on file.  Would you be
willing to do the legal paperwork for such an assignment?  If yes, I
will send you the form to start the paperwork rolling; when it is
completed, we can install your changes.

Thanks.

Message #11 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Antero Mejr <antero <at> mailbox.org>, 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH] Add
 permanently-enabled-local-variable-dirs variable.
Date: Thu, 02 Mar 2023 17:09:51 +0000

[v2-0001-Add-safe-local-variable-directories-variable.patch (text/x-patch, attachment)]

[Message part 2 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:
> This should explicitly allude to the '.dir-locals.el' files in those
> directories, since otherwise talking about "directories that contain
> variables" could be confusing.

Fixed in v2.

> I also suggest to rename the variable to something like
> 'permanently-safe-local-variable-directories', or maybe just
> 'safe-local-variable-directories' which IMO should express the purpose
> better.

I like 'safe-local-variable-directories', updated to use that.

> We quote `like this' in doc strings, to produce links in the *Help*
> buffers.

Fixed.

> The first line of a doc string should be a single complete sentence.
> (This is because the various apropos commands show only the first line
> of the doc string.)

Fixed.

> "permanently trust name" sounds confusing (what is "name"?).  How
> about this variant:
>
>   +  -- to apply the local variables list, and permanently trust
>         all directory-local variables in this directory

"name" is a variable that gets expanded to the directory name, but it's
redundant since it's already listed at the top. Updated to use your variant.

> Bother: AFAIU here we modify the user's custom file without asking for
> an explicit permission.  Should we ask for permission?

IMO they give sufficient permission when the use the "+" option.

> Last, but not least: this change is larger than what we can accept
> without you assigning to FSF the copyright for your changes, and I
> don't see any copyright assignment in your name on file.  Would you be
> willing to do the legal paperwork for such an assignment?  If yes, I
> will send you the form to start the paperwork rolling; when it is
> completed, we can install your changes.

I sent the request-assign.future doc to the FSF assignment email earlier
today, feel free to send me paperwork and I will fill it out.

Thank you for the review.

Message #14 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: antero <at> mailbox.org, 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH] Add
 permanently-enabled-local-variable-dirs variable.
Date: Thu, 02 Mar 2023 20:04:12 +0200

> From: Antero Mejr <antero <at> mailbox.org>
> Cc: Antero Mejr <antero <at> mailbox.org>, 61901 <at> debbugs.gnu.org
> Date: Thu, 02 Mar 2023 17:09:51 +0000
> 
> I sent the request-assign.future doc to the FSF assignment email earlier
> today, feel free to send me paperwork and I will fill it out.

That should be enough.  If they don't respond within two weeks, ping
them and CC me.

Thanks.

Message #17 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH] Add
 permanently-enabled-local-variable-dirs variable.
Date: Tue, 14 Mar 2023 18:46:24 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:
>> I sent the request-assign.future doc to the FSF assignment email earlier
>> today, feel free to send me paperwork and I will fill it out.
>
> That should be enough.  If they don't respond within two weeks, ping
> them and CC me.

I signed and returned the full copyright assignment paperwork last week,
so I assume that my copyright assignment is on file now.

Let me know if this patch needs a v3 or if there is anything else before it
can be applied, thanks.

Message #20 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH] Add
 permanently-enabled-local-variable-dirs variable.
Date: Tue, 14 Mar 2023 21:48:00 +0200

> From: Antero Mejr <antero <at> mailbox.org>
> Cc: 61901 <at> debbugs.gnu.org
> Date: Tue, 14 Mar 2023 18:46:24 +0000
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> >> I sent the request-assign.future doc to the FSF assignment email earlier
> >> today, feel free to send me paperwork and I will fill it out.
> >
> > That should be enough.  If they don't respond within two weeks, ping
> > them and CC me.
> 
> I signed and returned the full copyright assignment paperwork last week,
> so I assume that my copyright assignment is on file now.

It isn't, not yet.  You should receive another email with the
assignment counter-signed by the FSF, and then it will be added to the
DB.

Message #23 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: 61901 <at> debbugs.gnu.org
Subject: bug#61901: 30.0.50; [PATCH v3] Add safe-local-variable-directories
 variable.
Date: Tue, 25 Apr 2023 16:40:07 +0000

[Message part 1 (text/plain, inline)]

Updated safe-local-variable-directories patch onto master and added bug
number to commit message.

Also should I use git --reroll-count to make v2 patches, v3, etc?  If so
then I included another patch to gitignore rerolled patches, otherwise
please disregard it.

[v3-0001-Add-safe-local-variable-directories-variable.patch (text/x-patch, attachment)]

[0001-Ignore-rerolled-patches.patch (text/x-patch, attachment)]

Message #26 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50;
 [PATCH v3] Add safe-local-variable-directories variable.
Date: Tue, 25 Apr 2023 20:23:53 +0300

> Date: Tue, 25 Apr 2023 16:40:07 +0000
> From:  Antero Mejr via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Updated safe-local-variable-directories patch onto master and added bug
> number to commit message.

Thanks, see some comments below.

> Also should I use git --reroll-count to make v2 patches, v3, etc?

You don't have to.  The version part is removed by "git am" anyway,
and it is not important for patch review here.

> --- a/doc/lispref/variables.texi
> +++ b/doc/lispref/variables.texi
> @@ -1977,6 +1977,13 @@ this can be controlled by using this variable, which is a list of
>  symbols.
>  @end defvar
>  
> +@defvar safe-local-variable-directories
> +This is a list of directories where local variables are always enabled.
> +Directory-local variables loaded from these directories, such as the
> +variables in @file{.dir-locals.el}, will be enabled even if they are
> +risky.
> +@end defvar

This variable should also be documented in the Emacs user manual, not
only in the ELisp Reference manual -- it's a user option, and a very
important one at that.

> ++++
> +** New variable 'safe-local-variable-directories'.
> +This variable is used to to permanently trust directories containing
> +risky directory-local variables.

I would rephrase:

  This variable names directories in which Emacs will treat all
  directory-local variables as safe.

>  ALL-VARS is the list of all variables to be set up.
> @@ -3734,7 +3744,9 @@ n  -- to ignore the local variables list.")
>  !  -- to apply the local variables list, and permanently mark these
>        values (*) as safe (in the future, they will be set automatically.)
>  i  -- to ignore the local variables list, and permanently mark these
> -      values (*) as ignored\n\n")
> +      values (*) as ignored
> ++  -- to apply the local variables list, and permanently trust all
> +      directory-local variables in this directory\n\n")

I would remove the "permanently" part, it would just confuse here.

> @@ -3908,6 +3924,7 @@ DIR-NAME is the name of the associated directory.  Otherwise it is nil."
>  		  (null unsafe-vars)
>  		  (null risky-vars))
>  	     (memq enable-local-variables '(:all :safe))
> +             (member dir-name safe-local-variable-directories)

If you use 'member' for this test, then (a) the documentation of
safe-local-variable-directories should explicitly say that the
directories in the list must be in full absolute form, and (b) we
should consider the various issues with file names that are not
'equal' as strings, but still name the same directory, such as
letter-case differences on case-insensitive filesystems.  And what
about equality of "foo/" "and "foo"?

Also, is 'dir-name' above guaranteed to be a fully-expanded absolute
file name?

> +(ert-deftest files-tests-safe-local-variable-directories ()
> +  ;; safe-local-variable-directories should be risky,
> +  ;; so use it as an arbitrary risky variable.
> +  (let ((test-alist '((safe-local-variable-directories . "some_val")))
> +        (fakedir "test1/test2")
> +        (enable-local-eval t))
> +    (with-temp-buffer
> +      (setq safe-local-variable-directories (list fakedir))

The test should use absolute directory names for directories you put
into safe-local-variable-directories.

Message #29 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: eliz <at> gnu.org
Cc: 61901 <at> debbugs.gnu.org
Subject: bug#61901: 30.0.50; [PATCH v3] Add safe-local-variable-directories
 variable.
Date: Tue, 09 May 2023 21:29:54 +0000

[Message part 1 (text/plain, inline)]

>> +@defvar safe-local-variable-directories
>> +This is a list of directories where local variables are always enabled.
>> +Directory-local variables loaded from these directories, such as the
>> +variables in @file{.dir-locals.el}, will be enabled even if they are
>> +risky.
>> +@end defvar
>
>This variable should also be documented in the Emacs user manual, not
>only in the ELisp Reference manual -- it's a user option, and a very
>important one at that.

Added to the manual in custom.texi "Safe File Variables" subsection.

>> ++++
>> +** New variable 'safe-local-variable-directories'.
>> +This variable is used to to permanently trust directories containing
>> +risky directory-local variables.
>
>I would rephrase:
>
>  This variable names directories in which Emacs will treat all
>  directory-local variables as safe.

Fixed in attached patch.

>>  ALL-VARS is the list of all variables to be set up.
>> @@ -3734,7 +3744,9 @@ n  -- to ignore the local variables list.")
>>  !  -- to apply the local variables list, and permanently mark these
>>        values (*) as safe (in the future, they will be set automatically.)
>>  i  -- to ignore the local variables list, and permanently mark these
>> -      values (*) as ignored\n\n")
>> +      values (*) as ignored
>> ++  -- to apply the local variables list, and permanently trust all
>> +      directory-local variables in this directory\n\n")
>
>I would remove the "permanently" part, it would just confuse here.

Fixed.

>> @@ -3908,6 +3924,7 @@ DIR-NAME is the name of the associated directory.  
>> Otherwise it is nil."
>>                 (null unsafe-vars)
>>                 (null risky-vars))
>>            (memq enable-local-variables '(:all :safe))
>> +             (member dir-name safe-local-variable-directories)
>
>If you use 'member' for this test, then (a) the documentation of
>safe-local-variable-directories should explicitly say that the
>directories in the list must be in full absolute form, and (b) we
>should consider the various issues with file names that are not
>'equal' as strings, but still name the same directory, such as
>letter-case differences on case-insensitive filesystems.  And what
>about equality of "foo/" "and "foo"?

Clarified the documentation. The directory path requires a trailing
separator, and is case-sensitive regardless of the filesystem (tested
on vFAT).

>Also, is 'dir-name' above guaranteed to be a fully-expanded absolute
>file name?

Yes. For TRAMP connections it's a string with text properties, but it's
the same equality-wise.

#("/ssh:user:/home/user/src/" 5 6 (tramp-default t))

>> +(ert-deftest files-tests-safe-local-variable-directories ()
>> +  ;; safe-local-variable-directories should be risky,
>> +  ;; so use it as an arbitrary risky variable.
>> +  (let ((test-alist '((safe-local-variable-directories . "some_val")))
>> +        (fakedir "test1/test2")
>> +        (enable-local-eval t))
>> +    (with-temp-buffer
>> +      (setq safe-local-variable-directories (list fakedir))
>
>The test should use absolute directory names for directories you put
>into safe-local-variable-directories.

Fixed.

[0001-Add-safe-local-variable-directories-variable.patch (text/x-patch, attachment)]

Message #34 received at 61901-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901-done <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add safe-local-variable-directories
 variable.
Date: Thu, 11 May 2023 16:55:54 +0300

> From: Antero Mejr <antero <at> mailbox.org>
> Cc: 61901 <at> debbugs.gnu.org
> Date: Tue, 09 May 2023 21:29:54 +0000
> 
> >> +@defvar safe-local-variable-directories
> >> +This is a list of directories where local variables are always enabled.
> >> +Directory-local variables loaded from these directories, such as the
> >> +variables in @file{.dir-locals.el}, will be enabled even if they are
> >> +risky.
> >> +@end defvar
> >
> >This variable should also be documented in the Emacs user manual, not
> >only in the ELisp Reference manual -- it's a user option, and a very
> >important one at that.
> 
> Added to the manual in custom.texi "Safe File Variables" subsection.
> 
> >> ++++
> >> +** New variable 'safe-local-variable-directories'.
> >> +This variable is used to to permanently trust directories containing
> >> +risky directory-local variables.
> >
> >I would rephrase:
> >
> >  This variable names directories in which Emacs will treat all
> >  directory-local variables as safe.
> 
> Fixed in attached patch.
> 
> >>  ALL-VARS is the list of all variables to be set up.
> >> @@ -3734,7 +3744,9 @@ n  -- to ignore the local variables list.")
> >>  !  -- to apply the local variables list, and permanently mark these
> >>        values (*) as safe (in the future, they will be set automatically.)
> >>  i  -- to ignore the local variables list, and permanently mark these
> >> -      values (*) as ignored\n\n")
> >> +      values (*) as ignored
> >> ++  -- to apply the local variables list, and permanently trust all
> >> +      directory-local variables in this directory\n\n")
> >
> >I would remove the "permanently" part, it would just confuse here.
> 
> Fixed.
> 
> >> @@ -3908,6 +3924,7 @@ DIR-NAME is the name of the associated directory.  
> >> Otherwise it is nil."
> >>                 (null unsafe-vars)
> >>                 (null risky-vars))
> >>            (memq enable-local-variables '(:all :safe))
> >> +             (member dir-name safe-local-variable-directories)
> >
> >If you use 'member' for this test, then (a) the documentation of
> >safe-local-variable-directories should explicitly say that the
> >directories in the list must be in full absolute form, and (b) we
> >should consider the various issues with file names that are not
> >'equal' as strings, but still name the same directory, such as
> >letter-case differences on case-insensitive filesystems.  And what
> >about equality of "foo/" "and "foo"?
> 
> Clarified the documentation. The directory path requires a trailing
> separator, and is case-sensitive regardless of the filesystem (tested
> on vFAT).
> 
> >Also, is 'dir-name' above guaranteed to be a fully-expanded absolute
> >file name?
> 
> Yes. For TRAMP connections it's a string with text properties, but it's
> the same equality-wise.
> 
> #("/ssh:user:/home/user/src/" 5 6 (tramp-default t))
> 
> >> +(ert-deftest files-tests-safe-local-variable-directories ()
> >> +  ;; safe-local-variable-directories should be risky,
> >> +  ;; so use it as an arbitrary risky variable.
> >> +  (let ((test-alist '((safe-local-variable-directories . "some_val")))
> >> +        (fakedir "test1/test2")
> >> +        (enable-local-eval t))
> >> +    (with-temp-buffer
> >> +      (setq safe-local-variable-directories (list fakedir))
> >
> >The test should use absolute directory names for directories you put
> >into safe-local-variable-directories.
> 
> Fixed.

Thanks, I installed this on the master branch, and I'm therefore
closing this bug.

Please note some minor changes I made in the documentation parts of
the changeset, the most notable one being the use of "path" to allude
to file names or directory names: the Gnu Coding Standards frown on
that.  We use "path" only for lists of directories in the style of
PATH environment variable or load-path Lisp variable.

Thanks again for working on this feature.

Message #37 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Thu, 11 May 2023 19:10:54 +0300

> From: Antero Mejr <antero <at> mailbox.org>
> Date: Thu, 11 May 2023 15:42:38 +0000
> 
> Thanks. I looked it over and found a typo. Patch is attached.

Thanks, I removed the redundant text (not exactly the one you proposed
to remove).  It was your original text, which I replaced with modified
one, left there by mistake.

> diff --git a/doc/lispref/variables.texi b/doc/lispref/variables.texi
> index b3a8cd8110c..28deddf985d 100644
> --- a/doc/lispref/variables.texi
> +++ b/doc/lispref/variables.texi
> @@ -1986,7 +1986,7 @@ fully-expanded absolute file names that end in a directory separator
>  character.  They may also be remote directories if the variable
>  @code{enable-remote-dir-locals} is set non-@code{nil}.  Directories in
>  this list are matched case-sensitively, even if the filesystem is
> -case-sensitive.
> +case-insensitive.
>  @end defvar

This actually means that I misunderstood the code.  Now that I see the
truth, why is it a good idea to compare directories case-sensitively
when the filesystem is not?  That's not something users will expect.

(And why private email?  Please keep the bug address on the CC list.)

Message #40 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Thu, 11 May 2023 17:49:50 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> diff --git a/doc/lispref/variables.texi b/doc/lispref/variables.texi
>> index b3a8cd8110c..28deddf985d 100644
>> --- a/doc/lispref/variables.texi
>> +++ b/doc/lispref/variables.texi
>> @@ -1986,7 +1986,7 @@ fully-expanded absolute file names that end in a
>> directory separator
>>  character.  They may also be remote directories if the variable
>>  @code{enable-remote-dir-locals} is set non-@code{nil}.  Directories in
>>  this list are matched case-sensitively, even if the filesystem is
>> -case-sensitive.
>> +case-insensitive.
>>  @end defvar
>
> This actually means that I misunderstood the code.  Now that I see the
> truth, why is it a good idea to compare directories case-sensitively
> when the filesystem is not?  That's not something users will expect.

What if a directory's case sensitivity changes so that it previously did
not match, but now does? This could happen with Windows per-directory
case sensitivity modifications, mounted disks, or remote paths.

To accurately assess if a directory name matches with possible
case-sensitivity, the process would be:
1. check the case-sensitivity of the filesystem
2. If case insensitive, check the case-sensitivity of each subdirectory
(using Windows queryCaseSensitiveInfo if applicable)
3. map over the components of the directory name, checking each subdirectory
with the correct case-sensitivity setting

That logic would be difficult for users to reason about, so for features
with security considerations like this I think it's better to err on the
side of safety and simplicity even if the behavior is stricter than
expected.

Message #43 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Thu, 11 May 2023 21:11:52 +0300

> From: Antero Mejr <antero <at> mailbox.org>
> Cc: 61901 <at> debbugs.gnu.org
> Date: Thu, 11 May 2023 17:49:50 +0000
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > This actually means that I misunderstood the code.  Now that I see the
> > truth, why is it a good idea to compare directories case-sensitively
> > when the filesystem is not?  That's not something users will expect.
> 
> What if a directory's case sensitivity changes so that it previously did
> not match, but now does? This could happen with Windows per-directory
> case sensitivity modifications, mounted disks, or remote paths.
> 
> To accurately assess if a directory name matches with possible
> case-sensitivity, the process would be:
> 1. check the case-sensitivity of the filesystem
> 2. If case insensitive, check the case-sensitivity of each subdirectory
> (using Windows queryCaseSensitiveInfo if applicable)
> 3. map over the components of the directory name, checking each subdirectory
> with the correct case-sensitivity setting
> 
> That logic would be difficult for users to reason about, so for features
> with security considerations like this I think it's better to err on the
> side of safety and simplicity even if the behavior is stricter than
> expected.

We already have all that in file-equal-p.  We should just use is
there.

Message #46 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Thu, 11 May 2023 20:11:16 +0000

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> > This actually means that I misunderstood the code.  Now that I see the
>> > truth, why is it a good idea to compare directories case-sensitively
>> > when the filesystem is not?  That's not something users will expect.
>> 
>> To accurately assess if a directory name matches with possible
>> case-sensitivity, the process would be:
>> 1. check the case-sensitivity of the filesystem
>> 2. If case insensitive, check the case-sensitivity of each subdirectory
>> (using Windows queryCaseSensitiveInfo if applicable)
>> 3. map over the components of the directory name, checking each subdirectory
>> with the correct case-sensitivity setting
>
> We already have all that in file-equal-p.  We should just use is
> there.

Ok, patch is attached (tested on FAT32 disk).

[0001-Handle-case-insensitivity-for-safe-local-variable-di.patch (text/x-patch, attachment)]

Message #49 received at 61901 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61901 <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Thu, 11 May 2023 21:38:33 +0000

[Message part 1 (text/plain, inline)]

Antero Mejr <antero <at> mailbox.org> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> > This actually means that I misunderstood the code.  Now that I see the
>>> > truth, why is it a good idea to compare directories case-sensitively
>>> > when the filesystem is not?  That's not something users will expect.
>>> 
>>> To accurately assess if a directory name matches with possible
>>> case-sensitivity, the process would be:
>>> 1. check the case-sensitivity of the filesystem
>>> 2. If case insensitive, check the case-sensitivity of each subdirectory
>>> (using Windows queryCaseSensitiveInfo if applicable)
>>> 3. map over the components of the directory name, checking each subdirectory
>>> with the correct case-sensitivity setting
>>
>> We already have all that in file-equal-p.  We should just use is
>> there.
>
> Ok, patch is attached (tested on FAT32 disk).

Please ignore that last patch, here is a corrected version with updated
docs. Since file-equal-p handles the trailing slash, now it doesn't
matter if there is/isn't one.

[0001-Handle-case-insensitivity-for-safe-local-variable-di.patch (text/x-patch, attachment)]

Message #52 received at 61901-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Antero Mejr <antero <at> mailbox.org>
Cc: 61901-done <at> debbugs.gnu.org
Subject: Re: bug#61901: 30.0.50; [PATCH v3] Add
 safe-local-variable-directories variable.
Date: Fri, 12 May 2023 14:09:46 +0300

> From: Antero Mejr <antero <at> mailbox.org>
> Cc: 61901 <at> debbugs.gnu.org
> Date: Thu, 11 May 2023 21:38:33 +0000
> 
> Please ignore that last patch, here is a corrected version with updated
> docs. Since file-equal-p handles the trailing slash, now it doesn't
> matter if there is/isn't one.

Thanks, installed.

This change broke files-tests, so I installed a fix there.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.