fix(OSQUERY-002-2): CU-86akhf8u2 16 review findings across 12 files

[flamingo]

Closes 16 review findings across 12 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🔴 55 low — review closely	initOpenFrame swallows all initialization failures behind a broad try/catch instead of returning Status	osquery/core/init.cpp:220
2	🔴 40 low — review closely	openframe_authorization_manager.cpp is not the file included in init.cpp — OpenframeAuthorizationManagerProvider is used but no such symbol exists	osquery/core/init.cpp:225
3	🟢 95 high	std::cerr used for user-facing validator message instead of LOG()/systemLog only	osquery/core/init.cpp:258
4	🟡 65 medium	SystemStateTracker::create() and constructor throw Status instead of returning it	osquery/events/linux/bpf/systemstatetracker.cpp:41
5	🟢 95 high	expireProcessContexts frees process map entries on any fs.fileExists() failure, not just confirmed non-existence	osquery/events/linux/bpf/systemstatetracker.cpp:314
6	🔴 35 low — review closely	createConnection() throws std::runtime_error/std::system_error instead of returning Status	osquery/remote/http_client.cpp:131
7	🔴 35 low — review closely	initHTTPRequest() throws std::runtime_error for missing hostname	osquery/remote/http_client.cpp:351
8	🟢 95 high	aws_util.cpp validator callback writes to std::cerr and uses a stray namespace-close comment mid-function	osquery/utils/aws/aws_util.cpp:163
9	🟢 95 high	YARAEventSubscriber::Callback catches std::bad_cast from a non-throwing dynamic_pointer_cast	osquery/tables/yara/yara_events.cpp:156
10	🟡 88 medium	DistributedRunner::start() silently discards Status failures instead of surfacing them consistently	osquery/dispatcher/distributed_runner.cpp:52
11	🟡 80 medium	windows_crashes.cpp throws std::runtime_error inside osquery namespace instead of returning Status	osquery/tables/system/windows/services.cpp:102
12	🔴 30 low — review closely	PipeChannelFactory::createChannelTicket throws std::runtime_error instead of returning Status	osquery/worker/ipc/posix/pipe_channel_factory.cpp:26
13	🟡 80 medium	SQL built via string concatenation with unescaped key/domain in get()/scan()	plugins/database/sqlite.cpp:115
14	🟡 85 medium	crontab.cpp discards error Status from genBin without checking it	osquery/tables/system/posix/suid_bin.cpp:106
15	🟢 95 high	EtwPublisherBase::run() returns Status::failure(0, ...) — zero is treated as success by convention	osquery/events/windows/etw/etw_publisher.cpp:27
16	🟡 65 medium	parseOfficeData throws no Status and can silently produce incorrect application/version fields from fragile substring math	osquery/tables/applications/windows/office_mru.cpp:52

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 46e37628-c6c9-49a4-aa29-9b30b2c8be3f

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akhf8u2 Osquery review findings sweep (15 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

16 finding(s) fixed in this draft — 16 explained inline on the diff; 5 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 initOpenFrame swallows all initialization failures behind a broad try/catch instead of returning Status

initOpenFrame() in osquery/core/init.cpp changed from void with a bare try/catch(const std::exception&) to Status initOpenFrame(), removing the try/catch entirely and returning Status::failure()/Status::success() at each error/success path. The call site in the Initializer constructor now captures the returned Status and logs via LOG(ERROR) if !ok(). This makes failures observable to the caller as a Status rather than being silently absorbed. Risk: this assumes OpenframeEncryptionService/OpenframeTokenExtractor constructors no longer throw exceptions that need catching; since I cannot see those headers, if they still throw, an exception could now propagate uncaught out of the Initializer constructor. A complete fix would also update those classes to be noexcept/Status-returning, which is out of scope for this file.

🤖 Prompt for AI agents

In osquery/core/init.cpp around line 220, review and complete this code-review fix: initOpenFrame swallows all initialization failures behind a broad try/catch instead of returning Status.
What the draft fix changed: initOpenFrame() in osquery/core/init.cpp changed from `void` with a bare try/catch(const std::exception&) to `Status initOpenFrame()`, removing the try/catch entirely and returning Status::failure()/Status::success() at each error/success path. The call site in the Initializer constructor now captures the returned Status and logs via LOG(ERROR) if !ok(). This makes failures observable to the caller as a Status rather than being silently absorbed. Risk: this assumes OpenframeEncryptionService/OpenframeTokenExtractor constructors no longer throw exceptions that need catching; since I cannot see those headers, if they still throw, an exception could now propagate uncaught out of the Initializer constructor. A complete fix would also update those classes to be noexcept/Status-returning, which is out of scope for this file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 openframe_authorization_manager.cpp is not the file included in init.cpp — OpenframeAuthorizationManagerProvider is used but no such symbol exists

Replaced #include "openframe/openframe_authorization_manager_provider.h" with #include "openframe/openframe_authorization_manager.h" and changed OpenframeAuthorizationManagerProvider::getInstance() to OpenframeAuthorizationManager::getInstance() in initOpenFrame(), matching the class name given in the finding evidence (openframe_authorization_manager.cpp/.h defines OpenframeAuthorizationManager). This is a guess that a getInstance() singleton accessor and updateToken() member exist on that class since I cannot see openframe_authorization_manager.h; if the real API differs (e.g. no singleton, different method name), this will not compile and the header/class itself would need to be reconciled, which is outside this file.

🤖 Prompt for AI agents

In osquery/core/init.cpp around line 225, review and complete this code-review fix: openframe_authorization_manager.cpp is not the file included in init.cpp — OpenframeAuthorizationManagerProvider is used but no such symbol exists.
What the draft fix changed: Replaced `#include "openframe/openframe_authorization_manager_provider.h"` with `#include "openframe/openframe_authorization_manager.h"` and changed `OpenframeAuthorizationManagerProvider::getInstance()` to `OpenframeAuthorizationManager::getInstance()` in initOpenFrame(), matching the class name given in the finding evidence (openframe_authorization_manager.cpp/.h defines `OpenframeAuthorizationManager`). This is a guess that a `getInstance()` singleton accessor and `updateToken()` member exist on that class since I cannot see openframe_authorization_manager.h; if the real API differs (e.g. no singleton, different method name), this will not compile and the header/class itself would need to be reconciled, which is outside this file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 std::cerr used for user-facing validator message instead of LOG()/systemLog only

Removed the duplicate std::cerr << "Alarm timeout cannot be lower than 10 seconds" << std::endl; line from validateAlarmTimeout() in osquery/core/init.cpp, leaving only the osquery::systemLog(...) call, exactly matching the suggested fix.

🤖 Prompt for AI agents

In osquery/core/init.cpp around line 258, review and complete this code-review fix: std::cerr used for user-facing validator message instead of LOG()/systemLog only.
What the draft fix changed: Removed the duplicate `std::cerr << "Alarm timeout cannot be lower than 10 seconds" << std::endl;` line from validateAlarmTimeout() in osquery/core/init.cpp, leaving only the `osquery::systemLog(...)` call, exactly matching the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 SystemStateTracker::create() and constructor throw Status instead of returning it

In SystemStateTracker::create() (the static, no-arg overload), replaced throw status; on IProcessContextFactory::create() failure with a LOG(ERROR) and return nullptr;, matching the existing failure-handling convention already used a few lines below in create(IProcessContextFactory::Ref) (which returns nullptr on Status failures caught from the constructor). The private constructor's throw status; (called from restart() failure) was intentionally left unchanged: the constructor cannot return a Status since constructors have no return value, and the surrounding create(IProcessContextFactory::Ref) factory already wraps object construction in try/catch and converts the thrown Status into a nullptr return, so callers of the public create() API never see an uncaught exception. A complete fix per the letter of the finding (constructor never throwing) would require restructuring SystemStateTracker to use a two-phase init (private constructor + separate init()), which is an architectural change beyond a minimal fix; here the risk is fully contained because the only path to the constructor is through the try/catch-wrapped factory method.

🤖 Prompt for AI agents

In osquery/events/linux/bpf/systemstatetracker.cpp around line 41, review and complete this code-review fix: SystemStateTracker::create() and constructor throw Status instead of returning it.
What the draft fix changed: In `SystemStateTracker::create()` (the static, no-arg overload), replaced `throw status;` on `IProcessContextFactory::create()` failure with a `LOG(ERROR)` and `return nullptr;`, matching the existing failure-handling convention already used a few lines below in `create(IProcessContextFactory::Ref)` (which returns `nullptr` on Status failures caught from the constructor). The private constructor's `throw status;` (called from `restart()` failure) was intentionally left unchanged: the constructor cannot return a `Status` since constructors have no return value, and the surrounding `create(IProcessContextFactory::Ref)` factory already wraps object construction in try/catch and converts the thrown `Status` into a `nullptr` return, so callers of the public `create()` API never see an uncaught exception. A complete fix per the letter of the finding (constructor never throwing) would require restructuring `SystemStateTracker` to use a two-phase init (private constructor + separate `init()`), which is an architectural change beyond a minimal fix; here the risk is fully contained because the only path to the constructor is through the try/catch-wrapped factory method.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 expireProcessContexts frees process map entries on any fs.fileExists() failure, not just confirmed non-existence

In SystemStateTracker::expireProcessContexts, changed the loop body so that when fs.fileExists(...) fails, the code now sets return_error = true, advances the iterator (++process_map_it) and continues, skipping the erase entirely; the entry is only erased when the existence check succeeded and returned exists == false. This directly matches the suggested fix in the finding.

🤖 Prompt for AI agents

In osquery/events/linux/bpf/systemstatetracker.cpp around line 314, review and complete this code-review fix: expireProcessContexts frees process map entries on any fs.fileExists() failure, not just confirmed non-existence.
What the draft fix changed: In `SystemStateTracker::expireProcessContexts`, changed the loop body so that when `fs.fileExists(...)` fails, the code now sets `return_error = true`, advances the iterator (`++process_map_it`) and `continue`s, skipping the erase entirely; the entry is only erased when the existence check succeeded and returned `exists == false`. This directly matches the suggested fix in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 PipeChannelFactory::createChannelTicket throws std::runtime_error instead of returning Status

Changed PipeChannelFactory::createChannelTicket() in osquery/worker/ipc/posix/pipe_channel_factory.cpp from throwing std::runtime_error to returning osquery::Status (via Status::failure/Status::success), taking the PipeChannelTicket& as an out-parameter instead of returning it by value. This mechanically satisfies OSQUERY-002-2's Status-based error contract. HOWEVER: this change is incomplete and unverified because the corresponding header pipe_channel_factory.h (not in scope/not shown) still presumably declares PipeChannelTicket createChannelTicket(); and callers (e.g. LinuxTableContainerIPC::connectToContainer) still call it expecting the old value-returning signature — those must be updated in lockstep or the build will fail. A complete fix requires editing the header's declaration and all call sites in the same change, which was out of scope for a single-file edit; this entry should be treated as a starting point requiring header/caller changes to actually compile.

🤖 Prompt for AI agents

In osquery/worker/ipc/posix/pipe_channel_factory.cpp around line 26, review and complete this code-review fix: PipeChannelFactory::createChannelTicket throws std::runtime_error instead of returning Status.
What the draft fix changed: Changed `PipeChannelFactory::createChannelTicket()` in `osquery/worker/ipc/posix/pipe_channel_factory.cpp` from throwing `std::runtime_error` to returning `osquery::Status` (via `Status::failure`/`Status::success`), taking the `PipeChannelTicket&` as an out-parameter instead of returning it by value. This mechanically satisfies OSQUERY-002-2's Status-based error contract. HOWEVER: this change is incomplete and unverified because the corresponding header `pipe_channel_factory.h` (not in scope/not shown) still presumably declares `PipeChannelTicket createChannelTicket();` and callers (e.g. `LinuxTableContainerIPC::connectToContainer`) still call it expecting the old value-returning signature — those must be updated in lockstep or the build will fail. A complete fix requires editing the header's declaration and all call sites in the same change, which was out of scope for a single-file edit; this entry should be treated as a starting point requiring header/caller changes to actually compile.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 SQL built via string concatenation with unescaped key/domain in get()/scan()

Rewrote SQLiteDatabasePlugin::get(std::string,std::string,std::string&) and SQLiteDatabasePlugin::scan() in plugins/database/sqlite.cpp to use sqlite3_prepare_v2/sqlite3_bind_text with parameter placeholders (?1) instead of concatenating key/prefix directly into the SQL string, mirroring the bound-parameter pattern already used by putBatch/remove/removeRange. get() now binds key and iterates rows via sqlite3_step/sqlite3_column_text instead of the sqlite3_exec callback; scan() binds prefix and appends it to the LIKE pattern via SQL concatenation (?1 || '%') rather than string-splicing the value into the query text, eliminating the quote-escaping/injection risk. The domain table name is still interpolated directly since SQLite does not support binding identifiers/table names, and domain values originate from the internal kDomains list, not external input, so this is not part of the finding's injection concern. Risk: minor behavior change in how NULL/absent columns are represented (previously "" via getData for missing column, now "" via null check on sqlite3_column_text) — behaviorally equivalent for this schema (TEXT columns). The max limit is still concatenated as a string but derived from a uint64_t via std::to_string, not attacker-controlled text, so no injection surface there.

🤖 Prompt for AI agents

In plugins/database/sqlite.cpp around line 115, review and complete this code-review fix: SQL built via string concatenation with unescaped key/domain in get()/scan().
What the draft fix changed: Rewrote SQLiteDatabasePlugin::get(std::string,std::string,std::string&) and SQLiteDatabasePlugin::scan() in plugins/database/sqlite.cpp to use sqlite3_prepare_v2/sqlite3_bind_text with parameter placeholders (?1) instead of concatenating `key`/`prefix` directly into the SQL string, mirroring the bound-parameter pattern already used by putBatch/remove/removeRange. get() now binds `key` and iterates rows via sqlite3_step/sqlite3_column_text instead of the sqlite3_exec callback; scan() binds `prefix` and appends it to the LIKE pattern via SQL concatenation (`?1 || '%'`) rather than string-splicing the value into the query text, eliminating the quote-escaping/injection risk. The `domain` table name is still interpolated directly since SQLite does not support binding identifiers/table names, and `domain` values originate from the internal kDomains list, not external input, so this is not part of the finding's injection concern. Risk: minor behavior change in how NULL/absent columns are represented (previously "" via getData for missing column, now "" via null check on sqlite3_column_text) — behaviorally equivalent for this schema (TEXT columns). The `max` limit is still concatenated as a string but derived from a uint64_t via std::to_string, not attacker-controlled text, so no injection surface there.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 crontab.cpp discards error Status from genBin without checking it

In genSuidBinsFromPath (osquery/tables/system/posix/suid_bin.cpp), the return value of genBin() is now captured into a Status and checked with !status.ok(); on failure it logs a warning via logger.log(google::GLOG_WARNING, status.getMessage()), matching the pattern used elsewhere in the codebase (GLOGLogger implements Logger::log). This makes stat() failures visible instead of silently discarded, per the finding's suggested fix.

🤖 Prompt for AI agents

In osquery/tables/system/posix/suid_bin.cpp around line 106, review and complete this code-review fix: crontab.cpp discards error Status from genBin without checking it.
What the draft fix changed: In genSuidBinsFromPath (osquery/tables/system/posix/suid_bin.cpp), the return value of genBin() is now captured into a Status and checked with !status.ok(); on failure it logs a warning via logger.log(google::GLOG_WARNING, status.getMessage()), matching the pattern used elsewhere in the codebase (GLOGLogger implements Logger::log). This makes stat() failures visible instead of silently discarded, per the finding's suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 EtwPublisherBase::run() returns Status::failure(0, ...) — zero is treated as success by convention

Changed Status::failure(0, ...) to Status::failure(1, ...) in EtwPublisherBase::run() (osquery/events/windows/etw/etw_publisher.cpp), matching the codebase-wide convention of using non-zero (1) failure codes as used elsewhere in this file.

🤖 Prompt for AI agents

In osquery/events/windows/etw/etw_publisher.cpp around line 27, review and complete this code-review fix: EtwPublisherBase::run() returns Status::failure(0, ...) — zero is treated as success by convention.
What the draft fix changed: Changed `Status::failure(0, ...)` to `Status::failure(1, ...)` in `EtwPublisherBase::run()` (osquery/events/windows/etw/etw_publisher.cpp), matching the codebase-wide convention of using non-zero (1) failure codes as used elsewhere in this file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 parseOfficeData throws no Status and can silently produce incorrect application/version fields from fragile substring math

In parseOfficeData (osquery/tables/applications/windows/office_mru.cpp), replaced the unguarded chained find()/substr() arithmetic used to derive "version" and "application" with explicit bounds/npos checks after each find() call. If office_version does not contain "Office\", or the derived version substring is not found again, or "\"/" MRU" are missing, or the "MRU" position is less than 10 (which previously caused substr with a huge npos-derived length or underflowed size_t), the function now logs a warning and returns early instead of risking std::out_of_range or silently emitting garbage into results. This keeps the same happy-path parsing logic (no algorithmic rewrite) while making malformed registry paths fail safely. Since genOfficeMru only ever consumes the (mutated) results vector and parseOfficeData already had a void/early-return contract for malformed rows (the file_path.length() < 20 case), this fix follows the same existing error-handling idiom in the file rather than introducing a new Status-based API, which would require changing the function signature and all call sites (officeData/office365Data) — a larger change than the finding strictly requires. Not fully verified against real-world malformed registry data/tests, since none exist in the repo for this table.

🤖 Prompt for AI agents

In osquery/tables/applications/windows/office_mru.cpp around line 52, review and complete this code-review fix: parseOfficeData throws no Status and can silently produce incorrect application/version fields from fragile substring math.
What the draft fix changed: In parseOfficeData (osquery/tables/applications/windows/office_mru.cpp), replaced the unguarded chained find()/substr() arithmetic used to derive "version" and "application" with explicit bounds/npos checks after each find() call. If office_version does not contain "Office\\", or the derived version substring is not found again, or "\\"/" MRU" are missing, or the "MRU" position is less than 10 (which previously caused substr with a huge npos-derived length or underflowed size_t), the function now logs a warning and returns early instead of risking std::out_of_range or silently emitting garbage into results. This keeps the same happy-path parsing logic (no algorithmic rewrite) while making malformed registry paths fail safely. Since genOfficeMru only ever consumes the (mutated) results vector and parseOfficeData already had a void/early-return contract for malformed rows (the file_path.length() < 20 case), this fix follows the same existing error-handling idiom in the file rather than introducing a new Status-based API, which would require changing the function signature and all call sites (officeData/office365Data) — a larger change than the finding strictly requires. Not fully verified against real-world malformed registry data/tests, since none exist in the repo for this table.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer