fix(#1036): verify a resumed download was honoured (206 + Content-Range) before appending

CLAUDE.md

@@ -413,8 +413,8 @@ Video/webcam -> facial morph + head + skeletal body animation, built on the exis
 
 - **LLMManager** (`src/LLMManager.h/cpp`): QML_SINGLETON wrapping llama.cpp for local inference.
 - **LLMWorker**: Runs inference in a worker thread.
+- **ModelDownloader**: Downloads GGUF/ONNX models from HuggingFace — the ONE download path behind all 21 model consumers. **#1029 (CWE-494) hardening:** `startDownload` **refuses any URL that is not `https://<host>` or a host-less `file://`** (plain http is a byte-for-byte MITM injection point, and every base URL is user-overridable via env/QSettings; `file://<host>/share/…` is refused too — on Windows that is a UNC/SMB fetch over the network wearing a local scheme; only an empty or `localhost` host is accepted, and the error names the reason) — refused BEFORE any filesystem side effect, so no `.part` or directory is created. An optional 4th argument `expectedSha256` (case-insensitive hex; empty = legacy no-check, so the 21 consumers + the two QML call sites in `AISettingsDialog.qml` work unchanged) is verified by a local streamed `QCryptographicHash` helper **on the finished `.part` on disk, before the rename** (deliberately NOT the updater's `UpdateVerifier::sha256HexOfFile`: `qtmesh_updater` is only built/linked under `ENABLE_AUTO_UPDATER`, while `ModelDownloader` compiles unconditionally — reusing it broke the `-DENABLE_AUTO_UPDATER=OFF` link, caught in review; six lines of Qt API beat coupling every model download to the optional updater + libsodium) — never as a running hash in `onReadyRead`, because a resumed download appends to bytes this process never saw. On mismatch the `.part` is **deleted** (a poisoned partial must not be resumed from or cached) and `downloadError` fires. **Two gotchas verified empirically:** (1) the scheme refusal is emitted **QUEUED** (`QMetaObject::invokeMethod` + `Qt::QueuedConnection`), never synchronously — consumers connect, call `startDownload`, then `loop.exec()`, so a synchronous `loop.quit()` fires before `exec()` and is lost, hanging them for their full timeout (the #1017 review race); queued delivery lands inside `exec()` for every consumer without touching any — proven on `PhotoDepth`, which has no `settled` guard, failing in 1s instead of 600s. (2) Both refusals log at **`qCritical`**, not `qWarning`: `cliMessageHandler` drops warnings unless `--verbose`, and every consumer discards `downloadError`'s text (#1037), so a warning would leave the user with "offline?" and no way to learn the cause. **Resume verification (#1036):** a resume sends `Range: bytes=N-`, but a server that ignores it (`file://` always; any proxy that strips the header) answers **200 with the whole body**, and appending that after the stale `.part` produced a corrupt model — reproduced: a 29-byte stale prefix yielded a 208,044,845-byte file that **still loaded and ran** (ORT parsed the garbage as an unknown protobuf field) while a 30-byte prefix failed with "Protobuf parsing failed"; load success proves nothing about integrity. So on the FIRST `readyRead` of a resumed request (`m_resumeUnverified`, armed at both Range-request sites) the downloader requires **206 + a `Content-Range` starting exactly at the resume offset**; anything else is treated as the full body — the `.part` is reopened `Truncate`, `m_resumeOffset`/`m_bytesReceived` reset to 0 (else progress adds a phantom offset), and the download continues from byte 0 with no error (a 200 is recoverable). The range unit is compared **case-insensitively** (RFC 9110 §14.1 — `Bytes 9-12/13` is a valid honoured resume; a case-sensitive match misread it as "ignored" and fell into the truncate path with a PARTIAL body, i.e. an incomplete file a no-digest caller would rename — caught in review). A 206 whose window is neither ours nor the whole resource is a genuinely partial body we did not ask for: writing it from byte 0 would yield an INCOMPLETE file, so that case **aborts** with `downloadError` and removes the `.part` (next attempt starts clean); only a 206 covering exactly `0..total-1` — a full body wearing a partial status — is truncated-and-taken like a 200. Checked once, not per chunk. `FakeNetworkReply` in the tests defaults to a plain 200 (what an ignoring server returns) and `withPartialContent(first,last,total)` models an honoured resume. Both guards mutation-verified with correct selectivity. `ModelDownloader::isAllowedDownloadUrl(url)` is the pure predicate for checking a base URL up front. Tests: `ModelDownloader_test.cpp` (both guards mutation-verified). **Review round on the resume fix (#1039):** (1) *honoured* now also requires the window to REACH THE END — `first == offset && last == total-1` — because `bytes 9-10/13` starts right yet leaves 11-12 missing, and appending it would have promoted an incomplete file (that finding had been marked addressed by the bot without the code changing; verify against the code, not the bot's annotation); (2) every 'this partial is unusable' exit goes through ONE path, `discardPartialAndFail` — remove-or-truncate the `.part`, reset `m_bytesReceived`/`m_resumeOffset` (a stale offset would make `resumeDownload` re-request the old range against a fresh file), abort the reply under `m_abortingInternally` so the synchronously-delivered `errorOccurred`/`finished` step aside, end the download, emit exactly ONE `downloadError` (the test's `FakeNetworkReply::signalOnAbort` models the synchronous delivery); (3) `onDownloadFinished` refuses to promote when `m_resumeUnverified` is still set (a resume reply that finished without ever delivering data — verification never ran) or when the `.part` size differs from the size the response committed to (`m_expectedTotalBytes`: the 206 total, else a full body's Content-Length) — the `.part` is KEPT as a valid prefix for the next resume, not discarded like a digest mismatch. A server that declares no size, with no digest configured, is accepted with a `qWarning` (chunked transfer; HF/GitHub and QNAM's `file://` backend always send Content-Length, so real downloads take the strict path).
 - **ModelFetch** (`src/ModelFetch.{h,cpp}`, #1037): the ONE blocking "make sure this model file is on disk" primitive — `ModelFetch::ensureBlocking(Request{url,destination,label,timeoutMs,expectedSha256}) -> Outcome{ok,timedOut,path,error}`. Twenty consumers used to hand-roll the same nested-QEventLoop wait around `ModelDownloader`; the copies drifted: 18 DISCARDED the downloader's error text (so "refusing http://" / "SHA-256 mismatch" reached the user as "unavailable (offline?)"), and only 3 (`TextureInpaint`, `FaceRig/ArkitTemplate`, `FaceRig/FaceLandmarkDetector`) guarded the **synchronous-rejection race** (`startDownload` emits `downloadError` synchronously when busy → the handler's `loop.quit()` fires before `exec()` and is lost → the caller hangs for its full timeout). `ensureBlocking` owns only the wait and returns the downloader's own words; consumers keep what genuinely varies (base-URL env/QSettings/default resolution, the `*_NO_DOWNLOAD` guard, the timeout). **Convention for exposing the reason:** `ensureModelBlocking(QString* error = nullptr)` — done for `PhotoDepth` and `TextureInpaint` (their 4 CLI/MCP sites now print e.g. `…unavailable: Refusing to download …: scheme 'http' is not https://`, proven e2e); the other migrated consumers keep their signatures (no message site to enrich yet). Migrated (behaviour-preserving): every consumer with a blocking wait — `PhotoDepth`, `TextureInpaint` (with the `error` out-param), `AIAssistManager`, `ImageTo3D/ImageCaptioner`, `ImageTo3D/MeshGenPredictor`, `ImageTo3D/TripoSGPredictor`, `ImageTo3D/BackgroundRemover`, `MeshSegmenter`, `MotionInbetween`, `MotionGenerator`, `SkinTokensPredictor`, `UniRigPredictor`, `FaceRig/ArkitTemplate`, `FaceRig/FaceLandmarkDetector`, `Mocap/FaceCapPredictor`, `Mocap/PoseCapPredictor`, `Mocap/HandCapPredictor`. The two FaceRig consumers had their own `done` race guard — now `ModelFetch`'s `settled`; their `(timeout)` breadcrumb annotation comes from `Outcome::timedOut`. `MotionGenerator`'s old `guard` timer never cancelled the transfer on timeout; `ModelFetch` does. **Deliberately NOT migrated:** `MotionLibrary::ensureLibraryBlocking` (V1→V2 upgrade logic where a failed download must fall back to the local V1 file — `haveLocal ? dest : QString()` — not the canonical shape; the only hand-rolled **`ModelDownloader`** wait left — other `QEventLoop`s in the tree belong to the cloud client, HDR downloads and the updater, which have their own network paths). **Gotcha from this migration:** at least one source file is not valid UTF-8 — a Python `open(p).read()` sweep over `src/` raises `UnicodeDecodeError`; use `encoding='utf-8', errors='surrogateescape'` for read AND write so bytes round-trip exactly. `AIModelCatalog`/`LLMSettingsWidget` are GUI-async (no event loop) and already show the error text. Tests: `ModelFetch_test.cpp` drives the REAL singleton through QNAM's `file://` backend (existing-file short-circuit, real fetch, refusal text verbatim, missing-file network error, synchronous rejection returns in <1 s not after the timeout); the race guard and error capture are mutation-verified.
-- **ModelDownloader**: Downloads GGUF/ONNX models from HuggingFace — the ONE download path behind all 21 model consumers. **#1029 (CWE-494) hardening:** `startDownload` **refuses any URL that is not `https://<host>` or a host-less `file://`** (plain http is a byte-for-byte MITM injection point, and every base URL is user-overridable via env/QSettings; `file://<host>/share/…` is refused too — on Windows that is a UNC/SMB fetch over the network wearing a local scheme; only an empty or `localhost` host is accepted, and the error names the reason) — refused BEFORE any filesystem side effect, so no `.part` or directory is created. An optional 4th argument `expectedSha256` (case-insensitive hex; empty = legacy no-check, so the 21 consumers + the two QML call sites in `AISettingsDialog.qml` work unchanged) is verified by a local streamed `QCryptographicHash` helper **on the finished `.part` on disk, before the rename** (deliberately NOT the updater's `UpdateVerifier::sha256HexOfFile`: `qtmesh_updater` is only built/linked under `ENABLE_AUTO_UPDATER`, while `ModelDownloader` compiles unconditionally — reusing it broke the `-DENABLE_AUTO_UPDATER=OFF` link, caught in review; six lines of Qt API beat coupling every model download to the optional updater + libsodium) — never as a running hash in `onReadyRead`, because a resumed download appends to bytes this process never saw. On mismatch the `.part` is **deleted** (a poisoned partial must not be resumed from or cached) and `downloadError` fires. **Two gotchas verified empirically:** (1) the scheme refusal is emitted **QUEUED** (`QMetaObject::invokeMethod` + `Qt::QueuedConnection`), never synchronously — consumers connect, call `startDownload`, then `loop.exec()`, so a synchronous `loop.quit()` fires before `exec()` and is lost, hanging them for their full timeout (the #1017 review race); queued delivery lands inside `exec()` for every consumer without touching any — proven on `PhotoDepth`, which has no `settled` guard, failing in 1s instead of 600s. (2) Both refusals log at **`qCritical`**, not `qWarning`: `cliMessageHandler` drops warnings unless `--verbose`, and every consumer discards `downloadError`'s text (#1037), so a warning would leave the user with "offline?" and no way to learn the cause. **Known pre-existing bug (#1036):** the resume path never checks HTTP 206 vs 200 / `Content-Range`, so a server that ignores `Range` (`file://` always) appends the FULL body after a stale `.part` — reproduced: a 29-byte stale prefix yields a 208,044,845-byte model that **still loads and runs correctly** (ORT parses the garbage as an unknown protobuf field) while a 30-byte prefix fails with "Protobuf parsing failed". Load success proves nothing about integrity; only the digest does. `ModelDownloader::isAllowedDownloadUrl(url)` is the pure predicate for checking a base URL up front. Tests: `ModelDownloader_test.cpp` (both guards mutation-verified).
 
 ### AI Texture Generation