Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
100667d
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
3e20f5e
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
da0de83
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
ceccecd
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
81b4990
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
7df5b3b
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
4bfa7ee
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
a2f9cff
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
43a8f51
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
ed3005b
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
12ccd02
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
c4e3194
fix(OSQUERY-002-2): 16 review findings across 12 files
flamingo[bot] Sep 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 27 additions & 25 deletions osquery/core/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
#include <osquery/utils/info/platform_type.h>
#include <osquery/utils/info/version.h>
#include <osquery/utils/pidfile/pidfile.h>
#include <osquery/utils/status/status.h>
#include <osquery/utils/system/system.h>
#include <osquery/utils/system/time.h>

Expand Down Expand Up @@ -92,7 +93,7 @@ enum {
#endif

// OpenFrame includes
#include "openframe/openframe_authorization_manager_provider.h"
#include "openframe/openframe_authorization_manager.h"
#include "openframe/openframe_encryption_service.h"
#include "openframe/openframe_token_extractor.h"
#include "openframe/openframe_token_refresher.h"
Expand Down Expand Up @@ -208,35 +209,32 @@ void initWorkDirectories() {
}
}

void initOpenFrame() {
Status initOpenFrame() {
VLOG(1) << "OpenFrame mode enabled";

// Initialize OpenFrame components if secret is provided
if (FLAGS_openframe_secret.empty()) {
LOG(ERROR) << "OpenFrame mode enabled but secret not set";
return;
return Status::failure("OpenFrame mode enabled but secret not set");
}

try {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

// Create openframe token services
auto encryption_service = std::make_shared<OpenframeEncryptionService>(FLAGS_openframe_secret);
auto token_extractor = std::make_shared<OpenframeTokenExtractor>(encryption_service, FLAGS_openframe_token_path);

auto initial_token = token_extractor->extractToken();

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

if (!initial_token.empty()) {
auto& auth_manager = OpenframeAuthorizationManagerProvider::getInstance();
auth_manager.updateToken(initial_token);
LOG(INFO) << "OpenFrame token extracted successfully";
} else {
LOG(ERROR) << "Failed to get initial token from token file";
}

// Create and start token refresher
static auto token_refresher = std::make_shared<OpenframeTokenRefresher>(token_extractor);
token_refresher->start();
} catch (const std::exception& e) {
LOG(ERROR) << "Failed to initialize OpenFrame components: " << e.what();
// Create openframe token services
auto encryption_service = std::make_shared<OpenframeEncryptionService>(FLAGS_openframe_secret);
auto token_extractor = std::make_shared<OpenframeTokenExtractor>(encryption_service, FLAGS_openframe_token_path);

auto initial_token = token_extractor->extractToken();
if (!initial_token.empty()) {
auto& auth_manager = OpenframeAuthorizationManager::getInstance();
auth_manager.updateToken(initial_token);
LOG(INFO) << "OpenFrame token extracted successfully";
} else {
return Status::failure("Failed to get initial token from token file");
}

// Create and start token refresher
static auto token_refresher = std::make_shared<OpenframeTokenRefresher>(token_extractor);
token_refresher->start();

return Status::success();
}

void signalHandler(int num) {
Expand All @@ -258,7 +256,6 @@ void signalHandler(int num) {
bool validateAlarmTimeout(const char* flagname, std::uint64_t value) {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

if (value < 10) {
osquery::systemLog("Alarm timeout cannot be lower than 10 seconds");
std::cerr << "Alarm timeout cannot be lower than 10 seconds" << std::endl;
return false;
}

Expand Down Expand Up @@ -459,7 +456,11 @@ Initializer::Initializer(int& argc,

// Initialize OpenFrame authorization manager and token refresher if mode is enabled
if (FLAGS_openframe_mode) {
initOpenFrame();
auto openframe_status = initOpenFrame();
if (!openframe_status.ok()) {
LOG(ERROR) << "Failed to initialize OpenFrame components: "
<< openframe_status.getMessage();
}
} else {
VLOG(1) << "OpenFrame mode disabled";
}
Expand Down Expand Up @@ -954,3 +955,4 @@ void Initializer::shutdownNow(int retcode) {
_Exit(retcode);
}
} // namespace osquery

10 changes: 10 additions & 0 deletions osquery/dispatcher/distributed_runner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ void DistributedRunner::start() {
"Reading distributed queries", read_status, last_read_error);
logOutcomeChange(
"Writing distributed query results", write_status, last_write_error);
} else {
if (!read_status.ok()) {
LOG(ERROR) << "Error reading distributed queries: "
<< read_status.getMessage();
}
if (!write_status.ok()) {
LOG(ERROR) << "Error writing distributed query results: "
<< write_status.getMessage();
}
}

dist.cleanupExpiredRunningQueries();
Comment on lines 62 to 76

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 DistributedRunner::start() silently discards Status failures instead of surfacing them consistently

In DistributedRunner::start(), added an else branch alongside the existing if (FLAGS_openframe_mode) block that emits LOG(ERROR) for read_status and write_status failures whenever they are not ok, ensuring diagnostic logging happens regardless of FLAGS_openframe_mode. This restores unconditional error visibility for pullUpdates()/runQueries() failures while preserving the openframe_mode-specific change-tracking (logOutcomeChange) behavior unchanged.

🤖 Prompt for AI agents 
In osquery/dispatcher/distributed_runner.cpp around line 52, review and complete this code-review fix: DistributedRunner::start() silently discards Status failures instead of surfacing them consistently.
What the draft fix changed: In `DistributedRunner::start()`, added an `else` branch alongside the existing `if (FLAGS_openframe_mode)` block that emits `LOG(ERROR)` for `read_status` and `write_status` failures whenever they are not ok, ensuring diagnostic logging happens regardless of `FLAGS_openframe_mode`. This restores unconditional error visibility for pullUpdates()/runQueries() failures while preserving the openframe_mode-specific change-tracking (`logOutcomeChange`) behavior unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -89,3 +98,4 @@ Status startDistributed() {
}
}
} // namespace osquery

7 changes: 6 additions & 1 deletion osquery/events/linux/bpf/systemstatetracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ SystemStateTracker::Ref SystemStateTracker::create() {
IProcessContextFactory::Ref process_context_factory;
auto status = IProcessContextFactory::create(process_context_factory);
if (!status) {
throw status;
LOG(ERROR) << "Failed to create the state tracker: "
<< status.getMessage();
return nullptr;
}

return create(std::move(process_context_factory));
Comment on lines 42 to 50

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down Expand Up @@ -319,6 +321,8 @@ Status SystemStateTracker::expireProcessContexts(Context& context,
bool exists{false};
if (!fs.fileExists(exists, procfs_root.get(), process_id.c_str())) {
return_error = true;
++process_map_it;
continue;
}

if (!exists) {
Comment on lines 321 to 328

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -1357,3 +1361,4 @@ SystemStateTracker::Context SystemStateTracker::getContextCopy() const {
}

} // namespace osquery

3 changes: 2 additions & 1 deletion osquery/events/windows/etw/etw_publisher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ EtwController& EtwPublisherBase::EtwEngine() {
}

Status EtwPublisherBase::run() {

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

return Status::failure(0,
return Status::failure(1,
"ETW provider is driven by event callbacks. "
"A pooling thread is not required.");
}
Expand Down Expand Up @@ -120,3 +120,4 @@ void EtwPublisherBase::updateHardVolumeWithLogicalDrive(std::string& path) {
}

} // namespace osquery

Loading
Loading