Merge pull request #1616 from Expensify/main

Update expensify_prod branch

sqlitecluster/SQLite.cpp

@@ -540,8 +540,9 @@ bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) {
     SASSERT(_insideTransaction);
     _queryCache.clear();
     _queryCount++;
-    SASSERT(query.empty() || SEndsWith(query, ";"));                        // Must finish everything with semicolon
-    SASSERTWARN(SToUpper(query).find("CURRENT_TIMESTAMP") == string::npos); // Else will be replayed wrong
+
+    // Must finish everything with semicolon.
+    SASSERT(query.empty() || SEndsWith(query, ";"));
 
     // First, check our current state
     SQResult results;
@@ -550,6 +551,7 @@ bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) {
     uint64_t schemaBefore = SToUInt64(results[0][0]);
     uint64_t changesBefore = sqlite3_total_changes(_db);
 
+    _currentlyWriting = true;
     // Try to execute the query
     uint64_t before = STimeNow();
     bool usedRewrittenQuery = false;
@@ -573,12 +575,14 @@ bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) {
 
     // If we got a constraints error, throw that.
     if (resultCode == SQLITE_CONSTRAINT) {
+        _currentlyWriting = false;
         throw constraint_error();
     }
 
     _checkInterruptErrors("SQLite::write"s);
     _writeElapsed += STimeNow() - before;
     if (resultCode) {
+        _currentlyWriting = false;
         return false;
     }
 
@@ -592,6 +596,8 @@ bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) {
     if (alwaysKeepQueries || (schemaAfter > schemaBefore) || (changesAfter > changesBefore)) {
         _uncommittedQuery += usedRewrittenQuery ? _rewrittenQuery : query;
     }
+
+    _currentlyWriting = false;
     return true;
 }
 
@@ -929,6 +935,13 @@ int SQLite::_authorize(int actionCode, const char* detail1, const char* detail2,
         ) {
             _isDeterministicQuery = false;
         }
+
+        if (!strcmp(detail2, "current_timestamp")) {
+            if (_currentlyWriting) {
+                // Prevent using `current_timestamp` in writes which could cause synchronization with followers to result in inconsistent data.
+                return SQLITE_DENY;
+            }
+        }
     }
 
     // If the whitelist isn't set, we always return OK.

sqlitecluster/SQLite.h

@@ -506,4 +506,7 @@ class SQLite {
 
     // This is a string (which may be empty) containing the most recent logged error by SQLite in this thread.
     static thread_local string _mostRecentSQLiteErrorLog;
+
+    // Set to true inside of a write query.
+    bool _currentlyWriting{false};
 };

test/tests/WriteTest.cpp

@@ -18,6 +18,7 @@ struct WriteTest : tpunit::TestFixture {
                               TEST(WriteTest::updateAndInsertWithHttp),
                               TEST(WriteTest::shortHandSyntax),
                               TEST(WriteTest::keywordsAsValue),
+                              TEST(WriteTest::blockTimeFunctions),
                               AFTER_CLASS(WriteTest::tearDown)) { }
 
     BedrockTester* tester;
@@ -183,4 +184,18 @@ struct WriteTest : tpunit::TestFixture {
         tester->executeWaitVerifyContent(query3);
     }
 
+    void blockTimeFunctions() {
+        // Verify writing the string 'CURRENT_TIMESTAMP' is fine.
+        SData query("query: INSERT INTO stuff VALUES ( NULL, 11, 'CURRENT_TIMESTAMP' );");
+        tester->executeWaitVerifyContent(query);
+
+        // But verify calling the function CURRENT_TIMESTAMP is blocked when writing.
+        query.methodLine = "query: INSERT INTO stuff VALUES ( NULL, 11, CURRENT_TIMESTAMP );";
+        tester->executeWaitVerifyContent(query, "502 Query failed");
+
+        // But allow the function to run in reads.
+        query.methodLine = "query: SELECT CURRENT_TIMESTAMP;";
+        tester->executeWaitVerifyContent(query);
+    }
+
 } __WriteTest;