fix: add shlex.quote to all remote shell commands in filesystem.py (#154)

[aditya226-sharma]

Summary

Adds shlex.quote() to all nine remote shell commands in filesystem.py to prevent command injection via unsanitized paths and glob patterns.

Changes

• Added import shlex to filesystem.py
• Quoted all path arguments: ls, stat, test -e, test -d, test -f, du -sb, find
• Quoted all pattern arguments: find -name, ls -d
• All 9 vulnerable command-building sites now use shlex.quote()

Why

Every one of these functions (cluster_ls, cluster_find, cluster_stat, cluster_exists, cluster_isdir, cluster_isfile, cluster_glob, cluster_du, cluster_count_files) is public API reachable through documented convenience functions. A path or pattern carrying shell metacharacters runs as a command on the cluster, as the user, with their credentials. This is the same defect class PR #139 fixed in utils.py.

Closes #154

[jeremymanning]

Thank you for this — the vulnerability is real, your diagnosis of it is correct, and you found it independently. Eight of the nine sites in this diff are fixed properly, and the two find -name sites are genuinely better than what they replace: find interprets the pattern itself, so shlex.quote is equivalent to the hand-written '{pattern}' there while additionally surviving a pattern that contains an apostrophe. That is a real improvement and I would not have thought to call it out if it were not.

There is one blocking problem, and then some scope that the issue asks for.

Blocking: _remote_glob stops globbing

_remote_glob works by handing the pattern to the remote shell and letting the shell expand it. Quoting the pattern is precisely what stops it being a pattern:

>>> shlex.quote("*.csv")
"'*.csv'"

Reproduced against a real directory containing a.csv, b.csv, c.txt, running both command strings through a real shell:

cluster_glob('*.csv') on a remote config
  current code : ['a.csv', 'b.csv']
  this PR      : []

ls -d '*.csv' looks for a file literally named *.csv. So the function returns an empty list rather than raising, and a pipeline that globs for its inputs silently finds nothing. That trades a security bug for a correctness bug, and the quiet kind.

Nothing in the test suite catches this. Every existing cluster_glob test uses a local config — tests/test_filesystem.py:401, tests/test_filesystem_hybrid.py:190, tests/test_packaging_integration.py:430 — so none of them reach _remote_glob at all. Worth knowing when you re-run: a green suite is not evidence here.

Also worth flagging: no CI has run on this branch. gh pr checks 156 reports no checks, so nothing has been exercised yet.

The fix that keeps both properties

Escaping cannot solve this one, because the pattern has to stay a pattern. The way out is to stop involving a shell: expand the glob yourself against real directory entries over SFTP (paramiko's SFTPClient.listdir plus fnmatch), and the dilemma disappears — globbing still works and nothing reaches a shell.

That reasoning generalises, and it is what issue #154 asks for: "Where SFTP can replace a shell command outright, it does — that is a removal of the attack surface rather than an escape of it." SFTPClient.stat() similarly retires _remote_stat, _remote_exists, _remote_isdir and _remote_isfile — no shell, no quoting question, and it also fixes the portability bug below. Quoting is right for the places with no SFTP equivalent (find, du).

Remaining scope from #154

This PR says Closes #154, but two of the three defects in that issue are untouched:

1. _remote_stat is GNU-only, and its comment claims otherwise. Line 451 says # Use stat command with portable format; line 452 uses stat -c, which BSD and macOS reject. On such a host the command fails, 2>/dev/null swallows the error, output is empty, and the caller gets FileNotFoundError for a file that exists.
2. 2>/dev/null converts every failure of these commands into a false negative — not just this one. A permission error, a missing binary and a genuinely absent file are currently indistinguishable to the caller.

And the issue's definition of done asks for tests: hostile paths containing ;, &&, |, $(...), backticks, a single quote, a newline and a leading -, proving they are treated as filenames and never executed. tests/ssh_server.py on master is a real in-process paramiko server — real socket, real handshake, real SFTP over a real directory — so these can be tested against real SSH rather than a mock. A guard that fails when a new unquoted interpolation appears in the module is also on that list.

One thing you should know

I should be straight with you rather than let you discover it in a merge conflict: work on #154 was already in progress when this arrived, taking the SFTP approach described above. That does not make your contribution redundant — you identified the vulnerability independently and correctly, and the find -name observation stands on its own. But I would rather tell you now than have you invest another round without knowing.

If you would like to carry this forward, the glob fix is the interesting part and the piece a reviewer will care most about. If you would rather not, that is completely fair, and the report itself was worth having.

[jeremymanning]

@aditya226-sharma thank you for your contribution! i had @claude review and it found a few issues; i'll have it post a comment in case you're up for addressing them.

[jeremymanning]

ah-- it beat me to it 😄 . but: let me know if you have any questions.

[jeremymanning]

Update, since you should hear it from us rather than by noticing the issue went quiet: #154 is now closed, fixed by a different approach.

The blocking problem in this PR was the one I described above — quoting the pattern in _remote_glob stops it being a pattern, so cluster_glob('*.csv') silently returns nothing. That turned out to generalise. Escaping is the wrong tool for most of these sites, so seven of the nine moved to SFTP instead: ls, stat, exists, isdir, isfile, glob and du no longer touch a shell at all. Only find still shells out, because walking a tree over SFTP costs a round trip per directory, and both its arguments are shlex.quoted there — which is exactly what you did, and it was right for that site.

Your two find -name changes were the part of this diff I singled out as an improvement, and the shipped fix has the same thing in the same place for the same reason.

Some numbers, in case they are useful: an adversarial review then threw 14 payload families at the result — -, --, -exec, $IFS, ~root, trailing backslash, process substitution, $'..', heredoc, newline-in-pattern, ../../../etc/passwd — across both paths and patterns, and nothing executed. It also found the first anti-regression guard nearly blind: it matched variable names, so self._run_remote(f"ls -1 {full_path}") was exploitable and reported clean. The guard now tracks tainted values and discovers sinks by fixpoint.

I am leaving this PR open rather than closing it, because that is the maintainer's call and not mine to make unilaterally.

Thank you for the report. Finding a live command injection in a public API, diagnosing it correctly, and writing a patch for it is a real contribution, and the fact that the eventual fix took a different shape does not change that. If you want to pick something else up, #159 collects the defects that are live right now and each one has a reproduction in it.