From bdfbdaafcc78dbdb9ee1bb14d7cb30742b4e1c9c Mon Sep 17 00:00:00 2001
From: Fabian Boehm <FHomborg@gmail.com>
Date: Tue, 6 Feb 2024 22:12:55 +0100
Subject: [PATCH] Forbid subcommand keywords in variables-as-commands (#10249)

This stops you from doing e.g.

```fish
set pager command less
echo foo | $pager
```

Currently, it would run the command *builtin*, which can only do
`--search` and similar, and would most likely end up printing its own
help.

That means it very very likely won't work, and the code is misguided -
it is trying to defeat function resolution in a way that won't do what
the author wants it to.

The alternative would be to make the command *builtin* execute the
command, *but*

1. That would require rearchitecting and rewriting a bunch of it and
the parser
2. It would be a large footgun, in that `set EDITOR command foo` will
only ever work inside fish, but $EDITOR is also used outside.

I don't want to add a feature that we would immediately have to discourage.
---
 src/parse_execution.rs      | 15 +++++++++++++++
 tests/checks/expansion.fish |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/src/parse_execution.rs b/src/parse_execution.rs
index e03db1377..d93f70bd1 100644
--- a/src/parse_execution.rs
+++ b/src/parse_execution.rs
@@ -34,6 +34,7 @@ use crate::parse_constants::{
 use crate::parse_tree::{NodeRef, ParsedSourceRef};
 use crate::parse_util::parse_util_unescape_wildcards;
 use crate::parser::{Block, BlockId, BlockType, LoopStatus, Parser, ProfileItem};
+use crate::parser_keywords::parser_keywords_is_subcommand;
 use crate::path::{path_as_implicit_cd, path_try_get_path};
 use crate::pointer::ConstPointer;
 use crate::proc::{
@@ -567,6 +568,20 @@ impl<'a> ParseExecutionContext {
                 "The expanded command was empty."
             );
         }
+        // Complain if we've expanded to a subcommand keyword like "command" or "if",
+        // because these will call the corresponding *builtin*,
+        // which won't be doing what the user asks for
+        //
+        // (skipping in no-exec because we don't have the actual variable value)
+        if !no_exec() && parser_keywords_is_subcommand(out_cmd) && &unexp_cmd != out_cmd {
+            return report_error!(
+                self,
+                ctx,
+                STATUS_ILLEGAL_CMD.unwrap(),
+                &statement.command,
+                "The expanded command is a keyword."
+            );
+        }
         EndExecutionReason::ok
     }
 
diff --git a/tests/checks/expansion.fish b/tests/checks/expansion.fish
index d1bf5180d..1b6511ad4 100644
--- a/tests/checks/expansion.fish
+++ b/tests/checks/expansion.fish
@@ -334,3 +334,9 @@ printf '<%s>\n' ($fish -c 'echo "$abc["' 2>&1)
 #CHECK: <fish: Invalid index value>
 #CHECK: <echo "$abc[">
 #CHECK: <           ^>
+
+set -l pager command less
+echo foo | $pager
+#CHECKERR: checks/expansion.fish (line 339): The expanded command is a keyword.
+#CHECKERR: echo foo | $pager
+#CHECKERR:            ^~~~~^