From 4b20e3ad91665bc92a58f272c52286fcb06f44fe Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 9 Feb 2025 15:52:29 +0100 Subject: [PATCH] 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 e697add5b5ef2854e906814234850374ffac08ae. See #10987, #10991 --- CHANGELOG.rst | 5 ----- doc_src/language.rst | 2 -- src/future_feature_flags.rs | 12 ------------ src/input.rs | 15 ++------------- src/input_common.rs | 6 +----- src/reader.rs | 7 +------ tests/checks/status.fish | 1 - 7 files changed, 4 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index db1358b14..18b4e46ba 100644 --- a/CHANGELOG.rst +++ b/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. diff --git a/doc_src/language.rst b/doc_src/language.rst index fa6c66c9c..a62406fac 100644 --- a/doc_src/language.rst +++ b/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 `'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 ` 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:: diff --git a/src/future_feature_flags.rs b/src/future_feature_flags.rs index e4afbe73b..503da62bd 100644 --- a/src/future_feature_flags.rs +++ b/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), ], } } diff --git a/src/input.rs b/src/input.rs index b8d230479..eddd9eace 100644 --- a/src/input.rs +++ b/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); } diff --git a/src/input_common.rs b/src/input_common.rs index 5d9a02e8d..0802c33bd 100644 --- a/src/input_common.rs +++ b/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; diff --git a/src/reader.rs b/src/reader.rs index a2392e7f8..d3afcd016 100644 --- a/src/reader.rs +++ b/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(); } diff --git a/tests/checks/status.fish b/tests/checks/status.fish index 19b70f495..3b2973268 100644 --- a/tests/checks/status.fish +++ b/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