Parameterize offline-storage deletes and harden kill-switch response handling#1467
Open
bmehta001 wants to merge 7 commits into
Open
Parameterize offline-storage deletes and harden kill-switch response handling#1467bmehta001 wants to merge 7 commits into
bmehta001 wants to merge 7 commits into
Conversation
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>
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens offline-storage deletion and kill-switch handling by switching OfflineStorage_SQLite::DeleteRecords to a parameterized SQLite prepared statement, avoiding concatenation of untrusted values into SQL and preventing accidental table-wide deletes.
Changes:
- Reworks
OfflineStorage_SQLite::DeleteRecordsto use a whitelisted set of column names andsqlite3_bind_*value binding, and to bail out when no valid predicate is provided. - Makes
KillSwitchManager::handleResponseresilient to malformedRetry-After/kill-durationvalues and rejects malformed kill-token tenant IDs. - Adds unit tests covering deletion by
tenant_token(including SQL-injection-shaped input) and newKillSwitchManagerheader parsing behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/UnitTests.vcxproj.filters | Adds the new KillSwitchManager unit test source to VS project filters. |
| tests/unittests/UnitTests.vcxproj | Adds the new KillSwitchManager unit test source to the unit test project build. |
| tests/unittests/OfflineStorageTests_SQLite.cpp | Adds regression tests validating tenant-token deletes and SQL-injection-shaped input handling. |
| tests/unittests/KillSwitchManagerTests.cpp | Introduces coverage for Retry-After, kill-token, and kill-duration parsing/validation. |
| tests/unittests/CMakeLists.txt | Adds KillSwitchManager tests to the CMake unit test target sources. |
| lib/offline/OfflineStorage_SQLite.cpp | Implements parameterized DELETE statement creation/binding and avoids empty-predicate deletes. |
| lib/offline/KillSwitchManager.hpp | Adds safe duration parsing helper and tenant-token character validation before storing kill tokens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bmehta001
changed the title
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>
- 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>
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>
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>
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>
Comment on lines
+201
to
+206
| // A tenant token is the API ingestion key tenant id: alphanumeric with | ||
| // '-', '_' or '.' separators. Reject anything outside that set. | ||
| static bool isValidTenantToken(const std::string& token) | ||
| { | ||
| if (token.empty() || token.size() > 256) | ||
| { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds
OfflineStorage_SQLite::DeleteRecordsas a parameterized prepared statement and tightens kill-switch response handling.Changes
OfflineStorage_SQLite::DeleteRecordsnow builds a parameterized statement: column names are taken from a fixed whitelist and values are bound viasqlite3_bind_*on a single prepared 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.DeleteRecordsno longer issues aDELETEwith an empty predicate.KillSwitchManager::handleResponsevalidates kill-token values against the expected tenant-token character set before storing them.KillSwitchManager::handleResponsenow parses theRetry-Afterandkill-durationresponse headers through a non-throwing helper, so a malformed value (for example the RFC 7231 HTTP-date form ofRetry-After) is ignored rather than propagating out of the SDK worker thread.OfflineStorageTests_SQLiteregression tests for deletion bytenant_token(including values containing unusual characters), verifying that only matching records are removed.KillSwitchManagerTestscovering valid input plus malformedRetry-After/kill-durationvalues and a rejected malformed kill-token.Validation
Full
OfflineStorageTests_SQLitesuite (27 tests) plus newKillSwitchManagerTests(7 tests) pass on Windows x64 Debug - 34 total.