[PATCH] Make ispell localwords safe local variable

[PATCH] Make ispell localwords safe local variable

[Joseph Turner]

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

Hello,

In this PR <https://github.com/alphapapa/makem.sh/pull/39>, we made it
possible for checkdoc/ispell to ignore localwords stored in a
.dir-locals.el when run in batch mode.

It seemed appropriate to upstream this fix.

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-ispell-localwords-safe-local-variable.patch --]
[-- Type: text/x-diff, Size: 1015 bytes --]

From 3af6882c316de7a551a0ce25cd7b6c35b93cd90b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Mon, 14 Aug 2023 22:35:28 -0700
Subject: [PATCH] Make ispell localwords safe local variable

* lisp/textmodes/ispell.el (ispell-buffer-session-localwords):
Make safe local variable to so that checkdoc can ignore words in a
project in batch mode.
---
 lisp/textmodes/ispell.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 97c4ce9f32d..c73f92aa0b3 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1747,6 +1747,7 @@ Ispell is then restarted because the local words could conflict.")
 
 (defvar-local ispell-buffer-session-localwords nil
   "List of words accepted for session in this buffer.")
+(put 'ispell-buffer-session-localwords 'safe-local-variable #'list-of-strings-p)
 
 (defvar ispell-parser 'use-mode-name
   "Indicates whether ispell should parse the current buffer as TeX Code.
-- 
2.41.0

Re: [PATCH] Make ispell localwords safe local variable

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Mon, 14 Aug 2023 22:40:24 -0700
> 
> In this PR <https://github.com/alphapapa/makem.sh/pull/39>, we made it
> possible for checkdoc/ispell to ignore localwords stored in a
> .dir-locals.el when run in batch mode.
> 
> It seemed appropriate to upstream this fix.

I'm not sure this is indeed safe enough.  We send these words to the
spell-checker; are we sure this couldn't be abused by malicious people
to perform some destructive actions?

Re: [PATCH] Make ispell localwords safe local variable

[Joseph Turner]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Mon, 14 Aug 2023 22:40:24 -0700
>>
>> In this PR <https://github.com/alphapapa/makem.sh/pull/39>, we made it
>> possible for checkdoc/ispell to ignore localwords stored in a
>> .dir-locals.el when run in batch mode.
>>
>> It seemed appropriate to upstream this fix.
>
> I'm not sure this is indeed safe enough.  We send these words to the
> spell-checker; are we sure this couldn't be abused by malicious people
> to perform some destructive actions?

I am not sure. Unless someone can be certain that ispell can safely
handle arbitrary strings, I think it's prudent to discard this patch.

Joseph

Re: [PATCH] Make ispell localwords safe local variable

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: emacs-devel@gnu.org
> Date: Tue, 15 Aug 2023 18:41:18 -0700
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Joseph Turner <joseph@breatheoutbreathe.in>
> >> Date: Mon, 14 Aug 2023 22:40:24 -0700
> >>
> >> In this PR <https://github.com/alphapapa/makem.sh/pull/39>, we made it
> >> possible for checkdoc/ispell to ignore localwords stored in a
> >> .dir-locals.el when run in batch mode.
> >>
> >> It seemed appropriate to upstream this fix.
> >
> > I'm not sure this is indeed safe enough.  We send these words to the
> > spell-checker; are we sure this couldn't be abused by malicious people
> > to perform some destructive actions?
> 
> I am not sure. Unless someone can be certain that ispell can safely
> handle arbitrary strings, I think it's prudent to discard this patch.

Does anyone else have an opinion about this?

Re: [PATCH] Make ispell localwords safe local variable

[Jim Porter]

On 8/16/2023 5:20 AM, Eli Zaretskii wrote:
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: emacs-devel@gnu.org
>> Date: Tue, 15 Aug 2023 18:41:18 -0700
>>
>> I am not sure. Unless someone can be certain that ispell can safely
>> handle arbitrary strings, I think it's prudent to discard this patch.
> 
> Does anyone else have an opinion about this?

I'm not very familiar with how Emacs talks to ispell, but would the 
proposed change here be any less safe than the buffer-local "LocalWords" 
block?

Re: [PATCH] Make ispell localwords safe local variable

[Joseph Turner]

On August 16, 2023 10:12:30 AM PDT, Jim Porter <jporterbugs@gmail.com> wrote:
>On 8/16/2023 5:20 AM, Eli Zaretskii wrote:
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Cc: emacs-devel@gnu.org
>>> Date: Tue, 15 Aug 2023 18:41:18 -0700
>>> 
>>> I am not sure. Unless someone can be certain that ispell can safely
>>> handle arbitrary strings, I think it's prudent to discard this patch.
>> 
>> Does anyone else have an opinion about this?
>
>I'm not very familiar with how Emacs talks to ispell, but would the proposed change here be any less safe than the buffer-local "LocalWords" block?

(which are sent to ispell without user confirmation)

Re: [PATCH] Make ispell localwords safe local variable

[Eli Zaretskii]

> Date: Wed, 16 Aug 2023 16:04:48 -0700
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> CC: emacs-devel@gnu.org
> 
> 
> 
> On August 16, 2023 10:12:30 AM PDT, Jim Porter <jporterbugs@gmail.com> wrote:
> >On 8/16/2023 5:20 AM, Eli Zaretskii wrote:
> >>> From: Joseph Turner <joseph@breatheoutbreathe.in>
> >>> Cc: emacs-devel@gnu.org
> >>> Date: Tue, 15 Aug 2023 18:41:18 -0700
> >>> 
> >>> I am not sure. Unless someone can be certain that ispell can safely
> >>> handle arbitrary strings, I think it's prudent to discard this patch.
> >> 
> >> Does anyone else have an opinion about this?
> >
> >I'm not very familiar with how Emacs talks to ispell, but would the proposed change here be any less safe than the buffer-local "LocalWords" block?
> 
> (which are sent to ispell without user confirmation)

It sounds like no one sees a problem with this change, so I've now
installed it on the master branch.

Thanks.

Re: [PATCH] Make ispell localwords safe local variable

[Joseph Turner]

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

Eli Zaretskii <eliz@gnu.org> writes:

> It sounds like no one sees a problem with this change, so I've now
> installed it on the master branch.

Would you be willing to merge the same commit on the emacs-29 branch?

If ispell-buffer-session-localwords is not safe, then package-vc-install
fails to install packages which set it in a dir-locals.el.  This is
currently the case with hyperdrive.el, whose manual recommends that
users mark it as safe in their own configuration prior to installation.

(put 'ispell-buffer-session-localwords 'safe-local-variable #'list-of-strings-p)

It's a minor change, but worth asking for.

Thank you!!!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-ispell-localwords-safe-local-variable.patch --]
[-- Type: text/x-diff, Size: 1040 bytes --]

From ae3c7031ed7471900b0ae27eff4ccfb04fd7f597 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Thu, 30 Nov 2023 22:50:29 -0800
Subject: [PATCH] Make ispell localwords safe local variable

* lisp/textmodes/ispell.el (ispell-buffer-session-localwords):
Make safe local variable to so that checkdoc can ignore words in a
project in batch mode.  Do not merge to master.
---
 lisp/textmodes/ispell.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 48d48b07937..b71e85b0e37 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1747,6 +1747,7 @@ Ispell is then restarted because the local words could conflict.")
 
 (defvar-local ispell-buffer-session-localwords nil
   "List of words accepted for session in this buffer.")
+(put 'ispell-buffer-session-localwords 'safe-local-variable #'list-of-strings-p)
 
 (defvar ispell-parser 'use-mode-name
   "Indicates whether ispell should parse the current buffer as TeX Code.
-- 
2.41.0

Re: [PATCH] Make ispell localwords safe local variable

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: jporterbugs@gmail.com, emacs-devel@gnu.org
> Date: Thu, 30 Nov 2023 22:56:45 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It sounds like no one sees a problem with this change, so I've now
> > installed it on the master branch.
> 
> Would you be willing to merge the same commit on the emacs-29 branch?

Sorry, no.  There were some questions about this being safe that I
think only time can answer in a satisfactory manner.  Leaving this on
master will give users enough time to try this and provide feedback in
case we overlooked something, before we release that code.

> If ispell-buffer-session-localwords is not safe, then package-vc-install
> fails to install packages which set it in a dir-locals.el.  This is
> currently the case with hyperdrive.el, whose manual recommends that
> users mark it as safe in their own configuration prior to installation.

Users who are hit by this problem can indeed add the necessary form in
their init files.  Or they can use the development version from the
master branch.  Or install the change locally and rebuild Emacs 29.
I'm sorry, but declaring a variable safe is a serious business in
Emacs, and we cannot do that in the middle of releasing a version of
Emacs which was used and tested without such a declaration.

Re: [PATCH] Make ispell localwords safe local variable

[Joseph Turner]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: jporterbugs@gmail.com, emacs-devel@gnu.org
>> Date: Thu, 30 Nov 2023 22:56:45 -0800
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > It sounds like no one sees a problem with this change, so I've now
>> > installed it on the master branch.
>>
>> Would you be willing to merge the same commit on the emacs-29 branch?
>
> Sorry, no.  There were some questions about this being safe that I
> think only time can answer in a satisfactory manner.  Leaving this on
> master will give users enough time to try this and provide feedback in
> case we overlooked something, before we release that code.
>
>> If ispell-buffer-session-localwords is not safe, then package-vc-install
>> fails to install packages which set it in a dir-locals.el.  This is
>> currently the case with hyperdrive.el, whose manual recommends that
>> users mark it as safe in their own configuration prior to installation.
>
> Users who are hit by this problem can indeed add the necessary form in
> their init files.  Or they can use the development version from the
> master branch.  Or install the change locally and rebuild Emacs 29.
> I'm sorry, but declaring a variable safe is a serious business in
> Emacs, and we cannot do that in the middle of releasing a version of
> Emacs which was used and tested without such a declaration.

Thank you for your thorough explanation and your caution!!  I'm grateful
to have you as Emacs co-maintainer.

Joseph

Re: [PATCH] Make ispell localwords safe local variable

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: jporterbugs@gmail.com, emacs-devel@gnu.org
> Date: Fri, 01 Dec 2023 00:24:49 -0800
> 
> Thank you for your thorough explanation and your caution!!  I'm grateful
> to have you as Emacs co-maintainer.

Thanks.

Re: [PATCH] Make ispell localwords safe local variable

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > >> In this PR <https://github.com/alphapapa/makem.sh/pull/39>, we made it
  > >> possible for checkdoc/ispell to ignore localwords stored in a
  > >> .dir-locals.el when run in batch mode.

  > > I'm not sure this is indeed safe enough.  We send these words to the
  > > spell-checker; are we sure this couldn't be abused by malicious people
  > > to perform some destructive actions?

  > I am not sure. Unless someone can be certain that ispell can safely
  > handle arbitrary strings, I think it's prudent to discard this patch.

Suppose it doesn't handle arbitrary strings -- what's the worst that
could happen?  Ispell would crash perhaps?

I think it would be ok to give users a chance to try using this and see

Another idea: scan the localwords for "reasonableness", perhaps if in
each word all the characters fit into certain classes of reasonable
characters for wrds.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)