From 8772a382ff8d543bed79e4de96035f0b7a2ed657 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 05:34:09 +0000 Subject: [PATCH] fix: act on the post-0.2.0 exhaustive review findings Works the net-new findings from the September 2026 read-only review that were reproducible against the current tree, plus three adjacent defects the review did not name. Security / privacy - PDFLogScrubber scrubs credential material (URL userinfo such as a Sentry DSN, HTTP authorization values, secret-named key/value pairs) ahead of the existing path and email passes. The bare auth-scheme pass deliberately excludes "Token" so parser diagnostics are not redacted as secrets. - loop-ocr reads a staged raster once by descriptor instead of re-resolving the path for isfile(), PIL, and easyocr in turn, closing the TOCTOU window; language codes are shape-validated before they reach easyocr's model file names; PdfTool stages the raster 0600. - Diagnostics bundles truncate plugin display fields. Fail-closed behaviour - PdfTool extraction commands record output.empty-result and accept a shared --fail-if-empty (exit 1, findings) so an empty output directory cannot pass a pipeline that gates on produced files. - writeIncremental reports whether it appended or only byte-copied; damaged documents now carry a source digest, so its "file changed underneath us" guard is no longer silently disabled for permissively recovered documents. Bounds on attacker-controlled shapes - Damaged-document recovery bounds its dense object table by objects recovered, not by the highest declared object number. - PDFNameTreeLoader terminates cyclic Kids chains and caps depth, entry count, and key length (net-new: the cycle was unbounded recursion). - Structure-tree parsing bounds recursion depth on long acyclic chains. - PDFJBIG2Bitmap::paint validates grown dimensions on its expandY path, the one path that escaped the constructor's dimension check (net-new). Ergonomics - isPathContained no longer rejects a planned output whose target directory does not exist yet, keeping the stricter symlinked-parent rule for the file side. - makeUniqueFileName probes 128 sequential names, then random ones. - OCR option defaults are defined once and shared by capability discovery and the command-line parser. Not built or run here: this environment has no Qt, so the C++ changes are unverified by compilation. The Python sidecar tests pass and the new scrubber patterns were validated against a reference implementation of the same passes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FDJQDFR5ctoKDJbp7LG4GS --- LoopLibCore/sources/pdfdiagnostics.cpp | 27 ++++-- LoopLibCore/sources/pdfdocumentreader.cpp | 44 +++++++++- LoopLibCore/sources/pdfdocumentwriter.cpp | 40 ++++++--- LoopLibCore/sources/pdfdocumentwriter.h | 28 ++++-- LoopLibCore/sources/pdffilenamesanitizer.cpp | 21 ++++- LoopLibCore/sources/pdfjbig2decoder.cpp | 15 +++- LoopLibCore/sources/pdflogscrubber.cpp | 59 +++++++++++++ LoopLibCore/sources/pdflogscrubber.h | 14 +-- LoopLibCore/sources/pdfnametreeloader.h | 57 +++++++++++- LoopLibCore/sources/pdfobjectutils.h | 6 ++ LoopLibCore/sources/pdfsafefilewriter.cpp | 36 ++++++-- LoopLibCore/sources/pdfstructuretree.cpp | 14 +++ PdfTool/ocrsidecarprotocol.h | 15 +++- PdfTool/pdftoolabstractapplication.cpp | 63 ++++++++++++-- PdfTool/pdftoolabstractapplication.h | 20 +++++ PdfTool/pdftoolattachments.cpp | 14 ++- PdfTool/pdftoolfetchimages.cpp | 17 +++- PdfTool/pdftoolfetchtext.cpp | 21 ++++- PdfTool/pdftoolocr.cpp | 9 ++ UnitTests/CMakeLists.txt | 3 +- UnitTests/tst_budgetexhaustiontest.cpp | 50 +++++++++++ UnitTests/tst_diagnosticstest.cpp | 62 ++++++++++++- UnitTests/tst_filenamesanitizertest.cpp | 19 ++++ UnitTests/tst_incrementalsavetest.cpp | 72 +++++++++++++++ UnitTests/tst_jbig2decodertest.cpp | 29 +++++++ UnitTests/tst_pdftoolcontract.cpp | 87 +++++++++++++++++++ UnitTests/tst_processingbudgettest.cpp | 46 ++++++++++ UnitTests/tst_safefilewritertest.cpp | 22 +++++ .../claude-loop-exhaustive-review-73k4t5.md | 35 ++++++++ docs/PDFTOOL_CLI_CONTRACT.md | 29 ++++++- loop-ocr/schemas/ocr-sidecar.schema.json | 2 +- loop-ocr/service/engine.py | 54 +++++++++++- loop-ocr/tests/test_engine.py | 57 +++++++++++- 33 files changed, 1020 insertions(+), 67 deletions(-) create mode 100644 changes/claude-loop-exhaustive-review-73k4t5.md diff --git a/LoopLibCore/sources/pdfdiagnostics.cpp b/LoopLibCore/sources/pdfdiagnostics.cpp index f57069718..ac878f55e 100644 --- a/LoopLibCore/sources/pdfdiagnostics.cpp +++ b/LoopLibCore/sources/pdfdiagnostics.cpp @@ -138,18 +138,35 @@ QJsonObject buildSystemInfo(const QString& applicationId) return root; } +/// Longest plugin display string copied into a bundle. Plugin metadata is +/// author-supplied JSON that Loop does not size-validate at load time, so an +/// installed plugin with a multi-megabyte Description would otherwise bloat +/// every future support bundle. The cap is generous for a real display string +/// and the truncation is visible rather than silent. +constexpr int PLUGIN_FIELD_LENGTH_LIMIT = 2048; + +QString truncatePluginField(const QString& value) +{ + if (value.size() <= PLUGIN_FIELD_LENGTH_LIMIT) + { + return value; + } + + return value.left(PLUGIN_FIELD_LENGTH_LIMIT) + QStringLiteral("... "); +} + QJsonObject buildPlugins(const PDFPluginInfos& plugins) { QJsonArray array; for (const PDFPluginInfo& plugin : plugins) { QJsonObject entry; - entry[QStringLiteral("name")] = plugin.name; - entry[QStringLiteral("pluginId")] = plugin.pluginId; + entry[QStringLiteral("name")] = truncatePluginField(plugin.name); + entry[QStringLiteral("pluginId")] = truncatePluginField(plugin.pluginId); entry[QStringLiteral("abiVersion")] = static_cast(plugin.abiVersion); - entry[QStringLiteral("author")] = plugin.author; - entry[QStringLiteral("version")] = plugin.version; - entry[QStringLiteral("license")] = plugin.license; + entry[QStringLiteral("author")] = truncatePluginField(plugin.author); + entry[QStringLiteral("version")] = truncatePluginField(plugin.version); + entry[QStringLiteral("license")] = truncatePluginField(plugin.license); array.append(entry); } diff --git a/LoopLibCore/sources/pdfdocumentreader.cpp b/LoopLibCore/sources/pdfdocumentreader.cpp index 9f7f4e03d..3cb429338 100644 --- a/LoopLibCore/sources/pdfdocumentreader.cpp +++ b/LoopLibCore/sources/pdfdocumentreader.cpp @@ -40,6 +40,19 @@ namespace pdf { +namespace +{ + +// Bounds for the dense object table built by damaged-document recovery. The +// table is indexed by object number, so its size is driven by the highest +// number a malformed document happens to declare rather than by how much was +// actually recovered. A document numbered more sparsely than this is not a +// recoverable document, it is a document asking for a large allocation. +constexpr PDFInteger DAMAGED_DOCUMENT_MAX_OBJECT_NUMBER_DENSITY = 64; +constexpr PDFInteger DAMAGED_DOCUMENT_MINIMUM_OBJECT_SLOTS = 4096; + +} // namespace + PDFDocumentReader::PDFDocumentReader(PDFProgress* progress, const std::function& getPasswordCallback, bool permissive, @@ -884,7 +897,27 @@ PDFDocument PDFDocumentReader::readDamagedDocumentFromBuffer(const QByteArray& b if (!restoredObjects.empty()) { - objects.resize(restoredObjects.rbegin()->first.objectNumber + 1); + // The object table is dense: it is indexed by object number, so a + // single recovered object numbered 9999999 would allocate ten million + // entries. The per-reference check in restoreObjects() bounds an + // object number by the file size, which on a 50 MiB file still allows + // a table of fifty million entries. Bound the table by how many + // objects were actually recovered instead: real damaged documents are + // numbered densely, and a highest-number-to-recovered-count ratio far + // past that is a malformed document rather than a recoverable one. + const PDFInteger highestObjectNumber = restoredObjects.rbegin()->first.objectNumber; + const PDFInteger maximumObjectNumber = std::max( + DAMAGED_DOCUMENT_MINIMUM_OBJECT_SLOTS, + static_cast(restoredObjects.size()) * DAMAGED_DOCUMENT_MAX_OBJECT_NUMBER_DENSITY); + + if (highestObjectNumber > maximumObjectNumber) + { + throw PDFException(PDFTranslationContext::tr("Damaged document declares object number %1, but only %2 objects could be recovered; refusing to build an object table of that size.") + .arg(highestObjectNumber) + .arg(restoredObjects.size())); + } + + objects.resize(highestObjectNumber + 1); for (auto& objectItem : restoredObjects) { @@ -901,7 +934,14 @@ PDFDocument PDFDocumentReader::readDamagedDocumentFromBuffer(const QByteArray& b } PDFObjectStorage storage(std::move(objects), PDFObject(trailerDictionaryObject), qMove(m_securityHandler)); - return PDFDocument(std::move(storage), m_version, QByteArray()); + + // The recovered document corresponds to exactly these bytes, so it gets a + // real source digest like any other successful read. Returning an empty + // digest here silently disabled the "the file changed underneath us" guard + // in PDFDocumentWriter::writeIncremental for every permissively recovered + // document - the one class of document where appending to the wrong bytes + // is most likely. + return PDFDocument(std::move(storage), m_version, hash(buffer)); } catch (const PDFException &parserException) { diff --git a/LoopLibCore/sources/pdfdocumentwriter.cpp b/LoopLibCore/sources/pdfdocumentwriter.cpp index 78e3d0564..d8c6a26f3 100644 --- a/LoopLibCore/sources/pdfdocumentwriter.cpp +++ b/LoopLibCore/sources/pdfdocumentwriter.cpp @@ -284,9 +284,10 @@ PDFOperationResult PDFDocumentWriter::write(QIODevice* device, const PDFDocument } PDFOperationResult PDFDocumentWriter::writeIncremental(const QString& fileName, - const PDFDocument* originalDocument, - const PDFDocument* document, - bool safeWrite) + const PDFDocument* originalDocument, + const PDFDocument* document, + bool safeWrite, + IncrementalWriteOutcome* outcome) { if (!originalDocument || !document) { @@ -311,7 +312,7 @@ PDFOperationResult PDFDocumentWriter::writeIncremental(const QString& fileName, return tr("File '%1' can't be opened for incremental save. %2").arg(fileName, targetFile.errorString()); } - const PDFOperationResult result = writeIncremental(&targetFile, originalData, originalDocument, document); + const PDFOperationResult result = writeIncremental(&targetFile, originalData, originalDocument, document, outcome); if (result && !targetFile.commit()) { return tr("File '%1' can't be committed after incremental save. %2").arg(fileName, targetFile.errorString()); @@ -329,15 +330,16 @@ PDFOperationResult PDFDocumentWriter::writeIncremental(const QString& fileName, return tr("File '%1' can't be opened for incremental save. %2").arg(fileName, targetFile.errorString()); } - const PDFOperationResult result = writeIncremental(&targetFile, originalData, originalDocument, document); + const PDFOperationResult result = writeIncremental(&targetFile, originalData, originalDocument, document, outcome); targetFile.close(); return result; } PDFOperationResult PDFDocumentWriter::writeIncremental(QIODevice* device, - const QByteArray& originalData, - const PDFDocument* originalDocument, - const PDFDocument* document) + const QByteArray& originalData, + const PDFDocument* originalDocument, + const PDFDocument* document, + IncrementalWriteOutcome* outcome) { if (!device || !device->isWritable() || !originalDocument || !document) { @@ -411,9 +413,20 @@ PDFOperationResult PDFDocumentWriter::writeIncremental(QIODevice* device, if (changedObjects.empty()) { - return device->write(originalData) == originalData.size() - ? PDFOperationResult(true) - : PDFOperationResult(tr("Failed to copy the original PDF bytes.")); + // Nothing changed, so there is nothing to append. The bytes are copied + // verbatim - which is the right output - but it is not an append, and a + // caller that asked for one is told so through \p outcome. + if (device->write(originalData) != originalData.size()) + { + return PDFOperationResult(tr("Failed to copy the original PDF bytes.")); + } + + if (outcome) + { + *outcome = IncrementalWriteOutcome::CopiedUnchanged; + } + + return PDFOperationResult(true); } if (device->write(originalData) != originalData.size()) @@ -516,6 +529,11 @@ PDFOperationResult PDFDocumentWriter::writeIncremental(QIODevice* device, writeCRLF(device); device->write("%%EOF"); + if (outcome) + { + *outcome = IncrementalWriteOutcome::Appended; + } + return true; } diff --git a/LoopLibCore/sources/pdfdocumentwriter.h b/LoopLibCore/sources/pdfdocumentwriter.h index 911aebeae..b2e43e3ab 100644 --- a/LoopLibCore/sources/pdfdocumentwriter.h +++ b/LoopLibCore/sources/pdfdocumentwriter.h @@ -50,6 +50,18 @@ class LOOPLIBCORESHARED_EXPORT PDFDocumentWriter Incremental }; + /// What an incremental save actually did. Every refusal to append is already + /// reported as a failed PDFOperationResult naming the reason, but one success + /// path is not an append at all: a document with no changed objects is + /// byte-copied. A caller that asked for an incremental save specifically to + /// preserve `Prev`/signature coverage needs to be able to tell those apart, + /// so writeIncremental reports the outcome on request. + enum class IncrementalWriteOutcome + { + Appended, ///< Changed objects plus a new xref section were appended + CopiedUnchanged ///< Nothing changed; the original bytes were copied verbatim + }; + explicit inline PDFDocumentWriter(PDFProgress* progress, const PDFOperationControl* operationControl = nullptr) : m_operationControl(operationControl) @@ -83,18 +95,22 @@ class LOOPLIBCORESHARED_EXPORT PDFDocumentWriter /// Appends an incremental update to an existing PDF. The original bytes /// are copied unchanged and only changed objects plus a new xref/trailer /// section are appended. + /// \param outcome Optional; set on success to what the save actually did PDFOperationResult writeIncremental(const QString& fileName, - const PDFDocument* originalDocument, - const PDFDocument* document, - bool safeWrite); + const PDFDocument* originalDocument, + const PDFDocument* document, + bool safeWrite, + IncrementalWriteOutcome* outcome = nullptr); /// Writes an incremental update using the supplied original bytes. This /// overload is useful for callers that already hold the source buffer and /// for byte-preservation tests. + /// \param outcome Optional; set on success to what the save actually did PDFOperationResult writeIncremental(QIODevice* device, - const QByteArray& originalData, - const PDFDocument* originalDocument, - const PDFDocument* document); + const QByteArray& originalData, + const PDFDocument* originalDocument, + const PDFDocument* document, + IncrementalWriteOutcome* outcome = nullptr); /// Chooses the default save mode for an existing document. Save As and /// destructive operations must pass the corresponding opt-out flags. diff --git a/LoopLibCore/sources/pdffilenamesanitizer.cpp b/LoopLibCore/sources/pdffilenamesanitizer.cpp index fe93a8324..7e5922832 100644 --- a/LoopLibCore/sources/pdffilenamesanitizer.cpp +++ b/LoopLibCore/sources/pdffilenamesanitizer.cpp @@ -89,11 +89,26 @@ QString PDFFilenameSanitizer::sanitize(const QString& rawFilename, const QString bool PDFFilenameSanitizer::isPathContained(const QString& resolvedPath, const QString& targetDirectory) { - const QString canonicalTarget = QDir(targetDirectory).canonicalPath(); + // A target that does not exist yet cannot be canonicalized, but callers + // legitimately validate a planned output before creating its directory. Fall + // back to the cleaned absolute path in that case: traversal is still caught, + // because cleanPath() resolves "..", and there is no symlink to resolve in a + // directory that does not exist. Note that the *file* side keeps its + // stricter rule below - a name whose parent is a symlink is not treated as + // contained even when it resolves inside the target. + QString canonicalTarget = QDir(targetDirectory).canonicalPath(); if (canonicalTarget.isEmpty()) { - // Target directory does not exist — cannot verify containment - return false; + if (targetDirectory.isEmpty()) + { + return false; + } + + canonicalTarget = QDir::cleanPath(QDir(targetDirectory).absolutePath()); + if (canonicalTarget.isEmpty()) + { + return false; + } } const QString canonicalFilePath = QFileInfo(resolvedPath).canonicalFilePath(); diff --git a/LoopLibCore/sources/pdfjbig2decoder.cpp b/LoopLibCore/sources/pdfjbig2decoder.cpp index 6ee1addb4..28375c9a5 100644 --- a/LoopLibCore/sources/pdfjbig2decoder.cpp +++ b/LoopLibCore/sources/pdfjbig2decoder.cpp @@ -3918,10 +3918,21 @@ void PDFJBIG2Bitmap::paint(const PDFJBIG2Bitmap& bitmap, int offsetX, int offset return; } - // Expand, if it is allowed and target bitmap has too low height + // Expand, if it is allowed and target bitmap has too low height. + // + // This is the one path that grows a bitmap after construction, so it is also + // the one path that escapes the dimension check every constructor performs. + // offsetY is attacker-controlled and, since region offsets became correctly + // signed, may be as large as MAX_BITMAP_SIZE - so a region placed far down a + // wide page can ask for an allocation of hundreds of megabytes (and, on a + // wide enough page, overflow the int pixel count). Validate the grown + // dimensions exactly as a constructor would. if (expandY && offsetY + bitmap.getHeight() > m_height) { - m_height = offsetY + bitmap.getHeight(); + const int expandedHeight = offsetY + bitmap.getHeight(); + checkJBIG2BitmapDimensions(m_width, expandedHeight); + + m_height = expandedHeight; m_data.resize(getPixelCount(), expandPixel); } diff --git a/LoopLibCore/sources/pdflogscrubber.cpp b/LoopLibCore/sources/pdflogscrubber.cpp index 8e08e3765..94a480a93 100644 --- a/LoopLibCore/sources/pdflogscrubber.cpp +++ b/LoopLibCore/sources/pdflogscrubber.cpp @@ -148,6 +148,59 @@ QString scrubRemainingAbsolutePaths(const QString& text) return result; } +/// Replaces credential material with a placeholder. Three shapes are covered, +/// all of which are routinely logged verbatim by libraries that assume their +/// own configuration is not sensitive: +/// - URL userinfo ("https://key:secret@host/..."), which is the shape of a +/// Sentry DSN and of most ingest/webhook endpoints; +/// - HTTP authorization values ("Bearer ", "Basic "), as they +/// appear in request dumps; +/// - key/value pairs whose key names a secret ("token=", "\"api_key\": ...", +/// "password => ..."), in JSON, assignment, or query-string form. +/// The key is kept and only the value is replaced - knowing *which* setting was +/// misconfigured is the diagnostic value; the value after it is what has to go. +/// The key vocabulary matches isSensitiveKey() in pdfartifactidentity.cpp, minus +/// the path-shaped keys that the absolute-path pass already covers. +QString scrubCredentials(const QString& text) +{ + static const QString secretKey = QStringLiteral( + "[A-Za-z0-9_.-]*(?:password|passwd|pswd|passphrase|secret|token|api[_.-]?key|apikey|" + "access[_.-]?key|private[_.-]?key|credential|authorization|dsn|license[_.-]?key)[A-Za-z0-9_.-]*"); + + // Auth scheme prefixes are consumed together with the token that follows + // them, so "Authorization: Bearer abc" collapses to a single placeholder + // instead of redacting "Bearer" and leaving "abc" behind. + static const QString authScheme = QStringLiteral("(?:Bearer|Basic|Token|Digest|APIKey)\\s+"); + + // The same scheme list minus "Token", for the unanchored pass below: after + // a secret-named key the word is unambiguous, but on its own "token" is + // ordinary English ("Unexpected token appeared") and redacting it would + // eat parser diagnostics. + static const QString bareAuthScheme = QStringLiteral("(?:Bearer|Basic|Digest|APIKey)\\s+"); + + // '<' and '>' are excluded from every value class so an already-substituted + // "" is never matched again - scrub() must stay idempotent. + static const QRegularExpression urlUserInfoPattern( + QStringLiteral(R"((?|[:=])\s*"?)(?:%2)?[^\s"',;&}\]<>]+)").arg(secretKey, authScheme), + QRegularExpression::CaseInsensitiveOption); + + // The lookahead requires at least one non-letter character, so a scheme word + // used as prose ("Basic rendering enabled") is not mistaken for a header. + static const QRegularExpression authorizationPattern( + QStringLiteral(R"(\b(%1)(?=[A-Za-z0-9._~+/=-]*[0-9._~+/=-])[A-Za-z0-9._~+/=-]{8,})").arg(bareAuthScheme), + QRegularExpression::CaseInsensitiveOption); + + QString result = text; + result.replace(urlUserInfoPattern, QStringLiteral("\\1@")); + result.replace(secretKeyValuePattern, QStringLiteral("\\1\\2")); + result.replace(authorizationPattern, QStringLiteral("\\1")); + return result; +} + QString scrubEmailAddresses(const QString& text) { static const QRegularExpression emailPattern( @@ -202,6 +255,12 @@ QString PDFLogScrubber::scrub(const QString& text) result = replaceToken(result, loginName(), QStringLiteral("")); result = replaceToken(result, QSysInfo::machineHostName(), QStringLiteral("")); + // Credentials before the path/email passes: a DSN like + // "https://key@ingest.example.com/42" would otherwise have its key eaten by + // the email pass (leaving "", which reads like user data rather than + // a leaked secret) and its project id eaten by the path pass. + result = scrubCredentials(result); + result = scrubRemainingAbsolutePaths(result); result = scrubEmailAddresses(result); result = scrubIPv4Literals(result); diff --git a/LoopLibCore/sources/pdflogscrubber.h b/LoopLibCore/sources/pdflogscrubber.h index c475b4e09..0622358ec 100644 --- a/LoopLibCore/sources/pdflogscrubber.h +++ b/LoopLibCore/sources/pdflogscrubber.h @@ -48,11 +48,15 @@ class LOOPLIBCORESHARED_EXPORT PDFLogScrubber PDFLogScrubber() = delete; /// Scrubs \p text of the home and temp directories, the login name, the - /// machine host name, any remaining absolute path (Windows, UNC, or POSIX), - /// email addresses, and IPv4/IPv6 literals. Order matters: the home/temp - /// directory and login name/host name passes run first so a leftover - /// absolute path outside those roots is still caught by the generic path - /// pass. Applying scrub() to already-scrubbed text is a no-op. + /// machine host name, credential material (URL userinfo such as a Sentry + /// DSN, HTTP authorization values, and secret-named key/value pairs), any + /// remaining absolute path (Windows, UNC, or POSIX), email addresses, and + /// IPv4/IPv6 literals. Order matters: the home/temp directory and login + /// name/host name passes run first so a leftover absolute path outside + /// those roots is still caught by the generic path pass, and the credential + /// pass runs before the email/path passes so a DSN is reported as a leaked + /// secret (``) rather than as user data (``). Applying + /// scrub() to already-scrubbed text is a no-op. /// \param text Text to scrub static QString scrub(const QString& text); }; diff --git a/LoopLibCore/sources/pdfnametreeloader.h b/LoopLibCore/sources/pdfnametreeloader.h index d351fec6f..83839ed3e 100644 --- a/LoopLibCore/sources/pdfnametreeloader.h +++ b/LoopLibCore/sources/pdfnametreeloader.h @@ -27,6 +27,7 @@ #include #include +#include namespace pdf { @@ -41,21 +42,58 @@ class PDFNameTreeLoader using MappedObjects = std::map; using LoadMethod = std::function; + /// Longest accepted key in a name tree. Keys are attacker-controlled strings + /// that are stored verbatim in the document model (named destinations, + /// embedded-file names, multimedia assets), and nothing downstream bounds + /// them. A key past this length is not a name, it is a payload. + static constexpr int MAXIMUM_NAME_LENGTH = 4096; + + /// Largest accepted number of entries across the whole tree. Bounds the + /// "many small names" shape that the per-name cap alone does not. + static constexpr size_t MAXIMUM_ENTRY_COUNT = 65536; + + /// Deepest accepted Kids nesting. Together with the visited-node set below + /// this keeps a malformed tree from recursing without bound. + static constexpr int MAXIMUM_TREE_DEPTH = 64; + /// Parses the name tree and loads its items into the map. Some errors are ignored, /// e.g. when kid is null. Objects are retrieved by \p loadMethod. + /// + /// The tree is traversed defensively: a Kids chain that points back at a node + /// it already visited (directly or through a cycle) is not followed a second + /// time, nesting is bounded, and over-long or over-numerous keys are skipped. + /// A malformed name tree therefore costs a truncated map rather than + /// unbounded recursion or unbounded memory. /// \param storage Object storage /// \param root Root of the name tree /// \param loadMethod Parsing method, which retrieves parsed object static MappedObjects parse(const PDFObjectStorage* storage, const PDFObject& root, const LoadMethod& loadMethod) { MappedObjects result; - parseImpl(result, storage, root, loadMethod); + std::set visitedNodes; + parseImpl(result, storage, root, loadMethod, visitedNodes, 0); return result; } private: - static void parseImpl(MappedObjects& objects, const PDFObjectStorage* storage, const PDFObject& root, const LoadMethod& loadMethod) + static void parseImpl(MappedObjects& objects, + const PDFObjectStorage* storage, + const PDFObject& root, + const LoadMethod& loadMethod, + std::set& visitedNodes, + int depth) { + if (depth > MAXIMUM_TREE_DEPTH) + { + return; + } + + if (root.isReference() && !visitedNodes.insert(root.getReference()).second) + { + // Already expanded this node: the tree is cyclic. + return; + } + if (const PDFDictionary* dictionary = storage->getDictionaryFromObject(root)) { // Jakub Melka: First, load the objects into the map @@ -75,7 +113,18 @@ class PDFNameTreeLoader continue; } - objects[name.getString()] = loadMethod(storage, namedItemsArray->getItem(valueIndex)); + const QByteArray key = name.getString(); + if (key.size() > MAXIMUM_NAME_LENGTH) + { + continue; + } + + if (objects.size() >= MAXIMUM_ENTRY_COUNT && !objects.count(key)) + { + continue; + } + + objects[key] = loadMethod(storage, namedItemsArray->getItem(valueIndex)); } } @@ -87,7 +136,7 @@ class PDFNameTreeLoader const size_t count = kidsArray->getCount(); for (size_t i = 0; i < count; ++i) { - parseImpl(objects, storage, kidsArray->getItem(i), loadMethod); + parseImpl(objects, storage, kidsArray->getItem(i), loadMethod, visitedNodes, depth + 1); } } } diff --git a/LoopLibCore/sources/pdfobjectutils.h b/LoopLibCore/sources/pdfobjectutils.h index a0fa5d99c..7e9dfebeb 100644 --- a/LoopLibCore/sources/pdfobjectutils.h +++ b/LoopLibCore/sources/pdfobjectutils.h @@ -66,6 +66,12 @@ class PDFMarkedObjectsContext inline explicit PDFMarkedObjectsContext() = default; inline bool isMarked(PDFObjectReference reference) const { return m_markedReferences.count(reference); } + + /// Number of references currently marked. Marks are held by + /// PDFMarkedObjectsLock for exactly the span of the traversal that owns + /// them, so for a depth-first walk this is the depth of the current path - + /// which is what a recursive parser needs to bound its own recursion. + inline size_t getMarkedCount() const { return m_markedReferences.size(); } inline void mark(PDFObjectReference reference) { m_markedReferences.insert(reference); } inline void unmark(PDFObjectReference reference) { m_markedReferences.erase(reference); } diff --git a/LoopLibCore/sources/pdfsafefilewriter.cpp b/LoopLibCore/sources/pdfsafefilewriter.cpp index d1ad7e959..6683229b6 100644 --- a/LoopLibCore/sources/pdfsafefilewriter.cpp +++ b/LoopLibCore/sources/pdfsafefilewriter.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -134,19 +135,36 @@ QString PDFSafeFileWriter::makeUniqueFileName(const QString& fileName) const QString suffix = info.suffix(); const QString directory = info.absolutePath(); - // Bounded probe; the "base (n).ext" cascade frees the path quickly in practice. - for (qint64 n = 1; n < 100000; ++n) + // The "base (n).ext" cascade frees a path within a handful of probes for any + // directory a person assembled. A directory pre-filled with those names - by + // a document whose attachments are all called the same thing, say - would + // otherwise cost a hundred thousand synchronous stat() calls before giving + // up, so the sequential probe is short and a random suffix takes over. + constexpr int SEQUENTIAL_PROBE_LIMIT = 128; + constexpr int RANDOM_PROBE_LIMIT = 64; + + auto candidateFor = [&](const QString& discriminator) { - QString candidate; - if (suffix.isEmpty()) - { - candidate = QDir(directory).filePath(QStringLiteral("%1 (%2)").arg(baseName).arg(n)); - } - else + return suffix.isEmpty() + ? QDir(directory).filePath(QStringLiteral("%1 (%2)").arg(baseName, discriminator)) + : QDir(directory).filePath(QStringLiteral("%1 (%2).%3").arg(baseName, discriminator, suffix)); + }; + + for (int n = 1; n <= SEQUENTIAL_PROBE_LIMIT; ++n) + { + const QString candidate = candidateFor(QString::number(n)); + if (!QFile::exists(candidate)) { - candidate = QDir(directory).filePath(QStringLiteral("%1 (%2).%3").arg(baseName).arg(n).arg(suffix)); + return candidate; } + } + // Random discriminators also break the tie between two processes that start + // probing the same directory at the same moment: sequential names make them + // converge on the same candidate, random ones do not. + for (int n = 0; n < RANDOM_PROBE_LIMIT; ++n) + { + const QString candidate = candidateFor(QString::number(QRandomGenerator::global()->generate(), 16)); if (!QFile::exists(candidate)) { return candidate; diff --git a/LoopLibCore/sources/pdfstructuretree.cpp b/LoopLibCore/sources/pdfstructuretree.cpp index 697406c9e..14ea1f5c8 100644 --- a/LoopLibCore/sources/pdfstructuretree.cpp +++ b/LoopLibCore/sources/pdfstructuretree.cpp @@ -35,6 +35,11 @@ namespace pdf { +/// Deepest structure-tree nesting this parser will follow. Structure trees model +/// document semantics (sections, paragraphs, table cells); real ones are tens of +/// levels deep, not thousands, and the parser is recursive. +static constexpr size_t MAXIMUM_STRUCTURE_TREE_DEPTH = 512; + /// Attribute definition structure struct PDFStructureTreeAttributeDefinition { @@ -662,6 +667,15 @@ PDFStructureTree::ParentTreeEntry PDFStructureTree::getParentTreeEntry(PDFIntege PDFStructureItemPointer PDFStructureItem::parse(const PDFObjectStorage* storage, PDFObject object, PDFMarkedObjectsContext* context, PDFStructureItem* parent) { + // A cyclic structure tree is already refused by the marked-objects context, + // but an acyclic chain of tens of thousands of distinct StructElem nodes is + // not a cycle - it is just deep, and this parser is recursive. The marked + // set holds exactly the current path, so its size is that path's depth. + if (context && context->getMarkedCount() >= MAXIMUM_STRUCTURE_TREE_DEPTH) + { + return nullptr; + } + if (const PDFDictionary* dictionary = storage->getDictionaryFromObject(object)) { PDFDocumentDataLoaderDecorator loader(storage); diff --git a/PdfTool/ocrsidecarprotocol.h b/PdfTool/ocrsidecarprotocol.h index 0d3ae7107..7484fc9ec 100644 --- a/PdfTool/ocrsidecarprotocol.h +++ b/PdfTool/ocrsidecarprotocol.h @@ -32,6 +32,19 @@ namespace pdftool::ocr { +/// The OCR option defaults, in one place. They are consumed both by the +/// capability-discovery table (which tells callers what the defaults are) and by +/// the command-line parser (which applies them), so a single definition is what +/// keeps the advertised default and the applied default from drifting apart. +/// +/// Language codes are ISO 639-1 ("en", "de"), matching what the loop-ocr sidecar +/// normalizes to in engine.py::normalize_languages. Nothing in Loop emits ISO +/// 639-2 ("eng", "deu"); if that ever changes, convert at this boundary rather +/// than teaching downstream consumers both code sets. +inline constexpr QLatin1StringView DEFAULT_OCR_LANGUAGES = QLatin1StringView("en"); +inline constexpr QLatin1StringView DEFAULT_OCR_DPI = QLatin1StringView("300"); +inline constexpr QLatin1StringView DEFAULT_OCR_MIN_TEXT_CHARS = QLatin1StringView("20"); + inline QStringList normalizeLanguages(const QString& specification) { QStringList languages; @@ -46,7 +59,7 @@ inline QStringList normalizeLanguages(const QString& specification) if (languages.isEmpty()) { - languages.append(QStringLiteral("en")); + languages.append(QString(DEFAULT_OCR_LANGUAGES)); } else { diff --git a/PdfTool/pdftoolabstractapplication.cpp b/PdfTool/pdftoolabstractapplication.cpp index 927c43f7d..3cf62ffb6 100644 --- a/PdfTool/pdftoolabstractapplication.cpp +++ b/PdfTool/pdftoolabstractapplication.cpp @@ -24,6 +24,7 @@ #include "pdfdocumentreader.h" #include "pdfsafefilewriter.h" #include "pdfutils.h" +#include "ocrsidecarprotocol.h" #include #include @@ -334,6 +335,10 @@ QList PDFToolAbstractApplication::describeOptions(Optio add(QStringLiteral("output-intent"), { QStringLiteral("--output-intent") }, QStringLiteral("policy"), PDFToolValueType::Enum, { QStringLiteral("preserve-matching"), QStringLiteral("replace") }, QStringLiteral("replace")); } + if (optionFlags.testFlag(EmptyResultPolicy)) + { + add(QStringLiteral("fail-if-empty"), { QStringLiteral("--fail-if-empty") }, {}, PDFToolValueType::Boolean); + } if (optionFlags.testFlag(DestructiveWrite)) { add(QStringLiteral("dry-run"), { QStringLiteral("--dry-run") }, {}, PDFToolValueType::Boolean); @@ -374,9 +379,9 @@ QList PDFToolAbstractApplication::describeOptions(Optio if (optionFlags.testFlag(OcrOptions)) { add(QStringLiteral("sidecar"), { QStringLiteral("--sidecar") }, QStringLiteral("path"), PDFToolValueType::Path); - add(QStringLiteral("dpi"), { QStringLiteral("--dpi") }, QStringLiteral("dpi"), PDFToolValueType::Integer, {}, QStringLiteral("300")); - add(QStringLiteral("languages"), { QStringLiteral("--languages") }, QStringLiteral("codes"), PDFToolValueType::Csv, {}, QStringLiteral("en")); - add(QStringLiteral("min-text-chars"), { QStringLiteral("--min-text-chars") }, QStringLiteral("n"), PDFToolValueType::Integer, {}, QStringLiteral("20")); + add(QStringLiteral("dpi"), { QStringLiteral("--dpi") }, QStringLiteral("dpi"), PDFToolValueType::Integer, {}, QString(pdftool::ocr::DEFAULT_OCR_DPI)); + add(QStringLiteral("languages"), { QStringLiteral("--languages") }, QStringLiteral("codes"), PDFToolValueType::Csv, {}, QString(pdftool::ocr::DEFAULT_OCR_LANGUAGES)); + add(QStringLiteral("min-text-chars"), { QStringLiteral("--min-text-chars") }, QStringLiteral("n"), PDFToolValueType::Integer, {}, QString(pdftool::ocr::DEFAULT_OCR_MIN_TEXT_CHARS)); } if (optionFlags.testFlag(VerifyRedaction)) { @@ -643,6 +648,7 @@ QStringList PDFToolAbstractApplication::describeCapabilities(Options optionFlags add(Redact, QStringLiteral("document.redact")); add(VerifyRedaction, QStringLiteral("document.redaction.verify")); add(DestructiveWrite, QStringLiteral("document.write.destructive")); + add(EmptyResultPolicy, QStringLiteral("output.empty-result.policy")); add(AddBleed, QStringLiteral("fixup.add-bleed")); add(FlattenTransparency, QStringLiteral("fixup.flatten-transparency")); add(RgbToCmyk, QStringLiteral("fixup.rgb-to-cmyk")); @@ -796,6 +802,12 @@ void PDFToolAbstractApplication::initializeCommandLineParser(QCommandLineParser* addDescribedOption(parser, optionDescriptors, QStringLiteral("output-intent"), QStringLiteral("OutputIntent policy: replace|preserve-matching.")); } + if (optionFlags.testFlag(EmptyResultPolicy)) + { + addDescribedOption(parser, optionDescriptors, QStringLiteral("fail-if-empty"), + QStringLiteral("Exit with 1 (findings) when the command extracted nothing.")); + } + if (optionFlags.testFlag(DestructiveWrite)) { // add-bleed keeps --overwrite/--dry-run/--report shared with unite/separate via @@ -837,9 +849,9 @@ void PDFToolAbstractApplication::initializeCommandLineParser(QCommandLineParser* if (optionFlags.testFlag(OcrOptions)) { parser->addOption(QCommandLineOption("sidecar", "Path to LoopOcrService executable.", "path")); - parser->addOption(QCommandLineOption("dpi", "Rasterization DPI for OCR pages.", "dpi", "300")); - parser->addOption(QCommandLineOption("languages", "Comma-separated EasyOCR language codes.", "codes", "en")); - parser->addOption(QCommandLineOption("min-text-chars", "Skip OCR when page has at least this many non-whitespace characters.", "n", "20")); + parser->addOption(QCommandLineOption("dpi", "Rasterization DPI for OCR pages.", "dpi", QString(pdftool::ocr::DEFAULT_OCR_DPI))); + parser->addOption(QCommandLineOption("languages", "Comma-separated EasyOCR language codes (ISO 639-1).", "codes", QString(pdftool::ocr::DEFAULT_OCR_LANGUAGES))); + parser->addOption(QCommandLineOption("min-text-chars", "Skip OCR when page has at least this many non-whitespace characters.", "n", QString(pdftool::ocr::DEFAULT_OCR_MIN_TEXT_CHARS))); } if (optionFlags.testFlag(VerifyRedaction)) @@ -2217,6 +2229,11 @@ PDFToolOptions PDFToolAbstractApplication::getOptions(QCommandLineParser* parser options.encryptionPermissions = parser->value("enc-permissions").toUInt(); } + if (optionFlags.testFlag(EmptyResultPolicy)) + { + options.failIfEmpty = parser->isSet("fail-if-empty"); + } + if (optionFlags.testFlag(DestructiveWrite)) { options.destructiveDryRun = parser->isSet("dry-run"); @@ -2313,6 +2330,40 @@ bool PDFToolAbstractApplication::readDocument(const PDFToolOptions& options, pdf return true; } +PDFToolExitCode PDFToolAbstractApplication::reportEmptyResult(const PDFToolOptions& options, + const QString& subject, + PDFToolExitCode successCode) const +{ + const bool fail = options.failIfEmpty; + const QJsonObject context{ { QStringLiteral("subject"), subject }, + { QStringLiteral("fail_if_empty"), fail } }; + + if (fail) + { + reportDiagnostic(options, + PDFToolDiagnosticSeverity::Error, + QStringLiteral("output.empty-result"), + PDFToolTranslationContext::tr("No %1 were extracted from document '%2', and --fail-if-empty was requested.").arg(subject, options.document), + context); + return PDFToolExitCode::Findings; + } + + // Without the flag this stays a machine-readable note only: extraction + // commands are informational by default and must not start writing to stderr + // on documents that simply have nothing to extract. + if (options.executionContext) + { + PDFToolDiagnostic diagnostic; + diagnostic.severity = PDFToolDiagnosticSeverity::Info; + diagnostic.code = QStringLiteral("output.empty-result"); + diagnostic.message = PDFToolTranslationContext::tr("No %1 were extracted from document '%2'.").arg(subject, options.document); + diagnostic.context = context; + options.executionContext->addDiagnostic(std::move(diagnostic)); + } + + return successCode; +} + void PDFToolAbstractApplication::reportDiagnostic(const PDFToolOptions& options, PDFToolDiagnosticSeverity severity, const QString& code, diff --git a/PdfTool/pdftoolabstractapplication.h b/PdfTool/pdftoolabstractapplication.h index 2a15260db..b33ad7a3c 100644 --- a/PdfTool/pdftoolabstractapplication.h +++ b/PdfTool/pdftoolabstractapplication.h @@ -249,6 +249,11 @@ struct PDFToolOptions bool destructiveReport = false; bool destructiveOverwrite = false; + // Shared empty-result policy (fetch-images, fetch-text, attachments --save). + // Extraction commands are informational by default - "this document has no + // figures" is a legitimate answer - so the fail-closed reading is opt-in. + bool failIfEmpty = false; + // For option 'PreflightProfile' QString preflightProfilePath; QString preflightJobContextPath; @@ -399,6 +404,7 @@ class PDFToolAbstractApplication Repair = 0x800000000ULL, ///< Transactional prepress-safe repair operation ActionList = 0x1000000000ULL, ///< Reusable declarative Action List execution RenderPage = 0x4000000000ULL, ///< Settings for render-page STCH contract + EmptyResultPolicy = 0x8000000000ULL, ///< Shared --fail-if-empty for extraction commands }; Q_DECLARE_FLAGS(Options, Option) @@ -433,6 +439,20 @@ class PDFToolAbstractApplication const QString& message, QJsonObject context = QJsonObject()) const; + /// Reports that an extraction command completed without producing anything and + /// returns the exit code the command should use. Extraction is informational by + /// default: a document with no figures is not an error, so without + /// --fail-if-empty this records an `output.empty-result` note and returns + /// \p successCode. With --fail-if-empty it raises the same code to an error and + /// returns PDFToolExitCode::Findings, so a pipeline that gates on "figures were + /// produced" cannot be green-lit by an empty output directory. + /// \param options Options (carries execution context, output style, and the flag) + /// \param subject What was not produced, for the message (e.g. "images") + /// \param successCode Exit code to return when the flag was not requested + PDFToolExitCode reportEmptyResult(const PDFToolOptions& options, + const QString& subject, + PDFToolExitCode successCode = PDFToolExitCode::Success) const; + /// Tries to read the document. If document is successfully read, true is returned, /// if error occurs, then false is returned. Optionally, original document content /// can also be retrieved. diff --git a/PdfTool/pdftoolattachments.cpp b/PdfTool/pdftoolattachments.cpp index 2438d885d..b82639509 100644 --- a/PdfTool/pdftoolattachments.cpp +++ b/PdfTool/pdftoolattachments.cpp @@ -79,6 +79,10 @@ PDFToolExitCode PDFToolAttachmentsApplication::execute(const PDFToolOptions& opt QMimeDatabase mimeDatabase; + const bool saveRequested = options.attachmentsSaveAll || + !options.attachmentsSaveNumber.isEmpty() || + !options.attachmentsSaveFileName.isEmpty(); + size_t savedFileCount = 0; size_t no = 1; std::vector embeddedFiles; @@ -168,6 +172,14 @@ PDFToolExitCode PDFToolAttachmentsApplication::execute(const PDFToolOptions& opt { PDFConsole::writeText(formatter.getString(), options.outputCodec); } + + // Two different situations reach this branch: a plain listing, and a + // --save-* selection that matched nothing. Both produced no file, which + // is what --fail-if-empty asks about. + if (embeddedFiles.empty() || saveRequested) + { + return reportEmptyResult(options, PDFToolTranslationContext::tr("attachments")); + } } else { @@ -306,7 +318,7 @@ PDFToolExitCode PDFToolAttachmentsApplication::execute(const PDFToolOptions& opt PDFToolAbstractApplication::Options PDFToolAttachmentsApplication::getOptionsFlags() const { - return ConsoleFormat | OpenDocument | Attachments | DestructiveWrite; + return ConsoleFormat | OpenDocument | Attachments | DestructiveWrite | EmptyResultPolicy; } } // namespace pdftool diff --git a/PdfTool/pdftoolfetchimages.cpp b/PdfTool/pdftoolfetchimages.cpp index 3fbea9bc2..c9daa93fe 100644 --- a/PdfTool/pdftoolfetchimages.cpp +++ b/PdfTool/pdftoolfetchimages.cpp @@ -311,12 +311,25 @@ PDFToolExitCode PDFToolFetchImages::execute(const PDFToolOptions& options) auto imageRange = pdf::PDFIntegerRange(0, m_images.size()); pdf::PDFExecutionPolicy::execute(pdf::PDFExecutionPolicy::Scope::Page, imageRange.begin(), imageRange.end(), saveImage); - return m_failedWrites.load() > 0 ? PDFToolExitCode::PartialOutput : PDFToolExitCode::Success; + if (m_failedWrites.load() > 0) + { + return PDFToolExitCode::PartialOutput; + } + + // A vector-only document legitimately yields no images; a caller that gates a + // release on "figures were produced" must not be green-lit by an empty + // output directory, so --fail-if-empty turns that into a finding. + if (m_images.empty()) + { + return reportEmptyResult(options, PDFToolTranslationContext::tr("images")); + } + + return PDFToolExitCode::Success; } PDFToolAbstractApplication::Options PDFToolFetchImages::getOptionsFlags() const { - return ConsoleFormat | OpenDocument | PageSelector | ImageWriterSettings | ImageExportSettingsFiles | ColorManagementSystem | DestructiveWrite; + return ConsoleFormat | OpenDocument | PageSelector | ImageWriterSettings | ImageExportSettingsFiles | ColorManagementSystem | DestructiveWrite | EmptyResultPolicy; } void PDFToolFetchImages::onImageExtracted(pdf::PDFInteger pageIndex, pdf::PDFInteger order, const QImage& image) diff --git a/PdfTool/pdftoolfetchtext.cpp b/PdfTool/pdftoolfetchtext.cpp index 1dbc90896..1ddfd24f8 100644 --- a/PdfTool/pdftoolfetchtext.cpp +++ b/PdfTool/pdftoolfetchtext.cpp @@ -80,6 +80,10 @@ PDFToolExitCode PDFToolFetchTextApplication::execute(const PDFToolOptions& optio formatter.beginDocument("text-extraction", QString()); formatter.endl(); + // Counts the page text actually emitted, so --fail-if-empty tracks what the + // caller receives rather than what the flow happened to contain. + qsizetype extractedCharacters = 0; + for (const pdf::PDFDocumentTextFlow::Item& item : documentTextFlow.getItems()) { if (item.flags.testFlag(pdf::PDFDocumentTextFlow::StructureItemStart)) @@ -102,6 +106,13 @@ PDFToolExitCode PDFToolFetchTextApplication::execute(const PDFToolOptions& optio if (showText) { formatter.writeText("text", item.text); + + // Only page content counts: page-number and structure markers are + // emitted even for a document that contains no text at all. + if (item.flags.testFlag(pdf::PDFDocumentTextFlow::Text)) + { + extractedCharacters += item.text.size(); + } } } @@ -135,12 +146,20 @@ PDFToolExitCode PDFToolFetchTextApplication::execute(const PDFToolOptions& optio PDFConsole::writeText(formatter.getString(), options.outputCodec); } + // A document whose selected pages carry no text at all is a legitimate + // answer, but a pipeline that expects text needs to be able to tell that + // case apart from "extraction ran and produced nothing". + if (extractedCharacters == 0) + { + return reportEmptyResult(options, PDFToolTranslationContext::tr("text")); + } + return PDFToolExitCode::Success; } PDFToolAbstractApplication::Options PDFToolFetchTextApplication::getOptionsFlags() const { - return ConsoleFormat | OpenDocument | PageSelector | TextAnalysis | TextShow; + return ConsoleFormat | OpenDocument | PageSelector | TextAnalysis | TextShow | EmptyResultPolicy; } } // namespace pdftool diff --git a/PdfTool/pdftoolocr.cpp b/PdfTool/pdftoolocr.cpp index 9c434b9fd..36b30bd2d 100644 --- a/PdfTool/pdftoolocr.cpp +++ b/PdfTool/pdftoolocr.cpp @@ -39,6 +39,7 @@ #include #include #include +#include #include namespace pdftool @@ -199,6 +200,14 @@ bool renderPageToPng(pdf::PDFDocument* document, renderError = writer.errorString(); return; } + + // The staging directory is already private (QTemporaryDir uses mkdtemp, + // i.e. 0700), but the raster carries the document's content, so make the + // file itself owner-only too rather than relying on the directory mode + // alone. A failure here is not fatal - the enclosing directory still + // keeps other local users out. + QFile::setPermissions(outputPath, QFileDevice::ReadOwner | QFileDevice::WriteOwner); + rendered = true; }; diff --git a/UnitTests/CMakeLists.txt b/UnitTests/CMakeLists.txt index 3ab2ae461..c4fb92f75 100644 --- a/UnitTests/CMakeLists.txt +++ b/UnitTests/CMakeLists.txt @@ -512,7 +512,8 @@ add_executable(UnitTestsPdfToolContract target_link_libraries(UnitTestsPdfToolContract PRIVATE Qt6::Core Qt6::Test) add_dependencies(UnitTestsPdfToolContract PdfTool) target_compile_definitions(UnitTestsPdfToolContract PRIVATE - PDFTOOL_EXECUTABLE_PATH="$") + PDFTOOL_EXECUTABLE_PATH="$" + LOOP_PREFLIGHT_SOURCE_DIR="${CMAKE_SOURCE_DIR}/loop-preflight") set_target_properties(UnitTestsPdfToolContract PROPERTIES WIN32_EXECUTABLE OFF diff --git a/UnitTests/tst_budgetexhaustiontest.cpp b/UnitTests/tst_budgetexhaustiontest.cpp index b7334d866..fbbbea269 100644 --- a/UnitTests/tst_budgetexhaustiontest.cpp +++ b/UnitTests/tst_budgetexhaustiontest.cpp @@ -29,6 +29,7 @@ #include "pdfthinpartprobe.h" #include "preflightengine.h" +#include #include #include #include @@ -58,6 +59,8 @@ private slots: void generatedPdfCorpusHasProductionReaderInputs(); void preflightEvidenceBudgetIsIncomplete(); void rasterSizeBudgetIsIncomplete(); + void permissiveRecoveryRefusesSparseObjectNumbering(); + void permissiveRecoveryCarriesASourceDigest(); }; namespace @@ -559,6 +562,53 @@ void BudgetExhaustionTest::generatedPdfCorpusHasProductionReaderInputs() QVERIFY(!reader.getErrorMessage().contains(QStringLiteral("budget"), Qt::CaseInsensitive)); } +namespace +{ + +// A damaged document: no xref, no trailer offset, so the reader falls back to +// permissive recovery and rebuilds the object table from the object headers it +// can find. +QByteArray damagedDocument(const QByteArray& firstObjectNumber) +{ + QByteArray data = QByteArrayLiteral("%PDF-1.7\n"); + data += firstObjectNumber + QByteArrayLiteral(" 0 obj\n<< /Type /Catalog >>\nendobj\n"); + data += QByteArrayLiteral("trailer\n<< /Root ") + firstObjectNumber + QByteArrayLiteral(" 0 R >>\n%%EOF\n"); + return data; +} + +} // namespace + +void BudgetExhaustionTest::permissiveRecoveryRefusesSparseObjectNumbering() +{ + // One recovered object numbered 9999999 would otherwise resize the dense + // object table to ten million entries - an allocation the document asks for + // simply by naming a large object number. + pdf::PDFDocumentReader reader(nullptr, [](bool*) + { return QString(); }, true, false); + reader.readFromBuffer(damagedDocument(QByteArrayLiteral("9999999"))); + + QCOMPARE(reader.getReadingResult(), pdf::PDFDocumentReader::Result::Failed); +} + +void BudgetExhaustionTest::permissiveRecoveryCarriesASourceDigest() +{ + // A permissively recovered document must still carry the digest of the bytes + // it came from: PDFDocumentWriter::writeIncremental refuses to append when + // the source changed, and an empty digest silently disables that guard. + const QByteArray bytes = damagedDocument(QByteArrayLiteral("1")); + + pdf::PDFDocumentReader reader(nullptr, [](bool*) + { return QString(); }, true, false); + const pdf::PDFDocument document = reader.readFromBuffer(bytes); + + if (reader.getReadingResult() != pdf::PDFDocumentReader::Result::OK) + { + QSKIP("Permissive recovery did not accept the synthetic damaged document."); + } + + QCOMPARE(document.getSourceDataHash(), QCryptographicHash::hash(bytes, QCryptographicHash::Sha256)); +} + void BudgetExhaustionTest::generatedCorpusIsIncompleteNeverPass() { pdf::PDFProcessingLimits limits; diff --git a/UnitTests/tst_diagnosticstest.cpp b/UnitTests/tst_diagnosticstest.cpp index d3198c928..4a48eb701 100644 --- a/UnitTests/tst_diagnosticstest.cpp +++ b/UnitTests/tst_diagnosticstest.cpp @@ -97,6 +97,10 @@ private slots: void scrubber_windowsAbsolutePath(); void scrubber_uncPath(); void scrubber_posixAbsolutePath_dropsBasenameKeepsExtension(); + void scrubber_sentryDsn(); + void scrubber_authorizationHeader(); + void scrubber_secretKeyValuePairs(); + void scrubber_keepsNonSecretDiagnostics(); void scrubber_idempotent(); void scrubber_passthroughWhenNoMatches(); @@ -213,9 +217,65 @@ void DiagnosticsTest::scrubber_posixAbsolutePath_dropsBasenameKeepsExtension() QVERIFY(scrubbed.contains(QStringLiteral(""))); } +void DiagnosticsTest::scrubber_sentryDsn() +{ + const QString scrubbed = pdf::PDFLogScrubber::scrub( + QStringLiteral("Sentry init failed for https://0123456789abcdef@o42.ingest.sentry.io/1337")); + + QVERIFY(!scrubbed.contains(QStringLiteral("0123456789abcdef"))); + QVERIFY(scrubbed.contains(QStringLiteral(""))); + + const QString withPassword = pdf::PDFLogScrubber::scrub( + QStringLiteral("Connecting to https://svcuser:hunter2@ingest.example.com/api")); + + QVERIFY(!withPassword.contains(QStringLiteral("hunter2"))); + QVERIFY(!withPassword.contains(QStringLiteral("svcuser"))); + QVERIFY(withPassword.contains(QStringLiteral(""))); +} + +void DiagnosticsTest::scrubber_authorizationHeader() +{ + const QString headerLine = pdf::PDFLogScrubber::scrub( + QStringLiteral("Authorization: Bearer eyJhbGciOiJIUzI1NiJ9.payload.signature")); + + QVERIFY(!headerLine.contains(QStringLiteral("eyJhbGciOiJIUzI1NiJ9"))); + QVERIFY(headerLine.contains(QStringLiteral(""))); + // The key survives: knowing *which* header was set is the diagnostic value. + QVERIFY(headerLine.contains(QStringLiteral("Authorization"))); + + const QString bareScheme = pdf::PDFLogScrubber::scrub(QStringLiteral("Retrying with Basic dXNlcjpwYXNzd29yZA==")); + QVERIFY(!bareScheme.contains(QStringLiteral("dXNlcjpwYXNzd29yZA=="))); + QVERIFY(bareScheme.contains(QStringLiteral(""))); +} + +void DiagnosticsTest::scrubber_secretKeyValuePairs() +{ + const QString json = pdf::PDFLogScrubber::scrub(QStringLiteral("{\"api_key\": \"sk-live-abcdef123456\"}")); + QVERIFY(!json.contains(QStringLiteral("sk-live-abcdef123456"))); + QVERIFY(json.contains(QStringLiteral(""))); + QVERIFY(json.contains(QStringLiteral("api_key"))); + + const QString assignment = pdf::PDFLogScrubber::scrub(QStringLiteral("token=abc123def456 retries=3")); + QVERIFY(!assignment.contains(QStringLiteral("abc123def456"))); + QVERIFY(assignment.contains(QStringLiteral(""))); + // Only the secret value is replaced - neighbouring diagnostics survive. + QVERIFY(assignment.contains(QStringLiteral("retries=3"))); + + const QString password = pdf::PDFLogScrubber::scrub(QStringLiteral("password => s3cr3t!")); + QVERIFY(!password.contains(QStringLiteral("s3cr3t"))); + QVERIFY(password.contains(QStringLiteral(""))); +} + +void DiagnosticsTest::scrubber_keepsNonSecretDiagnostics() +{ + const QString text = QStringLiteral("Cannot read object. Unexpected token appeared. count=17"); + QCOMPARE(pdf::PDFLogScrubber::scrub(text), text); +} + void DiagnosticsTest::scrubber_idempotent() { - const QString text = QStringLiteral("User jane.doe@example.com opened /srv/documents/Report.pdf from 203.0.113.42"); + const QString text = QStringLiteral("User jane.doe@example.com opened /srv/documents/Report.pdf from 203.0.113.42 " + "with token=abc123def456 via https://key@ingest.example.com/9"); const QString once = pdf::PDFLogScrubber::scrub(text); const QString twice = pdf::PDFLogScrubber::scrub(once); QCOMPARE(twice, once); diff --git a/UnitTests/tst_filenamesanitizertest.cpp b/UnitTests/tst_filenamesanitizertest.cpp index 32a06f21c..1131e1a1c 100644 --- a/UnitTests/tst_filenamesanitizertest.cpp +++ b/UnitTests/tst_filenamesanitizertest.cpp @@ -25,6 +25,7 @@ #include #include +#include #include class FilenameSanitizerTest : public QObject @@ -48,6 +49,7 @@ private slots: void test_isPathContained_safe(); void test_isPathContained_traversal(); void test_isPathContained_symlinkParent(); + void test_isPathContained_targetNotCreatedYet(); void test_attachmentOpenPath_contained(); }; @@ -180,6 +182,23 @@ void FilenameSanitizerTest::test_isPathContained_symlinkParent() QVERIFY(!pdf::PDFFilenameSanitizer::isPathContained(escaped, realTarget)); } +void FilenameSanitizerTest::test_isPathContained_targetNotCreatedYet() +{ + // Callers validate a planned output before creating its directory. That must + // not read as "escapes the target" simply because nothing exists yet. + QTemporaryDir tempDir; + QVERIFY(tempDir.isValid()); + + const QString target = tempDir.filePath(QStringLiteral("not-created-yet")); + QVERIFY(!QFileInfo::exists(target)); + + QVERIFY(pdf::PDFFilenameSanitizer::isPathContained(target + QStringLiteral("/file.pdf"), target)); + + // Traversal is still refused without the directory existing. + QVERIFY(!pdf::PDFFilenameSanitizer::isPathContained(target + QStringLiteral("/../../escape.pdf"), target)); + QVERIFY(!pdf::PDFFilenameSanitizer::isPathContained(target, target)); +} + void FilenameSanitizerTest::test_attachmentOpenPath_contained() { QTemporaryDir tempDir; diff --git a/UnitTests/tst_incrementalsavetest.cpp b/UnitTests/tst_incrementalsavetest.cpp index beeb13c7e..7efaa5326 100644 --- a/UnitTests/tst_incrementalsavetest.cpp +++ b/UnitTests/tst_incrementalsavetest.cpp @@ -35,6 +35,8 @@ class IncrementalSaveTest : public QObject private slots: void preservesOriginalPrefixAndChangedObjects(); void rejectsChangedSourceBytes(); + void reportsWhetherTheSaveAppendedOrOnlyCopied(); + void refusalsNameTheirReason(); void selectsSafeWritePolicy(); void signedPdfIncrementalSave_preservesSignedPrefix(); void explicitPoliciesCannotBeDowngradedToIncremental(); @@ -135,6 +137,76 @@ void IncrementalSaveTest::rejectsChangedSourceBytes() QVERIFY(output.data().isEmpty()); } +void IncrementalSaveTest::reportsWhetherTheSaveAppendedOrOnlyCopied() +{ + const QByteArray originalData = writeDocument(createDocument()); + pdf::PDFDocumentReader reader(nullptr, [](bool*) + { return QString(); }, true, false); + const pdf::PDFDocument original = reader.readFromBuffer(originalData); + QVERIFY(reader.getReadingResult() == pdf::PDFDocumentReader::Result::OK); + + // A real change appends. + { + const pdf::PDFDocumentPointer modified = createModifiedDocument(original); + QVERIFY(modified); + + pdf::PDFDocumentWriter writer(nullptr); + QBuffer output; + output.open(QIODevice::WriteOnly); + + auto outcome = pdf::PDFDocumentWriter::IncrementalWriteOutcome::CopiedUnchanged; + QVERIFY(writer.writeIncremental(&output, originalData, &original, modified.data(), &outcome)); + QCOMPARE(outcome, pdf::PDFDocumentWriter::IncrementalWriteOutcome::Appended); + } + + // Saving a document against itself produces the right bytes, but it is a + // copy rather than an append - and the caller must be able to tell, because + // the two are indistinguishable from the success value alone. + { + pdf::PDFDocumentWriter writer(nullptr); + QBuffer output; + output.open(QIODevice::WriteOnly); + + auto outcome = pdf::PDFDocumentWriter::IncrementalWriteOutcome::Appended; + QVERIFY(writer.writeIncremental(&output, originalData, &original, &original, &outcome)); + QCOMPARE(outcome, pdf::PDFDocumentWriter::IncrementalWriteOutcome::CopiedUnchanged); + QCOMPARE(output.data(), originalData); + } +} + +void IncrementalSaveTest::refusalsNameTheirReason() +{ + // Every refusal to append must say which condition stopped it, not just + // "operation failed" - the caller has to know whether to retry as a full + // rewrite or to stop. + const QByteArray originalData = writeDocument(createDocument()); + pdf::PDFDocumentReader reader(nullptr, [](bool*) + { return QString(); }, true, false); + const pdf::PDFDocument original = reader.readFromBuffer(originalData); + const pdf::PDFDocumentPointer modified = createModifiedDocument(original); + QVERIFY(modified); + + pdf::PDFDocumentWriter writer(nullptr); + + { + QBuffer output; + output.open(QIODevice::WriteOnly); + const pdf::PDFOperationResult result = writer.writeIncremental(&output, originalData + QByteArrayLiteral("changed"), &original, modified.data()); + QVERIFY(!result); + QVERIFY2(result.getErrorMessage().contains(QStringLiteral("source PDF changed")), + qPrintable(result.getErrorMessage())); + } + + { + QBuffer output; + output.open(QIODevice::WriteOnly); + const pdf::PDFOperationResult result = writer.writeIncremental(&output, QByteArrayLiteral("not a pdf"), &original, modified.data()); + QVERIFY(!result); + QVERIFY2(result.getErrorMessage().contains(QStringLiteral("missing or invalid")), + qPrintable(result.getErrorMessage())); + } +} + void IncrementalSaveTest::selectsSafeWritePolicy() { const pdf::PDFDocument unsignedDocument = createDocument(); diff --git a/UnitTests/tst_jbig2decodertest.cpp b/UnitTests/tst_jbig2decodertest.cpp index e4d5f3e0c..48e7870c7 100644 --- a/UnitTests/tst_jbig2decodertest.cpp +++ b/UnitTests/tst_jbig2decodertest.cpp @@ -40,6 +40,7 @@ class Jbig2DecoderTest : public QObject private slots: void test_codeTables_rejectsOversizedRangeBitLength(); void test_codeTables_acceptsValidSmallTable(); + void test_paint_boundsTheExpansionAllocation(); }; void Jbig2DecoderTest::test_codeTables_rejectsOversizedRangeBitLength() @@ -114,6 +115,34 @@ void Jbig2DecoderTest::test_codeTables_acceptsValidSmallTable() } } +void Jbig2DecoderTest::test_paint_boundsTheExpansionAllocation() +{ + // paint() with expandY grows the target bitmap to offsetY + height. That is + // the one path that resizes a bitmap after construction, so it has to repeat + // the dimension check a constructor performs - otherwise a wide page plus a + // large (attacker-chosen, and legitimately signed) offset asks for an + // allocation no real JBIG2 page needs. + pdf::PDFJBIG2Bitmap page(8192, 8); + pdf::PDFJBIG2Bitmap region(8, 8); + + bool thrown = false; + try + { + page.paint(region, 0, 1 << 20, pdf::PDFJBIG2BitOperation::Or, true, 0x00); + } + catch (const pdf::PDFException&) + { + thrown = true; + } + + QVERIFY2(thrown, "An out-of-range expansion was allocated instead of refused"); + + // A modest expansion still works. + pdf::PDFJBIG2Bitmap smallPage(16, 8); + smallPage.paint(region, 0, 16, pdf::PDFJBIG2BitOperation::Or, true, 0x00); + QCOMPARE(smallPage.getHeight(), 24); +} + QTEST_GUILESS_MAIN(Jbig2DecoderTest) #include "tst_jbig2decodertest.moc" diff --git a/UnitTests/tst_pdftoolcontract.cpp b/UnitTests/tst_pdftoolcontract.cpp index de8a5f74a..43026370c 100644 --- a/UnitTests/tst_pdftoolcontract.cpp +++ b/UnitTests/tst_pdftoolcontract.cpp @@ -22,11 +22,13 @@ #include "processoutputcapture.h" +#include #include #include #include #include #include +#include #include namespace @@ -102,6 +104,9 @@ private slots: void unknownCommandIsInvalidInvocation(); void malformedInvocationIsWrapped(); void defaultPreflightMalformedInvocationIsWrapped(); + void fetchImagesOnVectorOnlyDocumentNotesEmptyResult(); + void fetchImagesFailIfEmptyIsFindings(); + void fetchTextFailIfEmptyKeepsSuccessWhenTextExists(); void preflightRejectsNonJsonOutput(); void preflightKeepsNestedReportBoundary(); }; @@ -214,6 +219,88 @@ void PdfToolContractTest::defaultPreflightMalformedInvocationIsWrapped() QCOMPARE(run.json.value(QStringLiteral("status")).toString(), QStringLiteral("invalid-invocation")); } +namespace +{ + +QString textOnlyFixturePath() +{ + return QDir(QStringLiteral(LOOP_PREFLIGHT_SOURCE_DIR)).filePath(QStringLiteral("testdata/fixtures/font-embedded.pdf")); +} + +QJsonObject findDiagnostic(const ToolRun& run, const QString& code) +{ + for (const QJsonValue& value : run.json.value(QStringLiteral("diagnostics")).toArray()) + { + const QJsonObject diagnostic = value.toObject(); + if (diagnostic.value(QStringLiteral("code")).toString() == code) + { + return diagnostic; + } + } + + return QJsonObject(); +} + +} // namespace + +void PdfToolContractTest::fetchImagesOnVectorOnlyDocumentNotesEmptyResult() +{ + // A text-only document has no images to extract. That is a legitimate + // answer, so the run still succeeds - but it must say so in a way a + // machine consumer can see, instead of being indistinguishable from a + // successful extraction of zero files. + QTemporaryDir outputDirectory; + QVERIFY(outputDirectory.isValid()); + + const ToolRun run = runPdfTool({ QStringLiteral("fetch-images"), + textOnlyFixturePath(), + QStringLiteral("--image-output-dir"), outputDirectory.path(), + QStringLiteral("--console-format"), QStringLiteral("json") }); + + verifyEnvelope(run, 0, QStringLiteral("fetch-images")); + QCOMPARE(run.json.value(QStringLiteral("status")).toString(), QStringLiteral("success")); + + const QJsonObject diagnostic = findDiagnostic(run, QStringLiteral("output.empty-result")); + QVERIFY2(!diagnostic.isEmpty(), "fetch-images produced no output.empty-result diagnostic"); + QCOMPARE(diagnostic.value(QStringLiteral("severity")).toString(), QStringLiteral("info")); + QCOMPARE(diagnostic.value(QStringLiteral("context")).toObject().value(QStringLiteral("fail_if_empty")).toBool(), false); + QVERIFY(run.json.value(QStringLiteral("outputs")).toArray().isEmpty()); +} + +void PdfToolContractTest::fetchImagesFailIfEmptyIsFindings() +{ + QTemporaryDir outputDirectory; + QVERIFY(outputDirectory.isValid()); + + const ToolRun run = runPdfTool({ QStringLiteral("fetch-images"), + textOnlyFixturePath(), + QStringLiteral("--image-output-dir"), outputDirectory.path(), + QStringLiteral("--fail-if-empty"), + QStringLiteral("--console-format"), QStringLiteral("json") }); + + verifyEnvelope(run, 1, QStringLiteral("fetch-images")); + QCOMPARE(run.json.value(QStringLiteral("status")).toString(), QStringLiteral("findings")); + + const QJsonObject diagnostic = findDiagnostic(run, QStringLiteral("output.empty-result")); + QVERIFY2(!diagnostic.isEmpty(), "fetch-images produced no output.empty-result diagnostic"); + QCOMPARE(diagnostic.value(QStringLiteral("severity")).toString(), QStringLiteral("error")); + QCOMPARE(diagnostic.value(QStringLiteral("context")).toObject().value(QStringLiteral("subject")).toString(), QStringLiteral("images")); + QCOMPARE(diagnostic.value(QStringLiteral("context")).toObject().value(QStringLiteral("fail_if_empty")).toBool(), true); +} + +void PdfToolContractTest::fetchTextFailIfEmptyKeepsSuccessWhenTextExists() +{ + // The flag must not turn a document that does have text into a finding - + // it only reports on the empty case. + const ToolRun run = runPdfTool({ QStringLiteral("fetch-text"), + textOnlyFixturePath(), + QStringLiteral("--fail-if-empty"), + QStringLiteral("--console-format"), QStringLiteral("json") }); + + verifyEnvelope(run, 0, QStringLiteral("fetch-text")); + QVERIFY(findDiagnostic(run, QStringLiteral("output.empty-result")).isEmpty()); +} + void PdfToolContractTest::preflightRejectsNonJsonOutput() { const ToolRun run = runPdfTool({ QStringLiteral("preflight"), QStringLiteral("--console-format"), QStringLiteral("text") }); diff --git a/UnitTests/tst_processingbudgettest.cpp b/UnitTests/tst_processingbudgettest.cpp index 89cb6daea..000d452f2 100644 --- a/UnitTests/tst_processingbudgettest.cpp +++ b/UnitTests/tst_processingbudgettest.cpp @@ -22,6 +22,7 @@ #include "pdfprocessingbudget.h" #include "pdfdocumentreader.h" +#include "pdfnametreeloader.h" #include "pdfparser.h" #include @@ -77,6 +78,7 @@ private slots: void sequentialInputIsBoundedBeforeParsing(); void namedPoolsMapEveryKind(); void evidenceUndoAndRollbackPoolsAreFinite(); + void nameTreeTraversalIsBounded(); }; void ProcessingBudgetTest::cumulativeDecodedBytesAreDocumentWide() @@ -250,5 +252,49 @@ void ProcessingBudgetTest::evidenceUndoAndRollbackPoolsAreFinite() } } +void ProcessingBudgetTest::nameTreeTraversalIsBounded() +{ + using Loader = pdf::PDFNameTreeLoader; + + // Object 1 is a name tree node whose Kids array points back at itself, and + // which also carries one usable entry and one absurdly long key. Before the + // traversal was bounded, following Kids here recursed until the stack ran + // out. + auto kids = std::make_shared(); + kids->appendItem(pdf::PDFObject::createReference(pdf::PDFObjectReference(1, 0))); + + const QByteArray oversizedKey(Loader::MAXIMUM_NAME_LENGTH + 1, 'a'); + + auto names = std::make_shared(); + names->appendItem(pdf::PDFObject::createString(QByteArray("usable"))); + names->appendItem(pdf::PDFObject::createInteger(42)); + names->appendItem(pdf::PDFObject::createString(oversizedKey)); + names->appendItem(pdf::PDFObject::createInteger(43)); + + auto node = std::make_shared(); + node->addEntry(pdf::PDFInplaceOrMemoryString("Names"), pdf::PDFObject::createArray(std::move(names))); + node->addEntry(pdf::PDFInplaceOrMemoryString("Kids"), pdf::PDFObject::createArray(std::move(kids))); + + pdf::PDFObjectStorage::PDFObjects objects; + objects.resize(2); + objects[1].generation = 0; + objects[1].object = pdf::PDFObject::createDictionary(std::move(node)); + + pdf::PDFObjectStorage storage(std::move(objects), pdf::PDFObject(), pdf::PDFSecurityHandlerPointer()); + + const auto loadObject = [](const pdf::PDFObjectStorage* objectStorage, const pdf::PDFObject& object) + { + return objectStorage->getObject(object); + }; + + const auto result = Loader::parse(&storage, pdf::PDFObject::createReference(pdf::PDFObjectReference(1, 0)), loadObject); + + // The cycle terminated, the usable key survived, and the oversized key was + // refused rather than stored verbatim in the document model. + QCOMPARE(result.size(), size_t(1)); + QVERIFY(result.count(QByteArray("usable")) == 1); + QVERIFY(result.count(oversizedKey) == 0); +} + QTEST_MAIN(ProcessingBudgetTest) #include "tst_processingbudgettest.moc" diff --git a/UnitTests/tst_safefilewritertest.cpp b/UnitTests/tst_safefilewritertest.cpp index db5a397bd..222068cef 100644 --- a/UnitTests/tst_safefilewritertest.cpp +++ b/UnitTests/tst_safefilewritertest.cpp @@ -79,6 +79,7 @@ private slots: void findOutputConflicts_allowsExistingDestinationsWithOverwrite(); void makeUniqueFileName_returnsInputWhenFree(); void makeUniqueFileName_appendsFreeVariant(); + void makeUniqueFileName_fallsBackToRandomSuffix(); }; void SafeFileWriterTest::writeData_success_placesFile() @@ -256,6 +257,27 @@ void SafeFileWriterTest::makeUniqueFileName_appendsFreeVariant() QVERIFY(!QFile::exists(secondUnique)); } +void SafeFileWriterTest::makeUniqueFileName_fallsBackToRandomSuffix() +{ + // A directory pre-filled with the whole sequential cascade must not cost an + // unbounded stat() scan - the writer switches to a random discriminator and + // still returns a free, non-colliding name. + QTemporaryDir temporaryDirectory; + QVERIFY(temporaryDirectory.isValid()); + const QString path = temporaryDirectory.filePath(QStringLiteral("report.pdf")); + + QVERIFY(writeRawContent(path, "occupied")); + for (int n = 1; n <= 128; ++n) + { + QVERIFY(writeRawContent(temporaryDirectory.filePath(QStringLiteral("report (%1).pdf").arg(n)), "occupied")); + } + + const QString unique = pdf::PDFSafeFileWriter::makeUniqueFileName(path); + QVERIFY2(unique != path, qPrintable(unique)); + QVERIFY2(!QFile::exists(unique), qPrintable(unique)); + QVERIFY2(unique.endsWith(QStringLiteral(".pdf")), qPrintable(unique)); +} + QTEST_APPLESS_MAIN(SafeFileWriterTest) #include "tst_safefilewritertest.moc" diff --git a/changes/claude-loop-exhaustive-review-73k4t5.md b/changes/claude-loop-exhaustive-review-73k4t5.md new file mode 100644 index 000000000..6fbd33e3e --- /dev/null +++ b/changes/claude-loop-exhaustive-review-73k4t5.md @@ -0,0 +1,35 @@ +Category: fixed +Audience: developers, users +Breaking-Change: no +Summary: Act on the post-0.2.0 exhaustive read-only review. PDFLogScrubber now scrubs credential +material - URL userinfo (the shape of a Sentry DSN), HTTP authorization values, and secret-named +key/value pairs - before the existing path/email passes, so a leaked token is reported as + rather than partially eaten by the email pass; the key vocabulary matches +isSensitiveKey() in pdfartifactidentity.cpp and the bare auth-scheme pass excludes "Token" so +parser diagnostics ("Unexpected token appeared") survive. PdfTool's extraction commands +(fetch-images, fetch-text, attachments) now always record an output.empty-result diagnostic when +they produce nothing and accept a shared --fail-if-empty that turns that into exit 1 (findings), +so a pipeline gating on "figures were produced" cannot be green-lit by an empty output directory; +documented in docs/PDFTOOL_CLI_CONTRACT.md. The loop-ocr sidecar reads a staged page raster once +by descriptor (O_NOFOLLOW where available, size-capped, regular-file checked) and passes the bytes +to PIL and easyocr instead of re-resolving the path three times, closing the TOCTOU window; +language codes are shape-validated so a traversal-shaped value cannot reach easyocr's model file +names (mirrored in ocr-sidecar.schema.json), and PdfTool sets the staged raster 0600. +PDFDocumentWriter::writeIncremental reports through an optional IncrementalWriteOutcome whether it +appended or only byte-copied an unchanged document - previously indistinguishable from the success +value alone. Damaged-document recovery bounds its dense object table by how many objects were +actually recovered instead of by the highest object number the document happens to declare, and +now carries a real source digest so writeIncremental's "the file changed underneath us" guard is +not silently disabled for permissively recovered documents. PDFNameTreeLoader bounds traversal: +cyclic Kids chains terminate, nesting and entry count are capped, and over-long keys are refused +instead of stored verbatim in the document model. Structure-tree parsing bounds its recursion +depth (cycles were already refused; long acyclic chains were not). PDFJBIG2Bitmap::paint validates +the grown dimensions on its expandY path, the one place a bitmap grows after construction and so +the one place that escaped the constructor's dimension check. PDFFilenameSanitizer::isPathContained +no longer reports a planned output as escaping simply because its target directory has not been +created yet, while keeping the stricter symlinked-parent rule for the file side. +PDFSafeFileWriter::makeUniqueFileName probes 128 sequential names then switches to a random +discriminator instead of scanning up to 100k candidates. Diagnostics bundles truncate plugin +display fields so an oversized plugin manifest cannot bloat every future support bundle. The OCR +option defaults (languages, dpi, min-text-chars) are defined once and shared between the +capability-discovery table and the command-line parser. diff --git a/docs/PDFTOOL_CLI_CONTRACT.md b/docs/PDFTOOL_CLI_CONTRACT.md index dddc4fc5c..b9a387e33 100644 --- a/docs/PDFTOOL_CLI_CONTRACT.md +++ b/docs/PDFTOOL_CLI_CONTRACT.md @@ -131,14 +131,39 @@ PdfTool diff old.pdf new.pdf --console-format json - `code`: stable kebab-case identifier, e.g. `cli.invalid-arguments`, `cli.unknown-command`, `pdf.document-unreadable`, `pdf.invalid-password`, `pdf.reader-warning`, `output.already-exists`, `output.write-failed`, - `operation.cancelled`. Machine consumers branch on these codes; treat them as - the stability contract. `message` is human-oriented and may change. + `output.empty-result`, `operation.cancelled`. Machine consumers branch on + these codes; treat them as the stability contract. `message` is + human-oriented and may change. - `context`: optional free-form object (e.g. the offending path). In JSON mode, handled errors and warnings are captured in `diagnostics` and are **not** additionally written to stderr. In text/XML/HTML mode the existing human-facing stderr behavior is preserved. +### Empty results + +Extraction commands (`fetch-images`, `fetch-text`, `attachments`) complete +successfully when a document simply has nothing to extract - a vector-only page +has no images, a scanned page has no text. That case always records an +`output.empty-result` diagnostic whose `context` carries `subject` (what was not +produced) and `fail_if_empty` (whether the caller asked to fail on it): + +```json +{ + "severity": "info", + "code": "output.empty-result", + "message": "No images were extracted from document 'vector-only.pdf'.", + "context": { "subject": "images", "fail_if_empty": false } +} +``` + +By default the note is informational and the command still exits `0 success`, +because "this document has no figures" is a legitimate answer. Passing +`--fail-if-empty` raises the same diagnostic to `error` and exits +`1 findings`, so a pipeline that gates on "figures were produced" cannot be +green-lit by an empty output directory. The flag never changes which files are +written; it only chooses how an empty result is reported. + ## Output records ```json diff --git a/loop-ocr/schemas/ocr-sidecar.schema.json b/loop-ocr/schemas/ocr-sidecar.schema.json index 98aecf705..2eb065af5 100644 --- a/loop-ocr/schemas/ocr-sidecar.schema.json +++ b/loop-ocr/schemas/ocr-sidecar.schema.json @@ -13,7 +13,7 @@ "dpi": { "type": "integer", "minimum": 1, "maximum": 1200 }, "languages": { "type": "array", - "items": { "type": "string", "minLength": 1 } + "items": { "type": "string", "minLength": 1, "pattern": "^[a-z]{2,3}(_[a-z]{2,4})?$" } }, "media_box": { "$ref": "#/$defs/mediaBox" }, "rotation": { "type": "integer", "enum": [0, 90, 180, 270] } diff --git a/loop-ocr/service/engine.py b/loop-ocr/service/engine.py index c7593f37d..b6faf283d 100644 --- a/loop-ocr/service/engine.py +++ b/loop-ocr/service/engine.py @@ -2,13 +2,26 @@ from __future__ import annotations +import io import math import os +import re +import stat from typing import Any DEFAULT_LANGUAGES = ["en"] DEFAULT_MEDIA_BOX = {"x": 0.0, "y": 0.0, "width": 612.0, "height": 792.0} MAX_DPI = 1200 + +# A staged page raster at 1200 dpi is large but bounded; anything past this is +# not something PdfTool produced, so refuse it rather than loading it. +MAX_IMAGE_BYTES = 512 * 1024 * 1024 + +# Language codes are ISO 639-1/639-2 style tokens, optionally with a script or +# region suffix ("ch_sim", "en"). Codes reach easyocr.Reader, which uses them to +# build model file names, so they are shape-checked here: a value like +# "../../etc" must never get that far. +_LANGUAGE_PATTERN = re.compile(r"^[a-z]{2,3}(?:_[a-z]{2,4})?$") _readers: dict[tuple[str, ...], object] = {} @@ -36,6 +49,10 @@ def normalize_languages(value: object) -> list[str]: if not languages: return list(DEFAULT_LANGUAGES) + for language in languages: + if not _LANGUAGE_PATTERN.match(language): + raise ValueError(f"language code is not a valid identifier: {language!r}") + return sorted(set(languages)) @@ -166,6 +183,33 @@ def pixel_bbox_to_pdf( } +def _read_staged_image(image_path: str) -> bytes: + """Reads a staged page raster exactly once, by descriptor. + + PdfTool stages the raster into a private temporary directory and hands us the + path. Re-resolving that path for every use - an existence check, then PIL, + then the OCR reader - is three chances for the file behind the name to change + between them. Opening once and passing the bytes onward removes the window, + and O_NOFOLLOW (where the platform has it) refuses a name that has been + turned into a symlink. + """ + flags = os.O_RDONLY | getattr(os, "O_BINARY", 0) | getattr(os, "O_NOFOLLOW", 0) + descriptor = os.open(image_path, flags) + try: + status = os.fstat(descriptor) + if not stat.S_ISREG(status.st_mode): + raise ValueError("staged image is not a regular file") + if status.st_size > MAX_IMAGE_BYTES: + raise ValueError(f"staged image exceeds {MAX_IMAGE_BYTES} bytes") + + with os.fdopen(descriptor, "rb") as handle: + descriptor = -1 + return handle.read() + finally: + if descriptor >= 0: + os.close(descriptor) + + def run_ocr(request: dict[str, Any]) -> dict[str, Any]: from PIL import Image @@ -179,14 +223,18 @@ def run_ocr(request: dict[str, Any]) -> dict[str, Any]: if not image_path: return {"page": page, "ok": False, "error": "missing image path"} - if not os.path.isfile(image_path): + try: + image_bytes = _read_staged_image(image_path) + except FileNotFoundError: return {"page": page, "ok": False, "error": f"image not found: {image_path}"} + except (IsADirectoryError, OSError, ValueError) as error: + return {"page": page, "ok": False, "error": f"image could not be read: {error}"} reader = get_reader(list(languages)) - with Image.open(image_path) as image: + with Image.open(io.BytesIO(image_bytes)) as image: image_width, image_height = image.size - results = reader.readtext(image_path) + results = reader.readtext(image_bytes) lines = [] text_parts: list[str] = [] for bbox_pixels, text, confidence in results: diff --git a/loop-ocr/tests/test_engine.py b/loop-ocr/tests/test_engine.py index 107b2a40b..266d995c0 100644 --- a/loop-ocr/tests/test_engine.py +++ b/loop-ocr/tests/test_engine.py @@ -3,13 +3,21 @@ from __future__ import annotations import math +import os import sys +import tempfile import unittest from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "service")) -from engine import normalize_languages, pixel_bbox_to_pdf, validate_request # noqa: E402 +from engine import ( # noqa: E402 + MAX_IMAGE_BYTES, + _read_staged_image, + normalize_languages, + pixel_bbox_to_pdf, + validate_request, +) class EngineContractTest(unittest.TestCase): @@ -21,6 +29,53 @@ def test_invalid_language_shape_is_rejected(self) -> None: with self.assertRaisesRegex(ValueError, "languages must be an array"): normalize_languages("en") + def test_language_codes_must_look_like_language_codes(self) -> None: + # Codes are used to build model file names, so a traversal-shaped value + # must be refused here rather than passed to easyocr. + for code in ["../../etc", "en/../..", "e", "toolongcode", "en-US", ""]: + with self.subTest(code=code): + if not code.strip(): + self.assertEqual(normalize_languages([code]), ["en"]) + continue + with self.assertRaises(ValueError): + normalize_languages([code]) + + self.assertEqual(normalize_languages(["ch_sim", "EN"]), ["ch_sim", "en"]) + + def test_staged_image_is_read_by_descriptor(self) -> None: + with tempfile.TemporaryDirectory() as directory: + path = os.path.join(directory, "page-1.png") + with open(path, "wb") as handle: + handle.write(b"raster-bytes") + + self.assertEqual(_read_staged_image(path), b"raster-bytes") + + missing = os.path.join(directory, "absent.png") + with self.assertRaises(FileNotFoundError): + _read_staged_image(missing) + + with self.assertRaises((ValueError, OSError)): + _read_staged_image(directory) + + @unittest.skipUnless(hasattr(os, "symlink") and hasattr(os, "O_NOFOLLOW"), "symlinks unavailable") + def test_staged_image_refuses_a_symlink(self) -> None: + with tempfile.TemporaryDirectory() as directory: + target = os.path.join(directory, "target.png") + with open(target, "wb") as handle: + handle.write(b"raster-bytes") + + link = os.path.join(directory, "page-1.png") + try: + os.symlink(target, link) + except (OSError, NotImplementedError): + self.skipTest("symlink creation not permitted") + + with self.assertRaises(OSError): + _read_staged_image(link) + + def test_staged_image_size_cap_is_sane(self) -> None: + self.assertGreater(MAX_IMAGE_BYTES, 0) + def test_request_limits_and_media_box_are_validated(self) -> None: normalized = validate_request( {