Feature flag to prevent executing off buffered keys

If I run "sleep 3", type a command and hit enter, then there is no
obvious way to cancel or edit the imminent command other than ctrl-c
but that also cancels sleep, and doesn't allow editing. (ctrl-z sort
of works, but also doesn't allow editing).

Let's try to limit ourselves to inserting the buffered command
(translating enter to a newline), and only execute once the user
actually presses enter after the previous command is done.
Hide it behind a new feature flag for now.

By making things less scary, this might be more user-friendly, at
the risk of breaking expectations in some cases.

This also fixes a class of security issues where a command like
`cat malicious-file.txt` might output escape sequences, causing
the terminal to echo back a malicious command; such files can still
insert into the command line but at least not execute it directly.
(Since it's only fixed partially I'm not really sure if the security
issue is a good enough motivation for this particular change.)

Note that bracketed paste probably has similar motivation as this feature.

Part of #10987
Closes #10991

CHANGELOG.rst

@@ -13,6 +13,10 @@ Scripting improvements
 Interactive improvements
 ------------------------
 - Autosuggestions are now also provided in  multi-line command lines. Like `ctrl-r`, autosuggestions operate only on the current line.
+- New feature flag ``buffered-enter-noexec`` with the following effect:
+  when typing a command and :kbd:`enter` while the previous one is still running, the new one will no longer execute immediately. Similarly, keys that are bound to shell commands will be ignored.
+  This mitigates a security issue where a command like ``cat malicious-file.txt`` could write terminal escape codes prompting the terminal to write arbitrary text to fish's standard input.
+  Such a malicious file can still potentially insert arbitrary text into the command line but can no longer execute it directly (:issue:`10987`).
 
 New or improved bindings
 ^^^^^^^^^^^^^^^^^^^^^^^^

doc_src/language.rst

@@ -2024,6 +2024,7 @@ You can see the current list of features via ``status features``::
     ampersand-nobg-in-token on  3.4 & only backgrounds if followed by a separating character
     remove-percent-self     off 3.8 %self is no longer expanded (use $fish_pid)
     test-require-arg        off 3.8 builtin test requires an argument
+    buffered-enter-noexec   off 4.1 enter typed while executing will not execute
 
 Here is what they mean:
 
@@ -2033,6 +2034,7 @@ Here is what they mean:
 - ``ampersand-nobg-in-token`` was introduced in fish 3.4. It makes it so a ``&`` i no longer interpreted as the backgrounding operator in the middle of a token, so dealing with URLs becomes easier. Either put spaces or a semicolon after the ``&``. This is recommended formatting anyway, and ``fish_indent`` will have done it for you already.
 - ``remove-percent-self`` turns off the special ``%self`` expansion. It was introduced in 3.8. To get fish's pid, you can use the :envvar:`fish_pid` variable.
 - ``test-require-arg`` removes :doc:`builtin test <cmds/test>`'s one-argument form (``test "string"``. It was introduced in 3.8. To test if a string is non-empty, use ``test -n "string"``. If disabled, any call to ``test`` that would change sends a :ref:`debug message <debugging-fish>` of category "deprecated-test", so starting fish with ``fish --debug=deprecated-test`` can be used to find offending calls.
+- ``buffered-enter-noexec`` typing enter during command execution will insert a newline into the next commandline instead of executing it.
 
 
 These changes are introduced off by default. They can be enabled on a per session basis::

src/future_feature_flags.rs

@@ -27,6 +27,9 @@ pub enum FeatureFlag {
 
     /// Remove `test`'s one and zero arg mode (make `test -n` return false etc)
     test_require_arg,
+
+    /// Buffered enter (typed wile running a command) does not execute.
+    buffered_enter_noexec,
 }
 
 struct Features {
@@ -107,6 +110,14 @@ pub const METADATA: &[FeatureMetadata] = &[
         default_value: false,
         read_only: false,
     },
+    FeatureMetadata {
+        flag: FeatureFlag::buffered_enter_noexec,
+        name: L!("buffered-enter-noexec"),
+        groups: L!("4.1"),
+        description: L!("enter typed while executing will not execute"),
+        default_value: false,
+        read_only: false,
+    },
 ];
 
 thread_local!(
@@ -168,6 +179,7 @@ impl Features {
                 AtomicBool::new(METADATA[3].default_value),
                 AtomicBool::new(METADATA[4].default_value),
                 AtomicBool::new(METADATA[5].default_value),
+                AtomicBool::new(METADATA[6].default_value),
             ],
         }
     }

src/input.rs

@@ -6,7 +6,7 @@ use crate::flog::FLOG;
 use crate::input_common::CursorPositionBlockingWait::MouseLeft;
 use crate::input_common::{
     CharEvent, CharInputStyle, CursorPositionWait, ImplicitEvent, InputData, InputEventQueuer,
-    ReadlineCmd, R_END_INPUT_FUNCTIONS,
+    ReadlineCmd, ReadlineCmdEvent, READING_BUFFERED_INPUT, R_END_INPUT_FUNCTIONS,
 };
 use crate::key::ViewportPosition;
 use crate::key::{self, canonicalize_raw_escapes, ctrl, Key, Modifiers};
@@ -886,6 +886,12 @@ impl<'a> Reader<'a> {
         }
         for cmd in m.commands.iter().rev() {
             let evt = match input_function_get_code(cmd) {
+                Some(ReadlineCmd::Execute) if READING_BUFFERED_INPUT.load() => {
+                    CharEvent::Readline(ReadlineCmdEvent {
+                        cmd: ReadlineCmd::SelfInsert,
+                        seq: WString::from_chars([key::Enter]),
+                    })
+                }
                 Some(code) => {
                     self.function_push_args(code);
                     // At this point, the sequence is only used for reinserting the keys into
@@ -899,7 +905,12 @@ impl<'a> Reader<'a> {
                             .collect(),
                     )
                 }
-                None => CharEvent::Command(cmd.clone()),
+                None => {
+                    if READING_BUFFERED_INPUT.load() {
+                        continue;
+                    }
+                    CharEvent::Command(cmd.clone())
+                }
             };
             self.push_front(evt);
         }

src/input_common.rs

@@ -291,6 +291,8 @@ static WAIT_ON_ESCAPE_MS: AtomicUsize = AtomicUsize::new(WAIT_ON_ESCAPE_DEFAULT)
 const WAIT_ON_SEQUENCE_KEY_INFINITE: usize = usize::MAX;
 static WAIT_ON_SEQUENCE_KEY_MS: AtomicUsize = AtomicUsize::new(WAIT_ON_SEQUENCE_KEY_INFINITE);
 
+pub(crate) static READING_BUFFERED_INPUT: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
+
 /// Internal function used by readch to read one byte.
 /// This calls select() on three fds: input (e.g. stdin), the ioport notifier fd (for main thread
 /// requests), and the uvar notifier. This returns either the byte which was read, or one of the
@@ -333,7 +335,7 @@ fn readb(in_fd: RawFd, blocking: bool) -> ReadbResult {
         }
 
         // Here's where we call select().
-        let select_res = fdset.check_readable(if blocking {
+        let select_res = fdset.check_readable(if blocking && !READING_BUFFERED_INPUT.load() {
             Timeout::Forever
         } else {
             Timeout::ZERO
@@ -372,6 +374,8 @@ fn readb(in_fd: RawFd, blocking: bool) -> ReadbResult {
             FLOG!(reader, "Read byte", char_to_symbol(char::from(c)));
             // The common path is to return a u8.
             return ReadbResult::Byte(c);
+        } else {
+            READING_BUFFERED_INPUT.store(false);
         }
         if !blocking {
             return ReadbResult::NothingToRead;

src/reader.rs

@@ -68,6 +68,8 @@ use crate::fds::{make_fd_blocking, wopen_cloexec, AutoCloseFd};
 use crate::flog::{FLOG, FLOGF};
 #[allow(unused_imports)]
 use crate::future::IsSomeAnd;
+use crate::future_feature_flags::feature_test;
+use crate::future_feature_flags::FeatureFlag;
 use crate::global_safety::RelaxedAtomicBool;
 use crate::highlight::{
     autosuggest_validate_from_history, highlight_shell, HighlightRole, HighlightSpec,
@@ -77,7 +79,6 @@ use crate::history::{
     SearchType,
 };
 use crate::input::init_input;
-use crate::input_common::terminal_protocols_disable_ifn;
 use crate::input_common::CursorPositionBlockingWait;
 use crate::input_common::CursorPositionWait;
 use crate::input_common::ImplicitEvent;
@@ -88,6 +89,7 @@ use crate::input_common::{
     terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, CharInputStyle, InputData,
     ReadlineCmd,
 };
+use crate::input_common::{terminal_protocols_disable_ifn, READING_BUFFERED_INPUT};
 use crate::input_common::{CURSOR_UP_SUPPORTED, SCROLL_FORWARD_SUPPORTED};
 use crate::io::IoChain;
 use crate::key::ViewportPosition;
@@ -4111,6 +4113,9 @@ fn term_steal(copy_modes: bool) {
             break;
         }
     }
+    if feature_test(FeatureFlag::buffered_enter_noexec) {
+        READING_BUFFERED_INPUT.store(true);
+    }
 
     termsize_invalidate_tty();
 }

tests/checks/status.fish

@@ -59,6 +59,7 @@ status features
 #CHECK: ampersand-nobg-in-token on  3.4 & only backgrounds if followed by a separator
 #CHECK: remove-percent-self     off 3.8 %self is no longer expanded (use $fish_pid)
 #CHECK: test-require-arg        off 3.8 builtin test requires an argument
+#CHECK: buffered-enter-noexec   off 4.1 enter typed while executing will not execute
 status test-feature stderr-nocaret
 echo $status
 #CHECK: 0