diff --git a/CLAUDE.md b/CLAUDE.md index 62833b42..e7a87255 100644 --- a/CLAUDE.md +++ b/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://` 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:///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://` 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:///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 diff --git a/src/ModelDownloader.cpp b/src/ModelDownloader.cpp index d1d1bd71..20c55997 100644 --- a/src/ModelDownloader.cpp +++ b/src/ModelDownloader.cpp @@ -142,6 +142,7 @@ void ModelDownloader::startDownload(const QString &url, const QString &destinati m_currentModelName = modelName; m_tempFilePath = destinationPath + ".part"; m_resumeOffset = 0; + m_expectedTotalBytes = -1; m_bytesReceived = 0; m_bytesTotal = 0; m_progress = 0.0f; @@ -181,6 +182,7 @@ void ModelDownloader::startDownload(const QString &url, const QString &destinati QString rangeHeader = QString("bytes=%1-").arg(m_resumeOffset); request.setRawHeader("Range", rangeHeader.toUtf8()); } + m_resumeUnverified = m_resumeOffset > 0; // #1036: prove the Range was honoured m_currentReply = m_networkManager->get(request); @@ -248,6 +250,7 @@ void ModelDownloader::resumeDownload() request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy); QString rangeHeader = QString("bytes=%1-").arg(m_resumeOffset); request.setRawHeader("Range", rangeHeader.toUtf8()); + m_resumeUnverified = true; // #1036 m_currentReply = m_networkManager->get(request); @@ -297,6 +300,8 @@ void ModelDownloader::cancelDownload() m_progress = 0.0f; m_downloadSpeed = 0.0f; m_resumeOffset = 0; + m_expectedTotalBytes = -1; + m_resumeUnverified = false; emit isDownloadingChanged(); emit currentModelNameChanged(); @@ -311,12 +316,166 @@ void ModelDownloader::cancelDownload() } } -void ModelDownloader::onReadyRead() +bool ModelDownloader::parseContentRange(const QByteArray& header, + qint64& first, qint64& last, qint64& total) +{ + first = -1; + last = -1; + total = -1; + const QByteArray cr = header.trimmed(); + // RFC 9110 §14.1: the range unit is CASE-INSENSITIVE — "Bytes 9-12/13" is + // a valid honoured resume (review on #1039). + const qsizetype sp = cr.indexOf(' '); + if (sp <= 0 || cr.left(sp).compare(QByteArrayLiteral("bytes"), Qt::CaseInsensitive) != 0) + return false; + const QByteArray range = cr.mid(sp + 1).trimmed(); + const qsizetype dash = range.indexOf('-'); + const qsizetype slash = range.indexOf('/'); + if (dash <= 0 || slash <= dash) + return false; + bool okFirst = false; + bool okLast = false; + bool okTotal = false; + const qint64 f = range.left(dash).trimmed().toLongLong(&okFirst); + const qint64 l = range.mid(dash + 1, slash - dash - 1).trimmed().toLongLong(&okLast); + const qint64 t = range.mid(slash + 1).trimmed().toLongLong(&okTotal); // "*" => unknown + if (okFirst) first = f; + if (okLast) last = l; + if (okTotal) total = t; + return okFirst && okLast; +} + +bool ModelDownloader::verifyResumeResponse() { - if (m_outputFile && m_currentReply) { - QByteArray data = m_currentReply->readAll(); - m_outputFile->write(data); + // #1036: a resume sends `Range: bytes=N-`, but nothing guaranteed the + // server HONOURED it. One that ignores Range (file:// always does; any + // proxy/CDN that strips the header will) 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). A load success proves nothing; only the response can tell us + // which bytes these are. Checked ONCE, on the first readyRead, because the + // status/headers are available then and re-checking per chunk is waste. + m_resumeUnverified = false; + const int status = m_currentReply->attribute( + QNetworkRequest::HttpStatusCodeAttribute).toInt(); + qint64 first = -1; + qint64 last = -1; + qint64 total = -1; + const bool parsed = status == 206 + && parseContentRange(m_currentReply->rawHeader("Content-Range"), first, last, total); + // Honoured = starts where we asked AND reaches the resource end (review: + // `bytes 9-10/13` starts right but leaves 11-12 missing — appending it and + // renaming on finish would promote an incomplete file). An unknown total + // ("*") cannot be checked here; the size check at finish stays unknown too. + if (parsed && first == m_resumeOffset && (total < 0 || last == total - 1)) { + m_expectedTotalBytes = total; + return true; // honoured: keep appending + } + + // A 206 whose window is the WHOLE resource (0..total-1) is a full body + // wearing a partial status — safe to truncate and take. Any OTHER 206 + // window is a genuinely partial body we did not ask for: writing it from + // byte 0 would yield an INCOMPLETE file, so that case must fail, not + // "recover". + if (const bool fullBodyDisguised = parsed && first == 0 && total > 0 && last == total - 1; + status == 206 && !fullBodyDisguised) { + qCritical() << "ModelDownloader: unusable 206 for" << m_currentModelName + << "— asked from byte" << m_resumeOffset << "got Content-Range first=" + << first << "last=" << last << "total=" << total << "— aborting"; + discardPartialAndFail( + QString("The server answered the resume request with a partial range that " + "does not match (asked from byte %1, got %2-%3/%4); the partial file " + "was discarded — retry to download from the start.") + .arg(m_resumeOffset).arg(first).arg(last).arg(total)); + return false; + } + + // Full body (200, or a 206 covering everything): discard the stale prefix + // and restart from byte 0 — never append. + qWarning() << "ModelDownloader: server ignored Range for" << m_currentModelName + << "(status" << status << ") — discarding" << m_resumeOffset + << "stale bytes and restarting from 0"; + // Reopen truncated: the Append-mode handle would keep writing after the + // stale prefix. A failed reopen is fatal for this download — appending + // would be worse than stopping. + m_outputFile->close(); + if (!m_outputFile->open(QIODevice::WriteOnly | QIODevice::Truncate)) { + emit downloadError(m_currentModelName, + QString("Server ignored the resume request and the partial file " + "could not be reset: %1").arg(m_tempFilePath)); + m_currentReply->abort(); + return false; + } + // The whole body is arriving; progress must not add the offset. + m_resumeOffset = 0; + m_bytesReceived = 0; + // Its size is what the response says it is (a 206 covering 0..total-1 + // carries the total; a 200 carries Content-Length), -1 when neither. + if (parsed && total > 0) m_expectedTotalBytes = total; + else { + const QVariant cl = m_currentReply->header(QNetworkRequest::ContentLengthHeader); + m_expectedTotalBytes = cl.isValid() && cl.toLongLong() > 0 ? cl.toLongLong() : -1; } + return true; +} + +void ModelDownloader::discardPartialAndFail(const QString& message) +{ + const QString name = m_currentModelName; + m_speedTimer->stop(); + if (m_outputFile) { + m_outputFile->close(); + delete m_outputFile; + m_outputFile = nullptr; + } + // Remove the poisoned partial; if removal fails, truncate it so a later + // attempt starts from byte 0 instead of resuming from garbage (the same + // fallback the SHA-256 path uses). Then reset the resume bookkeeping — + // review: resumeDownload() derives its offset from m_bytesReceived, and a + // stale value would re-request the old range against a fresh, empty file. + if (!m_tempFilePath.isEmpty() && !QFile::remove(m_tempFilePath)) { + QFile trunc(m_tempFilePath); + if (trunc.open(QIODevice::WriteOnly | QIODevice::Truncate)) trunc.close(); + else qCritical() << "ModelDownloader: could not delete or truncate" << m_tempFilePath; + } + m_bytesReceived = 0; + m_resumeOffset = 0; + m_expectedTotalBytes = -1; + m_resumeUnverified = false; + if (QNetworkReply* reply = m_currentReply) { + // abort() delivers errorOccurred + finished synchronously for a + // same-thread reply; the flag makes both handlers step aside so this + // remains the only cleanup and the only downloadError. Detach the + // pointer FIRST so a handler that does run can never leave us holding + // a reply it already released. + m_currentReply = nullptr; + m_abortingInternally = true; + reply->abort(); + m_abortingInternally = false; + reply->deleteLater(); + } + m_isDownloading = false; + m_isPaused = false; + m_currentUrl.clear(); + m_currentDestinationPath.clear(); + m_currentModelName.clear(); + m_tempFilePath.clear(); + m_expectedSha256.clear(); + m_downloadSpeed = 0.0f; + emit isDownloadingChanged(); + emit currentModelNameChanged(); + emit downloadSpeedChanged(); + emit downloadError(name, message); +} + +void ModelDownloader::onReadyRead() +{ + if (!m_outputFile || !m_currentReply) return; + if (m_resumeUnverified && !verifyResumeResponse()) + return; // aborted — an unusable partial window + QByteArray data = m_currentReply->readAll(); + m_outputFile->write(data); } void ModelDownloader::onDownloadProgress(qint64 bytesReceived, qint64 bytesTotal) @@ -343,6 +502,7 @@ void ModelDownloader::onDownloadProgress(qint64 bytesReceived, qint64 bytesTotal void ModelDownloader::onDownloadFinished() { + if (m_abortingInternally) return; // discardPartialAndFail() owns this cleanup m_speedTimer->stop(); if (m_currentReply && m_currentReply->error() == QNetworkReply::NoError) { @@ -354,6 +514,59 @@ void ModelDownloader::onDownloadFinished() m_outputFile = nullptr; } + // #1036 review, completeness before promotion. Two ways a "successful" + // finish can still be an incomplete file: + // (a) the resume reply finished WITHOUT ever delivering data — the + // verification in onReadyRead never ran, so the stale .part is + // unvetted (a 200/206 with an empty body, e.g. a server that treats + // an out-of-range Range as "nothing to send"); + // (b) the body was shorter than the size the response committed to. + // Neither is "NoError" to QNAM, and without a digest nothing else would + // catch it — the reproduced #1036 file LOADED. Keep the .part: it is a + // valid prefix, so the next startDownload resumes it (and re-verifies). + QString incomplete; + const qint64 actual = QFileInfo(m_tempFilePath).size(); + if (m_resumeUnverified) { + incomplete = QStringLiteral("the server answered the resume request without any " + "data, so the partial file could not be verified"); + } else { + qint64 expected = m_expectedTotalBytes; + if (expected < 0 && m_resumeOffset == 0) { + const QVariant cl = m_currentReply->header(QNetworkRequest::ContentLengthHeader); + if (cl.isValid() && cl.toLongLong() > 0) expected = cl.toLongLong(); + } + if (expected >= 0 && actual != expected) + incomplete = QStringLiteral("received %1 of %2 bytes").arg(actual).arg(expected); + else if (expected < 0 && m_expectedSha256.isEmpty()) + qWarning() << "ModelDownloader:" << m_currentModelName + << "— the server declared no size and no digest is configured; " + "completeness of the" << actual << "byte file cannot be verified"; + } + if (!incomplete.isEmpty()) { + qCritical() << "ModelDownloader: incomplete download for" << m_currentModelName + << "—" << incomplete << "— not promoting the partial file"; + emit downloadError(m_currentModelName, + QString("Download of %1 is incomplete: %2. The partial file was kept and " + "will be resumed on the next attempt.").arg(m_currentModelName, incomplete)); + m_resumeUnverified = false; + // shared cleanup below; the .part stays in place for a resume + if (m_currentReply) { m_currentReply->deleteLater(); m_currentReply = nullptr; } + m_isDownloading = false; + m_isPaused = false; + m_currentUrl.clear(); + m_currentDestinationPath.clear(); + m_currentModelName.clear(); + m_tempFilePath.clear(); + m_expectedSha256.clear(); + m_resumeOffset = 0; + m_expectedTotalBytes = -1; + m_downloadSpeed = 0.0f; + emit isDownloadingChanged(); + emit currentModelNameChanged(); + emit downloadSpeedChanged(); + return; + } + // #1029: verify the WHOLE finished .part file on disk, never a running // hash in onReadyRead — a resumed download appends to bytes this // process never saw, so only the file itself is authoritative. @@ -430,6 +643,7 @@ void ModelDownloader::onDownloadFinished() m_tempFilePath.clear(); m_expectedSha256.clear(); // #1029: never let one download's digest leak into the next m_resumeOffset = 0; + m_expectedTotalBytes = -1; m_downloadSpeed = 0.0f; emit isDownloadingChanged(); @@ -439,8 +653,8 @@ void ModelDownloader::onDownloadFinished() void ModelDownloader::onDownloadError(QNetworkReply::NetworkError error) { - if (error == QNetworkReply::OperationCanceledError && m_isPaused) { - // This is expected when pausing + if (error == QNetworkReply::OperationCanceledError && (m_isPaused || m_abortingInternally)) { + // Expected: pausing, or our own abort inside discardPartialAndFail() return; } diff --git a/src/ModelDownloader.h b/src/ModelDownloader.h index 5e674c72..0039bd75 100644 --- a/src/ModelDownloader.h +++ b/src/ModelDownloader.h @@ -34,6 +34,13 @@ class ModelDownloader : public QObject qint64 bytesTotal() const { return m_bytesTotal; } float downloadSpeed() const { return m_downloadSpeed; } + /// #1036: parse `Content-Range: -/`. The unit + /// is case-insensitive (RFC 9110 §14.1). Returns false unless first AND + /// last parsed; `total` is -1 for "*" (unknown). Pure — unit-tested directly. + static bool parseContentRange(const QByteArray& header, + qint64& first, qint64& last, qint64& total); + // NB: plain `public:`, NOT a slot — moc cannot register qint64& as a meta type. + public slots: /// Start a download. #1029 (CWE-494): /// - `url` MUST be https:// (or a host-less file:// for local @@ -103,6 +110,26 @@ private slots: QString m_currentModelName; QString m_tempFilePath; QString m_expectedSha256; ///< #1029 — empty = no verification + /// #1036: true until the FIRST readyRead of a resumed request has proven + /// the server honoured our Range header (206 + matching Content-Range). + bool m_resumeUnverified = false; + /// #1036 review: the resource size the response committed to (206 total, + /// or a full body's Content-Length). -1 = unknown. Checked against the + /// finished .part before the rename — a short body must never be promoted. + qint64 m_expectedTotalBytes = -1; + /// True while WE abort the reply: abort() delivers errorOccurred/finished + /// synchronously, and without this the handlers would emit a second + /// downloadError and run a second cleanup (review). + bool m_abortingInternally = false; + /// #1036: on the first bytes of a resumed request, decide whether the + /// server honoured our Range. Returns true to keep writing (appending, or + /// restarted from byte 0 after a truncate), false when the download was + /// aborted because the body is a partial window we cannot use. + bool verifyResumeResponse(); + /// The ONE path for "this partial file is unusable": remove (or truncate) + /// the .part, reset the resume bookkeeping, abort the reply silently, end + /// the download and emit exactly one downloadError. + void discardPartialAndFail(const QString& message); bool m_isDownloading = false; bool m_isPaused = false; diff --git a/src/ModelDownloader_test.cpp b/src/ModelDownloader_test.cpp index 54398000..a41775d0 100644 --- a/src/ModelDownloader_test.cpp +++ b/src/ModelDownloader_test.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,10 @@ class FakeNetworkReply : public QNetworkReply { QObject* parent = nullptr) : QNetworkReply(parent), m_payload(payload) { + // #1036: default to a plain 200 with no Content-Range — exactly what a + // server that ignores Range returns. Tests that model an honoured + // resume call withPartialContent() explicitly. + setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200); open(QIODevice::ReadOnly | QIODevice::Unbuffered); setUrl(QUrl("https://example.invalid/model.bin")); if (errorCode != QNetworkReply::NoError) @@ -27,7 +32,30 @@ class FakeNetworkReply : public QNetworkReply { setFinished(errorCode == QNetworkReply::NoError); } - void abort() override {} + /// #1036 review: a real same-thread reply delivers errorOccurred + finished + /// SYNCHRONOUSLY from abort(). Off by default (older tests never abort); + /// the single-error test turns it on to prove the handlers step aside. + bool signalOnAbort = false; + void abort() override + { + if (!signalOnAbort) return; + setError(QNetworkReply::OperationCanceledError, QStringLiteral("Operation canceled")); + emit errorOccurred(QNetworkReply::OperationCanceledError); + setFinished(true); + emit finished(); + } + void withContentLength(qint64 n) { setHeader(QNetworkRequest::ContentLengthHeader, n); } + + /// #1036: model a server that honoured `Range: bytes=-`. + FakeNetworkReply* withPartialContent(qint64 first, qint64 last, qint64 total, + const QByteArray& unit = QByteArrayLiteral("bytes")) + { + setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 206); + setRawHeader("Content-Range", + unit + ' ' + QByteArray::number(first) + '-' + + QByteArray::number(last) + '/' + QByteArray::number(total)); + return this; + } qint64 bytesAvailable() const override { @@ -539,3 +567,433 @@ TEST_F(ModelDownloaderTest, RejectedUrlErrorRedactsCredentialsAndQuery) EXPECT_FALSE(msg.contains("alice")) << msg.toStdString(); EXPECT_TRUE (msg.contains("mirror.example.invalid/models/m.bin")) << msg.toStdString(); } + + +// ---- #1036: a resume must PROVE the server honoured Range ------------------- + +TEST_F(ModelDownloaderTest, ResumeAgainst200RestartsFromZeroInsteadOfAppending) +{ + // The reproduced corruption: stale .part + a server that ignores Range + // (200, full body) => full body appended after the stale prefix. The + // result must be the payload ALONE. + const QString partialPath = tempFilePath("resume200.bin.part"); + QFile seed(partialPath); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write("STALEPREFIX"); // 11 bytes the server never saw + seed.close(); + + downloader->m_currentModelName = "Resume200"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 11; + downloader->m_bytesReceived = 11; + downloader->m_resumeUnverified = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + downloader->m_currentReply = new FakeNetworkReply("FULLBODY", QNetworkReply::NoError, {}, downloader); + + QSignalSpy errorSpy(downloader, &ModelDownloader::downloadError); + downloader->onReadyRead(); + downloader->m_outputFile->flush(); + + QFile out(partialPath); + ASSERT_TRUE(out.open(QIODevice::ReadOnly)); + EXPECT_EQ(out.readAll(), QByteArray("FULLBODY")) + << "stale prefix survived — the full body was appended, not written from 0"; + EXPECT_EQ(downloader->m_resumeOffset, 0) << "progress would add a phantom offset"; + EXPECT_EQ(downloader->m_bytesReceived, 0); + EXPECT_FALSE(downloader->m_resumeUnverified); + EXPECT_EQ(errorSpy.count(), 0) << "a 200 is recoverable, not an error"; +} + +TEST_F(ModelDownloaderTest, ResumeAgainst206WithMatchingRangeAppends) +{ + // The honoured case must be untouched: 206 + Content-Range starting at our + // offset => append, keep the offset. + const QString partialPath = tempFilePath("resume206.bin.part"); + QFile seed(partialPath); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write("FIRSTPART"); // 9 bytes + seed.close(); + + downloader->m_currentModelName = "Resume206"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 9; + downloader->m_bytesReceived = 9; + downloader->m_resumeUnverified = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("REST", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(9, 12, 13); + downloader->m_currentReply = reply; + + downloader->onReadyRead(); + downloader->m_outputFile->flush(); + + QFile out(partialPath); + ASSERT_TRUE(out.open(QIODevice::ReadOnly)); + EXPECT_EQ(out.readAll(), QByteArray("FIRSTPARTREST")); + EXPECT_EQ(downloader->m_resumeOffset, 9) << "an honoured resume must keep its offset"; +} + +TEST_F(ModelDownloaderTest, ResumeAgainst206CoveringWholeResourceRestartsFromZero) +{ + // 206 whose window is 0..total-1 is the FULL body wearing a partial + // status. Not what we asked for, but complete — so truncate and take it. + const QString partialPath = tempFilePath("resume206bad.bin.part"); + QFile seed(partialPath); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write("ABCDE"); // 5 bytes + seed.close(); + + downloader->m_currentModelName = "Resume206Bad"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 5; + downloader->m_bytesReceived = 5; + downloader->m_resumeUnverified = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("XYZ", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(0, 2, 3); // server restarted from 0 on its own + downloader->m_currentReply = reply; + + downloader->onReadyRead(); + downloader->m_outputFile->flush(); + + QFile out(partialPath); + ASSERT_TRUE(out.open(QIODevice::ReadOnly)); + EXPECT_EQ(out.readAll(), QByteArray("XYZ")); + EXPECT_EQ(downloader->m_resumeOffset, 0); +} + +TEST_F(ModelDownloaderTest, FreshDownloadNeverRunsTheResumeCheck) +{ + // No resume => no Range => the 200 is exactly what we asked for. The + // check must not fire and must not truncate a fresh write. + const QString path = tempFilePath("fresh.bin.part"); + downloader->m_resumeOffset = 0; + downloader->m_resumeUnverified = false; + downloader->m_outputFile = new QFile(path, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::WriteOnly)); + downloader->m_currentReply = new FakeNetworkReply("hello", QNetworkReply::NoError, {}, downloader); + + downloader->onReadyRead(); + downloader->m_outputFile->flush(); + QFile out(path); + ASSERT_TRUE(out.open(QIODevice::ReadOnly)); + EXPECT_EQ(out.readAll(), QByteArray("hello")); +} + + +TEST_F(ModelDownloaderTest, ResumeAgainst206WithUppercaseBytesUnitIsHonoured) +{ + // RFC 9110: the range unit is case-insensitive. "Bytes 9-12/13" is a + // valid honoured resume and MUST append — misreading it as "ignored" would + // truncate and keep only the suffix: an incomplete file that a no-digest + // caller then renames and caches. (Review on #1039.) + const QString partialPath = tempFilePath("resumeBytes.bin.part"); + QFile seed(partialPath); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write("FIRSTPART"); // 9 bytes + seed.close(); + + downloader->m_currentModelName = "ResumeBytes"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 9; + downloader->m_bytesReceived = 9; + downloader->m_resumeUnverified = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("REST", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(9, 12, 13, QByteArrayLiteral("Bytes")); + downloader->m_currentReply = reply; + + QSignalSpy errorSpy(downloader, &ModelDownloader::downloadError); + downloader->onReadyRead(); + downloader->m_outputFile->flush(); + + QFile out(partialPath); + ASSERT_TRUE(out.open(QIODevice::ReadOnly)); + EXPECT_EQ(out.readAll(), QByteArray("FIRSTPARTREST")) + << "an honoured resume with an uppercase unit was treated as ignored"; + EXPECT_EQ(downloader->m_resumeOffset, 9); + EXPECT_EQ(errorSpy.count(), 0); +} + +TEST_F(ModelDownloaderTest, ResumeAgainst206WithForeignPartialWindowAbortsAndDiscards) +{ + // 206 with a window that is neither ours nor the whole resource is a + // genuinely PARTIAL body we did not ask for. Writing it from byte 0 would + // produce an incomplete file, so this must FAIL, not "recover": error + // emitted, .part removed so the next attempt starts clean, nothing renamed. + const QString partialPath = tempFilePath("resumeForeign.bin.part"); + QFile seed(partialPath); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write("ABCDE"); // 5 bytes + seed.close(); + + downloader->m_currentModelName = "ResumeForeign"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 5; + downloader->m_bytesReceived = 5; + downloader->m_resumeUnverified = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("MID", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(3, 5, 10); // bytes 3..5 of a 10-byte resource + downloader->m_currentReply = reply; + + QSignalSpy errorSpy(downloader, &ModelDownloader::downloadError); + downloader->onReadyRead(); + + ASSERT_EQ(errorSpy.count(), 1); + EXPECT_TRUE(errorSpy.at(0).at(1).toString().contains("partial range")) + << errorSpy.at(0).at(1).toString().toStdString(); + EXPECT_FALSE(QFileInfo::exists(partialPath)) << "foreign-window .part must not survive to be resumed"; +} + + +// ---- #1036: the Content-Range parser, tested directly ----------------------- + +// --- #1036 review round: completeness before promotion, one error per failure --- + +namespace { +void seedPartial(const QString& path, const QByteArray& bytes) +{ + QFile seed(path); + ASSERT_TRUE(seed.open(QIODevice::WriteOnly)); + seed.write(bytes); + seed.close(); +} +} // namespace + +TEST_F(ModelDownloaderTest, ResumeAgainst206ShortWindowIsRejectedNotAppended) +{ + // `bytes 9-10/13` starts where we asked but stops short of the end: taking + // it and renaming on finish would promote a 11-byte file as the 13-byte + // model. Reviewer's exact case. + const QString partialPath = tempFilePath("shortwin.bin.part"); + seedPartial(partialPath, "FIRSTPART"); // 9 + downloader->m_currentModelName = "ShortWin"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 9; + downloader->m_bytesReceived = 9; + downloader->m_resumeUnverified = true; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("RE", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(9, 10, 13); + downloader->m_currentReply = reply; + QSignalSpy errors(downloader, &ModelDownloader::downloadError); + + downloader->onReadyRead(); + + EXPECT_EQ(errors.count(), 1); + EXPECT_FALSE(QFile::exists(partialPath)) << "an unusable window must not leave a resumable prefix"; + EXPECT_EQ(downloader->m_resumeOffset, 0); + EXPECT_EQ(downloader->m_bytesReceived, 0); + EXPECT_FALSE(downloader->isDownloading()); + EXPECT_EQ(downloader->m_currentReply, nullptr); +} + +TEST_F(ModelDownloaderTest, ForeignWindowEmitsExactlyOneErrorEvenWhenAbortSignalsSynchronously) +{ + // Reviewer: abort() runs onDownloadError() (and finished) before returning, + // so the old code emitted a generic error, ran a cleanup, and THEN emitted + // the range-mismatch error. One failure, one downloadError. + const QString partialPath = tempFilePath("oneerr.bin.part"); + seedPartial(partialPath, "ABCDE"); + downloader->m_currentModelName = "OneErr"; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 5; + downloader->m_bytesReceived = 5; + downloader->m_resumeUnverified = true; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("XYZ", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(100, 102, 500); // foreign window + reply->signalOnAbort = true; + downloader->m_currentReply = reply; + // Wire the reply exactly as resumeDownload() does — without these the + // synchronous abort signals reach nobody and the test proves nothing + // (a mutant with the guard removed passed the first version of this test). + QObject::connect(reply, &QNetworkReply::finished, downloader, &ModelDownloader::onDownloadFinished); + QObject::connect(reply, &QNetworkReply::errorOccurred, downloader, &ModelDownloader::onDownloadError); + QSignalSpy errors(downloader, &ModelDownloader::downloadError); + QSignalSpy completed(downloader, &ModelDownloader::downloadCompleted); + QSignalSpy downloadingChanged(downloader, &ModelDownloader::isDownloadingChanged); + + downloader->onReadyRead(); + + ASSERT_EQ(errors.count(), 1) << "exactly one downloadError for one failure"; + EXPECT_EQ(downloadingChanged.count(), 1) + << "one cleanup: onDownloadFinished must step aside during our own abort"; + EXPECT_TRUE(errors.at(0).at(1).toString().contains("does not match")); + EXPECT_EQ(completed.count(), 0); + EXPECT_FALSE(QFile::exists(partialPath)); + EXPECT_FALSE(downloader->isDownloading()); + EXPECT_FALSE(downloader->m_isPaused) << "a discarded partial is not resumable"; +} + +TEST_F(ModelDownloaderTest, FinishedWithoutBodyWhileResumeUnverifiedDoesNotRename) +{ + // Reviewer: an empty successful reply emits finished without readyRead, so + // the verification never ran — the stale .part must not be promoted. + const QString dest = tempFilePath("nobody.bin"); + const QString partialPath = dest + ".part"; + seedPartial(partialPath, "STALEPREFIX"); + downloader->m_currentModelName = "NoBody"; + downloader->m_currentDestinationPath = dest; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 11; + downloader->m_bytesReceived = 11; + downloader->m_resumeUnverified = true; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + downloader->m_currentReply = new FakeNetworkReply(QByteArray(), QNetworkReply::NoError, {}, downloader); + QSignalSpy errors(downloader, &ModelDownloader::downloadError); + QSignalSpy completed(downloader, &ModelDownloader::downloadCompleted); + + downloader->onDownloadFinished(); + + EXPECT_EQ(completed.count(), 0); + EXPECT_FALSE(QFile::exists(dest)) << "stale prefix must not become the model"; + ASSERT_EQ(errors.count(), 1); + EXPECT_TRUE(errors.at(0).at(1).toString().contains("without any data")); + EXPECT_TRUE(QFile::exists(partialPath)) << "kept: a valid prefix for the next resume"; +} + +TEST_F(ModelDownloaderTest, HonouredResumeWithShortBodyIsNotRenamed) +{ + // Range honoured (9-12/13) but the connection delivered only 3 of the 4 + // bytes: the .part is 12 bytes against a committed total of 13. + const QString dest = tempFilePath("shortbody.bin"); + const QString partialPath = dest + ".part"; + seedPartial(partialPath, "FIRSTPART"); // 9 + downloader->m_currentModelName = "ShortBody"; + downloader->m_currentDestinationPath = dest; + downloader->m_tempFilePath = partialPath; + downloader->m_resumeOffset = 9; + downloader->m_bytesReceived = 9; + downloader->m_resumeUnverified = true; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::Append)); + auto* reply = new FakeNetworkReply("RES", QNetworkReply::NoError, {}, downloader); + reply->withPartialContent(9, 12, 13); + downloader->m_currentReply = reply; + QSignalSpy errors(downloader, &ModelDownloader::downloadError); + QSignalSpy completed(downloader, &ModelDownloader::downloadCompleted); + + downloader->onReadyRead(); + EXPECT_EQ(downloader->m_expectedTotalBytes, 13); + downloader->onDownloadFinished(); + + EXPECT_EQ(completed.count(), 0); + EXPECT_FALSE(QFile::exists(dest)); + ASSERT_EQ(errors.count(), 1); + EXPECT_TRUE(errors.at(0).at(1).toString().contains("received 12 of 13 bytes")); + EXPECT_EQ(QFileInfo(partialPath).size(), 12) << "kept for resume"; +} + +TEST_F(ModelDownloaderTest, FreshDownloadShorterThanContentLengthIsNotRenamed) +{ + const QString dest = tempFilePath("freshshort.bin"); + const QString partialPath = dest + ".part"; + downloader->m_currentModelName = "FreshShort"; + downloader->m_currentDestinationPath = dest; + downloader->m_tempFilePath = partialPath; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::WriteOnly | QIODevice::Truncate)); + auto* reply = new FakeNetworkReply("1234", QNetworkReply::NoError, {}, downloader); + reply->withContentLength(10); + downloader->m_currentReply = reply; + QSignalSpy errors(downloader, &ModelDownloader::downloadError); + + downloader->onReadyRead(); + downloader->onDownloadFinished(); + + EXPECT_FALSE(QFile::exists(dest)); + ASSERT_EQ(errors.count(), 1); + EXPECT_TRUE(errors.at(0).at(1).toString().contains("received 4 of 10 bytes")); +} + +TEST_F(ModelDownloaderTest, FreshDownloadMatchingContentLengthIsRenamed) +{ + const QString dest = tempFilePath("freshok.bin"); + const QString partialPath = dest + ".part"; + downloader->m_currentModelName = "FreshOk"; + downloader->m_currentDestinationPath = dest; + downloader->m_tempFilePath = partialPath; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::WriteOnly | QIODevice::Truncate)); + auto* reply = new FakeNetworkReply("1234567890", QNetworkReply::NoError, {}, downloader); + reply->withContentLength(10); + downloader->m_currentReply = reply; + QSignalSpy completed(downloader, &ModelDownloader::downloadCompleted); + + downloader->onReadyRead(); + downloader->onDownloadFinished(); + + EXPECT_EQ(completed.count(), 1); + EXPECT_TRUE(QFile::exists(dest)); +} + +TEST_F(ModelDownloaderTest, UnknownLengthWithoutDigestIsAcceptedWithWarning) +{ + // Documented compatibility choice: a server that declares no size (chunked + // transfer) and no configured digest leaves nothing to check against, so + // the file is accepted with a warning rather than refused. HF/GitHub and + // QNAM's file:// backend always send Content-Length, so the strict path is + // the one real downloads take. + const QString dest = tempFilePath("nolen.bin"); + const QString partialPath = dest + ".part"; + downloader->m_currentModelName = "NoLen"; + downloader->m_currentDestinationPath = dest; + downloader->m_tempFilePath = partialPath; + downloader->m_isDownloading = true; + downloader->m_outputFile = new QFile(partialPath, downloader); + ASSERT_TRUE(downloader->m_outputFile->open(QIODevice::WriteOnly | QIODevice::Truncate)); + downloader->m_currentReply = new FakeNetworkReply("abc", QNetworkReply::NoError, {}, downloader); + QSignalSpy completed(downloader, &ModelDownloader::downloadCompleted); + + downloader->onReadyRead(); + downloader->onDownloadFinished(); + + EXPECT_EQ(completed.count(), 1); + EXPECT_TRUE(QFile::exists(dest)); +} + +TEST_F(ModelDownloaderTest, ContentRangeParserHandlesUnitCaseAndUnknownTotal) +{ + qint64 f = 0, l = 0, t = 0; + EXPECT_TRUE(ModelDownloader::parseContentRange("bytes 9-12/13", f, l, t)); + EXPECT_EQ(f, 9); EXPECT_EQ(l, 12); EXPECT_EQ(t, 13); + // RFC 9110 §14.1: unit is case-insensitive. + EXPECT_TRUE(ModelDownloader::parseContentRange("Bytes 9-12/13", f, l, t)); + EXPECT_EQ(f, 9); EXPECT_EQ(l, 12); EXPECT_EQ(t, 13); + EXPECT_TRUE(ModelDownloader::parseContentRange("BYTES 0-2/3", f, l, t)); + EXPECT_EQ(f, 0); EXPECT_EQ(l, 2); EXPECT_EQ(t, 3); + // "*" = total unknown => -1, but first/last still parse. + EXPECT_TRUE(ModelDownloader::parseContentRange("bytes 5-7/*", f, l, t)); + EXPECT_EQ(f, 5); EXPECT_EQ(l, 7); EXPECT_EQ(t, -1); + // Surrounding/extra whitespace is tolerated. + EXPECT_TRUE(ModelDownloader::parseContentRange(" bytes 9-12/13 ", f, l, t)); + EXPECT_EQ(f, 9); +} + +TEST_F(ModelDownloaderTest, ContentRangeParserRejectsMalformedHeaders) +{ + qint64 f = 1, l = 1, t = 1; + for (const char* h : {"", "bytes", "bytes 9", "bytes 9-12", "bytes -12/13", + "bytes 9/13", "items 9-12/13", "9-12/13", "bytes abc-12/13"}) { + EXPECT_FALSE(ModelDownloader::parseContentRange(h, f, l, t)) << "accepted: '" << h << "'"; + } + // On rejection the outputs are reset, never left at caller garbage. + ModelDownloader::parseContentRange("items 9-12/13", f, l, t); + EXPECT_EQ(f, -1); EXPECT_EQ(l, -1); EXPECT_EQ(t, -1); +}