Merge pull request #747 from python-cmd2/allow_redirection_fix

Fixed inconsistent treatment of allow_redirection

Author: tleonhardt

CHANGELOG.md

@@ -1,4 +1,7 @@
 ## 0.9.16 (TBD, 2019)
+* Bug Fixes
+    * Fixed inconsistent parsing/tab completion behavior based on the value of `allow_redirection`. This flag is
+    only meant to be a security setting that prevents redirection of stdout and should not alter parsing logic.
 * Enhancements
     * Raise `TypeError` if trying to set choices/completions on argparse action that accepts no arguments
     * Create directory for the persistent history file if it does not already exist

cmd2/cmd2.py

@@ -353,7 +353,8 @@ def __init__(self, completekey: str = 'tab', stdin=None, stdout=None, *,
                                commands to be run or, if -t is specified, transcript files to run.
                                This should be set to False if your application parses its own arguments.
         :param transcript_files: allow running transcript tests when allow_cli_args is False
-        :param allow_redirection: should output redirection and pipes be allowed
+        :param allow_redirection: should output redirection and pipes be allowed. this is only a security setting
+                                  and does not alter parsing behavior.
         :param multiline_commands: list of commands allowed to accept multi-line input
         :param terminators: list of characters that terminate a command. These are mainly intended for terminating
                             multiline commands, but will also terminate single-line commands. If not supplied, then
@@ -376,11 +377,14 @@ def __init__(self, completekey: str = 'tab', stdin=None, stdout=None, *,
         # Call super class constructor
         super().__init__(completekey=completekey, stdin=stdin, stdout=stdout)
 
-        # Attributes which should NOT be dynamically settable at runtime
+        # Attributes which should NOT be dynamically settable via the set command at runtime
+        # To prevent a user from altering these with the py/ipy commands, remove locals_in_py from the
+        # settable dictionary during your applications's __init__ method.
         self.default_to_shell = False  # Attempt to run unrecognized commands as shell commands
         self.quit_on_sigint = False  # Quit the loop on interrupt instead of just resetting prompt
+        self.allow_redirection = allow_redirection  # Security setting to prevent redirection of stdout
 
-        # Attributes which ARE dynamically settable at runtime
+        # Attributes which ARE dynamically settable via the set command at runtime
         self.continuation_prompt = '> '
         self.debug = False
         self.echo = False
@@ -440,8 +444,7 @@ def __init__(self, completekey: str = 'tab', stdin=None, stdout=None, *,
         # True if running inside a Python script or interactive console, False otherwise
         self._in_py = False
 
-        self.statement_parser = StatementParser(allow_redirection=allow_redirection,
-                                                terminators=terminators,
+        self.statement_parser = StatementParser(terminators=terminators,
                                                 multiline_commands=multiline_commands,
                                                 shortcuts=shortcuts)
 
@@ -616,16 +619,6 @@ def aliases(self) -> Dict[str, str]:
         """Read-only property to access the aliases stored in the StatementParser."""
         return self.statement_parser.aliases
 
-    @property
-    def allow_redirection(self) -> bool:
-        """Getter for the allow_redirection property that determines whether or not redirection of stdout is allowed."""
-        return self.statement_parser.allow_redirection
-
-    @allow_redirection.setter
-    def allow_redirection(self, value: bool) -> None:
-        """Setter for the allow_redirection property that determines whether or not redirection of stdout is allowed."""
-        self.statement_parser.allow_redirection = value
-
     def poutput(self, msg: Any, *, end: str = '\n') -> None:
         """Print message to self.stdout and appends a newline by default
 
@@ -831,61 +824,56 @@ def tokens_for_completion(self, line: str, begidx: int, endidx: int) -> Tuple[Li
                     # Return empty lists since this means the line is malformed.
                     return [], []
 
-        if self.allow_redirection:
+        # We need to treat redirection characters (|, <, >) as word breaks when they are in unquoted strings.
+        # Go through each token and further split them on these characters. Each run of redirect characters
+        # is treated as a single token.
+        raw_tokens = []
 
-            # Since redirection is enabled, we need to treat redirection characters (|, <, >)
-            # as word breaks when they are in unquoted strings. Go through each token
-            # and further split them on these characters. Each run of redirect characters
-            # is treated as a single token.
-            raw_tokens = []
+        for cur_initial_token in initial_tokens:
 
-            for cur_initial_token in initial_tokens:
+            # Save tokens up to 1 character in length or quoted tokens. No need to parse these.
+            if len(cur_initial_token) <= 1 or cur_initial_token[0] in constants.QUOTES:
+                raw_tokens.append(cur_initial_token)
+                continue
 
-                # Save tokens up to 1 character in length or quoted tokens. No need to parse these.
-                if len(cur_initial_token) <= 1 or cur_initial_token[0] in constants.QUOTES:
-                    raw_tokens.append(cur_initial_token)
-                    continue
+            # Iterate over each character in this token
+            cur_index = 0
+            cur_char = cur_initial_token[cur_index]
 
-                # Iterate over each character in this token
-                cur_index = 0
-                cur_char = cur_initial_token[cur_index]
+            # Keep track of the token we are building
+            cur_raw_token = ''
 
-                # Keep track of the token we are building
-                cur_raw_token = ''
+            while True:
+                if cur_char not in constants.REDIRECTION_CHARS:
 
-                while True:
-                    if cur_char not in constants.REDIRECTION_CHARS:
-
-                        # Keep appending to cur_raw_token until we hit a redirect char
-                        while cur_char not in constants.REDIRECTION_CHARS:
-                            cur_raw_token += cur_char
-                            cur_index += 1
-                            if cur_index < len(cur_initial_token):
-                                cur_char = cur_initial_token[cur_index]
-                            else:
-                                break
+                    # Keep appending to cur_raw_token until we hit a redirect char
+                    while cur_char not in constants.REDIRECTION_CHARS:
+                        cur_raw_token += cur_char
+                        cur_index += 1
+                        if cur_index < len(cur_initial_token):
+                            cur_char = cur_initial_token[cur_index]
+                        else:
+                            break
 
-                    else:
-                        redirect_char = cur_char
-
-                        # Keep appending to cur_raw_token until we hit something other than redirect_char
-                        while cur_char == redirect_char:
-                            cur_raw_token += cur_char
-                            cur_index += 1
-                            if cur_index < len(cur_initial_token):
-                                cur_char = cur_initial_token[cur_index]
-                            else:
-                                break
+                else:
+                    redirect_char = cur_char
+
+                    # Keep appending to cur_raw_token until we hit something other than redirect_char
+                    while cur_char == redirect_char:
+                        cur_raw_token += cur_char
+                        cur_index += 1
+                        if cur_index < len(cur_initial_token):
+                            cur_char = cur_initial_token[cur_index]
+                        else:
+                            break
 
-                    # Save the current token
-                    raw_tokens.append(cur_raw_token)
-                    cur_raw_token = ''
+                # Save the current token
+                raw_tokens.append(cur_raw_token)
+                cur_raw_token = ''
 
-                    # Check if we've viewed all characters
-                    if cur_index >= len(cur_initial_token):
-                        break
-        else:
-            raw_tokens = initial_tokens
+                # Check if we've viewed all characters
+                if cur_index >= len(cur_initial_token):
+                    break
 
         # Save the unquoted tokens
         tokens = [utils.strip_quotes(cur_token) for cur_token in raw_tokens]
@@ -1228,72 +1216,70 @@ def _redirect_complete(self, text: str, line: str, begidx: int, endidx: int, com
                          this will be called if we aren't completing for redirection
         :return: a list of possible tab completions
         """
-        if self.allow_redirection:
-
-            # Get all tokens through the one being completed. We want the raw tokens
-            # so we can tell if redirection strings are quoted and ignore them.
-            _, raw_tokens = self.tokens_for_completion(line, begidx, endidx)
-            if not raw_tokens:
-                return []
+        # Get all tokens through the one being completed. We want the raw tokens
+        # so we can tell if redirection strings are quoted and ignore them.
+        _, raw_tokens = self.tokens_for_completion(line, begidx, endidx)
+        if not raw_tokens:
+            return []
 
-            # Must at least have the command
-            if len(raw_tokens) > 1:
+        # Must at least have the command
+        if len(raw_tokens) > 1:
 
-                # True when command line contains any redirection tokens
-                has_redirection = False
+            # True when command line contains any redirection tokens
+            has_redirection = False
 
-                # Keep track of state while examining tokens
-                in_pipe = False
-                in_file_redir = False
-                do_shell_completion = False
-                do_path_completion = False
-                prior_token = None
+            # Keep track of state while examining tokens
+            in_pipe = False
+            in_file_redir = False
+            do_shell_completion = False
+            do_path_completion = False
+            prior_token = None
 
-                for cur_token in raw_tokens:
-                    # Process redirection tokens
-                    if cur_token in constants.REDIRECTION_TOKENS:
-                        has_redirection = True
+            for cur_token in raw_tokens:
+                # Process redirection tokens
+                if cur_token in constants.REDIRECTION_TOKENS:
+                    has_redirection = True
 
-                        # Check if we are at a pipe
-                        if cur_token == constants.REDIRECTION_PIPE:
-                            # Do not complete bad syntax (e.g cmd | |)
-                            if prior_token == constants.REDIRECTION_PIPE:
-                                return []
+                    # Check if we are at a pipe
+                    if cur_token == constants.REDIRECTION_PIPE:
+                        # Do not complete bad syntax (e.g cmd | |)
+                        if prior_token == constants.REDIRECTION_PIPE:
+                            return []
 
-                            in_pipe = True
-                            in_file_redir = False
+                        in_pipe = True
+                        in_file_redir = False
 
-                        # Otherwise this is a file redirection token
-                        else:
-                            if prior_token in constants.REDIRECTION_TOKENS or in_file_redir:
-                                # Do not complete bad syntax (e.g cmd | >) (e.g cmd > blah >)
-                                return []
+                    # Otherwise this is a file redirection token
+                    else:
+                        if prior_token in constants.REDIRECTION_TOKENS or in_file_redir:
+                            # Do not complete bad syntax (e.g cmd | >) (e.g cmd > blah >)
+                            return []
 
-                            in_pipe = False
-                            in_file_redir = True
+                        in_pipe = False
+                        in_file_redir = True
 
-                    # Not a redirection token
-                    else:
-                        do_shell_completion = False
-                        do_path_completion = False
+                # Not a redirection token
+                else:
+                    do_shell_completion = False
+                    do_path_completion = False
 
-                        if prior_token == constants.REDIRECTION_PIPE:
-                            do_shell_completion = True
-                        elif in_pipe or prior_token in (constants.REDIRECTION_OUTPUT, constants.REDIRECTION_APPEND):
-                            do_path_completion = True
+                    if prior_token == constants.REDIRECTION_PIPE:
+                        do_shell_completion = True
+                    elif in_pipe or prior_token in (constants.REDIRECTION_OUTPUT, constants.REDIRECTION_APPEND):
+                        do_path_completion = True
 
-                    prior_token = cur_token
+                prior_token = cur_token
 
-                if do_shell_completion:
-                    return self.shell_cmd_complete(text, line, begidx, endidx)
+            if do_shell_completion:
+                return self.shell_cmd_complete(text, line, begidx, endidx)
 
-                elif do_path_completion:
-                    return self.path_complete(text, line, begidx, endidx)
+            elif do_path_completion:
+                return self.path_complete(text, line, begidx, endidx)
 
-                # If there were redirection strings anywhere on the command line, then we
-                # are no longer tab completing for the current command
-                elif has_redirection:
-                    return []
+            # If there were redirection strings anywhere on the command line, then we
+            # are no longer tab completing for the current command
+            elif has_redirection:
+                return []
 
         # Call the command's completer function
         return compfunc(text, line, begidx, endidx)
@@ -2313,12 +2299,10 @@ def _set_up_cmd2_readline(self) -> _SavedReadlineSettings:
             readline_settings.completer = readline.get_completer()
             readline.set_completer(self.complete)
 
-            # Break words on whitespace and quotes when tab completing
-            completer_delims = " \t\n" + ''.join(constants.QUOTES)
-
-            if self.allow_redirection:
-                # If redirection is allowed, then break words on those characters too
-                completer_delims += ''.join(constants.REDIRECTION_CHARS)
+            # Break words on whitespace, quotes, and redirectors when tab completing
+            completer_delims = " \t\n"
+            completer_delims += ''.join(constants.QUOTES)
+            completer_delims += ''.join(constants.REDIRECTION_CHARS)
 
             readline_settings.delims = readline.get_completer_delims()
             readline.set_completer_delims(completer_delims)

cmd2/parsing.py

@@ -245,7 +245,6 @@ class StatementParser:
     the expansion.
     """
     def __init__(self,
-                 allow_redirection: bool = True,
                  terminators: Optional[Iterable[str]] = None,
                  multiline_commands: Optional[Iterable[str]] = None,
                  aliases: Optional[Dict[str, str]] = None,
@@ -257,13 +256,11 @@ def __init__(self,
         * multiline commands
         * shortcuts
 
-        :param allow_redirection: should redirection and pipes be allowed?
         :param terminators: iterable containing strings which should terminate commands
         :param multiline_commands: iterable containing the names of commands that accept multiline input
         :param aliases: dictionary containing aliases
         :param shortcuts: dictionary containing shortcuts
         """
-        self.allow_redirection = allow_redirection
         if terminators is None:
             self.terminators = (constants.MULTILINE_TERMINATOR,)
         else:
@@ -690,8 +687,7 @@ def _split_on_punctuation(self, tokens: List[str]) -> List[str]:
         """
         punctuation = []
         punctuation.extend(self.terminators)
-        if self.allow_redirection:
-            punctuation.extend(constants.REDIRECTION_CHARS)
+        punctuation.extend(constants.REDIRECTION_CHARS)
 
         punctuated_tokens = []
 

tests/test_cmd2.py

@@ -558,7 +558,7 @@ def test_feedback_to_output_false(base_app):
 
 def test_disallow_redirection(base_app):
     # Set allow_redirection to False
-    base_app.statement_parser.allow_redirection = False
+    base_app.allow_redirection = False
 
     filename = 'test_allow_redirect.txt'
 

tests/test_completion.py

@@ -695,7 +695,6 @@ def test_tokens_for_completion_redirect(cmd2_app):
     endidx = len(line)
     begidx = endidx - len(text)
 
-    cmd2_app.allow_redirection = True
     expected_tokens = ['command', '|', '<', '>>', 'file']
     expected_raw_tokens = ['command', '|', '<', '>>', 'file']
 
@@ -717,20 +716,6 @@ def test_tokens_for_completion_quoted_redirect(cmd2_app):
     assert expected_tokens == tokens
     assert expected_raw_tokens == raw_tokens
 
-def test_tokens_for_completion_redirect_off(cmd2_app):
-    text = '>file'
-    line = 'command {}'.format(text)
-    endidx = len(line)
-    begidx = endidx - len(text)
-
-    cmd2_app.statement_parser.allow_redirection = False
-    expected_tokens = ['command', '>file']
-    expected_raw_tokens = ['command', '>file']
-
-    tokens, raw_tokens = cmd2_app.tokens_for_completion(line, begidx, endidx)
-    assert expected_tokens == tokens
-    assert expected_raw_tokens == raw_tokens
-
 def test_add_opening_quote_basic_no_text(cmd2_app):
     text = ''
     line = 'test_basic {}'.format(text)

tests/test_history.py

@@ -268,7 +268,6 @@ def histitem():
 def parser():
     from cmd2.parsing import StatementParser
     parser = StatementParser(
-        allow_redirection=True,
         terminators=[';', '&'],
         multiline_commands=['multiline'],
         aliases={'helpalias': 'help',

tests/test_parsing.py

@@ -13,7 +13,6 @@
 @pytest.fixture
 def parser():
     parser = StatementParser(
-        allow_redirection=True,
         terminators=[';', '&'],
         multiline_commands=['multiline'],
         aliases={'helpalias': 'help',