vim-patch:9.0.1968: cmdline completion should consider key option

Problem:  cmdline completion should consider key option
Solution: Disable cmdline completion for key option, slightly
          refactor how P_NO_CMD_EXPAND is handled

Harden crypto 'key' option: turn off cmdline completion, disable set-=

"set-=" can be used maliciously with a crypto key, as it allows an
attacker (who either has access to the computer or a plugin author) to
guess a substring by observing the modified state. Simply turn off
set+=/-=/^= for this option as there is no good reason for them to be
used.

Update docs to make that clear as well.

Also, don't allow cmdline completion for 'key' as it just shows *****
which is not useful and confusing to the user what it means (if the user
accidentally hits enter they will have replaced their key with "*****"
instead).

Move logic to better location, don't use above 32-bit for flags

Move P_NO_CMD_EXPAND to use the unused 0x20 instead of going above
32-bits, as currently the flags parameter is only 32-bits on some
systems. Left a comment to warn that future additions will need to
change how the flags work either by making it 64-bit or split into two
member vars.

Also, move the logic for detecting P_NO_CMD_EXPAND earlier so it's not
up to each handler to decide, and you won't see the temporary "..." that
Vim shows while waiting for completion handler to complete.

closes: vim/vim#13224

6ee7b521fa

Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>

src/nvim/option.c

@@ -136,13 +136,6 @@ static char *p_vsts_nopaste;
 
 #define OPTION_COUNT ARRAY_SIZE(options)
 
-typedef enum {
-  OP_NONE = 0,
-  OP_ADDING,      ///< "opt+=arg"
-  OP_PREPENDING,  ///< "opt^=arg"
-  OP_REMOVING,    ///< "opt-=arg"
-} set_op_T;
-
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 # include "option.c.generated.h"
 #endif
@@ -1168,7 +1161,7 @@ static void do_set_option_string(int opt_idx, int opt_flags, char **argp, int ne
   // be triggered that can cause havoc.
   *errmsg = did_set_string_option(curbuf, curwin, opt_idx, (char **)varp, oldval,
                                   errbuf, errbuflen,
-                                  opt_flags, value_checked);
+                                  opt_flags, op, value_checked);
 
   secure = secure_saved;
 

src/nvim/option_defs.h

@@ -26,6 +26,14 @@ typedef struct {
   } data;
 } OptVal;
 
+/// :set operator types
+typedef enum {
+  OP_NONE = 0,
+  OP_ADDING,      ///< "opt+=arg"
+  OP_PREPENDING,  ///< "opt^=arg"
+  OP_REMOVING,    ///< "opt-=arg"
+} set_op_T;
+
 /// Argument for the callback function (opt_did_set_cb_T) invoked after an
 /// option value is modified.
 typedef struct {
@@ -34,6 +42,7 @@ typedef struct {
   void *os_varp;
   int os_idx;
   int os_flags;
+  set_op_T os_op;
 
   /// old value of the option (can be a string, number or a boolean)
   union {

src/nvim/option_vars.h

@@ -53,6 +53,9 @@
 #define P_MLE          0x20000000U  ///< under control of 'modelineexpr'
 #define P_FUNC         0x40000000U  ///< accept a function reference or a lambda
 #define P_COLON        0x80000000U  ///< values use colons to create sublists
+// Warning: Currently we have used all 32 bits for option flags, and adding more
+//          flags will overflow it. Adding another flag will need to change how
+//          it's stored first.
 
 #define HIGHLIGHT_INIT \
   "8:SpecialKey,~:EndOfBuffer,z:TermCursor,Z:TermCursorNC,@:NonText,d:Directory,e:ErrorMsg," \

src/nvim/optionstr.c

@@ -503,8 +503,9 @@ const char *set_string_option(const int opt_idx, void *varp_arg, const char *val
     secure = 1;
   }
 
-  const char *const errmsg = did_set_string_option(curbuf, curwin, opt_idx, varp, oldval, errbuf,
-                                                   errbuflen, opt_flags, value_checked);
+  const char *const errmsg = did_set_string_option(curbuf, curwin, opt_idx, varp, oldval,
+                                                   errbuf, errbuflen,
+                                                   opt_flags, OP_NONE, value_checked);
 
   secure = secure_saved;
 
@@ -2682,11 +2683,12 @@ static void do_spelllang_source(win_T *win)
 /// @param errbuf  buffer for errors, or NULL
 /// @param errbuflen  length of errors buffer
 /// @param opt_flags  OPT_LOCAL and/or OPT_GLOBAL
+/// @param op OP_ADDING/OP_PREPENDING/OP_REMOVING
 /// @param value_checked  value was checked to be safe, no need to set P_INSECURE
 ///
 /// @return  NULL for success, or an untranslated error message for an error
 const char *did_set_string_option(buf_T *buf, win_T *win, int opt_idx, char **varp, char *oldval,
-                                  char *errbuf, size_t errbuflen, int opt_flags,
+                                  char *errbuf, size_t errbuflen, int opt_flags, set_op_T op,
                                   bool *value_checked)
 {
   const char *errmsg = NULL;
@@ -2700,6 +2702,7 @@ const char *did_set_string_option(buf_T *buf, win_T *win, int opt_idx, char **va
     .os_varp = varp,
     .os_idx = opt_idx,
     .os_flags = opt_flags,
+    .os_op = op,
     .os_oldval.string = oldval,
     .os_newval.string = *varp,
     .os_value_checked = false,

test/old/testdir/test_history.vim

@@ -244,8 +244,13 @@ endfunc
 " Test for making sure the key value is not stored in history
 func Test_history_crypt_key()
   CheckFeature cryptv
+
   call feedkeys(":set bs=2 key=abc ts=8\<CR>", 'xt')
   call assert_equal('set bs=2 key= ts=8', histget(':'))
+
+  call assert_fails("call feedkeys(':set bs=2 key-=abc ts=8\<CR>', 'xt')")
+  call assert_equal('set bs=2 key-= ts=8', histget(':'))
+
   set key& bs& ts&
 endfunc
 

test/old/testdir/test_options.vim

@@ -369,13 +369,15 @@ func Test_set_completion()
   call feedkeys(":set spellsuggest=best,file:test_options.v\<Tab>\<C-B>\"\<CR>", 'xt')
   call assert_equal("\"set spellsuggest=best,file:test_options.vim", @:)
 
-  " Expand value for 'key'
-  " set key=abcd
-  " call feedkeys(":set key=\<Tab>\<C-B>\"\<CR>", 'xt')
-  " call assert_equal('"set key=*****', @:)
-  " call feedkeys(":set key-=\<Tab>\<C-B>\"\<CR>", 'xt')
-  " call assert_equal('"set key-=*****', @:)
-  " set key=
+  " Expanding value for 'key' is disallowed
+  if exists('+key')
+    set key=abcd
+    call feedkeys(":set key=\<Tab>\<C-B>\"\<CR>", 'xt')
+    call assert_equal('"set key=', @:)
+    call feedkeys(":set key-=\<Tab>\<C-B>\"\<CR>", 'xt')
+    call assert_equal('"set key-=', @:)
+    set key=
+  endif
 
   " Expand values for 'filetype'
   call feedkeys(":set filetype=sshdconfi\<Tab>\<C-B>\"\<CR>", 'xt')