Back out "Feature flag to prevent executing off buffered keys"

e697add5b5 (Feature flag to prevent executing off buffered keys, 2025-01-02)
breaks my expectations/habits, and it breaks Midnight Commander.
Additionally, I'm not aware of any case where it actually adds security.
We generally assume that terminal echoback sequences do not contain
control characters except for well-known escape sequences.

This backs out commit e697add5b5.

See #10987, #10991

CHANGELOG.rst

@@ -16,11 +16,6 @@ Interactive improvements
 ------------------------
 - Autosuggestions are now also provided in multi-line command lines. Like `ctrl-r`, autosuggestions operate only on the current line.
 - Autosuggestions used to not suggest multi-line commandlines from history; now autosuggestions include individual lines from multi-line command lines.
-- If you insert a character that invalidates an autosuggestion and type backspace immediately after, then the autosuggestion will be shown again.
-- 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`).
 - The history search now preserves ordering between :kbd:`ctrl-s` forward and :kbd:`ctrl-r` backward searches.
 - Left mouse click now can select pager items.
 - Instead of flashing all the text to the left of the cursor, fish now flashes the matched token during history token search, the completed token during completion (:issue:`11050`), the autosuggestion when deleting it, and the full command line in all other cases.

doc_src/language.rst

@@ -2024,7 +2024,6 @@ 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 4.0 %self is no longer expanded (use $fish_pid)
     test-require-arg        off 4.0 builtin test requires an argument
-    buffered-enter-noexec   off 4.1 enter typed while executing will not execute
 
 Here is what they mean:
 
@@ -2034,7 +2033,6 @@ Here is what they mean:
 - ``ampersand-nobg-in-token`` was introduced in fish 3.4 (and made the default in 3.5). 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 4.0. 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 4.0. 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,9 +27,6 @@ pub enum FeatureFlag {
 
     /// Remove `test`'s one and zero arg mode (make `test -n` return false etc)
     test_require_arg,
-
-    /// Buffered enter (typed while running a command) does not execute.
-    buffered_enter_noexec,
 }
 
 struct Features {
@@ -110,14 +107,6 @@ pub struct 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!(
@@ -179,7 +168,6 @@ const fn new() -> Self {
                 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

@@ -8,7 +8,7 @@
 use crate::future::IsSomeAnd;
 use crate::input_common::{
     BlockingWait, CharEvent, CharInputStyle, CursorPositionWait, ImplicitEvent, InputData,
-    InputEventQueuer, ReadlineCmd, ReadlineCmdEvent, READING_BUFFERED_INPUT, R_END_INPUT_FUNCTIONS,
+    InputEventQueuer, ReadlineCmd, R_END_INPUT_FUNCTIONS,
 };
 use crate::key::ViewportPosition;
 use crate::key::{self, canonicalize_raw_escapes, ctrl, Key, Modifiers};
@@ -879,12 +879,6 @@ fn mapping_execute(&mut self, m: &InputMapping) {
         }
         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
@@ -898,12 +892,7 @@ fn mapping_execute(&mut self, m: &InputMapping) {
                             .collect(),
                     )
                 }
-                None => {
-                    if READING_BUFFERED_INPUT.load() {
-                        continue;
-                    }
-                    CharEvent::Command(cmd.clone())
-                }
+                None => CharEvent::Command(cmd.clone()),
             };
             self.push_front(evt);
         }

src/input_common.rs

@@ -296,8 +296,6 @@ pub fn from_check_exit() -> CharEvent {
 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
@@ -340,7 +338,7 @@ fn readb(in_fd: RawFd, blocking: bool) -> ReadbResult {
         }
 
         // Here's where we call select().
-        let select_res = fdset.check_readable(if blocking && !READING_BUFFERED_INPUT.load() {
+        let select_res = fdset.check_readable(if blocking {
             Timeout::Forever
         } else {
             Timeout::ZERO
@@ -379,8 +377,6 @@ 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

@@ -70,8 +70,6 @@
 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,
@@ -82,6 +80,7 @@
 };
 use crate::input::init_input;
 use crate::input_common::kitty_progressive_enhancements_query;
+use crate::input_common::terminal_protocols_disable_ifn;
 use crate::input_common::unblock_input;
 use crate::input_common::BlockingWait;
 use crate::input_common::Capability;
@@ -97,7 +96,6 @@
     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;
@@ -4263,9 +4261,6 @@ 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,7 +59,6 @@ status features
 #CHECK: ampersand-nobg-in-token on  3.4 & only backgrounds if followed by a separator
 #CHECK: remove-percent-self     off 4.0 %self is no longer expanded (use $fish_pid)
 #CHECK: test-require-arg        off 4.0 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