fix: act on the post-0.2.0 exhaustive review findings

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("... <truncated>");
+}
+
 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<int>(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);
     }
 

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<QString(bool*)>& 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<PDFInteger>(
+                DAMAGED_DOCUMENT_MINIMUM_OBJECT_SLOTS,
+                static_cast<PDFInteger>(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)
     {

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;
 }
 

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.

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();

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);
     }
 

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 <token>", "Basic <base64>"), 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
+    // "<CREDENTIAL>" is never matched again - scrub() must stay idempotent.
+    static const QRegularExpression urlUserInfoPattern(
+        QStringLiteral(R"((?<![\w.+-])([A-Za-z][A-Za-z0-9+.-]*://)[^\s/@:"'<]+(?::[^\s/@"'<]*)?@)"),
+        QRegularExpression::CaseInsensitiveOption);
+
+    static const QRegularExpression secretKeyValuePattern(
+        QStringLiteral(R"(\b(%1)("?\s*(?:=>|[:=])\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<CREDENTIAL>@"));
+    result.replace(secretKeyValuePattern, QStringLiteral("\\1\\2<CREDENTIAL>"));
+    result.replace(authorizationPattern, QStringLiteral("\\1<CREDENTIAL>"));
+    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("<USER>"));
     result = replaceToken(result, QSysInfo::machineHostName(), QStringLiteral("<HOST>"));
 
+    // 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 "<EMAIL>", 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);

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 (`<CREDENTIAL>`) rather than as user data (`<EMAIL>`). Applying
+    /// scrub() to already-scrubbed text is a no-op.
     /// \param text Text to scrub
     static QString scrub(const QString& text);
 };