Fix shell injection vulnerability and preserve command substitution syntax

[EdNutting]

Fix shell injection vulnerability and preserve command substitution syntax

This PR addresses a potential security vulnerability, and fixes incorrect handling of shell command substitution syntax in job arguments.

Summary

Three key issues resolved:

1. Security fix: Replaced create_subprocess_shell() with create_subprocess_exec() in LocalScheduler to prevent arbitrary command execution through malicious job specifications
2. Command substitution preservation: Fixed expandvars incorrectly stripping command substitution syntax like $(hostname) and $((3 + 2)), which caused bash syntax errors
3. Quote-aware parsing: Enhanced command substitution detection to properly handle shell quoting, escaped characters, and parentheses inside quoted strings

Behavior

The fix ensures proper handling of shell syntax based on the command being executed:

!JobArray
repeats: 1
env:
    CUSTOM_VAR: Elrond
jobs:
  - !Job
      ident: test_bash
      command: bash
      args: ["-c", "echo Hello from $(hostname) with calculation 3 + 2 = $((3 + 2)) with custom env var ${CUSTOM_VAR}"]
  - !Job
      ident: test_raw_echo
      command: echo
      args: ["Hello from $(hostname) with calculation 3 + 2 = $((3 + 2)) with custom env var ${CUSTOM_VAR}"]

Execution results:

• With bash/sh: Command substitutions are executed as expected

  INFO  Launching task: bash -c 'echo Hello from $(hostname) with calculation 3 + 2 = $((3 + 2)) with custom env var Elrond'
  INFO  stdio  Hello from slurm-node01 with calculation 3 + 2 = 5 with custom env var Elrond
  

• Without shell: Literal strings are preserved; env vars are substituted

  INFO  Launching task: echo 'Hello from $(hostname) with calculation 3 + 2 = $((3 + 2)) with custom env var Elrond'
  INFO  stdio  Hello from $(hostname) with calculation 3 + 2 = $((3 + 2)) with custom env var Elrond
  

Technical Changes

• gator/scheduler/local.py: Replace shell-based subprocess execution with exec-based for security
• gator/common/utility.py: Add command substitution preservation utilities with quote-aware parsing
• gator/wrapper.py: Use new utility functions for argument expansion
• Tests: Added 134+ test cases covering security, command substitution, quoting, and integration scenarios

Test Plan

• All existing tests pass (106 tests)
• New security tests verify shell injection prevention (test_local_scheduler.py)
• Command substitution tests cover 28+ edge cases (test_command_substitution.py)
• Integration tests verify end-to-end behavior (test_wrapper.py)
• Manual verification with real execution (see example above with Slurm)

🤖 Co-authored with Claude Code

[Intuity]

I can't recreate the documented issue with expandvars - this needs more justification to add a custom expansion, as its significant code being introduced.

While I'm happy with the switch to using subprocess exec instead of shell due to lower resources, I'm not convinced in the justification - I don't believe it's in gator's purview to be sanity checking what the user is asking to execute.

[Intuity]

I don't understand the need for this, from trialling expandvars locally:

>>> import expandvars
>>> expandvars.expand("echo my command with $(pwd) and $USER $((1+2))")
'echo my command with $(pwd) and peterbirch $((1+2))'

The $(pwd) and $((1+2)) (examples given) are preserved and not eliminated

Can you provide an example of where this has been an issue?

[EdNutting]

I have given an example in the PR. The $ is stripped off meaning it's not preserved. expandvars does not handle $( properly.

[EdNutting]

Maybe there's a version mismatch issue on the package??

[EdNutting]

I just ran poetry run python within Gator, and this is what I get:

>>> import expandvars
>>> expandvars.expand("echo my command with $(pwd) and $USER $((1+2)) $(hostname)")
'echo my command with (pwd) and ednutting ((1+2)) (hostname)'
>>> 

[EdNutting]

For reference:

(gator-eda-py3.11) ednutting:Gator$ poetry show expandvars
 name         : expandvars                         
 version      : 0.9.0                              
 description  : Expand system variables Unix style 
(gator-eda-py3.11) ednutting:Gator$ 

[EdNutting]

Okay, confirmed, newer version of expandvars doesn't have the $( issue.

[EdNutting]

Upgraded to newer package version and simplified the code.

The code could be simplified further to make the warning-detection code simpler/shorter by using a simple regex rather than the basic parser, but at the risk of generating slightly "noisier" warnings as any dollar or backtick (after expandvars has done its thing) would cause a warning to be generated.

Which would you prefer, simpler/shorter code with noisier warnings, or the current simple parser with more accurate warnings? Or just eliminate the warning altogether? (Though I think eliminating the warning may end up being a gotcha for developers in the future)

[EdNutting]

Simpler approach committed on a separate branch: EdNutting@65237c5

[Intuity]

Having re-read the asyncio documentation, I can see why an LLM check may have flagged this as a security vulnerability as it says create_subprocess_shell is at risk of injection vulnerabilities

However...

Given that the whole point of gator is that you can submit any arbitrary command through a job - I'd argue this is not a "security fix".

I'm in agreement with the change as there's still a benefit here in that a sub-shell is not invoked, which makes it slightly lighter on resources.

[EdNutting]

I'd mostly agree - I had the same initial view - and then I realised this subprocess invocation isn't supposed to be causing user-provided commands/args to be interpreted at this point on the host that runs this invocation. So, whether you think it's a security issue or not, it's a genuine problem that commands may end up running on a different machine / in a different place than the user may have intended or expected.

[Intuity]

I'm not sure I agree with the intent of this test - I don't think it falls to gator to protect the user from submitting jobs that can do damage. Gator is scheduling arbitrary commands, if that happens to contain an rm -rf / then that's on the upstream/encasing system to detect/prevent.

[EdNutting]

This is protecting Gator from injecting stuff into itself that it didn't intend to, given that Gator invokes itself in a nested fashion where the nesting does end up including user-provided input which the user might not have expected would end up in an intermediary shell command and thus interpreted in the wrong place.

It happened to me, albeit with a non-destructive command.