exporthtml does not parameterize SQL statements

[flrgh]

I have a contact with the name Firstname "Some Nickname" Lastname, and this is triggering errors in SQL statements:

[Error]: During sqlite3_prepare_v2(): near "Some": syntax error
         -> Query: "SELECT json_object('id', message._id, 'b', message.body, 'f', message.from_recipient_id, 'tr', thread.recipient_id, 'o', (message.type & 0x1F) IN (2,11,21,22,23,24,25,26), 'd', (message.date_received / 1000 - 1404165600), 'p', SUBSTR("Firstname "Some Nickname" Lastname (_id40)/Firstname "Some Nickname" Lastname.html", 1, LENGTH("Firstname "Some Nickname" Lastname (_id40)/Firstname "Some Nickname" Lastname.html") - 5)) AS line, attachment._id AS rowid, -1 AS uniqueid FROM message LEFT JOIN thread ON thread._id IS message.thread_id LEFT JOIN attachment ON attachment.message_id IS message._id AND attachment.content_type = 'text/x-signal-plain' AND attachment.quote = 0 WHERE message._id = ?
[Warning]: Search_idx query failed or no results

I was surprised to see a SQL string escape error in a function with prepare in the name, but then I looked at the source code and saw that it is just string concatenating the query instead of parameterizing it.

[flrgh]

Was going to try opening a PR to fix but I just saw #256.

Here's my attempt at patching this. I don't know the codebase or C++ very well, so I am not sure if it's correct, but after re-compiling it seems to work without an errors.

diff --git signalbackup/exporthtml.cc signalbackup/exporthtml.cc
index 119f43b..25a6300 100644
--- signalbackup/exporthtml.cc
+++ signalbackup/exporthtml.cc
@@ -627,6 +627,10 @@ bool SignalBackup::exportHtml(std::string const &directory, std::vector<long lon
         if (searchpage && (!Types::isStatusMessage(msg_info.type) && !msg_info.body.empty()))
         {
           // because the body is already escaped for html at this point, we get it fresh from database (and have sqlite do the json formatting)
+
+	  // all pages end in ".html", slice it off
+	  std::string page_name = (msg_info.threaddir + "/" + msg_info.filename).substr(0, -5);
+
           if (!d_database.exec("SELECT json_object("
                                "'id', " + d_mms_table + "._id, "
                                "'b', " + d_mms_table + ".body, "
@@ -635,7 +639,7 @@ bool SignalBackup::exportHtml(std::string const &directory, std::vector<long lon
                                "'o', (" + d_mms_table + "." + d_mms_type + " & 0x1F) IN (2,11,21,22,23,24,25,26), "
                                "'d', (" + d_mms_table + ".date_received / 1000 - 1404165600), " // loose the last three digits (miliseconds, they are never displayed anyway).
                                                                                                 // subtract "2014-07-01". Signals initial release was 2014-07-29, negative numbers should work otherwise anyway.
-                               "'p', " + "SUBSTR(\"" + msg_info.threaddir + "/" + msg_info.filename + "\", 1, LENGTH(\"" + msg_info.threaddir + "/" + msg_info.filename + "\") - 5)" + ") AS line, " // all pages end in ".html", slice it off
+                               "'p', " + "?) AS line, "
                                + d_part_table + "._id AS rowid, " +
                                (d_database.tableContainsColumn(d_part_table, "unique_id") ?
                                 d_part_table + ".unique_id AS uniqueid" : "-1 AS uniqueid") +
@@ -643,7 +647,8 @@ bool SignalBackup::exportHtml(std::string const &directory, std::vector<long lon
                                "LEFT JOIN thread ON thread._id IS " + d_mms_table + ".thread_id "
                                "LEFT JOIN " + d_part_table + " ON " + d_part_table + "." + d_part_mid + " IS " + d_mms_table + "._id AND " + d_part_table + "." + d_part_ct + " = 'text/x-signal-plain' AND " + d_part_table + ".quote = 0 "
                                "WHERE " + d_mms_table + "._id = ?",
-                               msg_info.msg_id, &search_idx_results) ||
+			       {page_name, msg_info.msg_id},
+			       &search_idx_results) ||
               search_idx_results.rows() < 1) [[unlikely]]
           {
             Logger::warning("Search_idx query failed or no results");

[bepaald]

Thanks, that's a nice one! I'll include this fix (or something very similar) when I next push, probably later today.

[bepaald]

I had a go at this and ended up with a fix quite similar to yours. Comparing, I just don't do the superfluous concatination of string literals ("'p', " + "?" -> "'p', ?"), and use a different constructor to create the page-string. Note in C++'s std::string, the substr() function does not take negative values, so your patch — while getting through SQLite without errors — would result in the searchpage generating broken links.

Still it was a good find, while I do test all the HTML stuff with special characters, the " is a special case even within the group of special characters. During testing I also stumbled on a few more (Windows only) problems, that were not really related to this, but I would not have spotted without you report. Hopefully they are now fixed as well.

Thanks again, and let me know if you encounter any more problems.

[flrgh]

@bepaald awesome. I have never written a line of C++ in my life, so I'm not surprised I got something wrong 🙃 thanks so much for the fix!