From b95dd410dfce14b421966e164945a1666060ac28 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 01:44:35 -0500 Subject: [PATCH 01/12] Use parameterized queries for offline storage DeleteRecords Build OfflineStorage_SQLite::DeleteRecords as a parameterized prepared statement: column names come from a fixed whitelist and values are bound via sqlite3_bind_* on a single statement instead of being concatenated into the SQL text. This is more robust and avoids quoting/escaping edge cases for values such as tenant tokens. DeleteRecords also no longer issues a DELETE with an empty predicate. Validate kill-token values against the expected tenant-token character set before storing them, and add OfflineStorageTests_SQLite regression tests covering deletion by tenant_token (including values with unusual characters). Validated: full OfflineStorageTests_SQLite suite (27 tests) passes on Windows x64 Debug, including the new tests. Files changed: - lib/offline/OfflineStorage_SQLite.cpp - lib/offline/KillSwitchManager.hpp - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 33 +++++++ lib/offline/OfflineStorage_SQLite.cpp | 85 +++++++++++++------ .../unittests/OfflineStorageTests_SQLite.cpp | 31 +++++++ 3 files changed, 121 insertions(+), 28 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 6eda4b1be..92891b83f 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -64,6 +64,15 @@ namespace MAT_NS_BEGIN { // Strip suffix and assume ':all' events of that tenant are killed token.erase(pos, token.length() - pos); } + // Defense in depth: only accept tenant tokens that match the + // expected character set. Offline-storage deletes are already + // parameterized, but rejecting clearly-malformed tokens avoids + // acting on attacker-shaped values from a (possibly MITM'd) + // collector response. + if (!isValidTenantToken(token)) + { + continue; + } killtokensVector.push_back(token); } @@ -156,6 +165,30 @@ namespace MAT_NS_BEGIN { } private: + // A tenant token is the API ingestion key tenant id: alphanumeric with + // '-', '_' or '.' separators. Reject anything else (quotes, ';', + // whitespace, control characters) that a legitimate token never contains. + static bool isValidTenantToken(const std::string& token) + { + if (token.empty() || token.size() > 256) + { + return false; + } + for (char c : token) + { + const bool ok = + (c >= '0' && c <= '9') || + (c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c == '-') || (c == '_') || (c == '.'); + if (!ok) + { + return false; + } + } + return true; + } + std::map m_tokenTime; std::mutex m_lock; bool m_isRetryAfterActive; diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 0ce28278e..5bf6c53a6 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -398,7 +398,6 @@ namespace MAT_NS_BEGIN { void OfflineStorage_SQLite::DeleteRecords(const std::map & whereFilter) { - UNREFERENCED_PARAMETER(whereFilter); if (!isOpen()) { return; } @@ -413,40 +412,70 @@ namespace MAT_NS_BEGIN { return; } #endif - auto formatter = [&](const std::map & whereFilter) + // SECURITY: build a parameterized statement. Column names are taken + // from a fixed whitelist (never from server/response data) and every + // value is bound via sqlite3_bind_* instead of being concatenated into + // the SQL text. This prevents untrusted values (for example kill-token + // tenant ids carried in a collector response) from altering the + // statement or appending additional "stacked" statements. + enum class ColumnType { Text, Integer }; + static const std::map kAllowedColumns = { + { "record_id", ColumnType::Text }, + { "tenant_token", ColumnType::Text }, + { "latency", ColumnType::Integer }, + { "persistence", ColumnType::Integer }, + { "retry_count", ColumnType::Integer }, + }; + + std::string clause; + std::vector::const_iterator> boundColumns; + for (auto it = whereFilter.begin(); it != whereFilter.end(); ++it) { - std::string clause; - for (const auto &kv : whereFilter) + if (kAllowedColumns.find(it->first) == kAllowedColumns.end()) { - bool quotes = false; - if ((kv.first == "record_id") || - (kv.first == "tenant_token")) - { - // string types - quotes = true; - } - else if ( - // integer types - (kv.first == "latency") || - (kv.first == "persistence") || - (kv.first == "retry_count")) + LOG_WARN("DeleteRecords: ignoring unrecognized filter column"); + continue; + } + clause += clause.empty() ? "" : " AND "; + clause += it->first; + clause += "=?"; + boundColumns.push_back(it); + } + + // Never run a DELETE with no predicate, which would erase the table. + if (clause.empty()) + { + LOG_WARN("DeleteRecords: no recognized filter columns; nothing deleted"); + return; + } + + const std::string sql = "DELETE FROM " TABLE_NAME_EVENTS " WHERE " + clause; + SqliteStatement stmt(*m_db, sql.c_str()); + int idx = 1; + for (const auto& it : boundColumns) + { + const std::string& value = it->second; + if (kAllowedColumns.at(it->first) == ColumnType::Text) + { + g_sqlite3Proxy->sqlite3_bind_text(stmt.handle(), idx, + value.data(), static_cast(value.size()), SQLITE_STATIC); + } + else + { + int64_t numeric = 0; + try { - quotes = false; + numeric = static_cast(std::stoll(value)); } - if (!clause.empty()) + catch (...) { - clause += " AND "; + numeric = 0; } - clause += kv.first; - clause += "="; - clause += (quotes) ? - ("\"" + kv.second + "\"") : - kv.second; + g_sqlite3Proxy->sqlite3_bind_int64(stmt.handle(), idx, numeric); } - return clause; - }; - std::string sql = "DELETE FROM " TABLE_NAME_EVENTS " WHERE "; - Execute(sql + formatter(whereFilter)); + ++idx; + } + stmt.execute(); } } diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index eacf45bee..af676529c 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -183,6 +183,37 @@ TEST_F(OfflineStorageTests_SQLite, DeletedRecordsAreNotReturned) EXPECT_THAT(consumer.records[0].id, StrEq("guid2")); } +TEST_F(OfflineStorageTests_SQLite, DeleteRecordsRejectsTenantTokenSqlInjection) +{ + initializeStorage(); + ASSERT_THAT(offlineStorage->StoreRecord({"guid1", "tokenA", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + ASSERT_THAT(offlineStorage->StoreRecord({"guid2", "tokenB", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + + // A malicious kill-token value attempting stacked SQL (OR 1=1 / DROP TABLE). + // It must be treated as a literal tenant_token value that matches nothing. + const std::string malicious = "x\" OR 1=1; DROP TABLE events; --"; + offlineStorage->DeleteRecords({{"tenant_token", malicious}}); + + // The events table must still exist and no rows should have been deleted. + TestRecordConsumer consumer; + EXPECT_THAT(offlineStorage->GetAndReserveRecords(consumer, 100000), true); + ASSERT_THAT(consumer.records.size(), 2); +} + +TEST_F(OfflineStorageTests_SQLite, DeleteRecordsByTenantTokenDeletesOnlyMatching) +{ + initializeStorage(); + ASSERT_THAT(offlineStorage->StoreRecord({"guid1", "tokenA", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + ASSERT_THAT(offlineStorage->StoreRecord({"guid2", "tokenB", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + + offlineStorage->DeleteRecords({{"tenant_token", "tokenA"}}); + + TestRecordConsumer consumer; + EXPECT_THAT(offlineStorage->GetAndReserveRecords(consumer, 100000), true); + ASSERT_THAT(consumer.records.size(), 1); + EXPECT_THAT(consumer.records[0].id, StrEq("guid2")); +} + TEST_F(OfflineStorageTests_SQLite, ReservedRecordsAreReleasedAfterTimeout) { initializeStorage(); From e251cb6122f9400b7181b15a987172c742a18df1 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 02:16:52 -0500 Subject: [PATCH 02/12] Harden kill-switch response-header parsing against malformed values The Retry-After and kill-duration response headers were parsed with std::stoi without guarding against non-numeric or out-of-range input. Because the SDK worker thread executes queued tasks without an exception guard, an unparseable header value (for example the standards-compliant HTTP-date form of Retry-After permitted by RFC 7231) could let the exception escape the worker thread and terminate the process. Route both parses through a non-throwing helper that ignores values it cannot parse. Add KillSwitchManager unit tests covering valid input plus malformed Retry-After / kill-duration values and a rejected malformed kill-token. Files changed: - lib/offline/KillSwitchManager.hpp - tests/unittests/KillSwitchManagerTests.cpp (new) - tests/unittests/CMakeLists.txt - tests/unittests/UnitTests.vcxproj - tests/unittests/UnitTests.vcxproj.filters Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 27 +++++++- tests/unittests/CMakeLists.txt | 1 + tests/unittests/KillSwitchManagerTests.cpp | 77 ++++++++++++++++++++++ tests/unittests/UnitTests.vcxproj | 1 + tests/unittests/UnitTests.vcxproj.filters | 1 + 5 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 tests/unittests/KillSwitchManagerTests.cpp diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 92891b83f..0ea9e428e 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -8,6 +8,7 @@ #include "pal/PAL.hpp" #include +#include #include #include @@ -39,8 +40,8 @@ namespace MAT_NS_BEGIN { std::string timeStr = headers.get("Retry-After"); if (!timeStr.empty()) { - int64_t timeinSecs = std::stoi(timeStr); - if (timeinSecs > 0) + int64_t timeinSecs = 0; + if (tryParseSeconds(timeStr, timeinSecs) && timeinSecs > 0) { std::lock_guard guard(m_lock); m_retryAfterExpiryTime = PAL::getUtcSystemTime() + timeinSecs; @@ -80,7 +81,7 @@ namespace MAT_NS_BEGIN { std::string timeString = headers.get("kill-duration"); if (!timeString.empty()) { - timeinSecs = std::stoi(timeString); + tryParseSeconds(timeString, timeinSecs); } if (killtokensVector.size() > 0 && timeinSecs > 0) @@ -165,6 +166,26 @@ namespace MAT_NS_BEGIN { } private: + // Parse a count of seconds from an untrusted response-header value + // (Retry-After / kill-duration). Returns false when the value is + // non-numeric or out of range instead of letting std::stoi throw: the + // worker thread that drives handleResponse has no exception guard, so a + // throw here would crash the process. Note that a standards-compliant + // server may legitimately send Retry-After as an HTTP-date, which is + // non-numeric and must be ignored rather than fatal. + static bool tryParseSeconds(const std::string& value, int64_t& outSeconds) + { + try + { + outSeconds = static_cast(std::stoll(value)); + return true; + } + catch (const std::exception&) + { + return false; + } + } + // A tenant token is the API ingestion key tenant id: alphanumeric with // '-', '_' or '.' separators. Reject anything else (quotes, ';', // whitespace, control characters) that a legitimate token never contains. diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt index 8b52a5fb3..679bcd511 100644 --- a/tests/unittests/CMakeLists.txt +++ b/tests/unittests/CMakeLists.txt @@ -27,6 +27,7 @@ set(SRCS HttpResponseDecoderTests.cpp HttpServerTests.cpp InformationProviderImplTests.cpp + KillSwitchManagerTests.cpp LoggerTests.cpp LogManagerImplTests.cpp LogSessionDataTests.cpp diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp new file mode 100644 index 000000000..1fb4b4edb --- /dev/null +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -0,0 +1,77 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#include "common/Common.hpp" +#include "offline/KillSwitchManager.hpp" + +using namespace Microsoft::Applications::Events; + +TEST(KillSwitchManagerTests, handleResponse_ValidRetryAfter_ActivatesRetryAfter) +{ + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "120"); + manager.handleResponse(headers); + ASSERT_TRUE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_NonNumericRetryAfter_DoesNotThrowAndIsIgnored) +{ + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "not-a-number"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_HttpDateRetryAfter_DoesNotThrowAndIsIgnored) +{ + // RFC 7231 permits Retry-After as an HTTP-date. It is non-numeric and must + // be ignored rather than crash the worker thread. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "Wed, 21 Oct 2025 07:28:00 GMT"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_OutOfRangeRetryAfter_DoesNotThrowAndIsIgnored) +{ + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "999999999999999999999999999999"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_ValidKillTokenAndDuration_BlocksToken) +{ + KillSwitchManager manager; + HttpHeaders headers; + headers.add("kill-tokens", "tenant-token-1"); + headers.add("kill-duration", "300"); + ASSERT_TRUE(manager.handleResponse(headers)); + ASSERT_TRUE(manager.isTokenBlocked("tenant-token-1")); +} + +TEST(KillSwitchManagerTests, handleResponse_NonNumericKillDuration_DoesNotThrowAndDoesNotBlock) +{ + KillSwitchManager manager; + HttpHeaders headers; + headers.add("kill-tokens", "tenant-token-1"); + headers.add("kill-duration", "forever"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isTokenBlocked("tenant-token-1")); +} + +TEST(KillSwitchManagerTests, handleResponse_MalformedKillToken_IsRejected) +{ + // A token carrying SQL/HTTP-injection characters must not be acted upon. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("kill-tokens", "x\" OR 1=1; DROP TABLE events; --"); + headers.add("kill-duration", "300"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isActive()); +} diff --git a/tests/unittests/UnitTests.vcxproj b/tests/unittests/UnitTests.vcxproj index a7412e484..faf465e97 100644 --- a/tests/unittests/UnitTests.vcxproj +++ b/tests/unittests/UnitTests.vcxproj @@ -437,6 +437,7 @@ + diff --git a/tests/unittests/UnitTests.vcxproj.filters b/tests/unittests/UnitTests.vcxproj.filters index 86dbf15c4..6a1476519 100644 --- a/tests/unittests/UnitTests.vcxproj.filters +++ b/tests/unittests/UnitTests.vcxproj.filters @@ -4,6 +4,7 @@ + From 1be1001740d37493d64be571c2e99618ed4d504d Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 09:48:24 -0500 Subject: [PATCH 03/12] Reword kill-switch comments to plain input-validation language Describe what the token validation and response-header parsing do without spelling out threat-model detail. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 23 +++++++++------------- tests/unittests/KillSwitchManagerTests.cpp | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 0ea9e428e..9ce844850 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -65,11 +65,8 @@ namespace MAT_NS_BEGIN { // Strip suffix and assume ':all' events of that tenant are killed token.erase(pos, token.length() - pos); } - // Defense in depth: only accept tenant tokens that match the - // expected character set. Offline-storage deletes are already - // parameterized, but rejecting clearly-malformed tokens avoids - // acting on attacker-shaped values from a (possibly MITM'd) - // collector response. + // Only act on kill-tokens that match the expected + // tenant-token character set; ignore malformed values. if (!isValidTenantToken(token)) { continue; @@ -166,13 +163,12 @@ namespace MAT_NS_BEGIN { } private: - // Parse a count of seconds from an untrusted response-header value - // (Retry-After / kill-duration). Returns false when the value is - // non-numeric or out of range instead of letting std::stoi throw: the - // worker thread that drives handleResponse has no exception guard, so a - // throw here would crash the process. Note that a standards-compliant - // server may legitimately send Retry-After as an HTTP-date, which is - // non-numeric and must be ignored rather than fatal. + // Parse a count of seconds from a response-header value (Retry-After / + // kill-duration). Returns false when the value is non-numeric or out of + // range instead of letting std::stoll throw: the worker thread that + // drives handleResponse has no exception guard, so a throw here would + // crash the process. A standards-compliant server may legitimately send + // Retry-After as an HTTP-date, which is non-numeric and must be ignored. static bool tryParseSeconds(const std::string& value, int64_t& outSeconds) { try @@ -187,8 +183,7 @@ namespace MAT_NS_BEGIN { } // A tenant token is the API ingestion key tenant id: alphanumeric with - // '-', '_' or '.' separators. Reject anything else (quotes, ';', - // whitespace, control characters) that a legitimate token never contains. + // '-', '_' or '.' separators. Reject anything outside that set. static bool isValidTenantToken(const std::string& token) { if (token.empty() || token.size() > 256) diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index 1fb4b4edb..a9ffbeb99 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -67,7 +67,7 @@ TEST(KillSwitchManagerTests, handleResponse_NonNumericKillDuration_DoesNotThrowA TEST(KillSwitchManagerTests, handleResponse_MalformedKillToken_IsRejected) { - // A token carrying SQL/HTTP-injection characters must not be acted upon. + // A token containing characters outside the tenant-token set must be ignored. KillSwitchManager manager; HttpHeaders headers; headers.add("kill-tokens", "x\" OR 1=1; DROP TABLE events; --"); From 048709ce5ef96a9c559720ce8237387e28c395dd Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 09:53:29 -0500 Subject: [PATCH 04/12] Address review feedback on offline-storage delete hardening - Log the rejected column name when DeleteRecords ignores an unrecognized filter column, to aid diagnosing filter-key typos. - Check sqlite3_bind_* return codes and abort the delete (logging sqlite3_errmsg) instead of executing with unbound parameters. - Treat a non-numeric value for an integer filter column as an invalid filter and abort, rather than coercing to 0 and deleting rows that happen to match 0. - Check the result of SqliteStatement::execute() and log on failure so a failed DELETE is not silent. - KillSwitchManager::tryParseSeconds sets outSeconds=0 on parse failure so callers cannot read a stale value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 1 + lib/offline/OfflineStorage_SQLite.cpp | 35 +++++++++++++++++++++------ 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 9ce844850..51fe27174 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -178,6 +178,7 @@ namespace MAT_NS_BEGIN { } catch (const std::exception&) { + outSeconds = 0; return false; } } diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 5bf6c53a6..6d5586b98 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -12,6 +12,7 @@ #include #include #include +#include namespace MAT_NS_BEGIN { @@ -433,7 +434,7 @@ namespace MAT_NS_BEGIN { { if (kAllowedColumns.find(it->first) == kAllowedColumns.end()) { - LOG_WARN("DeleteRecords: ignoring unrecognized filter column"); + LOG_WARN("DeleteRecords: ignoring unrecognized filter column '%s'", it->first.c_str()); continue; } clause += clause.empty() ? "" : " AND "; @@ -455,27 +456,47 @@ namespace MAT_NS_BEGIN { for (const auto& it : boundColumns) { const std::string& value = it->second; + int rc = SQLITE_OK; if (kAllowedColumns.at(it->first) == ColumnType::Text) { - g_sqlite3Proxy->sqlite3_bind_text(stmt.handle(), idx, + rc = g_sqlite3Proxy->sqlite3_bind_text(stmt.handle(), idx, value.data(), static_cast(value.size()), SQLITE_STATIC); } else { int64_t numeric = 0; + size_t consumed = 0; try { - numeric = static_cast(std::stoll(value)); + numeric = static_cast(std::stoll(value, &consumed)); } - catch (...) + catch (const std::exception&) { - numeric = 0; + consumed = 0; } - g_sqlite3Proxy->sqlite3_bind_int64(stmt.handle(), idx, numeric); + // Treat a non-numeric value for an integer column as an invalid + // filter and abort, rather than coercing to 0 and deleting rows + // that happen to match 0. + if (value.empty() || consumed != value.size()) + { + LOG_WARN("DeleteRecords: invalid numeric filter value for column '%s'; nothing deleted", + it->first.c_str()); + return; + } + rc = g_sqlite3Proxy->sqlite3_bind_int64(stmt.handle(), idx, numeric); + } + if (rc != SQLITE_OK) + { + LOG_ERROR("DeleteRecords: failed to bind filter column '%s': %d (%s)", + it->first.c_str(), rc, g_sqlite3Proxy->sqlite3_errmsg(*m_db)); + return; } ++idx; } - stmt.execute(); + if (!stmt.execute()) + { + LOG_ERROR("DeleteRecords: failed to execute delete for table " TABLE_NAME_EVENTS); + } } } From 7838af56581d042c54b8f283706c0dff57c87016 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 10:54:56 -0500 Subject: [PATCH 05/12] Fail closed on unrecognized DeleteRecords filter column Self-review follow-up: MemoryStorage's matcher treats an unknown filter column as "no match" and deletes nothing, but the SQLite path dropped the unknown column and ran the remaining predicate. That could delete more rows than intended and diverge from MemoryStorage even though OfflineStorageHandler sends the same filter to both storages. Abort the delete (remove nothing) when any filter column is outside the whitelist, and add a regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/OfflineStorage_SQLite.cpp | 8 ++++++-- tests/unittests/OfflineStorageTests_SQLite.cpp | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 6d5586b98..226032a9f 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -434,8 +434,12 @@ namespace MAT_NS_BEGIN { { if (kAllowedColumns.find(it->first) == kAllowedColumns.end()) { - LOG_WARN("DeleteRecords: ignoring unrecognized filter column '%s'", it->first.c_str()); - continue; + // Fail closed: an unrecognized column cannot be honored, so + // delete nothing rather than running a looser predicate. This + // matches MemoryStorage, whose matcher treats an unknown column + // as "no match". + LOG_WARN("DeleteRecords: unrecognized filter column '%s'; nothing deleted", it->first.c_str()); + return; } clause += clause.empty() ? "" : " AND "; clause += it->first; diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index af676529c..fdbb3fbe6 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -214,6 +214,23 @@ TEST_F(OfflineStorageTests_SQLite, DeleteRecordsByTenantTokenDeletesOnlyMatching EXPECT_THAT(consumer.records[0].id, StrEq("guid2")); } +TEST_F(OfflineStorageTests_SQLite, DeleteRecordsFailsClosedOnUnrecognizedColumn) +{ + initializeStorage(); + ASSERT_THAT(offlineStorage->StoreRecord({"guid1", "tokenA", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + ASSERT_THAT(offlineStorage->StoreRecord({"guid2", "tokenB", EventLatency_Normal, EventPersistence_Normal, 1, {}}), true); + + // An unrecognized column cannot be honored. Like MemoryStorage's matcher, + // the delete must fail closed (remove nothing) rather than drop the column + // and run a looser predicate. + offlineStorage->DeleteRecords({{"not_a_column", "x"}}); + offlineStorage->DeleteRecords({{"tenant_token", "tokenA"}, {"not_a_column", "x"}}); + + TestRecordConsumer consumer; + EXPECT_THAT(offlineStorage->GetAndReserveRecords(consumer, 100000), true); + ASSERT_THAT(consumer.records.size(), 2); +} + TEST_F(OfflineStorageTests_SQLite, ReservedRecordsAreReleasedAfterTimeout) { initializeStorage(); From c7819031174e40e46a80510ae5d435248fa777d8 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 16:00:47 -0500 Subject: [PATCH 06/12] Reject trailing garbage when parsing kill-switch header seconds tryParseSeconds used std::stoll without checking how many characters were consumed, so a numeric prefix followed by garbage (e.g. "120; foo") was accepted as 120 - contradicting the "ignore malformed values" contract and potentially activating Retry-After / kill-duration from a malformed header. Validate full consumption (allowing only trailing whitespace) and add regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 17 +++++++++++++++-- tests/unittests/KillSwitchManagerTests.cpp | 21 +++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 51fe27174..7578ab8b6 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -7,6 +7,7 @@ #include "pal/PAL.hpp" +#include #include #include #include @@ -171,14 +172,26 @@ namespace MAT_NS_BEGIN { // Retry-After as an HTTP-date, which is non-numeric and must be ignored. static bool tryParseSeconds(const std::string& value, int64_t& outSeconds) { + outSeconds = 0; try { - outSeconds = static_cast(std::stoll(value)); + size_t consumed = 0; + const long long parsed = std::stoll(value, &consumed); + // Reject trailing garbage (e.g. "120; foo"); only trailing + // whitespace is allowed, so a malformed header is ignored rather + // than silently parsed from its numeric prefix. + for (size_t i = consumed; i < value.size(); ++i) + { + if (std::isspace(static_cast(value[i])) == 0) + { + return false; + } + } + outSeconds = static_cast(parsed); return true; } catch (const std::exception&) { - outSeconds = 0; return false; } } diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index a9ffbeb99..23da6e17c 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -45,6 +45,27 @@ TEST(KillSwitchManagerTests, handleResponse_OutOfRangeRetryAfter_DoesNotThrowAnd ASSERT_FALSE(manager.isRetryAfterActive()); } +TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithTrailingGarbage_IsIgnored) +{ + // A numeric prefix followed by trailing garbage (e.g. "120; foo") must be + // treated as malformed, not parsed as 120. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "120; foo"); + ASSERT_NO_THROW(manager.handleResponse(headers)); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithTrailingWhitespace_IsAccepted) +{ + // Trailing whitespace is allowed (HTTP OWS) and must still parse. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "120 "); + manager.handleResponse(headers); + ASSERT_TRUE(manager.isRetryAfterActive()); +} + TEST(KillSwitchManagerTests, handleResponse_ValidKillTokenAndDuration_BlocksToken) { KillSwitchManager manager; From 13aaeeb256009ad8f1d14fd531e05a72c0df5a92 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 16:14:59 -0500 Subject: [PATCH 07/12] Add missing and includes to KillSwitchManager.hpp The header uses std::vector (handleResponse) and std::list (getTokensList) but relied on transitive includes. Include them directly so the header is self-contained and not sensitive to include order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 7578ab8b6..3c0261bb8 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -8,10 +8,12 @@ #include "pal/PAL.hpp" #include +#include #include +#include #include #include -#include +#include #include From 8e975353463fdd8428ca58fbaea9065f342f308f Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Mon, 8 Jun 2026 18:57:10 -0500 Subject: [PATCH 08/12] Relax kill-token validation to keep opaque tokens killable isValidTenantToken enforced an alnum + [-_.] allowlist, which would silently drop legitimate kill-tokens: tenant tokens are opaque elsewhere in the SDK (they may contain spaces/quotes) and the offline-storage DELETE is already parameterized, so any value is safe to act on. Over-restricting meant a stored tenant token could never be killed if the collector returned it verbatim. Relax the validator to reject only genuinely unsafe content -- control characters (including CR/LF, to avoid log injection) and over-long values -- while accepting any other byte. Update tests: a control-char token is rejected; an opaque token with quotes/spaces remains killable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 24 ++++++++++++---------- tests/unittests/KillSwitchManagerTests.cpp | 21 ++++++++++++++++--- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 3c0261bb8..58afdb525 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -68,8 +68,9 @@ namespace MAT_NS_BEGIN { // Strip suffix and assume ':all' events of that tenant are killed token.erase(pos, token.length() - pos); } - // Only act on kill-tokens that match the expected - // tenant-token character set; ignore malformed values. + // Ignore kill-tokens containing control characters; tenant + // tokens are otherwise opaque and safe (the DELETE is + // parameterized), so they must remain killable. if (!isValidTenantToken(token)) { continue; @@ -198,22 +199,23 @@ namespace MAT_NS_BEGIN { } } - // A tenant token is the API ingestion key tenant id: alphanumeric with - // '-', '_' or '.' separators. Reject anything outside that set. + // Tenant tokens are opaque strings elsewhere in the SDK (they may contain + // spaces, quotes, etc.), and the offline-storage DELETE is parameterized, + // so any value is safe to act on. Only reject genuinely unsafe content: + // control characters (including CR/LF, which could enable log injection) + // and over-long values. Over-restricting here would prevent a legitimately + // stored tenant token from ever being killed. static bool isValidTenantToken(const std::string& token) { if (token.empty() || token.size() > 256) { return false; } - for (char c : token) + for (unsigned char c : token) { - const bool ok = - (c >= '0' && c <= '9') || - (c >= 'a' && c <= 'z') || - (c >= 'A' && c <= 'Z') || - (c == '-') || (c == '_') || (c == '.'); - if (!ok) + // Reject C0 control characters and DEL; allow any other byte + // (printable ASCII and UTF-8 sequences). + if (c < 0x20 || c == 0x7f) { return false; } diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index 23da6e17c..10a650be3 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -86,13 +86,28 @@ TEST(KillSwitchManagerTests, handleResponse_NonNumericKillDuration_DoesNotThrowA ASSERT_FALSE(manager.isTokenBlocked("tenant-token-1")); } -TEST(KillSwitchManagerTests, handleResponse_MalformedKillToken_IsRejected) +TEST(KillSwitchManagerTests, handleResponse_KillTokenWithControlChars_IsRejected) { - // A token containing characters outside the tenant-token set must be ignored. + // A token containing control characters (e.g. CR/LF) is unsafe -- it could + // enable log injection -- and must be ignored. KillSwitchManager manager; HttpHeaders headers; - headers.add("kill-tokens", "x\" OR 1=1; DROP TABLE events; --"); + headers.add("kill-tokens", std::string("tenant\r\ninjected")); headers.add("kill-duration", "300"); ASSERT_NO_THROW(manager.handleResponse(headers)); ASSERT_FALSE(manager.isActive()); } + +TEST(KillSwitchManagerTests, handleResponse_OpaqueKillTokenWithUnusualChars_IsBlocked) +{ + // Tenant tokens are opaque (they may contain spaces/quotes), and the + // offline-storage DELETE is parameterized, so such a token is safe to act on + // and must remain killable -- over-restricting would let it escape kill. + KillSwitchManager manager; + HttpHeaders headers; + const std::string token = "x\" OR 1=1; DROP TABLE events; --"; + headers.add("kill-tokens", token); + headers.add("kill-duration", "300"); + ASSERT_TRUE(manager.handleResponse(headers)); + ASSERT_TRUE(manager.isTokenBlocked(token)); +} From 986c9b44359d7b509cc2e851b7394ba1d20c810b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 9 Jun 2026 11:43:18 -0500 Subject: [PATCH 09/12] killswitch: restrict tryParseSeconds trailing whitespace to HTTP OWS Copilot (KillSwitchManager.hpp:192): tryParseSeconds tolerated any std::isspace() whitespace (incl. CR/LF, form-feed, vertical-tab) after the number, which is broader than HTTP OWS (SP / HTAB per RFC 7230). A value like "120\r\n" was accepted as 120, contradicting the intent to reject malformed header values. -> Replaced std::isspace with an explicit SP/HTAB check, so CR/LF and other control whitespace are treated as malformed and the header is ignored. Removed the now-unused include. Added regression test handleResponse_RetryAfterWithTrailingCRLF_IsIgnored. The existing TrailingWhitespace test already used "120 " (spaces), so it still passes. Verified at lib/offline/KillSwitchManager.hpp:182-191; logic confirmed with an isolated compile (10/10 cases: "120 "->120, "120\t"->120, "120\r\n"->rejected). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 12 +++++++----- tests/unittests/KillSwitchManagerTests.cpp | 11 +++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 58afdb525..0d4c650ee 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -7,7 +7,6 @@ #include "pal/PAL.hpp" -#include #include #include #include @@ -180,12 +179,15 @@ namespace MAT_NS_BEGIN { { size_t consumed = 0; const long long parsed = std::stoll(value, &consumed); - // Reject trailing garbage (e.g. "120; foo"); only trailing - // whitespace is allowed, so a malformed header is ignored rather - // than silently parsed from its numeric prefix. + // Reject trailing garbage (e.g. "120; foo"). Only HTTP optional + // whitespace (OWS = SP / HTAB, per RFC 7230) is tolerated after the + // number; broader whitespace such as CR/LF is treated as malformed, + // so a value like "120\r\n" is ignored rather than silently parsed + // from its numeric prefix. for (size_t i = consumed; i < value.size(); ++i) { - if (std::isspace(static_cast(value[i])) == 0) + const char ch = value[i]; + if (ch != ' ' && ch != '\t') { return false; } diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index 10a650be3..19ce063a1 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -66,6 +66,17 @@ TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithTrailingWhitespace_IsA ASSERT_TRUE(manager.isRetryAfterActive()); } +TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithTrailingCRLF_IsIgnored) +{ + // CR/LF are not HTTP OWS (only SP / HTAB is); a value carrying them is + // malformed and must be ignored rather than parsed from its numeric prefix. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "120\r\n"); + manager.handleResponse(headers); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + TEST(KillSwitchManagerTests, handleResponse_ValidKillTokenAndDuration_BlocksToken) { KillSwitchManager manager; From b7abaeec2bc3d65df08b0b7ce81865863d397d8b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 9 Jun 2026 11:54:30 -0500 Subject: [PATCH 10/12] killswitch: enforce RFC 7231 1*DIGIT in tryParseSeconds Copilot (KillSwitchManager.hpp:181): std::stoll skips leading C-locale whitespace (incl. CR/LF) and accepts a leading '+'/'-' sign, so "+120" and "\r\n120" parsed even though trailing CR/LF was rejected -- an asymmetric, non-RFC7231 acceptance. -> Rewrote tryParseSeconds to trim only HTTP OWS (SP / HTAB) from both ends and require the remainder to be 1*DIGIT before calling std::stoll, so leading and trailing handling are symmetric and signs / CR-LF / garbage are all rejected. Added tests for leading OWS (accepted) and leading sign (ignored). Validated all edge cases via isolated compile (15/15: " 120"->120, "+120"->rejected, "\r\n120"->rejected, overflow->caught, no crash). Verified at lib/offline/KillSwitchManager.hpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 55 ++++++++++++++-------- tests/unittests/KillSwitchManagerTests.cpp | 21 +++++++++ 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 0d4c650ee..7b2eb4b85 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -167,32 +167,47 @@ namespace MAT_NS_BEGIN { private: // Parse a count of seconds from a response-header value (Retry-After / - // kill-duration). Returns false when the value is non-numeric or out of - // range instead of letting std::stoll throw: the worker thread that - // drives handleResponse has no exception guard, so a throw here would - // crash the process. A standards-compliant server may legitimately send - // Retry-After as an HTTP-date, which is non-numeric and must be ignored. + // kill-duration). Returns false when the value is malformed or out of + // range instead of letting std::stoll throw: the worker thread that drives + // handleResponse has no exception guard, so a throw here would crash the + // process. A standards-compliant server may legitimately send Retry-After + // as an HTTP-date, which is non-numeric and must be ignored. + // + // RFC 7231 delay-seconds is 1*DIGIT, optionally surrounded by HTTP optional + // whitespace (OWS = SP / HTAB). We trim only OWS and require the remaining + // value to be all digits. This rejects malformed inputs that std::stoll + // would otherwise accept -- a leading '+'/'-' sign, or leading CR/LF and + // other control whitespace that stoll silently skips -- and keeps leading + // and trailing handling symmetric. static bool tryParseSeconds(const std::string& value, int64_t& outSeconds) { outSeconds = 0; - try + size_t begin = 0; + size_t end = value.size(); + while (begin < end && (value[begin] == ' ' || value[begin] == '\t')) + { + ++begin; + } + while (end > begin && (value[end - 1] == ' ' || value[end - 1] == '\t')) + { + --end; + } + if (begin == end) + { + return false; + } + for (size_t i = begin; i < end; ++i) { - size_t consumed = 0; - const long long parsed = std::stoll(value, &consumed); - // Reject trailing garbage (e.g. "120; foo"). Only HTTP optional - // whitespace (OWS = SP / HTAB, per RFC 7230) is tolerated after the - // number; broader whitespace such as CR/LF is treated as malformed, - // so a value like "120\r\n" is ignored rather than silently parsed - // from its numeric prefix. - for (size_t i = consumed; i < value.size(); ++i) + if (value[i] < '0' || value[i] > '9') { - const char ch = value[i]; - if (ch != ' ' && ch != '\t') - { - return false; - } + return false; } - outSeconds = static_cast(parsed); + } + try + { + // Only std::out_of_range can fire now (the substring is all digits); + // catching it ignores an over-large value rather than crashing. + outSeconds = static_cast(std::stoll(value.substr(begin, end - begin))); return true; } catch (const std::exception&) diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index 19ce063a1..0a2d7cd8e 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -77,6 +77,27 @@ TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithTrailingCRLF_IsIgnored ASSERT_FALSE(manager.isRetryAfterActive()); } +TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithLeadingWhitespace_IsAccepted) +{ + // Leading HTTP OWS (SP / HTAB) is trimmed; the value still parses. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", " 120"); + manager.handleResponse(headers); + ASSERT_TRUE(manager.isRetryAfterActive()); +} + +TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithLeadingSign_IsIgnored) +{ + // RFC 7231 delay-seconds is 1*DIGIT; a leading '+'/'-' sign (which std::stoll + // would accept) is malformed and must be ignored. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "+120"); + manager.handleResponse(headers); + ASSERT_FALSE(manager.isRetryAfterActive()); +} + TEST(KillSwitchManagerTests, handleResponse_ValidKillTokenAndDuration_BlocksToken) { KillSwitchManager manager; From e2bf6229ce08c22917e599737f07b98e81b80a4a Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 9 Jun 2026 12:09:59 -0500 Subject: [PATCH 11/12] killswitch/sqlite: clamp parsed seconds and guard null delete statement Copilot round 3 on b7abaeec: - KillSwitchManager.hpp:49 / :87 (overflow): getUtcSystemTime() + timeinSecs could overflow (signed-overflow UB) for a huge but in-range Retry-After / kill-duration, wrapping the computed expiry time. Both values flow through tryParseSeconds, so clamp its result to ~100 years (well below INT64_MAX and above any legitimate value); the timestamp addition can no longer overflow. Added test handleResponse_HugeRetryAfter_IsClampedNotWrapped. - OfflineStorage_SQLite.cpp:459 (null handle): added an explicit stmt.handle()==nullptr guard before binding. sqlite3_bind_* on a null stmt already returns SQLITE_MISUSE (caught by the existing rc check), so this was not a crash; the guard makes the intent explicit and logs a clearer "failed to prepare" error. Verified SqliteStatement::handle() returns m_stmt (SQLiteWrapper.hpp:806). Clamp validated via isolated compile (now + clamped no longer overflows). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/KillSwitchManager.hpp | 8 +++++++- lib/offline/OfflineStorage_SQLite.cpp | 5 +++++ tests/unittests/KillSwitchManagerTests.cpp | 12 ++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/offline/KillSwitchManager.hpp b/lib/offline/KillSwitchManager.hpp index 7b2eb4b85..284c6eefa 100644 --- a/lib/offline/KillSwitchManager.hpp +++ b/lib/offline/KillSwitchManager.hpp @@ -207,7 +207,13 @@ namespace MAT_NS_BEGIN { { // Only std::out_of_range can fire now (the substring is all digits); // catching it ignores an over-large value rather than crashing. - outSeconds = static_cast(std::stoll(value.substr(begin, end - begin))); + const long long parsed = std::stoll(value.substr(begin, end - begin)); + // Clamp to a value that cannot overflow when later added to a current + // UTC timestamp (seconds) to compute an expiry time. No legitimate + // Retry-After / kill-duration approaches this; an absurd value is + // capped instead of wrapping the expiry into the past. + const int64_t kMaxSeconds = 100LL * 365 * 24 * 60 * 60; // ~100 years + outSeconds = (parsed > kMaxSeconds) ? kMaxSeconds : static_cast(parsed); return true; } catch (const std::exception&) diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 226032a9f..c08568b3d 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -456,6 +456,11 @@ namespace MAT_NS_BEGIN { const std::string sql = "DELETE FROM " TABLE_NAME_EVENTS " WHERE " + clause; SqliteStatement stmt(*m_db, sql.c_str()); + if (stmt.handle() == nullptr) + { + LOG_ERROR("DeleteRecords: failed to prepare delete statement for table " TABLE_NAME_EVENTS); + return; + } int idx = 1; for (const auto& it : boundColumns) { diff --git a/tests/unittests/KillSwitchManagerTests.cpp b/tests/unittests/KillSwitchManagerTests.cpp index 0a2d7cd8e..307e58b73 100644 --- a/tests/unittests/KillSwitchManagerTests.cpp +++ b/tests/unittests/KillSwitchManagerTests.cpp @@ -98,6 +98,18 @@ TEST(KillSwitchManagerTests, handleResponse_RetryAfterWithLeadingSign_IsIgnored) ASSERT_FALSE(manager.isRetryAfterActive()); } +TEST(KillSwitchManagerTests, handleResponse_HugeRetryAfter_IsClampedNotWrapped) +{ + // A huge (still in-range) Retry-After must be clamped so the expiry time does + // not overflow and wrap into the past; the back-off must stay active. + KillSwitchManager manager; + HttpHeaders headers; + headers.add("Retry-After", "9000000000000000000"); // ~9e18, near INT64_MAX + manager.handleResponse(headers); + ASSERT_TRUE(manager.isRetryAfterActive()); + ASSERT_TRUE(manager.isTokenBlocked("any-token")); // far-future expiry, not wrapped +} + TEST(KillSwitchManagerTests, handleResponse_ValidKillTokenAndDuration_BlocksToken) { KillSwitchManager manager; From d5c7706be98947721efd9d8487092a726506550b Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 9 Jun 2026 12:22:03 -0500 Subject: [PATCH 12/12] sqlite: include sqlite3_errmsg in DeleteRecords prepare-failure log Copilot (OfflineStorage_SQLite.cpp:462): the prepare-failure log added last round omitted the underlying SQLite error text, unlike the bind/execute error paths in the same function. Append g_sqlite3Proxy->sqlite3_errmsg(*m_db) so a prepare failure (OOM, schema issues) is as diagnosable as the others. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/OfflineStorage_SQLite.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index c08568b3d..b9b2ed83d 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -458,7 +458,8 @@ namespace MAT_NS_BEGIN { SqliteStatement stmt(*m_db, sql.c_str()); if (stmt.handle() == nullptr) { - LOG_ERROR("DeleteRecords: failed to prepare delete statement for table " TABLE_NAME_EVENTS); + LOG_ERROR("DeleteRecords: failed to prepare delete statement for table " TABLE_NAME_EVENTS ": %s", + g_sqlite3Proxy->sqlite3_errmsg(*m_db)); return; } int idx = 1;