From 9f461f164ce736f2459d1c27343e387a4219c258 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 09:27:46 +0000 Subject: [PATCH 1/5] fix: quote SSH remote shell paths (SEC-01 / #18) Add SSHConnector._remote_shell_token (shlex.quote on POSIX) and use it for find/md5sum/mkdir and debug helpers so spaces and metacharacters cannot break or inject remote commands. Unit tests assert command shape; gated SSH integration covers paths with spaces. Co-authored-by: Jan Petter Maehlen --- .../01-current-issues/issue18_original.md | 19 ++++ .issueflows/01-current-issues/issue18_plan.md | 67 +++++++++++++ .../01-current-issues/issue18_status.md | 11 +++ .../code-review-improvement-backlog.md | 4 +- .../code-review-overview.md | 2 +- .../code-review-security.md | 19 +--- oeleo/connectors.py | 42 ++++++-- tests/test_ssh_integration.py | 98 ++++++++++++++++--- tests/test_ssh_shell_safety.py | 95 ++++++++++++++++++ 9 files changed, 315 insertions(+), 42 deletions(-) create mode 100644 .issueflows/01-current-issues/issue18_original.md create mode 100644 .issueflows/01-current-issues/issue18_plan.md create mode 100644 .issueflows/01-current-issues/issue18_status.md create mode 100644 tests/test_ssh_shell_safety.py diff --git a/.issueflows/01-current-issues/issue18_original.md b/.issueflows/01-current-issues/issue18_original.md new file mode 100644 index 0000000..2e807a4 --- /dev/null +++ b/.issueflows/01-current-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) diff --git a/.issueflows/01-current-issues/issue18_plan.md b/.issueflows/01-current-issues/issue18_plan.md new file mode 100644 index 0000000..edbdff3 --- /dev/null +++ b/.issueflows/01-current-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. diff --git a/.issueflows/01-current-issues/issue18_status.md b/.issueflows/01-current-issues/issue18_status.md new file mode 100644 index 0000000..f8df701 --- /dev/null +++ b/.issueflows/01-current-issues/issue18_status.md @@ -0,0 +1,11 @@ +# Status: Issue #18 + +- [ ] Done + +## What's done + +- Plan confirmed (POSIX `shlex.quote`; no SFTP rewrite; no path rejection). + +## Remaining work + +- Quote helper + SSH shell call sites; unit + gated SSH space-path tests; mark SEC-01 done in design docs. diff --git a/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md b/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md index 8396cbd..1ce273b 100644 --- a/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md +++ b/.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 — plan + confirm -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. diff --git a/.issueflows/04-designs-and-guides/code-review-overview.md b/.issueflows/04-designs-and-guides/code-review-overview.md index b4f7b2f..7e8bf8b 100644 --- a/.issueflows/04-designs-and-guides/code-review-overview.md +++ b/.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** (SSH thrashing) | correctness | ### Medium diff --git a/.issueflows/04-designs-and-guides/code-review-security.md b/.issueflows/04-designs-and-guides/code-review-security.md index da1ef12..2bc1b99 100644 --- a/.issueflows/04-designs-and-guides/code-review-security.md +++ b/.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. --- diff --git a/oeleo/connectors.py b/oeleo/connectors.py index 6a1c4d6..1d3542d 100644 --- a/oeleo/connectors.py +++ b/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,11 +229,14 @@ 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() @@ -228,11 +244,14 @@ def _check_connection_and_exit(self): 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,7 +326,7 @@ def calculate_checksum(self, f, hide=True): log.debug("Connecting ...") self.connect() - cmd = f'md5sum "{self.directory/f}"' + cmd = f"md5sum {self._remote_shell_token(self.directory / f)}" result = self.c.run(cmd, hide=hide) if not result.ok: log.debug("it failed - should raise an exception her (future work)") @@ -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) diff --git a/tests/test_ssh_integration.py b/tests/test_ssh_integration.py index 59e5aab..6377042 100644 --- a/tests/test_ssh_integration.py +++ b/tests/test_ssh_integration.py @@ -1,4 +1,5 @@ import os +import shlex from pathlib import PurePosixPath import pytest @@ -15,6 +16,12 @@ def _missing_env_vars(): return [key for key in required if not os.getenv(key)] +def _run_quoted(connector, template, *parts, **kwargs): + """Run a remote command with each path-like part shell-quoted.""" + quoted = [shlex.quote(str(p)) for p in parts] + return connector.c.run(template.format(*quoted), **kwargs) + + @pytest.fixture(scope="module") def ssh_remote_dir(): if os.getenv("OELEO_SSH_TESTS") != "1": @@ -35,14 +42,24 @@ def ssh_remote_dir(): result = setup_connector.c.run("mktemp -d", hide=True, in_stream=False) base_dir = result.stdout.strip() - setup_connector.c.run( - f"mkdir -p {base_dir}/sub", hide=True, in_stream=False + _run_quoted( + setup_connector, + "mkdir -p {}/sub", + base_dir, + hide=True, + in_stream=False, ) - setup_connector.c.run( - f"printf 'root' > {base_dir}/root.txt", hide=True, in_stream=False + _run_quoted( + setup_connector, + "printf 'root' > {}/root.txt", + base_dir, + hide=True, + in_stream=False, ) - setup_connector.c.run( - f"printf 'nested' > {base_dir}/sub/nested.txt", + _run_quoted( + setup_connector, + "printf 'nested' > {}/sub/nested.txt", + base_dir, hide=True, in_stream=False, ) @@ -50,8 +67,10 @@ def ssh_remote_dir(): yield base_dir finally: if base_dir: - setup_connector.c.run( - f"rm -rf {base_dir}", + _run_quoted( + setup_connector, + "rm -rf {}", + base_dir, warn=True, hide=True, in_stream=False, @@ -111,8 +130,10 @@ def test_ssh_connector_creates_missing_remote_dirs(ssh_remote_dir, tmp_path): remote_dir = PurePosixPath(ssh_remote_dir) / "newdir" remote_file = remote_dir / "local.txt" - connector.c.run( - f"rm -rf {remote_dir}", + _run_quoted( + connector, + "rm -rf {}", + remote_dir, hide=True, in_stream=False, warn=True, @@ -121,12 +142,63 @@ def test_ssh_connector_creates_missing_remote_dirs(ssh_remote_dir, tmp_path): success = connector.move_func(local_file, remote_file) assert success is True - result = connector.c.run( - f"test -f {remote_file}", + result = _run_quoted( + connector, + "test -f {}", + remote_file, hide=True, in_stream=False, warn=True, ) assert result.ok finally: - connector.close() \ No newline at end of file + connector.close() + + +@pytest.mark.ssh +def test_ssh_connector_handles_spaces_in_remote_path(ssh_remote_dir, tmp_path): + connector = SSHConnector( + directory=ssh_remote_dir, + use_password=True, + is_posix=True, + include_subdirs=True, + ) + connector.connect() + try: + local_file = tmp_path / "spaced name.txt" + local_file.write_text("hello spaces") + + remote_dir = PurePosixPath(ssh_remote_dir) / "dir with spaces" + remote_file = remote_dir / "spaced name.txt" + + _run_quoted( + connector, + "rm -rf {}", + remote_dir, + hide=True, + in_stream=False, + warn=True, + ) + + success = connector.move_func(local_file, remote_file) + assert success is True + + checksum = connector.calculate_checksum( + PurePosixPath("dir with spaces") / "spaced name.txt" + ) + assert isinstance(checksum, str) and len(checksum) == 32 + + files = connector.base_filter_sub_method(".txt") + assert any(f.name == "spaced name.txt" for f in files) + + result = _run_quoted( + connector, + "test -f {}", + remote_file, + hide=True, + in_stream=False, + warn=True, + ) + assert result.ok + finally: + connector.close() diff --git a/tests/test_ssh_shell_safety.py b/tests/test_ssh_shell_safety.py new file mode 100644 index 0000000..82c6125 --- /dev/null +++ b/tests/test_ssh_shell_safety.py @@ -0,0 +1,95 @@ +"""Unit tests for SEC-01: SSHConnector remote shell quoting (no live SSH).""" + +import shlex +from pathlib import PurePosixPath +from unittest.mock import MagicMock + +import pytest + +from oeleo.connectors import SSHConnector + + +@pytest.fixture +def ssh_env(monkeypatch): + monkeypatch.setenv("OELEO_PASSWORD", "secret") + monkeypatch.setenv("OELEO_USERNAME", "tester") + monkeypatch.setenv("OELEO_EXTERNAL_HOST", "localhost") + + +def _connector_with_mock_run(ssh_env, directory="/tmp/remote dest"): + connector = SSHConnector( + directory=directory, + use_password=True, + is_posix=True, + ) + fake = MagicMock() + result = MagicMock() + result.ok = True + result.stdout = "" + fake.run.return_value = result + connector.c = fake + return connector, fake + + +def test_remote_shell_token_quotes_spaces_and_metacharacters(ssh_env): + connector = SSHConnector( + directory="/tmp/safe", + use_password=True, + is_posix=True, + ) + awkward = "/tmp/dir with spaces; rm -rf /" + quoted = connector._remote_shell_token(awkward) + assert quoted == shlex.quote(awkward) + assert quoted.startswith("'") and quoted.endswith("'") + assert "; rm -rf" in quoted # still present, but only inside quotes + + +def test_list_content_quotes_directory_and_glob(ssh_env): + connector, fake = _connector_with_mock_run(ssh_env, directory="/data/out dir") + fake.run.return_value.stdout = "/data/out dir/file.txt\n" + + files = connector._list_content("*.txt", max_depth=1, hide=True) + + cmd = fake.run.call_args.args[0] + assert cmd == ( + f"find {shlex.quote('/data/out dir')} -maxdepth 1 " + f"-name {shlex.quote('*.txt')}" + ) + assert files == ["/data/out dir/file.txt"] + + +def test_list_content_quotes_injection_prone_glob(ssh_env): + connector, fake = _connector_with_mock_run(ssh_env, directory="/data") + evil_glob = "*.txt'; echo pwned" + + connector._list_content(evil_glob, max_depth=None, hide=True) + + cmd = fake.run.call_args.args[0] + assert cmd == ( + f"find {shlex.quote('/data')} -name {shlex.quote(evil_glob)}" + ) + assert "; echo pwned" not in cmd.replace(shlex.quote(evil_glob), "") + + +def test_calculate_checksum_quotes_remote_path(ssh_env): + connector, fake = _connector_with_mock_run( + ssh_env, directory="/data/my files" + ) + fake.run.return_value.stdout = "abc123 /data/my files/a b.txt\n" + + checksum = connector.calculate_checksum(PurePosixPath("a b.txt"), hide=True) + + cmd = fake.run.call_args.args[0] + expected_path = PurePosixPath("/data/my files") / "a b.txt" + assert cmd == f"md5sum {shlex.quote(str(expected_path))}" + assert checksum == "abc123" + + +def test_ensure_remote_dir_quotes_path(ssh_env): + connector, fake = _connector_with_mock_run(ssh_env, directory="/data") + remote_dir = PurePosixPath("/data/new dir/sub") + + connector._ensure_remote_dir(remote_dir) + + cmd = fake.run.call_args.args[0] + assert cmd == f"mkdir -p {shlex.quote(str(remote_dir))}" From e9e53d15dc1ae998a4a0ffa9d87bbfadb4f951d2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 09:27:52 +0000 Subject: [PATCH 2/5] docs: note passing tests in issue #18 status Co-authored-by: Jan Petter Maehlen --- .issueflows/01-current-issues/issue18_status.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.issueflows/01-current-issues/issue18_status.md b/.issueflows/01-current-issues/issue18_status.md index f8df701..83c7478 100644 --- a/.issueflows/01-current-issues/issue18_status.md +++ b/.issueflows/01-current-issues/issue18_status.md @@ -5,7 +5,12 @@ ## 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 -- Quote helper + SSH shell call sites; unit + gated SSH space-path tests; mark SEC-01 done in design docs. +- `/iflow-close` (Done checkbox, archive, PR merge). From e1c0b557c64931f9db112b25c53705fecb3ecc35 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 15:33:00 +0000 Subject: [PATCH 3/5] =?UTF-8?q?chore:=20close=20issue=20#18=20=E2=80=94=20?= =?UTF-8?q?mark=20done=20and=20archive=20tracking=20files?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move issue #18 original/plan/status into 03-solved-issues after SEC-01 shell-quoting fix landed and tests passed. Co-authored-by: Jan Petter Maehlen --- .../issue18_original.md | 0 .../{01-current-issues => 03-solved-issues}/issue18_plan.md | 0 .../{01-current-issues => 03-solved-issues}/issue18_status.md | 4 ++-- 3 files changed, 2 insertions(+), 2 deletions(-) rename .issueflows/{01-current-issues => 03-solved-issues}/issue18_original.md (100%) rename .issueflows/{01-current-issues => 03-solved-issues}/issue18_plan.md (100%) rename .issueflows/{01-current-issues => 03-solved-issues}/issue18_status.md (88%) diff --git a/.issueflows/01-current-issues/issue18_original.md b/.issueflows/03-solved-issues/issue18_original.md similarity index 100% rename from .issueflows/01-current-issues/issue18_original.md rename to .issueflows/03-solved-issues/issue18_original.md diff --git a/.issueflows/01-current-issues/issue18_plan.md b/.issueflows/03-solved-issues/issue18_plan.md similarity index 100% rename from .issueflows/01-current-issues/issue18_plan.md rename to .issueflows/03-solved-issues/issue18_plan.md diff --git a/.issueflows/01-current-issues/issue18_status.md b/.issueflows/03-solved-issues/issue18_status.md similarity index 88% rename from .issueflows/01-current-issues/issue18_status.md rename to .issueflows/03-solved-issues/issue18_status.md index 83c7478..9d28e33 100644 --- a/.issueflows/01-current-issues/issue18_status.md +++ b/.issueflows/03-solved-issues/issue18_status.md @@ -1,6 +1,6 @@ # Status: Issue #18 -- [ ] Done +- [x] Done ## What's done @@ -13,4 +13,4 @@ ## Remaining work -- `/iflow-close` (Done checkbox, archive, PR merge). +- None. From d18e1fc9160fae3aaf816a00177adfde48497e89 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 15:37:02 +0000 Subject: [PATCH 4/5] fix: pass in_stream=False to SSH checksum Fabric run calculate_checksum was the only production shell call missing in_stream=False, which breaks pytest-captured SSH CI when the new space-in-path integration test exercises md5sum. Co-authored-by: Jan Petter Maehlen --- oeleo/connectors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oeleo/connectors.py b/oeleo/connectors.py index 1d3542d..38f009d 100644 --- a/oeleo/connectors.py +++ b/oeleo/connectors.py @@ -327,7 +327,7 @@ def calculate_checksum(self, f, hide=True): self.connect() cmd = f"md5sum {self._remote_shell_token(self.directory / f)}" - result = self.c.run(cmd, hide=hide) + 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] From 4219a0e065f3f70ac790045410f3db091821ea43 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 15:43:54 +0000 Subject: [PATCH 5/5] docs: drop leftover conflict markers from #17/#18 merge Co-authored-by: Jan Petter Maehlen --- .../code-review-improvement-backlog.md | 7 +------ .issueflows/04-designs-and-guides/code-review-overview.md | 5 ----- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md b/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md index a9779d7..45a286a 100644 --- a/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md +++ b/.issueflows/04-designs-and-guides/code-review-improvement-backlog.md @@ -103,13 +103,8 @@ Filed on GitHub (2026-07-16). Two parallel tracks (separate branches/PRs): 1. [#14](https://github.com/ife-bat/oeleo/issues/14) BUG-03 — yolo 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 -<<<<<<< HEAD -4. [#17](https://github.com/ife-bat/oeleo/issues/17) REL-01 — plan + confirm -5. ~~[#18](https://github.com/ife-bat/oeleo/issues/18) SEC-01~~ — done -======= 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 ->>>>>>> origin/main +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. diff --git a/.issueflows/04-designs-and-guides/code-review-overview.md b/.issueflows/04-designs-and-guides/code-review-overview.md index 32720ec..3ea4b86 100644 --- a/.issueflows/04-designs-and-guides/code-review-overview.md +++ b/.issueflows/04-designs-and-guides/code-review-overview.md @@ -68,13 +68,8 @@ 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 | -<<<<<<< HEAD | ~~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** (SSH thrashing) | correctness | -======= -| SEC-01 | SSH `find` / `md5sum` / `mkdir` built via string interpolation (shell injection risk) | security | | ~~REL-01~~ | ~~`Worker.reconnect=True` reconnects **before every file**~~ — **resolved in #17** (`reconnect=False` default; opt-in via kwarg / `OELEO_RECONNECT`) | correctness | ->>>>>>> origin/main ### Medium