fix: shell-safe remote path handling in SSHConnector (#18)

.issueflows/03-solved-issues/issue18_original.md

@@ -0,0 +1,19 @@
+# Issue #18: Shell-safe remote path handling in SSHConnector
+
+Source: https://github.com/ife-bat/oeleo/issues/18
+
+## Original issue text
+
+## Summary
+`SSHConnector` builds remote shell commands with string interpolation (`find`, `md5sum`, `mkdir -p`). Metacharacters or awkward paths can break commands or enable injection.
+
+**Finding ID:** SEC-01  
+**Size:** standard  
+**Design doc:** `.issueflows/04-designs-and-guides/code-review-security.md`
+
+## Fix direction
+Prefer APIs that avoid a shell; otherwise `shlex.quote` every interpolated token. Add SSH tests with spaces in paths.
+
+## Acceptance
+- [ ] Remote path/glob interpolation is quoted or shell-free
+- [ ] Integration test covers a remote path containing spaces (where feasible)

.issueflows/03-solved-issues/issue18_plan.md

@@ -0,0 +1,67 @@
+# Plan: Issue #18 — Shell-safe remote path handling in SSHConnector
+
+## Goal
+
+Make every `SSHConnector` remote shell command quote interpolated paths/globs (and keep Windows remote construction safe enough not to regress), so spaces and metacharacters cannot break or inject commands. Prove with unit asserts on built commands plus an SSH integration case with spaces when feasible.
+
+## Constraints
+
+- Finding **SEC-01** only — do not expand into REL-02 (silent empty list), SEC-03 (password optional), or connector package split.
+- Prefer minimal change: `shlex.quote` on every interpolated token for POSIX remotes; keep Fabric `c.put` (already shell-free). Do **not** switch checksum to SFTP/`hashlib` in this PR unless quoting proves insufficient (that is a larger behavior/compat change).
+- Preserve public API of `SSHConnector` (`base_filter_sub_method`, `calculate_checksum`, `move_func`, etc.).
+- Tests: unit path via `uv run pytest -m "not ssh"`; space-path integration under `@pytest.mark.ssh` / `OELEO_SSH_TESTS=1`.
+- Follow agent rule from [code-review-security.md](.issueflows/04-designs-and-guides/code-review-security.md): any new SSH remote invocation must quote or avoid the shell.
+
+### Prior art
+
+- `SSHConnector._list_content` — [`oeleo/connectors.py`](oeleo/connectors.py): `find {directory} [-maxdepth N] -name '{glob}'` unquoted directory; single-quoted glob (breaks on `'` / metacharacters).
+- `SSHConnector.calculate_checksum` — `md5sum "{directory/f}"` (double quotes only; `$`, `` ` ``, `\` still dangerous).
+- `SSHConnector._ensure_remote_dir` — `mkdir -p "{remote_dir}"` (same).
+- Debug helpers `_check_connection_and_exit` / `check_connection_and_exit` — same `find` pattern; quote for consistency.
+- `SSHConnector.move_func` — uses `c.put` (shell-free); only `_ensure_remote_dir` is the shell risk on the copy path.
+- Design: [code-review-security.md](.issueflows/04-designs-and-guides/code-review-security.md) SEC-01 mitigations (quote preferred over SFTP for this issue size).
+- Integration: [`tests/test_ssh_integration.py`](tests/test_ssh_integration.py) — no spaces today; fixture itself builds unquoted setup cmds (fix when adding space-path coverage).
+- Toolbox: none. Graph: none. Grep: no existing `shlex` usage in package.
+
+## Approach
+
+1. **Helper** — Add a small private helper on `SSHConnector` (or module-level), e.g. `_remote_shell_token(value) -> str`, that stringifies path-likes and applies `shlex.quote` for POSIX (`is_posix=True`). Use it for every interpolated shell token.
+2. **Quote call sites (POSIX):**
+   - `_list_content`: quote `self.directory` and `glob_pattern`; keep `max_depth` as int-only (no string interpolation of untrusted depth).
+   - `calculate_checksum`: quote `self.directory / f`.
+   - `_ensure_remote_dir`: quote `remote_dir`.
+   - Both connection-check helpers: quote `self.directory`.
+3. **Windows remote (`is_posix=False`)** — Leave cmd shape (`dir` / `if not exist … mkdir`) but avoid naive `"` wrapping that still allows injection; either apply a documented Windows-safe escape or **scope Windows quoting as best-effort / follow-up** if unused by CI and lab path (see Open questions). Do not silently claim full Windows shell safety without a test.
+4. **Unit tests (no Docker)** — Mock `Connection.run` (or inject a fake `c`):
+   - Path with spaces builds a command containing `shlex.quote`’d tokens (and does not leave bare `my dir`).
+   - Path / name with `;` or `$()` appears only inside quoted tokens (assert exact command string for a few fixtures).
+   - Cover `_list_content`, `calculate_checksum`, `_ensure_remote_dir` at minimum.
+5. **SSH integration (gated)** — Extend `tests/test_ssh_integration.py`: create remote dir/file with a space in the path (quote fixture setup cmds too); assert `move_func` and/or `base_filter_sub_method` / checksum succeed. Skip remains when `OELEO_SSH_TESTS != 1`.
+6. **Design memory** — Mark SEC-01 done in security + overview + backlog when landing.
+
+## Files to touch
+
+| Path | Change |
+|------|--------|
+| [`oeleo/connectors.py`](oeleo/connectors.py) | Quote helper + apply at all SSH shell construction sites |
+| [`tests/test_oeleo.py`](tests/test_oeleo.py) or new `tests/test_ssh_shell_safety.py` | Unit tests with mocked Fabric `run` |
+| [`tests/test_ssh_integration.py`](tests/test_ssh_integration.py) | Space-in-path case; quote fixture remote cmds |
+| [`.issueflows/04-designs-and-guides/code-review-security.md`](.issueflows/04-designs-and-guides/code-review-security.md) | Mark SEC-01 addressed |
+| [`.issueflows/04-designs-and-guides/code-review-overview.md`](.issueflows/04-designs-and-guides/code-review-overview.md) | Strike SEC-01 in severity table |
+| [`.issueflows/04-designs-and-guides/code-review-improvement-backlog.md`](.issueflows/04-designs-and-guides/code-review-improvement-backlog.md) | Mark #18 done |
+
+## Test strategy
+
+- Default CI / local: `uv run pytest -m "not ssh"` — must cover quoting via mocks.
+- Optional: `OELEO_SSH_TESTS=1` + `uv run pytest -m ssh` for space-path integration (agent runs if env/Docker available; otherwise document as manual/CI-gated).
+- No change to SharePoint / local connector tests expected.
+
+## Open questions
+
+1. **Windows remote quoting scope?** Recommended: **POSIX is the acceptance bar** (matches Docker SSH tests and typical `is_posix=True` deployments); apply a light Windows escape or leave Windows cmds with a short code comment + backlog note if no Windows SSH test harness exists.
+2. **SFTP checksum/`mkdir` instead of shell?** Recommended: **no for this PR** — quoting satisfies SEC-01 acceptance; SFTP can be a follow-up if we want shell-free ops entirely.
+3. **Reject “dangerous” path characters at validate time?** Recommended: **no** — quoting is enough; rejection would break legitimate lab paths with spaces.
+
+## Scope check
+
+Single connector concern, focused files, one PR. Fits without splitting.

.issueflows/03-solved-issues/issue18_status.md

@@ -0,0 +1,16 @@
+# Status: Issue #18
+
+- [x] Done
+
+## What's done
+
+- Plan confirmed (POSIX `shlex.quote`; no SFTP rewrite; no path rejection).
+- `SSHConnector._remote_shell_token` + quoted `find` / `md5sum` / `mkdir` / debug helpers.
+- Unit tests in `tests/test_ssh_shell_safety.py` (mocked Fabric `run`).
+- Gated SSH integration: space-in-path case + quoted fixture cmds.
+- Design docs: SEC-01 marked done (security, overview, backlog).
+- `uv run pytest -m "not ssh"` — 25 passed (5 new shell-safety tests).
+
+## Remaining work
+
+- None.

.issueflows/04-designs-and-guides/code-review-improvement-backlog.md

@@ -41,7 +41,7 @@ Do this as a **dedicated PR**, not mixed with BUG-* logic fixes. Details: [`code
 
 | GitHub | Title | IDs | Size | Notes |
 |--------|-------|-----|------|-------|
-| [#18](https://github.com/ife-bat/oeleo/issues/18) | Shell-safe remote path handling in `SSHConnector` | SEC-01 | **standard** | `shlex.quote` or avoid shell; tests with spaces |
+| ~~[#18](https://github.com/ife-bat/oeleo/issues/18)~~ | ~~Shell-safe remote path handling in `SSHConnector`~~ | ~~SEC-01~~ | **done** | POSIX `shlex.quote`; unit + gated SSH space-path tests |
 | — | Make `OELEO_PASSWORD` optional for key-based SSH | SEC-03 | **yolo-fit** | Don’t require env when `use_password=False` · *not filed yet* |
 
 ## P2 — API cleanup
@@ -104,7 +104,7 @@ Filed on GitHub (2026-07-16). Two parallel tracks (separate branches/PRs):
 2. [#15](https://github.com/ife-bat/oeleo/issues/15) BUG-01 — yolo  
 3. [#16](https://github.com/ife-bat/oeleo/issues/16) BUG-04 — yolo  
 4. ~~[#17](https://github.com/ife-bat/oeleo/issues/17) REL-01~~ — done  
-5. [#18](https://github.com/ife-bat/oeleo/issues/18) SEC-01 — plan  
+5. ~~[#18](https://github.com/ife-bat/oeleo/issues/18) SEC-01~~ — done  
 6. [#19](https://github.com/ife-bat/oeleo/issues/19) DOC-01 — yolo  
 
 Later: open an **epic** for BUG-02 (relative-path bookkeeping) — not filed yet.

.issueflows/04-designs-and-guides/code-review-overview.md

@@ -68,7 +68,7 @@ env (.env) → factories (simple_worker / ssh_worker / sharepoint_worker)
 | BUG-01 | Frozen files (`code=2`) are **not** respected in `Worker.run` / `is_changed` | correctness |
 | BUG-02 | DB keys files by **basename only** → collisions when `include_subdirs=True` | correctness |
 | BUG-03 | `app/oa.pyw` logs `worker.db_path` which **does not exist** on `Worker` | correctness |
-| SEC-01 | SSH `find` / `md5sum` / `mkdir` built via string interpolation (shell injection risk) | security |
+| ~~SEC-01~~ | ~~SSH `find` / `md5sum` / `mkdir` string interpolation~~ — **resolved in #18** (`shlex.quote` / `_remote_shell_token`) | security |
 | ~~REL-01~~ | ~~`Worker.reconnect=True` reconnects **before every file**~~ — **resolved in #17** (`reconnect=False` default; opt-in via kwarg / `OELEO_RECONNECT`) | correctness |
 
 ### Medium

.issueflows/04-designs-and-guides/code-review-security.md

@@ -15,24 +15,9 @@ Trust boundaries:
 
 ## Findings
 
-### SEC-01 — Remote command construction (high)
+### ~~SEC-01 — Remote command construction (high)~~ (done in #18)
 
-`SSHConnector` builds shell commands with interpolated paths/patterns:
-
-- `find {self.directory} -name '{glob_pattern}'`
-- `md5sum "{self.directory/f}"`
-- `mkdir -p "{remote_dir}"`
-
-If `directory`, filenames, or glob patterns ever contain shell metacharacters (or attacker-controlled names under the source tree that become remote paths), this is classic injection. Even without a hostile actor, spaces/quotes in paths can break commands oddly.
-
-**Mitigations (prefer in order):**
-
-1. Pass paths via Fabric/`remote` APIs that avoid a shell where possible (`put` already used for upload).
-2. Use `shlex.quote` for every interpolated token.
-3. Reject path components with unexpected characters at connector validation time.
-4. Prefer `sftp`/`stat` for checksums if available instead of `md5sum` via shell.
-
-**Issue size:** medium; **not** pure yolo if behavior changes on exotic paths — add SSH tests with spaces in names.
+~~`SSHConnector` built shell commands with unquoted / lightly quoted interpolation.~~ **Resolved:** POSIX remotes use `_remote_shell_token` / `shlex.quote` for `find`, `md5sum`, and `mkdir -p` (and debug helpers). Windows remotes get best-effort `cmd.exe` quoting only. Unit tests assert quoted commands; gated SSH integration covers spaces in remote paths. Shell-free SFTP checksum/`mkdir` left as optional follow-up.
 
 ---
 

oeleo/connectors.py

@@ -2,6 +2,7 @@
 import hashlib
 import logging
 import os
+import shlex
 import sys
 import time
 from pathlib import Path, PurePosixPath, PureWindowsPath
@@ -187,6 +188,18 @@ def _validate(self):
             log.debug("Not on posix")
         log.debug(f"The ssh directory is: {self.directory}")
 
+    def _remote_shell_token(self, value: Any) -> str:
+        """Quote a path/token for remote shell interpolation (SEC-01).
+
+        POSIX remotes use ``shlex.quote`` (the supported / tested path).
+        Windows remotes get a best-effort ``cmd.exe`` double-quote wrap only;
+        full Windows shell safety is not claimed without a dedicated harness.
+        """
+        text = str(value)
+        if self.is_posix:
+            return shlex.quote(text)
+        return '"' + text.replace('"', '""') + '"'
+
     def connect(self, **kwargs) -> None:
         if self.use_password:
             connect_kwargs = {
@@ -216,23 +229,29 @@ def _check_connection_and_exit(self):
         # used only when developing oeleo
         log.debug("Connected?")
         if self.is_posix:
-            cmd = f"find {self.directory} -maxdepth 1 -name '*'"
+            cmd = (
+                f"find {self._remote_shell_token(self.directory)} "
+                f"-maxdepth 1 -name {self._remote_shell_token('*')}"
+            )
             log.debug(cmd)
             self.c.run(cmd)
         else:
-            cmd = f"dir {self.directory}"
+            cmd = f"dir {self._remote_shell_token(self.directory)}"
             log.debug(cmd)
             self.c.run(cmd)
         sys.exit()
 
     def check_connection_and_exit(self):
         log.debug("Connected?")
         if self.is_posix:
-            cmd = f"find {self.directory} -maxdepth 1 -name '*'"
+            cmd = (
+                f"find {self._remote_shell_token(self.directory)} "
+                f"-maxdepth 1 -name {self._remote_shell_token('*')}"
+            )
             log.debug(cmd)
             self.c.run(cmd)
         else:
-            cmd = f"dir {self.directory}"
+            cmd = f"dir {self._remote_shell_token(self.directory)}"
             log.debug(cmd)
             self.c.run(cmd)
         sys.exit()
@@ -282,10 +301,13 @@ def _list_content(self, glob_pattern="*", max_depth=1, hide=False):
             log.debug("Connecting ...")
             self.connect()
 
+        directory_q = self._remote_shell_token(self.directory)
+        pattern_q = self._remote_shell_token(glob_pattern)
         if max_depth is None:
-            cmd = f"find {self.directory} -name '{glob_pattern}'"
+            cmd = f"find {directory_q} -name {pattern_q}"
         else:
-            cmd = f"find {self.directory} -maxdepth {max_depth} -name '{glob_pattern}'"
+            depth = int(max_depth)
+            cmd = f"find {directory_q} -maxdepth {depth} -name {pattern_q}"
         log.debug(cmd)
         file_list = []
         try:
@@ -304,8 +326,8 @@ def calculate_checksum(self, f, hide=True):
             log.debug("Connecting ...")
             self.connect()
 
-        cmd = f'md5sum "{self.directory/f}"'
-        result = self.c.run(cmd, hide=hide)
+        cmd = f"md5sum {self._remote_shell_token(self.directory / f)}"
+        result = self.c.run(cmd, hide=hide, in_stream=False)
         if not result.ok:
             log.debug("it failed - should raise an exception her (future work)")
         checksum = result.stdout.strip().split()[0]
@@ -316,10 +338,12 @@ def _ensure_remote_dir(self, remote_dir: Path) -> None:
             log.debug("Connecting ...")
             self.connect()
 
+        remote_q = self._remote_shell_token(remote_dir)
         if self.is_posix:
-            cmd = f'mkdir -p "{remote_dir}"'
+            cmd = f"mkdir -p {remote_q}"
         else:
-            cmd = f'if not exist "{remote_dir}" mkdir "{remote_dir}"'
+            # Best-effort cmd.exe quoting only; POSIX is the SEC-01 acceptance path.
+            cmd = f"if not exist {remote_q} mkdir {remote_q}"
 
         log.debug(f"Ensuring remote dir exists: {remote_dir}")
         self.c.run(cmd, hide=True, in_stream=False)