Improve Entry placeholder resolution (#10846)

* Entry placeholder resolution: don't overdo it

After resolving placeholders, previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

* Entry tweaks and minor refactoring

- Entry::size(): when computing tag size, use same delimiter set as in other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern alternative
- Miscellanous minor code cleanups

* Entry: fix hitting recursion limit with {braces}

When encountering a {brace-enclosed} substring, the placeholder resolution logic would previously keep recursing until it hit the recursion depth limit (currently 10). This would lead to "Maximum depth of replacement has been reached" messages, and was also wasting CPU cycles.

Fixes #1741

---------

Co-authored-by: Jonathan White <support@dmapps.us>

src/core/Entry.cpp

@@ -28,12 +28,18 @@
 
 #include <QDir>
 #include <QRegularExpression>
+#include <QStringBuilder>
 #include <QUrl>
 
 const int Entry::DefaultIconNumber = 0;
-const int Entry::ResolveMaximumDepth = 10;
+
-const QString Entry::AutoTypeSequenceUsername = "{USERNAME}{ENTER}";
+namespace
-const QString Entry::AutoTypeSequencePassword = "{PASSWORD}{ENTER}";
+{
+    const int ResolveMaximumDepth = 10;
+    const QString AutoTypeSequenceUsername = "{USERNAME}{ENTER}";
+    const QString AutoTypeSequencePassword = "{PASSWORD}{ENTER}";
+    const QRegularExpression TagDelimiterRegex(R"([,;\t])");
+} // namespace
 
 Entry::Entry()
     : m_attributes(new EntryAttributes(this))
@@ -432,14 +438,11 @@ QString Entry::attribute(const QString& key) const
 int Entry::size() const
 {
     int size = 0;
-    const QRegularExpression delimiter(",|:|;");
+    size += attributes()->attributesSize();
-
+    size += autoTypeAssociations()->associationsSize();
-    size += this->attributes()->attributesSize();
+    size += attachments()->attachmentsSize();
-    size += this->autoTypeAssociations()->associationsSize();
+    size += customData()->dataSize();
-    size += this->attachments()->attachmentsSize();
+    for (const QString& tag : tags().split(TagDelimiterRegex, QString::SkipEmptyParts)) {
-    size += this->customData()->dataSize();
-    const QStringList tags = this->tags().split(delimiter, QString::SkipEmptyParts);
-    for (const QString& tag : tags) {
         size += tag.toUtf8().size();
     }
 
@@ -482,8 +485,7 @@ bool Entry::isAttributeReferenceOf(const QString& key, const QUuid& uuid) const
 
 bool Entry::hasReferences() const
 {
-    const QList<QString> keyList = EntryAttributes::DefaultAttributes;
+    for (const QString& key : EntryAttributes::DefaultAttributes) {
-    for (const QString& key : keyList) {
         if (m_attributes->isReference(key)) {
             return true;
         }
@@ -493,8 +495,7 @@ bool Entry::hasReferences() const
 
 bool Entry::hasReferencesTo(const QUuid& uuid) const
 {
-    const QList<QString> keyList = EntryAttributes::DefaultAttributes;
+    for (const QString& key : EntryAttributes::DefaultAttributes) {
-    for (const QString& key : keyList) {
         if (isAttributeReferenceOf(key, uuid)) {
             return true;
         }
@@ -670,11 +671,10 @@ void Entry::setOverrideUrl(const QString& url)
 
 void Entry::setTags(const QString& tags)
 {
-    static QRegExp rx("(\\,|\\t|\\;)");
+    auto taglist = tags.split(TagDelimiterRegex, QString::SkipEmptyParts);
-    auto taglist = tags.split(rx, QString::SkipEmptyParts);
     // Trim whitespace before/after tag text
-    for (auto itr = taglist.begin(); itr != taglist.end(); ++itr) {
+    for (auto& tag : taglist) {
-        *itr = itr->trimmed();
+        tag = tag.trimmed();
     }
     // Remove duplicates
     auto tagSet = QSet<QString>::fromList(taglist);
@@ -687,7 +687,7 @@ void Entry::setTags(const QString& tags)
 void Entry::addTag(const QString& tag)
 {
     auto cleanTag = tag.trimmed();
-    cleanTag.remove(QRegExp("(\\,|\\t|\\;)"));
+    cleanTag.remove(TagDelimiterRegex);
 
     auto taglist = m_data.tags;
     if (!taglist.contains(cleanTag)) {
@@ -700,7 +700,7 @@ void Entry::addTag(const QString& tag)
 void Entry::removeTag(const QString& tag)
 {
     auto cleanTag = tag.trimmed();
-    cleanTag.remove(QRegExp("(\\,|\\t|\\;)"));
+    cleanTag.remove(TagDelimiterRegex);
 
     auto taglist = m_data.tags;
     if (taglist.removeAll(tag) > 0) {
@@ -1016,25 +1016,23 @@ void Entry::updateModifiedSinceBegin()
 
 QString Entry::resolveMultiplePlaceholdersRecursive(const QString& str, int maxDepth) const
 {
-    static QRegularExpression placeholderRegEx("(\\{[^\\}]+?\\})", QRegularExpression::CaseInsensitiveOption);
+    static const QRegularExpression placeholderRegEx(R"(\{[^}]+\})");
 
     if (maxDepth <= 0) {
         qWarning("Maximum depth of replacement has been reached. Entry uuid: %s", uuid().toString().toLatin1().data());
         return str;
     }
 
-    QString result = str;
+    QString result;
     auto matches = placeholderRegEx.globalMatch(str);
+    int capEnd = 0;
     while (matches.hasNext()) {
-        auto match = matches.next();
+        const auto match = matches.next();
-        const auto found = match.captured(1);
+        result += str.midRef(capEnd, match.capturedStart() - capEnd);
-        result.replace(found, resolvePlaceholderRecursive(found, maxDepth - 1));
+        result += resolvePlaceholderRecursive(match.captured(), maxDepth - 1);
+        capEnd = match.capturedEnd();
     }
-
+    result += str.rightRef(str.length() - capEnd);
-    if (result != str) {
-        result = resolveMultiplePlaceholdersRecursive(result, maxDepth - 1);
-    }
-
     return result;
 }
 
@@ -1048,32 +1046,20 @@ QString Entry::resolvePlaceholderRecursive(const QString& placeholder, int maxDe
     const PlaceholderType typeOfPlaceholder = placeholderType(placeholder);
     switch (typeOfPlaceholder) {
     case PlaceholderType::NotPlaceholder:
-    case PlaceholderType::Unknown:
         return resolveMultiplePlaceholdersRecursive(placeholder, maxDepth - 1);
+    case PlaceholderType::Unknown: {
+        return "{" % resolveMultiplePlaceholdersRecursive(placeholder.mid(1, placeholder.length() - 2), maxDepth - 1)
+               % "}";
+    }
     case PlaceholderType::Title:
-        if (placeholderType(title()) == PlaceholderType::Title) {
-            return title();
-        }
         return resolveMultiplePlaceholdersRecursive(title(), maxDepth - 1);
     case PlaceholderType::UserName:
-        if (placeholderType(username()) == PlaceholderType::UserName) {
-            return username();
-        }
         return resolveMultiplePlaceholdersRecursive(username(), maxDepth - 1);
     case PlaceholderType::Password:
-        if (placeholderType(password()) == PlaceholderType::Password) {
-            return password();
-        }
         return resolveMultiplePlaceholdersRecursive(password(), maxDepth - 1);
     case PlaceholderType::Notes:
-        if (placeholderType(notes()) == PlaceholderType::Notes) {
-            return notes();
-        }
         return resolveMultiplePlaceholdersRecursive(notes(), maxDepth - 1);
     case PlaceholderType::Url:
-        if (placeholderType(url()) == PlaceholderType::Url) {
-            return url();
-        }
         return resolveMultiplePlaceholdersRecursive(url(), maxDepth - 1);
     case PlaceholderType::DbDir: {
         QFileInfo fileInfo(database()->filePath());
@@ -1123,60 +1109,45 @@ QString Entry::resolvePlaceholderRecursive(const QString& placeholder, int maxDe
 
 QString Entry::resolveDateTimePlaceholder(Entry::PlaceholderType placeholderType) const
 {
-    QDateTime time = Clock::currentDateTime();
+    const QDateTime time = Clock::currentDateTime();
-    QDateTime time_utc = Clock::currentDateTimeUtc();
+    const QDateTime time_utc = Clock::currentDateTimeUtc();
-    QString date_formatted{};
 
     switch (placeholderType) {
     case PlaceholderType::DateTimeSimple:
-        date_formatted = time.toString("yyyyMMddhhmmss");
+        return time.toString("yyyyMMddhhmmss");
-        break;
     case PlaceholderType::DateTimeYear:
-        date_formatted = time.toString("yyyy");
+        return time.toString("yyyy");
-        break;
     case PlaceholderType::DateTimeMonth:
-        date_formatted = time.toString("MM");
+        return time.toString("MM");
-        break;
     case PlaceholderType::DateTimeDay:
-        date_formatted = time.toString("dd");
+        return time.toString("dd");
-        break;
     case PlaceholderType::DateTimeHour:
-        date_formatted = time.toString("hh");
+        return time.toString("hh");
-        break;
     case PlaceholderType::DateTimeMinute:
-        date_formatted = time.toString("mm");
+        return time.toString("mm");
-        break;
     case PlaceholderType::DateTimeSecond:
-        date_formatted = time.toString("ss");
+        return time.toString("ss");
-        break;
     case PlaceholderType::DateTimeUtcSimple:
-        date_formatted = time_utc.toString("yyyyMMddhhmmss");
+        return time_utc.toString("yyyyMMddhhmmss");
-        break;
     case PlaceholderType::DateTimeUtcYear:
-        date_formatted = time_utc.toString("yyyy");
+        return time_utc.toString("yyyy");
-        break;
     case PlaceholderType::DateTimeUtcMonth:
-        date_formatted = time_utc.toString("MM");
+        return time_utc.toString("MM");
-        break;
     case PlaceholderType::DateTimeUtcDay:
-        date_formatted = time_utc.toString("dd");
+        return time_utc.toString("dd");
-        break;
     case PlaceholderType::DateTimeUtcHour:
-        date_formatted = time_utc.toString("hh");
+        return time_utc.toString("hh");
-        break;
     case PlaceholderType::DateTimeUtcMinute:
-        date_formatted = time_utc.toString("mm");
+        return time_utc.toString("mm");
-        break;
     case PlaceholderType::DateTimeUtcSecond:
-        date_formatted = time_utc.toString("ss");
+        return time_utc.toString("ss");
-        break;
     default: {
         Q_ASSERT_X(false, "Entry::resolveDateTimePlaceholder", "Bad DateTime placeholder type");
         break;
     }
     }
 
-    return date_formatted;
+    return {};
 }
 
 QString Entry::resolveReferencePlaceholderRecursive(const QString& placeholder, int maxDepth) const
@@ -1189,7 +1160,7 @@ QString Entry::resolveReferencePlaceholderRecursive(const QString& placeholder,
     // resolving references in format: {REF:<WantedField>@<SearchIn>:<SearchText>}
     // using format from http://keepass.info/help/base/fieldrefs.html at the time of writing
 
-    QRegularExpressionMatch match = EntryAttributes::matchReference(placeholder);
+    const QRegularExpressionMatch match = EntryAttributes::matchReference(placeholder);
     if (!match.hasMatch() || !m_group || !m_group->database()) {
         return placeholder;
     }
@@ -1318,9 +1289,7 @@ Database* Entry::database()
 
 QString Entry::maskPasswordPlaceholders(const QString& str) const
 {
-    QString result = str;
+    return QString{str}.replace(QStringLiteral("{PASSWORD}"), QStringLiteral("******"), Qt::CaseInsensitive);
-    result.replace(QRegExp("(\\{PASSWORD\\})", Qt::CaseInsensitive, QRegExp::RegExp2), "******");
-    return result;
 }
 
 Entry* Entry::resolveReference(const QString& str) const
@@ -1438,17 +1407,19 @@ QString Entry::resolveUrl(const QString& url) const
 {
     QString newUrl = url;
 
-    QRegExp fileRegEx("^([a-z]:)?[\\\\/]", Qt::CaseInsensitive, QRegExp::RegExp2);
+    static const QRegularExpression fileRegEx(R"(^(?:[A-Za-z]:)?[\\/])");
-    if (fileRegEx.indexIn(newUrl) != -1) {
+    if (url.contains(fileRegEx)) {
         // Match possible file paths without the scheme and convert it to a file URL
         newUrl = QDir::fromNativeSeparators(newUrl);
         newUrl = QUrl::fromLocalFile(newUrl).toString();
-    } else if (newUrl.startsWith("cmd://")) {
+    } else if (url.startsWith("cmd://")) {
         QStringList cmdList = newUrl.split(" ");
         for (int i = 1; i < cmdList.size(); ++i) {
+            QString& cmd = cmdList[i];
             // Don't pass arguments to the resolveUrl function (they look like URL's)
-            if (!cmdList[i].startsWith("-") && !cmdList[i].startsWith("/")) {
+            if (!cmd.startsWith("-") && !cmd.startsWith("/")) {
-                return resolveUrl(cmdList[i].remove(QRegExp("'|\"")));
+                static const QRegularExpression quotesRegEx("['\"]");
+                return resolveUrl(cmd.remove(quotesRegEx));
             }
         }
 
@@ -1462,13 +1433,13 @@ QString Entry::resolveUrl(const QString& url) const
     }
 
     // Validate the URL
-    QUrl tempUrl = QUrl(newUrl);
+    QUrl tempUrl(newUrl);
     if (tempUrl.isValid()
         && (tempUrl.scheme() == "http" || tempUrl.scheme() == "https" || tempUrl.scheme() == "file")) {
         return tempUrl.url();
     }
 
-    // No valid http URL's found
+    // No valid http URLs found
     return {};
 }
 

src/core/Entry.h

@@ -227,9 +227,6 @@ public:
     };
 
     static const int DefaultIconNumber;
-    static const int ResolveMaximumDepth;
-    static const QString AutoTypeSequenceUsername;
-    static const QString AutoTypeSequencePassword;
 
     /**
      * Creates a duplicate of this entry except that the returned entry isn't

src/core/EntryAttributes.cpp

@@ -309,8 +309,8 @@ bool EntryAttributes::operator!=(const EntryAttributes& other) const
 
 QRegularExpressionMatch EntryAttributes::matchReference(const QString& text)
 {
-    static QRegularExpression referenceRegExp(
+    static const QRegularExpression referenceRegExp(
-        "\\{REF:(?<WantedField>[TUPANI])@(?<SearchIn>[TUPANIO]):(?<SearchText>[^}]+)\\}",
+        R"(\{REF:(?<WantedField>[TUPANI])@(?<SearchIn>[TUPANIO]):(?<SearchText>[^}]+)\})",
         QRegularExpression::CaseInsensitiveOption);
 
     return referenceRegExp.match(text);

tests/TestEntry.cpp

@@ -86,6 +86,7 @@ void TestEntry::testClone()
 {
     QScopedPointer<Entry> entryOrg(new Entry());
     entryOrg->setUuid(QUuid::createUuid());
+    entryOrg->setPassword("pass");
     entryOrg->setTitle("Original Title");
     entryOrg->beginUpdate();
     entryOrg->setTitle("New Title");
@@ -320,10 +321,12 @@ void TestEntry::testResolveRecursivePlaceholders()
     entry7->setTitle(QString("{REF:T@I:%1} and something else").arg(entry3->uuidToHex()));
     entry7->setUsername(QString("{TITLE}"));
     entry7->setPassword(QString("PASSWORD"));
+    entry7->setNotes(QString("{lots} {of} {braces}"));
 
     QCOMPARE(entry7->resolvePlaceholder(entry7->title()), QString("Entry2Title and something else"));
     QCOMPARE(entry7->resolvePlaceholder(entry7->username()), QString("Entry2Title and something else"));
     QCOMPARE(entry7->resolvePlaceholder(entry7->password()), QString("PASSWORD"));
+    QCOMPARE(entry7->resolvePlaceholder(entry7->notes()), QString("{lots} {of} {braces}"));
 }
 
 void TestEntry::testResolveReferencePlaceholders()