From 4b0e5400f9b643574a90ed5d87a577a7eef3ed66 Mon Sep 17 00:00:00 2001 From: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com> Date: Wed, 1 Nov 2023 11:07:26 +0000 Subject: [PATCH 1/6] Fix handling of complex environment variables Changes: - Now detects environment variables within and between strings for more complex variables - Raises error if an environment variable is missing - Passes string referring to environment variable transparently to prevent accidental variable leakage. --- seqerakit/seqeraplatform.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/seqerakit/seqeraplatform.py b/seqerakit/seqeraplatform.py index 61bc31a..a11e9fa 100644 --- a/seqerakit/seqeraplatform.py +++ b/seqerakit/seqeraplatform.py @@ -73,14 +73,13 @@ def _construct_command(self, cmd, *args, **kwargs): def _check_env_vars(self, command): full_cmd_parts = [] for arg in command: - if arg.startswith("$"): - env_var = arg[1:] - if env_var not in os.environ: - logging.error(f" Environment variable '{env_var}' is not set.") - return None # handle as desired - full_cmd_parts.append(os.environ[env_var]) - else: - full_cmd_parts.append(shlex.quote(arg)) + if "$" in arg: + for env_var in re.findall(r"\$\{?[\w]+\}?", arg): + if env_var.replace("$", "") not in os.environ: + raise EnvironmentError( + f" Environment variable '{env_var}' is not in environment." + ) + full_cmd_parts.append(shlex.quote(arg)) return " ".join(full_cmd_parts) # Executes a 'tw' command in a subprocess and returns the output. From 94010d591efd4cf014ef25949d80525ff688b615 Mon Sep 17 00:00:00 2001 From: Adam Talbot <12817534+adamrtalbot@users.noreply.github.com> Date: Wed, 1 Nov 2023 12:12:26 +0000 Subject: [PATCH 2/6] Apply suggestions from code review --- seqerakit/seqeraplatform.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/seqerakit/seqeraplatform.py b/seqerakit/seqeraplatform.py index a11e9fa..430ec9f 100644 --- a/seqerakit/seqeraplatform.py +++ b/seqerakit/seqeraplatform.py @@ -77,7 +77,7 @@ def _check_env_vars(self, command): for env_var in re.findall(r"\$\{?[\w]+\}?", arg): if env_var.replace("$", "") not in os.environ: raise EnvironmentError( - f" Environment variable '{env_var}' is not in environment." + f" Environment variable '{env_var}' not found!" ) full_cmd_parts.append(shlex.quote(arg)) return " ".join(full_cmd_parts) From 3cd910d64438eca63c3aa0caa839e64a4ae4f90b Mon Sep 17 00:00:00 2001 From: ejseqera Date: Wed, 1 Nov 2023 10:23:14 -0400 Subject: [PATCH 3/6] fix: avoid quoting of env vars --- seqerakit/seqeraplatform.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/seqerakit/seqeraplatform.py b/seqerakit/seqeraplatform.py index 430ec9f..0c88ab8 100644 --- a/seqerakit/seqeraplatform.py +++ b/seqerakit/seqeraplatform.py @@ -79,7 +79,9 @@ def _check_env_vars(self, command): raise EnvironmentError( f" Environment variable '{env_var}' not found!" ) - full_cmd_parts.append(shlex.quote(arg)) + full_cmd_parts.append(arg) + else: + full_cmd_parts.append(shlex.quote(arg)) return " ".join(full_cmd_parts) # Executes a 'tw' command in a subprocess and returns the output. From b03f416d4611447bff5ea56f6f6d5ff79463f525 Mon Sep 17 00:00:00 2001 From: ejseqera Date: Wed, 1 Nov 2023 10:40:56 -0400 Subject: [PATCH 4/6] tests: add tests for env var checking --- tests/unit/test_seqeraplatform.py | 42 +++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/unit/test_seqeraplatform.py b/tests/unit/test_seqeraplatform.py index 65a95d4..d034f45 100644 --- a/tests/unit/test_seqeraplatform.py +++ b/tests/unit/test_seqeraplatform.py @@ -3,6 +3,7 @@ from seqerakit import seqeraplatform import json import subprocess +import os class TestSeqeraPlatform(unittest.TestCase): @@ -179,5 +180,46 @@ def test_dryrun_call(self, mock_subprocess): mock_subprocess.assert_not_called() +class TestCheckEnvVars(unittest.TestCase): + def setUp(self): + self.sp = seqeraplatform.SeqeraPlatform() + self.original_environ = dict(os.environ) + + def tearDown(self): + # Restore the original environment after each test + os.environ.clear() + os.environ.update(self.original_environ) + + def test_with_set_env_vars(self): + # Set environment variables for the test + os.environ["VAR1"] = "value1" + + command = ["tw", "pipelines", "list", "-w", "$VAR1"] + expected = "tw pipelines list -w $VAR1" + result = self.sp._check_env_vars(command) + self.assertEqual(result, expected) + + def test_without_env_vars(self): + # Test case where there are no environment variables in the command + command = ["tw", "info"] + expected = "tw info" # shlex.quote() will not alter these + result = self.sp._check_env_vars(command) + self.assertEqual(result, expected) + + def test_error_raised_for_unset_env_vars(self): + # Unset environment variables for this test + if "UNSET_VAR" in os.environ: + del os.environ["UNSET_VAR"] + + command = ["tw", "pipelines", "list", "-w", "$UNSET_VAR"] + + # Assert that EnvironmentError is raised + with self.assertRaises(EnvironmentError) as context: + self.sp._check_env_vars(command) + self.assertIn( + " Environment variable '$UNSET_VAR' not found!", str(context.exception) + ) + + if __name__ == "__main__": unittest.main() From bb2c491cd7afd5630c7401f317f3f38bcb11bf28 Mon Sep 17 00:00:00 2001 From: ejseqera Date: Wed, 1 Nov 2023 10:58:22 -0400 Subject: [PATCH 5/6] fix: env var name extraction with brackets --- seqerakit/seqeraplatform.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/seqerakit/seqeraplatform.py b/seqerakit/seqeraplatform.py index 0c88ab8..fd4853a 100644 --- a/seqerakit/seqeraplatform.py +++ b/seqerakit/seqeraplatform.py @@ -75,9 +75,9 @@ def _check_env_vars(self, command): for arg in command: if "$" in arg: for env_var in re.findall(r"\$\{?[\w]+\}?", arg): - if env_var.replace("$", "") not in os.environ: + if re.sub(r"[${}]", "", env_var) not in os.environ: raise EnvironmentError( - f" Environment variable '{env_var}' not found!" + f" Environment variable {env_var} not found!" ) full_cmd_parts.append(arg) else: From c190d925ad42c5e3fb3fe850edd6b4c36870ed24 Mon Sep 17 00:00:00 2001 From: ejseqera Date: Wed, 1 Nov 2023 11:50:45 -0400 Subject: [PATCH 6/6] tests: fix failing env var unset test --- tests/unit/test_seqeraplatform.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_seqeraplatform.py b/tests/unit/test_seqeraplatform.py index d034f45..10239f0 100644 --- a/tests/unit/test_seqeraplatform.py +++ b/tests/unit/test_seqeraplatform.py @@ -208,16 +208,17 @@ def test_without_env_vars(self): def test_error_raised_for_unset_env_vars(self): # Unset environment variables for this test + unset_var = "{UNSET_VAR}" if "UNSET_VAR" in os.environ: del os.environ["UNSET_VAR"] - command = ["tw", "pipelines", "list", "-w", "$UNSET_VAR"] + command = ["tw", "pipelines", "list", "-w", "${UNSET_VAR}"] # Assert that EnvironmentError is raised with self.assertRaises(EnvironmentError) as context: self.sp._check_env_vars(command) - self.assertIn( - " Environment variable '$UNSET_VAR' not found!", str(context.exception) + self.assertEqual( + str(context.exception), f" Environment variable ${unset_var} not found!" )