|
1 | 1 | # Changelog |
2 | 2 |
|
3 | | -## Unreleased — Audit & Test Infrastructure |
| 3 | +## Unreleased — Audit, Bug Fixes, Test Infrastructure |
4 | 4 |
|
5 | | -A multi-tool security/correctness audit was run (ruff, mypy, bandit, pylint, |
6 | | -vulture, pyflakes), every medium+ finding was fixed, and a pytest suite was |
| 5 | +A multi-tool security/correctness audit (ruff, mypy, bandit, pylint, |
| 6 | +vulture, pyflakes) was run across the codebase. Every medium+ finding was |
| 7 | +either fixed or annotated with rationale, and a 557-test pytest suite was |
7 | 8 | built up alongside CI to lock in those fixes. |
8 | 9 |
|
9 | | -### Bug fixes (high impact) |
10 | | - |
11 | | -- **`utils/api.py`** — `delete_permanently()` built `data={'items':[…]}` but |
12 | | - the inner `delete()` method never accepted a body, so the items list was |
13 | | - silently dropped on the wire. Backup-file purges after `safety_pattern` |
14 | | - uploads were no-ops, leaking trash storage. **Fixed:** extended `delete()` |
15 | | - to accept and forward `data=`, and updated `delete_permanently()` to |
16 | | - pass it through. |
17 | | -- **`__init__.py`** — file was missing the opening `"""` of its docstring, |
18 | | - so importing the package raised `SyntaxError`. **Fixed.** |
19 | | -- **`config/config.py`** — `'NETWORK_URL'` was defined twice in the config |
20 | | - dict; the second definition (`https://api.internxt.com`) silently |
21 | | - overrode the first. The dead key has been removed; the active value is |
22 | | - unchanged. |
23 | | -- **`services/drive.py`** — `create_folder_recursive` was defined twice |
24 | | - (104 lines of dead code). **Fixed:** removed the dead first definition. |
25 | | -- **`utils/api.py`** — `move_file` and `move_folder` were each defined twice |
26 | | - (the second silently overriding the first). **Fixed:** removed the |
27 | | - duplicates. |
28 | | -- **`services/webdav_server.py`** — `start()` referenced `active_server` |
29 | | - unbound when `server_choice` was anything other than |
30 | | - `auto`/`waitress`/`cheroot`, raising `UnboundLocalError`. **Fixed:** |
31 | | - initialize upfront and raise an explicit `ValueError` for unknown choices. |
32 | | -- **`services/webdav_server.py`** — `start()` hardcoded `host="0.0.0.0"`, |
33 | | - ignoring the `host` setting in webdav config (which itself was being |
34 | | - silently dropped by `read_webdav_config`). **Fixed:** default to |
35 | | - `127.0.0.1` (loopback only) and pass through the user-set `host` from |
36 | | - config so `0.0.0.0` is opt-in. |
37 | | -- **`services/webdav_provider.py`** — the `finally:` clause in `end_write` |
38 | | - called `self._upload_buffer.cleanup()` without checking `hasattr`, so an |
39 | | - exception before `begin_write` had set the buffer would crash with |
40 | | - `AttributeError` masking the real error. **Fixed.** |
41 | | -- **`services/crypto.py`** — `generate_filename_encryption_iv` called |
42 | | - `hmac.new(key, hashlib.sha512)` — `hashlib.sha512` was being passed as |
43 | | - `msg` instead of `digestmod`. The function would `TypeError` at |
44 | | - runtime. (Currently dead code, but a real latent bug.) **Fixed.** |
45 | | -- **`services/webdav_provider.py`** — `set_property()` (the PROPPATCH |
46 | | - handler used by macOS Finder & Windows Explorer to set folder |
47 | | - timestamps) imported two functions from `wsgidav.util` that don't exist |
48 | | - in any released wsgidav version (`rfc_1123_to_timestamp`, |
49 | | - `rfc_3339_to_timestamp`). Any timestamp PROPPATCH would crash with |
50 | | - `ImportError`. **Fixed:** use `parse_time_string` for RFC 1123 with a |
51 | | - stdlib `datetime.fromisoformat` fallback for ISO 8601 / RFC 3339. |
52 | | -- **`services/webdav_provider.py` / `services/drive.py`** — `set_property()` |
53 | | - called `drive_service.set_folder_timestamps(...)`, but that method was |
54 | | - never defined. **Fixed:** added the method, which calls |
55 | | - `api.update_folder_metadata` and invalidates the parent-folder cache. |
56 | | -- **`cli.py config` command** — referenced `DRIVE_WEB_URL` which had been |
57 | | - commented out of the config dict. The `config` command crashed with |
58 | | - "Config key DRIVE_WEB_URL was not found in process.env" on every run. |
59 | | - **Fixed:** tolerate missing keys with `(not configured)` placeholder. |
60 | | -- **`install.py`** — used `subprocess.run(..., shell=True)` with a |
61 | | - string-concatenated `pip install` command. **Fixed:** switched to argv |
62 | | - form using `sys.executable`. |
63 | | - |
64 | | -### Security hardening (Bandit medium+ findings) |
65 | | - |
66 | | -All Bandit medium+ findings (was 14, now 0) are resolved or annotated: |
67 | | - |
68 | | -- **`services/network_utils.py:test_webdav_connection`** — `verify=False` |
69 | | - was unconditional. Restricted to localhost loopback; remote URLs now |
70 | | - use real TLS verification. |
71 | | -- **`services/webdav_server.py`** — multiple `0.0.0.0` binds replaced |
72 | | - with the configurable `host` from webdav config (defaults to loopback). |
73 | | -- **`services/crypto.py:206`** (SHA1 in PBKDF2) and |
74 | | - **`services/crypto.py:257`** (MD5 in OpenSSL EVP-style key derivation) |
75 | | - — both required for protocol compatibility with the Internxt server. |
76 | | - Annotated with `# nosec` and rationale comments; MD5 marked |
77 | | - `usedforsecurity=False`. |
78 | | -- **`services/drive.py:739`** — MD5 used as a non-cryptographic |
79 | | - cache-key hash. Switched to SHA-256. |
80 | | - |
81 | | -### Hygiene |
82 | | - |
83 | | -- All bare `except:` clauses replaced with `except Exception:` or |
84 | | - specific exception types. |
| 10 | +End state: **0 ruff errors, 0 mypy errors, 0 Bandit medium+ findings, |
| 11 | +557/557 pytest passing in ~3 seconds, 90% line coverage**. |
| 12 | + |
| 13 | +--- |
| 14 | + |
| 15 | +### Bug fixes |
| 16 | + |
| 17 | +#### Critical |
| 18 | + |
| 19 | +1. **`utils/api.py` — silent data loss in `delete_permanently()`** |
| 20 | + The function built `data={'items': [{'uuid': ..., 'type': ...}]}` but |
| 21 | + the inner `delete()` method had no `data` parameter, so the items list |
| 22 | + was silently dropped on the wire. Backup-file purges after the |
| 23 | + `safety_pattern` upload (called whenever an existing file is replaced) |
| 24 | + were no-ops, leaking trash storage indefinitely. |
| 25 | + **Fixed:** extended `delete()` to accept and forward `data=`, and |
| 26 | + updated `delete_permanently()` to pass it through. |
| 27 | + |
| 28 | +2. **`__init__.py` — package wouldn't import** |
| 29 | + File started with text but had no opening `"""` for its docstring, so |
| 30 | + `import internxt_cli` raised `SyntaxError`. |
| 31 | + **Fixed:** added the missing triple-quote. |
| 32 | + |
| 33 | +3. **`services/webdav_provider.py:set_property` — every PROPPATCH crashed** |
| 34 | + This is the handler macOS Finder and Windows Explorer use to set |
| 35 | + creation/modification times on remote folders. Two broken pieces: |
| 36 | + - It imported `rfc_1123_to_timestamp` and `rfc_3339_to_timestamp` from |
| 37 | + `wsgidav.util`. Neither has ever existed in any released wsgidav |
| 38 | + version. Every PROPPATCH crashed with `ImportError`. |
| 39 | + - It then called `drive_service.set_folder_timestamps(...)`, which was |
| 40 | + never defined on `DriveService`. Even if the imports had worked, the |
| 41 | + call would `AttributeError`. |
| 42 | + |
| 43 | + Both were caught by the outer `try/except` and surfaced as silent |
| 44 | + HTTP 403 responses to the client. |
| 45 | + **Fixed:** replaced the imports with `parse_time_string` (handles |
| 46 | + RFC 1123) plus a stdlib `datetime.fromisoformat` fallback for |
| 47 | + ISO 8601 / RFC 3339; added the missing `set_folder_timestamps` method, |
| 48 | + which calls `api.update_folder_metadata` and invalidates the parent |
| 49 | + cache. |
| 50 | + |
| 51 | +#### High |
| 52 | + |
| 53 | +4. **`services/webdav_server.py:start()` — `UnboundLocalError` on bad input** |
| 54 | + `active_server` was only assigned inside `if/elif/elif` branches for |
| 55 | + `auto`/`waitress`/`cheroot`. Any other value made the subsequent |
| 56 | + `if active_server is None:` check raise `UnboundLocalError`. |
| 57 | + **Fixed:** initialise `active_server: Optional[str] = None` upfront and |
| 58 | + raise an explicit `ValueError` for unknown choices. |
| 59 | + |
| 60 | +5. **`services/webdav_server.py:start()` — bound to `0.0.0.0` ignoring config** |
| 61 | + The `host` was hardcoded to `"0.0.0.0"`, exposing the WebDAV server |
| 62 | + on every interface regardless of the `host` setting in the user's |
| 63 | + webdav config. Compounded by `read_webdav_config` silently dropping |
| 64 | + the `host` key during deserialization. |
| 65 | + **Fixed:** default to `127.0.0.1` (loopback only); pass user-set |
| 66 | + `host` from config through `read_webdav_config` so `0.0.0.0` is |
| 67 | + explicit opt-in. |
| 68 | + |
| 69 | +6. **`services/webdav_provider.py:end_write` — `AttributeError` masked real errors** |
| 70 | + The `finally:` clause called `self._upload_buffer.cleanup()` without |
| 71 | + checking whether `_upload_buffer` had ever been assigned. Any |
| 72 | + exception before `begin_write` ran would crash with |
| 73 | + `AttributeError: 'InternxtDAVResource' object has no attribute |
| 74 | + '_upload_buffer'` and bury the real error. |
| 75 | + **Fixed:** initialise to `None` in `__init__`; guard the cleanup with |
| 76 | + `if self._upload_buffer is not None`. |
| 77 | + |
| 78 | +7. **`services/crypto.py:generate_filename_encryption_iv` — `TypeError` at runtime** |
| 79 | + `hmac.new(bytes.fromhex(bucket_key), hashlib.sha512)` passes |
| 80 | + `hashlib.sha512` as the `msg` argument instead of `digestmod`, so the |
| 81 | + call raises `TypeError("Missing required argument 'digestmod'")` at |
| 82 | + runtime. (Currently dead code — `encrypt_filename` is not yet called |
| 83 | + anywhere — but a real latent bug that would fire as soon as someone |
| 84 | + wired it up.) |
| 85 | + **Fixed:** `hmac.new(..., digestmod=hashlib.sha512)`. |
| 86 | + |
| 87 | +8. **`config/config.py` — duplicate `NETWORK_URL` key** |
| 88 | + `'NETWORK_URL'` was defined twice in the `self.config` dict literal. |
| 89 | + The second definition (`https://api.internxt.com`) silently overrode |
| 90 | + the first (`https://gateway.internxt.com/network`). The runtime |
| 91 | + behaviour was correct (the active value is the live network base URL) |
| 92 | + but the duplicate was an obvious code smell hiding a stale value. |
| 93 | + **Fixed:** removed the dead duplicate. |
| 94 | + |
| 95 | +9. **`services/drive.py` and `utils/api.py` — duplicate function definitions** |
| 96 | + - `create_folder_recursive` was defined twice in `drive.py` (104 |
| 97 | + lines of dead code in the first definition). |
| 98 | + - `move_file` and `move_folder` were each defined twice in `api.py`. |
| 99 | + In all cases the later definition silently overrode the earlier one. |
| 100 | + **Fixed:** removed the dead first definitions. |
| 101 | + |
| 102 | +10. **`cli.py config` — command crashed on every invocation** |
| 103 | + Referenced `DRIVE_WEB_URL` which has been commented out of the config |
| 104 | + dict for an unknown amount of time. Every `python cli.py config` |
| 105 | + aborted with "Config key DRIVE_WEB_URL was not found in process.env" |
| 106 | + after only printing the section header. |
| 107 | + **Fixed:** tolerate missing keys with a `(not configured)` placeholder |
| 108 | + so the rest of the command runs. |
| 109 | + |
| 110 | +11. **`install.py` — `shell=True` subprocess injection (Bandit B602)** |
| 111 | + `subprocess.run("pip install '" + dep + "'", shell=True)` would have |
| 112 | + accepted shell metacharacters from the dependency name. |
| 113 | + **Fixed:** switched to argv form using `sys.executable`. |
| 114 | + |
| 115 | +#### Medium (security hardening, all Bandit medium+ findings cleared) |
| 116 | + |
| 117 | +12. **`services/network_utils.py:test_webdav_connection` — unconditional `verify=False`** |
| 118 | + TLS verification was disabled for *all* requests, including remote |
| 119 | + hosts. |
| 120 | + **Fixed:** restricted `verify=False` to localhost loopback addresses |
| 121 | + only; remote URLs now use real TLS verification. |
| 122 | + |
| 123 | +13. **`services/webdav_server.py` — multiple `0.0.0.0` binds (Bandit B104)** |
| 124 | + Three call sites hardcoded binding to all interfaces. |
| 125 | + **Fixed:** all replaced with the configurable `host` from webdav |
| 126 | + config, defaulting to loopback. |
| 127 | + |
| 128 | +14. **`services/crypto.py:206` (SHA1 in PBKDF2) and `:257` (MD5 in EVP)** |
| 129 | + Both are required for protocol compatibility with the Internxt |
| 130 | + server (PBKDF2-HMAC-SHA1 password hash format; OpenSSL |
| 131 | + `EVP_BytesToKey` with MD5 for credential file encryption). Cannot be |
| 132 | + changed without backend cooperation. |
| 133 | + **Annotated:** `# nosec` with rationale comments; MD5 marked |
| 134 | + `usedforsecurity=False`. |
| 135 | + |
| 136 | +15. **`services/drive.py:739` — MD5 used as cache-key hash** |
| 137 | + Non-cryptographic use (just deriving a stable filename for resumable |
| 138 | + upload checkpoints). |
| 139 | + **Fixed:** switched to SHA-256 (truncated to 32 hex chars). |
| 140 | + |
| 141 | +#### Hygiene |
| 142 | + |
| 143 | +- All bare `except:` clauses (8 of them) replaced with `except Exception:` |
| 144 | + or specific types. |
85 | 145 | - All unused imports removed (cli.py, services/*, utils/api.py, debug/*). |
86 | | -- All ~180 empty f-strings (`f"static text"`) replaced with regular strings. |
87 | | -- `raise ... from e` chains added where pylint suggested (proper |
88 | | - exception causality preservation). |
| 146 | +- All ~180 empty f-strings (`f"static text"`) replaced with plain strings. |
| 147 | +- `raise … from e` chains added where pylint suggested (proper exception |
| 148 | + causality preservation). |
89 | 149 | - mypy implicit-Optional defaults annotated explicitly. |
90 | | -- `cli.py test` smoke command's two stale URL assertions updated to |
91 | | - match the actual configured endpoints. |
| 150 | +- `cli.py test` smoke command's two stale URL assertions updated to match |
| 151 | + the actual configured endpoints (was 5/7 passing, now 7/7). |
| 152 | + |
| 153 | +--- |
92 | 154 |
|
93 | 155 | ### Test infrastructure (new) |
94 | 156 |
|
95 | | -- **`tests/`** — 403 pytest tests covering crypto round-trips, path |
96 | | - resolution, upload/download conflict handling (skip/overwrite/safety |
97 | | - pattern), WebDAV provider (resource + collection + isolated session), |
98 | | - WebDAV server lifecycle, all major CLI commands (Click `CliRunner`), |
99 | | - SSL cert lifecycle, range parsing, credential persistence, batch `mv` |
100 | | - with wildcards, memory-gated upload concurrency. **71% line coverage** |
101 | | - across the codebase. |
102 | | -- **End-to-end cycle test** (`test_upload_download_e2e.py`) — writes a |
103 | | - real local file, runs `upload_file_to_folder` (real crypto, mocked |
104 | | - network), captures the encrypted bytes "on the wire", then runs |
105 | | - `download_file` against them and asserts byte-for-byte recovery. This |
106 | | - catches any drift in the Internxt encryption protocol. |
| 157 | +#### Suite |
| 158 | + |
| 159 | +**557 pytest tests** across 33 test files in `tests/`, covering: |
| 160 | + |
| 161 | +- **Crypto** (`test_crypto.py`, `test_crypto_filename_meta.py`) — AES-256-CTR |
| 162 | + file round-trip across 0 B – 1 MB sizes, AES-256-GCM metadata cipher, |
| 163 | + PBKDF2 determinism, bucket-key derivation, filename encryption |
| 164 | + protocol round-trip + deterministic IV + cross-bucket isolation, |
| 165 | + decrypt-with-wrong-key returns None. |
| 166 | +- **Auth** (`test_auth_login.py`, `test_auth_refresh.py`) — full hydrated |
| 167 | + login flow with mocked API, email lowercasing, missing-sKey error, |
| 168 | + token rotation with bridgeAuth recompute, persistence on success and |
| 169 | + not on failure. |
| 170 | +- **Drive service** (`test_drive_*.py`) — path resolution (10 cases incl. |
| 171 | + legacy `name` field, file-vs-folder ambiguity), recursive folder |
| 172 | + creation with race-on-conflict fallback, content cache TTL, trash/ |
| 173 | + delete dispatch, pagination with multi-page recursion, upload conflict |
| 174 | + state machine (skip/overwrite/safety-pattern with rollback), copy |
| 175 | + with timestamp preservation, memory-gated concurrency with **real |
| 176 | + threading test** for blocking + wakeup, validate_upload_sources, |
| 177 | + get_upload_statistics across recursive/non-recursive/empty. |
| 178 | +- **WebDAV provider** (`test_webdav_*.py`) — resource metadata accessors, |
| 179 | + StreamingFileUpload memory↔disk hybrid, end_write upload-on-close |
| 180 | + cycle (small + large + update-existing + pending-uuid), get_content |
| 181 | + download path with real crypto round-trip, set_property PROPPATCH |
| 182 | + timestamps (RFC 1123 + RFC 3339 + Win32 namespace), thread-local |
| 183 | + isolated sessions, mark_deleted bookkeeping. |
| 184 | +- **WebDAV server** (`test_webdav_server*.py`) — start() server-choice |
| 185 | + routing (auto/waitress/cheroot/invalid), HTTPS-with-waitress fallback, |
| 186 | + HTTPS-with-cheroot SSL adapter setup, missing-cert auto-generation, |
| 187 | + thread runner (waitress + cheroot branches, KeyboardInterrupt caught, |
| 188 | + silent shutdown), stop() (foreground + background via psutil), status |
| 189 | + (foreground + background + stale-pid cleanup), test_connection |
| 190 | + PROPFIND probe, mount instructions for all platforms, |
| 191 | + _check_port_available with real socket. |
| 192 | +- **API client** (`test_api_*.py`) — every endpoint URL+method+payload |
| 193 | + pinned (folders pagination, create_folder validation, metadata GET/PUT, |
| 194 | + ancestors list-or-empty, trash bulk, network upload v2, search, login |
| 195 | + with email lowering); `_make_request` json-vs-data routing, Bearer |
| 196 | + strip when basic auth, HTTPError → ValueError, timeout → ConnectionError; |
| 197 | + `robust_request` 401-rotate-and-retry with refreshed token; raw |
| 198 | + upload_chunk/download_chunk via direct `requests.put`/`get`. |
| 199 | +- **Config persistence** (`test_credentials_persistence.py`) — encrypted |
| 200 | + credentials round-trip on disk, clear-when-empty error semantics, |
| 201 | + webdav config + pid file lifecycle. |
| 202 | +- **Network utils** (`test_network_utils.py`, `test_ssl_lifecycle.py`) — |
| 203 | + Range header parsing (7 cases), self-signed cert generate→save→get→ |
| 204 | + validate cycle, `0o600` private-key permissions, regen-on-garbage-cert, |
| 205 | + loopback-only `verify=False` regression. |
| 206 | +- **CLI commands** (`test_cli_*.py`, `test_mv_command.py`) — Click |
| 207 | + `CliRunner` against `whoami`, `logout`, `login`, `trash-path`, |
| 208 | + `delete-path` (force vs. confirmation prompts), `list-path`, `mkdir`, |
| 209 | + `search`, `find` (-name + -iname), `resolve`, `tree`, `upload` (no-source |
| 210 | + / target-creation / target-is-file rejection), `download-path` (folder- |
| 211 | + without-r / missing / filter-skipped), `webdav-stop/status/mount`, |
| 212 | + `config`, `mv` (single + glob expansion + conflict skip/overwrite + |
| 213 | + dry-run + already-in-target + parallel error propagation). |
| 214 | + |
| 215 | +#### Headline test |
| 216 | + |
| 217 | +`test_upload_download_e2e.py` — writes a 19 KB file locally, runs |
| 218 | +`upload_file_to_folder` (real crypto, mocked network), captures the |
| 219 | +encrypted bytes that would have hit the upload URL, then runs |
| 220 | +`download_file` against those bytes and asserts byte-for-byte recovery. |
| 221 | +This catches any drift in the Internxt encryption protocol that unit |
| 222 | +mocks would miss. |
| 223 | + |
| 224 | +#### CI gates |
| 225 | + |
107 | 226 | - **`pyproject.toml`** — pytest, coverage, ruff config. |
108 | | -- **`requirements-dev.txt`** — pinned floor for pytest, pytest-cov, ruff, |
109 | | - mypy, bandit. |
110 | | -- **`.github/workflows/ci.yml`** — runs ruff → mypy → bandit (medium+) → |
111 | | - pytest with coverage on Python 3.10 / 3.11 / 3.12. |
| 227 | +- **`requirements-dev.txt`** — pinned floor for pytest, pytest-cov, |
| 228 | + ruff, mypy, bandit. |
| 229 | +- **`.github/workflows/ci.yml`** — runs **ruff → mypy → bandit |
| 230 | + (medium+) → pytest+coverage** on Python 3.10 / 3.11 / 3.12. Coverage |
| 231 | + XML uploaded as an artifact on Python 3.11. |
112 | 232 |
|
113 | | -### Test coverage by module |
| 233 | +#### Coverage by module |
114 | 234 |
|
115 | 235 | | Module | Coverage | |
116 | 236 | |---------------------------------|---------:| |
| 237 | +| `config/__init__.py` | 100% | |
117 | 238 | | `config/config.py` | 85% | |
| 239 | +| `services/__init__.py` | 100% | |
118 | 240 | | `services/auth.py` | 100% | |
119 | 241 | | `services/crypto.py` | 100% | |
120 | 242 | | `services/drive.py` | 89% | |
121 | 243 | | `services/network_utils.py` | 90% | |
122 | 244 | | `services/webdav_provider.py` | 91% | |
123 | 245 | | `services/webdav_server.py` | 83% | |
| 246 | +| `utils/__init__.py` | 100% | |
124 | 247 | | `utils/api.py` | 98% | |
125 | 248 | | **Total** | **90%** | |
126 | 249 |
|
127 | | -(Total tests: **557** passing in ~3 seconds.) |
| 250 | +The trust roots (auth + crypto) are at 100%. The remaining gaps in |
| 251 | +`webdav_server.py` and `drive.py` are platform-specific fallback paths |
| 252 | +(`_available_memory` for non-darwin/win32 systems) and `print()`-heavy |
| 253 | +diagnostic branches that are best exercised by integration tests against |
| 254 | +a live Internxt backend, not more unit mocks. |
| 255 | + |
| 256 | +#### Not yet covered (intentional) |
| 257 | + |
| 258 | +- A vcrpy-recorded contract test against the real Internxt API for one |
| 259 | + full upload→download cycle. Needs test credentials and a one-time |
| 260 | + recording session; would lock in the exact wire-format expectations of |
| 261 | + the production backend so future API changes fail the test in CI |
| 262 | + rather than in the field. |
0 commit comments